[PATCH] D15961: [WinEH] Verify unwind edges against EH pad tree

[Joseph Tremoulet via llvm-commits]

JosephTremoulet marked 5 inline comments as done.
JosephTremoulet added a comment.

Feedback addressed.


================
Comment at: docs/ExceptionHandling.rst:801
@@ +800,3 @@
+* Any of these pads is exited when control unwinds to the function's caller,
+  either by a ``call`` which unwinds all the way to the function's caller,
+  a nested ``catchswitch`` marked "``unwinds to caller``", or a nested
----------------
Updated

================
Comment at: docs/LangRef.rst:5597
@@ +5596,3 @@
+which must be the label of another basic block beginning with either a
+``cleanuppad`` or ``catchswitch`` instruction.  This unwind destination must
+be a legal target with respect to the ``parent`` links, as described in the
----------------
Updated (here and catchswitch)

================
Comment at: docs/LangRef.rst:5598
@@ +5597,3 @@
+``cleanuppad`` or ``catchswitch`` instruction.  This unwind destination must
+be a legal target with respect to the ``parent`` links, as described in the
+`exception handling documentation\ <ExceptionHandling.html#wineh-constraints>`_.
----------------
Changed from "unwind edge must have a legal target" to "unwind destination must be a legal target" (here and catchswitch)

================
Comment at: lib/IR/Verifier.cpp:2937
@@ +2936,3 @@
+  // pad relationship.
+  Instruction *ToPad = &I;
+  Value *ToPadParent = getParentPad(ToPad);
----------------
Yep, thanks; kept the `ToPad` name to help readability below, but assigned from `&I` and rewrote appearances of `&I` to `ToPad` below.

================
Comment at: lib/IR/Verifier.cpp:2951
@@ +2950,3 @@
+      FromPad = CRI->getCleanupPad();
+      Assert(FromPad != ToPadParent, "A cleanupret must exit its cleanup", CRI);
+    } else if (auto *CSI = dyn_cast<CatchSwitchInst>(TI)) {
----------------
Sounds like you're understanding correctly.  I thought about it, and think it's preferable to use `CSI` and catch the self-switch case here, so I've updated to do so and included a test.  Also realized looking at this that we're allowing catchswitches to target their child catchpads, so added a check for that in D16011.


http://reviews.llvm.org/D15961