[llvm-dev] Stripping Debug Locations on cross BB moves, part 2 (PR31891)

[Adrian Prantl via llvm-dev]

> On Feb 8, 2017, at 9:44 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Wed, Feb 8, 2017 at 9:36 AM Adrian Prantl <aprantl at apple.com <mailto:aprantl at apple.com>> wrote:
> 
> > On Feb 8, 2017, at 9:25 AM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
> >
> > So Reid came across a case where the current strategy (dropping locations when they move across basic blocks) proves to be at odds with another precept we have: inlinable calls must have locations, so that if/when they are inlined the location can be accurately described (due to the nature of our IR representation of inlining, a location must be given for the call site so that the DIEs for the inlined subroutine can be described)
> >
> > Any ideas on how we should approach this?
> >
> > We could have a bit in DebugLoc (or other choice of representation) for the non-line location. For the line table this would work the same as absent DebugLoc - for calls it would cause call_file/line/discriminator (do we have a call_discriminator? We probably should, don't know if we do) to be omitted which is something the LLVM IR representation currently can't express.
> 
> Even if the function calls are not inlined it could be preferable to keep a debug location. It's a very odd debugging experience to be stopped at a breakpoint and then do "up" (or just "bt") to look at the parent frame and not have a location for the call in the parent frame.
> 
> Usually what will happen is it will have a location, it'll be the flow-on from the previous location (ie: we won't emit any line entry for the call, so it naturally ends up at the line of whatever instruction came before).
>  
> Is the stripping in this case motivated by correctness for profiling or just to smooth the line table to improve the stepping experience?
> 
> Mostly it seems the people pushing/contributing patches are motivated by profile correctness, as far as I understand (with the line table/debugging behavior 'improvement' being a nod to "there's at least an argument to be made that this could be justified by debuggability too"). At least that's the impression I get. I may've misunderstood.
>  
> If its about correctness for profiling, we should consider having a separate profiling location or a skip-for-profiling bit as David suggests, that honored when the CU has the new tune-for-profiling flag set.
> 
> I think that might be a bit too much, in my mind, for the tune-for-profiling flag. Adding things to make the profile more accurate seems good - removing things and 'hurting' debuggability because you opted for profiling seems like a different and more difficult-to-tolerate situation.
>  
> If it is about a better stepping experience, we just shouldn't do this for call sites.
> 
> Why? A call could produce weird stepping behavior as much as a non-call instruction, and could be similarly mis-attributed. CSE could hoist two distinct calls to pure functions into a common block and we'd pick one location - the stack trace and location would be confusing to the debugger user (it could end up indicating that the if branch was taken when it was really the else branch, being the canonical example)
> 
> Calls don't seem fundamentally special here - except due to limitations of the IR representation.

The CSE example is a good one, though I *think* in this case we will end up calling getMergedDebugLocation() which will return a line 0 location. Which brings me to the next idea: Couldn't we just assign a line 0 location instead of removing the DebugLoc and everybody would be happy?
Or should we treat moving an instruction to a different BB different from CSE?

-- adrian
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170208/9712cfd1/attachment.html>