On Thu, Aug 25, 2016 at 11:04 AM, Frédéric Riss <fr...@apple.com> wrote: > >> On Aug 25, 2016, at 7:44 AM, Aaron Ballman <aa...@aaronballman.com> wrote: >> >> On Tue, Aug 23, 2016 at 9:32 PM, Frédéric Riss <fr...@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?
Yes, I think a PR is reasonable. There's also the packed attribute on an enum which acts the same as passing -fshort-enums, and there is the language extension for allowing enumerators larger than what fits into an int. ~Aaron > > Fred > >> ~Aaron >> >>> >>> Thanks, >>> Fred >>> >>> >>>> On Apr 24, 2016, at 6:30 AM, Aaron Ballman via cfe-commits >>>> <cfe-commits@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@lists.llvm.org >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>> > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
