<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 16, 2014 at 9:31 PM, Frédéric Riss <span dir="ltr"><<a href="mailto:friss@apple.com" target="_blank">friss@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div class="h5"><blockquote type="cite"><div>On 16 Oct 2014, at 19:59, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><br><br style="font-family:Menlo-Regular;font-size:11px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_quote" style="font-family:Menlo-Regular;font-size:11px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">On Thu, Oct 16, 2014 at 7:26 PM, Frédéric Riss<span> </span><span dir="ltr"><<a href="mailto:friss@apple.com" target="_blank">friss@apple.com</a>></span><span> </span>wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><br><div><blockquote type="cite"><div>On 16 Oct 2014, at 18:58, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 16, 2014 at 6:20 PM, Duncan P. N. Exon Smith<span> </span><span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span><span> </span>wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span><br>> On Oct 16, 2014, at 6:08 PM, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>> wrote:<br>><br>> On Thu, Oct 16, 2014 at 6:06 PM, Duncan P. N. Exon Smith<br>> <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>>><br>>>> On Oct 16, 2014, at 3:54 PM, Frederic Riss <<a href="mailto:friss@apple.com" target="_blank">friss@apple.com</a>> wrote:<br>>>><br>>>> 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.<br>>>><br>>>> The code in the testcase is reduced from a huge LTO link that encountered this situation in a much more complicated setup.<br>>>><br>>>><span> </span><a href="http://reviews.llvm.org/D5828" target="_blank">http://reviews.llvm.org/D5828</a><br>>><br>>> Weird.  This LGTM (after fixing a few super minor whitespace changes);<br>>> not sure if Eric is still looking.<br>><br>> Enh, I was trying to figure out if there was some way to avoid the<br>> void metadata node being create rather than avoiding RAUW'ing it. It's<br>> probably not a huge distinction at the moment.<br><br></span>I don't think it is a void metadata node; it's a void `CallInst`.<br><br>We have:<br><br>   <span> </span>call void @llvm.dbg.declare(metadata !{i32* %deadvar}, ...)<br>   <span> </span>%call = ...<br><span>   <span> </span>store i32 %call, i32* %deadvar, align 4, !dbg !21<br><br></span>Which converts to:<br><br>   <span> </span>call void @llvm.dbg.value(metadata !{i32* %call}, ...)<br>   <span> </span>%call = ...<br><br>Then, when %call gets RAUW'ed, `metadata !{i32* %call}` gets updated<br>to `void`.<br></blockquote><div><br></div><div>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)</div></div></div></div></div></blockquote><br></div></div></div><div>Are you advocating against the patch, or merely pointing out that the compiler should handle the testcase code differently?</div></div></blockquote><div><br></div><div>A little of both.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div>In the former case, here’s my answer :-) :</div><div><br></div><div>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).</div><div><br></div><div>The counter argument could be that only input code with UB could trigger such RAUW calls, but I wouldn’t bet on it.</div></div></blockquote><div><br></div><div>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 <span style="font-family:monospace">219702</span> - 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.<br><br>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).<br><br>Not a "over my dead body" argument against this patch, just a thought/perspective.<br></div></div></div></blockquote><div><br></div></div></div><div>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.</div></div></div></blockquote><div><br></div><div>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).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div>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.<br></div></div></div></blockquote><div><br></div><div>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.<br><br>- David</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div></div><div><br></div><div>Fred</div><div><br></div><div><br></div><blockquote type="cite"><div><div class="gmail_quote" style="font-family:Menlo-Regular;font-size:11px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div>- David  </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><br></div><div>Fred</div></div></blockquote></div></div></blockquote></div><br></div></blockquote></div><br></div></div>