[PATCH] Prevent MDNode's RAUW from introducing a reference to a void Value.

[Duncan P. N. Exon Smith]

> On Oct 19, 2014, at 6:43 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
>> On Sat, Oct 18, 2014 at 11:06 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>> > On 2014 Oct 17, at 10:08, David Blaikie <dblaikie at gmail.com> wrote:
>> >
>> > I'm floating the idea that the right fix might be to drop the code, yes. We do this will null pointer dereferences and other cases of UB already (& yes, in LTO this means you might get some surprises - it's rather the nature of UB and optimizations).
>> >
>> >> Coming back to the patch in itself, I also don’t see how it could be the wrong thing to do. The TrackingVH in the MDNode is meant to handle the case of the Value going away. RAUWing with a void value is just another case of a vanishing value.
>> >
>> > I don't think it's really the same though, or at least I don't think it's clean that it needs to be the same. It's a pretty weird way for a value to go away & I'd consider not accepting it as a valid way for a value to go away without, possibly, a stronger example of where this is a reasonable way for a value to go away.
>> 
>> I guess you're suggesting that optimizations shouldn't be allowed to
>> RAUW a non-void instruction to a void one.  Assuming all the IR `Use`s
>> have been updated,
> 
> What uses of a void value can/could exist? I assume I can't have an Instruction who's operand is a void value... 
>  

The case is:

 1. You have one call instruction, whose type is not `void`.
 2. You want to replace it with another, whose type is `void`.
 3. You update all the users to do something reasonable.  Now the
    original call instruction has no users.
 4. You RAUW, to update all the ValueHandles, which (depending on the
    handle) will change to null or track the change.

>> this RAUW seems legitimate -- it's the only way to
>> update all the ValueHandles to the instruction.
>> I think you'd need strong justification to remove this feature.
> 
> I suppose the narrower claim would be: If an Instruction has (even debug/metadata) uses, you can't replace all its uses with a void value.

This implies that metadata imposes a restriction on optimizations,
which I understand to be an anti-goal.

> I don't think it's too unreasonable to ask for a single example of a reasonable use of this feature if we're going to consciously support it.

I maintain that the above use case is reasonable, and IMO the burden is
on you to show otherwise.  I believe your claim would be a major change
to the semantics of metadata operands.

Maybe you're just looking for a reliable testcase, though.  If that's
the case, a unit test would expose this flaw easily and reliably.

> Though I appreciate orthogonality of features and the ability to RAUW any value, even one with no uses, if its compatible with its users (a value with no uses is trivially compatible with all its users). But once this requires explicit support, I'm not sure how orthogonal it is.


Metadata operands are second-class.