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).

llvm-svn: 267338

6 files changed, 44 insertions, 19 deletions

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 053dca3de89..f24bf870113 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7435,8 +7435,10 @@ def err_ms_va_start_used_in_sysv_function : Error<
 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<
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index fb11adb4518..f70c06377e2 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -2702,6 +2702,7 @@ bool Sema::SemaBuiltinVAStartImpl(CallExpr *TheCall) {
   // 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(CallExpr *TheCall) {
 
       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;
   }
 
diff --git a/clang/test/Sema/varargs-x86-64.c b/clang/test/Sema/varargs-x86-64.c
index e3ded6fbf6b..0929c0d9147 100644
--- a/clang/test/Sema/varargs-x86-64.c
+++ b/clang/test/Sema/varargs-x86-64.c
@@ -26,11 +26,11 @@ void __attribute__((ms_abi)) g2(int a, int b, ...) {
   __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() {
diff --git a/clang/test/Sema/varargs.c b/clang/test/Sema/varargs.c
index 82fa42b2c1c..457d84c212f 100644
--- a/clang/test/Sema/varargs.c
+++ b/clang/test/Sema/varargs.c
@@ -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);
+}
diff --git a/clang/test/Sema/varargs.cpp b/clang/test/Sema/varargs.cpp
deleted file mode 100644
index 48a7b2fdf10..00000000000
--- a/clang/test/Sema/varargs.cpp
+++ /dev/null
@@ -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}}
-}
diff --git a/clang/test/SemaCXX/varargs.cpp b/clang/test/SemaCXX/varargs.cpp
new file mode 100644
index 00000000000..6a1883786a3
--- /dev/null
+++ b/clang/test/SemaCXX/varargs.cpp
@@ -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
+}