<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Dec 19, 2014 at 5:13 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-19, at 15:10, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Wed, Dec 3, 2014 at 2:18 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Wed, Dec 3, 2014 at 2:08 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
><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>
> >> 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>
> 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>
><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>
> Let's wait then.  I'm hoping to get things moving along soon.<br>
><br>
> Sure thing.<br>
><br>
> Ping on this - but expect it'll be some time in the new year,<br>
<br>
</span>Leaving in 2 minutes for some holiday :).<br>
<span class=""><br>
> so I'll try to remember to check back in a month or so.<br>
<br>
</span>Please do!<br></blockquote><div><br>Pinging just so I don't forget.<br><br>Now that we have 'distinct', I guess I just need to change the stuff that builds 'inlinedAt' location nodes to use that flag, perhaps?<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I've delayed a little bit introducing `distinct` because it's not clear<br>
what behaviour is correct from `MapMetadata()`.<br>
<br>
Right now, if you send a node into `MapMetadata` that's part of a cycle,<br>
you get a completely new copy of the cycle (every node is duplicated).<br>
<br>
I think this is bad behaviour for debug info, but that's been the<br>
behaviour for a long time, and I suspect alias scopes and loops (non<br>
debug info metadata schemas that use self-references) rely on this<br>
behaviour during `CloneFunction()`.<br>
<br>
My idea for `distinct` was that it's a more clear approach than using<br>
a self-reference -- but I don't think it makes sense to get a new one<br>
out of `MapMetadata` unless operands change... at least, not in general.<br>
Some places you probably want a separate copy.<br>
<br>
(Still thinking.)<br>
<br>
(BTW, I haven't confirmed, but I suspect a lot of memory gets wasted<br>
this way during `llvm-link`.  We still have plenty of type cycles for<br>
types with local scope (defined in anonymous namespaces or functions).)<br>
<span class=""><br>
> Attaching a related clang patch for this just so I can get rid of it from my local stash & record it here with the related work.<br>
><br>
><br>
><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>
> You split your description up into "incomplete" and "insufficient":<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>
> Ah, right right.<br>
><br>
><br>
><br>
> >> 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>
> *nod*<br>
><br>
> (Although if the debuggers could understand it, I think having debug info<br>
> for macros would be pretty cool.)<br>
><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>
><br>
><br>
</span>> <clang-call.diff><br>
<br>
</blockquote></div><br></div></div>