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

On Wed, Feb 8, 2017 at 9:56 AM Adrian Prantl <aprantl at apple.com> wrote:
> 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>
wrote:
>
>
> > On Feb 8, 2017, at 9:25 AM, David Blaikie <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.
>
I think getMergedDebugLocation (or whatever it's called) currently returns
a null location, which is a bit different from line 0.

Line zero is tricky to use as it can make the line table larger (file
size)/worse for the user (similarly jumpy behavior, possibly, depending on
how the DWARF consumer handles it).

> 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?
>
Will make the line table larger, which might not be ideal (I remember Paul
tried this, then went to only using zero loc at the start of basic blocks
to ensure that block ordering didn't affect locations by causing un-lined
instructions at the start of a block to inherit the location at the end of
the previous block). But maybe for this new use case/concern the cost might
be worthwhile? Not sure.

> Or should we treat moving an instruction to a different BB different from
> CSE?
>
CSE's just the most compelling example, even if it's only a single move
it
could still be confusing to the user and the profile (eg: the block might
be conditional, the destination unconditional - so as a debugger user you'd
still be mislead into believing the condition is true because your code is
executing, when that's not the case)

- Dave

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

> On Feb 8, 2017, at 10:17 AM, David Blaikie <dblaikie at gmail.com>
wrote:
> 
> 
> 
> On Wed, Feb 8, 2017 at 9:56 AM Adrian Prantl <aprantl at apple.com
<mailto:aprantl at apple.com>> wrote:
>> On Feb 8, 2017, at 9:44 AM, David Blaikie <dblaikie at gmail.com
<mailto: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.
> 
> I think getMergedDebugLocation (or whatever it's called) currently
returns a null location, which is a bit different from line 0.
Indeed it does, I misremembered. Looks like we have the same latent problem with
all users of getMergedDebugLocation(), too.
> Line zero is tricky to use as it can make the line table larger (file
size)/worse for the user (similarly jumpy behavior, possibly, depending on how
the DWARF consumer handles it).
LLDB handles line 0 on call sites well (and we could easily fix it if it
didn't), I can't speak for gdb or the sce debugger. Do we have any data?
>  
> 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?
> 
> Will make the line table larger, which might not be ideal (I remember Paul
tried this, then went to only using zero loc at the start of basic blocks to
ensure that block ordering didn't affect locations by causing un-lined
instructions at the start of a block to inherit the location at the end of the
previous block). But maybe for this new use case/concern the cost might be
worthwhile? Not sure.
If we just limit it to function calls, the size increase might be acceptable.
We'd have to measure it.

> Or should we treat moving an instruction to a different BB different from
CSE?
> 
> CSE's just the most compelling example, even if it's only a single
move it could still be confusing to the user and the profile (eg: the block
might be conditional, the destination unconditional - so as a debugger user
you'd still be mislead into believing the condition is true because your
code is executing, when that's not the case)
> 
> - Dave
>  
> 
> -- adrian
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170208/c77f5397/attachment.html>

On Wed, Feb 8, 2017 at 10:32 AM Adrian Prantl <aprantl at apple.com>
wrote:
> On Feb 8, 2017, at 10:17 AM, David Blaikie <dblaikie at gmail.com>
wrote:
>
>
>
> On Wed, Feb 8, 2017 at 9:56 AM Adrian Prantl <aprantl at apple.com>
wrote:
>
> 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>
wrote:
>
>
> > On Feb 8, 2017, at 9:25 AM, David Blaikie <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.
>
>
> I think getMergedDebugLocation (or whatever it's called) currently
returns
> a null location, which is a bit different from line 0.
>
>
> Indeed it does, I misremembered. Looks like we have the same latent
> problem with all users of getMergedDebugLocation(), too.
>
Yep - though I imagine not all of them are liable to move calls.

>
> Line zero is tricky to use as it can make the line table larger (file
> size)/worse for the user (similarly jumpy behavior, possibly, depending on
> how the DWARF consumer handles it).
>
>
> LLDB handles line 0 on call sites well (and we could easily fix it if it
> didn't), I can't speak for gdb or the sce debugger. Do we have any
data?
>
Not off-hand, no.

>
>
>
> 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?
>
>
> Will make the line table larger, which might not be ideal (I remember Paul
> tried this, then went to only using zero loc at the start of basic blocks
> to ensure that block ordering didn't affect locations by causing
un-lined
> instructions at the start of a block to inherit the location at the end of
> the previous block). But maybe for this new use case/concern the cost might
> be worthwhile? Not sure.
>
>
> If we just limit it to function calls, the size increase might be
> acceptable. We'd have to measure it.
>
Ah, true

>
>
> Or should we treat moving an instruction to a different BB different from
> CSE?
>
>
> CSE's just the most compelling example, even if it's only a single
move it
> could still be confusing to the user and the profile (eg: the block might
> be conditional, the destination unconditional - so as a debugger user
you'd
> still be mislead into believing the condition is true because your code is
> executing, when that's not the case)
>
> - Dave
>
>
>
> -- adrian
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170209/1b0eec25/attachment.html>