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

[David Blaikie]

On Wed, Dec 3, 2014 at 2:08 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > 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.
>

Sure thing.


>
> >> 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).
>

Ah, right right.


>
>
> >> 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.)


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.

- David
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141203/897c5310/attachment.html>