Don't crash on leaving nested __finally blocks through an EH edge.

[thakis]

Summary

The finally emission block tries to be clever by removing unused continuation edges if there's an unconditional jump out of the finally block. With exception edges, the EH continuation edge isn't always unused though and we'd crash in a few places.

Just don't be clever. That makes the IR for __finally blocks a bit longer in some cases (hence small and behavior-preserving changes to existing tests), but it makes no difference in general and it fixes the last crash from PR22553.

[rnk]

I have one idea for how we can still be clever, but if it's too hard, then yeah let's be dumb.

lib/CodeGen/CGException.cpp

1936

Can we check !HaveInsertPoint() && FI.ContBB->hasZeroUses() instead and still be clever?

[thakis]

lib/CodeGen/CGException.cpp

1936

You mean use_empty()? I tried that (if (FI.ContBB.use_empty() && (!FI.ResumeBB || FI.ResumeBB.use_empty())) and that didn't immediately work. But then I though even if it did, he benefits of being clever seem not worth it over having more control flow, and I stopped trying immediately and sent this out as-is :-)

[rnk]

lgtm

lib/CodeGen/CGException.cpp

1936

OK. SEH finally is super rare. In general, though, the best way to speed up -O0 codegen is to emit less code.

[thakis]

r230697

lib/CodeGen/CGException.cpp

llvm::BasicBlock *CodeGenFunction::getEHResumeBlock(bool isCleanup) {
 
   const EHPersonality &Personality = EHPersonality::get(*this);
 
   // This can always be a call because we necessarily didn't find
   // anything on the EH stack which needs our help.
   const char *RethrowName = Personality.CatchallRethrowFn;
   if (RethrowName != nullptr && !isCleanup) {
     EmitRuntimeCall(getCatchallRethrowFn(CGM, RethrowName),
-                    getExceptionFromSlot())
-      ->setDoesNotReturn();
+                    getExceptionFromSlot())->setDoesNotReturn();
     Builder.CreateUnreachable();
     Builder.restoreIP(SavedIP);
     return EHResumeBlock;
   }
 
   // Recreate the landingpad's return value for the 'resume' instruction.
   llvm::Value *Exn = getExceptionFromSlot();
   llvm::Value *Sel = getSelectorFromSlot();
if (const SEHFinallyStmt *Finally = S.getFinallyHandler()) {
     PopCleanupBlock();
     assert(FI.ContBB && "did not emit normal cleanup");
 
     // Emit the code into FinallyBB.
     CGBuilderTy::InsertPoint SavedIP = Builder.saveIP();
     Builder.SetInsertPoint(FI.FinallyBB);
     EmitStmt(Finally->getBlock());
 
-    // If the finally block doesn't fall through, we don't need these blocks.
-    if (!HaveInsertPoint()) {
-      FI.ContBB->eraseFromParent();
-      if (FI.ResumeBB)
-        FI.ResumeBB->eraseFromParent();
-      return;
-    }
-
+    if (HaveInsertPoint()) {
       if (FI.ResumeBB) {
         llvm::Value *IsEH = Builder.CreateLoad(getAbnormalTerminationSlot(),
                                                "abnormal.termination");
         IsEH = Builder.CreateICmpEQ(IsEH, llvm::ConstantInt::get(Int8Ty, 0));
         Builder.CreateCondBr(IsEH, FI.ContBB, FI.ResumeBB);
       } else {
         // There was nothing exceptional in the try body, so we only have normal
         // control flow.
         Builder.CreateBr(FI.ContBB);
       }
+    }
 
     Builder.restoreIP(SavedIP);
 
     return;
   }
 
   // Otherwise, we must have an __except block.
   const SEHExceptStmt *Except = S.getExceptHandler();

test/CodeGen/exceptions-seh-finally.c

int nested___finally___finally() {
   }
   return 0;
 }
 // CHECK-LABEL: define i32 @nested___finally___finally
 // CHECK: store i8 0, i8* %
 // CHECK-NEXT: br label %[[finally:[^ ]*]]
 //
 // CHECK: [[finally]]
-// CHECK-NEXT:  store i32 1, i32* %
-// CHECK-NEXT:  store i8 0, i8* %
+// CHECK-NEXT: store i32 1, i32* %
+// CHECK-NEXT: store i32 1, i32* %
+// CHECK-NEXT: br label %[[cleanup:[^ ]*]]
+//
+// The finally's unreachable continuation block:
+// CHECK: store i32 0, i32* %
+// CHECK-NEXT: br label %[[cleanup]]
+//
+// CHECK: [[cleanup]]
+// CHECK-NEXT: store i8 0, i8* %
 // CHECK-NEXT: br label %[[outerfinally:[^ ]*]]
 //
 // CHECK: [[outerfinally]]
 // CHECK-NEXT: br label %[[finallycont:[^ ]*]]
 //
 // CHECK: [[finallycont]]
-// CHECK-NEXT: ret i32 1
+// CHECK-NEXT: %[[dest:[^ ]*]] = load i32* %
+// CHECK-NEXT: switch i32 %[[dest]]
+// CHECK-NEXT: i32 0, label %[[cleanupcont:[^ ]*]]
+//
+// CHECK: [[cleanupcont]]
+// CHECK-NEXT: store i32 0, i32* %
+// CHECK-NEXT: br label %[[return:[^ ]*]]
+//
+// CHECK: [[return]]
+// CHECK-NEXT: %[[reg:[^ ]*]] = load i32* %
+// CHECK-NEXT: ret i32 %[[reg]]
+
+int nested___finally___finally_with_eh_edge() {
+  __try {
+    __try {
+      might_crash();
+    } __finally {
+      return 899;
+    }
+  } __finally {
+    // Intentionally no return here.
+  }
+  return 912;
+}
+// CHECK-LABEL: define i32 @nested___finally___finally_with_eh_edge
+// CHECK: invoke void @might_crash() #3
+// CHECK-NEXT: to label %[[invokecont:[^ ]*]] unwind label %[[lpad:[^ ]*]]
+//
+// CHECK: [[invokecont]]
+// CHECK-NEXT: store i8 0, i8* %[[abnormal:[^ ]*]]
+// CHECK-NEXT: br label %[[finally:[^ ]*]]
+
+// CHECK: [[finally]]
+// CHECK-NEXT: store i32 899, i32* %
+// CHECK-NEXT: store i32 1, i32* %
+// CHECK-NEXT: br label %[[cleanup:[^ ]*]]
+//
+// The inner finally's unreachable continuation block:
+// CHECK: store i32 0, i32* %
+// CHECK-NEXT: br label %[[cleanup]]
+//
+// CHECK: [[cleanup]]
+// CHECK-NEXT: store i8 0, i8* %
+// CHECK-NEXT: br label %[[outerfinally:[^ ]*]]
+//
+// CHECK: [[outerfinally]]
+// CHECK-NEXT: %[[abnormallocal:[^ ]*]] = load i8* %[[abnormal]]
+// CHECK-NEXT: %[[reg:[^ ]*]] = icmp eq i8 %[[abnormallocal]], 0
+// CHECK-NEXT: br i1 %[[reg]], label %[[finallycont:[^ ]*]], label %[[finallyresume:[^ ]*]]
+//
+// CHECK: [[finallycont]]
+// CHECK-NEXT: %[[dest:[^ ]*]] = load i32* %
+// CHECK-NEXT: switch i32 %[[dest]]
+// CHECK-NEXT: i32 0, label %[[cleanupcont:[^ ]*]]
+//
+// CHECK: [[cleanupcont]]
+// CHECK-NEXT: store i32 912, i32* %
+// CHECK-NEXT: br label %[[return:[^ ]*]]
+//
+//
+// CHECK: [[lpad]]
+// CHECK-NEXT: landingpad
+// CHECK-NEXT: cleanup
+// CHECK: store i8 1, i8* %[[abnormal]]
+// CHECK: br label %[[finally]]
+//
+// The inner finally's unreachable resume block:
+// CHECK: store i8 1, i8* %[[abnormal]]
+// CHECK-NEXT: br label %[[outerfinally]]
+//
+// CHECK: [[finallyresume]]
+// CHECK-NEXT: br label %[[ehresume:[^ ]*]]
+//
+// CHECK: [[return]]
+// CHECK-NEXT: %[[reg:[^ ]*]] = load i32* %
+// CHECK-NEXT: ret i32 %[[reg]]
+//
+// The ehresume block, not reachable either.
+// CHECK: [[ehresume]]
+// CHECK: resume

test/CodeGen/exceptions-seh-leave.c

 // CHECK: [[finally]]
 // CHECK-NEXT: invoke void bitcast (void (...)* @g to void ()*)()
 // CHECK-NEXT:       to label %[[g2_cont:.*]] unwind label %[[g2_lpad:.*]]
 
 // CHECK: [[g2_cont]]
 // CHECK-NEXT: br label %[[tryleave:[^ ]*]]
 // CHECK-NOT: store i32 23
 
+// Unused __finally continuation block
+// CHECK: store i32 51, i32* %
+// CHECK-NEXT: br label %[[tryleave]]
+
+// CHECK: [[tryleave]]
+// CHECK-NEXT: br label %[[trycont:[^ ]*]]
+
 // CHECK: [[g1_lpad]]
 // CHECK: store i8 1, i8* %
 // CHECK-NEXT:  br label %[[finally]]
 
 // CHECK: [[g2_lpad]]
 // CHECK-NOT: %[[abnormal]]
 // CHECK: br label %[[except:[^ ]*]]
 
 // CHECK: [[except]]
-// CHECK-NEXT: br label %[[trycont:[^ ]*]]
+// CHECK-NEXT: br label %[[trycont]]
 
 // CHECK: [[trycont]]
 // CHECK-NEXT: ret i32 1
 
-// CHECK: [[tryleave]]
-// CHECK-NEXT: br label %[[trycont]]
-
 int nested___except___except() {
   int myres = 0;
   __try {
     __try {
       g();
       myres = 16;
     } __except (1) {
       g();