[BOLT][DWARF] Fix parent chain in debug_names entries with foward

declaration.

Summary:
Previously when an entry was skipped in parent chain a child will point to the
next valid entry in the chain. After discussion in
llvm#91808 this is not very useful.
Changed implemenation so that all the children of the entry that is skipped
won't have DW_IDX_parent.

Author: Alexander Yermolovich

bolt/include/bolt/Core/DIEBuilder.h

@@ -215,10 +215,9 @@ class DIEBuilder {
   /// Along with current CU, and DIE being processed and the new DIE offset to
   /// be updated, it takes in Parents vector that can be empty if this DIE has
   /// no parents.
-  uint32_t
-  finalizeDIEs(DWARFUnit &CU, DIE &Die,
-               std::vector<std::optional<BOLTDWARF5AccelTableData *>> &Parents,
-               uint32_t &CurOffset);
+  uint32_t finalizeDIEs(DWARFUnit &CU, DIE &Die,
+                        std::optional<BOLTDWARF5AccelTableData *> Parent,
+                        uint32_t NumberParentsInChain, uint32_t &CurOffset);
 
   void registerUnit(DWARFUnit &DU, bool NeedSort);
 

bolt/include/bolt/Core/DebugNames.h

@@ -24,23 +24,25 @@ class BOLTDWARF5AccelTableData : public DWARF5AccelTableData {
   BOLTDWARF5AccelTableData(const uint64_t DieOffset,
                            const std::optional<uint64_t> DefiningParentOffset,
                            const unsigned DieTag, const unsigned UnitID,
-                           const bool IsTU,
+                           const bool IsParentRoot, const bool IsTU,
                            const std::optional<unsigned> SecondUnitID)
       : DWARF5AccelTableData(DieOffset, DefiningParentOffset, DieTag, UnitID,
                              IsTU),
-        SecondUnitID(SecondUnitID) {}
+        SecondUnitID(SecondUnitID), IsParentRoot(IsParentRoot) {}
 
   uint64_t getDieOffset() const { return DWARF5AccelTableData::getDieOffset(); }
   unsigned getDieTag() const { return DWARF5AccelTableData::getDieTag(); }
   unsigned getUnitID() const { return DWARF5AccelTableData::getUnitID(); }
   bool isTU() const { return DWARF5AccelTableData::isTU(); }
+  bool isParentRoot() const { return IsParentRoot; }
   std::optional<unsigned> getSecondUnitID() const { return SecondUnitID; }
 
   void setPatchOffset(uint64_t PatchOffset) { OffsetVal = PatchOffset; }
   uint64_t getPatchOffset() const { return std::get<uint64_t>(OffsetVal); }
 
 private:
   std::optional<unsigned> SecondUnitID;
+  bool IsParentRoot;
 };
 
 class DWARF5AcceleratorTable {
@@ -57,6 +59,7 @@ class DWARF5AcceleratorTable {
   std::optional<BOLTDWARF5AccelTableData *>
   addAccelTableEntry(DWARFUnit &Unit, const DIE &Die,
                      const std::optional<uint64_t> &DWOID,
+                     const uint32_t NumberParentsInChain,
                      std::optional<BOLTDWARF5AccelTableData *> &Parent);
   /// Set current unit being processed.
   void setCurrentUnit(DWARFUnit &Unit, const uint64_t UnitStartOffset);

bolt/lib/Core/DIEBuilder.cpp

@@ -461,32 +461,42 @@ getUnitForOffset(DIEBuilder &Builder, DWARFContext &DWCtx,
   return nullptr;
 }
 
-uint32_t DIEBuilder::finalizeDIEs(
-    DWARFUnit &CU, DIE &Die,
-    std::vector<std::optional<BOLTDWARF5AccelTableData *>> &Parents,
-    uint32_t &CurOffset) {
+uint32_t
+DIEBuilder::finalizeDIEs(DWARFUnit &CU, DIE &Die,
+                         std::optional<BOLTDWARF5AccelTableData *> Parent,
+                         uint32_t NumberParentsInChain, uint32_t &CurOffset) {
   getState().DWARFDieAddressesParsed.erase(Die.getOffset());
   uint32_t CurSize = 0;
   Die.setOffset(CurOffset);
   std::optional<BOLTDWARF5AccelTableData *> NameEntry =
       DebugNamesTable.addAccelTableEntry(
           CU, Die, SkeletonCU ? SkeletonCU->getDWOId() : std::nullopt,
-          Parents.back());
+          NumberParentsInChain, Parent);
   // It is possible that an indexed debugging information entry has a parent
   // that is not indexed (for example, if its parent does not have a name
   // attribute). In such a case, a parent attribute may point to a nameless
   // index entry (that is, one that cannot be reached from any entry in the name
   // table), or it may point to the nearest ancestor that does have an index
   // entry.
+  // Although as discussed in https://github.com/llvm/llvm-project/pull/91808
+  // skipping entry is not very useful. So this follows clang where children of
+  // forward declaration won't have DW_IDX_parent.
+
+  // If Parent is nullopt and NumberParentsInChain is not zero, then forward
+  // declaration was encountered in this DF traversal. Propogating nullopt for
+  // Parent to children.
+  if (!Parent && NumberParentsInChain)
+    NameEntry = std::nullopt;
   if (NameEntry)
-    Parents.push_back(std::move(NameEntry));
+    ++NumberParentsInChain;
   for (DIEValue &Val : Die.values())
     CurSize += Val.sizeOf(CU.getFormParams());
   CurSize += getULEB128Size(Die.getAbbrevNumber());
   CurOffset += CurSize;
 
   for (DIE &Child : Die.children()) {
-    uint32_t ChildSize = finalizeDIEs(CU, Child, Parents, CurOffset);
+    uint32_t ChildSize =
+        finalizeDIEs(CU, Child, NameEntry, NumberParentsInChain, CurOffset);
     CurSize += ChildSize;
   }
   // for children end mark.
@@ -496,9 +506,6 @@ uint32_t DIEBuilder::finalizeDIEs(
   }
 
   Die.setSize(CurSize);
-  if (NameEntry)
-    Parents.pop_back();
-
   return CurSize;
 }
 
@@ -510,7 +517,7 @@ void DIEBuilder::finish() {
     DebugNamesTable.setCurrentUnit(CU, UnitStartOffset);
     std::vector<std::optional<BOLTDWARF5AccelTableData *>> Parents;
     Parents.push_back(std::nullopt);
-    finalizeDIEs(CU, *UnitDIE, Parents, CurOffset);
+    finalizeDIEs(CU, *UnitDIE, std::nullopt, 0, CurOffset);
 
     DWARFUnitInfo &CurUnitInfo = getUnitInfoByDwarfUnit(CU);
     CurUnitInfo.UnitOffset = UnitStartOffset;

bolt/lib/Core/DebugNames.cpp

@@ -220,6 +220,7 @@ static uint64_t getEntryID(const BOLTDWARF5AccelTableData &Entry) {
 std::optional<BOLTDWARF5AccelTableData *>
 DWARF5AcceleratorTable::addAccelTableEntry(
     DWARFUnit &Unit, const DIE &Die, const std::optional<uint64_t> &DWOID,
+    const uint32_t NumberParentsInChain,
     std::optional<BOLTDWARF5AccelTableData *> &Parent) {
   if (Unit.getVersion() < 5 || !NeedToCreate)
     return std::nullopt;
@@ -312,8 +313,14 @@ DWARF5AcceleratorTable::addAccelTableEntry(
     // Keeping memory footprint down.
     if (ParentOffset)
       EntryRelativeOffsets.insert({*ParentOffset, 0});
+    bool IsParentRoot = false;
+    // If there is no parent and no valid Entries in parent chain this is a root
+    // to be marked with a flag.
+    if (!Parent && !NumberParentsInChain)
+      IsParentRoot = true;
     It.Values.push_back(new (Allocator) BOLTDWARF5AccelTableData(
-        Die.getOffset(), ParentOffset, DieTag, UnitID, IsTU, SecondIndex));
+        Die.getOffset(), ParentOffset, DieTag, UnitID, IsParentRoot, IsTU,
+        SecondIndex));
     return It.Values.back();
   };
 
@@ -462,7 +469,7 @@ void DWARF5AcceleratorTable::populateAbbrevsMap() {
         Abbrev.addAttribute({dwarf::DW_IDX_die_offset, dwarf::DW_FORM_ref4});
         if (std::optional<uint64_t> Offset = Value->getParentDieOffset())
           Abbrev.addAttribute({dwarf::DW_IDX_parent, dwarf::DW_FORM_ref4});
-        else
+        else if (Value->isParentRoot())
           Abbrev.addAttribute(
               {dwarf::DW_IDX_parent, dwarf::DW_FORM_flag_present});
         FoldingSetNodeID ID;