rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.2k stars 12.7k forks source link

Initialization of single-use-consts happens before the function prelude #128945

Closed saethlin closed 1 month ago

saethlin commented 2 months ago

I ran into this as part of https://github.com/rust-lang/rust/pull/128913

Compile this example program extracted from tests/debuginfo/function-arg-initialization.rs

fn binding(a: i64, b: u64, c: f64) {
    let x = 0;
}

fn main() {
    binding(19, 20, 21.5);
}

with rustc test.rs -g then load it into gdb with gdb test and behold:

(gdb) b test.rs:2
Breakpoint 1 at 0x15c90: file test.rs, line 2.
(gdb) run
Starting program: /tmp/test 
Downloading separate debug info for system-supplied DSO at 0x7ffff7fc5000
[Thread debugging using libthread_db enabled]                                                                                   
Using host libthread_db library "/usr/lib/libthread_db.so.1".

Breakpoint 1, test::binding (a=56, b=8, c=6.9533558074966682e-310) at test.rs:2
2      let x = 0;
(gdb) disas
Dump of assembler code for function _ZN4test7binding17h4d177e65956385d9E:
=> 0x0000555555569c90 <+0>: movl   $0x0,-0x1c(%rsp)
   0x0000555555569c98 <+8>: mov    %rdi,-0x18(%rsp)
   0x0000555555569c9d <+13>:    mov    %rsi,-0x10(%rsp)
   0x0000555555569ca2 <+18>:    movsd  %xmm0,-0x8(%rsp)
   0x0000555555569ca8 <+24>:    ret
End of assembler dump.

The bug here is that a breakpoint set inside a function can land on an instruction before the function prelude.

As far as I can tell, this is only possible with single-use-consts. You can get the right codegen in current rustc by setting -Zmir-enable-passes=-SingleUseConsts. But I think blaming that MIR pass would be mistake because that MIR pass is just a reimplementation of the same concept that used to be called ConstDebugInfo.

Naive attempts to bisect this using the breakpoint method above will all land on https://github.com/rust-lang/rust/pull/107404. But I think that is a wrong bisection, because that PR just fixed an existing MIR pass so that we produce the right span information in our debuginfo.

I think the root cause or the PR that first introduced the bug is https://github.com/rust-lang/rust/pull/73210. You can see in this godbolt demo that rustc 1.50.0 is the version where we start initializing x before the function prelude: https://godbolt.org/z/r5W3M4sG9. It's likely that nobody noticed or appreciated how codegen has changed because the debuginfo test for this scenario had been disabled.


This problem also afflicts tests/debuginfo/macro-stepping.rs, because its strange combination of single-use-consts on the same line as function calls makes stepping through the function hit all those lines before the prelude when the consts are initialized, then again when the functions are called.

khuey commented 1 month ago

Merely moving the constant initializations to after the prologue won't fix the stepping issue. We either need the ability to control which DILocations are treated as breakpoint locations by LLVM (a behavior which is currently entirely implicit and that cannot be controlled in the IR) or we would need to initialize constants at their point of use.

saethlin commented 1 month ago

@khuey I am very much a debugging/debuginfo/debugger newbie. If you have an amendment or an complete replacement for the issue wording or title (since I don't think you have permission to just modify this issue) please don't hesitate to open a new issue or tell me how to fix the wording above.

I agree that what you're describing sounds a lot more like the root cause of this and other debuginfo weirdness I saw in the test suite.

khuey commented 1 month ago

I don't have a simple suggestion on how to modify this issue because there are at least 3 potential user-visible issues here and they may all have different solutions:

  1. Moving these constants around at all results in the nonsensical stepping behavior you observe. Fixing that requires not moving them or LLVM changes to improve which lines are marked as breakpoint locations. Even then there's not a perfect solution: if the line let x = 42; is optimized down to a single store to a stack slot and that store is moved to the top of the function like it is today it's either going to appear out of line in stepping or it's not going to appear in stepping at all.
  2. Moving these constants above the prologue results in LLVM marking the wrong location as the prologue end, which is where the debugger stops at function entry. This results in the bogus argument values you mention. There are actually two distinct subissues here: a) rustc leaves the location of the constant on the IRBuilder to "contaminate" later spills such as those for arguments, and b) LLVM seems to infer that the prologue ends the moment it sees another DILocation. The former will be fixed by #130052. The latter I plan to try to workaround after that is merged by placing the argument spills ahead of the constant spills.
  3. Moving these constants around may result in unexpected behavior in the debugger where the constant either shadows or is shadowed by another variable and the user tries to print the variable in question. So far every case of this I've seen is attributable to the nonsensical stepping behavior but there may be additional cases that persist even after that is fixed.
khuey commented 1 month ago

@rustbot claim