Skip to content

Commit

Permalink
Auto merge of rust-lang#115417 - dpaoliello:fixdi, r=wesleywiser
Browse files Browse the repository at this point in the history
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
```
  • Loading branch information
bors committed Sep 8, 2023
2 parents ffc48e3 + 0689077 commit 62ebe3a
Show file tree
Hide file tree
Showing 7 changed files with 61 additions and 22 deletions.
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_gcc/src/debuginfo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ impl<'gcc, 'tcx> DebugInfoMethods<'tcx> for CodegenCx<'gcc, 'tcx> {
_fn_abi: &FnAbi<'tcx, Ty<'tcx>>,
_llfn: RValue<'gcc>,
_mir: &mir::Body<'tcx>,
) -> Option<FunctionDebugContext<Self::DIScope, Self::DILocation>> {
) -> Option<FunctionDebugContext<'tcx, Self::DIScope, Self::DILocation>> {
// TODO(antoyo)
None
}
Expand Down
32 changes: 18 additions & 14 deletions compiler/rustc_codegen_llvm/src/debuginfo/create_scope_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ pub fn compute_mir_scopes<'ll, 'tcx>(
cx: &CodegenCx<'ll, 'tcx>,
instance: Instance<'tcx>,
mir: &Body<'tcx>,
debug_context: &mut FunctionDebugContext<&'ll DIScope, &'ll DILocation>,
debug_context: &mut FunctionDebugContext<'tcx, &'ll DIScope, &'ll DILocation>,
) {
// Find all scopes with variables defined in them.
let variables = if cx.sess().opts.debuginfo == DebugInfo::Full {
Expand Down Expand Up @@ -51,7 +51,7 @@ fn make_mir_scope<'ll, 'tcx>(
instance: Instance<'tcx>,
mir: &Body<'tcx>,
variables: &Option<BitSet<SourceScope>>,
debug_context: &mut FunctionDebugContext<&'ll DIScope, &'ll DILocation>,
debug_context: &mut FunctionDebugContext<'tcx, &'ll DIScope, &'ll DILocation>,
instantiated: &mut BitSet<SourceScope>,
scope: SourceScope,
) {
Expand Down Expand Up @@ -86,7 +86,7 @@ fn make_mir_scope<'ll, 'tcx>(
let loc = cx.lookup_debug_loc(scope_data.span.lo());
let file_metadata = file_metadata(cx, &loc.file);

let dbg_scope = match scope_data.inlined {
let parent_dbg_scope = match scope_data.inlined {
Some((callee, _)) => {
// FIXME(eddyb) this would be `self.monomorphize(&callee)`
// if this is moved to `rustc_codegen_ssa::mir::debuginfo`.
Expand All @@ -95,18 +95,22 @@ fn make_mir_scope<'ll, 'tcx>(
ty::ParamEnv::reveal_all(),
ty::EarlyBinder::bind(callee),
);
let callee_fn_abi = cx.fn_abi_of_instance(callee, ty::List::empty());
cx.dbg_scope_fn(callee, callee_fn_abi, None)
debug_context.inlined_function_scopes.entry(callee).or_insert_with(|| {
let callee_fn_abi = cx.fn_abi_of_instance(callee, ty::List::empty());
cx.dbg_scope_fn(callee, callee_fn_abi, None)
})
}
None => unsafe {
llvm::LLVMRustDIBuilderCreateLexicalBlock(
DIB(cx),
parent_scope.dbg_scope,
file_metadata,
loc.line,
loc.col,
)
},
None => parent_scope.dbg_scope,
};

let dbg_scope = unsafe {
llvm::LLVMRustDIBuilderCreateLexicalBlock(
DIB(cx),
parent_dbg_scope,
file_metadata,
loc.line,
loc.col,
)
};

let inlined_at = scope_data.inlined.map(|(_, callsite_span)| {
Expand Down
8 changes: 5 additions & 3 deletions compiler/rustc_codegen_llvm/src/debuginfo/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ impl<'ll, 'tcx> DebugInfoMethods<'tcx> for CodegenCx<'ll, 'tcx> {
fn_abi: &FnAbi<'tcx, Ty<'tcx>>,
llfn: &'ll Value,
mir: &mir::Body<'tcx>,
) -> Option<FunctionDebugContext<&'ll DIScope, &'ll DILocation>> {
) -> Option<FunctionDebugContext<'tcx, &'ll DIScope, &'ll DILocation>> {
if self.sess().opts.debuginfo == DebugInfo::None {
return None;
}
Expand All @@ -304,8 +304,10 @@ impl<'ll, 'tcx> DebugInfoMethods<'tcx> for CodegenCx<'ll, 'tcx> {
file_start_pos: BytePos(0),
file_end_pos: BytePos(0),
};
let mut fn_debug_context =
FunctionDebugContext { scopes: IndexVec::from_elem(empty_scope, &mir.source_scopes) };
let mut fn_debug_context = FunctionDebugContext {
scopes: IndexVec::from_elem(empty_scope, &mir.source_scopes),
inlined_function_scopes: Default::default(),
};

// Fill in all the scopes, with the information from the MIR body.
compute_mir_scopes(self, instance, mir, &mut fn_debug_context);
Expand Down
9 changes: 7 additions & 2 deletions compiler/rustc_codegen_ssa/src/mir/debuginfo.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
use crate::traits::*;
use rustc_data_structures::fx::FxHashMap;
use rustc_index::IndexVec;
use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags;
use rustc_middle::mir;
use rustc_middle::ty;
use rustc_middle::ty::layout::TyAndLayout;
use rustc_middle::ty::layout::{HasTyCtxt, LayoutOf};
use rustc_middle::ty::Instance;
use rustc_middle::ty::Ty;
use rustc_session::config::DebugInfo;
use rustc_span::symbol::{kw, Symbol};
Expand All @@ -17,10 +19,13 @@ use super::{FunctionCx, LocalRef};

use std::ops::Range;

pub struct FunctionDebugContext<S, L> {
pub struct FunctionDebugContext<'tcx, S, L> {
/// Maps from source code to the corresponding debug info scope.
pub scopes: IndexVec<mir::SourceScope, DebugScope<S, L>>,
}

/// Maps from an inlined function to its debug info declaration.
pub inlined_function_scopes: FxHashMap<Instance<'tcx>, S>,
}
#[derive(Copy, Clone)]
pub enum VariableKind {
ArgumentVariable(usize /*index*/),
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_ssa/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ pub struct FunctionCx<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> {

mir: &'tcx mir::Body<'tcx>,

debug_context: Option<FunctionDebugContext<Bx::DIScope, Bx::DILocation>>,
debug_context: Option<FunctionDebugContext<'tcx, Bx::DIScope, Bx::DILocation>>,

llfn: Bx::Function,

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_ssa/src/traits/debuginfo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ pub trait DebugInfoMethods<'tcx>: BackendTypes {
fn_abi: &FnAbi<'tcx, Ty<'tcx>>,
llfn: Self::Function,
mir: &mir::Body<'tcx>,
) -> Option<FunctionDebugContext<Self::DIScope, Self::DILocation>>;
) -> Option<FunctionDebugContext<'tcx, Self::DIScope, Self::DILocation>>;

// FIXME(eddyb) find a common convention for all of the debuginfo-related
// names (choose between `dbg`, `debug`, `debuginfo`, `debug_info` etc.).
Expand Down
28 changes: 28 additions & 0 deletions tests/codegen/debuginfo-inline-callsite-location.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// compile-flags: -g -O

// Check that each inline call site for the same function uses the same "sub-program" so that LLVM
// can correctly merge the debug info if it merges the inlined code (e.g., for merging of tail
// calls to panic.

// CHECK: tail call void @_ZN4core9panicking5panic17h{{([0-9a-z]{16})}}E
// CHECK-SAME: !dbg ![[#first_dbg:]]
// CHECK: tail call void @_ZN4core9panicking5panic17h{{([0-9a-z]{16})}}E
// CHECK-SAME: !dbg ![[#second_dbg:]]

// CHECK-DAG: ![[#func_dbg:]] = distinct !DISubprogram(name: "unwrap<i32>"
// CHECK-DAG: ![[#first_scope:]] = distinct !DILexicalBlock(scope: ![[#func_dbg]],
// CHECK: ![[#second_scope:]] = distinct !DILexicalBlock(scope: ![[#func_dbg]],
// CHECK: ![[#first_dbg]] = !DILocation(line: [[#]]
// CHECK-SAME: scope: ![[#first_scope]], inlinedAt: ![[#]])
// CHECK: ![[#second_dbg]] = !DILocation(line: [[#]]
// CHECK-SAME: scope: ![[#second_scope]], inlinedAt: ![[#]])

#![crate_type = "lib"]

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

x1 + y1
}

0 comments on commit 62ebe3a

Please sign in to comment.