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

[David Blaikie]

On Thu, Oct 16, 2014 at 9:31 PM, Frédéric Riss <friss at apple.com> wrote:

>
> On 16 Oct 2014, at 19:59, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Thu, Oct 16, 2014 at 7:26 PM, Frédéric Riss <friss at apple.com> wrote:
>
>>
>> On 16 Oct 2014, at 18:58, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> On Thu, Oct 16, 2014 at 6:20 PM, Duncan P. N. Exon Smith <
>> dexonsmith at apple.com> wrote:
>>
>>>
>>> > On Oct 16, 2014, at 6:08 PM, Eric Christopher <echristo at gmail.com>
>>> wrote:
>>> >
>>> > On Thu, Oct 16, 2014 at 6:06 PM, Duncan P. N. Exon Smith
>>> > <dexonsmith at apple.com> wrote:
>>> >>
>>> >>> On Oct 16, 2014, at 3:54 PM, Frederic Riss <friss at apple.com> wrote:
>>> >>>
>>> >>> If you look at the added testcase, it cast a void (*func)() to a int
>>> (*func)(). The int typed result is referenced in a debug info metadata node
>>> (an argument to the dbg.value intrinsic). Then instcombine would remove the
>>> bitcast and turn the int returning function into a void returning function
>>> that gets RAUWd over the value in the MDNode. And at this point in the
>>> current code you have invalid IR.
>>> >>>
>>> >>> The code in the testcase is reduced from a huge LTO link that
>>> encountered this situation in a much more complicated setup.
>>> >>>
>>> >>> http://reviews.llvm.org/D5828
>>> >>
>>> >> Weird.  This LGTM (after fixing a few super minor whitespace changes);
>>> >> not sure if Eric is still looking.
>>> >
>>> > Enh, I was trying to figure out if there was some way to avoid the
>>> > void metadata node being create rather than avoiding RAUW'ing it. It's
>>> > probably not a huge distinction at the moment.
>>>
>>> I don't think it is a void metadata node; it's a void `CallInst`.
>>>
>>> We have:
>>>
>>>     call void @llvm.dbg.declare(metadata !{i32* %deadvar}, ...)
>>>     %call = ...
>>>     store i32 %call, i32* %deadvar, align 4, !dbg !21
>>>
>>> Which converts to:
>>>
>>>     call void @llvm.dbg.value(metadata !{i32* %call}, ...)
>>>     %call = ...
>>>
>>> Then, when %call gets RAUW'ed, `metadata !{i32* %call}` gets updated
>>> to `void`.
>>>
>>
>> My only thought is that this code has UB, right? I don't know whether we
>> should do something else with it, which might involve killing the dbg.value
>> metadata along with all the other instructions that are fruit of the UB
>> tree. (the whole basic block, anything else reachable from it, etc)
>>
>>
>> Are you advocating against the patch, or merely pointing out that the
>> compiler should handle the testcase code differently?
>>
>
> A little of both.
>
>
>> In the former case, here’s my answer :-) :
>>
>> Yes, it involves UB (at least I think, the bogus return value is never
>> used, but I suppose the call through the wrong pointer type in itself is
>> UB). And thus we would be in our right to just drop the call and the
>> associated dbg.value, but the fix in itself has nothing to do with UB. It
>> prevents any call of RAUW with a void type replacement Value to generate
>> invalid MDNodes. We could audit all RAUV callers to never do that, but
>> handling it directly in MDNode seems safer (and what would the caller do
>> anyway? the end-result would certainly be the same).
>>
>> The counter argument could be that only input code with UB could trigger
>> such RAUW calls, but I wouldn’t bet on it.
>>
>
> Generally my interest is in not conceding conditionals like this until we
> know they're necessary. It's an easy example, but consider my recent commit
> 219702 - a fairly innocuous conditional allowed a slew of bugs to be
> introduced. Where possible I'd like to understand the root cause of bugs
> and continue to fix those until we find cases where a certain thing
> actually needs to be supported. We can add assertions to make sure
> violations of those invariants fail early and are easier to debug, but
> silently accepting them because its expedient now may cost us not just the
> bugs we have today but many more we may introduce in the future by giving
> up that invariant.
>
> So it's not that I bet there wouldn't be other violations of this rule - I
> just wouldn't bet that the next one we find will be for unquestionably
> valid code (maybe the 5th one we find is, and we have to remove this
> invariant then - but if so, we've just found/fixed 5 bugs in the mean time
> - granted, we still lose the ability to enforce the invariant, but we have
> a solid example as to why its not a reasonable invariant).
>
> Not a "over my dead body" argument against this patch, just a
> thought/perspective.
>
>
> I can see your general point, but I have a hard time applying it to this
> case. Could you explicit what you think would be the right fix? From your
> first message I gather that you’d want to just drop the code in the name of
> UB, but I'm not sure this would always be the right thing to do (I must
> concede that I’ve never been a proponent of compilers using UB to prune the
> code. Especially so when there is no diagnostic of this behaviour.) In the
> trivial case of the testcase, the issue would be diagnosed to the user
> through warnings. But in an LTO scenario, this issue can be introduced
> without any diagnostic message.
>

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.

- David


>
> Fred
>
>
> - David
>
>
>>
>> Fred
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141017/e319d0ab/attachment.html>