r267338 - Improve diagnostic checking for va_start to also warn on other instances of undefined behavior, such as a parameter declared with the register keyword in C, or a parameter of a type that undergoes default argument promotion.
Frédéric Riss via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 25 08:04:17 PDT 2016
> On Aug 25, 2016, at 7:44 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
>
> On Tue, Aug 23, 2016 at 9:32 PM, Frédéric Riss <friss at apple.com> wrote:
>> Hey Aaron,
>>
>> This commit triggers warnings when the argument to va_start is an enum type. The standard says:
>>
>> 7.16.1.4 The va_start macro
>>
>> • 4 The parameter parmN is the identifier of the rightmost parameter in the variable parameter list in the function definition (the one just before the , ...). If the parameter parmN is declared with the register storage class, with a function or array type, or with a type that is not compatible with the type that results after application of the default argument promotions, the behavior is undefined.
>>
>> It seems to me that using an enum as the argument to va_start is not UB when the implementation chose to use an int to store the enum.
>
> The implementation isn't required to choose int as the compatible type
> for the enumeration. See 6.7.2.2p4: "Each enumerated type shall be
> compatible with char, a signed integer type, or an unsigned integer
> type." In the case where the compatible type is char, this use is UB.
>
>> Would you agree the warning as implemented gives false positives in that case?
>
> It depends entirely on the enumeration and what compatible type is
> chosen for it. If the compatible type is signed or unsigned int, then
> the warning would be a false-positive and we should silence it. If the
> compatible type is char, then the warning is not a false positive.
I think we agree? If I’m not mistaken, in clang’s case, an int compatible type will always be chosen except if -fshort-enums is passed and the enum fits in a smaller type. Do you want a PR?
Fred
> ~Aaron
>
>>
>> Thanks,
>> Fred
>>
>>
>>> On Apr 24, 2016, at 6:30 AM, Aaron Ballman via cfe-commits <cfe-commits at lists.llvm.org> wrote:
>>>
>>> Author: aaronballman
>>> Date: Sun Apr 24 08:30:21 2016
>>> New Revision: 267338
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=267338&view=rev
>>> Log:
>>> Improve diagnostic checking for va_start to also warn on other instances of undefined behavior, such as a parameter declared with the register keyword in C, or a parameter of a type that undergoes default argument promotion.
>>>
>>> This helps cover some more of the CERT secure coding rule EXP58-CPP. Pass an object of the correct type to va_start (https://www.securecoding.cert.org/confluence/display/cplusplus/EXP58-CPP.+Pass+an+object+of+the+correct+type+to+va_start).
>>>
>>> Added:
>>> cfe/trunk/test/SemaCXX/varargs.cpp
>>> Removed:
>>> cfe/trunk/test/Sema/varargs.cpp
>>> Modified:
>>> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>>> cfe/trunk/lib/Sema/SemaChecking.cpp
>>> cfe/trunk/test/Sema/varargs-x86-64.c
>>> cfe/trunk/test/Sema/varargs.c
>>>
>>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=267338&r1=267337&r2=267338&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Sun Apr 24 08:30:21 2016
>>> @@ -7435,8 +7435,10 @@ def err_ms_va_start_used_in_sysv_functio
>>> def warn_second_arg_of_va_start_not_last_named_param : Warning<
>>> "second argument to 'va_start' is not the last named parameter">,
>>> InGroup<Varargs>;
>>> -def warn_va_start_of_reference_type_is_undefined : Warning<
>>> - "'va_start' has undefined behavior with reference types">, InGroup<Varargs>;
>>> +def warn_va_start_type_is_undefined : Warning<
>>> + "passing %select{an object that undergoes default argument promotion|"
>>> + "an object of reference type|a parameter declared with the 'register' "
>>> + "keyword}0 to 'va_start' has undefined behavior">, InGroup<Varargs>;
>>> def err_first_argument_to_va_arg_not_of_type_va_list : Error<
>>> "first argument to 'va_arg' is of type %0 and not 'va_list'">;
>>> def err_second_parameter_to_va_arg_incomplete: Error<
>>>
>>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=267338&r1=267337&r2=267338&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Sun Apr 24 08:30:21 2016
>>> @@ -2702,6 +2702,7 @@ bool Sema::SemaBuiltinVAStartImpl(CallEx
>>> // block.
>>> QualType Type;
>>> SourceLocation ParamLoc;
>>> + bool IsCRegister = false;
>>>
>>> if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(Arg)) {
>>> if (const ParmVarDecl *PV = dyn_cast<ParmVarDecl>(DR->getDecl())) {
>>> @@ -2718,15 +2719,21 @@ bool Sema::SemaBuiltinVAStartImpl(CallEx
>>>
>>> Type = PV->getType();
>>> ParamLoc = PV->getLocation();
>>> + IsCRegister =
>>> + PV->getStorageClass() == SC_Register && !getLangOpts().CPlusPlus;
>>> }
>>> }
>>>
>>> if (!SecondArgIsLastNamedArgument)
>>> Diag(TheCall->getArg(1)->getLocStart(),
>>> diag::warn_second_arg_of_va_start_not_last_named_param);
>>> - else if (Type->isReferenceType()) {
>>> - Diag(Arg->getLocStart(),
>>> - diag::warn_va_start_of_reference_type_is_undefined);
>>> + else if (IsCRegister || Type->isReferenceType() ||
>>> + Type->isPromotableIntegerType() ||
>>> + Type->isSpecificBuiltinType(BuiltinType::Float)) {
>>> + unsigned Reason = 0;
>>> + if (Type->isReferenceType()) Reason = 1;
>>> + else if (IsCRegister) Reason = 2;
>>> + Diag(Arg->getLocStart(), diag::warn_va_start_type_is_undefined) << Reason;
>>> Diag(ParamLoc, diag::note_parameter_type) << Type;
>>> }
>>>
>>>
>>> Modified: cfe/trunk/test/Sema/varargs-x86-64.c
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/varargs-x86-64.c?rev=267338&r1=267337&r2=267338&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/test/Sema/varargs-x86-64.c (original)
>>> +++ cfe/trunk/test/Sema/varargs-x86-64.c Sun Apr 24 08:30:21 2016
>>> @@ -26,11 +26,11 @@ void __attribute__((ms_abi)) g2(int a, i
>>> __builtin_ms_va_start(ap, b);
>>> }
>>>
>>> -void __attribute__((ms_abi)) g3(float a, ...) {
>>> +void __attribute__((ms_abi)) g3(float a, ...) { // expected-note 2{{parameter of type 'float' is declared here}}
>>> __builtin_ms_va_list ap;
>>>
>>> - __builtin_ms_va_start(ap, a);
>>> - __builtin_ms_va_start(ap, (a));
>>> + __builtin_ms_va_start(ap, a); // expected-warning {{passing an object that undergoes default argument promotion to 'va_start' has undefined behavior}}
>>> + __builtin_ms_va_start(ap, (a)); // expected-warning {{passing an object that undergoes default argument promotion to 'va_start' has undefined behavior}}
>>> }
>>>
>>> void __attribute__((ms_abi)) g5() {
>>>
>>> Modified: cfe/trunk/test/Sema/varargs.c
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/varargs.c?rev=267338&r1=267337&r2=267338&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/test/Sema/varargs.c (original)
>>> +++ cfe/trunk/test/Sema/varargs.c Sun Apr 24 08:30:21 2016
>>> @@ -18,12 +18,11 @@ void f2(int a, int b, ...)
>>> __builtin_va_start(ap, b);
>>> }
>>>
>>> -void f3(float a, ...)
>>> -{
>>> +void f3(float a, ...) { // expected-note 2{{parameter of type 'float' is declared here}}
>>> __builtin_va_list ap;
>>>
>>> - __builtin_va_start(ap, a);
>>> - __builtin_va_start(ap, (a));
>>> + __builtin_va_start(ap, a); // expected-warning {{passing an object that undergoes default argument promotion to 'va_start' has undefined behavior}}
>>> + __builtin_va_start(ap, (a)); // expected-warning {{passing an object that undergoes default argument promotion to 'va_start' has undefined behavior}}
>>> }
>>>
>>>
>>> @@ -83,3 +82,15 @@ void f10(int a, ...) {
>>> i = __builtin_va_start(ap, a); // expected-error {{assigning to 'int' from incompatible type 'void'}}
>>> __builtin_va_end(ap);
>>> }
>>> +
>>> +void f11(short s, ...) { // expected-note {{parameter of type 'short' is declared here}}
>>> + __builtin_va_list ap;
>>> + __builtin_va_start(ap, s); // expected-warning {{passing an object that undergoes default argument promotion to 'va_start' has undefined behavior}}
>>> + __builtin_va_end(ap);
>>> +}
>>> +
>>> +void f12(register int i, ...) { // expected-note {{parameter of type 'int' is declared here}}
>>> + __builtin_va_list ap;
>>> + __builtin_va_start(ap, i); // expected-warning {{passing a parameter declared with the 'register' keyword to 'va_start' has undefined behavior}}
>>> + __builtin_va_end(ap);
>>> +}
>>>
>>> Removed: cfe/trunk/test/Sema/varargs.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/varargs.cpp?rev=267337&view=auto
>>> ==============================================================================
>>> --- cfe/trunk/test/Sema/varargs.cpp (original)
>>> +++ cfe/trunk/test/Sema/varargs.cpp (removed)
>>> @@ -1,7 +0,0 @@
>>> -// RUN: %clang_cc1 -fsyntax-only -verify %s
>>> -
>>> -class string;
>>> -void f(const string& s, ...) { // expected-note {{parameter of type 'const string &' is declared here}}
>>> - __builtin_va_list ap;
>>> - __builtin_va_start(ap, s); // expected-warning {{'va_start' has undefined behavior with reference types}}
>>> -}
>>>
>>> Added: cfe/trunk/test/SemaCXX/varargs.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/varargs.cpp?rev=267338&view=auto
>>> ==============================================================================
>>> --- cfe/trunk/test/SemaCXX/varargs.cpp (added)
>>> +++ cfe/trunk/test/SemaCXX/varargs.cpp Sun Apr 24 08:30:21 2016
>>> @@ -0,0 +1,12 @@
>>> +// RUN: %clang_cc1 -fsyntax-only -std=c++03 -verify %s
>>> +
>>> +class string;
>>> +void f(const string& s, ...) { // expected-note {{parameter of type 'const string &' is declared here}}
>>> + __builtin_va_list ap;
>>> + __builtin_va_start(ap, s); // expected-warning {{passing an object of reference type to 'va_start' has undefined behavior}}
>>> +}
>>> +
>>> +void g(register int i, ...) {
>>> + __builtin_va_list ap;
>>> + __builtin_va_start(ap, i); // okay
>>> +}
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
More information about the cfe-commits
mailing list