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

Keep track of how scopes and locals are duplicated during inlining so that they can be correctly de-duplicated when emitting debug info #115455

Open
dpaoliello opened this issue Sep 1, 2023 · 0 comments
Labels
A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-debugging Working group: Bad Rust debugging experiences

Comments

@dpaoliello
Copy link
Contributor

dpaoliello commented Sep 1, 2023

Background

Original issue

The call to panic within a function like Option::unwrap is translated to LLVM as a tail call (as it will never return), when multiple calls to the same function like this are inlined LLVM will notice the common tail call block (i.e., loading the same panic string + location info and then calling panic) and merge them together.

When merging these instructions together, LLVM will also attempt to merge the debug locations as well, but this fails (i.e., debug info is dropped) as Rust emits a new DISubprogram at each inline site thus LLVM doesn't recognize that these are actually the same function and so thinks that there isn't a common debug location.

As an example of this, consider the following program:

#[no_mangle]
fn add_numbers(x: &Option<i32>, y: &Option<i32>) -> i32 {
    let x1 = x.unwrap();
    let y1 = y.unwrap();

    x1 + y1
}

When building for x86_64 Windows using 1.72 it generates (note the lack of .cv_loc before the call to panic, thus it will be attributed to the same line at the addq instruction):

	.cv_loc	0 1 3 0                        # src\lib.rs:3:0
	addq	$40, %rsp
	retq
	leaq	.Lalloc_f570dea0a53168780ce9a91e67646421(%rip), %rcx
	leaq	.Lalloc_629ace53b7e5b76aaa810d549cc84ea3(%rip), %r8
	movl	$43, %edx
	callq	_ZN4core9panicking5panic17h12e60b9063f6dee8E
	int3

Ideally, we would generate debug information that would allow LLVM to (partially) merge the locations together and thus maintain the correct inlining information and partial line info (since the inlined function was de-duplicated, we can't give the exact line number as that is ambiguous between the two inline sites, but we also shouldn't show an incorrect line number).

For example, we could generate something like below (which shows the panic! was inlined from unwrap in option.rs at line 935 into the current function in lib.rs at line 0):

	.cv_loc	0 1 3 0                        # src\lib.rs:3:0
	addq	$40, %rsp
	retq
	.cv_inline_site_id 6 within 0 inlined_at 1 0 0
	.cv_loc	6 2 935 0                       # library\core\src\option.rs:935:0
	leaq	.Lalloc_5f55955de67e57c79064b537689facea(%rip), %rcx
	leaq	.Lalloc_e741d4de8cb5801e1fd7a6c6795c1559(%rip), %r8
	movl	$43, %edx
	callq	_ZN4core9panicking5panic17hde1558f32d5b1c04E
	int3

New issue

I attempted to fix this by deduplicating sub-program, lexical block and variable debug info in #114643 however this resulted in asserts in LLVM when attempting to build for Linux (see #115156).

The root cause of those asserts is that Rust was generating overlapping DW_OP_LLVM_fragment info for the same local variable in two different @llvm.dbg.declare calls (i.e., that two different stack allocations represented the same part ("fragment") of the same local variable).

My current plan (#115417) is to work around this by not deduplicating lexical scopes and variables, but instead create a new lexical scope every time that a function is inlined so that subsequent lexical scopes and variables do not appear to be duplicates (from LLVM's perspective).

Long-term fix

Given that LLVM relies on exact matching to merge together debug locations, and that Clang appears to do this deduplicating in its debug information, it seems that the correct long-term fix is for Rust to also do this deduplicating.

However, to do this Rust needs to keep careful track of how locals and scopes are duplicated while inlining - either to avoid the duplication at the point of inlining or to be able to deduplicate them when emitting debug information.

@dpaoliello dpaoliello added the C-bug Category: This is a bug. label Sep 1, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 1, 2023
@wesleywiser wesleywiser added the A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) label Sep 5, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 8, 2023
Use the same DISubprogram for each instance of the same inlined function within a caller

# Issue Details:
The call to `panic` within a function like `Option::unwrap` is translated to LLVM as a `tail call` (as it will never return), when multiple calls to the same function like this are inlined LLVM will notice the common `tail call` block (i.e., loading the same panic string + location info and then calling `panic`) and merge them together.

When merging these instructions together, LLVM will also attempt to merge the debug locations as well, but this fails (i.e., debug info is dropped) as Rust emits a new `DISubprogram` at each inline site thus LLVM doesn't recognize that these are actually the same function and so thinks that there isn't a common debug location.

As an example of this, consider the following program:
```rust
#[no_mangle]
fn add_numbers(x: &Option<i32>, y: &Option<i32>) -> i32 {
    let x1 = x.unwrap();
    let y1 = y.unwrap();

    x1 + y1
}
```

 When building for x86_64 Windows using 1.72 it generates (note the lack of `.cv_loc` before the call to `panic`, thus it will be attributed to the same line at the `addq` instruction):

```llvm
	.cv_loc	0 1 3 0                        # src\lib.rs:3:0
	addq	$40, %rsp
	retq
	leaq	.Lalloc_f570dea0a53168780ce9a91e67646421(%rip), %rcx
	leaq	.Lalloc_629ace53b7e5b76aaa810d549cc84ea3(%rip), %r8
	movl	$43, %edx
	callq	_ZN4core9panicking5panic17h12e60b9063f6dee8E
	int3
```

# Fix Details:
Cache the `DISubprogram` emitted for each inlined function instance within a caller so that this can be reused if that instance is encountered again.

Ideally, we would also deduplicate child scopes and variables, however my attempt to do that with rust-lang#114643 resulted in asserts when building for Linux (rust-lang#115156) which would require some deep changes to Rust to fix (rust-lang#115455).

Instead, when using an inlined function as a debug scope, we will also create a new child scope such that subsequent child scopes and variables do not collide (from LLVM's perspective).

After this change the above assembly now (with <https://reviews.llvm.org/D159226> as well) shows the `panic!` was inlined from `unwrap` in `option.rs` at line 935 into the current function in `lib.rs` at line 0 (line 0 is emitted since it is ambiguous which line to use as there were two inline sites that lead to this same code):

```llvm
	.cv_loc	0 1 3 0                        # src\lib.rs:3:0
	addq	$40, %rsp
	retq
	.cv_inline_site_id 6 within 0 inlined_at 1 0 0
	.cv_loc	6 2 935 0                       # library\core\src\option.rs:935:0
	leaq	.Lalloc_5f55955de67e57c79064b537689facea(%rip), %rcx
	leaq	.Lalloc_e741d4de8cb5801e1fd7a6c6795c1559(%rip), %r8
	movl	$43, %edx
	callq	_ZN4core9panicking5panic17hde1558f32d5b1c04E
	int3
```
@jieyouxu jieyouxu added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-debugging Working group: Bad Rust debugging experiences and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 18, 2024
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.) C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-debugging Working group: Bad Rust debugging experiences
Projects
None yet
Development

No branches or pull requests

4 participants