Skip to content

Commit

Permalink
debuginfo: Make sure that all calls to drop glue are associated with …
Browse files Browse the repository at this point in the history
…debug locations.

This commit makes rustc emit debug locations for all call
and invoke statements in LLVM IR, if they are contained
within a function that debuginfo is enabled for. This is
important because LLVM does not handle the case where a
function body containing debuginfo is inlined into another
function with debuginfo, but the inlined call statement
does not have a debug location. In this case, LLVM will
not know where (in terms of source code coordinates) the
function was inlined to and we end up with some statements
still linked to the source locations in there original,
non-inlined function without any indication that they are
indeed an inline-copy. Later, when generating DWARF from
the IR, LLVM will interpret this as corrupt IR and abort.

Unfortunately, the undesirable case described above can
still occur when using LTO. If there is a crate compiled
without debuginfo calling into a crate compiled with
debuginfo, we again end up with the conditions triggering
the error. This is why some LTO tests still fail with the
dreaded assertion, if the standard library was built with
debuginfo enabled.
That is, `RUSTFLAGS_STAGE2=-g make rustc-stage2` will
succeed but `RUSTFLAGS_STAGE2=-g make check` will still
fail after this commit has been merged. This is a problem
that has to be dealt with separately.

Fixes #17201
Fixes #15816
Fixes #15156
  • Loading branch information
michaelwoerister committed Sep 25, 2014
1 parent d299baf commit 302486e
Show file tree
Hide file tree
Showing 10 changed files with 284 additions and 73 deletions.
25 changes: 18 additions & 7 deletions src/librustc/middle/trans/base.rs
Expand Up @@ -1791,7 +1791,7 @@ pub fn trans_closure(ccx: &CrateContext,
body: &ast::Block,
llfndecl: ValueRef,
param_substs: &param_substs,
id: ast::NodeId,
fn_ast_id: ast::NodeId,
_attributes: &[ast::Attribute],
arg_types: Vec<ty::t>,
output_type: ty::t,
Expand All @@ -1811,7 +1811,7 @@ pub fn trans_closure(ccx: &CrateContext,
let arena = TypedArena::new();
let fcx = new_fn_ctxt(ccx,
llfndecl,
id,
fn_ast_id,
has_env,
output_type,
param_substs,
Expand All @@ -1820,7 +1820,9 @@ pub fn trans_closure(ccx: &CrateContext,
let mut bcx = init_function(&fcx, false, output_type);

// cleanup scope for the incoming arguments
let arg_scope = fcx.push_custom_cleanup_scope();
let fn_cleanup_debug_loc =
debuginfo::get_cleanup_debug_loc_for_ast_node(fn_ast_id, body.span, true);
let arg_scope = fcx.push_custom_cleanup_scope_with_debug_loc(fn_cleanup_debug_loc);

let block_ty = node_id_type(bcx, body.id);

Expand Down Expand Up @@ -1969,7 +1971,9 @@ pub fn trans_named_tuple_constructor<'blk, 'tcx>(mut bcx: Block<'blk, 'tcx>,
ctor_ty: ty::t,
disr: ty::Disr,
args: callee::CallArgs,
dest: expr::Dest) -> Result<'blk, 'tcx> {
dest: expr::Dest,
call_info: Option<NodeInfo>)
-> Result<'blk, 'tcx> {

let ccx = bcx.fcx.ccx;
let tcx = ccx.tcx();
Expand Down Expand Up @@ -1999,8 +2003,13 @@ pub fn trans_named_tuple_constructor<'blk, 'tcx>(mut bcx: Block<'blk, 'tcx>,
match args {
callee::ArgExprs(exprs) => {
let fields = exprs.iter().map(|x| &**x).enumerate().collect::<Vec<_>>();
bcx = expr::trans_adt(bcx, result_ty, disr, fields.as_slice(),
None, expr::SaveIn(llresult));
bcx = expr::trans_adt(bcx,
result_ty,
disr,
fields.as_slice(),
None,
expr::SaveIn(llresult),
call_info);
}
_ => ccx.sess().bug("expected expr as arguments for variant/struct tuple constructor")
}
Expand All @@ -2010,7 +2019,9 @@ pub fn trans_named_tuple_constructor<'blk, 'tcx>(mut bcx: Block<'blk, 'tcx>,
// drop the temporary we made
let bcx = match dest {
expr::SaveIn(_) => bcx,
expr::Ignore => glue::drop_ty(bcx, llresult, result_ty)
expr::Ignore => {
glue::drop_ty(bcx, llresult, result_ty, call_info)
}
};

Result::new(bcx, llresult)
Expand Down
10 changes: 7 additions & 3 deletions src/librustc/middle/trans/callee.rs
Expand Up @@ -714,8 +714,12 @@ pub fn trans_call_inner<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
fcx.pop_custom_cleanup_scope(arg_cleanup_scope);

let ctor_ty = callee_ty.subst(bcx.tcx(), &substs);
return base::trans_named_tuple_constructor(bcx, ctor_ty, disr,
args, dest.unwrap());
return base::trans_named_tuple_constructor(bcx,
ctor_ty,
disr,
args,
dest.unwrap(),
call_info);
}
};

Expand Down Expand Up @@ -835,7 +839,7 @@ pub fn trans_call_inner<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
match (dest, opt_llretslot) {
(Some(expr::Ignore), Some(llretslot)) => {
// drop the value if it is not being saved.
bcx = glue::drop_ty(bcx, llretslot, ret_ty);
bcx = glue::drop_ty(bcx, llretslot, ret_ty, call_info);
call_lifetime_end(bcx, llretslot);
}
_ => {}
Expand Down
120 changes: 96 additions & 24 deletions src/librustc/middle/trans/cleanup.rs
Expand Up @@ -18,7 +18,8 @@ use middle::trans::base;
use middle::trans::build;
use middle::trans::callee;
use middle::trans::common;
use middle::trans::common::{Block, FunctionContext, ExprId};
use middle::trans::common::{Block, FunctionContext, ExprId, NodeInfo};
use middle::trans::debuginfo;
use middle::trans::glue;
use middle::trans::type_::Type;
use middle::ty;
Expand All @@ -36,6 +37,10 @@ pub struct CleanupScope<'blk, 'tcx: 'blk> {
// Cleanups to run upon scope exit.
cleanups: Vec<CleanupObj>,

// The debug location any drop calls generated for this scope will be
// associated with.
debug_loc: Option<NodeInfo>,

cached_early_exits: Vec<CachedEarlyExit>,
cached_landing_pad: Option<BasicBlockRef>,
}
Expand Down Expand Up @@ -69,7 +74,10 @@ pub struct CachedEarlyExit {
pub trait Cleanup {
fn must_unwind(&self) -> bool;
fn clean_on_unwind(&self) -> bool;
fn trans<'blk, 'tcx>(&self, bcx: Block<'blk, 'tcx>) -> Block<'blk, 'tcx>;
fn trans<'blk, 'tcx>(&self,
bcx: Block<'blk, 'tcx>,
debug_loc: Option<NodeInfo>)
-> Block<'blk, 'tcx>;
}

pub type CleanupObj = Box<Cleanup+'static>;
Expand All @@ -80,14 +88,14 @@ pub enum ScopeId {
}

impl<'blk, 'tcx> CleanupMethods<'blk, 'tcx> for FunctionContext<'blk, 'tcx> {
fn push_ast_cleanup_scope(&self, id: ast::NodeId) {
fn push_ast_cleanup_scope(&self, debug_loc: NodeInfo) {
/*!
* Invoked when we start to trans the code contained
* within a new cleanup scope.
*/

debug!("push_ast_cleanup_scope({})",
self.ccx.tcx().map.node_to_string(id));
self.ccx.tcx().map.node_to_string(debug_loc.id));

// FIXME(#2202) -- currently closure bodies have a parent
// region, which messes up the assertion below, since there
Expand All @@ -101,10 +109,15 @@ impl<'blk, 'tcx> CleanupMethods<'blk, 'tcx> for FunctionContext<'blk, 'tcx> {
// this new AST scope had better be its immediate child.
let top_scope = self.top_ast_scope();
if top_scope.is_some() {
assert_eq!(self.ccx.tcx().region_maps.opt_encl_scope(id), top_scope);
assert_eq!(self.ccx
.tcx()
.region_maps
.opt_encl_scope(debug_loc.id),
top_scope);
}

self.push_scope(CleanupScope::new(AstScopeKind(id)));
self.push_scope(CleanupScope::new(AstScopeKind(debug_loc.id),
Some(debug_loc)));
}

fn push_loop_cleanup_scope(&self,
Expand All @@ -114,13 +127,38 @@ impl<'blk, 'tcx> CleanupMethods<'blk, 'tcx> for FunctionContext<'blk, 'tcx> {
self.ccx.tcx().map.node_to_string(id));
assert_eq!(Some(id), self.top_ast_scope());

self.push_scope(CleanupScope::new(LoopScopeKind(id, exits)));
// Just copy the debuginfo source location from the enclosing scope
let debug_loc = self.scopes
.borrow()
.last()
.unwrap()
.debug_loc;

self.push_scope(CleanupScope::new(LoopScopeKind(id, exits), debug_loc));
}

fn push_custom_cleanup_scope(&self) -> CustomScopeIndex {
let index = self.scopes_len();
debug!("push_custom_cleanup_scope(): {}", index);
self.push_scope(CleanupScope::new(CustomScopeKind));

// Just copy the debuginfo source location from the enclosing scope
let debug_loc = self.scopes
.borrow()
.last()
.map(|opt_scope| opt_scope.debug_loc)
.unwrap_or(None);

self.push_scope(CleanupScope::new(CustomScopeKind, debug_loc));
CustomScopeIndex { index: index }
}

fn push_custom_cleanup_scope_with_debug_loc(&self,
debug_loc: NodeInfo)
-> CustomScopeIndex {
let index = self.scopes_len();
debug!("push_custom_cleanup_scope(): {}", index);

self.push_scope(CleanupScope::new(CustomScopeKind, Some(debug_loc)));
CustomScopeIndex { index: index }
}

Expand All @@ -141,7 +179,6 @@ impl<'blk, 'tcx> CleanupMethods<'blk, 'tcx> for FunctionContext<'blk, 'tcx> {

let scope = self.pop_scope();
self.trans_scope_cleanups(bcx, &scope)

}

fn pop_loop_cleanup_scope(&self,
Expand Down Expand Up @@ -175,9 +212,9 @@ impl<'blk, 'tcx> CleanupMethods<'blk, 'tcx> for FunctionContext<'blk, 'tcx> {
}

fn pop_and_trans_custom_cleanup_scope(&self,
bcx: Block<'blk, 'tcx>,
custom_scope: CustomScopeIndex)
-> Block<'blk, 'tcx> {
bcx: Block<'blk, 'tcx>,
custom_scope: CustomScopeIndex)
-> Block<'blk, 'tcx> {
/*!
* Removes the top cleanup scope from the stack, which must be
* a temporary scope, and generates the code to do its
Expand Down Expand Up @@ -503,7 +540,7 @@ impl<'blk, 'tcx> CleanupHelperMethods<'blk, 'tcx> for FunctionContext<'blk, 'tcx
let mut bcx = bcx;
if !bcx.unreachable.get() {
for cleanup in scope.cleanups.iter().rev() {
bcx = cleanup.trans(bcx);
bcx = cleanup.trans(bcx, scope.debug_loc);
}
}
bcx
Expand Down Expand Up @@ -671,7 +708,8 @@ impl<'blk, 'tcx> CleanupHelperMethods<'blk, 'tcx> for FunctionContext<'blk, 'tcx
let mut bcx_out = bcx_in;
for cleanup in scope.cleanups.iter().rev() {
if cleanup_is_suitable_for(&**cleanup, label) {
bcx_out = cleanup.trans(bcx_out);
bcx_out = cleanup.trans(bcx_out,
scope.debug_loc);
}
}
build::Br(bcx_out, prev_llbb);
Expand Down Expand Up @@ -785,9 +823,12 @@ impl<'blk, 'tcx> CleanupHelperMethods<'blk, 'tcx> for FunctionContext<'blk, 'tcx
}

impl<'blk, 'tcx> CleanupScope<'blk, 'tcx> {
fn new(kind: CleanupScopeKind<'blk, 'tcx>) -> CleanupScope<'blk, 'tcx> {
fn new(kind: CleanupScopeKind<'blk, 'tcx>,
debug_loc: Option<NodeInfo>)
-> CleanupScope<'blk, 'tcx> {
CleanupScope {
kind: kind,
debug_loc: debug_loc,
cleanups: vec!(),
cached_early_exits: vec!(),
cached_landing_pad: None,
Expand Down Expand Up @@ -902,11 +943,14 @@ impl Cleanup for DropValue {
self.must_unwind
}

fn trans<'blk, 'tcx>(&self, bcx: Block<'blk, 'tcx>) -> Block<'blk, 'tcx> {
fn trans<'blk, 'tcx>(&self,
bcx: Block<'blk, 'tcx>,
debug_loc: Option<NodeInfo>)
-> Block<'blk, 'tcx> {
let bcx = if self.is_immediate {
glue::drop_ty_immediate(bcx, self.val, self.ty)
glue::drop_ty_immediate(bcx, self.val, self.ty, debug_loc)
} else {
glue::drop_ty(bcx, self.val, self.ty)
glue::drop_ty(bcx, self.val, self.ty, debug_loc)
};
if self.zero {
base::zero_mem(bcx, self.val, self.ty);
Expand Down Expand Up @@ -935,7 +979,12 @@ impl Cleanup for FreeValue {
true
}

fn trans<'blk, 'tcx>(&self, bcx: Block<'blk, 'tcx>) -> Block<'blk, 'tcx> {
fn trans<'blk, 'tcx>(&self,
bcx: Block<'blk, 'tcx>,
debug_loc: Option<NodeInfo>)
-> Block<'blk, 'tcx> {
apply_debug_loc(bcx.fcx, debug_loc);

match self.heap {
HeapManaged => {
glue::trans_free(bcx, self.ptr)
Expand Down Expand Up @@ -963,7 +1012,12 @@ impl Cleanup for FreeSlice {
true
}

fn trans<'blk, 'tcx>(&self, bcx: Block<'blk, 'tcx>) -> Block<'blk, 'tcx> {
fn trans<'blk, 'tcx>(&self,
bcx: Block<'blk, 'tcx>,
debug_loc: Option<NodeInfo>)
-> Block<'blk, 'tcx> {
apply_debug_loc(bcx.fcx, debug_loc);

match self.heap {
HeapManaged => {
glue::trans_free(bcx, self.ptr)
Expand All @@ -988,7 +1042,11 @@ impl Cleanup for LifetimeEnd {
true
}

fn trans<'blk, 'tcx>(&self, bcx: Block<'blk, 'tcx>) -> Block<'blk, 'tcx> {
fn trans<'blk, 'tcx>(&self,
bcx: Block<'blk, 'tcx>,
debug_loc: Option<NodeInfo>)
-> Block<'blk, 'tcx> {
apply_debug_loc(bcx.fcx, debug_loc);
base::call_lifetime_end(bcx, self.ptr);
bcx
}
Expand Down Expand Up @@ -1023,15 +1081,29 @@ fn cleanup_is_suitable_for(c: &Cleanup,
!label.is_unwind() || c.clean_on_unwind()
}

fn apply_debug_loc(fcx: &FunctionContext, debug_loc: Option<NodeInfo>) {
match debug_loc {
Some(ref src_loc) => {
debuginfo::set_source_location(fcx, src_loc.id, src_loc.span);
}
None => {
debuginfo::clear_source_location(fcx);
}
}
}

///////////////////////////////////////////////////////////////////////////
// These traits just exist to put the methods into this file.

pub trait CleanupMethods<'blk, 'tcx> {
fn push_ast_cleanup_scope(&self, id: ast::NodeId);
fn push_ast_cleanup_scope(&self, id: NodeInfo);
fn push_loop_cleanup_scope(&self,
id: ast::NodeId,
exits: [Block<'blk, 'tcx>, ..EXIT_MAX]);
id: ast::NodeId,
exits: [Block<'blk, 'tcx>, ..EXIT_MAX]);
fn push_custom_cleanup_scope(&self) -> CustomScopeIndex;
fn push_custom_cleanup_scope_with_debug_loc(&self,
debug_loc: NodeInfo)
-> CustomScopeIndex;
fn pop_and_trans_ast_cleanup_scope(&self,
bcx: Block<'blk, 'tcx>,
cleanup_scope: ast::NodeId)
Expand Down
12 changes: 8 additions & 4 deletions src/librustc/middle/trans/controlflow.rs
Expand Up @@ -22,6 +22,7 @@ use middle::trans::cleanup;
use middle::trans::common::*;
use middle::trans::consts;
use middle::trans::datum;
use middle::trans::debuginfo;
use middle::trans::expr;
use middle::trans::meth;
use middle::trans::type_::Type;
Expand Down Expand Up @@ -53,7 +54,9 @@ pub fn trans_stmt<'blk, 'tcx>(cx: Block<'blk, 'tcx>,
let mut bcx = cx;

let id = ast_util::stmt_id(s);
fcx.push_ast_cleanup_scope(id);
let cleanup_debug_loc =
debuginfo::get_cleanup_debug_loc_for_ast_node(id, s.span, false);
fcx.push_ast_cleanup_scope(cleanup_debug_loc);

match s.node {
ast::StmtExpr(ref e, _) | ast::StmtSemi(ref e, _) => {
Expand All @@ -75,8 +78,7 @@ pub fn trans_stmt<'blk, 'tcx>(cx: Block<'blk, 'tcx>,
ast::StmtMac(..) => cx.tcx().sess.bug("unexpanded macro")
}

bcx = fcx.pop_and_trans_ast_cleanup_scope(
bcx, ast_util::stmt_id(s));
bcx = fcx.pop_and_trans_ast_cleanup_scope(bcx, ast_util::stmt_id(s));

return bcx;
}
Expand All @@ -100,7 +102,9 @@ pub fn trans_block<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
let fcx = bcx.fcx;
let mut bcx = bcx;

fcx.push_ast_cleanup_scope(b.id);
let cleanup_debug_loc =
debuginfo::get_cleanup_debug_loc_for_ast_node(b.id, b.span, true);
fcx.push_ast_cleanup_scope(cleanup_debug_loc);

for s in b.stmts.iter() {
bcx = trans_stmt(bcx, &**s);
Expand Down

5 comments on commit 302486e

@bors
Copy link
Contributor

@bors bors commented on 302486e Sep 29, 2014

Choose a reason for hiding this comment

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

saw approval from luqmana
at michaelwoerister@302486e

@bors
Copy link
Contributor

@bors bors commented on 302486e Sep 29, 2014

Choose a reason for hiding this comment

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

merging michaelwoerister/rust/function-call-locs = 302486e into auto

@bors
Copy link
Contributor

@bors bors commented on 302486e Sep 29, 2014

Choose a reason for hiding this comment

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

michaelwoerister/rust/function-call-locs = 302486e merged ok, testing candidate = 496b68d

@bors
Copy link
Contributor

@bors bors commented on 302486e Sep 29, 2014

Choose a reason for hiding this comment

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

@bors
Copy link
Contributor

@bors bors commented on 302486e Sep 29, 2014

Choose a reason for hiding this comment

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

fast-forwarding master to auto = 496b68d

Please sign in to comment.