[llvm] 0af7d97 - [AppleAccelTable][NFC] Refactor iterator class
Felipe de Azevedo Piovezan via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 9 09:42:10 PDT 2023
Author: Felipe de Azevedo Piovezan
Date: 2023-06-09T12:40:08-04:00
New Revision: 0af7d97fb55380c149f7282add452e8eb1a5f6e8
URL: https://github.com/llvm/llvm-project/commit/0af7d97fb55380c149f7282add452e8eb1a5f6e8
DIFF: https://github.com/llvm/llvm-project/commit/0af7d97fb55380c149f7282add452e8eb1a5f6e8.diff
LOG: [AppleAccelTable][NFC] Refactor iterator class
The current implementation of the AppleAcceleratorTable::Entry is problematic
for a few reasons:
1. It is heavyweight. Iterators should be cheap, but the current implementation
tracks 3 different integer values, one "Entry" object, and one pointer to the
actual accelerator table. Most of this information is redundant and can be
removed.
2. It performs "memory reads" outside of the dereference operation. This
violates the usual expectations of iterators, whereby we don't access anything
so long as we don't dereference the iterator.
3. It doesn't commit to tracking _one_ thing only. It tries to track both an
"index" into a list of HashData entries and a pointer in a blob of data. For
this reason, it allows for multiple "invalid" states, keeps redundant
information around and is difficult to understand.
4. It couples the interpretation of the data with the iterator increment. As
such, if the *interpretation* fails, the iterator will keep on producing garbage
values without ever indicating so to consumers.
The problem this iterator is trying to solve is simple: we have a blob of data
containing many "HashData" entries and we want to iterate over them. As such,
this commit makes the iterator only track a pointer over that data, and it
decouples the iterator increments from the interpretation of this blob of data.
We maintain the already existing assumption that failures never happen, but now
make it explicit with an assert.
Depends on D152158
Differential Revision: https://reviews.llvm.org/D152159
Added:
Modified:
llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h
llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
Removed:
################################################################################
diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h
index c85fca4f50b5e..a19a0fc484018 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h
+++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h
@@ -189,12 +189,10 @@ class AppleAcceleratorTable : public DWARFAcceleratorTable {
public:
/// Apple-specific implementation of an Accelerator Entry.
class Entry final : public DWARFAcceleratorTable::Entry {
- const HeaderData *HdrData = nullptr;
+ const AppleAcceleratorTable &Table;
- Entry(const HeaderData &Data);
- Entry() = default;
-
- void extract(const AppleAcceleratorTable &AccelTable, uint64_t *Offset);
+ Entry(const AppleAcceleratorTable &Table);
+ void extract(uint64_t *Offset);
public:
std::optional<uint64_t> getCUOffset() const override;
@@ -215,37 +213,25 @@ class AppleAcceleratorTable : public DWARFAcceleratorTable {
friend class ValueIterator;
};
+ /// An iterator for Entries all having the same string as key.
class ValueIterator {
- const AppleAcceleratorTable *AccelTable = nullptr;
- Entry Current; ///< The current entry.
- uint64_t DataOffset = 0; ///< Offset into the section.
- unsigned Data = 0; ///< Current data entry.
- unsigned NumData = 0; ///< Number of data entries.
+ Entry Current;
+ uint64_t Offset = 0;
- /// Advance the iterator.
- void Next();
+ void Next() { Offset += Current.Table.getHashDataEntryLength(); }
public:
- using iterator_category = std::input_iterator_tag;
- using value_type = Entry;
- using
diff erence_type = std::ptr
diff _t;
- using pointer = value_type *;
- using reference = value_type &;
-
/// Construct a new iterator for the entries at \p DataOffset.
ValueIterator(const AppleAcceleratorTable &AccelTable, uint64_t DataOffset);
- /// End marker.
- ValueIterator() = default;
- const Entry &operator*() const { return Current; }
- ValueIterator &operator++() { Next(); return *this; }
- ValueIterator operator++(int) {
- ValueIterator I = *this;
- Next();
- return I;
+ const Entry &operator*() {
+ uint64_t OffsetCopy = Offset;
+ Current.extract(&OffsetCopy);
+ return Current;
}
+ ValueIterator &operator++() { Next(); return *this; }
friend bool operator==(const ValueIterator &A, const ValueIterator &B) {
- return A.NumData == B.NumData && A.DataOffset == B.DataOffset;
+ return A.Offset == B.Offset;
}
friend bool operator!=(const ValueIterator &A, const ValueIterator &B) {
return !(A == B);
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp b/llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
index 8cfd9e42edb24..cda822d92aead 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
@@ -267,26 +267,21 @@ LLVM_DUMP_METHOD void AppleAcceleratorTable::dump(raw_ostream &OS) const {
}
}
-AppleAcceleratorTable::Entry::Entry(
- const AppleAcceleratorTable::HeaderData &HdrData)
- : HdrData(&HdrData) {
- Values.reserve(HdrData.Atoms.size());
- for (const auto &Atom : HdrData.Atoms)
+AppleAcceleratorTable::Entry::Entry(const AppleAcceleratorTable &Table)
+ : Table(Table) {
+ Values.reserve(Table.HdrData.Atoms.size());
+ for (const auto &Atom : Table.HdrData.Atoms)
Values.push_back(DWARFFormValue(Atom.second));
}
-void AppleAcceleratorTable::Entry::extract(
- const AppleAcceleratorTable &AccelTable, uint64_t *Offset) {
+void AppleAcceleratorTable::Entry::extract(uint64_t *Offset) {
for (auto &FormValue : Values)
- FormValue.extractValue(AccelTable.AccelSection, Offset,
- AccelTable.FormParams);
+ FormValue.extractValue(Table.AccelSection, Offset, Table.FormParams);
}
std::optional<DWARFFormValue>
AppleAcceleratorTable::Entry::lookup(HeaderData::AtomType AtomToFind) const {
- assert(HdrData && "Dereferencing end iterator?");
- assert(HdrData->Atoms.size() == Values.size());
- for (auto [Atom, FormValue] : zip_equal(HdrData->Atoms, Values))
+ for (auto [Atom, FormValue] : zip_equal(Table.HdrData.Atoms, Values))
if (Atom.first == AtomToFind)
return FormValue;
return std::nullopt;
@@ -294,11 +289,11 @@ AppleAcceleratorTable::Entry::lookup(HeaderData::AtomType AtomToFind) const {
std::optional<uint64_t>
AppleAcceleratorTable::Entry::getDIESectionOffset() const {
- return HdrData->extractOffset(lookup(dwarf::DW_ATOM_die_offset));
+ return Table.HdrData.extractOffset(lookup(dwarf::DW_ATOM_die_offset));
}
std::optional<uint64_t> AppleAcceleratorTable::Entry::getCUOffset() const {
- return HdrData->extractOffset(lookup(dwarf::DW_ATOM_cu_offset));
+ return Table.HdrData.extractOffset(lookup(dwarf::DW_ATOM_cu_offset));
}
std::optional<dwarf::Tag> AppleAcceleratorTable::Entry::getTag() const {
@@ -311,32 +306,14 @@ std::optional<dwarf::Tag> AppleAcceleratorTable::Entry::getTag() const {
}
AppleAcceleratorTable::ValueIterator::ValueIterator(
- const AppleAcceleratorTable &AccelTable, uint64_t Offset)
- : AccelTable(&AccelTable), Current(AccelTable.HdrData), DataOffset(Offset) {
- if (!AccelTable.AccelSection.isValidOffsetForDataOfSize(DataOffset, 4))
- return;
-
- // Read the first entry.
- NumData = AccelTable.AccelSection.getU32(&DataOffset);
- Next();
-}
-
-void AppleAcceleratorTable::ValueIterator::Next() {
- assert(NumData > 0 && "attempted to increment iterator past the end");
- auto &AccelSection = AccelTable->AccelSection;
- if (Data >= NumData ||
- !AccelSection.isValidOffsetForDataOfSize(DataOffset, 4)) {
- NumData = 0;
- DataOffset = 0;
- return;
- }
- Current.extract(*AccelTable, &DataOffset);
- ++Data;
+ const AppleAcceleratorTable &AccelTable, uint64_t DataOffset)
+ : Current(AccelTable), Offset(DataOffset) {
}
iterator_range<AppleAcceleratorTable::ValueIterator>
AppleAcceleratorTable::equal_range(StringRef Key) const {
- const auto EmptyRange = make_range(ValueIterator(), ValueIterator());
+ const auto EmptyRange =
+ make_range(ValueIterator(*this, 0), ValueIterator(*this, 0));
if (!IsValid)
return EmptyRange;
@@ -362,10 +339,13 @@ AppleAcceleratorTable::equal_range(StringRef Key) const {
return EmptyRange;
std::optional<StringRef> MaybeStr = readStringFromStrSection(*StrOffset);
- if (!MaybeStr)
+ std::optional<uint32_t> NumEntries = this->readU32FromAccel(DataOffset);
+ if (!MaybeStr || !NumEntries)
return EmptyRange;
- if (Key == *MaybeStr)
- return make_range({*this, DataOffset}, ValueIterator());
+ if (Key == *MaybeStr) {
+ uint64_t EndOffset = DataOffset + *NumEntries * getHashDataEntryLength();
+ return make_range({*this, DataOffset}, ValueIterator{*this, EndOffset});
+ }
// FIXME: this shouldn't return, we haven't checked all the colliding strings
// in the bucket!
More information about the llvm-commits
mailing list