From 512069f8e51c975aac6bab662d9fccbf019d2a27 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 26 Mar 2019 15:30:41 -0400 Subject: [PATCH 1/6] Fix stack overflow when generating debuginfo for 'recursive' type By using 'impl trait', it's possible to create a self-referential type as follows: fn foo() -> impl Copy { foo } This is a function which returns itself. Normally, the signature of this function would be impossible to write - it would look like 'fn foo() -> fn() -> fn() ...' e.g. a function which returns a function, which returns a function... Using 'impl trait' allows us to avoid writing this infinitely long type. While it's useless for practical purposes, it does compile and run However, issues arise when we try to generate llvm debuginfo for such a type. All 'impl trait' types (e.g. ty::Opaque) are resolved when we generate debuginfo, which can lead to us recursing back to the original 'fn' type when we try to process its return type. To resolve this, I've modified debuginfo generation to account for these kinds of weird types. Unfortunately, there's no 'correct' debuginfo that we can generate - 'impl trait' does not exist in debuginfo, and this kind of recursive type is impossible to directly represent. To ensure that we emit *something*, this commit emits dummy debuginfo/type names whenever it encounters a self-reference. In practice, this should never happen - it's just to ensure that we can emit some kind of debuginfo, even if it's not particularly meaningful Fixes #58463 --- .../debuginfo/metadata.rs | 68 +++++++++++++++++-- .../debuginfo/type_names.rs | 49 +++++++++---- src/test/run-pass/issues/issue-58463.rs | 8 +++ 3 files changed, 108 insertions(+), 17 deletions(-) create mode 100644 src/test/run-pass/issues/issue-58463.rs diff --git a/src/librustc_codegen_llvm/debuginfo/metadata.rs b/src/librustc_codegen_llvm/debuginfo/metadata.rs index e549b120da979..3d742811ff049 100644 --- a/src/librustc_codegen_llvm/debuginfo/metadata.rs +++ b/src/librustc_codegen_llvm/debuginfo/metadata.rs @@ -117,6 +117,32 @@ impl TypeMap<'ll, 'tcx> { } } + // Removes a Ty to metadata mapping + // This is useful when computing the metadata for a potentially + // recursive type (e.g. a function ptr of the form: + // + // fn foo() -> impl Copy { foo } + // + // This kind of type cannot be properly represented + // via LLVM debuginfo. As a workaround, + // we register a temporary Ty to metadata mapping + // for the function before we compute its actual metadat.a + // If the metadata computation ends up recursing back to the + // original function, it will use the temporary mapping + // for the inner self-reference, preventing us from + // recursing forever. + // + // This function is used to remove the temporary metadata + // mapping after we've computed the actual metadat + fn remove_type( + &mut self, + type_: Ty<'tcx>, + ) { + if self.type_to_metadata.remove(type_).is_none() { + bug!("Type metadata Ty '{}' is not in the TypeMap!", type_); + } + } + // Adds a UniqueTypeId to metadata mapping to the TypeMap. The method will // fail if the mapping already exists. fn register_unique_id_with_metadata( @@ -608,10 +634,7 @@ pub fn type_metadata( } } ty::FnDef(..) | ty::FnPtr(_) => { - let fn_metadata = subroutine_type_metadata(cx, - unique_type_id, - t.fn_sig(cx.tcx), - usage_site_span).metadata; + if let Some(metadata) = debug_context(cx).type_map .borrow() .find_metadata_for_unique_id(unique_type_id) @@ -619,6 +642,43 @@ pub fn type_metadata( return metadata; } + // It's possible to create a self-referential + // type in Rust by using 'impl trait': + // + // fn foo() -> impl Copy { foo } + // + // See TypeMap::remove_type for more detals + // about the workaround + + let temp_type = { + unsafe { + // The choice of type here is pretty arbitrary - + // anything reading the debuginfo for a recursive + // type is going to see *somthing* weird - the only + // question is what exactly it will see + let (size, align) = cx.size_and_align_of(t); + llvm::LLVMRustDIBuilderCreateBasicType( + DIB(cx), + SmallCStr::new("").as_ptr(), + size.bits(), + align.bits() as u32, + DW_ATE_unsigned) + + + } + }; + + let type_map = &debug_context(cx).type_map; + type_map.borrow_mut().register_type_with_metadata(t, temp_type); + + let fn_metadata = subroutine_type_metadata(cx, + unique_type_id, + t.fn_sig(cx.tcx), + usage_site_span).metadata; + + type_map.borrow_mut().remove_type(t); + + // This is actually a function pointer, so wrap it in pointer DI MetadataCreationResult::new(pointer_type_metadata(cx, t, fn_metadata), false) diff --git a/src/librustc_codegen_llvm/debuginfo/type_names.rs b/src/librustc_codegen_llvm/debuginfo/type_names.rs index 8b218ab39d99b..024840a3fa8ad 100644 --- a/src/librustc_codegen_llvm/debuginfo/type_names.rs +++ b/src/librustc_codegen_llvm/debuginfo/type_names.rs @@ -5,6 +5,7 @@ use rustc::hir::def_id::DefId; use rustc::ty::subst::SubstsRef; use rustc::ty::{self, Ty}; use rustc_codegen_ssa::traits::*; +use rustc_data_structures::fx::FxHashSet; use rustc::hir; @@ -17,7 +18,8 @@ pub fn compute_debuginfo_type_name<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, qualified: bool) -> String { let mut result = String::with_capacity(64); - push_debuginfo_type_name(cx, t, qualified, &mut result); + let mut visited = FxHashSet::default(); + push_debuginfo_type_name(cx, t, qualified, &mut result, &mut visited); result } @@ -26,7 +28,27 @@ pub fn compute_debuginfo_type_name<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, pub fn push_debuginfo_type_name<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, t: Ty<'tcx>, qualified: bool, - output: &mut String) { + output: &mut String, + visited: &mut FxHashSet>) { + + // We've encountered a weird 'recursive type' + // Currently, the only way to generate such a type + // is by using 'impl trait': + // + // fn foo() -> impl Copy { foo } + // + // There's not really a sensible name we can generate, + // since we don't include 'impl trait' types (e.g. ty::Opaque) + // in the output + // + // Since we need to generate *something*, we just + // use a dummy string that should make it clear + // that something unusual is going on + if visited.insert(t) { + output.push_str(""); + return; + } + // When targeting MSVC, emit C++ style type names for compatibility with // .natvis visualizers (and perhaps other existing native debuggers?) let cpp_like_names = cx.sess().target.target.options.is_like_msvc; @@ -42,12 +64,12 @@ pub fn push_debuginfo_type_name<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, ty::Foreign(def_id) => push_item_name(cx, def_id, qualified, output), ty::Adt(def, substs) => { push_item_name(cx, def.did, qualified, output); - push_type_params(cx, substs, output); + push_type_params(cx, substs, output, visited); }, ty::Tuple(component_types) => { output.push('('); for &component_type in component_types { - push_debuginfo_type_name(cx, component_type, true, output); + push_debuginfo_type_name(cx, component_type, true, output, visited); output.push_str(", "); } if !component_types.is_empty() { @@ -65,7 +87,7 @@ pub fn push_debuginfo_type_name<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, hir::MutMutable => output.push_str("mut "), } - push_debuginfo_type_name(cx, inner_type, true, output); + push_debuginfo_type_name(cx, inner_type, true, output, visited); if cpp_like_names { output.push('*'); @@ -79,7 +101,7 @@ pub fn push_debuginfo_type_name<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, output.push_str("mut "); } - push_debuginfo_type_name(cx, inner_type, true, output); + push_debuginfo_type_name(cx, inner_type, true, output, visited); if cpp_like_names { output.push('*'); @@ -87,7 +109,7 @@ pub fn push_debuginfo_type_name<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, }, ty::Array(inner_type, len) => { output.push('['); - push_debuginfo_type_name(cx, inner_type, true, output); + push_debuginfo_type_name(cx, inner_type, true, output, visited); output.push_str(&format!("; {}", len.unwrap_usize(cx.tcx))); output.push(']'); }, @@ -98,7 +120,7 @@ pub fn push_debuginfo_type_name<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, output.push('['); } - push_debuginfo_type_name(cx, inner_type, true, output); + push_debuginfo_type_name(cx, inner_type, true, output, visited); if cpp_like_names { output.push('>'); @@ -113,7 +135,7 @@ pub fn push_debuginfo_type_name<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, &principal, ); push_item_name(cx, principal.def_id, false, output); - push_type_params(cx, principal.substs, output); + push_type_params(cx, principal.substs, output, visited); } else { output.push_str("dyn '_"); } @@ -136,7 +158,7 @@ pub fn push_debuginfo_type_name<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, let sig = cx.tcx.normalize_erasing_late_bound_regions(ty::ParamEnv::reveal_all(), &sig); if !sig.inputs().is_empty() { for ¶meter_type in sig.inputs() { - push_debuginfo_type_name(cx, parameter_type, true, output); + push_debuginfo_type_name(cx, parameter_type, true, output, visited); output.push_str(", "); } output.pop(); @@ -155,7 +177,7 @@ pub fn push_debuginfo_type_name<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, if !sig.output().is_unit() { output.push_str(" -> "); - push_debuginfo_type_name(cx, sig.output(), true, output); + push_debuginfo_type_name(cx, sig.output(), true, output, visited); } }, ty::Closure(..) => { @@ -200,7 +222,8 @@ pub fn push_debuginfo_type_name<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, // common denominator - otherwise we would run into conflicts. fn push_type_params<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, substs: SubstsRef<'tcx>, - output: &mut String) { + output: &mut String, + visited: &mut FxHashSet>) { if substs.types().next().is_none() { return; } @@ -208,7 +231,7 @@ pub fn push_debuginfo_type_name<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, output.push('<'); for type_parameter in substs.types() { - push_debuginfo_type_name(cx, type_parameter, true, output); + push_debuginfo_type_name(cx, type_parameter, true, output, visited); output.push_str(", "); } diff --git a/src/test/run-pass/issues/issue-58463.rs b/src/test/run-pass/issues/issue-58463.rs new file mode 100644 index 0000000000000..8ab845366b7b4 --- /dev/null +++ b/src/test/run-pass/issues/issue-58463.rs @@ -0,0 +1,8 @@ +// run-pass +// compile-flags:-C debuginfo=2 +fn foo() -> impl Copy { + foo +} +fn main() { + foo(); +} From e1837a0d1aa9fa9484bdc7f85c785c9fba6d7300 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 26 Mar 2019 19:49:14 -0400 Subject: [PATCH 2/6] Fix inverted panic check --- src/librustc_codegen_llvm/debuginfo/type_names.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_codegen_llvm/debuginfo/type_names.rs b/src/librustc_codegen_llvm/debuginfo/type_names.rs index 024840a3fa8ad..5854610256110 100644 --- a/src/librustc_codegen_llvm/debuginfo/type_names.rs +++ b/src/librustc_codegen_llvm/debuginfo/type_names.rs @@ -44,7 +44,7 @@ pub fn push_debuginfo_type_name<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, // Since we need to generate *something*, we just // use a dummy string that should make it clear // that something unusual is going on - if visited.insert(t) { + if !visited.insert(t) { output.push_str(""); return; } From aed7ec42b3615d0b469029a73a8638fcd57281cd Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 26 Mar 2019 22:38:55 -0400 Subject: [PATCH 3/6] Add codegen test --- src/test/codegen/fn-impl-trait-self.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 src/test/codegen/fn-impl-trait-self.rs diff --git a/src/test/codegen/fn-impl-trait-self.rs b/src/test/codegen/fn-impl-trait-self.rs new file mode 100644 index 0000000000000..f9113d50197c1 --- /dev/null +++ b/src/test/codegen/fn-impl-trait-self.rs @@ -0,0 +1,15 @@ +// compile-flags: -g +// +// CHECK-LABEL: @main +// CHECK: {{.*}}DIDerivedType(tag: DW_TAG_pointer_type, name: "fn() -> ",{{.*}} +// +// CHECK: {{.*}}DISubroutineType{{.*}} +// CHECK: {{.*}}DIBasicType(name: "", encoding: DW_ATE_unsigned) + +pub fn foo() -> impl Copy { + foo +} + +fn main() { + let my_res = foo(); +} From c4556a5a656b70a8be726847f540c2184dec4ef4 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Thu, 28 Mar 2019 12:22:08 -0400 Subject: [PATCH 4/6] Fix typos --- src/librustc_codegen_llvm/debuginfo/metadata.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/librustc_codegen_llvm/debuginfo/metadata.rs b/src/librustc_codegen_llvm/debuginfo/metadata.rs index 3d742811ff049..94d520ec78c71 100644 --- a/src/librustc_codegen_llvm/debuginfo/metadata.rs +++ b/src/librustc_codegen_llvm/debuginfo/metadata.rs @@ -126,14 +126,14 @@ impl TypeMap<'ll, 'tcx> { // This kind of type cannot be properly represented // via LLVM debuginfo. As a workaround, // we register a temporary Ty to metadata mapping - // for the function before we compute its actual metadat.a + // for the function before we compute its actual metadata. // If the metadata computation ends up recursing back to the // original function, it will use the temporary mapping // for the inner self-reference, preventing us from // recursing forever. // // This function is used to remove the temporary metadata - // mapping after we've computed the actual metadat + // mapping after we've computed the actual metadata fn remove_type( &mut self, type_: Ty<'tcx>, @@ -663,8 +663,6 @@ pub fn type_metadata( size.bits(), align.bits() as u32, DW_ATE_unsigned) - - } }; From d2584e3b6aa8ed60f291d71a86f849a7f667c2fd Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 31 Mar 2019 22:53:27 -0400 Subject: [PATCH 5/6] Only track 'visited' state for function types --- .../debuginfo/type_names.rs | 49 ++++++++++++------- 1 file changed, 31 insertions(+), 18 deletions(-) diff --git a/src/librustc_codegen_llvm/debuginfo/type_names.rs b/src/librustc_codegen_llvm/debuginfo/type_names.rs index 5854610256110..05c6c9877acd7 100644 --- a/src/librustc_codegen_llvm/debuginfo/type_names.rs +++ b/src/librustc_codegen_llvm/debuginfo/type_names.rs @@ -31,24 +31,6 @@ pub fn push_debuginfo_type_name<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, output: &mut String, visited: &mut FxHashSet>) { - // We've encountered a weird 'recursive type' - // Currently, the only way to generate such a type - // is by using 'impl trait': - // - // fn foo() -> impl Copy { foo } - // - // There's not really a sensible name we can generate, - // since we don't include 'impl trait' types (e.g. ty::Opaque) - // in the output - // - // Since we need to generate *something*, we just - // use a dummy string that should make it clear - // that something unusual is going on - if !visited.insert(t) { - output.push_str(""); - return; - } - // When targeting MSVC, emit C++ style type names for compatibility with // .natvis visualizers (and perhaps other existing native debuggers?) let cpp_like_names = cx.sess().target.target.options.is_like_msvc; @@ -141,6 +123,25 @@ pub fn push_debuginfo_type_name<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, } }, ty::FnDef(..) | ty::FnPtr(_) => { + // We've encountered a weird 'recursive type' + // Currently, the only way to generate such a type + // is by using 'impl trait': + // + // fn foo() -> impl Copy { foo } + // + // There's not really a sensible name we can generate, + // since we don't include 'impl trait' types (e.g. ty::Opaque) + // in the output + // + // Since we need to generate *something*, we just + // use a dummy string that should make it clear + // that something unusual is going on + if !visited.insert(t) { + output.push_str(""); + return; + } + + let sig = t.fn_sig(cx.tcx); if sig.unsafety() == hir::Unsafety::Unsafe { output.push_str("unsafe "); @@ -179,6 +180,18 @@ pub fn push_debuginfo_type_name<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, output.push_str(" -> "); push_debuginfo_type_name(cx, sig.output(), true, output, visited); } + + + // We only keep the type in 'visited' + // for the duration of the body of this method. + // It's fine for a particular function type + // to show up multiple times in one overall type + // (e.g. MyType u8, fn() -> u8> + // + // We only care about avoiding recursing + // directly back to the type we're currentlu + // processing + visited.remove(t); }, ty::Closure(..) => { output.push_str("closure"); From c13daeb036273d3fd0544c3cc58592f2a86d1631 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Mon, 1 Apr 2019 13:41:41 -0400 Subject: [PATCH 6/6] Fix typo --- src/librustc_codegen_llvm/debuginfo/type_names.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_codegen_llvm/debuginfo/type_names.rs b/src/librustc_codegen_llvm/debuginfo/type_names.rs index 05c6c9877acd7..eff7cd1bc8a48 100644 --- a/src/librustc_codegen_llvm/debuginfo/type_names.rs +++ b/src/librustc_codegen_llvm/debuginfo/type_names.rs @@ -189,7 +189,7 @@ pub fn push_debuginfo_type_name<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, // (e.g. MyType u8, fn() -> u8> // // We only care about avoiding recursing - // directly back to the type we're currentlu + // directly back to the type we're currently // processing visited.remove(t); },