[PATCH] D15325: [WinEH] Update CoreCLR EH state numbering

[Joseph Tremoulet via llvm-commits]

JosephTremoulet added inline comments.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:450
@@ +449,3 @@
+  // when really what it proved is that the catchswitch never takes its unwind
+  // edge (e.g. SimplifyUnreachable).  If we hit that case here, we will end up
+  // reporting such a catchswitch as top-level -- i.e. not inside any protected
----------------
andrew.w.kaylor wrote:
> This comment makes me nervous.  Besides indicating where exceptions that occur inside the catchpads unwind the unwind clause of a catchswitch also indicates where an exception will go if it is not caught by any of the handlers in the catchswitch.  So in order to change this clause from some EH pad to "unwinds to caller" we'd need to know both that none of the catch pads ever throws an exception and that all exceptions that come to this catch switch are caught by one of the catch pads (and even then, it seems like a bad idea).  It doesn't seem like any optimization outside of the WinEH-specific handling should be able to confirm this second condition since it depends on the personality function.  Is this really happening?
The only place I'm aware of making this transformation is in SimplifyCFG's SimplifyUnreachable -- if the catchswitch (and invokes etc within the catches) unwinds to a cleanuppad that immediately its `unreachable`, we'll `removeUnwindEdge` each of those preds -- so in that case,
 a) the inference is coming from the unwind destination, not analysis of the switch clauses, and
 b) the same transformation will get applied to the invokes and such in the catches
which may be of some comfort.

Point well taken that preserving the agreement between unwind destinations in catches w.r.t. each other and the catchswitch in general requires careful non-local analysis.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:464
@@ +463,3 @@
+  // The pad is a cleanuppad, which is the tricky case because what we want
+  // to know is if exceptions propagating out of it escape to the caller or
+  // not, but don't have direct linkage to that information.  The information
----------------
andrew.w.kaylor wrote:
> Does the phrase "exceptions propogating out of it" include both exceptions that cause the cleanup pad to be executed but reach the cleanupret instruction and exceptions that occur within the cleanup handler?  It seems from the code that follows that it does, but it wasn't entirely clear to me.
Yeah, it needs to mean both, I'll try to make that more clear.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:477
@@ +476,3 @@
+        if (const auto *CRI = dyn_cast<CleanupReturnInst>(U))
+          return CRI->unwindsToCaller();
+        if (const auto *CSI = dyn_cast<CatchSwitchInst>(U)) {
----------------
andrew.w.kaylor wrote:
> This doesn't seem quite right.  Consider the following scenario:
> ```
> cleanup:
>   %cl = cleanuppad within none []
>   invoke void @g()
>     to label %cleanup.cont unwind label %cleanup.cs
> cleanup.cs:
>   %cs = catchswitch within %cl [label %cleanup.catch] unwind label %nested.cleanup
> cleanup.catch:
>   %cp = catchpad within %cs []
>   catchret from %cp to label %cleanup.cont
> nested.cleanup:
>   %nc = cleanuppad within %cl []
>   call void @h()
>   cleanupret from %nc unwind to caller
> cleanup.cont:
>   cleanupret from %cl unwind to caller
> ```
> This case may return either true or false, depending on whether it encounters the nested catchswitch or the outer cleanupret first in the user list.
Yes, this was supposed to check if the catchswitch dest's parent is also the cleanup and continue to the next use if so.  Good catch, thanks, I'll fix and expand the test.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:492
@@ +491,3 @@
+        if (const auto *CPI = dyn_cast<CleanupPadInst>(U))
+          if (UselessCleanups.count(CPI))
+            Worklist.push_back(CPI);
----------------
andrew.w.kaylor wrote:
> Shouldn't this be "if (!UselessCleanups.count(CPI))"?
Whoops, yes, of course.  I'll maybe just remove this loop, though -- when the invokes are tagged with the cleanup, getting to this point means no exceptions escape the cleanup that aren't supposed to reach all the way up to caller, and so we can report it as top-level by the same reasoning that we use for faux-`unwinds to caller` catchswitches on line 454.


http://reviews.llvm.org/D15325