[PATCH] D11041: New EH representation for MSVC compatibility
Joseph Tremoulet
jotrem at microsoft.com
Thu Jul 9 09:27:05 PDT 2015
JosephTremoulet added a comment.
I'm happy to see this coming online! I have some inline questions about the details.
================
Comment at: docs/LangRef.rst:4730-4734
@@ -4729,3 +4729,7 @@
':ref:`indirectbr <i_indirectbr>`', ':ref:`invoke <i_invoke>`',
-':ref:`resume <i_resume>`', and ':ref:`unreachable <i_unreachable>`'.
+':ref:`resume <i_resume>`', ':ref:`catchblock <i_catchblock>`',
+':ref:`catchendblock <i_catchendblock>`',
+':ref:`catchret <i_catchret>`',
+':ref:`terminateblock <i_terminateblock>`',
+and ':ref:`unreachable <i_unreachable>`'.
----------------
Missing cleanupret here
================
Comment at: docs/LangRef.rst:5123-5125
@@ +5122,5 @@
+
+The ``catchblock`` must be provided a ``normal`` label to transfer control
+to if the ``catchblock`` matches the exception and an ``exception``
+label to transfer control to if it doesn't.
+
----------------
What should the exception label look like for a catch that is unconditionally entered (like catch(...) in C++)? Or do front-ends need to translate such things to cleanups?
================
Comment at: docs/LangRef.rst:5140
@@ +5139,3 @@
+
+The ``catchblock`` instruction has several restrictions:
+
----------------
Is there also a restriction on the target of the exception label? E.g. that it must be a catchblock or catchendblock?
================
Comment at: docs/LangRef.rst:5144-5149
@@ +5143,8 @@
+ an exceptional instruction.
+- A catch block must have a '``catchblock``' instruction as its
+ first non-PHI instruction.
+- There can be only one '``catchblock``' instruction within the
+ catch block.
+- A basic block that is not a catch block may not include a
+ '``catchblock``' instruction.
+
----------------
I'd imagine we're going to want a word for this set of three restrictions; we have a few of these and I expect some optimizations will want to know that these are in effect without caring which specific block type this is. I think I saw the term "unsplittable block" used for this in one of the discussions on the dev alias, which I like because the restriction is very similar to that of an unsplittable edge (i.e. you can't put code in it).
I'm not sure what's the right way to express that in these documents. E.g. should it just say "A catch block is unsplittable" and there's a glossary or something that can spell out what that means? Having an explicit section discussing unsplittable blocks may be warranted since it may be a construct that many aren't used to.
I think my best suggestion for now is to add a bullet that says "A catch block is 'unsplittable':" and then the bullets you have become sub-bullets of that (and likewise for the other unsplittable blocks).
================
Comment at: docs/LangRef.rst:5184
@@ +5183,3 @@
+in-flight exception.
+
+If a ``nextaction`` label is not present, the instruction unwinds out of
----------------
If we have
```
try {
code;
try {
code;
} catch (T e) { // <--- catch 1
MayThrow();
}
code;
} catch (T2 e2) { // <---- catch 2
code;
}
```
is the invoke for MayThrow supposed to target the catchbolck for catch2, or the catchendblock for catch1? If it's the catchendblock, having some verbiage here explaining that would be good.
================
Comment at: docs/LangRef.rst:5256
@@ +5255,3 @@
+:ref:`catchblock <i_catchblock>` instruction and transfers control to
+``normal``.
+
----------------
Mention that this reads and writes memory?
================
Comment at: docs/LangRef.rst:5263
@@ +5262,3 @@
+
+ catchret unwind label %continue
+
----------------
That "unwind" shouldn't be there, should it?
================
Comment at: docs/LangRef.rst:5265
@@ +5264,3 @@
+
+.. _i_cleanupret:
+
----------------
nit: You introduce cleanupret before you introduce cleanupblock; that reads strangely.
================
Comment at: docs/LangRef.rst:5275-5276
@@ +5274,4 @@
+
+ cleanupret <type> <value> unwind label <continue>
+ cleanupret <type> <value> unwind to caller
+
----------------
My impression from the RFC discussion was that cleanupret would also have a 'from' label indicating its cleanupblock (though I didn't see a reply to John's questions about that). What happened to that?
================
Comment at: docs/LangRef.rst:5289-5290
@@ +5288,4 @@
+The '``cleanupret``' instruction requires one argument, which must have the
+same type as the result of any '``cleanupblock``' instruction in the same
+function. It also has an optional successor, ``continue``.
+
----------------
This implies that all cleanupblocks in a given function have the same result type; should that be mentioned as a restriction on cleanupblock and/or checked in the verifier?
================
Comment at: docs/LangRef.rst:5296-5297
@@ +5295,4 @@
+The '``cleanupret``' instruction indicates to the
+:ref:`personality function <personalityfn>` that the
+:ref:`cleanupblock <i_cleanupblock>` it transfered control to has ended.
+It transfers control to ``continue`` or unwinds out of the function.
----------------
Since cleanupblocks can nest, I think "one cleanupblock" would be more clear here than "the cleanupblock". Also s/transfered/transferred/.
================
Comment at: docs/LangRef.rst:8292-8293
@@ +8291,4 @@
+execute the cleanup.
+:ref:`personality function <personalityfn>` upon re-entry to the
+function. The ``resultval`` has the type ``resultty``.
+
----------------
I think you're missing some words here ("personality function upon re-entry to the function." is a fragment).
================
Comment at: include/llvm/IR/Instructions.h:3804
@@ +3803,3 @@
+ BasicBlock *getSuccessor(unsigned i) const {
+ assert(i < 2 && "Successor # out of range for invoke!");
+ return i == 0 ? getNormalDest() : getUnwindDest();
----------------
s/invoke/catchblock/
================
Comment at: include/llvm/IR/Instructions.h:3809
@@ +3808,3 @@
+ void setSuccessor(unsigned idx, BasicBlock *NewSucc) {
+ assert(idx < 2 && "Successor # out of range for invoke!");
+ *(&Op<-2>() + idx) = reinterpret_cast<Value *>(NewSucc);
----------------
s/invoke/catchblock/
================
Comment at: lib/AsmParser/LLParser.cpp:4997
@@ +4996,3 @@
+ Lex.Lex();
+ if (ParseToken(lltok::kw_caller, "expected 'caller' in cleanupret"))
+ return true;
----------------
s/cleanupret/terminateblock/
================
Comment at: lib/AsmParser/LLParser.cpp:5025
@@ +5024,3 @@
+bool LLParser::ParseCatchEndBlock(Instruction *&Inst, PerFunctionState &PFS) {
+ if (ParseToken(lltok::kw_unwind, "expected 'unwind' in cleanupret"))
+ return true;
----------------
s/cleanupret/catchendblock/
================
Comment at: lib/IR/Instruction.cpp:468-471
@@ -457,2 +467,6 @@
return !CI->doesNotThrow();
+ if (const auto *CRI = dyn_cast<CleanupReturnInst>(this))
+ return CRI->unwindsToCaller();
+ if (const auto *CEBI = dyn_cast<CatchEndBlockInst>(this))
+ return CEBI->unwindsToCaller();
return isa<ResumeInst>(this);
----------------
Should there be a clause for TerminateBlock here?
================
Comment at: lib/IR/Verifier.cpp:383-386
@@ -382,2 +382,6 @@
void visitLandingPadInst(LandingPadInst &LPI);
+ void visitCatchBlockInst(CatchBlockInst &CBI);
+ void visitCatchEndBlockInst(CatchEndBlockInst &CEBI);
+ void visitCleanupBlockInst(CleanupBlockInst &CBI);
+ void visitTerminateBlockInst(TerminateBlockInst &TBI);
----------------
Nothing to check for CatchRet or CleanupRet? I think you could check that the CleanupRet unwind block is a (non-Landingpad?) EH block.
================
Comment at: lib/IR/Verifier.cpp:2840
@@ +2839,3 @@
+ &CBI);
+
+ visitTerminatorInst(CBI);
----------------
Would be good to check that the unwind successor has an appropriate target (must be an EH block, can't be a LandingPad, I'm not sure if you're intending cleanupblock and/or terminateblock to be allowed).
================
Comment at: lib/IR/Verifier.cpp:2840
@@ +2839,3 @@
+ &CBI);
+
+ visitTerminatorInst(CBI);
----------------
JosephTremoulet wrote:
> Would be good to check that the unwind successor has an appropriate target (must be an EH block, can't be a LandingPad, I'm not sure if you're intending cleanupblock and/or terminateblock to be allowed).
Check that all predecessors have exceptional terminators? Likewise for the other new EH blocks.
================
Comment at: lib/IR/Verifier.cpp:2857
@@ +2856,3 @@
+ &CEBI);
+
+ visitTerminatorInst(CEBI);
----------------
Check here that this is the target of no more than one catchblock?
================
Comment at: lib/IR/Verifier.cpp:2857
@@ +2856,3 @@
+ &CEBI);
+
+ visitTerminatorInst(CEBI);
----------------
JosephTremoulet wrote:
> Check here that this is the target of no more than one catchblock?
If there's an unwind successor, it must be a (non-Landingpad?) EHBlock.
================
Comment at: lib/IR/Verifier.cpp:2890
@@ +2889,3 @@
+ &TBI);
+
+ visitTerminatorInst(TBI);
----------------
If there's an unwind successor, it must be a (non-Landingpad?) EHBlock.
http://reviews.llvm.org/D11041
More information about the llvm-commits
mailing list