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 10:08:06 PDT 2016
> On Aug 25, 2016, at 8:17 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
>
> On Thu, Aug 25, 2016 at 11:04 AM, Frédéric Riss <friss at apple.com> wrote:
>>
>>> 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?
>
> 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 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