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

[Andy Kaylor via llvm-commits]

andrew.w.kaylor 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
----------------
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?

================
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
----------------
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.

================
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)) {
----------------
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.

================
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);
----------------
Shouldn't this be "if (!UselessCleanups.count(CPI))"?


http://reviews.llvm.org/D15325