<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 27, 2014 at 9:41 AM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@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 class="HOEnZb"><div class="h5"><br>
> On 2014 Nov 24, at 12:32, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
> Hi dexonsmith, echristo, aprantl,<br>
><br>
> Starting in r176895 Clang began including column information on function calls in an attempt to fix inline debug info - if two calls from the same location (prior to r176895 this meant the same line, after r176895 it meant the same line and column) were both inlined, they'd get coalesced and treated as a single inline call ({call location} x {original location} was the uniqueness used to differentiate callers, if this was the same, they were the same call).<br>
><br>
> This solution (doubly so after it was narrowed in r177164) is incomplete (it wasn't applied to constructor calls (where it is still causing issues, such as the one worked around in r220923/PR21408) or member function calls) and insufficient: in the worst case, two calls from within a macro will be attributed to both the same line and column. It also produces possibly surprising and unintended side effects by adding column information to calls (increasing debug info size, surprising debuggers that aren't necessarily ready to cope with this, increasing the size of line tables, etc).<br>
><br>
> So, I present herein, a more robust solution to the problem. When inlining, uniquely identify the metadata associated with the inlined location ("InlinedAt") by adding an extra field to the end with a circular reference. In this way no two callers can be conflated together.<br>
><br>
> Once this is committed, I'll remove the hacks/workarounds from Clang. (& there's a test in Clang (test/CodeGenCXX/debug-info-same-line.cpp) that's currently testing this stuff in LLVM, so it'll need to be changed at the same time as this LLVM change (or removed/modified before it))<br>
><br>
> <a href="http://reviews.llvm.org/D6390" target="_blank">http://reviews.llvm.org/D6390</a><br>
><br>
> Files:<br>
>  lib/Transforms/Utils/InlineFunction.cpp<br>
>  test/DebugInfo/inline-debug-info-multiret.ll<br>
>  test/DebugInfo/inline-debug-info.ll<br>
>  test/DebugInfo/inline-no-debug-info.ll<br>
>  test/Transforms/Inline/debug-info-duplicate-calls.ll<br>
>  test/Transforms/Inline/debug-invoke.ll<br>
</div></div>> <D6390.16575.patch><br>
<br>
<br>
Have you looked at how many extra locations get created in an LTO<br>
bootstrap? </blockquote><div><br>Nope - that'd be one of the reasons I sent this for review. I haven't tried/got an LTO setup (open to instructions as to how) nor have much of an idea of how you've been doing the stat gathering for your analysis here.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Disabling uniquing for inlined locations seems like a pretty<br>
heavy hammer to me, and I worry that in contexts with heavy inlining it<br>
could cause an explosion.<br></blockquote><div><br>Any case where this creates more nodes due to inlining are cases that are buggy currently, so far as I know.<br><br>The point of adding the column info was to make every call site distinct from every other call site - this uniquing just does the same thing but entirely consistently, as far as I understand it.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Assuming disabling uniquing is the right goal, I really don't like this<br>
self-reference pattern.  I'm hoping to have a patch out early next week<br>
(I've been on holiday this week) that splits metadata and value -- and<br>
one of the first follow-ups will be a change to MDNode that allows<br>
"opting-out" of uniquing.  Do you think it makes sense to wait and use<br>
that, rather than introducing and pulling out an extra field?<br></blockquote><div><br>Another reason I sent this for review - I don't mind much waiting/how it gets done, just that it gets done.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Idea: assume we solved the "incomplete" problems, leaving just (?) the<br>
macro problem. </blockquote><div><br></div><div>Not sure what you mean by this - the macro problem is sort of the example of how the current solution is incomplete.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Doesn't clang keep track of the locations from inside<br>
the macros?  Could we, in theory, treat macro invocations as inlined<br>
locations from the macro definitions, at least for the sake of defining<br>
DILocations/DebugLocs?<br></blockquote><div><br>This, I think, would just create as many locations (and would make them pre-emptively, as the current solution does - even when no inlining occurs) as the uniquing approach I'm proposing here (essentially any attempt to unique from the frontend produces unique locations for every call site in the IR - doing it during inlining means we don't need to unique every call site, only those that actually get inlined, which seems like a strict improvement).<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Another idea (a little less optimistic) that might reduce the scope of<br>
the change:  can we detect call sites that won't be correctly uniqued,<br>
and turn off uniquing on them from the start?  This would be a way to<br>
limit the number of DILocation sites that lost uniquing.  </blockquote><div><br>It's only the "inlined at" ones that need uniquing (& the only ones I'm adding it to with this patch). These /could/ be reused for two calls from the same location to distinct functions, so we could be a bit fancier if we can opt in/out to each debug location, but I'm not sure whether that's of much/any benefit.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">(I guess I'm<br>
assuming here that turning off uniquing for DILocation is a bad thing.<br>
I'm almost certain that the way `DebugLoc` is currently implemented and<br>
used, this would cause a big problem.  I'm not sure, outside of<br>
`DebugLoc`, how much uniquing matters for `DILocation` though.  After<br>
the metadata-value split -- and `DebugLoc` is just a pointer to an<br>
`MDNode` -- it may not matter at all.)<br>
<br>
</blockquote></div><br></div></div>