Skip to content

rustc_codegen_ssa: only "spill" SSA-like values to the stack for debuginfo. #68961

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

Merged
merged 2 commits into from
Feb 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 0 additions & 19 deletions src/librustc_codegen_ssa/mir/analyze.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ use rustc::mir::visit::{
MutatingUseContext, NonMutatingUseContext, NonUseContext, PlaceContext, Visitor,
};
use rustc::mir::{self, Location, TerminatorKind};
use rustc::session::config::DebugInfo;
use rustc::ty;
use rustc::ty::layout::{HasTyCtxt, LayoutOf};
use rustc_data_structures::graph::dominators::Dominators;
Expand All @@ -24,15 +23,6 @@ pub fn non_ssa_locals<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
analyzer.visit_body(mir);

for (local, decl) in mir.local_decls.iter_enumerated() {
// FIXME(eddyb): We should figure out how to use llvm.dbg.value instead
// of putting everything in allocas just so we can use llvm.dbg.declare.
if fx.cx.sess().opts.debuginfo == DebugInfo::Full {
if fx.mir.local_kind(local) == mir::LocalKind::Arg {
analyzer.not_ssa(local);
continue;
}
}

let ty = fx.monomorphize(&decl.ty);
debug!("local {:?} has type `{}`", local, ty);
let layout = fx.cx.spanned_layout_of(ty, decl.source_info.span);
Expand Down Expand Up @@ -281,15 +271,6 @@ impl<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> Visitor<'tcx>
self.assign(local, location);
}

PlaceContext::NonUse(NonUseContext::VarDebugInfo) => {
// We need to keep locals in `alloca`s for debuginfo.
// FIXME(eddyb): We should figure out how to use `llvm.dbg.value` instead
// of putting everything in allocas just so we can use `llvm.dbg.declare`.
if self.fx.cx.sess().opts.debuginfo == DebugInfo::Full {
self.not_ssa(local);
}
}

PlaceContext::NonUse(_) | PlaceContext::MutatingUse(MutatingUseContext::Retag) => {}

PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy)
Expand Down
3 changes: 3 additions & 0 deletions src/librustc_codegen_ssa/mir/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1145,6 +1145,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let op = bx.load_operand(place);
place.storage_dead(bx);
self.locals[index] = LocalRef::Operand(Some(op));
self.debug_introduce_local(bx, index);
}
LocalRef::Operand(Some(op)) => {
assert!(op.layout.is_zst(), "assigning to initialized SSAtemp");
Expand Down Expand Up @@ -1186,6 +1187,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let op = bx.load_operand(tmp);
tmp.storage_dead(bx);
self.locals[index] = LocalRef::Operand(Some(op));
self.debug_introduce_local(bx, index);
}
DirectOperand(index) => {
// If there is a cast, we have to store and reload.
Expand All @@ -1200,6 +1202,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
OperandRef::from_immediate_or_packed_pair(bx, llval, ret_abi.layout)
};
self.locals[index] = LocalRef::Operand(Some(op));
self.debug_introduce_local(bx, index);
}
}
}
Expand Down
47 changes: 34 additions & 13 deletions src/librustc_codegen_ssa/mir/debuginfo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ use rustc_index::vec::IndexVec;
use rustc_span::symbol::{kw, Symbol};
use rustc_span::{BytePos, Span};

use super::OperandValue;
use super::operand::OperandValue;
use super::place::PlaceRef;
use super::{FunctionCx, LocalRef};

pub struct FunctionDebugContext<D> {
Expand Down Expand Up @@ -111,8 +112,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {

/// Apply debuginfo and/or name, after creating the `alloca` for a local,
/// or initializing the local with an operand (whichever applies).
// FIXME(eddyb) use `llvm.dbg.value` (which would work for operands),
// not just `llvm.dbg.declare` (which requires `alloca`).
pub fn debug_introduce_local(&self, bx: &mut Bx, local: mir::Local) {
let full_debug_info = bx.sess().opts.debuginfo == DebugInfo::Full;

Expand Down Expand Up @@ -180,38 +179,60 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {

let local_ref = &self.locals[local];

if !bx.sess().fewer_names() {
let name = match whole_local_var.or(fallback_var) {
let name = if bx.sess().fewer_names() {
None
} else {
Some(match whole_local_var.or(fallback_var) {
Some(var) if var.name != kw::Invalid => var.name.to_string(),
_ => format!("{:?}", local),
};
})
};

if let Some(name) = &name {
match local_ref {
LocalRef::Place(place) | LocalRef::UnsizedPlace(place) => {
bx.set_var_name(place.llval, &name);
bx.set_var_name(place.llval, name);
}
LocalRef::Operand(Some(operand)) => match operand.val {
OperandValue::Ref(x, ..) | OperandValue::Immediate(x) => {
bx.set_var_name(x, &name);
bx.set_var_name(x, name);
}
OperandValue::Pair(a, b) => {
// FIXME(eddyb) these are scalar components,
// maybe extract the high-level fields?
bx.set_var_name(a, &(name.clone() + ".0"));
bx.set_var_name(b, &(name + ".1"));
bx.set_var_name(b, &(name.clone() + ".1"));
}
},
LocalRef::Operand(None) => {}
}
}

if !full_debug_info {
if !full_debug_info || vars.is_empty() && fallback_var.is_none() {
return;
}

// FIXME(eddyb) add debuginfo for unsized places too.
let base = match local_ref {
LocalRef::Place(place) => place,
_ => return,
LocalRef::Operand(None) => return,

LocalRef::Operand(Some(operand)) => {
// "Spill" the value onto the stack, for debuginfo,
// without forcing non-debuginfo uses of the local
// to also load from the stack every single time.
// FIXME(#68817) use `llvm.dbg.value` instead,
// at least for the cases which LLVM handles correctly.
let spill_slot = PlaceRef::alloca(bx, operand.layout);
if let Some(name) = name {
bx.set_var_name(spill_slot.llval, &(name + ".dbg.spill"));
}
operand.val.store(bx, spill_slot);
spill_slot
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this code triggered anywhere? Do we have a test for related behaviours? In particular that the value and name as seen by debugger is right and remains in scope for as long as necessary (i.e. the problem that appears to be preventing our use to llvm-dbg.value)

Might be worth adding a couple now if they don’t yet exist so that we catch issues when we move to llvm.dbg.value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the debuginfo tests fail with llvm.dbg.value, which is how I found all the issues with it.

A common pattern with debuginfo tests is that they breakpoint at the end of a function, so all the variables are as close to the end of their scope as possible, stressing even llvm.dbg.value+inline asm (which only gives you the value if you breakpoint earlier).

}

LocalRef::Place(place) => *place,

// FIXME(eddyb) add debuginfo for unsized places too.
LocalRef::UnsizedPlace(_) => return,
};

let vars = vars.iter().copied().chain(fallback_var);
Expand Down
8 changes: 3 additions & 5 deletions src/test/codegen/naked-functions.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
// ignore-tidy-linelength

// compile-flags: -C no-prepopulate-passes

#![crate_type = "lib"]
Expand Down Expand Up @@ -61,19 +59,19 @@ pub fn naked_recursive() {

naked_empty();

// CHECK-NEXT: %{{[0-9]+}} = call i{{[0-9]+}} @naked_with_return()
// CHECK-NEXT: %_4 = call i{{[0-9]+}} @naked_with_return()

// FIXME(#39685) Avoid one block per call.
// CHECK-NEXT: br label %bb2
// CHECK: bb2:

// CHECK-NEXT: %{{[0-9]+}} = call i{{[0-9]+}} @naked_with_args_and_return(i{{[0-9]+}} %{{[0-9]+}})
// CHECK-NEXT: %_3 = call i{{[0-9]+}} @naked_with_args_and_return(i{{[0-9]+}} %_4)

// FIXME(#39685) Avoid one block per call.
// CHECK-NEXT: br label %bb3
// CHECK: bb3:

// CHECK-NEXT: call void @naked_with_args(i{{[0-9]+}} %{{[0-9]+}})
// CHECK-NEXT: call void @naked_with_args(i{{[0-9]+}} %_3)

// FIXME(#39685) Avoid one block per call.
// CHECK-NEXT: br label %bb4
Expand Down
2 changes: 1 addition & 1 deletion src/test/debuginfo/issue-12886.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

// gdb-command:run
// gdb-command:next
// gdb-check:[...]25[...]s
// gdb-check:[...]24[...]let s = Some(5).unwrap(); // #break
// gdb-command:continue

#![feature(omit_gdb_pretty_printer_section)]
Expand Down