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

[David Blaikie]

On Mon, Oct 20, 2014 at 10:39 AM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On Oct 20, 2014, at 10:34 AM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Mon, Oct 20, 2014 at 9:55 AM, Duncan P. N. Exon Smith <
> dexonsmith 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.
> > >
> > >
> > >>> 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.
> > >
> > > (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)
> >
> > I disagree with this direction.  I think ValueHandles should be updated
> > to track the new call instruction.
> >
> > What use would other ValueHandle users have for a void value? Should
> metadata be updated to handle referencing a void value instead, if it's a
> meaningful/useful thing to reference?
>
> There are uses for ValueHandles other than as metadata operands.
>
> This particular `void` value is a call instruction.  If an optimization
> (or anything else) has a side map of interesting calls (for some
> definition of "interesting"), the map should get updated when this call
> is replaced... not just redirected at null.  For someone interested in
> call instructions, the return type isn't necessarily relevant.


& why would this not be true of metadata? Its a reference to a value of
interest, even if that value is void, no?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141020/d37cffe8/attachment.html>