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

[Reid Kleckner]

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

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:479
@@ +478,3 @@
+      if (OriginalLPad == LPad) {
+        NestedLandingPads[LPadPair.first] = cast<LandingPadInst>(NewLPad);
+      }
----------------
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.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:537
@@ +536,3 @@
+    // That should leave OutlinedLPad as the last instruction in its block.
+    assert(&(OutlinedBB->getInstList().back()) == OutlinedLPad);
+
----------------
In general, you shouldn't need to reach for getInstList(). This can be written:
  assert(&OutlinedBB->back() == OutlinedLPad);

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:558
@@ +557,3 @@
+    SmallVector<BlockAddress *, 4> ActionTargets;
+    CallInst *EHActions = cast<CallInst>(ClonedRecover);
+    unsigned NumArgs = EHActions->getNumArgOperands();
----------------
You could probably just cast the result of the clone() call and save a local variable.

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

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:575
@@ +574,3 @@
+              EHActions->setArgOperand(i, MappedAV);
+              Found = true;
+            }
----------------
break?

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

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

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:592
@@ +591,3 @@
+        FunctionType *FnType = HandlerArg->getFunctionType();
+        if (FnType->getReturnType() == Int8PtrType) {
+          // Verify the rest of the function signature.
----------------
You can exit early if it's not i8*.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:600-601
@@ +599,4 @@
+          for (BasicBlock &NestedHandlerBB : *HandlerArg) {
+            if (auto *Ret =
+                    dyn_cast<ReturnInst>(NestedHandlerBB.getTerminator())) {
+              // Handler functions must always return a block address.
----------------
You can continue if it's not ReturnInst

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:608
@@ +607,3 @@
+              assert(BA->getFunction() == &F);
+              if (LPadTargetBlocks.count(BA->getBasicBlock())) {
+                // If the return value is the address of a block that we
----------------
Ditto, continue

================
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)
 
----------------
These "End" comments seem excessive, for this short of a loop.

================
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());
----------------
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."

http://reviews.llvm.org/D8596

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