From e8de4c3b1f0ab51fe3ea0a494cf9727ee3e0bf1d Mon Sep 17 00:00:00 2001 From: Philip Craig Date: Sat, 23 Mar 2019 17:00:04 +1000 Subject: [PATCH 1/2] Fix invalid DWARF for enums when using thinlto We were setting the same identifier for both the DW_TAG_structure_type and the DW_TAG_variant_part. This becomes a problem when using thinlto becauses it uses the identifier as a key for a map of types that is used to delete duplicates based on the ODR, so one of them is deleted as a duplicate, resulting in invalid DWARF. The DW_TAG_variant_part isn't a standalone type, so it doesn't need an identifier. Fix by omitting its identifier. --- .../debuginfo/metadata.rs | 20 +++++--- src/librustc_codegen_llvm/llvm/ffi.rs | 3 +- src/rustllvm/RustWrapper.cpp | 4 +- src/test/debuginfo/enum-thinlto.rs | 48 +++++++++++++++++++ 4 files changed, 64 insertions(+), 11 deletions(-) create mode 100644 src/test/debuginfo/enum-thinlto.rs diff --git a/src/librustc_codegen_llvm/debuginfo/metadata.rs b/src/librustc_codegen_llvm/debuginfo/metadata.rs index ddcbf29da832b..1859b1f4792e1 100644 --- a/src/librustc_codegen_llvm/debuginfo/metadata.rs +++ b/src/librustc_codegen_llvm/debuginfo/metadata.rs @@ -266,6 +266,7 @@ impl RecursiveTypeDescription<'ll, 'tcx> { // ... and attach them to the stub to complete it. set_members_of_composite_type(cx, unfinished_type, + metadata_stub, member_holding_stub, member_descriptions); return MetadataCreationResult::new(metadata_stub, true); @@ -1215,6 +1216,7 @@ impl EnumMemberDescriptionFactory<'ll, 'tcx> { set_members_of_composite_type(cx, self.enum_type, variant_type_metadata, + variant_type_metadata, member_descriptions); vec![ MemberDescription { @@ -1256,6 +1258,7 @@ impl EnumMemberDescriptionFactory<'ll, 'tcx> { set_members_of_composite_type(cx, self.enum_type, variant_type_metadata, + variant_type_metadata, member_descriptions); MemberDescription { name: if fallback { @@ -1298,6 +1301,7 @@ impl EnumMemberDescriptionFactory<'ll, 'tcx> { set_members_of_composite_type(cx, self.enum_type, variant_type_metadata, + variant_type_metadata, variant_member_descriptions); // Encode the information about the null variant in the union @@ -1358,6 +1362,7 @@ impl EnumMemberDescriptionFactory<'ll, 'tcx> { set_members_of_composite_type(cx, self.enum_type, variant_type_metadata, + variant_type_metadata, member_descriptions); let niche_value = if i == dataful_variant { @@ -1698,8 +1703,7 @@ fn prepare_enum_metadata( layout.align.abi.bits() as u32, DIFlags::FlagZero, discriminator_metadata, - empty_array, - unique_type_id_str.as_ptr()) + empty_array) }; // The variant part must be wrapped in a struct according to DWARF. @@ -1770,6 +1774,7 @@ fn composite_type_metadata( set_members_of_composite_type(cx, composite_type, composite_type_metadata, + composite_type_metadata, member_descriptions); composite_type_metadata @@ -1777,7 +1782,8 @@ fn composite_type_metadata( fn set_members_of_composite_type(cx: &CodegenCx<'ll, 'tcx>, composite_type: Ty<'tcx>, - composite_type_metadata: &'ll DICompositeType, + metadata_stub: &'ll DICompositeType, + member_holding_stub: &'ll DICompositeType, member_descriptions: Vec>) { // In some rare cases LLVM metadata uniquing would lead to an existing type // description being used instead of a new one created in @@ -1788,11 +1794,11 @@ fn set_members_of_composite_type(cx: &CodegenCx<'ll, 'tcx>, { let mut composite_types_completed = debug_context(cx).composite_types_completed.borrow_mut(); - if composite_types_completed.contains(&composite_type_metadata) { + if composite_types_completed.contains(&metadata_stub) { bug!("debuginfo::set_members_of_composite_type() - \ Already completed forward declaration re-encountered."); } else { - composite_types_completed.insert(composite_type_metadata); + composite_types_completed.insert(metadata_stub); } } @@ -1803,7 +1809,7 @@ fn set_members_of_composite_type(cx: &CodegenCx<'ll, 'tcx>, unsafe { Some(llvm::LLVMRustDIBuilderCreateVariantMemberType( DIB(cx), - composite_type_metadata, + member_holding_stub, member_name.as_ptr(), unknown_file_metadata(cx), UNKNOWN_LINE_NUMBER, @@ -1824,7 +1830,7 @@ fn set_members_of_composite_type(cx: &CodegenCx<'ll, 'tcx>, unsafe { let type_array = create_DIArray(DIB(cx), &member_metadata[..]); llvm::LLVMRustDICompositeTypeReplaceArrays( - DIB(cx), composite_type_metadata, Some(type_array), type_params); + DIB(cx), member_holding_stub, Some(type_array), type_params); } } diff --git a/src/librustc_codegen_llvm/llvm/ffi.rs b/src/librustc_codegen_llvm/llvm/ffi.rs index 2ad6d9c053a20..eddc509a585d7 100644 --- a/src/librustc_codegen_llvm/llvm/ffi.rs +++ b/src/librustc_codegen_llvm/llvm/ffi.rs @@ -1587,8 +1587,7 @@ extern "C" { AlignInBits: u32, Flags: DIFlags, Discriminator: Option<&'a DIDerivedType>, - Elements: &'a DIArray, - UniqueId: *const c_char) + Elements: &'a DIArray) -> &'a DIDerivedType; pub fn LLVMSetUnnamedAddr(GlobalVar: &Value, UnnamedAddr: Bool); diff --git a/src/rustllvm/RustWrapper.cpp b/src/rustllvm/RustWrapper.cpp index a00417a362927..470cdcdd613a5 100644 --- a/src/rustllvm/RustWrapper.cpp +++ b/src/rustllvm/RustWrapper.cpp @@ -723,12 +723,12 @@ extern "C" LLVMMetadataRef LLVMRustDIBuilderCreateVariantPart( LLVMRustDIBuilderRef Builder, LLVMMetadataRef Scope, const char *Name, LLVMMetadataRef File, unsigned LineNumber, uint64_t SizeInBits, uint32_t AlignInBits, LLVMRustDIFlags Flags, LLVMMetadataRef Discriminator, - LLVMMetadataRef Elements, const char *UniqueId) { + LLVMMetadataRef Elements) { #if LLVM_VERSION_GE(7, 0) return wrap(Builder->createVariantPart( unwrapDI(Scope), Name, unwrapDI(File), LineNumber, SizeInBits, AlignInBits, fromRust(Flags), unwrapDI(Discriminator), - DINodeArray(unwrapDI(Elements)), UniqueId)); + DINodeArray(unwrapDI(Elements)))); #else abort(); #endif diff --git a/src/test/debuginfo/enum-thinlto.rs b/src/test/debuginfo/enum-thinlto.rs new file mode 100644 index 0000000000000..ae736e40bcc1c --- /dev/null +++ b/src/test/debuginfo/enum-thinlto.rs @@ -0,0 +1,48 @@ +// ignore-tidy-linelength + +// Require LLVM with DW_TAG_variant_part and a gdb that can read it. +// min-system-llvm-version: 7.0 +// min-gdb-version: 8.2 + +// compile-flags:-g -Z thinlto + +// === GDB TESTS =================================================================================== + +// gdb-command:run + +// gdb-command:print *abc +// gdbr-check:$1 = enum_thinlto::ABC::TheA{x: 0, y: 8970181431921507452} + +// === LLDB TESTS ================================================================================== + +// lldb-command:run + +// lldb-command:print *abc +// lldbg-check:(enum_thinlto::ABC) $0 = ABC { } + +#![allow(unused_variables)] +#![feature(omit_gdb_pretty_printer_section)] +#![omit_gdb_pretty_printer_section] + +// The first element is to ensure proper alignment, irrespective of the machines word size. Since +// the size of the discriminant value is machine dependent, this has be taken into account when +// datatype layout should be predictable as in this case. +#[derive(Debug)] +enum ABC { + TheA { x: i64, y: i64 }, + TheB (i64, i32, i32), +} + +fn main() { + let abc = ABC::TheA { x: 0, y: 0x7c7c_7c7c_7c7c_7c7c }; + + f(&abc); +} + +fn f(abc: &ABC) { + zzz(); // #break + + println!("{:?}", abc); +} + +fn zzz() {()} From 3a5a8a529a14271f5d8c21bec8746edfa93eec5f Mon Sep 17 00:00:00 2001 From: Philip Craig Date: Wed, 27 Mar 2019 15:22:37 +1000 Subject: [PATCH 2/2] Give variant parts their own unique id and bump llvm version in test --- .../debuginfo/metadata.rs | 36 ++++++++++++------- src/librustc_codegen_llvm/llvm/ffi.rs | 3 +- src/rustllvm/RustWrapper.cpp | 4 +-- src/test/debuginfo/enum-thinlto.rs | 2 +- 4 files changed, 28 insertions(+), 17 deletions(-) diff --git a/src/librustc_codegen_llvm/debuginfo/metadata.rs b/src/librustc_codegen_llvm/debuginfo/metadata.rs index 1859b1f4792e1..e7183df2ec3a5 100644 --- a/src/librustc_codegen_llvm/debuginfo/metadata.rs +++ b/src/librustc_codegen_llvm/debuginfo/metadata.rs @@ -188,6 +188,17 @@ impl TypeMap<'ll, 'tcx> { let interner_key = self.unique_id_interner.intern(&enum_variant_type_id); UniqueTypeId(interner_key) } + + // Get the unique type id string for an enum variant part. + // Variant parts are not types and shouldn't really have their own id, + // but it makes set_members_of_composite_type() simpler. + fn get_unique_type_id_str_of_enum_variant_part<'a>(&mut self, + enum_type_id: UniqueTypeId) -> &str { + let variant_part_type_id = format!("{}_variant_part", + self.get_unique_type_id_as_string(enum_type_id)); + let interner_key = self.unique_id_interner.intern(&variant_part_type_id); + self.unique_id_interner.get(interner_key) + } } // A description of some recursive type. It can either be already finished (as @@ -266,7 +277,6 @@ impl RecursiveTypeDescription<'ll, 'tcx> { // ... and attach them to the stub to complete it. set_members_of_composite_type(cx, unfinished_type, - metadata_stub, member_holding_stub, member_descriptions); return MetadataCreationResult::new(metadata_stub, true); @@ -1216,7 +1226,6 @@ impl EnumMemberDescriptionFactory<'ll, 'tcx> { set_members_of_composite_type(cx, self.enum_type, variant_type_metadata, - variant_type_metadata, member_descriptions); vec![ MemberDescription { @@ -1258,7 +1267,6 @@ impl EnumMemberDescriptionFactory<'ll, 'tcx> { set_members_of_composite_type(cx, self.enum_type, variant_type_metadata, - variant_type_metadata, member_descriptions); MemberDescription { name: if fallback { @@ -1301,7 +1309,6 @@ impl EnumMemberDescriptionFactory<'ll, 'tcx> { set_members_of_composite_type(cx, self.enum_type, variant_type_metadata, - variant_type_metadata, variant_member_descriptions); // Encode the information about the null variant in the union @@ -1362,7 +1369,6 @@ impl EnumMemberDescriptionFactory<'ll, 'tcx> { set_members_of_composite_type(cx, self.enum_type, variant_type_metadata, - variant_type_metadata, member_descriptions); let niche_value = if i == dataful_variant { @@ -1691,6 +1697,11 @@ fn prepare_enum_metadata( }, }; + let variant_part_unique_type_id_str = SmallCStr::new( + debug_context(cx).type_map + .borrow_mut() + .get_unique_type_id_str_of_enum_variant_part(unique_type_id) + ); let empty_array = create_DIArray(DIB(cx), &[]); let variant_part = unsafe { llvm::LLVMRustDIBuilderCreateVariantPart( @@ -1703,7 +1714,8 @@ fn prepare_enum_metadata( layout.align.abi.bits() as u32, DIFlags::FlagZero, discriminator_metadata, - empty_array) + empty_array, + variant_part_unique_type_id_str.as_ptr()) }; // The variant part must be wrapped in a struct according to DWARF. @@ -1774,7 +1786,6 @@ fn composite_type_metadata( set_members_of_composite_type(cx, composite_type, composite_type_metadata, - composite_type_metadata, member_descriptions); composite_type_metadata @@ -1782,8 +1793,7 @@ fn composite_type_metadata( fn set_members_of_composite_type(cx: &CodegenCx<'ll, 'tcx>, composite_type: Ty<'tcx>, - metadata_stub: &'ll DICompositeType, - member_holding_stub: &'ll DICompositeType, + composite_type_metadata: &'ll DICompositeType, member_descriptions: Vec>) { // In some rare cases LLVM metadata uniquing would lead to an existing type // description being used instead of a new one created in @@ -1794,11 +1804,11 @@ fn set_members_of_composite_type(cx: &CodegenCx<'ll, 'tcx>, { let mut composite_types_completed = debug_context(cx).composite_types_completed.borrow_mut(); - if composite_types_completed.contains(&metadata_stub) { + if composite_types_completed.contains(&composite_type_metadata) { bug!("debuginfo::set_members_of_composite_type() - \ Already completed forward declaration re-encountered."); } else { - composite_types_completed.insert(metadata_stub); + composite_types_completed.insert(composite_type_metadata); } } @@ -1809,7 +1819,7 @@ fn set_members_of_composite_type(cx: &CodegenCx<'ll, 'tcx>, unsafe { Some(llvm::LLVMRustDIBuilderCreateVariantMemberType( DIB(cx), - member_holding_stub, + composite_type_metadata, member_name.as_ptr(), unknown_file_metadata(cx), UNKNOWN_LINE_NUMBER, @@ -1830,7 +1840,7 @@ fn set_members_of_composite_type(cx: &CodegenCx<'ll, 'tcx>, unsafe { let type_array = create_DIArray(DIB(cx), &member_metadata[..]); llvm::LLVMRustDICompositeTypeReplaceArrays( - DIB(cx), member_holding_stub, Some(type_array), type_params); + DIB(cx), composite_type_metadata, Some(type_array), type_params); } } diff --git a/src/librustc_codegen_llvm/llvm/ffi.rs b/src/librustc_codegen_llvm/llvm/ffi.rs index eddc509a585d7..2ad6d9c053a20 100644 --- a/src/librustc_codegen_llvm/llvm/ffi.rs +++ b/src/librustc_codegen_llvm/llvm/ffi.rs @@ -1587,7 +1587,8 @@ extern "C" { AlignInBits: u32, Flags: DIFlags, Discriminator: Option<&'a DIDerivedType>, - Elements: &'a DIArray) + Elements: &'a DIArray, + UniqueId: *const c_char) -> &'a DIDerivedType; pub fn LLVMSetUnnamedAddr(GlobalVar: &Value, UnnamedAddr: Bool); diff --git a/src/rustllvm/RustWrapper.cpp b/src/rustllvm/RustWrapper.cpp index 470cdcdd613a5..a00417a362927 100644 --- a/src/rustllvm/RustWrapper.cpp +++ b/src/rustllvm/RustWrapper.cpp @@ -723,12 +723,12 @@ extern "C" LLVMMetadataRef LLVMRustDIBuilderCreateVariantPart( LLVMRustDIBuilderRef Builder, LLVMMetadataRef Scope, const char *Name, LLVMMetadataRef File, unsigned LineNumber, uint64_t SizeInBits, uint32_t AlignInBits, LLVMRustDIFlags Flags, LLVMMetadataRef Discriminator, - LLVMMetadataRef Elements) { + LLVMMetadataRef Elements, const char *UniqueId) { #if LLVM_VERSION_GE(7, 0) return wrap(Builder->createVariantPart( unwrapDI(Scope), Name, unwrapDI(File), LineNumber, SizeInBits, AlignInBits, fromRust(Flags), unwrapDI(Discriminator), - DINodeArray(unwrapDI(Elements)))); + DINodeArray(unwrapDI(Elements)), UniqueId)); #else abort(); #endif diff --git a/src/test/debuginfo/enum-thinlto.rs b/src/test/debuginfo/enum-thinlto.rs index ae736e40bcc1c..7f15ed90e67b3 100644 --- a/src/test/debuginfo/enum-thinlto.rs +++ b/src/test/debuginfo/enum-thinlto.rs @@ -1,7 +1,7 @@ // ignore-tidy-linelength // Require LLVM with DW_TAG_variant_part and a gdb that can read it. -// min-system-llvm-version: 7.0 +// min-system-llvm-version: 8.0 // min-gdb-version: 8.2 // compile-flags:-g -Z thinlto