Merging r281797:

------------------------------------------------------------------------
r281797 | richard-llvm | 2016-09-16 16:30:39 -0700 (Fri, 16 Sep 2016) | 10 lines

Fix a couple of wrong-code bugs in switch-on-constant optimization:

 * recurse through intermediate LabelStmts and AttributedStmts when checking
   whether a statement inside a switch declares a variable
 * if the end of a compound statement is reachable from the chosen case label,
   and the compound statement contains a variable declaration, it's not valid
   to just emit the contents of the compound statement -- we must emit the
   statement itself or we lose the scope (and thus end lifetimes at the wrong
   point)

------------------------------------------------------------------------

llvm-svn: 287790

4 files changed, 138 insertions, 4 deletions

diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index 77879021f9af..d815863e929d 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -1323,6 +1323,10 @@ static CSFC_Result CollectStatementsForCase(const Stmt *S,
     // Handle this as two cases: we might be looking for the SwitchCase (if so
     // the skipped statements must be skippable) or we might already have it.
     CompoundStmt::const_body_iterator I = CS->body_begin(), E = CS->body_end();
+    bool StartedInLiveCode = FoundCase;
+    unsigned StartSize = ResultStmts.size();
+
+    // If we've not found the case yet, scan through looking for it.
     if (Case) {
       // Keep track of whether we see a skipped declaration.  The code could be
       // using the declaration even if it is skipped, so we can't optimize out
@@ -1332,7 +1336,7 @@ static CSFC_Result CollectStatementsForCase(const Stmt *S,
       // If we're looking for the case, just see if we can skip each of the
       // substatements.
       for (; Case && I != E; ++I) {
-        HadSkippedDecl |= isa<DeclStmt>(*I);
+        HadSkippedDecl |= CodeGenFunction::mightAddDeclToScope(*I);
 
         switch (CollectStatementsForCase(*I, Case, FoundCase, ResultStmts)) {
         case CSFC_Failure: return CSFC_Failure;
@@ -1368,11 +1372,19 @@ static CSFC_Result CollectStatementsForCase(const Stmt *S,
           break;
         }
       }
+
+      if (!FoundCase)
+        return CSFC_Success;
+
+      assert(!HadSkippedDecl && "fallthrough after skipping decl");
     }
 
     // If we have statements in our range, then we know that the statements are
     // live and need to be added to the set of statements we're tracking.
+    bool AnyDecls = false;
     for (; I != E; ++I) {
+      AnyDecls |= CodeGenFunction::mightAddDeclToScope(*I);
+
       switch (CollectStatementsForCase(*I, nullptr, FoundCase, ResultStmts)) {
       case CSFC_Failure: return CSFC_Failure;
       case CSFC_FallThrough:
@@ -1390,7 +1402,24 @@ static CSFC_Result CollectStatementsForCase(const Stmt *S,
       }
     }
 
-    return Case ? CSFC_Success : CSFC_FallThrough;
+    // If we're about to fall out of a scope without hitting a 'break;', we
+    // can't perform the optimization if there were any decls in that scope
+    // (we'd lose their end-of-lifetime).
+    if (AnyDecls) {
+      // If the entire compound statement was live, there's one more thing we
+      // can try before giving up: emit the whole thing as a single statement.
+      // We can do that unless the statement contains a 'break;'.
+      // FIXME: Such a break must be at the end of a construct within this one.
+      // We could emit this by just ignoring the BreakStmts entirely.
+      if (StartedInLiveCode && !CodeGenFunction::containsBreak(S)) {
+        ResultStmts.resize(StartSize);
+        ResultStmts.push_back(S);
+      } else {
+        return CSFC_Failure;
+      }
+    }
+
+    return CSFC_FallThrough;
   }
 
   // Okay, this is some other statement that we don't handle explicitly, like a
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index eed4ee0eac88..11e4ad9ecefa 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -25,6 +25,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/StmtCXX.h"
+#include "clang/AST/StmtObjC.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
@@ -1159,6 +1160,28 @@ bool CodeGenFunction::containsBreak(const Stmt *S) {
   return false;
 }
 
+bool CodeGenFunction::mightAddDeclToScope(const Stmt *S) {
+  if (!S) return false;
+
+  // Some statement kinds add a scope and thus never add a decl to the current
+  // scope. Note, this list is longer than the list of statements that might
+  // have an unscoped decl nested within them, but this way is conservatively
+  // correct even if more statement kinds are added.
+  if (isa<IfStmt>(S) || isa<SwitchStmt>(S) || isa<WhileStmt>(S) ||
+      isa<DoStmt>(S) || isa<ForStmt>(S) || isa<CompoundStmt>(S) ||
+      isa<CXXForRangeStmt>(S) || isa<CXXTryStmt>(S) ||
+      isa<ObjCForCollectionStmt>(S) || isa<ObjCAtTryStmt>(S))
+    return false;
+
+  if (isa<DeclStmt>(S))
+    return true;
+
+  for (const Stmt *SubStmt : S->children())
+    if (mightAddDeclToScope(SubStmt))
+      return true;
+
+  return false;
+}
 
 /// ConstantFoldsToSimpleInteger - If the specified expression does not fold
 /// to a constant, or if it does but contains a label, return false.  If it
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index 4ad4440bc9cb..f61ba69e3a0c 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -3192,6 +3192,10 @@ public:
   /// If the statement (recursively) contains a switch or loop with a break
   /// inside of it, this is fine.
   static bool containsBreak(const Stmt *S);
+
+  /// Determine if the given statement might introduce a declaration into the
+  /// current scope, by being a (possibly-labelled) DeclStmt.
+  static bool mightAddDeclToScope(const Stmt *S);
   
   /// ConstantFoldsToSimpleInteger - If the specified expression does not fold
   /// to a constant, or if it does but contains a label, return false.  If it
diff --git a/clang/test/CodeGenCXX/switch-case-folding-2.cpp b/clang/test/CodeGenCXX/switch-case-folding-2.cpp
index 558ca3c87d9b..cfb8447ee326 100644
--- a/clang/test/CodeGenCXX/switch-case-folding-2.cpp
+++ b/clang/test/CodeGenCXX/switch-case-folding-2.cpp
@@ -1,12 +1,14 @@
 // RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm %s -o - | FileCheck %s
-// CHECK that we don't crash.
 
 extern int printf(const char*, ...);
+
+// CHECK-LABEL: @_Z4testi(
 int test(int val){
  switch (val) {
  case 4:
    do {
      switch (6) {
+       // CHECK: call i32 (i8*, ...) @_Z6printfPKcz
        case 6: do { case 5: printf("bad\n"); } while (0);
      };
    } while (0);
@@ -18,6 +20,7 @@ int main(void) {
  return test(5);
 }
 
+// CHECK-LABEL: @_Z10other_testv(
 void other_test() {
   switch(0) {
   case 0:
@@ -27,4 +30,79 @@ void other_test() {
   }
 }
 
-// CHECK: call i32 (i8*, ...) @_Z6printfPKcz
+struct X { X(); ~X(); };
+
+void dont_call();
+void foo();
+
+// CHECK-LABEL: @_Z13nested_scopesv(
+void nested_scopes() {
+  switch (1) {
+  case 0:
+    // CHECK-NOT: @_Z9dont_callv(
+    dont_call();
+    break;
+
+  default:
+    // CHECK: call {{.*}} @_ZN1XC1Ev(
+    // CHECK: call {{.*}} @_Z3foov(
+    // CHECK-NOT: call {{.*}} @_Z3foov(
+    // CHECK: call {{.*}} @_ZN1XD1Ev(
+    { X x; foo(); }
+
+    // CHECK: call {{.*}} @_ZN1XC1Ev(
+    // CHECK: call {{.*}} @_Z3foov(
+    // CHECK: call {{.*}} @_ZN1XD1Ev(
+    { X x; foo(); }
+
+    // CHECK: call {{.*}} @_ZN1XC1Ev(
+    // CHECK: call {{.*}} @_Z3foov(
+    // CHECK: call {{.*}} @_ZN1XD1Ev(
+    { X x; foo(); }
+    break;
+  }
+}
+
+// CHECK-LABEL: @_Z17scope_fallthroughv(
+void scope_fallthrough() {
+  switch (1) {
+    // CHECK: call {{.*}} @_ZN1XC1Ev(
+    // CHECK-NOT: call {{.*}} @_Z3foov(
+    // CHECK: call {{.*}} @_ZN1XD1Ev(
+    { default: X x; }
+    // CHECK: call {{.*}} @_Z3foov(
+    foo();
+    break;
+  }
+}
+
+// CHECK-LABEL: @_Z12hidden_breakb(
+void hidden_break(bool b) {
+  switch (1) {
+  default:
+    // CHECK: br
+    if (b)
+      break;
+    // CHECK: call {{.*}} @_Z3foov(
+    foo();
+    break;
+  }
+}
+
+// CHECK-LABEL: @_Z10hidden_varv(
+int hidden_var() {
+  switch (1) {
+  // CHECK: %[[N:.*]] = alloca i32
+  case 0: int n;
+  // CHECK: store i32 0, i32* %[[N]]
+  // CHECK: load i32, i32* %[[N]]
+  // CHECK: ret
+  default: n = 0; return n;
+  }
+}
+
+// CHECK-LABEL: @_Z13case_in_labelv(
+void case_in_label() {
+  // CHECK: br label %
+  switch (1) case 1: foo: case 0: goto foo;
+}