[PATCH] D15139: [IR] Reformulate LLVM's EH funclet IR

[Joseph Tremoulet via llvm-commits]

JosephTremoulet added inline comments.

================
Comment at: lib/Analysis/EHPersonalities.cpp:54
@@ +53,3 @@
+  // For any block B, the "colors" of B are the set of funclets F (possibly
+  // including a root "funclet" representing the main function), such that
+  // F will need to directly contain B or a copy of B (where the term "directly
----------------
rnk wrote:
> I think the comment means that the list of colors may include the entry block, which isn't strictly speaking a funclet.
Right, I'm pretty sure I wrote the comment, and that's what I meant -- that the colors *for any given block* may or may not include the root.

================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:799-804
@@ +798,8 @@
+             FuncInfo, FuncletStart, FuncletEnd, BaseState)) {
+      // Compute the label to report as the start of this entry; use the EH
+      // start
+      // label for the invoke if we have one, otherwise (this is a call which
+      // may
+      // unwind to our caller and does not have an EH start label, so) use the
+      // previous end label.
+      const MCSymbol *ChangeLabel = StateChange.NewStartLabel;
----------------
Looks like clang-format mangled this comment's line wrapping

================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:415-419
@@ -423,7 +414,7 @@
       const MachineInstr &MI = *MBBI;
-      if (!VisitingInvoke && LastStateChange.NewState != NullState &&
+      if (!VisitingInvoke && LastStateChange.NewState != BaseState &&
           MI.isCall() && !EHStreamer::callToNoUnwindFunction(&MI)) {
         // Indicate a change of state to the null state.  We don't have
         // start/end EH labels handy but the caller won't expect them for
         // null state regions.
         LastStateChange.PreviousEndLabel = CurrentEndLabel;
----------------
rnk wrote:
> The way this is supposed to work is that we're not supposed to emit separate start/end labels for an invoke that happens to be in BaseState. This would occur in this kind of situation:
>   void f() {
>     HasDtor o;
>     try {
>       g(1);
>     } catch(...) {
>       g(2); // Also in BaseState
>     }
>   }
> 
> In this case, we don't need separate start/end labels for the g(2) call, we can just use the funclet start entry in BaseState. We can treat it just like a potentially throwing call.
Ok, I think I get it now.  The comment here isn't quite right, though -- should probably be something like

  Indicate a change of state to the base state.  The caller won't expect start/end
  EH labels for this transition, which is handy since we won't have them available
  in the case that BaseState == NullState and this is a call rather than an invoke.

================
Comment at: lib/CodeGen/AsmPrinter/WinException.cpp:469-473
@@ -478,7 +468,7 @@
   // Iteration hit the end of the block range.
-  if (LastStateChange.NewState != NullState) {
+  if (LastStateChange.NewState != BaseState) {
     // Report the end of the last new state
     LastStateChange.PreviousEndLabel = CurrentEndLabel;
     LastStateChange.NewStartLabel = nullptr;
-    LastStateChange.NewState = NullState;
+    LastStateChange.NewState = BaseState;
     // Leave CurrentEndLabel non-null to distinguish this state from end.
----------------
rnk wrote:
> I don't think I understand the question.
> 
> I think the idea is, when we get to the end of the funclet, if there was some previous state change out of the base state, we should emit label entries to transition back to the base state for the epilogue.
Makes sense now, thanks.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:180-184
@@ +179,7 @@
+    int BaseState = -1;
+    if (FuncletUnwindDest == InvokeUnwindDest) {
+      auto BaseStateI = FuncInfo.FuncletBaseStateMap.find(FuncletPad);
+      if (BaseStateI != FuncInfo.FuncletBaseStateMap.end())
+        BaseState = BaseStateI->second;
+    }
+
----------------
rnk wrote:
> I would expect the CLR to have the same kinds of constraints as C++ here.
> 
> The base state is the state of the funclet prologues and epilogues, and it is the state that we previously assigned to the endpads. If there is a plain callsite at the beginning of a funclet before any new exceptional scopes are created, it should be in the base state. If that callsite throws, the runtime should take the appropriate "next action", which would follow the catchswitch unwind edge or the cleanupret unwind edge.
> 
> Now that we don't have endpads, we need to identify callsites that unwind out of the funclet. We do this by comparing the unwind destination of the funclet with the unwind destination of each call site, and if they agree, they must be in the base state.
> 
> If we don't do this, we would end up in a situation where a call inside a catch funclet gets a state outside of the CatchLow/CatchHigh interval. I'm pretty sure this would confuse CxxFrameHandler3.
Thanks for the explanation.  CoreCLR wants what it calls "duplicate clauses" reported for funclets that correspond to handlers which were themselves in try blocks, so we actually do want to report a state range with the outer state number for a call at the top-level of such a funclet, so for CoreCLR `BaseState` is always `NullState` to force that reporting, and I hadn't been thinking of them as distinct concepts.


http://reviews.llvm.org/D15139