[LLVM][DWARF] Add support for monolithic types in .debug_names #70515
Conversation
|
@llvm/pr-subscribers-backend-webassembly @llvm/pr-subscribers-debuginfo Author: Alexander Yermolovich (ayermolo) ChangesEnable Type Units with DWARF5 accelerator tables for monolithic DWARF. Patch is 49.94 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/70515.diff 16 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/AccelTable.h b/llvm/include/llvm/CodeGen/AccelTable.h
index d4e21b2ac8e7ebc..d948b7d82b85979 100644
--- a/llvm/include/llvm/CodeGen/AccelTable.h
+++ b/llvm/include/llvm/CodeGen/AccelTable.h
@@ -16,7 +16,6 @@
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/MapVector.h"
#include "llvm/ADT/STLFunctionalExtras.h"
-#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/BinaryFormat/Dwarf.h"
#include "llvm/CodeGen/DIE.h"
@@ -104,10 +103,13 @@
namespace llvm {
class AsmPrinter;
-class DwarfCompileUnit;
+class DwarfUnit;
class DwarfDebug;
+class DwarfTypeUnit;
class MCSymbol;
class raw_ostream;
+struct TypeUnitMetaInfo;
+using TUVectorTy = SmallVector<TypeUnitMetaInfo, 1>;
/// Interface which the different types of accelerator table data have to
/// conform. It serves as a base class for different values of the template
@@ -197,6 +199,9 @@ template <typename DataT> class AccelTable : public AccelTableBase {
template <typename... Types>
void addName(DwarfStringPoolEntryRef Name, Types &&... Args);
+ void clear() { Entries.clear(); }
+ void addEntries(AccelTable<DataT> &Table);
+ const StringEntries getEntries() const { return Entries; }
};
template <typename AccelTableDataT>
@@ -250,11 +255,21 @@ class AppleAccelTableData : public AccelTableData {
/// emitDWARF5AccelTable function.
class DWARF5AccelTableData : public AccelTableData {
public:
+ struct AttributeEncoding {
+ dwarf::Index Index;
+ dwarf::Form Form;
+ };
static uint32_t hash(StringRef Name) { return caseFoldingDjbHash(Name); }
- DWARF5AccelTableData(const DIE &Die, const DwarfCompileUnit &CU);
- DWARF5AccelTableData(uint64_t DieOffset, unsigned DieTag, unsigned CUIndex)
- : OffsetVal(DieOffset), DieTag(DieTag), UnitID(CUIndex) {}
+ DWARF5AccelTableData(const DIE &Die, const DwarfUnit &CU,
+ const bool IsTU = false);
+ DWARF5AccelTableData(const uint64_t DieOffset, const unsigned DieTag,
+ const unsigned Index, const bool IsTU = false)
+ : OffsetVal(DieOffset) {
+ Data.DieTag = DieTag;
+ Data.UnitID = Index;
+ Data.IsTU = IsTU;
+ }
#ifndef NDEBUG
void print(raw_ostream &OS) const override;
@@ -265,18 +280,25 @@ class DWARF5AccelTableData : public AccelTableData {
"Accessing DIE Offset before normalizing.");
return std::get<uint64_t>(OffsetVal);
}
- unsigned getDieTag() const { return DieTag; }
- unsigned getUnitID() const { return UnitID; }
+ unsigned getDieTag() const { return Data.DieTag; }
+ unsigned getUnitID() const { return Data.UnitID; }
+ bool isTU() const { return Data.IsTU; }
void normalizeDIEToOffset() {
assert(std::holds_alternative<const DIE *>(OffsetVal) &&
"Accessing offset after normalizing.");
OffsetVal = std::get<const DIE *>(OffsetVal)->getOffset();
}
+ bool isNormalized() const {
+ return std::holds_alternative<uint64_t>(OffsetVal);
+ }
protected:
std::variant<const DIE *, uint64_t> OffsetVal;
- unsigned DieTag;
- unsigned UnitID;
+ struct MetaData {
+ uint32_t DieTag : 16;
+ uint32_t UnitID : 15;
+ uint32_t IsTU : 1;
+ } Data;
uint64_t order() const override { return getDieOffset(); }
};
@@ -288,7 +310,19 @@ class DWARF5AccelTable : public AccelTable<DWARF5AccelTableData> {
void convertDieToOffset() {
for (auto &Entry : Entries) {
for (AccelTableData *Value : Entry.second.Values) {
- static_cast<DWARF5AccelTableData *>(Value)->normalizeDIEToOffset();
+ DWARF5AccelTableData *Data = static_cast<DWARF5AccelTableData *>(Value);
+ if (!Data->isNormalized())
+ Data->normalizeDIEToOffset();
+ }
+ }
+ }
+
+ void addTypeEntries(DWARF5AccelTable &Table) {
+ for (auto &Entry : Table.getEntries()) {
+ for (AccelTableData *Value : Entry.second.Values) {
+ DWARF5AccelTableData *Data = static_cast<DWARF5AccelTableData *>(Value);
+ addName(Entry.second.Name, Data->getDieOffset(), Data->getDieTag(),
+ Data->getUnitID(), true);
}
}
}
@@ -310,8 +344,10 @@ void emitAppleAccelTable(AsmPrinter *Asm, AccelTable<DataT> &Contents,
void emitDWARF5AccelTable(AsmPrinter *Asm, DWARF5AccelTable &Contents,
const DwarfDebug &DD,
- ArrayRef<std::unique_ptr<DwarfCompileUnit>> CUs);
-
+ ArrayRef<std::unique_ptr<DwarfCompileUnit>> CUs,
+ TUVectorTy TUSymbols);
+using GetIndexForEntryReturnType =
+ std::optional<std::pair<unsigned, DWARF5AccelTableData::AttributeEncoding>>;
/// Emit a DWARFv5 Accelerator Table consisting of entries in the specified
/// AccelTable. The \p CUs contains either symbols keeping offsets to the
/// start of compilation unit, either offsets to the start of compilation
@@ -319,8 +355,8 @@ void emitDWARF5AccelTable(AsmPrinter *Asm, DWARF5AccelTable &Contents,
void emitDWARF5AccelTable(
AsmPrinter *Asm, DWARF5AccelTable &Contents,
ArrayRef<std::variant<MCSymbol *, uint64_t>> CUs,
- llvm::function_ref<unsigned(const DWARF5AccelTableData &)>
- getCUIndexForEntry);
+ llvm::function_ref<GetIndexForEntryReturnType(const DWARF5AccelTableData &)>
+ getIndexForEntry);
/// Accelerator table data implementation for simple Apple accelerator tables
/// with just a DIE reference.
diff --git a/llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp b/llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp
index e393951744117eb..90715a805cc5e41 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp
@@ -12,6 +12,7 @@
#include "llvm/CodeGen/AccelTable.h"
#include "DwarfCompileUnit.h"
+#include "DwarfUnit.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/Twine.h"
#include "llvm/BinaryFormat/Dwarf.h"
@@ -199,32 +200,30 @@ class Dwarf5AccelTableWriter : public AccelTableWriter {
uint32_t AugmentationStringSize = sizeof(AugmentationString);
char AugmentationString[8] = {'L', 'L', 'V', 'M', '0', '7', '0', '0'};
- Header(uint32_t CompUnitCount, uint32_t BucketCount, uint32_t NameCount)
- : CompUnitCount(CompUnitCount), BucketCount(BucketCount),
- NameCount(NameCount) {}
+ Header(uint32_t CompUnitCount, uint32_t LocalTypeUnitCount,
+ uint32_t BucketCount, uint32_t NameCount)
+ : CompUnitCount(CompUnitCount), LocalTypeUnitCount(LocalTypeUnitCount),
+ BucketCount(BucketCount), NameCount(NameCount) {}
void emit(Dwarf5AccelTableWriter &Ctx);
};
- struct AttributeEncoding {
- dwarf::Index Index;
- dwarf::Form Form;
- };
Header Header;
- DenseMap<uint32_t, SmallVector<AttributeEncoding, 2>> Abbreviations;
+ DenseMap<uint32_t, SmallVector<DWARF5AccelTableData::AttributeEncoding, 2>>
+ Abbreviations;
ArrayRef<std::variant<MCSymbol *, uint64_t>> CompUnits;
- llvm::function_ref<unsigned(const DataT &)> getCUIndexForEntry;
+ ArrayRef<std::variant<MCSymbol *, uint64_t>> TypeUnits;
+ llvm::function_ref<GetIndexForEntryReturnType(const DataT &)>
+ getIndexForEntry;
MCSymbol *ContributionEnd = nullptr;
MCSymbol *AbbrevStart = Asm->createTempSymbol("names_abbrev_start");
MCSymbol *AbbrevEnd = Asm->createTempSymbol("names_abbrev_end");
MCSymbol *EntryPool = Asm->createTempSymbol("names_entries");
- DenseSet<uint32_t> getUniqueTags() const;
-
- // Right now, we emit uniform attributes for all tags.
- SmallVector<AttributeEncoding, 2> getUniformAttributes() const;
+ void populateAbbrevsMap();
void emitCUList() const;
+ void emitTUList() const;
void emitBuckets() const;
void emitStringOffsets() const;
void emitAbbrevs() const;
@@ -235,7 +234,9 @@ class Dwarf5AccelTableWriter : public AccelTableWriter {
Dwarf5AccelTableWriter(
AsmPrinter *Asm, const AccelTableBase &Contents,
ArrayRef<std::variant<MCSymbol *, uint64_t>> CompUnits,
- llvm::function_ref<unsigned(const DataT &)> GetCUIndexForEntry);
+ ArrayRef<std::variant<MCSymbol *, uint64_t>> TypeUnits,
+ llvm::function_ref<GetIndexForEntryReturnType(const DataT &)>
+ getIndexForEntry);
void emit();
};
@@ -358,8 +359,13 @@ void AppleAccelTableWriter::emit() const {
}
DWARF5AccelTableData::DWARF5AccelTableData(const DIE &Die,
- const DwarfCompileUnit &CU)
- : OffsetVal(&Die), DieTag(Die.getTag()), UnitID(CU.getUniqueID()) {}
+ const DwarfUnit &Unit,
+ const bool IsTU)
+ : OffsetVal(&Die) {
+ Data.DieTag = Die.getTag();
+ Data.UnitID = Unit.getUniqueID();
+ Data.IsTU = IsTU;
+}
template <typename DataT>
void Dwarf5AccelTableWriter<DataT>::Header::emit(Dwarf5AccelTableWriter &Ctx) {
@@ -391,31 +397,39 @@ void Dwarf5AccelTableWriter<DataT>::Header::emit(Dwarf5AccelTableWriter &Ctx) {
Asm->OutStreamer->emitBytes({AugmentationString, AugmentationStringSize});
}
+static uint32_t constexpr LowerBitSize = dwarf::DW_IDX_type_hash;
+static uint32_t getTagFromAbbreviationTag(const uint32_t AbbrvTag) {
+ return AbbrvTag >> LowerBitSize;
+}
+static uint32_t
+constructAbbreviationTag(const unsigned Tag,
+ const GetIndexForEntryReturnType &EntryRet) {
+ uint32_t AbbrvTag = 0;
+ if (EntryRet)
+ AbbrvTag |= 1 << EntryRet->second.Index;
+ AbbrvTag |= 1 << dwarf::DW_IDX_die_offset;
+ AbbrvTag |= Tag << LowerBitSize;
+ return AbbrvTag;
+}
template <typename DataT>
-DenseSet<uint32_t> Dwarf5AccelTableWriter<DataT>::getUniqueTags() const {
- DenseSet<uint32_t> UniqueTags;
+void Dwarf5AccelTableWriter<DataT>::populateAbbrevsMap() {
for (auto &Bucket : Contents.getBuckets()) {
for (auto *Hash : Bucket) {
for (auto *Value : Hash->Values) {
+ GetIndexForEntryReturnType EntryRet =
+ getIndexForEntry(*static_cast<const DataT *>(Value));
unsigned Tag = static_cast<const DataT *>(Value)->getDieTag();
- UniqueTags.insert(Tag);
+ uint32_t AbbrvTag = constructAbbreviationTag(Tag, EntryRet);
+ if (Abbreviations.count(AbbrvTag) == 0) {
+ SmallVector<DWARF5AccelTableData::AttributeEncoding, 2> UA;
+ if (EntryRet)
+ UA.push_back(EntryRet->second);
+ UA.push_back({dwarf::DW_IDX_die_offset, dwarf::DW_FORM_ref4});
+ Abbreviations.try_emplace(AbbrvTag, UA);
+ }
}
}
}
- return UniqueTags;
-}
-
-template <typename DataT>
-SmallVector<typename Dwarf5AccelTableWriter<DataT>::AttributeEncoding, 2>
-Dwarf5AccelTableWriter<DataT>::getUniformAttributes() const {
- SmallVector<AttributeEncoding, 2> UA;
- if (CompUnits.size() > 1) {
- size_t LargestCUIndex = CompUnits.size() - 1;
- dwarf::Form Form = DIEInteger::BestForm(/*IsSigned*/ false, LargestCUIndex);
- UA.push_back({dwarf::DW_IDX_compile_unit, Form});
- }
- UA.push_back({dwarf::DW_IDX_die_offset, dwarf::DW_FORM_ref4});
- return UA;
}
template <typename DataT>
@@ -429,6 +443,17 @@ void Dwarf5AccelTableWriter<DataT>::emitCUList() const {
}
}
+template <typename DataT>
+void Dwarf5AccelTableWriter<DataT>::emitTUList() const {
+ for (const auto &TU : enumerate(TypeUnits)) {
+ Asm->OutStreamer->AddComment("Type unit " + Twine(TU.index()));
+ if (std::holds_alternative<MCSymbol *>(TU.value()))
+ Asm->emitDwarfSymbolReference(std::get<MCSymbol *>(TU.value()));
+ else
+ Asm->emitDwarfLengthOrOffset(std::get<uint64_t>(TU.value()));
+ }
+}
+
template <typename DataT>
void Dwarf5AccelTableWriter<DataT>::emitBuckets() const {
uint32_t Index = 1;
@@ -456,10 +481,11 @@ void Dwarf5AccelTableWriter<DataT>::emitAbbrevs() const {
Asm->OutStreamer->emitLabel(AbbrevStart);
for (const auto &Abbrev : Abbreviations) {
Asm->OutStreamer->AddComment("Abbrev code");
- assert(Abbrev.first != 0);
- Asm->emitULEB128(Abbrev.first);
- Asm->OutStreamer->AddComment(dwarf::TagString(Abbrev.first));
+ uint32_t Tag = getTagFromAbbreviationTag(Abbrev.first);
+ assert(Tag != 0);
Asm->emitULEB128(Abbrev.first);
+ Asm->OutStreamer->AddComment(dwarf::TagString(Tag));
+ Asm->emitULEB128(Tag);
for (const auto &AttrEnc : Abbrev.second) {
Asm->emitULEB128(AttrEnc.Index, dwarf::IndexString(AttrEnc.Index).data());
Asm->emitULEB128(AttrEnc.Form,
@@ -474,16 +500,21 @@ void Dwarf5AccelTableWriter<DataT>::emitAbbrevs() const {
template <typename DataT>
void Dwarf5AccelTableWriter<DataT>::emitEntry(const DataT &Entry) const {
- auto AbbrevIt = Abbreviations.find(Entry.getDieTag());
+ GetIndexForEntryReturnType EntryRet = getIndexForEntry(Entry);
+ uint32_t AbbrvTag = constructAbbreviationTag(Entry.getDieTag(), EntryRet);
+ auto AbbrevIt = Abbreviations.find(AbbrvTag);
assert(AbbrevIt != Abbreviations.end() &&
"Why wasn't this abbrev generated?");
-
+ assert(getTagFromAbbreviationTag(AbbrevIt->first) == Entry.getDieTag() &&
+ "Invalid Tag");
Asm->emitULEB128(AbbrevIt->first, "Abbreviation code");
+
for (const auto &AttrEnc : AbbrevIt->second) {
Asm->OutStreamer->AddComment(dwarf::IndexString(AttrEnc.Index));
switch (AttrEnc.Index) {
- case dwarf::DW_IDX_compile_unit: {
- DIEInteger ID(getCUIndexForEntry(Entry));
+ case dwarf::DW_IDX_compile_unit:
+ case dwarf::DW_IDX_type_unit: {
+ DIEInteger ID(EntryRet->first);
ID.emitValue(Asm, AttrEnc.Form);
break;
}
@@ -515,22 +546,21 @@ template <typename DataT>
Dwarf5AccelTableWriter<DataT>::Dwarf5AccelTableWriter(
AsmPrinter *Asm, const AccelTableBase &Contents,
ArrayRef<std::variant<MCSymbol *, uint64_t>> CompUnits,
- llvm::function_ref<unsigned(const DataT &)> getCUIndexForEntry)
+ ArrayRef<std::variant<MCSymbol *, uint64_t>> TypeUnits,
+ llvm::function_ref<GetIndexForEntryReturnType(const DataT &)>
+ getIndexForEntry)
: AccelTableWriter(Asm, Contents, false),
- Header(CompUnits.size(), Contents.getBucketCount(),
+ Header(CompUnits.size(), TypeUnits.size(), Contents.getBucketCount(),
Contents.getUniqueNameCount()),
- CompUnits(CompUnits), getCUIndexForEntry(std::move(getCUIndexForEntry)) {
- DenseSet<uint32_t> UniqueTags = getUniqueTags();
- SmallVector<AttributeEncoding, 2> UniformAttributes = getUniformAttributes();
-
- Abbreviations.reserve(UniqueTags.size());
- for (uint32_t Tag : UniqueTags)
- Abbreviations.try_emplace(Tag, UniformAttributes);
+ CompUnits(CompUnits), TypeUnits(TypeUnits),
+ getIndexForEntry(std::move(getIndexForEntry)) {
+ populateAbbrevsMap();
}
template <typename DataT> void Dwarf5AccelTableWriter<DataT>::emit() {
Header.emit(*this);
emitCUList();
+ emitTUList();
emitBuckets();
emitHashes();
emitStringOffsets();
@@ -548,12 +578,16 @@ void llvm::emitAppleAccelTableImpl(AsmPrinter *Asm, AccelTableBase &Contents,
AppleAccelTableWriter(Asm, Contents, Atoms, SecBegin).emit();
}
-void llvm::emitDWARF5AccelTable(
- AsmPrinter *Asm, DWARF5AccelTable &Contents, const DwarfDebug &DD,
- ArrayRef<std::unique_ptr<DwarfCompileUnit>> CUs) {
+void llvm::emitDWARF5AccelTable(AsmPrinter *Asm, DWARF5AccelTable &Contents,
+ const DwarfDebug &DD,
+ ArrayRef<std::unique_ptr<DwarfCompileUnit>> CUs,
+ TUVectorTy TUSymbols) {
std::vector<std::variant<MCSymbol *, uint64_t>> CompUnits;
+ std::vector<std::variant<MCSymbol *, uint64_t>> TypeUnits;
SmallVector<unsigned, 1> CUIndex(CUs.size());
- int Count = 0;
+ DenseMap<unsigned, unsigned> TUIndex(TUSymbols.size());
+ int CUCount = 0;
+ int TUCount = 0;
for (const auto &CU : enumerate(CUs)) {
switch (CU.value()->getCUNode()->getNameTableKind()) {
case DICompileUnit::DebugNameTableKind::Default:
@@ -562,13 +596,18 @@ void llvm::emitDWARF5AccelTable(
default:
continue;
}
- CUIndex[CU.index()] = Count++;
+ CUIndex[CU.index()] = CUCount++;
assert(CU.index() == CU.value()->getUniqueID());
const DwarfCompileUnit *MainCU =
DD.useSplitDwarf() ? CU.value()->getSkeleton() : CU.value().get();
CompUnits.push_back(MainCU->getLabelBegin());
}
+ for (const auto &TU : TUSymbols) {
+ TUIndex[TU.UniqueID] = TUCount++;
+ TypeUnits.push_back(TU.Label);
+ }
+
if (CompUnits.empty())
return;
@@ -576,10 +615,21 @@ void llvm::emitDWARF5AccelTable(
Asm->getObjFileLowering().getDwarfDebugNamesSection());
Contents.finalize(Asm, "names");
+ dwarf::Form CUIndexForm =
+ DIEInteger::BestForm(/*IsSigned*/ false, CompUnits.size() - 1);
+ dwarf::Form TUIndexForm =
+ DIEInteger::BestForm(/*IsSigned*/ false, TypeUnits.size() - 1);
Dwarf5AccelTableWriter<DWARF5AccelTableData>(
- Asm, Contents, CompUnits,
- [&](const DWARF5AccelTableData &Entry) {
- return CUIndex[Entry.getUnitID()];
+ Asm, Contents, CompUnits, TypeUnits,
+ [&](const DWARF5AccelTableData &Entry) -> GetIndexForEntryReturnType {
+ GetIndexForEntryReturnType Index = std::nullopt;
+ if (Entry.isTU())
+ Index = {TUIndex[Entry.getUnitID()],
+ {dwarf::DW_IDX_type_unit, TUIndexForm}};
+ else if (CUIndex.size() > 1)
+ Index = {CUIndex[Entry.getUnitID()],
+ {dwarf::DW_IDX_compile_unit, CUIndexForm}};
+ return Index;
})
.emit();
}
@@ -587,11 +637,12 @@ void llvm::emitDWARF5AccelTable(
void llvm::emitDWARF5AccelTable(
AsmPrinter *Asm, DWARF5AccelTable &Contents,
ArrayRef<std::variant<MCSymbol *, uint64_t>> CUs,
- llvm::function_ref<unsigned(const DWARF5AccelTableData &)>
- getCUIndexForEntry) {
+ llvm::function_ref<GetIndexForEntryReturnType(const DWARF5AccelTableData &)>
+ getIndexForEntry) {
+ std::vector<std::variant<MCSymbol *, uint64_t>> TypeUnits;
Contents.finalize(Asm, "names");
- Dwarf5AccelTableWriter<DWARF5AccelTableData>(Asm, Contents, CUs,
- getCUIndexForEntry)
+ Dwarf5AccelTableWriter<DWARF5AccelTableData>(Asm, Contents, CUs, TypeUnits,
+ getIndexForEntry)
.emit();
}
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
index 453dcc1c90bbf49..70dbd8e17c933ed 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
@@ -151,7 +151,7 @@ class DwarfCompileUnit final : public DwarfUnit {
UnitKind Kind = UnitKind::Full);
bool hasRangeLists() const { return HasRangeLists; }
- unsigned getUniqueID() const { return UniqueID; }
+ unsigned getUniqueID() const override { return UniqueID; }
DwarfCompileUnit *getSkeleton() const {
return Skeleton;
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index 61ac492a9097063..3923ad61d825c8b 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -305,6 +305,7 @@ void Loc::MMI::addFrameIndexExpr(const DIExpression *Expr, int FI) {
static AccelTableKind computeAccelTableKind(unsigned DwarfVersion,
bool GenerateTypeUnits,
+ bool HasSplitDwarf,
DebuggerKind Tuning,
const Triple &TT) {
// Honor an explicit request.
@@ -312,7 +313,8 @@ static AccelTableKind computeAccelTableKind(unsigned DwarfVersion,
return AccelTables;
// Accelerator tables with type units are currently not supported.
- if (GenerateTypeUnits)
+ if (GenerateTypeUnits &&
+ (DwarfVersion < 5 || HasSplitDwarf || !TT.isOSBinFormatELF()))
return AccelTableKind::None;
// Accelerator tables get emitted if targetting DWARF v5 or LLDB. DWARF v5
@@ -325,6 +327,9 @@ static AccelTableKind computeAccelTableKind(unsigned DwarfVersion,
: AccelTableKind::Dwarf;
return AccelTableKind::None;
}
+void DwarfDebug::addTypeUnitSymbol(DwarfTypeUnit &U) {
+ InfoHolder.addTypeUnitSymbol(U);
+}
DwarfDebug::DwarfDebug(AsmPrinter *A)
: DebugHandlerBase(A), DebugLocs(A->OutStreamer->isV...
[truncated]
|
| // Accelerator tables with type units are currently not supported. | ||
| if (GenerateTypeUnits) | ||
| if (GenerateTypeUnits && | ||
| (DwarfVersion < 5 || HasSplitDwarf || !TT.isOSBinFormatELF())) |
There was a problem hiding this comment.
Comment probably needs updating - and I'm not sure I understand why these three conditions are the right ones - so maybe the comment will help me understand better what's being added/handled here.
There was a problem hiding this comment.
Helpful, but I'm still curious why this is only supported for ELF at the moment.
| : OffsetVal(&Die), DieTag(Die.getTag()), UnitID(Unit.getUniqueID()), | ||
| IsTU(IsTU) {} |
There was a problem hiding this comment.
Not sure if it'd be better or worse, but this could use a forwarding ctor call here
There was a problem hiding this comment.
Sorry, not sure I follow.
There was a problem hiding this comment.
Ah, sorry, used the wrong term - /delegating/ ctor was what I was thinking of. This ctor could call the other "init all the members" ctor, rather than initing them directly itself. It's much of a muchness though, since they don't do anything als that interesting anyway.
There was a problem hiding this comment.
I don't think it can. Unless I am missing something. When this constructor is called the DIE offset is not known yet. I guess after the call we can over-write OffsetVal with Die again?
| dwarf::Form Form = DIEInteger::BestForm(/*IsSigned*/ false, | ||
| (uint64_t)UniqueIdToCuMap.size() - 1); | ||
| /// llvm-dwarfutil doesn't support type units + .debug_names right now anyway, | ||
| /// so just keeping current behavior. |
There was a problem hiding this comment.
"keeping current behavior" describes the patch, rather than the static state of the code - so this comment seems like it might be confusing. Could this be a statement of the current state of the system instead? ("FIXME: add support for type units + .debug_names. For now the behavior is " (ignores/rejects/corrupts/etc))
There was a problem hiding this comment.
updated. Something like that?
There was a problem hiding this comment.
I'm still a bit confused by "keeping the current behavior" - it still sounds like a description of the patch, not of the state of the code. The "current behavior" is sort of tautological - the code is the current behavior, by definition/always?
I'm also not sure what this is mean to say. So what /would/ you be changing here if dwarfutil did support type units + .debug_names? What is it that is staying the same/isn't changing in this code, that would/should be changed?
There was a problem hiding this comment.
How does current wording sound?
/// llvm-dwarfutil doesn't support type units + .debug_names right now.
// FIXME: add support for type units + .debug_names. For now the behavior is
// unsuported.
| if (getCurrentAccelTableKind() == DWARF5AccelTableKind::CU) { | ||
| DwarfCompileUnit *Unit = CUMap.lookup(&CU); | ||
| Current.addName(Ref, Die, *Unit); | ||
| } else { | ||
| DwarfTypeUnit *Unit = TypeUnitsUnderConstruction.back().first.get(); | ||
| Current.addName(Ref, Die, *Unit); | ||
| } |
There was a problem hiding this comment.
Could the unit or its index be passed in from the caller? Rather than special casing this in here? (especially the TypeUnitsUnderConstruction.back().first.get() seems pretty subtle - we might not be emitting the most recent type unit under construction, right?)
There was a problem hiding this comment.
In theory.... It means plumbing it through few layers of API calls.
The most recent one under construction is always pushed to the back. Not sure I follow.
There was a problem hiding this comment.
We might not be adding a name from the most recent one under construction (eg: one type unit leads to creating another type unit, but then the second type unit is completed, but the first isn't complete & so we add another name and might that accidentally get associated with the second type unit instead of the first?)
There was a problem hiding this comment.
OK, plumbed through DwarfUnit and TableKind. Side affect I think it also simplifies things because we don't need to use CUMap.
|
@dwblaikie Any other changers I need to make to move this PR forward and then start on the next phase (split-dwarf)? :) |
Yeah, sorry for the delay - working on another round of review feedback. |
| dwarf::Form Form = DIEInteger::BestForm(/*IsSigned*/ false, | ||
| (uint64_t)UniqueIdToCuMap.size() - 1); | ||
| /// llvm-dwarfutil doesn't support type units + .debug_names right now anyway, | ||
| /// so just keeping current behavior. |
There was a problem hiding this comment.
I'm still a bit confused by "keeping the current behavior" - it still sounds like a description of the patch, not of the state of the code. The "current behavior" is sort of tautological - the code is the current behavior, by definition/always?
I'm also not sure what this is mean to say. So what /would/ you be changing here if dwarfutil did support type units + .debug_names? What is it that is staying the same/isn't changing in this code, that would/should be changed?
| // For TU we normalize as each Unit is emitted. | ||
| // So when this is invoked after CU construction we will be in mixed | ||
| // state. | ||
| if (!Data->isNormalized()) |
There was a problem hiding this comment.
We could check that it's only already normalized if it's a TU? (we shouldn't expect to see any prenormalized CU entries, yeah?)
There was a problem hiding this comment.
Not sure I follow. This function is invoked when we write out finished type units on AccelTypeUnitsDebugNames. Which only has not-normalized TUs. It is also invoked in ::finalizeModuleInfo. At which point it will have normalized TU entries and not-normalized CU entries.
Are you saying we should expose IsTU() and check that?
There was a problem hiding this comment.
Not sure about "exposed" - I think it's already accessible from Data->IsTU() here? And yeah, something like assert(!Data->isNormalized || Data->isTU())
There was a problem hiding this comment.
Ah IC.
So when we normalize in CU it will be in mix state. The TU DIEs will be normalized because we do it as we write them out. The CU DIEs will not be. Without the check it will runtime assert in:
(without explicit assert somewhere in the std::get).
void normalizeDIEToOffset() {
assert(std::holds_alternative<const DIE *>(OffsetVal) &&
"Accessing offset after normalizing.");
OffsetVal = std::get<const DIE *>(OffsetVal)->getOffset();
}
| // Accelerator tables with type units are currently not supported. | ||
| if (GenerateTypeUnits) | ||
| if (GenerateTypeUnits && | ||
| (DwarfVersion < 5 || HasSplitDwarf || !TT.isOSBinFormatELF())) |
There was a problem hiding this comment.
Helpful, but I'm still curious why this is only supported for ELF at the moment.
| : OffsetVal(&Die), DieTag(Die.getTag()), UnitID(Unit.getUniqueID()), | ||
| IsTU(IsTU) {} |
There was a problem hiding this comment.
Ah, sorry, used the wrong term - /delegating/ ctor was what I was thinking of. This ctor could call the other "init all the members" ctor, rather than initing them directly itself. It's much of a muchness though, since they don't do anything als that interesting anyway.
| if (getCurrentAccelTableKind() == DWARF5AccelTableKind::CU) { | ||
| DwarfCompileUnit *Unit = CUMap.lookup(&CU); | ||
| Current.addName(Ref, Die, *Unit); | ||
| } else { | ||
| DwarfTypeUnit *Unit = TypeUnitsUnderConstruction.back().first.get(); | ||
| Current.addName(Ref, Die, *Unit); | ||
| } |
There was a problem hiding this comment.
We might not be adding a name from the most recent one under construction (eg: one type unit leads to creating another type unit, but then the second type unit is completed, but the first isn't complete & so we add another name and might that accidentally get associated with the second type unit instead of the first?)
| if (getCurrentAccelTableKind() == DWARF5AccelTableKind::CU) { | ||
| DwarfCompileUnit *Unit = CUMap.lookup(&CU); | ||
| Current.addName(Ref, Die, *Unit); | ||
| } else { | ||
| DwarfTypeUnit *Unit = TypeUnitsUnderConstruction.back().first.get(); | ||
| Current.addName(Ref, Die, *Unit); | ||
| } |
There was a problem hiding this comment.
OK, plumbed through DwarfUnit and TableKind. Side affect I think it also simplifies things because we don't need to use CUMap.
| // For TU we normalize as each Unit is emitted. | ||
| // So when this is invoked after CU construction we will be in mixed | ||
| // state. | ||
| if (!Data->isNormalized()) |
There was a problem hiding this comment.
Not sure I follow. This function is invoked when we write out finished type units on AccelTypeUnitsDebugNames. Which only has not-normalized TUs. It is also invoked in ::finalizeModuleInfo. At which point it will have normalized TU entries and not-normalized CU entries.
Are you saying we should expose IsTU() and check that?
| : OffsetVal(&Die), DieTag(Die.getTag()), UnitID(Unit.getUniqueID()), | ||
| IsTU(IsTU) {} |
There was a problem hiding this comment.
I don't think it can. Unless I am missing something. When this constructor is called the DIE offset is not known yet. I guess after the call we can over-write OffsetVal with Die again?
| dwarf::Form Form = DIEInteger::BestForm(/*IsSigned*/ false, | ||
| (uint64_t)UniqueIdToCuMap.size() - 1); | ||
| /// llvm-dwarfutil doesn't support type units + .debug_names right now anyway, | ||
| /// so just keeping current behavior. |
There was a problem hiding this comment.
How does current wording sound?
/// llvm-dwarfutil doesn't support type units + .debug_names right now.
// FIXME: add support for type units + .debug_names. For now the behavior is
// unsuported.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Looks like at some point I need to rebase mine on top of #71724 to make sure everything still works. |
Probably easier to keep reviewing as-is, though, yeah, you can experiment locally to see if there's anything interesting there you might need to deal with once we're done with most of the review here. |
|
(if we don't get this perfect by Friday, it's probably close enough I'm happy to approve and I can do some follow-ups post-commit, FWIW) |
I know very little about If you want to approve and land this and do follow-ups, it this looks great to me! I just need something I can play with to better review the lld/ELF patch. (As we now need |
Not sure I follow - certainly before it's pushed it should be squashed, but during review I think we're still encouraging folks to push additional fixup commits to make it easy to see how different feedback has been addressed & I think it lowers the risk of previous comments going missing. ( https://llvm.org/docs/GitHub.html#updating-pull-requests ) |
Personally I think the commit messages have somewhat lost its value to describe what has been changed between commits as the commit message includes "fixup! fixup! fixup! ... " but not what the change is about. I could click "Commits" and then click the individual commit - and I need to do it 10 times with relatively little information for each click. At this point, I think a rebase isn't that bad, especially for new reviewers who want to look at this change. That said, you have been reviewing this patch so far (thanks!) and I am happy that your opinion takes precedence. |
Yeah, agreed - we should be encouraging/requiring/asking folks to write meaningful incremental commit messages in PRs (same situation existing in phab too, though, yeah? Though at least I think Phab didn't prepopulate a description - but it also didn't really promote the description of the incremental changes - you mostly just viewed them in the "history" tab, and often there was no description at all, I think?)
Fair enough -perhaps a suggestion to change the docs then, might be useful? Though I still do find the individual history relevant/useful as I did with Phab, even in the absence of detailed/specific descriptions of each change.
But you can look at the whole change in the "Files Changed" tab, right, if you want to see it squashed together, right?
Sure enough, though good to have consistency/documenting a best practice, etc. |
|
Ah yeah. I kind of viewed this whole fixup business as deficiency in github not to allow rebase. |
Eh, like on Phab, I think it's handy to be able to see the incremental progress of the review. If someone's already reviewed a patch, they might benefit from being able to see the differences only from the last update to the previous, rather than the whole thing. Giving the steps names/descriptions is helpful too, helps to know what to look for in the changes, etc. |
ok, thx. Let me rebase, and I'll push once it's all green. Allthe fixups will go away on squash. |
https://discourse.llvm.org/t/force-push-and-rebase/73766/13 I think the foremost problem of GitHub UI is that the "compare button" is not that useful for a rebase-to-upstream workflow. If you amend commits without rebase, the "compare button" is good. But in cases when you merge upstream changes into the branch, the adjustment in the merge commit itself is also not good. In any case, making a comment after an update will be useful. |
|
Question I have is when rebase is needed (for example to fix merge conflicts, what should happen? |
ayermolo
force-pushed
the
typesDebugNamesMonolithic
branch
from
17d9825
to
f937bfb
Compare
Yes, as per the docs: https://llvm.org/docs/GitHub.html#landing-your-change |
Enable Type Units with DWARF5 accelerator tables for monolithic DWARF. Implementation relies on linker to tombstone offset in LocalTU list to -1 when it deduplciates type units using COMDAT.
ayermolo
force-pushed
the
typesDebugNamesMonolithic
branch
from
f937bfb
to
0038f21
Compare
I meant just in general if review is not finished, but good to know about that also. I have just been relying on UI squash/merge, and then edit commit message. P.S. Ugh missed couple of your comments from couple of days ago. For some reason I don't get reliable email notifications though github unlike from phabricator. I can follow up on those when I am back from PTO? |
…70515) Enable Type Units with DWARF5 accelerator tables for monolithic DWARF. Implementation relies on linker to tombstone offset in LocalTU list to -1 when it deduplciates type units using COMDAT.
…70515) Enable Type Units with DWARF5 accelerator tables for monolithic DWARF. Implementation relies on linker to tombstone offset in LocalTU list to -1 when it deduplciates type units using COMDAT.
This is a follow up patch after .debug_names can now emit local type unit entries when we compile with type units + DWARF5 + .debug_names. The pull request that added this functionality was: #70515 This patch makes sure that the DebugNamesDWARFIndex in LLDB will not manually need to parse type units if they have a valid index. It also fixes the index to be able to correctly extract name entries that reference type unit DIEs. Added a test to verify things work as expected.
This is a follow up patch after .debug_names can now emit local type unit entries when we compile with type units + DWARF5 + .debug_names. The pull request that added this functionality was: llvm#70515 This patch makes sure that the DebugNamesDWARFIndex in LLDB will not manually need to parse type units if they have a valid index. It also fixes the index to be able to correctly extract name entries that reference type unit DIEs. Added a test to verify things work as expected.
|
@dwblaikie Which comments here would you like me to follow up on? |
This is a follow up patch after .debug_names can now emit local type unit entries when we compile with type units + DWARF5 + .debug_names. The pull request that added this functionality was: llvm#70515 This patch makes sure that the DebugNamesDWARFIndex in LLDB will not manually need to parse type units if they have a valid index. It also fixes the index to be able to correctly extract name entries that reference type unit DIEs. Added a test to verify things work as expected.
Enable Type Units with DWARF5 accelerator tables for monolithic DWARF.
Implementation relies on linker to tombstone offset in LocalTU list to -1 when
it deduplciates type units using COMDAT.