Page MenuHomePhabricator

Ensure debug info for two calls to the same function from the same location are not merged
AbandonedPublic

Authored by dblaikie on Nov 24 2014, 12:32 PM.

Details

Summary

Starting in r176895 Clang began including column information on function calls in an attempt to fix inline debug info - if two calls from the same location (prior to r176895 this meant the same line, after r176895 it meant the same line and column) were both inlined, they'd get coalesced and treated as a single inline call ({call location} x {original location} was the uniqueness used to differentiate callers, if this was the same, they were the same call).

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

So, I present herein, a more robust solution to the problem. When inlining, uniquely identify the metadata associated with the inlined location ("InlinedAt") by adding an extra field to the end with a circular reference. In this way no two callers can be conflated together.

Once this is committed, I'll remove the hacks/workarounds from Clang. (& there's a test in Clang (test/CodeGenCXX/debug-info-same-line.cpp) that's currently testing this stuff in LLVM, so it'll need to be changed at the same time as this LLVM change (or removed/modified before it))

Diff Detail

Event Timeline

dblaikie updated this revision to Diff 16575.Nov 24 2014, 12:32 PM
dblaikie retitled this revision from to Ensure debug info for two calls to the same function from the same location are not merged.
dblaikie updated this object.
dblaikie added reviewers: dexonsmith, echristo, aprantl.
dblaikie added a subscriber: Unknown Object (MLST).
dblaikie added inline comments.Nov 24 2014, 12:33 PM
lib/Transforms/Utils/InlineFunction.cpp
829

Honestly I find this recursive loop function a bit hard to follow & wonder about refactoring it into a loop.

838

This code duplication I'm introducing here should be refactored away before the change is submitted, but I haven't decided how - open to ideas. (the obvious one is just to write a small utility function, but I'm wondering if it can be more refactored-by-design)

echristo edited edge metadata.Dec 3 2014, 2:57 PM

A few comments inline, but this type of problem is why we have discriminators as well - to be able to distinguish between two things that happen at the same location - this isn't the case for that, those are designed around things that happen at the same file/line/column but are in different basic blocks.

But essentially you're adding a unique discriminator to each inlined call which works for me to solve the problem.

lib/Transforms/Utils/InlineFunction.cpp
829

I think naming changes might make it easier, but I have no problem with you removing the recursion either.

866

Needs some comments here, it's a bit hard to follow the new logic. Maybe document the entire function algorithm now.

dexonsmith resigned from this revision.Fri, Jun 26, 3:12 PM
dblaikie abandoned this revision.Fri, Jun 26, 6:55 PM

This ended up being fixed by df706288fba670943e164893e1fa0ccda3e24895