Skip to content
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.

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

[DebugNames] Implement Entry::GetParentEntry query #78760

Merged

Conversation

felipepiovezan
Copy link
Contributor

This commit introduces a helper function to DWARFAcceleratorTable::Entry which follows DW_IDX_Parent attributes to returns the corresponding parent Entry in the table.

It is tested by enhancing dwarfdump so that it now prints:

  1. When data is corrupt.
  2. When parent information is present, but the parent is not indexed.
  3. The parent entry offset, when the parent is present and indexed. This is printed in terms a real entry offset (the same that gets printed at the start of each entry: "Entry @ 0x..."), instead of the encoded number in the table (which is an offset from the start off the Entry list). This makes it easy to visually inspect the dwarfdump and check what the parent is.

This commit introduces a helper function to DWARFAcceleratorTable::Entry which
follows DW_IDX_Parent attributes to returns the corresponding parent Entry in
the table.

It is tested by enhancing dwarfdump so that it now prints:

1. When data is corrupt.
2. When parent information is present, but the parent is not indexed.
3. The parent entry offset, when the parent is present and indexed. This is
printed in terms a real entry offset (the same that gets printed at the start of
each entry: "Entry @ 0x..."), instead of the encoded number in the table (which
is an offset from the start off the Entry list). This makes it easy to visually
inspect the dwarfdump and check what the parent is.
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 19, 2024

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-debuginfo

Author: Felipe de Azevedo Piovezan (felipepiovezan)

Changes

This commit introduces a helper function to DWARFAcceleratorTable::Entry which follows DW_IDX_Parent attributes to returns the corresponding parent Entry in the table.

It is tested by enhancing dwarfdump so that it now prints:

  1. When data is corrupt.
  2. When parent information is present, but the parent is not indexed.
  3. The parent entry offset, when the parent is present and indexed. This is printed in terms a real entry offset (the same that gets printed at the start of each entry: "Entry @ 0x..."), instead of the encoded number in the table (which is an offset from the start off the Entry list). This makes it easy to visually inspect the dwarfdump and check what the parent is.

Full diff: https://github.com/llvm/llvm-project/pull/78760.diff

8 Files Affected:

  • (modified) llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h (+18)
  • (modified) llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp (+41-2)
  • (added) llvm/test/CodeGen/X86/dwarf-headers.o ()
  • (modified) llvm/test/DebugInfo/Generic/debug-names-one-cu.ll (+2-2)
  • (modified) llvm/test/DebugInfo/X86/debug-names-dwarf64.ll (+5-5)
  • (modified) llvm/test/DebugInfo/X86/debug-names-parents-same-offset.ll (+19-5)
  • (modified) llvm/test/DebugInfo/X86/debug-names-types.ll (+10-10)
  • (modified) llvm/test/tools/dsymutil/ARM/accel-imported-declarations.test (+2-2)
diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h
index b89536bc0c7230c..57f74d70cf9671c 100644
--- a/llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h
+++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h
@@ -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;
+
     /// Return the Abbreviation that can be used to interpret the raw values of
     /// this Accelerator Entry.
     const Abbrev &getAbbrev() const { return *Abbr; }
@@ -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;
@@ -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;
 
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp b/llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
index 0f9c8ef485d456e..03ad5d133caddf4 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
@@ -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();
@@ -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';
   }
 }
diff --git a/llvm/test/CodeGen/X86/dwarf-headers.o b/llvm/test/CodeGen/X86/dwarf-headers.o
new file mode 100644
index 000000000000000..e69de29bb2d1d64
diff --git a/llvm/test/DebugInfo/Generic/debug-names-one-cu.ll b/llvm/test/DebugInfo/Generic/debug-names-one-cu.ll
index 92212a24bd153ad..46b753e9e2def8d 100644
--- a/llvm/test/DebugInfo/Generic/debug-names-one-cu.ll
+++ b/llvm/test/DebugInfo/Generic/debug-names-one-cu.ll
@@ -35,7 +35,7 @@
 ; 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"
@@ -43,7 +43,7 @@
 ; 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.
diff --git a/llvm/test/DebugInfo/X86/debug-names-dwarf64.ll b/llvm/test/DebugInfo/X86/debug-names-dwarf64.ll
index a6855d77a34ac23..c15e2ad1d56b0c4 100644
--- a/llvm/test/DebugInfo/X86/debug-names-dwarf64.ll
+++ b/llvm/test/DebugInfo/X86/debug-names-dwarf64.ll
@@ -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:   ]
@@ -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:   ]
@@ -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:   ]
diff --git a/llvm/test/DebugInfo/X86/debug-names-parents-same-offset.ll b/llvm/test/DebugInfo/X86/debug-names-parents-same-offset.ll
index 9170cfc8508fa5e..95d85ab0ffe0fb3 100644
--- a/llvm/test/DebugInfo/X86/debug-names-parents-same-offset.ll
+++ b/llvm/test/DebugInfo/X86/debug-names-parents-same-offset.ll
@@ -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 }
diff --git a/llvm/test/DebugInfo/X86/debug-names-types.ll b/llvm/test/DebugInfo/X86/debug-names-types.ll
index c44e82578f14a42..f41bb5524b9c33f 100644
--- a/llvm/test/DebugInfo/X86/debug-names-types.ll
+++ b/llvm/test/DebugInfo/X86/debug-names-types.ll
@@ -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:   ]
@@ -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:   ]
@@ -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:   ]
@@ -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:   ]
@@ -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:   ]
@@ -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:   ]
@@ -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:   ]
@@ -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:   ]
diff --git a/llvm/test/tools/dsymutil/ARM/accel-imported-declarations.test b/llvm/test/tools/dsymutil/ARM/accel-imported-declarations.test
index a8bf191e036f75a..8afbe34dcdbfb1d 100644
--- a/llvm/test/tools/dsymutil/ARM/accel-imported-declarations.test
+++ b/llvm/test/tools/dsymutil/ARM/accel-imported-declarations.test
@@ -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:   }
 

Copy link
Collaborator

@clayborg clayborg left a comment

Choose a reason for hiding this comment

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

Looks good. The only question that comes to mind is if we want APIS with both expected and optional in the return value. Double unwrapping required for each, and for the errors that are returned in this patch, they are just consumed and ignored.

/// `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

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.

@clayborg
Copy link
Collaborator

I am ok with this as it is.

@felipepiovezan felipepiovezan merged commit 380ac53 into llvm:main Jan 24, 2024
3 of 4 checks passed
@felipepiovezan felipepiovezan deleted the felipe/get_parent_entry_query branch January 24, 2024 14:44
felipepiovezan added a commit to felipepiovezan/llvm-project that referenced this pull request Feb 2, 2024
This commit introduces a helper function to DWARFAcceleratorTable::Entry
which follows DW_IDX_Parent attributes to returns the corresponding
parent Entry in the table.

It is tested by enhancing dwarfdump so that it now prints:

1. When data is corrupt.
2. When parent information is present, but the parent is not indexed.
3. The parent entry offset, when the parent is present and indexed. This
is printed in terms a real entry offset (the same that gets printed at
the start of each entry: "Entry @ 0x..."), instead of the encoded number
in the table (which is an offset from the start off the Entry list).
This makes it easy to visually inspect the dwarfdump and check what the
parent is.

(cherry picked from commit 380ac53)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants