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

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170208/90e9c7e9/attachment.html>

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

Is the stripping in this case motivated by correctness for profiling or just to
smooth the line table to improve the stepping experience?

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.

If it is about a better stepping experience, we just shouldn't do this for
call sites.

Other thoughts?
-- adrian

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.

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

This was my concrete example from the PR (
https://llvm.org/bugs/show_bug.cgi?id=31891):

void useit1(float);
void useit2(float);
double fabs(double);
float fabsf(float f) {
  return (float)fabs((double)f);
}
void hoistit(int cond, float f) {
  if (cond) {
    useit1(fabsf(f));
  } else {
    useit2(fabsf(f));
  }
}

Obviously, hoisting the common call to fabsf is profitable. What line
information should we assign to it, both for profiling and for line table
purposes?

Stripping the location info or saying "line 0" is equivalent to saying
"hoistit line 0", because we'll add a line table entry for the
prologue, as
David says. The alternative is to arbitrarily choose one of the others,
leading to a potentially confusing backtrace or profile. For the debugging
use case, this just seems like a cost of doing business with an optimized
binary, but for PGO, it creates a bad profile.

I think the proposed solution of stripping the line info but leaving scope
info on the call is probably the right thing to do for now. It handles all
use cases that we have today. Eventually I like Adrian's idea of a bit on
the DILocation or Instruction that says "sloc info consumer beware: this
instruction was hoisted or speculated". That preserves the most
information. The one use case I can think of for that bit it is sanitizer
instrumentation, where any source location is better than no source
location (want file, function, and approximate line), and grovelling around
in the nearby instruction stream isn't easy.

On Wed, 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170208/b6d419e2/attachment.html>