> 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? 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
