Skip to content

[DebugNames] Implement Entry::GetParentEntry query #78760

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 24, 2024
Merged

[DebugNames] Implement Entry::GetParentEntry query #78760

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f935d8b
[DebugNames] Implement Entry::GetParentEntry query
felipepiovezan Dec 7, 2023
3b6189c
fixup! Use /// for func docs
felipepiovezan Jan 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,16 @@ class DWARFDebugNames : public DWARFAcceleratorTable {
/// Returns the Offset of the DIE within the containing CU or TU.
std::optional<uint64_t> getDIEUnitOffset() const;

/// Returns true if this Entry has information about its parent DIE (i.e. if
/// it has an IDX_parent attribute)
bool hasParentInformation() const;

/// Returns the Entry corresponding to the parent of the DIE represented by
/// `this` Entry. If the parent is not in the table, nullopt is returned.
/// Precondition: hasParentInformation() == true.
/// An error is returned for ill-formed tables.
Expected<std::optional<DWARFDebugNames::Entry>> getParentDIEEntry() const;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want expected and optional? Seems a bit much to double unwrap each time we call this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is the bit about this patch that I really dislike. But we have two distinct things we need to account for here:

  1. Invalid data: a bad offset for some reason. This is what the Expected is handling.
  2. When the parent is present but not indexed by the table (e.g. the end of the parent chain). This is what the optional is handling.

We could remove the error handling portion (the Expected) and treat case 1 as if it were case 2, but it would make debugging the invalid data case a bit difficult, if we ever have to do that.

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Drive-by) can you return some kind of no-data error for the parent-not-present case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmm let me look into that, but I am not aware of other instances in LLVM where we use run-time type checking on the Error class to make decisions 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have the boolean that tell us if the entry has a parent index case, can we remove the need for an error if we get a std::nullopt back but Entry::hasParentInformation() == true? I do agree this is nice for llvm-dwarfdump to get an error back and display it. If we just use the error somewhere, then it is worth adding.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another idea would be to have this function return an expected pair, where there is a bool indicating if there is parent info + Entry. Something like:

Expected<std::pair<bool, DWARFDebugNames::Entry>> getParentDIEEntry() const;

I am not a fan of std::pair in general though, so maybe this isn't any better.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you reinvented std::optional there ;-)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Drive-by) can you return some kind of no-data error for the parent-not-present case?

I like the clean separation of errors that need to be logged/reported and unsuccessful lookups the current interface provides, as clumsy as it looks.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair

Copy link
Contributor Author

@felipepiovezan felipepiovezan Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, given this discussion, I would like to propose we move forward with the interface as is.
As much as it looks off, I did put a lot of thought into this and our brainstorm session did not yield a better API.
This interface catches the reader's attention -- as it should -- because there are nuances on the return value that are only explained by reading the docs.


/// Return the Abbreviation that can be used to interpret the raw values of
/// this Accelerator Entry.
const Abbrev &getAbbrev() const { return *Abbr; }
Expand All @@ -469,6 +479,7 @@ class DWARFDebugNames : public DWARFAcceleratorTable {
std::optional<DWARFFormValue> lookup(dwarf::Index Index) const;

void dump(ScopedPrinter &W) const;
void dumpParentIdx(ScopedPrinter &W, const DWARFFormValue &FormValue) const;

friend class NameIndex;
friend class ValueIterator;
Expand Down Expand Up @@ -609,6 +620,13 @@ class DWARFDebugNames : public DWARFAcceleratorTable {

Expected<Entry> getEntry(uint64_t *Offset) const;

// Returns the Entry at the relative `Offset` from the start of the Entry
// pool.
Expected<Entry> getEntryAtRelativeOffset(uint64_t Offset) const {
auto OffsetFromSection = Offset + this->EntriesBase;
return getEntry(&OffsetFromSection);
}

/// Look up all entries in this Name Index matching \c Key.
iterator_range<ValueIterator> equal_range(StringRef Key) const;

Expand Down
43 changes: 41 additions & 2 deletions llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,6 +611,10 @@ DWARFDebugNames::Entry::lookup(dwarf::Index Index) const {
return std::nullopt;
}

bool DWARFDebugNames::Entry::hasParentInformation() const {
return lookup(dwarf::DW_IDX_parent).has_value();
}

std::optional<uint64_t> DWARFDebugNames::Entry::getDIEUnitOffset() const {
if (std::optional<DWARFFormValue> Off = lookup(dwarf::DW_IDX_die_offset))
return Off->getAsReferenceUVal();
Expand Down Expand Up @@ -650,13 +654,48 @@ std::optional<uint64_t> DWARFDebugNames::Entry::getLocalTUIndex() const {
return std::nullopt;
}

Expected<std::optional<DWARFDebugNames::Entry>>
DWARFDebugNames::Entry::getParentDIEEntry() const {
// The offset of the accelerator table entry for the parent.
std::optional<DWARFFormValue> ParentEntryOff = lookup(dwarf::DW_IDX_parent);
assert(ParentEntryOff.has_value() && "hasParentInformation() must be called");

if (ParentEntryOff->getForm() == dwarf::Form::DW_FORM_flag_present)
return std::nullopt;
return NameIdx->getEntryAtRelativeOffset(ParentEntryOff->getRawUValue());
}

void DWARFDebugNames::Entry::dumpParentIdx(
ScopedPrinter &W, const DWARFFormValue &FormValue) const {
Expected<std::optional<Entry>> ParentEntry = getParentDIEEntry();
if (!ParentEntry) {
W.getOStream() << "<invalid offset data>";
consumeError(ParentEntry.takeError());
return;
}

if (!ParentEntry->has_value()) {
W.getOStream() << "<parent not indexed>";
return;
}

auto AbsoluteOffset = NameIdx->EntriesBase + FormValue.getRawUValue();
W.getOStream() << "Entry @ 0x" + Twine::utohexstr(AbsoluteOffset);
}

void DWARFDebugNames::Entry::dump(ScopedPrinter &W) const {
W.startLine() << formatv("Abbrev: {0:x}\n", Abbr->Code);
W.startLine() << formatv("Tag: {0}\n", Abbr->Tag);
assert(Abbr->Attributes.size() == Values.size());
for (auto Tuple : zip_first(Abbr->Attributes, Values)) {
W.startLine() << formatv("{0}: ", std::get<0>(Tuple).Index);
std::get<1>(Tuple).dump(W.getOStream());
auto Index = std::get<0>(Tuple).Index;
W.startLine() << formatv("{0}: ", Index);

auto FormValue = std::get<1>(Tuple);
if (Index == dwarf::Index::DW_IDX_parent)
dumpParentIdx(W, FormValue);
else
FormValue.dump(W.getOStream());
W.getOStream() << '\n';
}
}
Expand Down
Empty file added llvm/test/CodeGen/X86/dwarf-headers.o
View file
Open in desktop
Empty file.
4 changes: 2 additions & 2 deletions llvm/test/DebugInfo/Generic/debug-names-one-cu.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,15 @@
; CHECK-NEXT: Abbrev: [[ABBREV]]
; CHECK-NEXT: Tag: DW_TAG_variable
; CHECK-NEXT: DW_IDX_die_offset: 0x{{[0-9a-f]*}}
; CHECK-NEXT: DW_IDX_parent: true
; CHECK-NEXT: DW_IDX_parent: <parent not indexed>
; CHECK-NEXT: }

; CHECK: String: 0x{{[0-9a-f]*}} "foobar"
; CHECK-NEXT: Entry
; CHECK-NEXT: Abbrev: [[ABBREV]]
; CHECK-NEXT: Tag: DW_TAG_variable
; CHECK-NEXT: DW_IDX_die_offset: 0x{{[0-9a-f]*}}
; CHECK-NEXT: DW_IDX_parent: true
; CHECK-NEXT: DW_IDX_parent: <parent not indexed>
; CHECK-NEXT: }

; VERIFY: No errors.
Expand Down
10 changes: 5 additions & 5 deletions llvm/test/DebugInfo/X86/debug-names-dwarf64.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
; CHECK-NEXT: Abbrev: [[ABBREV]]
; CHECK-NEXT: Tag: DW_TAG_base_type
; CHECK-NEXT: DW_IDX_die_offset: [[TYPEDIE]]
; CHECK-NEXT: DW_IDX_parent: true
; CHECK-NEXT: DW_IDX_parent: <parent not indexed>
; CHECK-NEXT: }
; CHECK-NEXT: }
; CHECK-NEXT: ]
Expand All @@ -71,17 +71,17 @@
; CHECK-NEXT: Abbrev: [[ABBREV1]]
; CHECK-NEXT: Tag: DW_TAG_variable
; CHECK-NEXT: DW_IDX_die_offset: [[VARDIE]]
; CHECK-NEXT: DW_IDX_parent: true
; CHECK-NEXT: DW_IDX_parent: <parent not indexed>
; CHECK-NEXT: }
; CHECK-NEXT: }
; CHECK-NEXT: Name 3 {
; CHECK-NEXT: Hash: 0x7C96FE71
; CHECK-NEXT: String: {{.+}} "func"
; CHECK-NEXT: Entry @ {{.+}} {
; CHECK-NEXT: Entry @ [[FUNC_ENTRY:0x.+]] {
; CHECK-NEXT: Abbrev: [[ABBREV_SP]]
; CHECK-NEXT: Tag: DW_TAG_subprogram
; CHECK-NEXT: DW_IDX_die_offset: [[SPDIE]]
; CHECK-NEXT: DW_IDX_parent: true
; CHECK-NEXT: DW_IDX_parent: <parent not indexed>
; CHECK-NEXT: }
; CHECK-NEXT: }
; CHECK-NEXT: ]
Expand All @@ -96,7 +96,7 @@
; CHECK-NEXT: Abbrev: [[ABBREV_LABEL]]
; CHECK-NEXT: Tag: DW_TAG_label
; CHECK-NEXT: DW_IDX_die_offset: [[LABELDIE]]
; CHECK-NEXT: DW_IDX_parent: 0x{{.*}}
; CHECK-NEXT: DW_IDX_parent: Entry @ [[FUNC_ENTRY]]
; CHECK-NEXT: }
; CHECK-NEXT: }
; CHECK-NEXT: ]
Expand Down
24 changes: 19 additions & 5 deletions llvm/test/DebugInfo/X86/debug-names-parents-same-offset.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,30 @@
; CHECK-NEXT: Abbrev: [[ABBREV:0x.*]]
; CHECK-NEXT: Tag: DW_TAG_structure_type
; CHECK-NEXT: DW_IDX_type_unit: 0x01
; CHECK-NEXT: DW_IDX_die_offset: [[DieOffset:0x.*]]
; CHECK-NEXT: DW_IDX_parent: [[Parent1:0x.*]]
; CHECK-NEXT: DW_IDX_die_offset: [[DieOffsetStruct:0x.*]]
; CHECK-NEXT: DW_IDX_parent: Entry @ [[Parent2:0x.*]]
; CHECK: String: {{.*}} "MyNamespace"
; CHECK-NEXT: Entry @ [[Parent1:0x.*]] {
; CHECK-NEXT: Abbrev: {{.*}}
; CHECK-NEXT: Tag: DW_TAG_namespace
; CHECK-NEXT: DW_IDX_type_unit: 0x00
; CHECK-NEXT: DW_IDX_die_offset: [[DieOffsetNamespace:0x.*]]
; CHECK-NEXT: DW_IDX_parent: <parent not indexed>
; CHECK-NEXT: }
; CHECK-NEXT: Entry @ [[Parent2]] {
; CHECK-NEXT: Abbrev: {{.*}}
; CHECK-NEXT: Tag: DW_TAG_namespace
; CHECK-NEXT: DW_IDX_type_unit: 0x01
; CHECK-NEXT: DW_IDX_die_offset: [[DieOffsetNamespace:0x.*]]
; CHECK-NEXT: DW_IDX_parent: <parent not indexed>
; CHECK-NEXT: }
; CHECK: String: {{.*}} "MyStruct1"
; CHECK-NEXT: Entry @ {{.*}} {
; CHECK-NEXT: Abbrev: [[ABBREV]]
; CHECK-NEXT: Tag: DW_TAG_structure_type
; CHECK-NEXT: DW_IDX_type_unit: 0x00
; CHECK-NEXT: DW_IDX_die_offset: [[DieOffset:0x.*]]
; CHECK-NOT: DW_IDX_parent: [[Parent1]]
; CHECK-NEXT: DW_IDX_parent: 0x{{.*}}
; CHECK-NEXT: DW_IDX_die_offset: [[DieOffsetStruct:0x.*]]
; CHECK-NEXT: DW_IDX_parent: Entry @ [[Parent1]]


%"struct.MyNamespace::MyStruct1" = type { i8 }
Expand Down
20 changes: 10 additions & 10 deletions llvm/test/DebugInfo/X86/debug-names-types.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@
; CHECK-NEXT: Abbrev: [[ABBREV]]
; CHECK-NEXT: Tag: DW_TAG_base_type
; CHECK-NEXT: DW_IDX_die_offset: 0x0000003e
; CHECK-NEXT: DW_IDX_parent: true
; CHECK-NEXT: DW_IDX_parent: <parent not indexed>
; CHECK-NEXT: }
; CHECK-NEXT: }
; CHECK-NEXT: ]
Expand All @@ -86,13 +86,13 @@
; CHECK-NEXT: Tag: DW_TAG_structure_type
; CHECK-NEXT: DW_IDX_type_unit: 0x00
; CHECK-NEXT: DW_IDX_die_offset: 0x00000023
; CHECK-NEXT: DW_IDX_parent: true
; CHECK-NEXT: DW_IDX_parent: <parent not indexed>
; CHECK-NEXT: }
; CHECK-NEXT: Entry @ {{.+}} {
; CHECK-NEXT: Abbrev: [[ABBREV1]]
; CHECK-NEXT: Tag: DW_TAG_structure_type
; CHECK-NEXT: DW_IDX_die_offset: 0x00000042
; CHECK-NEXT: DW_IDX_parent: true
; CHECK-NEXT: DW_IDX_parent: <parent not indexed>
; CHECK-NEXT: }
; CHECK-NEXT: }
; CHECK-NEXT: ]
Expand All @@ -104,7 +104,7 @@
; CHECK-NEXT: Abbrev: [[ABBREV2]]
; CHECK-NEXT: Tag: DW_TAG_subprogram
; CHECK-NEXT: DW_IDX_die_offset: 0x00000023
; CHECK-NEXT: DW_IDX_parent: true
; CHECK-NEXT: DW_IDX_parent: <parent not indexed>
; CHECK-NEXT: }
; CHECK-NEXT: }
; CHECK-NEXT: ]
Expand All @@ -117,7 +117,7 @@
; CHECK-NEXT: Tag: DW_TAG_base_type
; CHECK-NEXT: DW_IDX_type_unit: 0x00
; CHECK-NEXT: DW_IDX_die_offset: 0x00000038
; CHECK-NEXT: DW_IDX_parent: true
; CHECK-NEXT: DW_IDX_parent: <parent not indexed>
; CHECK-NEXT: }
; CHECK-NEXT: }
; CHECK-NEXT: ]
Expand Down Expand Up @@ -176,7 +176,7 @@
; CHECK-SPLIT-NEXT: Abbrev: [[ABBREV2]]
; CHECK-SPLIT-NEXT: Tag: DW_TAG_base_type
; CHECK-SPLIT-NEXT: DW_IDX_die_offset: 0x00000035
; CHECK-SPLIT-NEXT: DW_IDX_parent: true
; CHECK-SPLIT-NEXT: DW_IDX_parent: <parent not indexed>
; CHECK-SPLIT-NEXT: }
; CHECK-SPLIT-NEXT: }
; CHECK-SPLIT-NEXT: ]
Expand All @@ -189,13 +189,13 @@
; CHECK-SPLIT-NEXT: Tag: DW_TAG_structure_type
; CHECK-SPLIT-NEXT: DW_IDX_type_unit: 0x00
; CHECK-SPLIT-NEXT: DW_IDX_die_offset: 0x00000021
; CHECK-SPLIT-NEXT: DW_IDX_parent: true
; CHECK-SPLIT-NEXT: DW_IDX_parent: <parent not indexed>
; CHECK-SPLIT-NEXT: }
; CHECK-SPLIT-NEXT: Entry @ {{.*}} {
; CHECK-SPLIT-NEXT: Abbrev: [[ABBREV]]
; CHECK-SPLIT-NEXT: Tag: DW_TAG_structure_type
; CHECK-SPLIT-NEXT: DW_IDX_die_offset: 0x00000039
; CHECK-SPLIT-NEXT: DW_IDX_parent: true
; CHECK-SPLIT-NEXT: DW_IDX_parent: <parent not indexed>
; CHECK-SPLIT-NEXT: }
; CHECK-SPLIT-NEXT: }
; CHECK-SPLIT-NEXT: ]
Expand All @@ -207,7 +207,7 @@
; CHECK-SPLIT-NEXT: Abbrev: [[ABBREV3]]
; CHECK-SPLIT-NEXT: Tag: DW_TAG_subprogram
; CHECK-SPLIT-NEXT: DW_IDX_die_offset: 0x0000001a
; CHECK-SPLIT-NEXT: DW_IDX_parent: true
; CHECK-SPLIT-NEXT: DW_IDX_parent: <parent not indexed>
; CHECK-SPLIT-NEXT: }
; CHECK-SPLIT-NEXT: }
; CHECK-SPLIT-NEXT: ]
Expand All @@ -220,7 +220,7 @@
; CHECK-SPLIT-NEXT: Tag: DW_TAG_base_type
; CHECK-SPLIT-NEXT: DW_IDX_type_unit: 0x00
; CHECK-SPLIT-NEXT: DW_IDX_die_offset: 0x00000036
; CHECK-SPLIT-NEXT: DW_IDX_parent: true
; CHECK-SPLIT-NEXT: DW_IDX_parent: <parent not indexed>
; CHECK-SPLIT-NEXT: }
; CHECK-SPLIT-NEXT: }
; CHECK-SPLIT-NEXT: ]
Expand Down
4 changes: 2 additions & 2 deletions llvm/test/tools/dsymutil/ARM/accel-imported-declarations.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,13 @@ DWARF-NEXT: Entry {{.*}} {
DWARF-NEXT: Abbrev: {{.*}}
DWARF-NEXT: Tag: DW_TAG_namespace
DWARF: DW_IDX_die_offset: [[NAMESPACE]]
DWARF-NEXT: DW_IDX_parent: 0x{{.*}}
DWARF-NEXT: DW_IDX_parent: Entry @ 0x{{.*}}
DWARF-NEXT: }
DWARF-NEXT: Entry {{.*}} {
DWARF-NEXT: Abbrev: {{.*}}
DWARF: Tag: DW_TAG_imported_declaration
DWARF: DW_IDX_die_offset: 0x0000005c
DWARF-NEXT: DW_IDX_parent: 0x{{.*}}
DWARF-NEXT: DW_IDX_parent: Entry @ 0x{{.*}}
DWARF-NEXT: }
DWARF-NEXT: }

Expand Down