[CLANG][DWARF] Handle DIE offset collision in DW_IDX_parent #95039
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fixes llvm#93886. The UnitID is not unique between CUs and TUs. This led to DW_IDX_parent to point ot an entry for a DIE in CU if it had the same relative offset as TU die. Added a IsTU to the hash for parent chain.
|
@llvm/pr-subscribers-debuginfo Author: Alexander Yermolovich (ayermolo) ChangesThis fixes #93886. The UnitID is not Added a IsTU to the hash for parent chain. Full diff: https://github.com/llvm/llvm-project/pull/95039.diff 5 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/AccelTable.h b/llvm/include/llvm/CodeGen/AccelTable.h
index cff8fcbaf2cd7..622fcf019aad6 100644
--- a/llvm/include/llvm/CodeGen/AccelTable.h
+++ b/llvm/include/llvm/CodeGen/AccelTable.h
@@ -257,17 +257,37 @@ class AppleAccelTableData : public AccelTableData {
/// Helper class to identify an entry in DWARF5AccelTable based on their DIE
/// offset and UnitID.
-struct OffsetAndUnitID : std::pair<uint64_t, uint32_t> {
- using Base = std::pair<uint64_t, uint32_t>;
- OffsetAndUnitID(Base B) : Base(B) {}
-
- OffsetAndUnitID(uint64_t Offset, uint32_t UnitID) : Base(Offset, UnitID) {}
- uint64_t offset() const { return first; };
- uint32_t unitID() const { return second; };
+struct OffsetAndUnitID {
+ uint64_t Offset = 0;
+ uint32_t UnitID = 0;
+ bool IsTU = false;
+ OffsetAndUnitID() = default;
+ OffsetAndUnitID(uint64_t Offset, uint32_t UnitID, bool IsTU)
+ : Offset(Offset), UnitID(UnitID), IsTU(IsTU) {}
+ uint64_t offset() const { return Offset; };
+ uint32_t unitID() const { return UnitID; };
+ bool isTU() const { return IsTU; }
};
-template <>
-struct DenseMapInfo<OffsetAndUnitID> : DenseMapInfo<OffsetAndUnitID::Base> {};
+template <> struct DenseMapInfo<OffsetAndUnitID> {
+ static inline OffsetAndUnitID getEmptyKey() {
+ OffsetAndUnitID Entry;
+ Entry.Offset = uint64_t(-1);
+ return Entry;
+ }
+ static inline OffsetAndUnitID getTombstoneKey() {
+ OffsetAndUnitID Entry;
+ Entry.Offset = uint64_t(-2);
+ return Entry;
+ }
+ static unsigned getHashValue(const OffsetAndUnitID &Val) {
+ return (unsigned)llvm::hash_combine(Val.offset(), Val.unitID(), Val.IsTU);
+ }
+ static bool isEqual(const OffsetAndUnitID &LHS, const OffsetAndUnitID &RHS) {
+ return LHS.offset() == RHS.offset() && LHS.unitID() == RHS.unitID() &&
+ LHS.IsTU == RHS.isTU();
+ }
+};
/// The Data class implementation for DWARF v5 accelerator table. Unlike the
/// Apple Data classes, this class is just a DIE wrapper, and does not know to
@@ -277,12 +297,11 @@ class DWARF5AccelTableData : public AccelTableData {
public:
static uint32_t hash(StringRef Name) { return caseFoldingDjbHash(Name); }
- DWARF5AccelTableData(const DIE &Die, const uint32_t UnitID,
- const bool IsTU = false);
+ DWARF5AccelTableData(const DIE &Die, const uint32_t UnitID, const bool IsTU);
DWARF5AccelTableData(const uint64_t DieOffset,
const std::optional<uint64_t> DefiningParentOffset,
const unsigned DieTag, const unsigned UnitID,
- const bool IsTU = false)
+ const bool IsTU)
: OffsetVal(DieOffset), ParentOffset(DefiningParentOffset),
DieTag(DieTag), AbbrevNumber(0), IsTU(IsTU), UnitID(UnitID) {}
@@ -296,7 +315,7 @@ class DWARF5AccelTableData : public AccelTableData {
}
OffsetAndUnitID getDieOffsetAndUnitID() const {
- return {getDieOffset(), UnitID};
+ return {getDieOffset(), getUnitID(), isTU()};
}
unsigned getDieTag() const { return DieTag; }
@@ -322,7 +341,7 @@ class DWARF5AccelTableData : public AccelTableData {
assert(isNormalized() && "Accessing DIE Offset before normalizing.");
if (!ParentOffset)
return std::nullopt;
- return OffsetAndUnitID(*ParentOffset, getUnitID());
+ return OffsetAndUnitID(*ParentOffset, getUnitID(), isTU());
}
/// Sets AbbrevIndex for an Entry.
@@ -416,7 +435,7 @@ class DWARF5AccelTable : public AccelTable<DWARF5AccelTableData> {
for (auto *Data : Entry.second.getValues<DWARF5AccelTableData *>()) {
addName(Entry.second.Name, Data->getDieOffset(),
Data->getParentDieOffset(), Data->getDieTag(),
- Data->getUnitID(), true);
+ Data->getUnitID(), Data->isTU());
}
}
}
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index b9c02aed848cc..7de9432325d8a 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -3592,7 +3592,8 @@ void DwarfDebug::addAccelNameImpl(
"Kind is TU but CU is being processed.");
// The type unit can be discarded, so need to add references to final
// acceleration table once we know it's complete and we emit it.
- Current.addName(Ref, Die, Unit.getUniqueID());
+ Current.addName(Ref, Die, Unit.getUniqueID(),
+ Unit.getUnitDie().getTag() == dwarf::DW_TAG_type_unit);
break;
}
case AccelTableKind::Default:
diff --git a/llvm/lib/DWARFLinker/Classic/DWARFLinker.cpp b/llvm/lib/DWARFLinker/Classic/DWARFLinker.cpp
index 2544d97eaafd0..f6aaf88bb5fb7 100644
--- a/llvm/lib/DWARFLinker/Classic/DWARFLinker.cpp
+++ b/llvm/lib/DWARFLinker/Classic/DWARFLinker.cpp
@@ -2247,17 +2247,20 @@ void DWARFLinker::emitAcceleratorEntriesForUnit(CompileUnit &Unit) {
DebugNames.addName(
Namespace.Name, Namespace.Die->getOffset(),
DWARF5AccelTableData::getDefiningParentDieOffset(*Namespace.Die),
- Namespace.Die->getTag(), Unit.getUniqueID());
+ Namespace.Die->getTag(), Unit.getUniqueID(),
+ Unit.getOutputUnitDIE()->getTag() == dwarf::DW_TAG_type_unit);
for (const auto &Pubname : Unit.getPubnames())
DebugNames.addName(
Pubname.Name, Pubname.Die->getOffset(),
DWARF5AccelTableData::getDefiningParentDieOffset(*Pubname.Die),
- Pubname.Die->getTag(), Unit.getUniqueID());
+ Pubname.Die->getTag(), Unit.getUniqueID(),
+ Unit.getOutputUnitDIE()->getTag() == dwarf::DW_TAG_type_unit);
for (const auto &Pubtype : Unit.getPubtypes())
DebugNames.addName(
Pubtype.Name, Pubtype.Die->getOffset(),
DWARF5AccelTableData::getDefiningParentDieOffset(*Pubtype.Die),
- Pubtype.Die->getTag(), Unit.getUniqueID());
+ Pubtype.Die->getTag(), Unit.getUniqueID(),
+ Unit.getOutputUnitDIE()->getTag() == dwarf::DW_TAG_type_unit);
} break;
}
}
diff --git a/llvm/lib/DWARFLinker/Parallel/DWARFLinkerImpl.cpp b/llvm/lib/DWARFLinker/Parallel/DWARFLinkerImpl.cpp
index e68bf0c227a0a..a068ec1a3056d 100644
--- a/llvm/lib/DWARFLinker/Parallel/DWARFLinkerImpl.cpp
+++ b/llvm/lib/DWARFLinker/Parallel/DWARFLinkerImpl.cpp
@@ -1356,9 +1356,10 @@ void DWARFLinkerImpl::emitDWARFv5DebugNamesSection(const Triple &TargetTriple) {
case DwarfUnit::AccelType::Name:
case DwarfUnit::AccelType::Namespace:
case DwarfUnit::AccelType::Type: {
- DebugNames->addName(*DebugStrStrings.getExistingEntry(Info.String),
- Info.OutOffset, std::nullopt /*ParentDIEOffset*/,
- Info.Tag, CU->getUniqueID());
+ DebugNames->addName(
+ *DebugStrStrings.getExistingEntry(Info.String), Info.OutOffset,
+ std::nullopt /*ParentDIEOffset*/, Info.Tag, CU->getUniqueID(),
+ CU->getOutUnitDIE()->getTag() == dwarf::DW_TAG_type_unit);
} break;
default:
diff --git a/llvm/test/DebugInfo/X86/debug-names-types-die-offset-collision.ll b/llvm/test/DebugInfo/X86/debug-names-types-die-offset-collision.ll
new file mode 100644
index 0000000000000..104a166ffd3ad
--- /dev/null
+++ b/llvm/test/DebugInfo/X86/debug-names-types-die-offset-collision.ll
@@ -0,0 +1,69 @@
+; UNSUPPORTED: system-windows
+
+;; This test checks that DW_IDX_parent is generated correctly when there is DIE relative offset collision between CU and TU.
+
+; RUN: llc -mtriple=x86_64 -generate-type-units -dwarf-version=5 -filetype=obj %s -o %t
+; RUN: llvm-dwarfdump -debug-info -debug-names %t | FileCheck %s
+
+; CHECK: .debug_info contents:
+; CHECK: 0x00000023: DW_TAG_namespace
+; CHECK-NEXT: DW_AT_name ("B")
+; CHECK: 0x00000023: DW_TAG_subprogram
+; CHECK-NEXT: DW_AT_low_pc
+; CHECK-NEXT: DW_AT_high_pc
+; CHECK-NEXT: DW_AT_frame_base
+; CHECK-NEXT: DW_AT_linkage_name ("_Z9get_statev")
+; CHECK-NEXT: DW_AT_name ("get_state")
+
+; CHECK: .debug_names contents:
+; CHECK: String: {{.*}} "B"
+; CHECK: Entry @ [[ENTRY:0x[0-9a-f]*]]
+; CHECK: String: {{.*}} "State"
+; CHECK: Entry @ 0xd3 {
+; CHECK: Abbrev: 0x4
+; CHECK: Tag: DW_TAG_structure_type
+; CHECK: DW_IDX_type_unit: 0x00
+; CHECK: DW_IDX_die_offset: 0x00000025
+; CHECK: DW_IDX_parent: Entry @ [[ENTRY:0x[0-9a-f]*]]
+; CHECK: }
+
+
+;; namespace B { struct State { class InnerState{}; }; }
+;; B::State::InnerState get_state() { return B::State::InnerState(); }
+;; clang++ main.cpp -g2 -O0 -fdebug-types-section -gpubnames
+
+; ModuleID = 'main.cpp'
+source_filename = "main.cpp"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: mustprogress noinline nounwind optnone uwtable
+define dso_local void @_Z9get_statev() #0 !dbg !10 {
+entry:
+ ret void, !dbg !17
+}
+
+attributes #0 = { mustprogress noinline nounwind optnone uwtable "frame-pointer"="all" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5, !6, !7, !8}
+!llvm.ident = !{!9}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 19.0.0git", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false)
+!1 = !DIFile(filename: "main.cpp", directory: "/folder", checksumkind: CSK_MD5, checksum: "a84fe2e4ecb77633f6c33f3b6833b9e7")
+!2 = !{i32 7, !"Dwarf Version", i32 5}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"wchar_size", i32 4}
+!5 = !{i32 8, !"PIC Level", i32 2}
+!6 = !{i32 7, !"PIE Level", i32 2}
+!7 = !{i32 7, !"uwtable", i32 2}
+!8 = !{i32 7, !"frame-pointer", i32 2}
+!9 = !{!"clang version 19.0.0git"}
+!10 = distinct !DISubprogram(name: "get_state", linkageName: "_Z9get_statev", scope: !1, file: !1, line: 2, type: !11, scopeLine: 2, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0)
+!11 = !DISubroutineType(types: !12)
+!12 = !{!13}
+!13 = distinct !DICompositeType(tag: DW_TAG_class_type, name: "InnerState", scope: !14, file: !1, line: 1, size: 8, flags: DIFlagTypePassByValue, elements: !16, identifier: "_ZTSN1B5State10InnerStateE")
+!14 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "State", scope: !15, file: !1, line: 1, size: 8, flags: DIFlagTypePassByValue, elements: !16, identifier: "_ZTSN1B5StateE")
+!15 = !DINamespace(name: "B", scope: null)
+!16 = !{}
+!17 = !DILocation(line: 2, column: 36, scope: !10)
|
ayermolo
changed the title
felipepiovezan
approved these changes
Jun 10, 2024
| @@ -257,17 +257,37 @@ class AppleAccelTableData : public AccelTableData { | |||
|
|
|||
| /// Helper class to identify an entry in DWARF5AccelTable based on their DIE | |||
| /// offset and UnitID. | |||
| struct OffsetAndUnitID : std::pair<uint64_t, uint32_t> { | |||
There was a problem hiding this comment.
would it make it sense to just use tuple here to avoid having to define the dense map boilerplate?
There was a problem hiding this comment.
I don't really have a strong opinion. Poking around I don't really see tuple being used with DenseMap. More standard approach seems this boilerplate code.
@dwblaikie to break a tie? :)
|
Merged, if there is a strong opinion about tuple I can put up follow up PR. |
|
Out on vacation until tomorrow, but did you try making the die numbering unique across type and compile units? I would've thought that might be simpler, but maybe had too many knock-on effects/other things that'd need fixing/changing. |
I thought this would be a less intrusive change. |
|
I think this broke the ASan bot: |
fmayer
added a commit
that referenced
this pull request
Jun 12, 2024
…95302) Reverts #95039 This looks like it caused the ASan bot to fail: https://lab.llvm.org/buildbot/#/builders/168/builds/20912 Offending line was changed in this PR
|
Thanks for flagging. Passing false instead fixes asan. The UnitDie is getting set in CompileUnit::cloneAndEmit. After it gets cloned using local Allocator. Maybe worth resetting to nullptr at the end? |
|
Passing false to what exactly? |
to DebugNames->addName, instead of CU->getOutUnitDIE()->getTag() == dwarf::DW_TAG_type_unit. Basically issue was I was accessing getOutUnitDIE which was allocated local BumpAllocator in CompileUnit::cloneAndEmit. So memory was garbage. To be clear passing false always is not a fix. It was just a quick test. |
|
Fixed asan: #95339 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes #93886. The UnitID is not
unique between CUs and TUs. This led to DW_IDX_parent to point ot an entry for a
DIE in a CU if it had the same relative offset as a TU die.
Added a IsTU to the hash for parent chain.