Add support for using foreign type units in .debug_names. #87740
Conversation
|
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-lldb Author: Greg Clayton (clayborg) ChangesThis patch adds support for the new foreign type unit support in .debug_names. Features include:
Patch is 22.47 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/87740.diff 11 Files Affected:
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
index 44febcfac3b000..0e111c8ec47f45 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
@@ -223,7 +223,17 @@ DWARFUnit *DWARFDebugInfo::GetUnitAtOffset(DIERef::Section section,
}
DWARFUnit *DWARFDebugInfo::GetUnit(const DIERef &die_ref) {
- return GetUnitContainingDIEOffset(die_ref.section(), die_ref.die_offset());
+ // Make sure we get the correct SymbolFileDWARF from the DIERef before
+ // asking for information from a debug info object. We might start with the
+ // DWARFDebugInfo for the main executable in a split DWARF and the DIERef
+ // might be pointing to a specific .dwo file or to the .dwp file. So this
+ // makes sure we get the right SymbolFileDWARF instance before finding the
+ // DWARFUnit that contains the offset. If we just use this object to do the
+ // search, we might be using the wrong .debug_info section from the wrong
+ // file with an offset meant for a different section.
+ SymbolFileDWARF *dwarf = m_dwarf.GetDIERefSymbolFile(die_ref);
+ return dwarf->DebugInfo().GetUnitContainingDIEOffset(die_ref.section(),
+ die_ref.die_offset());
}
DWARFUnit *
@@ -236,6 +246,10 @@ DWARFDebugInfo::GetUnitContainingDIEOffset(DIERef::Section section,
return result;
}
+const std::shared_ptr<SymbolFileDWARFDwo> DWARFDebugInfo::GetDwpSymbolFile() {
+ return m_dwarf.GetDwpSymbolFile();
+}
+
DWARFTypeUnit *DWARFDebugInfo::GetTypeUnitForHash(uint64_t hash) {
auto pos = llvm::lower_bound(m_type_hash_to_unit_index,
std::make_pair(hash, 0u), llvm::less_first());
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h
index c1f0cb0203fb76..b7f99ce6282b1f 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h
@@ -58,6 +58,8 @@ class DWARFDebugInfo {
const DWARFDebugAranges &GetCompileUnitAranges();
+ const std::shared_ptr<SymbolFileDWARFDwo> GetDwpSymbolFile();
+
protected:
typedef std::vector<DWARFUnitSP> UnitColl;
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
index 4da0d56fdcacb4..ba35b3b872e6c6 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
@@ -34,6 +34,18 @@ DebugNamesDWARFIndex::Create(Module &module, DWARFDataExtractor debug_names,
module, std::move(index_up), debug_names, debug_str, dwarf));
}
+
+llvm::DenseSet<uint64_t>
+DebugNamesDWARFIndex::GetTypeUnitSigs(const DebugNames &debug_names) {
+ llvm::DenseSet<uint64_t> result;
+ for (const DebugNames::NameIndex &ni : debug_names) {
+ const uint32_t num_tus = ni.getForeignTUCount();
+ for (uint32_t tu = 0; tu < num_tus; ++tu)
+ result.insert(ni.getForeignTUSignature(tu));
+ }
+ return result;
+}
+
llvm::DenseSet<dw_offset_t>
DebugNamesDWARFIndex::GetUnits(const DebugNames &debug_names) {
llvm::DenseSet<dw_offset_t> result;
@@ -48,6 +60,15 @@ DebugNamesDWARFIndex::GetUnits(const DebugNames &debug_names) {
return result;
}
+DWARFTypeUnit *
+DebugNamesDWARFIndex::GetForeignTypeUnit(const DebugNames::Entry &entry) const {
+ std::optional<uint64_t> type_sig = entry.getForeignTUTypeSignature();
+ if (type_sig)
+ if (auto dwp_sp = m_debug_info.GetDwpSymbolFile())
+ return dwp_sp->DebugInfo().GetTypeUnitForHash(*type_sig);
+ return nullptr;
+}
+
std::optional<DIERef>
DebugNamesDWARFIndex::ToDIERef(const DebugNames::Entry &entry) const {
// Look for a DWARF unit offset (CU offset or local TU offset) as they are
@@ -55,8 +76,15 @@ DebugNamesDWARFIndex::ToDIERef(const DebugNames::Entry &entry) const {
std::optional<uint64_t> unit_offset = entry.getCUOffset();
if (!unit_offset) {
unit_offset = entry.getLocalTUOffset();
- if (!unit_offset)
+ if (!unit_offset) {
+ if (DWARFTypeUnit *tu = GetForeignTypeUnit(entry)) {
+ if (std::optional<uint64_t> die_offset = entry.getDIEUnitOffset())
+ return DIERef(tu->GetSymbolFileDWARF().GetFileIndex(),
+ DIERef::Section::DebugInfo,
+ tu->GetOffset() + *die_offset);
+ }
return std::nullopt;
+ }
}
DWARFUnit *cu =
@@ -273,6 +301,44 @@ void DebugNamesDWARFIndex::GetFullyQualifiedType(
if (!isType(entry.tag()))
continue;
+
+ DWARFTypeUnit *foreign_tu = GetForeignTypeUnit(entry);
+ if (foreign_tu) {
+ // If this entry represents a foreign type unit, we need to verify that
+ // the type unit that ended up in the final .dwp file is the right type
+ // unit. Type units have signatures which are the same across multiple
+ // .dwo files, but only one of those type units will end up in the .dwp
+ // file. The contents of type units for the same type can be different
+ // in different .dwo file, which means the DIE offsets might not be the
+ // same between two different type units. So we need to determine if this
+ // accelerator table matches the type unit in the .dwp file. If it doesn't
+ // match, then we need to ignore this accelerator table entry as the type
+ // unit that is in the .dwp file will have its own index.
+ const llvm::DWARFDebugNames::NameIndex *name_index = entry.getNameIndex();
+ if (name_index == nullptr)
+ continue;
+ // In order to determine if the type unit that ended up in a .dwp file
+ // is valid, we need to grab the type unit and check the attribute on the
+ // type unit matches the .dwo file. For this to happen we rely on each
+ // .dwo file having its own .debug_names table with a single compile unit
+ // and multiple type units. This is the only way we can tell if a type
+ // unit came from a specific .dwo file.
+ if (name_index->getCUCount() == 1) {
+ dw_offset_t cu_offset = name_index->getCUOffset(0);
+ DWARFUnit *cu = m_debug_info.GetUnitAtOffset(DIERef::DebugInfo,
+ cu_offset);
+ if (cu) {
+ DWARFBaseDIE cu_die = cu->GetUnitDIEOnly();
+ DWARFBaseDIE tu_die = foreign_tu->GetUnitDIEOnly();
+ llvm::StringRef cu_dwo_name =
+ cu_die.GetAttributeValueAsString(DW_AT_dwo_name, nullptr);
+ llvm::StringRef tu_dwo_name =
+ tu_die.GetAttributeValueAsString(DW_AT_dwo_name, nullptr);
+ if (cu_dwo_name != tu_dwo_name)
+ continue; // Ignore this entry, the CU DWO doesn't match the TU DWO
+ }
+ }
+ }
// Grab at most one extra parent, subsequent parents are not necessary to
// test equality.
std::optional<llvm::SmallVector<Entry, 4>> parent_chain =
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
index b54dd1162d20ab..6b48ce4eea1875 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
@@ -71,7 +71,8 @@ class DebugNamesDWARFIndex : public DWARFIndex {
: DWARFIndex(module), m_debug_info(dwarf.DebugInfo()),
m_debug_names_data(debug_names_data), m_debug_str_data(debug_str_data),
m_debug_names_up(std::move(debug_names_up)),
- m_fallback(module, dwarf, GetUnits(*m_debug_names_up)) {}
+ m_fallback(module, dwarf, GetUnits(*m_debug_names_up),
+ GetTypeUnitSigs(*m_debug_names_up)) {}
DWARFDebugInfo &m_debug_info;
@@ -84,6 +85,7 @@ class DebugNamesDWARFIndex : public DWARFIndex {
std::unique_ptr<DebugNames> m_debug_names_up;
ManualDWARFIndex m_fallback;
+ DWARFTypeUnit *GetForeignTypeUnit(const DebugNames::Entry &entry) const;
std::optional<DIERef> ToDIERef(const DebugNames::Entry &entry) const;
bool ProcessEntry(const DebugNames::Entry &entry,
llvm::function_ref<bool(DWARFDIE die)> callback);
@@ -97,6 +99,7 @@ class DebugNamesDWARFIndex : public DWARFIndex {
llvm::StringRef name);
static llvm::DenseSet<dw_offset_t> GetUnits(const DebugNames &debug_names);
+ static llvm::DenseSet<uint64_t> GetTypeUnitSigs(const DebugNames &debug_names);
};
} // namespace dwarf
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp b/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
index 92275600f99caa..103e157d3cac59 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -60,8 +60,10 @@ void ManualDWARFIndex::Index() {
}
if (dwp_info && dwp_info->ContainsTypeUnits()) {
for (size_t U = 0; U < dwp_info->GetNumUnits(); ++U) {
- if (auto *tu = llvm::dyn_cast<DWARFTypeUnit>(dwp_info->GetUnitAtIndex(U)))
- units_to_index.push_back(tu);
+ if (auto *tu = llvm::dyn_cast<DWARFTypeUnit>(dwp_info->GetUnitAtIndex(U))) {
+ if (m_type_sigs_to_avoid.count(tu->GetTypeHash()) == 0)
+ units_to_index.push_back(tu);
+ }
}
}
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.h b/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.h
index 0126e587e52d85..3f0bb39dfc20c7 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.h
@@ -21,9 +21,11 @@ class SymbolFileDWARFDwo;
class ManualDWARFIndex : public DWARFIndex {
public:
ManualDWARFIndex(Module &module, SymbolFileDWARF &dwarf,
- llvm::DenseSet<dw_offset_t> units_to_avoid = {})
+ llvm::DenseSet<dw_offset_t> units_to_avoid = {},
+ llvm::DenseSet<uint64_t> type_sigs_to_avoid = {})
: DWARFIndex(module), m_dwarf(&dwarf),
- m_units_to_avoid(std::move(units_to_avoid)) {}
+ m_units_to_avoid(std::move(units_to_avoid)),
+ m_type_sigs_to_avoid(type_sigs_to_avoid) {}
void Preload() override { Index(); }
@@ -170,6 +172,7 @@ class ManualDWARFIndex : public DWARFIndex {
SymbolFileDWARF *m_dwarf;
/// Which dwarf units should we skip while building the index.
llvm::DenseSet<dw_offset_t> m_units_to_avoid;
+ llvm::DenseSet<uint64_t> m_type_sigs_to_avoid;
IndexSet m_set;
bool m_indexed = false;
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 1164bc62682a9a..b828b56cc36606 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -693,7 +693,6 @@ llvm::DWARFDebugAbbrev *SymbolFileDWARF::DebugAbbrev() {
if (debug_abbrev_data.GetByteSize() == 0)
return nullptr;
- ElapsedTime elapsed(m_parse_time);
auto abbr =
std::make_unique<llvm::DWARFDebugAbbrev>(debug_abbrev_data.GetAsLLVM());
llvm::Error error = abbr->parse();
@@ -1726,14 +1725,7 @@ lldb::ModuleSP SymbolFileDWARF::GetExternalModule(ConstString name) {
return pos->second;
}
-DWARFDIE
-SymbolFileDWARF::GetDIE(const DIERef &die_ref) {
- // This method can be called without going through the symbol vendor so we
- // need to lock the module.
- std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
-
- SymbolFileDWARF *symbol_file = nullptr;
-
+SymbolFileDWARF *SymbolFileDWARF::GetDIERefSymbolFile(const DIERef &die_ref) {
// Anytime we get a "lldb::user_id_t" from an lldb_private::SymbolFile API we
// must make sure we use the correct DWARF file when resolving things. On
// MacOSX, when using SymbolFileDWARFDebugMap, we will use multiple
@@ -1741,29 +1733,51 @@ SymbolFileDWARF::GetDIE(const DIERef &die_ref) {
// references to other DWARF objects and we must be ready to receive a
// "lldb::user_id_t" that specifies a DIE from another SymbolFileDWARF
// instance.
+
std::optional<uint32_t> file_index = die_ref.file_index();
+
+ // If the file index matches, then we have the right SymbolFileDWARF already.
+ // This will work for both .dwo file and DWARF in .o files for mac. Also if
+ // both the file indexes are invalid, then we have a match.
+ if (GetFileIndex() == file_index)
+ return this;
+
+ // If we are currently in a .dwo file and our file index doesn't match we need
+ // to let the base symbol file handle this.
+ SymbolFileDWARFDwo *dwo = llvm::dyn_cast_or_null<SymbolFileDWARFDwo>(this);
+ if (dwo)
+ return dwo->GetBaseSymbolFile().GetDIERefSymbolFile(die_ref);
+
if (file_index) {
- if (SymbolFileDWARFDebugMap *debug_map = GetDebugMapSymfile()) {
- symbol_file = debug_map->GetSymbolFileByOSOIndex(*file_index); // OSO case
- if (symbol_file)
- return symbol_file->DebugInfo().GetDIE(die_ref);
- return DWARFDIE();
- }
+ SymbolFileDWARFDebugMap *debug_map = GetDebugMapSymfile();
+ if (debug_map) {
+ // We have a SymbolFileDWARFDebugMap, so let it find the right file
+ return debug_map->GetSymbolFileByOSOIndex(*file_index);
+ } else {
+ // Handle the .dwp file case correctly
+ if (*file_index == DIERef::k_file_index_mask)
+ return GetDwpSymbolFile().get(); // DWP case
- if (*file_index == DIERef::k_file_index_mask)
- symbol_file = GetDwpSymbolFile().get(); // DWP case
- else
- symbol_file = this->DebugInfo()
- .GetUnitAtIndex(*die_ref.file_index())
+ // Handle the .dwo file case correctly
+ return DebugInfo().GetUnitAtIndex(*die_ref.file_index())
->GetDwoSymbolFile(); // DWO case
- } else if (die_ref.die_offset() == DW_INVALID_OFFSET) {
- return DWARFDIE();
+ }
}
+ return this;
+}
- if (symbol_file)
- return symbol_file->GetDIE(die_ref);
+DWARFDIE
+SymbolFileDWARF::GetDIE(const DIERef &die_ref) {
+ if (die_ref.die_offset() == DW_INVALID_OFFSET)
+ return DWARFDIE();
- return DebugInfo().GetDIE(die_ref);
+ // This method can be called without going through the symbol vendor so we
+ // need to lock the module.
+ std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
+ SymbolFileDWARF *symbol_file = GetDIERefSymbolFile(die_ref);
+ if (symbol_file)
+ return symbol_file->DebugInfo().GetDIE(die_ref);
+ return DWARFDIE();
}
/// Return the DW_AT_(GNU_)dwo_id.
@@ -2717,7 +2731,6 @@ void SymbolFileDWARF::FindTypes(const TypeQuery &query, TypeResults &results) {
die_context = die.GetDeclContext();
else
die_context = die.GetTypeLookupContext();
- assert(!die_context.empty());
if (!query.ContextMatches(die_context))
return true; // Keep iterating over index types, context mismatch.
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
index 2f8f80f8765cb8..a3e34278ec9422 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -241,6 +241,15 @@ class SymbolFileDWARF : public SymbolFileCommon {
return m_external_type_modules;
}
+ /// Given a DIERef, find the correct SymbolFileDWARF.
+ ///
+ /// A DIERef contains a file index that can uniquely identify a N_OSO file for
+ /// DWARF in .o files on mac, or a .dwo or .dwp file index for split DWARF.
+ /// Calling this function will find the correct symbol file to use so that
+ /// further lookups can be done on the correct symbol file so that the DIE
+ /// offset makes sense in the DIERef.
+ SymbolFileDWARF *GetDIERefSymbolFile(const DIERef &die_ref);
+
virtual DWARFDIE GetDIE(const DIERef &die_ref);
DWARFDIE GetDIE(lldb::user_id_t uid);
diff --git a/lldb/test/Shell/SymbolFile/DWARF/x86/dwp-foreign-type-units.cpp b/lldb/test/Shell/SymbolFile/DWARF/x86/dwp-foreign-type-units.cpp
new file mode 100644
index 00000000000000..36620591667901
--- /dev/null
+++ b/lldb/test/Shell/SymbolFile/DWARF/x86/dwp-foreign-type-units.cpp
@@ -0,0 +1,91 @@
+// REQUIRES: lld
+
+// This test will make a type that will be compiled differently into two
+// different .dwo files in a type unit with the same type hash, but with
+// differing contents. I have discovered that the hash for the type unit is
+// simply based off of the typename and doesn't seem to differ when the contents
+// differ, so that will help us test foreign type units in the .debug_names
+// section of the main executable. When a DWP file is made, only one type unit
+// will be kept and the type unit that is kept has the .dwo file name that it
+// came from. When LLDB loads the foreign type units, it needs to verify that
+// any entries from foreign type units come from the right .dwo file. We test
+// this since the contents of type units are not always the same even though
+// they have the same type hash. We don't want invalid accelerator table entries
+// to come from one .dwo file and be used on a type unit from another since this
+// could cause invalid lookups to happen. LLDB knows how to track down which
+// .dwo file a type unit comes from by looking at the DW_AT_dwo_name attribute
+// in the DW_TAG_type_unit.
+
+// Now test with DWARF5
+// RUN: %clang -target x86_64-pc-linux -gdwarf-5 -gsplit-dwarf \
+// RUN: -fdebug-types-section -gpubnames -c %s -o %t.main.o
+// RUN: %clang -target x86_64-pc-linux -gdwarf-5 -gsplit-dwarf -DVARIANT \
+// RUN: -fdebug-types-section -gpubnames -c %s -o %t.foo.o
+// RUN: ld.lld %t.main.o %t.foo.o -o %t
+
+// First we check when we make the .dwp file with %t.main.dwo first so it will
+// pick the type unit from %t.main.dwo. Verify we find only the types from
+// %t.main.dwo's type unit.
+// RUN: llvm-dwp %t.main.dwo %t.foo.dwo -o %t.dwp
+// RUN: %lldb \
+// RUN: -o "type lookup IntegerType" \
+// RUN: -o "type lookup FloatType" \
+// RUN: -o "type lookup IntegerType" \
+// RUN: -b %t | FileCheck %s
+// CHECK: (lldb) type lookup IntegerType
+// CHECK-NEXT: int
+// CHECK-NEXT: (lldb) type lookup FloatType
+// CHECK-NEXT: double
+// CHECK-NEXT: (lldb) type lookup IntegerType
+// CHECK-NEXT: int
+
+// Next we check when we make the .dwp file with %t.foo.dwo first so it will
+// pick the type unit from %t.main.dwo. Verify we find only the types from
+// %t.main.dwo's type unit.
+// RUN: llvm-dwp %t.foo.dwo %t.main.dwo -o %t.dwp
+// RUN: %lldb \
+// RUN: -o "type lookup IntegerType" \
+// RUN: -o "type lookup FloatType" \
+// RUN: -o "type lookup IntegerType" \
+// RUN: -b %t | FileCheck %s --check-prefix=VARIANT
+
+// VARIANT: (lldb) type lookup IntegerType
+// VARIANT-NEXT: unsigned int
+// VARIANT-NEXT: (lldb) type lookup FloatType
+// VARIANT-NEXT: float
+// VARIANT-NEXT: (lldb) type lookup IntegerType
+// VARIANT-NEXT: unsigned int
+
+
+// We need to do this so we end with a type unit in each .dwo file and that has
+// the same signature but different contents. When we make the .dwp file, then
+// one of the type units will end up in the .dwp file and we will have
+// .debug_names accelerator tables for both type units and we need to ignore
+// the type units .debug_names entries that don't match the .dwo file whose
+// copy of the type unit ends up in the final .dwp file. To do this, LLDB will
+// look at the type unit and take the DWO name attribute and make sure it
+// matches, and if it doesn't, it will ignore the accelerator table entry.
+struct CustomType {
+ // We switch the order of "FloatType" and "IntegerType" so that if we do
+ // end up reading the wrong accelerator table entry, that we would end up
+ // getting an invalid offset and not find anything, or the offset would have
+ // matched and we would find the wrong thing.
+#ifdef VARIANT
+ typedef float FloatType;
+ typedef unsigned IntegerType;
+#else
+ typedef int IntegerType;
+ typedef double FloatType;
+#endif
+ IntegerType x;
+ FloatType y;
+};
+
+#ifdef VARIANT
+int foo() {
+#else
+int main() {
+#endif
+ CustomType c = {1, 2.0};
+ return 0;
+}
diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h
index f1d4fc72d5a727..2fe33ca6d29c30 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DW...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| @@ -34,6 +34,18 @@ DebugNamesDWARFIndex::Create(Module &module, DWARFDataExtractor debug_names, | |||
| module, std::move(index_up), debug_names, debug_str, dwarf)); | |||
| } | |||
|
|
|||
|
|
|||
| llvm::DenseSet<uint64_t> | |||
| DebugNamesDWARFIndex::GetTypeUnitSigs(const DebugNames &debug_names) { | |||
There was a problem hiding this comment.
nit: Type out the full word GetTypeUnitSignatures for consistency. DebugNames::NameIndex::getForeignTUSignature does this.
| DWARFTypeUnit * | ||
| DebugNamesDWARFIndex::GetForeignTypeUnit(const DebugNames::Entry &entry) const { | ||
| std::optional<uint64_t> type_sig = entry.getForeignTUTypeSignature(); | ||
| if (type_sig) |
There was a problem hiding this comment.
nit: You can merge these lines:
if (std::optional<uint64_t> type_sig = entry.getForeignTUTypeSignature())
| @@ -273,6 +301,44 @@ void DebugNamesDWARFIndex::GetFullyQualifiedType( | |||
| if (!isType(entry.tag())) | |||
| continue; | |||
|
|
|||
|
|
|||
| DWARFTypeUnit *foreign_tu = GetForeignTypeUnit(entry); | |||
| if (foreign_tu) { | |||
There was a problem hiding this comment.
nit: Merge these two lines
if (DWARFTypeUnit *foreign_tu = GetForeignTypeUnit(entry)) {
| dw_offset_t cu_offset = name_index->getCUOffset(0); | ||
| DWARFUnit *cu = m_debug_info.GetUnitAtOffset(DIERef::DebugInfo, | ||
| cu_offset); | ||
| if (cu) { |
| cu_die.GetAttributeValueAsString(DW_AT_dwo_name, nullptr); | ||
| llvm::StringRef tu_dwo_name = | ||
| tu_die.GetAttributeValueAsString(DW_AT_dwo_name, nullptr); | ||
| if (cu_dwo_name != tu_dwo_name) |
There was a problem hiding this comment.
Is it possible for cu_dwo_name and tu_dwo_name to both be empty/invalid?
| @@ -693,7 +693,6 @@ llvm::DWARFDebugAbbrev *SymbolFileDWARF::DebugAbbrev() { | |||
| if (debug_abbrev_data.GetByteSize() == 0) | |||
| return nullptr; | |||
|
|
|||
| ElapsedTime elapsed(m_parse_time); | |||
| // If we are currently in a .dwo file and our file index doesn't match we need | ||
| // to let the base symbol file handle this. | ||
| SymbolFileDWARFDwo *dwo = llvm::dyn_cast_or_null<SymbolFileDWARFDwo>(this); | ||
| if (dwo) |
| return DWARFDIE(); | ||
| } | ||
| SymbolFileDWARFDebugMap *debug_map = GetDebugMapSymfile(); | ||
| if (debug_map) { |
There was a problem hiding this comment.
+1, and also remove the else after return.
| @@ -2717,7 +2731,6 @@ void SymbolFileDWARF::FindTypes(const TypeQuery &query, TypeResults &results) { | |||
| die_context = die.GetDeclContext(); | |||
| else | |||
| die_context = die.GetTypeLookupContext(); | |||
| assert(!die_context.empty()); | |||
There was a problem hiding this comment.
Why remove the assertion? Not saying we should keep it, but why doesn't this hold?
| // RUN: %lldb \ | ||
| // RUN: -o "type lookup IntegerType" \ | ||
| // RUN: -o "type lookup FloatType" \ | ||
| // RUN: -o "type lookup IntegerType" \ |
There was a problem hiding this comment.
Why look up IntegerType twice?
| @@ -58,6 +58,8 @@ class DWARFDebugInfo { | |||
|
|
|||
| const DWARFDebugAranges &GetCompileUnitAranges(); | |||
|
|
|||
| const std::shared_ptr<SymbolFileDWARFDwo> GetDwpSymbolFile(); | |||
There was a problem hiding this comment.
Remove const from by-value return. (it messes with move semantics and some other things) - or was this meant to return by reference? - yeah, I guess this latter, the underlying m_dwarf.GetDwpSymbolFile() seems to return by const reference, so this function probably should too?
| DebugNamesDWARFIndex::GetForeignTypeUnit(const DebugNames::Entry &entry) const { | ||
| std::optional<uint64_t> type_sig = entry.getForeignTUTypeSignature(); | ||
| if (type_sig) | ||
| if (auto dwp_sp = m_debug_info.GetDwpSymbolFile()) |
There was a problem hiding this comment.
this auto should probably be const ref, to avoid inc/dec on the ref counted smart pointer's ref count
| // type unit matches the .dwo file. For this to happen we rely on each | ||
| // .dwo file having its own .debug_names table with a single compile unit | ||
| // and multiple type units. This is the only way we can tell if a type | ||
| // unit came from a specific .dwo file. |
There was a problem hiding this comment.
Sounds like this wouldn't work for a merged .debug_names table? Could you leave a FIXME/do you plan to fix this?
Oh, it also wouldn't work for any kind of LTO which could have multiple CUs in a single object file/dwo file.
(FYI @cmtice )
There was a problem hiding this comment.
I think our conclusion from previous discussions was to put DW_IDX_compile_unit, in addition to the DW_IDX_type_unit on the entries in the .debug_names table - and add the CU column to the .debug_tu_index in the dwp file, to match these things up?
There was a problem hiding this comment.
I think our conclusion from previous discussions was to put
DW_IDX_compile_unit, in addition to theDW_IDX_type_uniton the entries in the .debug_names table - and add the CU column to the .debug_tu_index in the dwp file, to match these things up?
We went with adding comp_dir/name to type unit unit die:
https://discourse.llvm.org/t/debuginfo-dwarfv5-lld-debug-names-with-fdebug-type-sections/73445/29?u=ayermolo
A bit of a shortcut so we don't have to deal with non-standar dwp.
There was a problem hiding this comment.
Sounds like this wouldn't work for a merged
.debug_namestable? Could you leave a FIXME/do you plan to fix this? Oh, it also wouldn't work for any kind of LTO which could have multiple CUs in a single object file/dwo file.(FYI @cmtice )
Does "multiple cus" in .o/dwo actually happen?
From spec perspective I don't understand how this will work. Skeleton CU points to .o/dwo which then has multiple cus? What does it mean?
There was a problem hiding this comment.
Ah, indeed - no, currently there's no well defined way to do LTO (LTO is basically the only time you get multiple CUs in a single .o straight out of the compiler) and Split DWARF (there's that long-standing issue that Cary and I should discuss how that should work & propose something to the DWARF committee, but we haven't as-yet).
I guess even if we did make that work, all the skeleton CUs in the dwo file would have the same DW_AT_dwo_name
| SymbolFileDWARF *symbol_file = GetDIERefSymbolFile(die_ref); | ||
| if (symbol_file) |
There was a problem hiding this comment.
Similar feedback others have given elsewhere, roll the variable declaration into the if condition
| // differing contents. I have discovered that the hash for the type unit is | ||
| // simply based off of the typename and doesn't seem to differ when the contents | ||
| // differ, so that will help us test foreign type units in the .debug_names |
There was a problem hiding this comment.
Rather than the personal statement, could maybe just be explicit:
"Clang's type unit signature is based only on the mangled name of the type, regardless of the contents of the type"
(I can tell you that's how it works - that's how I implemented it - it is a violation of the DWARF spec (the spec says to use a content hash - though that content hash is of the semantic contents, not the syntactic content (eg: if the TU contained a type with a single int member, the spec-compliant hash would be the same whether it was the type DIE followed by the int DIE, or the other way around) - so it would still be the same despite some variations in layout, so the index entries still wouldn't be portable to all matched-hash definitions))
| struct CustomType { | ||
| // We switch the order of "FloatType" and "IntegerType" so that if we do | ||
| // end up reading the wrong accelerator table entry, that we would end up | ||
| // getting an invalid offset and not find anything, or the offset would have | ||
| // matched and we would find the wrong thing. |
There was a problem hiding this comment.
Couldn't we have a difference between these type variants that's simpler - like one version of the struct with a single int member, and one version of the type with no members? Then do lookup of the CustomType and make sure the type we get back is the one with the int member? (or not, depending on the test)
|
@clayborg |
|
All feedback has been addressed and this now supports loading accelerator table entries from .dwo files for foreign type units as well. |
| std::optional<uint64_t> | ||
| DWARFDebugNames::Entry::getForeignTUSkeletonCUOffset() const { | ||
| // Must have a DW_IDX_type_unit and it must be a foreign type unit. | ||
| if (!getForeignTUTypeSignature()) |
There was a problem hiding this comment.
I think according to spec if there is only one TU DW_IDX_type_unit is not needed. Just like if there is only one CU.
There was a problem hiding this comment.
How can you end up with a .debug_names table that contains only one type unit? If this can happen, I can modify getForeignTUTypeSignature() to do the right thing. But file would need to have a single foreign or local TU only, and no CUs at all?
There was a problem hiding this comment.
Spec states:
6.1.1.4.2 List of CUs
The list of CUs immediately follows the header. Each entry in the list is an offset of the corresponding compilation unit in the .debug_info section. In the DWARF-32 format, a section offset is 4 bytes, while in the DWARF-64 format, a section offset is 8 bytes.
The total number of entries in the list is given by comp_unit_count. There must be at least one CU.
There was a problem hiding this comment.
Sorry wasn't clear. I wasn't saying it will have only TU. I was saying it can have one TU (with a CU), at which point the DW_IDX_type_unit is not necessary.
There was a problem hiding this comment.
It is still necessary otherwise we will think it is just the one and only DW_IDX_comp_unit, not the one any only DW_IDX_type_unit...
There was a problem hiding this comment.
I guess I miss understood "Must have a DW_IDX_type_unit". I thought you meant that an entry for foreign type unit must have DW_IDX_type_unit attribute.
| std::optional<uint64_t> unit_offset = entry.getForeignTUSkeletonCUOffset(); | ||
| if (!unit_offset) | ||
| return nullptr; // Return NULL, this is a type unit, but couldn't find it. | ||
| DWARFUnit *cu = | ||
| m_debug_info.GetUnitAtOffset(DIERef::Section::DebugInfo, *unit_offset); | ||
| if (!cu) | ||
| return nullptr; // Return NULL, this is a type unit, but couldn't find it. |
There was a problem hiding this comment.
I think this part could be moved into the common (dwp and non-dwp) code.
| std::optional<uint64_t> cu_offset = entry.getForeignTUSkeletonCUOffset(); | ||
| if (cu_offset) { | ||
| DWARFUnit *cu = m_debug_info.GetUnitAtOffset(DIERef::DebugInfo, *cu_offset); | ||
| if (cu) { |
| /// doesn't match the originating skeleton compile unit's entry | ||
| /// Returns std::nullopt if this entry is not a foreign type unit entry. | ||
| std::optional<DWARFTypeUnit *> | ||
| IsForeignTypeUnit(const DebugNames::Entry &entry) const; |
There was a problem hiding this comment.
Nit: I would expect a function called IsSomething to return bool. Maybe just call it GetForeignTypeUnit ?
| SymbolFileDWARFDwo *dwo = llvm::dyn_cast_or_null<SymbolFileDWARFDwo>(this); | ||
| if (dwo) | ||
| return dwo->GetBaseSymbolFile().GetDIERefSymbolFile(die_ref); |
There was a problem hiding this comment.
A better (and more consistent) way to implement this would be to make GetDIERefSymbolFile virtual, and then have the Dwo implementation do return GetBaseSymbolFile().GetDIERefSymbolFile(die_ref).
|
I think I have addressed all issues. Does anyone have time to review? |
There was a problem hiding this comment.
Most of the patch is very clean, but I'm bothered by the getForeignTUSkeletonCUOffset function, how it opens up with a mostly redundant (the callers checks this already) call to getForeignTUTypeSignature, and then proceeds with a reimplementation of getCUOffset (sans the type unit check). I think we could design a better set of APIs for this, but I'm not sure what those is, because I'm unclear of the meaning of various combinations of DW_IDX unit entries.
What the new function (I think) essentially does is "give me the CU associated with this entry even if the entry does not describe a DIE in this CU" (because DW_IDX_die_offset is relative to a type unit)". I think this API would make sense, even without needing to talk about type units. However, is it actually implementable? For single-CU indexes, that's fine, because we can kind of assume all entries are related to that CU. But what about multi-CU indexes? The only way to determine the CU there is to have an explicit attribute. Are we saying that each entry in a multi-CU index must have a DW_IDX_compile_unit (in addition to a DW_IDX_type_unit)?
If the answer is yes, then this function can be implemented, but then I think the current implementation of getCUOffset (which I interpret as "give me the CU offset IF this entry is relative to that CU") doesn't make sense -- because it treats entries with an explicit DW_IDX_compile_unit different from an implicit/missing DW_IDX_compile_unit. And in this world, we assume that DW_IDX_type_unit takes precedence over DW_IDX_compile_unit -- if both are present then DW_IDX_die_offset is relative to the former. And I'm not sure if we would actually want to take up space by putting both values into the index. Looking at existing producers would be interesting, but I'm not sure if there are any (lld does not merge type unit indexes right now, possibly for this very reason). Maybe one could produce something with LTO?
OTOH, if the answer is no, then the function isn't implementable in the generic case. That doesn't mean you can't implement this feature -- which is useful for guarding against ODR violations (exacerbated by llvm's nonconforming type signature computation algorithm...). However, I think it could be implemented in a much simpler way. For example, lldb could just check if it's looking at a single-CU index, and get the CU offset that way. No extra llvm APIs would be needed.
| // If we are currently in a .dwo file and our file index doesn't match we need | ||
| // to let the base symbol file handle this. | ||
| SymbolFileDWARFDwo *dwo = llvm::dyn_cast_or_null<SymbolFileDWARFDwo>(this); | ||
| if (dwo) | ||
| return dwo->GetDIERefSymbolFile(die_ref); |
There was a problem hiding this comment.
This isn't necessary now that SymbolFileDWARFDwo overrides this function.
There was a problem hiding this comment.
This comment still holds (please remove the dyn_cast and the associated code)
| return DWARFDIE(); | ||
| } | ||
| SymbolFileDWARFDebugMap *debug_map = GetDebugMapSymfile(); | ||
| if (debug_map) { |
There was a problem hiding this comment.
+1, and also remove the else after return.
| // type unit comes from by looking at the DW_AT_dwo_name attribute in the | ||
| // DW_TAG_type_unit. | ||
|
|
||
| // Now test with DWARF5 |
There was a problem hiding this comment.
This line doesn't make sense here. I guess it was copied from a test with multiple flavours.
| // We need to do this so we end with a type unit in each .dwo file and that has | ||
| // the same signature but different contents. When we make the .dwp file, then | ||
| // one of the type units will end up in the .dwp file and we will have | ||
| // .debug_names accelerator tables for both type units and we need to ignore | ||
| // the type units .debug_names entries that don't match the .dwo file whose | ||
| // copy of the type unit ends up in the final .dwp file. To do this, LLDB will | ||
| // look at the type unit and take the DWO name attribute and make sure it | ||
| // matches, and if it doesn't, it will ignore the accelerator table entry. |
There was a problem hiding this comment.
This feels very repetitive. Can you merge this comment with the one at the top. (Optionally, you can also move the source code closer to the top of the file, that one will already know what to expect when one gets to the expectations, if one reads file top to bottom)
| // Returns the the CU offset for a foreign TU. | ||
| // | ||
| // Entries that represent foreign type units can have both a | ||
| // DW_IDX_compile_unit and a DW_IDX_type_unit. In this case the |
There was a problem hiding this comment.
Reading about DW_IDX_compile_unit in a supposedly-generic interface feels a bit out of place. Do these even need to be defined on the interface? AFAICT, the only callers are in DebugNamesDWARFIndex, which already know they are dealing with a debug_names table, and the debug_names entry class already has a bunch of non-virtual methods (hasParentInformation and friends) for debug_names-specific functionality.
BOLT does as I mentioned in: It will put all the CUs/Tus into one one module, thus for foreign TUs both DW_IDX_compile_unit and DW_IDX_type_unit is needed. For LLD I believe this will be in follow up patch. Right now it does it only for CUs. |
I'm sorry, I missed that part. I'm bit of late to the party here.
Right, the CU is necessary to find the DWO file containing the type unit. I forgot about that. That resolves some of my questions. However, I still find the current implementation of
This doesn't seem particularly useful. I think it should either always return CU offset (regardless of whether it's implicit or explicit) or only return it if the DW_IDX_type_unit is not present. And of these two, I think the first one is better, because the caller can always explicitly check for the presence of the type attribute, and it avoids the need to introduce another function (which is needed because we now have a caller who wants the CU offset even though the entry refers to a type unit). From what I can see, that type unit check was added relatively recently (#72952), and it was to support this piece of code in lldb: If we just reversed the order of those checks in lldb, then it wouldn't matter that this function return a CU offset even if the index entry does not refer to a DIE in that CU. Or am I (still) missing something? |
If we have an entry with a So if you just ask an entry for its So this function is named
Yes, but only if the I would rather have had a
It doesn't treat them differently. if you ask an entry for its
LTO and bolt and soon lld will produce these single tables with multiple compile units. And we MUST have the
All of these APIs get the single CU if there is only one for both the case where you only have a So not sure what you are asking for here, but this is the best way I can represent the messy state of the .debug_names tables. Monolithic tables with multiple CUs are here now from BOLT, and will soon be from lld, so we need to be able to handle these. |
|
My previous message reflected my confusion about the usefulness about the DW_IDX_cu+DW_IDX_tu combo. Let me try to start over. The thing I don't like about this patch is the duplication between To see if that's possible, let's consider the possible cases (I'm putting cases with DW_IDX_tu first, as those are the interesting ones here):
These two definitely are the most common (of those including type units) ones, but we can also have:
For completeness, we can also have the cases without type units. I will include only include one here, as I think the others are obvious:
Now that we have that, what are the possible a) "return the value of DW_IDX_cu. Period." If an explicit value is not present return nothing. I don't think anyone wants this, but I'm mentioning it for completeness. With this definition, the function should return the cu offset only in the second and third cases ( Now, the problem I see is that the current implementation of Therefore, my proposal is to redefine the function to do (b), which essentially amounts to deleting these two lines: Besides providing a consistent definition of the function (YMMV, if you can provide an explanation for the current behavior, I'd like to hear it), it would also enable you to reuse it in Changing the definition of course requires going through the call sites and making sure they are okay with that definition. I counted a total of three, one in llvm and two in lldb. The call in llvm (llvm-dwarfdump.cpp) is broken for type units, both before and after this change. Maybe the same goes for one of the calls in lldb (DebugNamesDWARFIndex::GetGlobalVariables -- I'm not sure if you can have global (static class member) variables in a type unit). The last one is definitely broken (and the test suite attests that, but fixing it is trivial: see c5c2a9d. I'll also note that this code was also broken for the case (2) -- a multi CU-index with the accelerator entry referring to a DIE in a (local) type unit, and that that patch fixes that. |
|
Just a reminder: Once we complete the work of adding type units to the merged debug_names index, foreign type unit entries will need (and have) DW_IDX_compile_unit values, even in the case without .dwp files, and will need to be handled properly. So that should be kept in mind while implementing this. |
But we won't want a CU index back if we have a Then we implement a
That is fine with me as long as you agree with the above fix. Let me know if the above fix to |
Can you elaborate on that? I believe it doesn't make sense to you, but I just don't see the logic behind it. In particular I don't understand why it makes sense to return the CU offset when the DW_IDX_compile_unit attribute is explicitly specified (cases 2 and 3), but not when the compile unit is referred to implicitly via the lack of the attribute on a single-cu index (case 1) |
When we have an entry like: And type unit at index 0 is a local type unit, then the entry describes a type unit DIE in the .debug_info of the current executable. The current API we have is in So if we have an entry mentioned above where the type unit lives in the local .debug_info section, we should get an invalid value back from a call to Lets say we do allow an entry with a local type unit to return the first CU from the .debug_names only because there is only one CU in the .debug_names table. Soon Any foreign type units that must be associated with a skeleton compile unit, should have both a |
|
Thank you. I now have a better idea of where you're coming from.
This sounds to me like you'd prefer that getCUOffset behave like the (c) definition from my post above: "return the DW_IDX_cu value, only if the entry describes a DIE within that CU". Do you concur? If so I think mutually acceptable version (the thing I want to avoid the most is the duplicate implementation of DW_IDX_cu retrieval) could be to have flavours of getCUOffset, a higher-level version doing (c), and a lower-level version doing (b), which would be called into from (c). Something like: and ... although I'd prefer to not have that function on the llvm interface, but rather inline it into lldb. How does that sound?
Okay, I can see how that can appear to be inconsistent. However, at the same time, I think it's the most accurate representation of what's in the debug info. If you have a type unit in a multi-CU index without a DW_IDX_cu attribute, then you really cannot tell which CU does it relate to. It doesn't matter if it a local or foreign type unit -- the information is just not there. You may not need this information for local type units, but I don't think that means we should deliberately suppress it -- particularly if it means that the function can be used as building block to implement higher level functionality (like I did with
I'm not sure if this refers to multi-CU indexes or indexes in general, but if it's the former, then I totally agree. (In a single-CU index, I think that connection can be given implicitly through the sole CU in the index) |
|
All issues resolved except the change of |
| /// further lookups can be done on the correct symbol file so that the DIE | ||
| /// offset makes sense in the DIERef. | ||
| virtual SymbolFileDWARF *GetDIERefSymbolFile(const DIERef &die_ref); | ||
|
|
||
| virtual DWARFDIE GetDIE(const DIERef &die_ref); |
There was a problem hiding this comment.
Now GetDIE is virtual and handles SymbolFile resolution, this one doesn't need to be.
| // This method can be called without going through the symbol vendor so we | ||
| // need to lock the module. | ||
| std::lock_guard<std::recursive_mutex> guard(GetModuleMutex()); | ||
| SymbolFileDWARF *symbol_file = GetDIERefSymbolFile(die_ref); |
There was a problem hiding this comment.
I don't think the file index needs to necessarily come from a DIERef. I can imagine other scenarios where it might be interesting to ask for a symbol file, but I think we can deal with it when the time comes.
This patch adds support for the new foreign type unit support in .debug_names. Features include: - don't manually index foreign TUs if we have info for them - only use the type unit entries that match the .dwo files when we have a .dwp file - fix crashers that happen due to PeekDIEName() using wrong offsets
BOLT creates a single .debug_names table where foreign type units have both a DW_IDX_type_unit and a DW_IDX_compile_unit to uniquely identify the .dwo file that the type unit came from.
…rk with new DWARF changes.
- Make GetDIERefSymbolFile() virtual in SymbolFileDWARF and override in SymbolFileDWARFDwo - Rename IsForeignTypeUnit to GetForeignTypeUnit
…#94982) Reverts llvm#94212 Some codes assume that `NDEBUG` implies `LLVM_ENABLE_ABI_BREAKING_CHECKS`, thus llvm#94212 breaks some build bots.
|
An extra character snuck in and messed with the buildbots, fixed with: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/60/builds/803 Here is the relevant piece of the build log for the reference: |
|
It looks like the test is flaky: https://lab.llvm.org/buildbot/#/builders/59/builds/535 It looks like the order of the types is nondeterministic. I think you should be able to use CHECK-DAG along with this trick to match newlines to make the test accept both orderings. |
|
This build got the same failed test on the windows host - https://lab.llvm.org/buildbot/#/builders/141/builds/318 |
What are lldb's guarantees in this regard? Clang/LLVM/etc require nondeterministic output - presumably the same would be valuable to lldb, but I don't know what lldb's precedents are in this regard. |
|
Here is a PR to fix the flaky test: |
It would be useful, but we've never made a serious effort to try to achieve that. |
This patch adds support for the new foreign type unit support in .debug_names. Features include: - don't manually index foreign TUs if we have info for them - only use the type unit entries that match the .dwo files when we have a .dwp file - fix type unit lookups for .dwo files - fix crashers that happen due to PeekDIEName() using wrong offsets where an entry had DW_IDX_comp_unit and DW_IDX_type_unit entries and when we had no type unit support, it would cause us to think it was a normal DIE in .debug_info from the main executable. --------- Co-authored-by: paperchalice <[email protected]>
This patch adds support for the new foreign type unit support in .debug_names. Features include: