[PATCH] Handle nested landing pads in outlined catch handlers for Windows C++ EH

[Andy Kaylor]

REPOSITORY
  rL LLVM

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:99
@@ +98,3 @@
+  DenseMap<const LandingPadInst *, LandingPadMap> LPadMaps;
+  DenseMap<LandingPadInst *, const LandingPadInst *> NestedLandingPads;
+  DenseMap<const BasicBlock *, BasicBlock *> LPadTargetBlocks;
----------------
rnk wrote:
> Can you add a comment about which is the key and which is the value? This goes from cloned LP to original LP if I read the code right. Maybe ClonedLPToOriginalLP would be a better name.
I'm not sure ClonedLPToOriginalLP is especially helpful, as it doesn't indicate why the landing pads in question were mapped.

I do agree that something needs to be done to make this more clear and that a comment doesn't seem quite sufficient.

How about NestedLPToOriginalLP?

Incidentally, I did the mapping in this way because I think that it is possible for the same landing pad to be nested within multiple handlers.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:479
@@ +478,3 @@
+      if (OriginalLPad == LPad) {
+        NestedLandingPads[LPadPair.first] = cast<LandingPadInst>(NewLPad);
+      }
----------------
rnk wrote:
> I *think* `LPadPair.second = NewLPad` will update the container. The subscript operation is making me nervous because it looks like mutation during iteration, even though we already know that the key exists in the container. DenseMap doesn't have this bug, but just the other day we were looking at an old implementation of TR1 hash_map which had a bug where it would potentially resize on this exact operation.
Alright, I'll give that a try.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:562-564
@@ +561,5 @@
+      Value *V = EHActions->getArgOperand(i);
+      // Find the exception variable operands and remap them into the
+      // outlined function. Exception variables should always have an alloca.
+      if (auto *AV = dyn_cast<AllocaInst>(V)) {
+        // See if this handler already has a copy of the needed value.
----------------
rnk wrote:
> I have an idea for how we can completely eliminate the need for this. What if we changed the catch object argument from an AllocaInst to the i32 value that would be used with llvm.framerecover? That way the i32 would mean the same thing in the parent function and the child functions.
Yeah, that should work and would clean up a lot of mess here.  It's not as readable, but hopefully no one will be reading these things much anyway.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:575
@@ +574,3 @@
+              EHActions->setArgOperand(i, MappedAV);
+              Found = true;
+            }
----------------
rnk wrote:
> break?
Yes, if we have to keep this code.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:585
@@ +584,3 @@
+        }
+      } else if (auto *HandlerArg = dyn_cast<Function>(V)) {
+        // This is checking for catch handler arguments. Cleanup handlers will
----------------
rnk wrote:
> Both the AllocaInst and Function cases of are pretty heavy. What do you think of pulling them out into helper functions? prepareExceptionHandlers is pretty long.
Sounds reasonable.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:586-591
@@ +585,8 @@
+      } else if (auto *HandlerArg = dyn_cast<Function>(V)) {
+        // This is checking for catch handler arguments. Cleanup handlers will
+        // have a void return type and filter functions will be bitcast to i8*.
+        // The return value alone is enough to identify an argument as a catch
+        // handler.  We could also pay attention to the sentinel value but that
+        // seems like more work than is necessary.
+        FunctionType *FnType = HandlerArg->getFunctionType();
+        if (FnType->getReturnType() == Int8PtrType) {
----------------
rnk wrote:
> We have a parseEHActions function implementation in a pending patch. It goes from llvm.eh.actions call back into a list of ActionHandlers. It may ultimately be cleaner to use that, but I don't want to block your patch on ours. Let's throw a FIXME in here to come around and do that.
OK

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:888-893
@@ -731,1 +887,8 @@
+              LPadTargetBlocks[MappedBB] = cast<BasicBlock>(MapEntry.second);
+            } // End if (match(II, eh_endcatch))
+          }   // End if (UnconditionalBr)
+        }     // End for (predecessors)
+      }       // End if (MapEntry is a BB in the handler)
+    }         // End for VMap entries
+  }           // End if (CatchAction)
 
----------------
rnk wrote:
> These "End" comments seem excessive, for this short of a loop.
I just really hate seeing that many consecutive end brackets.  They'll go away after I clean up the loops as you suggest above.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1196-1198
@@ +1195,5 @@
+                                       BasicBlock *NewBB) {
+  // We shouldn't encounter landing pads in the actual cleanup code, but they
+  // will appear in catch blocks.  Depending on where we started cloning, we may
+  // see one, but it will get dropped during dead block pruning.
+  Instruction *NewInst = new UnreachableInst(NewBB->getContext());
----------------
rnk wrote:
> I wouldn't say "We shouldn't encounter landing pads in the actual cleanup code." This is currently only true because the outlining process unconditionally turns invokes in cleanups into calls. In general, destructors with real invokes can be inlined, and we will need to handle that one day. For now, I would rephrase this comment along the lines of "Cleanup outlining currently demotes all invokes to calls, so all landingpads are trivially unreachable."
You mentioned this before, and I think I said then that I don't believe the MS runtime will allow an exception to be thrown in a cleanup handler.  Do you disagree?

http://reviews.llvm.org/D8596

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/