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

[David Blaikie]

On Mon, Oct 20, 2014 at 11:24 AM, Frédéric Riss <friss at apple.com> wrote:

>
> On Oct 20, 2014, at 9:36 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Mon, Oct 20, 2014 at 8:45 AM, Frédéric Riss <friss at apple.com> wrote:
>
>>
>> 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...
>>
>>
>>> 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.
>>
>>
>> You are aware of that, but I’ll mention it anyway. This sentence makes it
>> look like having a debug metadata node referencing a value could change the
>> outcome of an optimization. That would be very bad.
>>
>
> Yep (though I'm not sure how bad it is - we don't actually have any
> continuous integration tests to ensure we hold this invariant, but we do
> try to avoid breaking it and fix it when we discover it).
>
> I wasn't suggesting that - I was merely suggesting changing algorithms to
> ensure this doesn't happen ever (even when debug info is in use).
>
>
>>
>> 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 tried hard to come up with non-UB code that triggers this code path but
>> couldn’t find one that gets us exactly in that situation. Using the
>> DeadArgumentElimination pass to remove the unused return value gets us
>> close, but it special cases a replacement by a void value by setting all
>> references to a null value.
>>
>
> Sounds plausible - though I'm not sure which uses (other than debug
> info/metadata) of the return value exist and why the comment says "they
> will get removed later on" - I wonder what the comment is alluding to.
>
>
> I wondered also when reading it.
>
>
>> 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.
>>
>>
>> Would you prefer a patch that introduces an assert in the MDNode RAUW
>> callback and that special cases the introduction of a void call in
>> InstCombine (In the same way it is done in DAE for example) ?
>>
>
> I think that might be reasonable - I'd be inclined to remove the UB code
> as dead to address this, but at least replacing with null limits the scope
> of the weirdness to InstCombine, rather than allowing it for all uses of
> metadata.
>
>
> I’m wondering: are you talking about UB at the language level, or at the
> IR level? I produced the IR with C code that exposes UB per the C standard,
> but isn’t it true that the produced IR might be legit if you don’t know
> where it comes from? i couldn’t find anything in the langref that makes the
> test UB per the IR semantics. But maybe there is some unwritten rules that
> I’m not aware of.
>

Fair question, and I haven't looked, but I would kind of assume that
calling a function using the wrong type is UB in LLVM IR, even if it's not
written down explicitly to be so, because it pretty easily causes
weirdness. (what if it was calling a void() as an int() function - what's
the return value? Is it signalling?)

But I haven't looked at the IR spec to find supporting/refuting words for
this.


>
> Fred
>
> (again, other people might disagree with me & think what I'm suggesting is
> silly/wrong - I'm not the owner/arbiter/gate-keeper here, just trying to
> describe my perspective on the matter - if other people who have more
> experience with/own metadata/IR think the original patch is reasonable &
> I'm being silly, they can sign off on it)
>
> - David
>
>
>>
>> Fred
>>
>> - David
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141020/04bbc33b/attachment.html>