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 08:17:53 PDT 2016
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.
~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