[PATCH] Ensure debug info for two calls to the same function from the same location are not merged

[Duncan P. N. Exon Smith]

> On 2014-Dec-03, at 13:12, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Thu, Nov 27, 2014 at 9:41 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>> Disabling uniquing for inlined locations seems like a pretty
>> heavy hammer to me, and I worry that in contexts with heavy inlining it
>> could cause an explosion.
>> 
> Any case where this creates more nodes due to inlining are cases that are buggy currently, so far as I know.
> 
> 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.

Ah, okay.  This makes sense to me now.

I made a few testcases of my own and ran with/without your patch and I
better understand what you've done.  I'd imagined a bunch of interactions
with `DebugLoc` that your approach avoids.

>> Assuming disabling uniquing is the right goal, I really don't like this
>> self-reference pattern.  I'm hoping to have a patch out early next week
>> (I've been on holiday this week) that splits metadata and value -- and
>> one of the first follow-ups will be a change to MDNode that allows
>> "opting-out" of uniquing.  Do you think it makes sense to wait and use
>> that, rather than introducing and pulling out an extra field?
> 
> Another reason I sent this for review - I don't mind much waiting/how it gets done, just that it gets done.

Let's wait then.  I'm hoping to get things moving along soon.

>> Idea: assume we solved the "incomplete" problems, leaving just (?) the
>> macro problem. 
> 
> Not sure what you mean by this - the macro problem is sort of the example of how the current solution is incomplete.

You split your description up into "incomplete" and "insufficient":
>>> 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).


>> Doesn't clang keep track of the locations from inside
>> the macros?  Could we, in theory, treat macro invocations as inlined
>> locations from the macro definitions, at least for the sake of defining
>> DILocations/DebugLocs?
> 
> 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).

*nod*

(Although if the debuggers could understand it, I think having debug info
for macros would be pretty cool.)