[WinEH] Disallow cyclic unwinds

Summary:
Funclet-based EH personalities/tables likely can't handle these, and they
can't be generated at source, so make them officially illegal in IR as
well.


Reviewers: andrew.w.kaylor, rnk, majnemer

Subscribers: llvm-commits

Differential Revision: http://reviews.llvm.org/D15963

llvm-svn: 257274

docs/ExceptionHandling.rst

@@ -835,3 +835,7 @@ unwind as ``nounwind``, it is legal to nest a ``call`` or an "``unwind to
 caller``\ " ``catchswitch`` within a funclet pad that has an unwind
 destination other than caller; it is undefined behavior for such a ``call``
 or ``catchswitch`` to unwind.
+
+Finally, the funclet pads' unwind destinations cannot form a cycle.  This
+ensures that EH lowering can construct "try regions" with a tree-like
+structure, which funclet-based personalities may require.

lib/IR/Verifier.cpp

@@ -45,6 +45,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/IR/Verifier.h"
+#include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallPtrSet.h"
@@ -145,6 +146,11 @@ private:
     OS << *C;
   }
 
+  template <typename T> void Write(ArrayRef<T> Vs) {
+    for (const T &V : Vs)
+      Write(V);
+  }
+
   template <typename T1, typename... Ts>
   void WriteTs(const T1 &V1, const Ts &... Vs) {
     Write(V1);
@@ -204,6 +210,10 @@ class Verifier : public InstVisitor<Verifier>, VerifierSupport {
   /// given function and the largest index passed to llvm.localrecover.
   DenseMap<Function *, std::pair<unsigned, unsigned>> FrameEscapeInfo;
 
+  // Maps catchswitches and cleanuppads that unwind to siblings to the
+  // terminators that indicate the unwind, used to detect cycles therein.
+  MapVector<Instruction *, TerminatorInst *> SiblingFuncletInfo;
+
   /// Cache of constants visited in search of ConstantExprs.
   SmallPtrSet<const Constant *, 32> ConstantExprVisited;
 
@@ -245,9 +255,11 @@ public:
     Broken = false;
     // FIXME: We strip const here because the inst visitor strips const.
     visit(const_cast<Function &>(F));
+    verifySiblingFuncletUnwinds();
     InstsInThisBlock.clear();
     LandingPadResultTy = nullptr;
     SawFrameEscape = false;
+    SiblingFuncletInfo.clear();
 
     return !Broken;
   }
@@ -429,6 +441,7 @@ private:
   void visitConstantExpr(const ConstantExpr *CE);
   void VerifyStatepoint(ImmutableCallSite CS);
   void verifyFrameRecoverIndices();
+  void verifySiblingFuncletUnwinds();
 
   // Module-level debug info verification...
   void verifyTypeRefs();
@@ -1697,6 +1710,59 @@ void Verifier::verifyFrameRecoverIndices() {
   }
 }
 
+static Instruction *getSuccPad(TerminatorInst *Terminator) {
+  BasicBlock *UnwindDest;
+  if (auto *II = dyn_cast<InvokeInst>(Terminator))
+    UnwindDest = II->getUnwindDest();
+  else if (auto *CSI = dyn_cast<CatchSwitchInst>(Terminator))
+    UnwindDest = CSI->getUnwindDest();
+  else
+    UnwindDest = cast<CleanupReturnInst>(Terminator)->getUnwindDest();
+  return UnwindDest->getFirstNonPHI();
+}
+
+void Verifier::verifySiblingFuncletUnwinds() {
+  SmallPtrSet<Instruction *, 8> Visited;
+  SmallPtrSet<Instruction *, 8> Active;
+  for (const auto &Pair : SiblingFuncletInfo) {
+    Instruction *PredPad = Pair.first;
+    if (Visited.count(PredPad))
+      continue;
+    Active.insert(PredPad);
+    TerminatorInst *Terminator = Pair.second;
+    do {
+      Instruction *SuccPad = getSuccPad(Terminator);
+      if (Active.count(SuccPad)) {
+        // Found a cycle; report error
+        Instruction *CyclePad = SuccPad;
+        SmallVector<Instruction *, 8> CycleNodes;
+        do {
+          CycleNodes.push_back(CyclePad);
+          TerminatorInst *CycleTerminator = SiblingFuncletInfo[CyclePad];
+          if (CycleTerminator != CyclePad)
+            CycleNodes.push_back(CycleTerminator);
+          CyclePad = getSuccPad(CycleTerminator);
+        } while (CyclePad != SuccPad);
+        Assert(false, "EH pads can't handle each other's exceptions",
+               ArrayRef<Instruction *>(CycleNodes));
+      }
+      // Don't re-walk a node we've already checked
+      if (!Visited.insert(SuccPad).second)
+        break;
+      // Walk to this successor if it has a map entry.
+      PredPad = SuccPad;
+      auto TermI = SiblingFuncletInfo.find(PredPad);
+      if (TermI == SiblingFuncletInfo.end())
+        break;
+      Terminator = TermI->second;
+      Active.insert(PredPad);
+    } while (true);
+    // Each node only has one successor, so we've walked all the active
+    // nodes' successors.
+    Active.clear();
+  }
+}
+
 // visitFunction - Verify that a function is ok.
 //
 void Verifier::visitFunction(const Function &F) {
@@ -3150,6 +3216,10 @@ void Verifier::visitFuncletPadInst(FuncletPadInst &FPI) {
         } else {
           FirstUser = U;
           FirstUnwindPad = UnwindPad;
+          // Record cleanup sibling unwinds for verifySiblingFuncletUnwinds
+          if (isa<CleanupPadInst>(&FPI) && !isa<ConstantTokenNone>(UnwindPad) &&
+              getParentPad(UnwindPad) == getParentPad(&FPI))
+            SiblingFuncletInfo[&FPI] = cast<TerminatorInst>(U);
         }
       }
       // Make sure we visit all uses of FPI, but for nested pads stop as
@@ -3227,17 +3297,21 @@ void Verifier::visitCatchSwitchInst(CatchSwitchInst &CatchSwitch) {
          "CatchSwitchInst not the first non-PHI instruction in the block.",
          &CatchSwitch);
 
+  auto *ParentPad = CatchSwitch.getParentPad();
+  Assert(isa<ConstantTokenNone>(ParentPad) || isa<FuncletPadInst>(ParentPad),
+         "CatchSwitchInst has an invalid parent.", ParentPad);
+
   if (BasicBlock *UnwindDest = CatchSwitch.getUnwindDest()) {
     Instruction *I = UnwindDest->getFirstNonPHI();
     Assert(I->isEHPad() && !isa<LandingPadInst>(I),
            "CatchSwitchInst must unwind to an EH block which is not a "
            "landingpad.",
            &CatchSwitch);
-  }
 
-  auto *ParentPad = CatchSwitch.getParentPad();
-  Assert(isa<ConstantTokenNone>(ParentPad) || isa<FuncletPadInst>(ParentPad),
-         "CatchSwitchInst has an invalid parent.", ParentPad);
+    // Record catchswitch sibling unwinds for verifySiblingFuncletUnwinds
+    if (getParentPad(I) == ParentPad)
+      SiblingFuncletInfo[&CatchSwitch] = &CatchSwitch;
+  }
 
   Assert(CatchSwitch.getNumHandlers() != 0,
          "CatchSwitchInst cannot have empty handler list", &CatchSwitch);

test/CodeGen/WinEH/wineh-cloning.ll

@@ -233,48 +233,6 @@ exit:
 ; CHECK-NEXT:   br label %outer.ret
 
 
-define void @test9() personality i32 (...)* @__C_specific_handler {
-entry:
-  invoke void @f()
-    to label %invoke.cont unwind label %left
-invoke.cont:
-  invoke void @f()
-    to label %unreachable unwind label %right
-left:
-  %cp.left = cleanuppad within none []
-  call void @llvm.foo(i32 1)
-  invoke void @f() [ "funclet"(token %cp.left) ]
-    to label %unreachable unwind label %right
-right:
-  %cp.right = cleanuppad within none []
-  call void @llvm.foo(i32 2)
-  invoke void @f() [ "funclet"(token %cp.right) ]
-    to label %unreachable unwind label %left
-unreachable:
-  unreachable
-}
-; This is an irreducible loop with two funclets that enter each other.
-; CHECK-LABEL: define void @test9(
-; CHECK:     entry:
-; CHECK:               to label %invoke.cont unwind label %[[LEFT:.+]]
-; CHECK:     invoke.cont:
-; CHECK:               to label %[[UNREACHABLE_ENTRY:.+]] unwind label %[[RIGHT:.+]]
-; CHECK:     [[LEFT]]:
-; CHECK:       call void @llvm.foo(i32 1)
-; CHECK:       invoke void @f()
-; CHECK:               to label %[[UNREACHABLE_LEFT:.+]] unwind label %[[RIGHT]]
-; CHECK:     [[RIGHT]]:
-; CHECK:       call void @llvm.foo(i32 2)
-; CHECK:       invoke void @f()
-; CHECK:               to label %[[UNREACHABLE_RIGHT:.+]] unwind label %[[LEFT]]
-; CHECK:     [[UNREACHABLE_RIGHT]]:
-; CHECK:       unreachable
-; CHECK:     [[UNREACHABLE_LEFT]]:
-; CHECK:       unreachable
-; CHECK:     [[UNREACHABLE_ENTRY]]:
-; CHECK:       unreachable
-
-
 define void @test10() personality i32 (...)* @__CxxFrameHandler3 {
 entry:
   invoke void @f()

test/Verifier/invalid-eh.ll

@@ -15,6 +15,8 @@
 ; RUN: sed -e s/.T15:// %s | not opt -verify -disable-output 2>&1 | FileCheck --check-prefix=CHECK15 %s
 ; RUN: sed -e s/.T16:// %s | not opt -verify -disable-output 2>&1 | FileCheck --check-prefix=CHECK16 %s
 ; RUN: sed -e s/.T17:// %s | not opt -verify -disable-output 2>&1 | FileCheck --check-prefix=CHECK17 %s
+; RUN: sed -e s/.T18:// %s | not opt -verify -disable-output 2>&1 | FileCheck --check-prefix=CHECK18 %s
+; RUN: sed -e s/.T19:// %s | not opt -verify -disable-output 2>&1 | FileCheck --check-prefix=CHECK19 %s
 
 declare void @g()
 
@@ -282,3 +284,58 @@ declare void @g()
 ;T17:     cleanuppad within none []
 ;T17:     unreachable
 ;T17: }
+
+;T18: define void @f() personality void ()* @g {
+;T18:   entry:
+;T18:     invoke void @g()
+;T18:       to label %invoke.cont unwind label %left
+;T18:   invoke.cont:
+;T18:     invoke void @g()
+;T18:       to label %unreachable unwind label %right
+;T18:   left:
+;T18:     %cp.left = cleanuppad within none []
+;T18:     invoke void @g() [ "funclet"(token %cp.left) ]
+;T18:       to label %unreachable unwind label %right
+;T18:   right:
+;T18:     %cp.right = cleanuppad within none []
+;T18:     invoke void @g() [ "funclet"(token %cp.right) ]
+;T18:       to label %unreachable unwind label %left
+;T18:     ; CHECK18: EH pads can't handle each other's exceptions
+;T18:     ; CHECK18-NEXT: %cp.left = cleanuppad within none []
+;T18:     ; CHECK18-NEXT:  invoke void @g() [ "funclet"(token %cp.left) ]
+;T18:     ; CHECK18-NEXT:          to label %unreachable unwind label %right
+;T18:     ; CHECK18-NEXT:  %cp.right = cleanuppad within none []
+;T18:     ; CHECK18-NEXT:  invoke void @g() [ "funclet"(token %cp.right) ]
+;T18:     ; CHECK18-NEXT:          to label %unreachable unwind label %left
+;T18:   unreachable:
+;T18:     unreachable
+;T18: }
+
+;T19: define void @f() personality void ()* @g {
+;T19:   entry:
+;T19:     ret void
+;T19:   red:
+;T19:     %redpad = cleanuppad within none []
+;T19:     unreachable
+;T19:   red.inner:
+;T19:     %innerpad = cleanuppad within %redpad []
+;T19:     invoke void @g() [ "funclet"(token %innerpad) ]
+;T19:       to label %unreachable unwind label %green
+;T19:   green:
+;T19:     %greenswitch = catchswitch within none [label %catch] unwind label %blue
+;T19:   catch:
+;T19:     catchpad within %greenswitch [i32 42]
+;T19:     unreachable
+;T19:   blue:
+;T19:     %bluepad = cleanuppad within none []
+;T19:     cleanupret from %bluepad unwind label %red
+;T19:     ; CHECK19: EH pads can't handle each other's exceptions
+;T19:     ; CHECK19-NEXT: %redpad = cleanuppad within none []
+;T19:     ; CHECK19-NEXT: invoke void @g() [ "funclet"(token %innerpad) ]
+;T19:     ; CHECK19-NEXT:         to label %unreachable unwind label %green
+;T19:     ; CHECK19-NEXT: %greenswitch = catchswitch within none [label %catch] unwind label %blue
+;T19:     ; CHECK19-NEXT: %bluepad = cleanuppad within none []
+;T19:     ; CHECK19-NEXT: cleanupret from %bluepad unwind label %red
+;T19:   unreachable:
+;T19:     unreachable
+;T19: }