[PATCH] D11041: New EH representation for MSVC compatibility

[Reid Kleckner]

rnk added inline comments.

================
Comment at: docs/LangRef.rst:5185-5186
@@ +5184,4 @@
+
+If a ``nextaction`` label is not present, the instruction unwinds out of
+the function it is located in.  The
+:ref:`personality function <personalityfn>` will look for an appropriate
----------------
s/the function it is located in/its parent function/?

================
Comment at: docs/LangRef.rst:5187-5188
@@ +5186,4 @@
+the function it is located in.  The
+:ref:`personality function <personalityfn>` will look for an appropriate
+catch block in the caller.
+
----------------
s/look for an appropriate catch block in the caller/continue processing exception handling actions in the caller/?

Also, the personality function isn't really responsible for moving to the caller, since the caller might have a different EH personality.

================
Comment at: docs/LangRef.rst:5194
@@ +5193,3 @@
+The instruction optionally takes a label, ``nextaction``, indicating
+where control should transfer to if none of the constituent
+``catchblock`` instructions are suitable for the in-flight exception.
----------------
s/constituent/preceding/?

================
Comment at: docs/LangRef.rst:5213-5214
@@ +5212,4 @@
+   catch block.
+-  A basic block that is not a catch-end block may not include a
+   '``catchendblock``' instruction.
+
----------------
This seems to be missing the restriction that exactly one catchblock must unwind to a catchendblock.

================
Comment at: docs/LangRef.rst:5254
@@ +5253,3 @@
+whose unwinding was interrupted with a
+:ref:`catchblock <i_catchblock>` instruction and transfer control to
+``normal``.
----------------
s/transfer/transfers/

================
Comment at: docs/LangRef.rst:5304
@@ +5303,3 @@
+
+      cleanupret { i8*, i32 } %exn unwind label %continue
+
----------------
Maybe add the void example:
  cleanupret void unwind to caller

================
Comment at: docs/LangRef.rst:5316
@@ +5315,3 @@
+
+      terminateblock [<args>*] unwind label <exception label>
+
----------------
The other instructions have the 'unwind to caller' form. Any reason not to list that here?

================
Comment at: docs/LangRef.rst:5323-5324
@@ +5322,4 @@
+system <ExceptionHandling.html#overview>`_ to specify that a basic block
+is a terminate block --- one where a personality routine attempts to transfer
+control to terminate the program.
+The ``args`` correspond to whatever information the personality
----------------
s/attempts to transfer control to terminate the program/may decide to terminate the program/?

================
Comment at: docs/LangRef.rst:5327-5328
@@ +5326,4 @@
+routine requires to know if this is an appropriate place to terminate the
+program.  Control is tranfered to the ``exception`` label if the
+``terminateblock`` is an appropriate handler for the in-flight exception.
+If the ``terminateblock`` is not an appropriate handler, execution of
----------------
s/if the terminateblock is an appropriate handler for the in-flight exception/if the personality routine decides not to terminate the program/?

================
Comment at: docs/LangRef.rst:5329-5331
@@ +5328,5 @@
+``terminateblock`` is an appropriate handler for the in-flight exception.
+If the ``terminateblock`` is not an appropriate handler, execution of
+the program is terminated via
+:ref:`personality function <personalityfn>` specific means.
+
----------------
I think we can drop this sentence from the overview.

================
Comment at: docs/LangRef.rst:5339-5340
@@ +5338,4 @@
+
+The ``terminateblock`` must be provided an ``exception`` label to
+transfer control to if the in-flight exception matches the ``args``.
+
----------------
Why? It might unwind to caller, consider a noexcept function.

================
Comment at: docs/LangRef.rst:8329
@@ +8328,3 @@
+
+      %res = cleanupblock { i8*, i32 } [label %nextaction]
+
----------------
I wonder if we should just do Itanium and MSVC examples for every instruction, since together they motivate why the instructions have the flexibility that they do.

================
Comment at: lib/IR/Verifier.cpp:2411-2412
@@ -2406,3 +2410,4 @@
 
-  // Verify that there is a landingpad instruction as the first non-PHI
+  // Verify that there is an exception block instruction is the first non-PHI
   // instruction of the 'unwind' destination.
+  Assert(
----------------
Comment needs more edits for grammar:
Verify that the first non-PHI instruction of the unwind destination is an exception handling instruction.

================
Comment at: lib/Transforms/Instrumentation/MemorySanitizer.cpp:2656-2674
@@ -2655,1 +2655,21 @@
 
+  void visitCleanupBlockInst(CleanupBlockInst &I) {
+    setShadow(&I, getCleanShadow(&I));
+    setOrigin(&I, getCleanOrigin());
+  }
+
+  void visitCatchBlock(CatchBlockInst &I) {
+    setShadow(&I, getCleanShadow(&I));
+    setOrigin(&I, getCleanOrigin());
+  }
+
+  void visitTerminateBlock(TerminateBlockInst &I) {
+    setShadow(&I, getCleanShadow(&I));
+    setOrigin(&I, getCleanOrigin());
+  }
+
+  void visitCatchEndBlockInst(CatchEndBlockInst &I) {
+    setShadow(&I, getCleanShadow(&I));
+    setOrigin(&I, getCleanOrigin());
+  }
+
----------------
Our new instructions may be void, and we don't need to set shadow memory for void instructions. As written, this fills the DenseMaps with a bunch of useless values. Let's avoid that.


http://reviews.llvm.org/D11041