[WinEH] Fix single-block cleanup coloring

JosephTremoulet · JosephTremoulet · commit f3aff3140185 · 2015-09-10T16:51:25.000Z
Summary: The coloring code in WinEHPrepare queues cleanuprets' successors with the correct color (the parent one) when it sees their cleanuppad, and so later when iterating successors knows to skip processing cleanuprets since they've already been queued. This latter check was incorrectly under an 'else' condition and so inadvertently was not kicking in for single-block cleanups. This change sinks the check out of the 'else' to fix the bug. Reviewers: majnemer, andrew.w.kaylor, rnk Subscribers: llvm-commits Differential Revision: http://reviews.llvm.org/D12751 llvm-svn: 247299
diff --git a/llvm/lib/CodeGen/WinEHPrepare.cpp b/llvm/lib/CodeGen/WinEHPrepare.cpp
@@ -3245,14 +3245,15 @@ void WinEHPrepare::colorFunclets(Function &F,
     } else {
       // Note that this is a member of the given color.
       FuncletBlocks[Color].insert(Visiting);
-      TerminatorInst *Terminator = Visiting->getTerminator();
-      if (isa<CleanupReturnInst>(Terminator) ||
-          isa<CatchReturnInst>(Terminator) ||
-          isa<CleanupEndPadInst>(Terminator)) {
-        // These block's successors have already been queued with the parent
-        // color.
-        continue;
-      }
+    }
+
+    TerminatorInst *Terminator = Visiting->getTerminator();
+    if (isa<CleanupReturnInst>(Terminator) ||
+        isa<CatchReturnInst>(Terminator) ||
+        isa<CleanupEndPadInst>(Terminator)) {
+      // These blocks' successors have already been queued with the parent
+      // color.
+      continue;
     }
     for (BasicBlock *Succ : successors(Visiting)) {
       if (isa<CatchEndPadInst>(Succ->getFirstNonPHI())) {
diff --git a/llvm/test/CodeGen/WinEH/wineh-cloning.ll b/llvm/test/CodeGen/WinEH/wineh-cloning.ll
@@ -27,7 +27,7 @@ endcatch:
 ; Need two copies of the call to @h, one under entry and one under catch.
 ; Currently we generate a load for each, though we shouldn't need one
 ; for the use in entry's copy.
-; CHECK-LABEL: @test1(
+; CHECK-LABEL: define void @test1(
 ; CHECK: entry:
 ; CHECK:   store i32 %x, i32* [[Slot:%[^ ]+]]
 ; CHECK:   invoke void @f()
@@ -56,7 +56,7 @@ exit:
 ; Need two copies of %exit's call to @f -- the subsequent ret is only
 ; valid when coming from %entry, but on the path from %cleanup, this
 ; might be a valid call to @f which might dynamically not return.
-; CHECK-LABEL: @test2(
+; CHECK-LABEL: define void @test2(
 ; CHECK: entry:
 ; CHECK:   invoke void @f()
 ; CHECK:     to label %[[exit:[^ ]+]] unwind label %cleanup
@@ -91,7 +91,7 @@ exit:
 }
 ; Need two copies of %shared's call to @f (similar to @test2 but
 ; the two regions here are siblings, not parent-child).
-; CHECK-LABEL: @test3(
+; CHECK-LABEL: define void @test3(
 ; CHECK:   invoke void @f()
 ; CHECK:   invoke void @f()
 ; CHECK:     to label %[[exit:[^ ]+]] unwind
@@ -143,7 +143,7 @@ exit:
 ; Make sure we can clone regions that have internal control
 ; flow and SSA values.  Here we need two copies of everything
 ; from %shared to %exit.
-; CHECK-LABEL: @test4(
+; CHECK-LABEL: define void @test4(
 ; CHECK:  entry:
 ; CHECK:    to label %[[shared_E:[^ ]+]] unwind label %catch
 ; CHECK:  catch:
@@ -221,7 +221,7 @@ exit:
 ; Simple nested case (catch-inside-cleanup).  Nothing needs
 ; to be cloned.  The def and use of %x are both in %outer
 ; and so don't need to be spilled.
-; CHECK-LABEL: @test5(
+; CHECK-LABEL: define void @test5(
 ; CHECK:      outer:
 ; CHECK:        %x = call i32 @g()
 ; CHECK-NEXT:   invoke void @f()
@@ -277,7 +277,7 @@ exit:
 ; %left still needs to be created because it's possible
 ; the dynamic path enters %left, then enters %inner,
 ; then calls @h, and that the call to @h doesn't return.
-; CHECK-LABEL: @test6(
+; CHECK-LABEL: define void @test6(
 ; TODO: CHECKs
 
 
@@ -309,7 +309,7 @@ unreachable:
 ; Another case of a two-parent child (like @test6), this time
 ; with the join at the entry itself instead of following a
 ; non-pad join.
-; CHECK-LABEL: @test7(
+; CHECK-LABEL: define void @test7(
 ; TODO: CHECKs
 
 
@@ -347,7 +347,7 @@ unreachable:
 }
 ; %inner is a two-parent child which itself has a child; need
 ; to make two copies of both the %inner and %inner.child.
-; CHECK-LABEL: @test8(
+; CHECK-LABEL: define void @test8(
 ; TODO: CHECKs
 
 
@@ -380,5 +380,41 @@ unreachable:
 ; the parent of the other, but that we'd somehow lost track in the CFG
 ; of which was which along the way; generating each possibility lets
 ; whichever case was correct execute correctly.
-; CHECK-LABEL: @test9(
+; CHECK-LABEL: define void @test9(
 ; TODO: CHECKs
+
+define void @test10() personality i32 (...)* @__CxxFrameHandler3 {
+entry:
+  invoke void @f()
+    to label %unreachable unwind label %inner
+inner:
+  %cleanup = cleanuppad []
+  ; make sure we don't overlook this cleanupret and try to process
+  ; successor %outer as a child of inner.
+  cleanupret %cleanup unwind label %outer
+outer:
+  %catch = catchpad [] to label %catch.body unwind label %endpad
+catch.body:
+  catchret %catch to label %exit
+endpad:
+  catchendpad unwind to caller
+exit:
+  ret void
+unreachable:
+  unreachable
+}
+; CHECK-LABEL: define void @test10(
+; CHECK-NEXT: entry:
+; CHECK-NEXT:   invoke
+; CHECK-NEXT:     to label %unreachable unwind label %inner
+; CHECK:      inner:
+; CHECK-NEXT:   %cleanup = cleanuppad
+; CHECK-NEXT:   cleanupret %cleanup unwind label %outer
+; CHECK:      outer:
+; CHECK-NEXT:   %catch = catchpad [] to label %catch.body unwind label %endpad
+; CHECK:      catch.body:
+; CHECK-NEXT:   catchret %catch to label %exit
+; CHECK:      endpad:
+; CHECK-NEXT:   catchendpad unwind to caller
+; CHECK:      exit:
+; CHECK-NEXT:   ret void
