Debug location mismatch after argument promotion pass

[afikcohen1]

When ArgumentPromotionPass optimizes a function, it changes some arguments to be passed by value instead of by pointer or reference. As part of this optimization, it creates a load instruction before the function call and removes the original load inside the function. However, the debug location of the new load instruction is incorrectly set to the location of the call instruction, instead of the original load inside the function. This results in incorrect debug information and makes it harder to trace the source code during debugging.

https://godbolt.org/z/7d969doK6

[llvmbot]

@llvm/issue-subscribers-debuginfo

Author: None (afikcohen1)

When ArgumentPromotionPass optimizes a function, it changes some arguments to be passed by value instead of by pointer or reference. As part of this optimization, it creates a load instruction before the function call and removes the original load inside the function. However, the debug location of the new load instruction is incorrectly set to the location of the call instruction, instead of the original load inside the function. This results in incorrect debug information and makes it harder to trace the source code during debugging.

https://godbolt.org/z/7d969doK6

[jmorse]

I suppose there's a loss of debuggability if the extracted load causes a crash to happen in a different context to where it happens in the source-code / at -O0. However, to present the source-line where the load comes from, we also need to present it in its correct scope, otherwise incorrect types / variables could be presented in a deubgger. And I suppose that means we need to represent this as a partial inlining in debug-info metadata, which is potentially quite a bit of work. Debuggers would then appear to step in-and-out of the function between an inlined and non-inlined context, which might be unpleasant too.

I'm not aware of a C / C++ construct where this occurs, is it exclusive to OpenCL (which I see is in the reproducer)? Perhaps it's conceivable in an LTO context where functions become ìnternal` perhaps? It'd help to be able to rationalise what information could / should be presented in a debugger if there's a source-reproducer available.

[pogo59]

I would expect argument promotion to be possible in cases where the callee is static, or LTO knows all possible callers. This is clearly possible in C/C++.

[jmorse]

Hmmm -- and I wonder if the load that's being promoted can come from any part of the source function: it's it's guaranteed to execute then it might get hoisted to the entry block, or if it's a guaranteed-dereferencable perhaps it might be speculated? I can imagine a horror(ible to maintain) scenario where argument promotion passes a load across multiple internalised functions, and we have to synthesise inlining sites each time.

[dwblaikie]

yeah, here's an example of the load being hoisted multiple levels: https://godbolt.org/z/85948dh93

void f1(int);
__attribute__((noinline)) static void f2(int* i) {
    f1(*i);
}
__attribute__((noinline)) static void f3(int* i) {
    f2(i);
}
void f4(int* i) {
    f3(i);
}

f4(int*):
        mov     edi, dword ptr [rdi]
        jmp     f3(int*)

f3(int*):
        jmp     f2(int*)

f2(int*):
        jmp     f1(int)@PLT

[jmorse]

Interesting, that isn't an optimisation I've seen before. IIRC, seeing how scopes "inject" themselves into their parent scope, we might be able to get away with just attaching an inlinedAt to a DILocation for the extracted load, no further work required.

The stepping behaviours that will come with this strike me as being a major pain; handily I think the "key instructions" work might discourage LLVM from putting an is_stmt on the hoisted load, and thus it might not always appear when debugging.

[dwblaikie]

yeah... not sure - worth a shot? Feels like it'll be a bit awkward... let's assume the hoisting doesn't do anything related to changing sequencing or hoisting across conditionals... but can hoist through many layers of intermediate functions.

So like the example above, f4 could contain an inlined subroutine (f3) which contains an inlined subroutine (f2) which contains the load, then a call to f3, f3 has a call to f2, etc...

So it's still going to look like two distinct functions - because we can't put the call to f3 inside the the inlined subroutines, because then it'd look like f3(inlined) -> f2(inlined) -> f3 -> f2 -> f1, which isn't right...