<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Dec 3, 2014 at 2:08 PM, 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"><span class=""><br>
> On 2014-Dec-03, at 13:12, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Thu, Nov 27, 2014 at 9:41 AM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
</span><span class="">>> 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>
>><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>
<br>
</span>Ah, okay.  This makes sense to me now.<br>
<br>
I made a few testcases of my own and ran with/without your patch and I<br>
better understand what you've done.  I'd imagined a bunch of interactions<br>
with `DebugLoc` that your approach avoids.<br>
<span class=""><br>
>> 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>
><br>
> Another reason I sent this for review - I don't mind much waiting/how it gets done, just that it gets done.<br>
<br>
</span>Let's wait then.  I'm hoping to get things moving along soon.<br></blockquote><div><br>Sure thing.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
>> Idea: assume we solved the "incomplete" problems, leaving just (?) the<br>
>> macro problem.<br>
><br>
> Not sure what you mean by this - the macro problem is sort of the example of how the current solution is incomplete.<br>
<br>
</span>You split your description up into "incomplete" and "insufficient":<br>
<span class="">>>> 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></span></blockquote><div><br>Ah, right right.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
<br>
<br>
</span><span class="">>> 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>
><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<br>
> (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>
<br>
</span>*nod*<br>
<br>
(Although if the debuggers could understand it, I think having debug info<br>
for macros would be pretty cool.)</blockquote><div><br>Yep, could be rather handy. I think the full generality of that feature would probably look something like "multiple source locations" (maybe some kind of source location stack) - which could probably also be used to provide multiple levels of debug info - eg: allowing source location information for asm, LLVM IR, C(++) source, macro expansions, and maybe like a protobuf file or something - so you could step up & down through the abstractions.<br><br>- David<br> </div></div><br></div></div>