[PATCH] D12108: [WinEH] Require token linkage in EH pad/ret signatures

[Joseph Tremoulet via llvm-commits]

JosephTremoulet added a comment.

Tightening the constructors and accessors isn't straightforward because we allow `UndefValue` in addition to `FooPadInst`.  I thought it was important to allow `UndefValue` so that it's legal to `replaceAllUsesWith(UndefValue)` on, say, an unreachable instruction that happens to be an EH pad.

Since we allow `UndefValue`, we need to be able to serialize it, which in turn means we need to deserialize it, so we need a constructor that takes `UndefValue *`.  I thought typing the constructor as `Value *` was cleanest, but I could switch to different overloads for the Undef and non-Undef cases if you prefer (assuming I was misinterpreting the dummy operands thing).

I actually first tried this change with the accessors tightened.  The `getFooPad()` methods were straightforward enough; just `dynamic_cast` and let `UndefValue` get translated to `nullptr`.  The `setFooPad()` methods are where I got stuck.  I felt like you should be able to round-trip `FooRetX->setFooPad(FooRetY->getFooPad())`, like we do in the copy ctor, and since `getFooPad()` can return `nullptr` that meant that `setFooPad` needed to accept `nullptr` and translate it to `UndefValue`.  With `setFooPad` doing that translation, I thought that `FooPadInst *` was a more appropriate type for the argument than `Value *`.  But once I made `setFooPad` take a `FooPadInst *`, the constructor (that I had taking a `Value *` for the reasons above) couldn't pass it straight through, and that didn't feel right either.


================
Comment at: docs/LangRef.rst:5197-5198
@@ +5196,4 @@
+-  It is undefined behavior for control to transfer from a ``catchpad`` to
+   itself without first executing a ``catchret`` that consumes the
+   ``catchpad``'s value (counting instances in of the same ``catchpad`` in
+   different call frames as distinct).
----------------
rnk wrote:
> s/consumes the catchpad's value/consumes the catchpad/ maybe?
> 
> I don't think the parenthetical makes this any clearer. WDYT?
> s/consumes the catchpad's value/consumes the catchpad/ maybe?
Yeah, that's better, will update.

> I don't think the parenthetical makes this any clearer. WDYT?
I wish the parenthetical were more clear, but I think the sentence would be incorrect without it.  I need EH Prep to be able to assume that there isn't a cycle from a pad to itself in a single function, but entering the same pad twice is of course totally fine in different activations of a recursive function.  I'd be happy to reword if you have a suggestion that makes it clear.  And I can remove the parenthetical if you think it's obvious that the recursive case is allowed, but that was my thinking.


================
Comment at: include/llvm/IR/Instructions.h:4022-4025
@@ -4039,7 +4021,6 @@
 private:
-  void init(BasicBlock *BB, Value *RetVal);
-  CatchReturnInst(BasicBlock *BB, Value *RetVal, unsigned Values,
+  void init(Value *CatchPad, BasicBlock *BB);
+  CatchReturnInst(Value *CatchPad, BasicBlock *BB,
                   Instruction *InsertBefore = nullptr);
-  CatchReturnInst(BasicBlock *BB, Value *RetVal, unsigned Values,
-                  BasicBlock *InsertAtEnd);
+  CatchReturnInst(Value *CatchPad, BasicBlock *BB, BasicBlock *InsertAtEnd);
 
----------------
rnk wrote:
> Ditto, should we be more strict, or does that have wide-ranging complexity? At the very least, we should assert in init if we don't already.
>At the very least, we should assert in init if we don't already

I originally had an assert in init, but it was failing running llvm-as over feature/exception.ll.  I hypothesized that maybe dummy operands are created during parsing to facilitate lexically-forward references?  I will put that assert back and debug deeper.


http://reviews.llvm.org/D12108