[PATCH] D59272: [DebugInfo] Select debug intrinsic line-numbers more carefully when promoting dbg.declare

Mon Mar 25 07:40:26 PDT 2019

jmorse added a comment.

Hi,

In D59272#1439778 <https://reviews.llvm.org/D59272#1439778>, @bjope wrote:

> Isn't there a risk that this (partially) is hiding bugs in some other passes, where the DebugLoc is picked from the wrong place. I guess that either it should be picked from an adjacent non-meta instruction. Or it should be set to unknown.
>  With this patch we only eliminate the risk that the faulty passes are picking an "incorrect" DebugLoc from a dbg.value intrinsic (or DBG_VALUE instruction). But the "faulty" pass could still pick incorrect DebugLoc from other instructions, right? Or is it always correct to take the DebugLoc from all other non-dbg.value instructions, including all other kinds of meta-instructions?
>
> Might be hard to find all such bugs. But with your known examples it shouldn't be too hard to find at least some places in opt/llc where the DebugLoc from the dbg.value is infecting some other instruction. Then perhaps we want to land this patch as well, just do avoid some not-yet-detected problems.

All true -- this patch can be characterised as simply reducing the risk of leaking definitely-wrong line numbers into non-meta IR instructions, which I think has some value.

Quickly testing what happens if IRBuilder is adjusted to not pick take DebugLocs from debug-intrinsics [0], roughly 750 of the 900 entries this patch changes, are also changed, possibly to different lines though. (I'll upload this trivial change sometime soon too).

That's likely the bulk of the leaking line numbers; while the rest could be hunted down, they're likely to be scattered through many places, so IMO this patch is the most time-efficient solution going forwards.

[0] https://github.com/llvm-mirror/llvm/blob/e3696113b639c8bf0b72d6c27dd76d6fdd8ebf61/include/llvm/IR/IRBuilder.h#L137

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59272/new/

https://reviews.llvm.org/D59272