[PATCH] D100137: [JumpThreading] merge debug info when merging select+br

Thu Apr 8 20:16:15 PDT 2021

dblaikie added a comment.

In D100137#2678335 <https://reviews.llvm.org/D100137#2678335>, @nickdesaulniers wrote:

>> I guess it'd probably be worth fixing that code to do something to add a valid location to those function calls too
>
> Yeah, that sounds like the best long term solution to me; otherwise we'll be playing whack-a-mole with destructive transforms losing debug and results in confusing-for-developers error messages.  When splitting a critical edge, what location do we use if neither contain debug info?  If only one is missing debug info, I'd guess that `Instruction::applyMergedLocation` will pick the one existing one?

Not necessarily - if the code is going into another basic block, we shouldn't preserve the existing location

>> then the sanitizer shouldn't rely on this guarantee
>
> It's not so much the sanitizer relying on this, but the inliner. See http://reviews.llvm.org/rG93035c8f47589590b65041603524f48a7c007e1f which added it.  LTO just exposes it since normally we can't/don't inline callees from compiler-rt, IIUC.

Sorry, to phrase this more clearly - yes, for the sake of the inliner we added an invariant/constraint that calls from functions that have a !dbg attachment where the call may be inlined must have a !dbg attachment that has a scope chain that leads to the same DISubprogram as the caller's !dbg attachment.

The sanitizer is relying on a further guarantee that any instruction it builds a call from must have this requirement too - that guarantee isn't... guaranteed (ie: it's not enforced by the verifier/hasn't been determined to be a requirement/invariant of the IR).

>> then we probably want to add that to the verifier
>
> And then have to fix all of the bugs it finds...? A worthy cause, but perhaps a severe undertaking (as you alluded to with "calls must have locations if the function they're in have a location").

If they're all real bugs, I'm OK saying we should go that way and fix them as we go - that's what we did with the calls for the inliner & fixed a bunch of location quality bugs along the way. If there's a for-sure example where this new guarantee cannot be met (ie: a case that's definitely not a bug (because an instruction is synthesized from nothing or moved over between basic blocks or the like - whatever the things we do that let instructions not have locations) that isn't a bug, then it's clear we should allow/support it and stop the sanitizers from depending on this unguaranteed constraint.

But yeah, not really going to insist on pursuing it until the counterexample is provided - but it should be one or the other at least: Fix compiler-rt to not require the guarantee that all instructions it builds calls from have locations, or enforce that guarantee in the verifier.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100137