[PATCH] Add eh.begincatch and eh.endcatch intrinsics

[Reid Kleckner]

This change is 50% docs and 50% verifier. I'm happy with the docs, but it's not clear to me that we can really have these verifier checks.


REPOSITORY
  rL LLVM

================
Comment at: docs/ExceptionHandling.rst:328
@@ +327,3 @@
+
+The Windows runtime function for C++ exception handling uses a mutli-phase
+approach.  When an exception occurs it searches the current callstack for a
----------------
"multi"

Would you say this is a two-phase approach? That's how I've always thought of it.

================
Comment at: docs/ExceptionHandling.rst:331
@@ +330,3 @@
+frame that has a handler for the exception.  If a handler is found, it then
+calls the unwind handler for each frame above the handler which has an
+unwind handler before calling the catch handler.  These calls are all made
----------------
I don't think "unwind handler" is a term used elsewhere in this document. "cleanup" would be consistent with the rest. Can you define the term if you want to use it in this section?

================
Comment at: docs/ExceptionHandling.rst:334-335
@@ +333,4 @@
+from a stack context different from the original frame in which the handler
+is defined.  Therefore, it is necessary for these handlers to be outlined
+from their original context before code generation.
+
----------------
maybe "it is necessary to outline these handlers from their original context before code generation"?

================
Comment at: docs/ExceptionHandling.rst:338
@@ +337,3 @@
+Catch handlers are called with a pointer to the handler itself as the first
+argument and a pointer to the parent frame's stack pointer as the second
+argument.  The catch handler uses the `llvm.recoverframe
----------------
I guess the stack pointer is really whatever pointer is established by the Win64 unwind opcodes. If the unwind codes end in .setframe, it's that register plus the specified offset, and otherwise it's the stack pointer. I'm not sure if it's worth writing this down here.

================
Comment at: docs/ExceptionHandling.rst:355
@@ +354,3 @@
+Unwind handlers perform any cleanup necessary as the frame goes out of scope,
+but the runtime handles the actual unwinding of the stack.  If an exception
+occurs in an unwind handler the runtime manages termination of the process.
----------------
Maybe "... scope, while the runtime" or "and the runtime"? I don't think the second part of the sentence opposes the first.

================
Comment at: docs/ExceptionHandling.rst:369-372
@@ +368,6 @@
+
+During the WinEHPrepare pass, the handler functions are outlined and the
+``landingpad`` instruction is followed by a call to the ``llvm.eh.actions``
+intrinsic which describes the order in which handlers will be processed from
+the logical location of the landing pad.  The ``llvm.eh.actions`` intrinsic is
+defined as returning the address at which execution will continue.  This is a
----------------
I don't think it's quite clear what's going to be left in the landingpad afterwards. Maybe:

During the WinEHPrepare pass, landingpad code is outlined into handler functions. Only stub landingpad basic blocks are left in the IR. Each landingpad contains a landingpad instruction, a call to @llvm.eh.actions, and an indirectbr to indicate possible catch handler destinations.

Then the bit about what eh.actions holds.

================
Comment at: lib/IR/Verifier.cpp:1052-1053
@@ -1048,1 +1051,4 @@
 
+/// \brief Verify that the begin catch intrinsic follows a landingpad and leads
+///        to an eh.endcatch.
+void Verifier::VerifyEHBeginCatch(CallInst &CI) {
----------------
At a high level, I'm not sure we should be verifying these things. I always try to keep in mind a transform that tries to merge similar basic blocks, like this:
  void f() {
    if (b) {
      try {
      } catch (int) {
        g(); g(); g();
      }
    } else {
      g(); g(); g();
    }
  }

If we deduplicated the calls to g(), we would end up with a CFG where the begincatch doesn't dominate the endcatch:
  g_bb:
    %dst = phi i1 [0, %else], [1, %catch]
    call void @g()
    call void @g()
    call void @g()
    br i1 %dst, label %catch.cont, label %else.cont

Note that the outliner has to leave this bb behind in the parent function, and it will also have to leave behind a call to endcatch which is statically reachable from the entry block. We know after preparation, however, that endcatch is dynamically unreachable, so we could transform it to unreachable and run SimplifyCFG to clean it up.

I agree this is a good check to run on frontend generated, IR, though. Maybe we should move this to the Lint pass? Maybe clang should run the lint pass in asserts mode?

================
Comment at: lib/IR/Verifier.cpp:1061
@@ +1060,3 @@
+  // to it must have come from a landing pad.
+  std::function<bool(BasicBlock *)> proceedsFromLPad = [&](BasicBlock *BB) {
+    VisitedBlocks.insert(BB);
----------------
First, I think it would be clearer to simply hoist this into a static local function. The only state you need is VisitedBlocks.

Second, std::function is more expensive than using auto. std::function will make an unnecessary opaque wrapper class for the lambda, which we don't need.

================
Comment at: lib/IR/Verifier.cpp:1068
@@ +1067,3 @@
+      return false;
+    for (pred_iterator I = pred_begin(BB), E = pred_end(BB); I != E; ++I) {
+      if (VisitedBlocks.find(*I) != VisitedBlocks.end())
----------------
I added a predecessors() range-based-for adapter for this.

================
Comment at: lib/IR/Verifier.cpp:1071
@@ +1070,3 @@
+        continue;
+      if (!proceedsFromLPad(*I))
+        return false;
----------------
You should use a BB worklist. This causes O(n) recursion in the depth of the CFG, which could be large.

http://reviews.llvm.org/D7398

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