Fix bugs checking va_start in lambdas and erroneous contexts

Summary:
First, getCurFunction looks through blocks and lambdas, which is wrong.
Inside a lambda, va_start should refer to the lambda call operator
prototype. This fixes PR32737.

Second, we shouldn't use any of the getCur* methods, because they look
through contexts that we don't want to look through (EnumDecl,
CapturedStmtDecl). We can use CurContext directly as the calling
context.

Finally, this code assumed that CallExprs would never appear outside of
code contexts (block, function, obj-c method), which is wrong. Struct
member initializers are an easy way to create and parse exprs in a
non-code context.

Reviewers: rsmith

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D32761

llvm-svn: 302188

4 files changed, 78 insertions, 12 deletions

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index cc530a6dd49..ff9626d79da 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7920,7 +7920,11 @@ def warn_empty_switch_body : Warning<
 def note_empty_body_on_separate_line : Note<
   "put the semicolon on a separate line to silence this warning">;
 
-def err_va_start_used_in_non_variadic_function : Error<
+def err_va_start_captured_stmt : Error<
+  "'va_start' cannot be used in a captured statement">;
+def err_va_start_outside_function : Error<
+  "'va_start' cannot be used outside a function">;
+def err_va_start_fixed_function : Error<
   "'va_start' used in function with fixed args">;
 def err_va_start_used_in_wrong_abi_function : Error<
   "'va_start' used in %select{System V|Win64}0 ABI function">;
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index a206100b89e..14dd6267b85 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -3652,22 +3652,29 @@ static bool checkVAStartIsInVariadicFunction(Sema &S, Expr *Fn,
   // and get its parameter list.
   bool IsVariadic = false;
   ArrayRef<ParmVarDecl *> Params;
-  if (BlockScopeInfo *CurBlock = S.getCurBlock()) {
-    IsVariadic = CurBlock->TheDecl->isVariadic();
-    Params = CurBlock->TheDecl->parameters();
-  } else if (FunctionDecl *FD = S.getCurFunctionDecl()) {
+  DeclContext *Caller = S.CurContext;
+  if (auto *Block = dyn_cast<BlockDecl>(Caller)) {
+    IsVariadic = Block->isVariadic();
+    Params = Block->parameters();
+  } else if (auto *FD = dyn_cast<FunctionDecl>(Caller)) {
     IsVariadic = FD->isVariadic();
     Params = FD->parameters();
-  } else if (ObjCMethodDecl *MD = S.getCurMethodDecl()) {
+  } else if (auto *MD = dyn_cast<ObjCMethodDecl>(Caller)) {
     IsVariadic = MD->isVariadic();
     // FIXME: This isn't correct for methods (results in bogus warning).
     Params = MD->parameters();
+  } else if (isa<CapturedDecl>(Caller)) {
+    // We don't support va_start in a CapturedDecl.
+    S.Diag(Fn->getLocStart(), diag::err_va_start_captured_stmt);
+    return true;
   } else {
-    llvm_unreachable("unknown va_start context");
+    // This must be some other declcontext that parses exprs.
+    S.Diag(Fn->getLocStart(), diag::err_va_start_outside_function);
+    return true;
   }
 
   if (!IsVariadic) {
-    S.Diag(Fn->getLocStart(), diag::err_va_start_used_in_non_variadic_function);
+    S.Diag(Fn->getLocStart(), diag::err_va_start_fixed_function);
     return true;
   }
 
diff --git a/clang/test/OpenMP/varargs.cpp b/clang/test/OpenMP/varargs.cpp
new file mode 100644
index 00000000000..7738f83fab2
--- /dev/null
+++ b/clang/test/OpenMP/varargs.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -verify -fopenmp %s
+void f(int a, ...) {
+#pragma omp parallel for
+  for (int i = 0; i < 100; ++i) {
+    __builtin_va_list ap;
+    __builtin_va_start(ap, a); // expected-error {{'va_start' cannot be used in a captured statement}}
+  }
+};
diff --git a/clang/test/SemaCXX/varargs.cpp b/clang/test/SemaCXX/varargs.cpp
index 6a1883786a3..f9027c24799 100644
--- a/clang/test/SemaCXX/varargs.cpp
+++ b/clang/test/SemaCXX/varargs.cpp
@@ -1,12 +1,59 @@
-// RUN: %clang_cc1 -fsyntax-only -std=c++03 -verify %s
+// RUN: %clang_cc1 -std=c++03 -verify %s
+// RUN: %clang_cc1 -std=c++11 -verify %s
+
+__builtin_va_list ap;
 
 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
+  __builtin_va_start(ap, i); // UB in C, OK in C++
+}
+
+// Don't crash when there is no last parameter.
+void no_params(...) {
+  int a;
+  __builtin_va_start(ap, a); // expected-warning {{second argument to 'va_start' is not the last named parameter}}
+}
+
+// Reject this. The __builtin_va_start would execute in Foo's non-variadic
+// default ctor.
+void record_context(int a, ...) {
+  struct Foo {
+    // expected-error@+1 {{'va_start' cannot be used outside a function}}
+    void meth(int a, int b = (__builtin_va_start(ap, a), 0)) {}
+  };
+}
+
+#if __cplusplus >= 201103L
+// We used to have bugs identifying the correct enclosing function scope in a
+// lambda.
+
+void fixed_lambda_varargs_function(int a, ...) {
+  [](int b) {
+    __builtin_va_start(ap, b); // expected-error {{'va_start' used in function with fixed args}}
+  }(42);
+}
+void varargs_lambda_fixed_function(int a) {
+  [](int b, ...) {
+    __builtin_va_start(ap, b); // correct
+  }(42);
+}
+
+auto fixed_lambda_global = [](int f) {
+  __builtin_va_start(ap, f); // expected-error {{'va_start' used in function with fixed args}}
+};
+auto varargs_lambda_global = [](int f, ...) {
+  __builtin_va_start(ap, f); // correct
+};
+
+void record_member_init(int a, ...) {
+  struct Foo {
+    int a = 0;
+    // expected-error@+1 {{'va_start' cannot be used outside a function}}
+    int b = (__builtin_va_start(ap, a), 0);
+  };
 }
+#endif