> On Aug 25, 2016, at 8:17 AM, Aaron Ballman <aa...@aaronballman.com> wrote: > > 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.
PR29140 Thanks! Fred > ~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
