Extend checking of va_start builtin

[aaron.ballman]

Summary

The va_start macro currently diagnoses passing a reference as the second argument (the one representing the parameter before the ellipsis) because this is undefined behavior in C++. This patch extends the checking to cover additional cases of undefined behavior in both C and C++. The C11 Standard, 7.16.1.4p4 states, in part:

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.

(This is picked up by reference in C++ under [support.runtime]p3.)

This patch adds a check for default argument promotions as well as parameters declared with the register storage class. 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).

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

7417–7421

I'm not sure that

'va_start' has undefined behavior with an object declared with the register storage class

is clear enough about what object it's talking about. Can you explicitly state that the problem is with the type of the parameter name passed to va_start here, somehow?

lib/Sema/SemaChecking.cpp

2722

Where is this restriction specified? Does this only apply to C?

[aaron.ballman]

include/clang/Basic/DiagnosticSemaKinds.td

7417–7421

How about: "'va_start' has undefined behavior with a parameter declared with the 'register' keyword"?

Then again, we could reword the entire diagnostic and I wouldn't be sad. I notice that it's using the plural "types" already instead of the singular form, for instance. How about:

"Passing %select{an object of reference type|an object that undergoes default argument promotion|a parameter declared with the 'register' keyword}0 to 'va_start' is undefined behavior"

lib/Sema/SemaChecking.cpp

2722

7.16.1.4p3:

If the parameter parmN is declared with the register storage class ... the behavior is undefined.

I think this may only apply in C, actually, because [support.runtime]p3 does not mention it in its description (even as far back as C++03). Whether that's an explicit omission (to make va_start() more powerful than in C) or an oversight is kind of moot since register was removed in C++17. Thoughts?

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

7417–7421

I think either of these is fine. (Though in the latter one, I'd say "has undefined behavior" or "results in undefined behavior" rather than "is undefined behavior".)

lib/Sema/SemaChecking.cpp

2722

I think it's an intentional difference with C. The intent of disallowing register is to allow va_start to be implemented as a macro that takes the address of the parameter. This would be ill-formed in C for a register parameter, whereas in C++ it is (was) valid to take the address of a variable declared with the register specifier.

[aaron.ballman]

Addressed review comments:

• Allow the register keyword for C++ code
• Reword the diagnostic to be a bit more clear
• Moved the varargs.cpp test case from Sema to SemaCXX

[aaron.ballman]

lib/Sema/SemaChecking.cpp

2722

That makes sense, thank you for the explanation.

[aaron.ballman]

Ping.

[aaron.ballman]

Thanks! I've commit in r267338.

include/clang/Basic/DiagnosticSemaKinds.td

 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<

lib/Sema/SemaChecking.cpp

   // 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())) {

 
       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;
   }
 

test/Sema/varargs-x86-64.c

   __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() {

test/Sema/varargs.c

     __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}}
 }
 
 

   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);
+}

test/Sema/varargs.cpp

-// 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}}
-}

test/SemaCXX/varargs.cpp

+// 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
+}