[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:18 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> 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.
>

Ping on this - but expect it'll be some time in the new year, so I'll try
to remember to check back in a month or so.

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.


>
>
>>
>> >> 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/20141219/7424a43a/attachment.html>
-------------- next part --------------
diff --git test/CodeGenCXX/debug-info-same-line.cpp test/CodeGenCXX/debug-info-same-line.cpp
index 965a538..30af240 100644
--- test/CodeGenCXX/debug-info-same-line.cpp
+++ test/CodeGenCXX/debug-info-same-line.cpp
@@ -38,18 +38,17 @@ int main() {
 // CHECK: call {{.*}}noinline{{.*}}({{i32[ ]?[a-z]*}} 5, {{i32[ ]?[a-z]*}} 6), !dbg [[NOINLINE:![0-9]*]]
 // CHECK: call {{.*}}noinline{{.*}}({{i32[ ]?[a-z]*}} 7, {{i32[ ]?[a-z]*}} 8), !dbg [[NOINLINE]]
 
-// FIXME: These should be separate locations but because the two calls have the
-// same line /and/ column, they get coalesced into a single inlined call by
-// accident. We need discriminators or some other changes to LLVM to cope with
-// this. (this is, unfortunately, an LLVM test disguised as a Clang test - since
-// inlining is forced to happen here). It's possible this could be fixed in
-// Clang, but I doubt it'll be the right place for the fix.
+// Ensure that, even in the worst case (where the calls are on the same line and
+// column) inlining can still distinguish them. This is an LLVM test disguised
+// as a Clang test, unfortunately, but it demonstrates the point.
 // CHECK: = add {{.*}} !dbg [[FIRST_MACRO_INLINE:![0-9]*]]
-// CHECK: = add {{.*}} !dbg [[FIRST_MACRO_INLINE]]
+// CHECK-NOT: = add {{.*}} !dbg [[FIRST_MACRO_INLINE]]
 
-// Even if the functions are marked inline but do not get inlined, they
+// FIXME: Even if the functions are marked inline but do not get inlined, they
 // shouldn't use column information, and thus should be at the same debug
 // location.
+// Now that LLVM can handle inlining two calls from the same location, we should
+// remove the (imperfect) column info hacks like this.
 // CHECK: call {{.*}}inlsum{{.*}}({{i32[ ]?[a-z]*}} 13, {{i32[ ]?[a-z]*}} 14), !dbg [[INL_FIRST:![0-9]*]]
 // CHECK: call {{.*}}inlsum{{.*}}({{i32[ ]?[a-z]*}} 15, {{i32[ ]?[a-z]*}} 16), !dbg [[INL_SECOND:![0-9]*]]