From 85deeeab59e7b5845590b40d8a430b51580f1428 Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Tue, 1 Oct 2013 12:24:50 +0200 Subject: [PATCH] debuginfo: Unified namespace generation approach for crate-local and external items. Fixed bug related to LLVM metadata uniquing. --- src/librustc/lib/llvm.rs | 3 +- src/librustc/middle/trans/base.rs | 5 - src/librustc/middle/trans/debuginfo.rs | 304 ++++++++----------------- src/rustllvm/RustWrapper.cpp | 19 +- 4 files changed, 109 insertions(+), 222 deletions(-) diff --git a/src/librustc/lib/llvm.rs b/src/librustc/lib/llvm.rs index 6fe8d65cc39d9..1c56f699c130d 100644 --- a/src/librustc/lib/llvm.rs +++ b/src/librustc/lib/llvm.rs @@ -1968,7 +1968,8 @@ pub mod llvm { DerivedFrom: DIType, Elements: DIArray, RunTimeLang: c_uint, - VTableHolder: ValueRef) + VTableHolder: ValueRef, + UniqueId: *c_char) -> DICompositeType; #[fast_ffi] diff --git a/src/librustc/middle/trans/base.rs b/src/librustc/middle/trans/base.rs index 73b197ec36199..9b6db062233a5 100644 --- a/src/librustc/middle/trans/base.rs +++ b/src/librustc/middle/trans/base.rs @@ -3119,11 +3119,6 @@ pub fn trans_crate(sess: session::Session, symbol_hasher, link_meta, analysis.reachable); - - if ccx.sess.opts.debuginfo { - debuginfo::initialize(ccx, &crate); - } - { let _icx = push_ctxt("text"); trans_mod(ccx, &crate.module); diff --git a/src/librustc/middle/trans/debuginfo.rs b/src/librustc/middle/trans/debuginfo.rs index 52695100b3ee7..a982a4767fdcf 100644 --- a/src/librustc/middle/trans/debuginfo.rs +++ b/src/librustc/middle/trans/debuginfo.rs @@ -104,11 +104,13 @@ use util::ppaux; use std::c_str::ToCStr; use std::hashmap::HashMap; +use std::hashmap::HashSet; use std::libc::{c_uint, c_ulonglong, c_longlong}; use std::ptr; +use std::unstable::atomics; use std::vec; use syntax::codemap::{Span, Pos}; -use syntax::{ast, codemap, ast_util, ast_map, opt_vec, visit}; +use syntax::{ast, codemap, ast_util, ast_map, opt_vec}; use syntax::parse::token; use syntax::parse::token::special_idents; @@ -136,8 +138,10 @@ pub struct CrateDebugContext { priv current_debug_location: DebugLocation, priv created_files: HashMap<~str, DIFile>, priv created_types: HashMap, - priv local_namespace_map: HashMap, - priv extern_namespaces: HashMap<~[ast::Ident], @NamespaceTreeNode>, + priv namespace_map: HashMap<~[ast::Ident], @NamespaceTreeNode>, + // This collection is used to assert that composite types (structs, enums, ...) have their + // members only set once: + priv composite_types_completed: HashSet, } impl CrateDebugContext { @@ -153,8 +157,8 @@ impl CrateDebugContext { current_debug_location: UnknownLocation, created_files: HashMap::new(), created_types: HashMap::new(), - local_namespace_map: HashMap::new(), - extern_namespaces: HashMap::new(), + namespace_map: HashMap::new(), + composite_types_completed: HashSet::new(), }; } } @@ -224,16 +228,6 @@ enum VariableKind { CapturedVariable, } -pub fn initialize(cx: &mut CrateContext, crate: &ast::Crate) { - if cx.dbg_cx.is_none() { - return; - } - - let crate_namespace_ident = token::str_to_ident(cx.link_meta.name); - let mut visitor = NamespaceVisitor::new_crate_visitor(cx, crate_namespace_ident); - visit::walk_crate(&mut visitor, crate, ()); -} - /// Create any deferred debug metadata nodes pub fn finalize(cx: @mut CrateContext) { if cx.dbg_cx.is_none() { @@ -548,11 +542,11 @@ pub fn create_function_debug_context(cx: &mut CrateContext, let empty_generics = ast::Generics { lifetimes: opt_vec::Empty, ty_params: opt_vec::Empty }; let fnitem = cx.tcx.items.get_copy(&fn_ast_id); - let (ident, fn_decl, generics, top_level_block, span) = match fnitem { + let (ident, fn_decl, generics, top_level_block, span, has_path) = match fnitem { ast_map::node_item(ref item, _) => { match item.node { ast::item_fn(ref fn_decl, _, _, ref generics, ref top_level_block) => { - (item.ident, fn_decl, generics, top_level_block, item.span) + (item.ident, fn_decl, generics, top_level_block, item.span, true) } _ => { cx.sess.span_bug(item.span, @@ -571,7 +565,7 @@ pub fn create_function_debug_context(cx: &mut CrateContext, }, _, _) => { - (ident, fn_decl, generics, top_level_block, span) + (ident, fn_decl, generics, top_level_block, span, true) } ast_map::node_expr(ref expr) => { match expr.node { @@ -583,7 +577,9 @@ pub fn create_function_debug_context(cx: &mut CrateContext, // enclosing function. &empty_generics, top_level_block, - expr.span) + expr.span, + // Don't try to lookup the item path: + false) } _ => cx.sess.span_bug(expr.span, "create_function_debug_context: expected an expr_fn_block here") @@ -601,7 +597,7 @@ pub fn create_function_debug_context(cx: &mut CrateContext, }), _, _) => { - (ident, fn_decl, generics, top_level_block, span) + (ident, fn_decl, generics, top_level_block, span, true) } ast_map::node_foreign_item(@ast::foreign_item { _ }, _, _, _) | ast_map::node_variant(*) | @@ -633,18 +629,16 @@ pub fn create_function_debug_context(cx: &mut CrateContext, file_metadata, &mut function_name); - let namespace_node = debug_context(cx).local_namespace_map.find_copy(&fn_ast_id); - let (linkage_name, containing_scope) = match namespace_node { - Some(namespace_node) => { - (namespace_node.mangled_name_of_contained_item(function_name), namespace_node.scope) - } - None => { - // This branch is only hit when there is a bug in the NamespaceVisitor. - cx.sess.span_warn(span, format!("debuginfo: Could not find namespace node for function - with name {}. This is a bug! Please report this to - github.com/mozilla/rust/issues", function_name)); - (function_name.clone(), file_metadata) - } + // There is no ast_map::path for ast::ExprFnBlock-type functions. For now, just don't put them + // into a namespace. In the future this could be improved somehow (storing a path in the + // ast_map, or construct a path using the enclosing function). + let (linkage_name, containing_scope) = if has_path { + let namespace_node = namespace_for_item(cx, ast_util::local_def(fn_ast_id), span); + let linkage_name = namespace_node.mangled_name_of_contained_item(function_name); + let containing_scope = namespace_node.scope; + (linkage_name, containing_scope) + } else { + (function_name.clone(), file_metadata) }; let scope_line = get_scope_line(cx, top_level_block, loc.line); @@ -682,16 +676,6 @@ pub fn create_function_debug_context(cx: &mut CrateContext, let arg_pats = do fn_decl.inputs.map |arg_ref| { arg_ref.pat }; populate_scope_map(cx, arg_pats, top_level_block, fn_metadata, &mut fn_debug_context.scope_map); - // Create namespaces for the interior of this function - { - let mut namespace_visitor = NamespaceVisitor::new_function_visitor(cx, - function_name, - namespace_node, - file_metadata, - span); - visit::walk_block(&mut namespace_visitor, top_level_block, ()); - } - return FunctionDebugContext(fn_debug_context); fn get_function_signature(cx: &mut CrateContext, @@ -1584,6 +1568,17 @@ fn set_members_of_composite_type(cx: &mut CrateContext, member_descriptions: &[MemberDescription], file_metadata: DIFile, definition_span: Span) { + // In some rare cases LLVM metadata uniquing would lead to an existing type description being + // used instead of a new one created in create_struct_stub. This would cause a hard to trace + // assertion in DICompositeType::SetTypeArray(). The following check makes sure that we get a + // better error message if this should happen again due to some regression. + if debug_context(cx).composite_types_completed.contains(&composite_type_metadata) { + cx.sess.span_bug(definition_span, "debuginfo::set_members_of_composite_type() - Already \ + completed forward declaration re-encountered."); + } else { + debug_context(cx).composite_types_completed.insert(composite_type_metadata); + } + let loc = span_start(cx, definition_span); let member_metadata: ~[DIDescriptor] = member_descriptions @@ -1632,8 +1627,16 @@ fn create_struct_stub(cx: &mut CrateContext, let loc = span_start(cx, definition_span); let (struct_size, struct_align) = size_and_align_of(cx, struct_llvm_type); - return do struct_type_name.with_c_str |name| { - unsafe { + // We assign unique IDs to the type stubs so LLVM metadata uniquing does not reuse instances + // where we don't want it. + let unique_id = unsafe { + static mut unique_id_counter: atomics::AtomicUint = atomics::INIT_ATOMIC_UINT; + format!("DiStructStub{}", unique_id_counter.fetch_add(1, atomics::SeqCst)) + }; + + return unsafe { + do struct_type_name.with_c_str |name| { + do unique_id.with_c_str |unique_id| { // LLVMDIBuilderCreateStructType() wants an empty array. A null pointer will lead to // hard to trace and debug LLVM assertions later on in llvm/lib/IR/Value.cpp let empty_array = create_DIArray(DIB(cx), []); @@ -1650,8 +1653,9 @@ fn create_struct_stub(cx: &mut CrateContext, ptr::null(), empty_array, 0, - ptr::null()) - }}; + ptr::null(), + unique_id) + }}}; } fn boxed_type_metadata(cx: &mut CrateContext, @@ -2199,8 +2203,8 @@ fn get_namespace_and_span_for_item(cx: &mut CrateContext, def_id: ast::DefId, warning_span: Span) -> (DIScope, Span) { - if def_id.crate == ast::LOCAL_CRATE { - let containing_scope = debug_context(cx).local_namespace_map.get_copy(&def_id.node).scope; + let containing_scope = namespace_for_item(cx, def_id, warning_span).scope; + let definition_span = if def_id.crate == ast::LOCAL_CRATE { let definition_span = match cx.tcx.items.find(&def_id.node) { Some(&ast_map::node_item(@ast::item { span, _ }, _)) => span, ref node => { @@ -2210,12 +2214,13 @@ fn get_namespace_and_span_for_item(cx: &mut CrateContext, codemap::dummy_sp() } }; - (containing_scope, definition_span) + definition_span } else { - let item_path = ty::item_path(cx.tcx, def_id); // For external items there is no span information - (namespace_for_external_item(cx, &item_path).scope, codemap::dummy_sp()) - } + codemap::dummy_sp() + }; + + (containing_scope, definition_span) } // This procedure builds the *scope map* for a given function, which maps any given ast::NodeId in @@ -2703,29 +2708,41 @@ impl NamespaceTreeNode { } } -fn namespace_for_external_item(cx: &mut CrateContext, - item_path: &ast_map::path) - -> @NamespaceTreeNode { - if item_path.len() < 2 { - cx.sess.bug(format!("debuginfo::namespace_for_external_item() - Invalid item_path: {}", - ast_map::path_to_str(*item_path, token::get_ident_interner()))); - } +fn namespace_for_item(cx: &mut CrateContext, + def_id: ast::DefId, + warning_span: Span) + -> @NamespaceTreeNode { + let namespace_path = { + let mut item_path = ty::item_path(cx.tcx, def_id); - let path_excluding_item = item_path.slice_to(item_path.len() - 1); - let mut current_key = vec::with_capacity(path_excluding_item.len()); - let mut parent_node: Option<@NamespaceTreeNode> = None; - let last_index = path_excluding_item.len() - 1; + if (def_id.crate == ast::LOCAL_CRATE && item_path.len() < 1) || + (def_id.crate != ast::LOCAL_CRATE && item_path.len() < 2) { + cx.sess.bug(format!("debuginfo::namespace_for_item() - Item path too short: {}", + ast_map::path_to_str(item_path, token::get_ident_interner()))); + } - for (i, &path_element) in path_excluding_item.iter().enumerate() { - let ident = match path_element { - ast_map::path_mod(ident) | - ast_map::path_name(ident) | - ast_map::path_pretty_name(ident, _) => ident - }; + // remove the name of the item + item_path.pop(); + + if def_id.crate == ast::LOCAL_CRATE { + // prepend crate name if not already present + let crate_namespace_ident = token::str_to_ident(cx.link_meta.name); + item_path.insert(0, ast_map::path_mod(crate_namespace_ident)); + } + item_path + }; + + let mut current_key = vec::with_capacity(namespace_path.len()); + let mut parent_node: Option<@NamespaceTreeNode> = None; + let last_index = namespace_path.len() - 1; + + // Create/Lookup namespace for each element of the path. + for (i, &path_element) in namespace_path.iter().enumerate() { + let ident = path_element.ident(); current_key.push(ident); - let existing_node = debug_context(cx).extern_namespaces.find_copy(¤t_key); + let existing_node = debug_context(cx).namespace_map.find_copy(¤t_key); let current_node = match existing_node { Some(existing_node) => existing_node, None => { @@ -2743,7 +2760,7 @@ fn namespace_for_external_item(cx: &mut CrateContext, parent_scope, namespace_name, ptr::null(), // cannot reconstruct file ... - 0) // ... or line information + 0) // ... or line information, but that's not so important. } }; @@ -2753,7 +2770,7 @@ fn namespace_for_external_item(cx: &mut CrateContext, parent: parent_node, }; - debug_context(cx).extern_namespaces.insert(current_key.clone(), node); + debug_context(cx).namespace_map.insert(current_key.clone(), node); node } @@ -2766,142 +2783,9 @@ fn namespace_for_external_item(cx: &mut CrateContext, } } - cx.sess.bug("debuginfo::namespace_for_external_item() - Code path should be unreachable"); -} - -struct NamespaceVisitor<'self> { - module_ident: ast::Ident, - scope_stack: ~[@NamespaceTreeNode], - crate_context: &'self mut CrateContext, -} - -impl<'self> NamespaceVisitor<'self> { - - fn new_crate_visitor<'a>(cx: &'a mut CrateContext, - crate_ident: ast::Ident) - -> NamespaceVisitor<'a> { - NamespaceVisitor { - module_ident: crate_ident, - scope_stack: ~[], - crate_context: cx, - } - } - - fn new_function_visitor<'a>(cx: &'a mut CrateContext, - function_name: &str, - parent_node: Option<@NamespaceTreeNode>, - file_metadata: DIFile, - span: Span) - -> NamespaceVisitor<'a> { - let companion_name = function_name + "()"; - let companion_ident = token::str_to_ident(companion_name); - let parent_scope = match parent_node { - Some(parent_node) => parent_node.scope, - None => ptr::null() - }; - let line = span_start(cx, span).line as c_uint; - - let namespace_metadata = unsafe { - do companion_name.with_c_str |companion_name| { - llvm::LLVMDIBuilderCreateNameSpace( - DIB(cx), - parent_scope, - companion_name, - file_metadata, - line) - } - }; - - let function_node = @NamespaceTreeNode { - scope: namespace_metadata, - ident: companion_ident, - parent: parent_node, - }; - - return NamespaceVisitor { - module_ident: special_idents::invalid, - scope_stack: ~[function_node], - crate_context: cx, - }; - } -} - -// Possible optimization: Only recurse if needed. -impl<'self> visit::Visitor<()> for NamespaceVisitor<'self> { - - fn visit_mod(&mut self, - module: &ast::_mod, - span: Span, - _: ast::NodeId, - _: ()) { - let module_name = token::ident_to_str(&self.module_ident); - - let (parent_node, parent_scope) = if self.scope_stack.len() > 0 { - let parent_node = *self.scope_stack.last(); - (Some(parent_node), parent_node.scope) - } else { - (None, ptr::null()) - }; - - let loc = span_start(self.crate_context, span); - let file_metadata = file_metadata(self.crate_context, loc.file.name); - - let namespace_metadata = unsafe { - do module_name.with_c_str |module_name| { - llvm::LLVMDIBuilderCreateNameSpace( - DIB(self.crate_context), - parent_scope, - module_name, - file_metadata, - loc.line as c_uint) - } - }; - - let this_node = @NamespaceTreeNode { - scope: namespace_metadata, - ident: self.module_ident, - parent: parent_node, - }; - - self.scope_stack.push(this_node); - - visit::walk_mod(self, module, ()); - - self.scope_stack.pop(); - } - - fn visit_item(&mut self, item: @ast::item, _: ()) { - match item.node { - ast::item_mod(*) => { - // always store the last module ident so visit_mod() has it available - self.module_ident = item.ident; - } - ast::item_fn(*) => { /* handled by visit_fn */ } - _ => { - debug_context(self.crate_context) - .local_namespace_map - .insert(item.id, *self.scope_stack.last()); - } - } - - visit::walk_item(self, item, ()); - } - - fn visit_foreign_item(&mut self, item: @ast::foreign_item, _: ()) { - debug_context(self.crate_context) - .local_namespace_map - .insert(item.id, *self.scope_stack.last()); - } - - fn visit_fn(&mut self, - _: &visit::fn_kind, - _: &ast::fn_decl, - _: &ast::Block, - _: Span, - node_id: ast::NodeId, - _: ()) { - debug_context(self.crate_context) - .local_namespace_map - .insert(node_id, *self.scope_stack.last()); - } + // Should be unreachable: + let error_message = format!("debuginfo::namespace_for_item() - Code path should be \ + unreachable. namespace_path was {}", + ast_map::path_to_str(namespace_path, token::get_ident_interner())); + cx.sess.span_bug(warning_span, error_message); } diff --git a/src/rustllvm/RustWrapper.cpp b/src/rustllvm/RustWrapper.cpp index 63d42816207cd..31a02dceb1c09 100644 --- a/src/rustllvm/RustWrapper.cpp +++ b/src/rustllvm/RustWrapper.cpp @@ -548,14 +548,21 @@ extern "C" LLVMValueRef LLVMDIBuilderCreateStructType( LLVMValueRef DerivedFrom, LLVMValueRef Elements, unsigned RunTimeLang, - LLVMValueRef VTableHolder) { + LLVMValueRef VTableHolder, + const char *UniqueId) { return wrap(Builder->createStructType( - unwrapDI(Scope), Name, - unwrapDI(File), LineNumber, - SizeInBits, AlignInBits, Flags, + unwrapDI(Scope), + Name, + unwrapDI(File), + LineNumber, + SizeInBits, + AlignInBits, + Flags, unwrapDI(DerivedFrom), - unwrapDI(Elements), RunTimeLang, - unwrapDI(VTableHolder))); + unwrapDI(Elements), + RunTimeLang, + unwrapDI(VTableHolder), + UniqueId)); } extern "C" LLVMValueRef LLVMDIBuilderCreateMemberType(