[PATCH] D100137: [JumpThreading] merge debug info when merging select+br
David Blaikie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
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
More information about the llvm-commits
mailing list