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.
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 25 07:44:52 PDT 2016
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.
~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