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.

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

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

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.

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

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.

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

Sorry to be late to the party, I was out yesterday.

I do want to point out that there are different motivations for the two cases. When moving/hoisting a single instruction, the motivation is to avoid bad debugger stepping and bad sample-profile data (we get a lot of complaints about bad debugger stepping, btw) for which we pay the price of not having totally precise instruction-to-source mapping. I think this is the right tradeoff, fwiw. But, when combining or merging instructions (with different source locations), the problem is that we can’t provide a correct source location, because there is not a single correct source location and we can’t do a one-to-many mapping. As it happens, we’re using the same mechanism to solve these two different problems, i.e. erase the debug location from the moved/combined instruction, but the motivations are still different.

Why the fuss about motivation? Because it means the moved-call and merged-call cases are different, and might want different solutions. It would be fine to add an “I’ve been moved” flag to the debug location of a moved instruction, with whatever necessary effects that flag might need to have; but it would not be fine to use this flag for merged/combined instructions, because in those cases we do not have a correct “original” source location to preserve. In the latter case, we still need to use line-0 for calls, because there’s no other correct thing to do.

Probably getMergedLocation should optionally return line-0 instead of null; whether it’s an optional don’t-return-null parameter or a separate getMergedLocationForCall API is an implementation detail. The code-motion cases won’t be using getMergedLocation anyway, so if we want to make a new API to set a “been-moved” flag, that seems pretty orthogonal to the merged-call problem. Or, of course, we can always skip the flag idea, using line-0 for moved calls and null for moved anything-else.

There was also a question about whether the SCE debugger would tolerate a call instruction with line-0. I expect it’s fine, but I will check. I might not have an answer right away as I am off again tomorrow.

–paulr

Sorry to be late to the party, I was out yesterday.

I do want to point out that there are different motivations for the two cases. When moving/hoisting a single instruction, the motivation is to avoid bad debugger stepping and bad sample-profile data (we get a lot of complaints about bad debugger stepping, btw) for which we pay the price of not having totally precise instruction-to-source mapping. I think this is the right tradeoff, fwiw. But, when *combining* or merging instructions (with different source locations), the problem is that we *can't* provide a correct source location, because there is not a single correct source location and we can't do a one-to-many mapping. As it happens, we're using the same mechanism to solve these two different problems, i.e. erase the debug location from the moved/combined instruction, but the motivations are still different.

Why the fuss about motivation? Because it means the moved-call and merged-call cases are different, and might want different solutions. It would be fine to add an "I've been moved" flag to the debug location of a moved instruction, with whatever necessary effects that flag might need to have; but it would *not* be fine to use this flag for merged/combined instructions, because in those cases we do not have a correct "original" source location to preserve. In the latter case, we still need to use line-0 for calls, because there's no other correct thing to do.

Probably getMergedLocation should optionally return line-0 instead of null; whether it's an optional don’t-return-null parameter or a separate getMergedLocationForCall API is an implementation detail.

We need something to this end to support getMergedLocationForCall for CallInstructions. I'm not yet sure what the best API design for this. It feels dangerous to let the caller decide whether a nullptr is acceptable.

The code-motion cases won't be using getMergedLocation anyway, so if we want to make a new API to set a "been-moved" flag, that seems pretty orthogonal to the merged-call problem. Or, of course, we can always skip the flag idea, using line-0 for moved calls and null for moved anything-else.

David, do you want to make an argument for why the "been-moved" flag is superior over using line-0?

There was also a question about whether the SCE debugger would tolerate a call instruction with line-0. I expect it's fine, but I will check. I might not have an answer right away as I am off again tomorrow.

Thanks for checking!

-- adrian

Sorry to be late to the party, I was out yesterday.

I do want to point out that there are different motivations for the two cases. When moving/hoisting a single instruction, the motivation is to avoid bad debugger stepping and bad sample-profile data (we get a lot of complaints about bad debugger stepping, btw) for which we pay the price of not having totally precise instruction-to-source mapping. I think this is the right tradeoff, fwiw. But, when combining or merging instructions (with different source locations), the problem is that we can’t provide a correct source location, because there is not a single correct source location and we can’t do a one-to-many mapping. As it happens, we’re using the same mechanism to solve these two different problems, i.e. erase the debug location from the moved/combined instruction, but the motivations are still different.

Why the fuss about motivation? Because it means the moved-call and merged-call cases are different, and might want different solutions. It would be fine to add an “I’ve been moved” flag to the debug location of a moved instruction, with whatever necessary effects that flag might need to have; but it would not be fine to use this flag for merged/combined instructions, because in those cases we do not have a correct “original” source location to preserve. In the latter case, we still need to use line-0 for calls, because there’s no other correct thing to do.

Probably getMergedLocation should optionally return line-0 instead of null; whether it’s an optional don’t-return-null parameter or a separate getMergedLocationForCall API is an implementation detail.

We need something to this end to support getMergedLocationForCall for CallInstructions. I’m not yet sure what the best API design for this. It feels dangerous to let the caller decide whether a nullptr is acceptable.

Yeah, I’d just have getMergedLocation check if it’s merging a call if that’s the way we want to go. (assuming that’s sufficient - I guess it probably is - a call couldn’t move implicitly due to its user moving or anything, it can’t be a ConstantExpr I wouldn’t think)

The code-motion cases won’t be using getMergedLocation anyway, so if we want to make a new API to set a “been-moved” flag, that seems pretty orthogonal to the merged-call problem. Or, of course, we can always skip the flag idea, using line-0 for moved calls and null for moved anything-else.

David, do you want to make an argument for why the “been-moved” flag is superior over using line-0?

Would allow us to keep the/a source location for other things like asan, but we aren’t already - so I’m fine with line 0 (& presumably no/zero discriminator).

Sorry to be late to the party, I was out yesterday.

I do want to point out that there are different motivations for the two cases. When moving/hoisting a single instruction, the motivation is to avoid bad debugger stepping and bad sample-profile data (we get a lot of complaints about bad debugger stepping, btw) for which we pay the price of not having totally precise instruction-to-source mapping. I think this is the right tradeoff, fwiw. But, when combining or merging instructions (with different source locations), the problem is that we can’t provide a correct source location, because there is not a single correct source location and we can’t do a one-to-many mapping. As it happens, we’re using the same mechanism to solve these two different problems, i.e. erase the debug location from the moved/combined instruction, but the motivations are still different.

Why the fuss about motivation? Because it means the moved-call and merged-call cases are different, and might want different solutions. It would be fine to add an “I’ve been moved” flag to the debug location of a moved instruction, with whatever necessary effects that flag might need to have; but it would not be fine to use this flag for merged/combined instructions, because in those cases we do not have a correct “original” source location to preserve. In the latter case, we still need to use line-0 for calls, because there’s no other correct thing to do.

Probably getMergedLocation should optionally return line-0 instead of null; whether it’s an optional don’t-return-null parameter or a separate getMergedLocationForCall API is an implementation detail.

We need something to this end to support getMergedLocationForCall for CallInstructions. I’m not yet sure what the best API design for this. It feels dangerous to let the caller decide whether a nullptr is acceptable.

Yeah, I’d just have getMergedLocation check if it’s merging a call if that’s the way we want to go. (assuming that’s sufficient - I guess it probably is - a call couldn’t move implicitly due to its user moving or anything, it can’t be a ConstantExpr I wouldn’t think)

I posted a proposal for that to https://reviews.llvm.org/D29833.

– adrian