From 550a211fa3b0f5c738e8616d408738c0681b9cc0 Mon Sep 17 00:00:00 2001
From: Richard Smith <richard-llvm@metafoo.co.uk>
Date: Fri, 16 Sep 2016 23:30:39 +0000
Subject: [PATCH] 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)
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@281797 91177308-0d34-0410-b5e6-96231b3b80d8
---
lib/CodeGen/CGStmt.cpp | 33 ++++++++-
lib/CodeGen/CodeGenFunction.cpp | 23 +++++++
lib/CodeGen/CodeGenFunction.h | 4 ++
test/CodeGenCXX/switch-case-folding-2.cpp | 82 ++++++++++++++++++++++-
4 files changed, 138 insertions(+), 4 deletions(-)
diff --git a/lib/CodeGen/CGStmt.cpp b/lib/CodeGen/CGStmt.cpp
index aa457ad5f3f..9447ae56c7a 100644
--- a/lib/CodeGen/CGStmt.cpp
+++ b/lib/CodeGen/CGStmt.cpp
@@ -1329,6 +1329,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
@@ -1338,7 +1342,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;
@@ -1374,11 +1378,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:
@@ -1396,7 +1408,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/lib/CodeGen/CodeGenFunction.cpp b/lib/CodeGen/CodeGenFunction.cpp
index 0b9fe29a9ce..1959f468318 100644
--- a/lib/CodeGen/CodeGenFunction.cpp
+++ b/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"
@@ -1168,6 +1169,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/lib/CodeGen/CodeGenFunction.h b/lib/CodeGen/CodeGenFunction.h
index db8b634539f..7820fda6ba5 100644
--- a/lib/CodeGen/CodeGenFunction.h
+++ b/lib/CodeGen/CodeGenFunction.h
@@ -3168,6 +3168,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/test/CodeGenCXX/switch-case-folding-2.cpp b/test/CodeGenCXX/switch-case-folding-2.cpp
index 558ca3c87d9..cfb8447ee32 100644
--- a/test/CodeGenCXX/switch-case-folding-2.cpp
+++ b/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;
+}
--
GitLab