Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Debug info after MIR inlining losses track which variables represent arguments #83217

Closed
Tracked by #81567
tmiasko opened this issue Mar 16, 2021 · 1 comment · Fixed by #109466
Closed
Tracked by #81567

Debug info after MIR inlining losses track which variables represent arguments #83217

tmiasko opened this issue Mar 16, 2021 · 1 comment · Fixed by #109466
Labels
A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) A-mir-opt-inlining Area: MIR inlining C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@tmiasko
Copy link
Contributor

tmiasko commented Mar 16, 2021

For example:

fn main() {
    println!("{}", f(std::env::args().count(), 1));
}

#[inline]
pub fn f(x: usize, y: usize) -> usize {
    x + y
}

Running rustc -Zinline-mir a.rs -g && gdb ./a currently results in:

(gdb) break a::f
(gdb) run
Breakpoint 1, a::f () at a.rs:7
7	    x + y
(gdb) info args
No arguments.
(gdb) info locals
x = 1
y = 1

It should be more or less as follows:

(gdb) break a::f
(gdb) run
Breakpoint 1, a::f (x=1, y=1) at a.rs:7
7	    x + y
(gdb) info args
x = 1
y = 1
(gdb) info locals
No locals.

@rustbot label +A-mir-opt-inlining +A-debuginfo

@rustbot rustbot added A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) A-mir-opt-inlining Area: MIR inlining labels Mar 16, 2021
@jonas-schievink jonas-schievink added C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 16, 2021
@khuey
Copy link
Contributor

khuey commented Oct 24, 2021

I took a look at this and it's non-trivial to fix. The good news is that the LLVM IR looks almost entirely correct to me. The line/scope/etc information looks good. The bad news is that the DILocalVariables that are emitted for the arguments of the inlined function are missing argument indexes, which causes LLVM to emit DW_TAG_variable instead of DW_TAG_formal_parameter, and for these to be interpreted by debuggers as local variables instead of arguments.

Whether a variable has an argument index or not is entirely inferred by the LLVM IR generation step. Only LocalKind::Arg locals are converted to VariableKind::ArgumentVariable, and the argument index is inferred from the Local::index (as index - 1, since index == 0 is reserved for the return value). When a function is inlined its arguments are converted to LocalKind::Temps in the caller. In order to fix this the compiler would need to track which LocalKind::Temps originated in which inline function, at least for the arguments of inline functions. This information doesn't currently survive the MIR inlining pass and there isn't an obvious (to me) place to store it for later use.

compiler-errors added a commit to compiler-errors/rust that referenced this issue Apr 12, 2023
…debug-info, r=wesleywiser

Preserve argument indexes when inlining MIR

We store argument indexes on VarDebugInfo. Unlike the previous method of relying on the variable index to know whether a variable is an argument, this survives MIR inlining.

We also no longer check if var.source_info.scope is the outermost scope. When a function gets inlined, the arguments to the inner function will no longer be in the outermost scope. What we care about though is whether they were in the outermost scope prior to inlining, which we know by whether we assigned an argument index.

Fixes rust-lang#83217

I considered using `Option<NonZeroU16>` instead of `Option<u16>` to store the index. I didn't because `TypeFoldable` isn't implemented for `NonZeroU16` and because it looks like due to padding, it currently wouldn't make any difference. But I indexed from 1 anyway because (a) it'll make it easier if later it becomes worthwhile to use a `NonZeroU16` and because the arguments were previously indexed from 1, so it made for a smaller change.

This is my first PR on rust-lang/rust, so apologies if I've gotten anything not quite right.
@bors bors closed this as completed in d8fc819 Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) A-mir-opt-inlining Area: MIR inlining C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants