Fix a latent bug where, after emitting an expression statement, we would

delete the block we began emitting into if it had no predecessors.  We never
want to do this, because there are several valid cases during statement
emission where an existing block has no known predecessors but will acquire
some later.  The case in my test case doesn't inherently fall into this
category, because we could safely emit the case-range code before the statement
body, but there are examples with labels that can't be fallen into
that would also demonstrate this bug.

rdar://problem/8837067

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@123303 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/CodeGen/CGBuiltin.cpp b/lib/CodeGen/CGBuiltin.cpp
index 46873bf0621da32d5d85ac7f209a65e155e39d05..0b7f3a06a7016051e8c735014ea7ea3e4e942d61 100644 (file)
--- a/lib/CodeGen/CGBuiltin.cpp
+++ b/lib/CodeGen/CGBuiltin.cpp
@@ -357,11 +357,15 @@ RValue CodeGenFunction::EmitBuiltinExpr(const FunctionDecl *FD,
     return RValue::get(Builder.CreateCall(F));
   }
   case Builtin::BI__builtin_unreachable: {
-    if (CatchUndefined && HaveInsertPoint())
+    if (CatchUndefined)
       EmitBranch(getTrapBB());
-    Value *V = Builder.CreateUnreachable();
-    Builder.ClearInsertionPoint();
-    return RValue::get(V);
+    else
+      Builder.CreateUnreachable();
+
+    // We do need to preserve an insertion point.
+    CGF.EmitBlock(createBasicBlock("unreachable.cont"));
+
+    return RValue::get(0);
   }
       
   case Builtin::BI__builtin_powi:
@@ -629,9 +633,12 @@ RValue CodeGenFunction::EmitBuiltinExpr(const FunctionDecl *FD,
                                   : Intrinsic::eh_return_i64,
                                 0, 0);
     Builder.CreateCall2(F, Int, Ptr);
-    Value *V = Builder.CreateUnreachable();
-    Builder.ClearInsertionPoint();
-    return RValue::get(V);
+    Builder.CreateUnreachable();
+
+    // We do need to preserve an insertion point.
+    CGF.EmitBlock(CGF.createBasicBlock("builtin_eh_return.cont"));
+
+    return RValue::get(0);
   }
   case Builtin::BI__builtin_unwind_init: {
     Value *F = CGM.getIntrinsic(Intrinsic::eh_unwind_init, 0, 0);
@@ -694,10 +701,13 @@ RValue CodeGenFunction::EmitBuiltinExpr(const FunctionDecl *FD,
     // Call LLVM's EH longjmp, which is lightweight.
     Builder.CreateCall(CGM.getIntrinsic(Intrinsic::eh_sjlj_longjmp), Buf);
 
-    // longjmp doesn't return; mark this as unreachable
-    Value *V = Builder.CreateUnreachable();
-    Builder.ClearInsertionPoint();
-    return RValue::get(V);
+    // longjmp doesn't return; mark this as unreachable.
+    Builder.CreateUnreachable();
+
+    // We do need to preserve an insertion point.
+    CGF.EmitBlock(CGF.createBasicBlock("longjmp.cont"));
+
+    return RValue::get(0);
   }
   case Builtin::BI__sync_fetch_and_add:
   case Builtin::BI__sync_fetch_and_sub:

diff --git a/lib/CodeGen/CGException.cpp b/lib/CodeGen/CGException.cpp
index 11e5e6452809bd9ea627e4bcc45f1bd9493d2998..252e547b5d1171e71d07573ab799af768b7db592 100644 (file)
--- a/lib/CodeGen/CGException.cpp
+++ b/lib/CodeGen/CGException.cpp
@@ -577,8 +577,10 @@ void CodeGenFunction::EmitCXXThrowExpr(const CXXThrowExpr *E) {
       Builder.CreateUnreachable();
     }
 
-    // Clear the insertion point to indicate we are in unreachable code.
-    Builder.ClearInsertionPoint();
+    // throw is an expression, and the expression emitters expect us
+    // to leave ourselves at a valid insertion point.
+    EmitBlock(createBasicBlock("throw.cont"));
+
     return;
   }
 
@@ -627,12 +629,9 @@ void CodeGenFunction::EmitCXXThrowExpr(const CXXThrowExpr *E) {
     Builder.CreateUnreachable();
   }
 
-  // Clear the insertion point to indicate we are in unreachable code.
-  Builder.ClearInsertionPoint();
-
-  // FIXME: For now, emit a dummy basic block because expr emitters in generally
-  // are not ready to handle emitting expressions at unreachable points.
-  EnsureInsertPoint();
+  // throw is an expression, and the expression emitters expect us
+  // to leave ourselves at a valid insertion point.
+  EmitBlock(createBasicBlock("throw.cont"));
 }
 
 void CodeGenFunction::EmitStartEHSpec(const Decl *D) {

diff --git a/lib/CodeGen/CGStmt.cpp b/lib/CodeGen/CGStmt.cpp
index dbef09f63a8e36ca1a7309e2564a33b7c167fbbb..b0278c7d02df7c35b279c008ddeb6f7c8b70a3f5 100644 (file)
--- a/lib/CodeGen/CGStmt.cpp
+++ b/lib/CodeGen/CGStmt.cpp
@@ -89,18 +89,34 @@ void CodeGenFunction::EmitStmt(const Stmt *S) {
 #define EXPR(Type, Base) \
   case Stmt::Type##Class:
 #include "clang/AST/StmtNodes.inc"
+  {
+    // Remember the block we came in on.
+    llvm::BasicBlock *incoming = Builder.GetInsertBlock();
+    assert(incoming && "expression emission must have an insertion point");
+
     EmitIgnoredExpr(cast<Expr>(S));
 
-    // Expression emitters don't handle unreachable blocks yet, so look for one
-    // explicitly here. This handles the common case of a call to a noreturn
-    // function.
-    if (llvm::BasicBlock *CurBB = Builder.GetInsertBlock()) {
-      if (CurBB->empty() && CurBB->use_empty()) {
-        CurBB->eraseFromParent();
-        Builder.ClearInsertionPoint();
-      }
+    llvm::BasicBlock *outgoing = Builder.GetInsertBlock();
+    assert(outgoing && "expression emission cleared block!");
+
+    // The expression emitters assume (reasonably!) that the insertion
+    // point is always set.  To maintain that, the call-emission code
+    // for noreturn functions has to enter a new block with no
+    // predecessors.  We want to kill that block and mark the current
+    // insertion point unreachable in the common case of a call like
+    // "exit();".  Since expression emission doesn't otherwise create
+    // blocks with no predecessors, we can just test for that.
+    // However, we must be careful not to do this to our incoming
+    // block, because *statement* emission does sometimes create
+    // reachable blocks which will have no predecessors until later in
+    // the function.  This occurs with, e.g., labels that are not
+    // reachable by fallthrough.
+    if (incoming != outgoing && outgoing->use_empty()) {
+      outgoing->eraseFromParent();
+      Builder.ClearInsertionPoint();
     }
     break;
+  }
 
   case Stmt::IndirectGotoStmtClass:
     EmitIndirectGotoStmt(cast<IndirectGotoStmt>(*S)); break;

diff --git a/test/CodeGen/switch.c b/test/CodeGen/switch.c
index dc2d27bc16da348a4f598485bf94ea2ef54d6a01..8b94a0976e6ee7d4cd15cb170733297cd1050a79 100644 (file)
--- a/test/CodeGen/switch.c
+++ b/test/CodeGen/switch.c
@@ -194,3 +194,20 @@ int f13(unsigned x) {
     return 0;
   }
 }
+
+// Don't delete a basic block that we want to introduce later references to.
+// This isn't really specific to switches, but it's easy to show with them.
+// rdar://problem/8837067
+int f14(int x) {
+  switch (x) {
+
+  // case range so that the case block has no predecessors
+  case 0 ... 15:
+    // any expression which doesn't introduce a new block
+    (void) 0;
+    // kaboom
+
+  default:
+    return x;
+  }
+}