Get all (but one) of debuginfo tests to pass with MIR codegen. #32952
Conversation
|
r? @Aatch (rust_highfive has picked a reviewer for you, use r? to override) |
|
I expect the lldb part in |
|
swoon |
| // gdb-check:$2 = true | ||
| // gdb-command:print c | ||
| // gdb-check:$3 = 2.5 | ||
| // gdb-command:info args |
There was a problem hiding this comment.
I'm a bit hesitant about using info args for debuginfo tests. A gdb-check instruction just verifies that the argument can be found somewhere in the output, so without the numbering it's possible to overlook errors. For example, given the following program...
let x = 1;
breakpoint();
{
let x = 2;
breakpoint();
}
breakpoint();... we get a correct result with print:
gdb-command:print x
gdb-command:continue
gdb-command:print x
gdb-command:continue
gdb-command:print x
gdb-check:$1 = 1
gdb-check:$2 = 2
gdb-check:$3 = 2
produces:
$1 = 1
$2 = 2
$3 = 2 // let's say there's a bug in scoping and x still refers to wrong address
The error will be detected because there is not $3 = 1 in the output. But with info, the overlapping names might hide the error:
gdb-command:info locals
gdb-command:continue
gdb-command:info locals
gdb-command:continue
gdb-command:info locals
gdb-check:x = 1
gdb-check:x = 2
gdb-check:x = 1 // <-- this is actually redundant
produces:
x = 1
x = 2
x = 2 // the test would still pass, because `x = 1` can be found in the output.
There was a problem hiding this comment.
That's certainly a problem. I don't think I can get this test to work without either this workaround or a generalized solution for #32949.
I'm not entirely sure the test makes any sense anymore, btw, a the whole morestack stuff is gone.
I'd rather have a test that only makes sure the backtrace contains the argument names and their values, and it wouldn't need the myriad of cases found here.
There was a problem hiding this comment.
If the morestack stuff is gone for good, then this test should deleted, but function-prologue-stepping-regular.rs should then also handle GDB. I'm surprised that info args and print give different results here...
There was a problem hiding this comment.
Because of #32949, there isn't actually a point for the debugger to break on where the variables for arguments are not in scope, and the first point where it would stop is before those variables are actually initialized, so you get garbage locals shadowing the perfectly good args.
There was a problem hiding this comment.
Shouldn't there just be the arguments? I.e. do we have separate DWARF entries for the arguments and some later incarnation of the arguments in stack frame?
There was a problem hiding this comment.
Yes, because they are distinct lvalues in MIR. Even if we might skip building allocas for arguments (which would require us using llvm.dbg.value instead) and/or elide their moves to variables, we'd still need a notion of "call arguments" (e.g. what shows up in the backtrace) different from pattern bindings.
There was a problem hiding this comment.
OK, makes sense. However, this is exactly what these test cases are for: That print arg will print the correct value when setting a function breakpoint. So using info args is a bit like cheating :P
|
r? @michaelwoerister |
|
Why is there a |
|
@arielb1 It's pretty much how it works in old trans as well. |
eddyb
changed the title
|
@michaelwoerister Rebased, addressed the #32949 postponement and updated title & description. |
|
@bors r+ |
|
📌 Commit e2ac989 has been approved by |
|
Thanks, @eddyb! |
|
⌛ Testing commit e2ac989 with merge 6892277... |
Get all (but one) of debuginfo tests to pass with MIR codegen. I didn't get much feedback in #31005 so I went ahead and implemented something simple. Closes #31005, as MIR debuginfo should work now for most usecases. The `no-debug-attribute` test no longer assumes variables are in scope of `return`. We might also want to revisit that in #32949, but the test is more reliable now either way. In order to get one last function in the `associated-type` test pass, this PR also fixes #32790.
I didn't get much feedback in #31005 so I went ahead and implemented something simple.
Closes #31005, as MIR debuginfo should work now for most usecases.
#32949 tracks remaining shadowing problems that break
function-prologue-stepping-no-stack-check.The
no-debug-attributetest no longer assumes variables are in scope ofreturn.We might also want to revisit that in #32949, but the test is more reliable now either way.
In order to get one last function in the
associated-typetest pass, this PR also fixes #32790.