Skip to content

[BOLT][DWARF] Refactor legacy ranges writers #96006

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

Jump to bottom
Merged
merged 6 commits into from
Jul 3, 2024
Merged

[BOLT][DWARF] Refactor legacy ranges writers #96006

merged 6 commits into from
Jul 3, 2024

Conversation

sayhaan
Copy link
Member

@sayhaan sayhaan commented Jun 18, 2024 •
edited
Loading

Refactors legacy ranges writers to create a writer for each instance of a DWO file.

We now write out everything into .debug_ranges after the all the DWO files are processed. This also changes the order that ranges is written out in, as before we wrote out while in the main CU processing loop and we now iterate through the CU buckets created by partitionCUs, after the main processing loop.

@sayhaan sayhaan marked this pull request as ready for review June 20, 2024 16:25
@llvmbot llvmbot added the BOLT label Jun 20, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 20, 2024

@llvm/pr-subscribers-bolt

Author: Sayhaan Siddiqui (sayhaan)

Changes

Refactors updateDWARFObjectAddressRanges to create a writer for each instance of a DWO file.

Patch is 23.55 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/96006.diff

8 Files Affected:

  • (modified) bolt/include/bolt/Core/DebugData.h (+2)
  • (modified) bolt/include/bolt/Rewrite/DWARFRewriter.h (+7)
  • (modified) bolt/lib/Core/DebugData.cpp (+7)
  • (modified) bolt/lib/Rewrite/DWARFRewriter.cpp (+68-44)
  • (modified) bolt/test/X86/debug-fission-single-convert.s (+3-3)
  • (modified) bolt/test/X86/dwarf4-df-dualcu.test (+18-16)
  • (modified) bolt/test/X86/dwarf4-df-input-lowpc-ranges-cus.test (+40-38)
  • (modified) bolt/test/X86/dwarf4-df-input-lowpc-ranges.test (+19-18)
diff --git a/bolt/include/bolt/Core/DebugData.h b/bolt/include/bolt/Core/DebugData.h
index 585bafa088849..5c730e63ae0aa 100644
--- a/bolt/include/bolt/Core/DebugData.h
+++ b/bolt/include/bolt/Core/DebugData.h
@@ -210,6 +210,8 @@ class DebugRangesSectionWriter {
   static bool classof(const DebugRangesSectionWriter *Writer) {
     return Writer->getKind() == RangesWriterKind::DebugRangesWriter;
   }
+  
+  void updateRangeBuffer(std::unique_ptr<DebugBufferVector> &CUBuffer);
 
   /// Writes out range lists for a current CU being processed.
   void virtual finalizeSection(){};
diff --git a/bolt/include/bolt/Rewrite/DWARFRewriter.h b/bolt/include/bolt/Rewrite/DWARFRewriter.h
index 8dec32de9008e..c97f25125c099 100644
--- a/bolt/include/bolt/Rewrite/DWARFRewriter.h
+++ b/bolt/include/bolt/Rewrite/DWARFRewriter.h
@@ -89,6 +89,13 @@ class DWARFRewriter {
   /// Store Rangelists writer for each DWO CU.
   RangeListsDWOWriers RangeListsWritersByCU;
 
+  using LegacyRangesDWOWriers =
+      std::unordered_map<uint64_t, std::unique_ptr<DebugRangesSectionWriter>>;
+  /// Store Rangelists writer for each DWO CU.
+  LegacyRangesDWOWriers LegacyRangesWritersByCU;
+
+  std::unordered_map<uint64_t, DIE *> UpdatedDIEsByDWO;
+
   std::mutex LocListDebugInfoPatchesMutex;
 
   /// Dwo id specific its RangesBase.
diff --git a/bolt/lib/Core/DebugData.cpp b/bolt/lib/Core/DebugData.cpp
index f502a50312470..8895b4923294a 100644
--- a/bolt/lib/Core/DebugData.cpp
+++ b/bolt/lib/Core/DebugData.cpp
@@ -177,6 +177,13 @@ uint64_t DebugRangesSectionWriter::getSectionOffset() {
   return SectionOffset;
 }
 
+void DebugRangesSectionWriter::updateRangeBuffer(std::unique_ptr<DebugBufferVector> &CUBuffer) {
+  for(auto DebugInfo : *CUBuffer){
+    RangesBuffer->push_back(DebugInfo);
+  }
+  SectionOffset = RangesBuffer->size();
+}
+
 DebugAddrWriter *DebugRangeListsSectionWriter::AddrWriter = nullptr;
 
 uint64_t DebugRangeListsSectionWriter::addRanges(
diff --git a/bolt/lib/Rewrite/DWARFRewriter.cpp b/bolt/lib/Rewrite/DWARFRewriter.cpp
index 8814ebbd10aa5..f831e7188da32 100644
--- a/bolt/lib/Rewrite/DWARFRewriter.cpp
+++ b/bolt/lib/Rewrite/DWARFRewriter.cpp
@@ -646,6 +646,15 @@ void DWARFRewriter::updateDebugInfo() {
 
     } else {
       LocListWritersByCU[CUIndex] = std::make_unique<DebugLocWriter>();
+      if (std::optional<uint64_t> DWOId = CU.getDWOId()) {
+        assert(LegacyRangesWritersByCU.count(*DWOId) == 0 &&
+               "LegacyRangeLists writer for DWO unit already exists.");
+        auto LegacyRangesSectionWriterByCU =
+            std::make_unique<DebugRangesSectionWriter>();
+        LegacyRangesSectionWriterByCU->initSection(CU);
+        LegacyRangesWritersByCU[*DWOId] =
+            std::move(LegacyRangesSectionWriterByCU);
+      }
     }
     return LocListWritersByCU[CUIndex++].get();
   };
@@ -692,6 +701,7 @@ void DWARFRewriter::updateDebugInfo() {
       if (Unit->getVersion() >= 5) {
         TempRangesSectionWriter = RangeListsWritersByCU[*DWOId].get();
       } else {
+        TempRangesSectionWriter = LegacyRangesWritersByCU[*DWOId].get();
         RangesBase = RangesSectionWriter->getSectionOffset();
         setDwoRangesBase(*DWOId, *RangesBase);
       }
@@ -1270,10 +1280,14 @@ void DWARFRewriter::updateDWARFObjectAddressRanges(
     }
 
     if (RangesBaseInfo) {
-      DIEBldr.replaceValue(&Die, RangesBaseInfo.getAttribute(),
-                           RangesBaseInfo.getForm(),
-                           DIEInteger(static_cast<uint32_t>(*RangesBase)));
-      RangesBase = std::nullopt;
+      if (RangesBaseInfo.getAttribute() == dwarf::DW_AT_GNU_ranges_base) {
+        UpdatedDIEsByDWO[*Unit.getDWOId()] = &Die;
+      } else {
+        DIEBldr.replaceValue(&Die, RangesBaseInfo.getAttribute(),
+                             RangesBaseInfo.getForm(),
+                             DIEInteger(static_cast<uint32_t>(*RangesBase)));
+        RangesBase = std::nullopt;
+      }
     }
   }
 
@@ -1290,11 +1304,9 @@ void DWARFRewriter::updateDWARFObjectAddressRanges(
         RangesAttrInfo.getForm() == dwarf::DW_FORM_sec_offset)
       NeedConverted = true;
 
-    uint64_t CurRangeBase = 0;
     if (Unit.isDWOUnit()) {
-      if (std::optional<uint64_t> DWOId = Unit.getDWOId())
-        CurRangeBase = getDwoRangesBase(*DWOId);
-      else
+      std::optional<uint64_t> DWOId = Unit.getDWOId();
+      if (!DWOId)
         errs() << "BOLT-WARNING: [internal-dwarf-error]: DWOId is not found "
                   "for DWO Unit.";
     }
@@ -1303,7 +1315,7 @@ void DWARFRewriter::updateDWARFObjectAddressRanges(
                            DIEInteger(DebugRangesOffset));
     else
       DIEBldr.replaceValue(&Die, dwarf::DW_AT_ranges, RangesAttrInfo.getForm(),
-                           DIEInteger(DebugRangesOffset - CurRangeBase));
+                           DIEInteger(DebugRangesOffset));
 
     if (!RangesBase) {
       if (LowPCAttrInfo &&
@@ -1320,10 +1332,11 @@ void DWARFRewriter::updateDWARFObjectAddressRanges(
 
     // If we are at this point we are in the CU/Skeleton CU, and
     // DW_AT_GNU_ranges_base or DW_AT_rnglists_base doesn't exist.
-    if (Unit.getVersion() <= 4)
+    if (Unit.getVersion() <= 4) {
       DIEBldr.addValue(&Die, dwarf::DW_AT_GNU_ranges_base, dwarf::DW_FORM_data4,
-                       DIEInteger(*RangesBase));
-    else if (Unit.getVersion() == 5)
+                       DIEInteger(INT_MAX));
+      UpdatedDIEsByDWO[*Unit.getDWOId()] = &Die;
+    } else if (Unit.getVersion() == 5)
       DIEBldr.addValue(&Die, dwarf::DW_AT_rnglists_base,
                        dwarf::DW_FORM_sec_offset, DIEInteger(*RangesBase));
     else
@@ -1605,6 +1618,26 @@ void DWARFRewriter::finalizeCompileUnits(DIEBuilder &DIEBlder,
                                          DIEStreamer &Streamer,
                                          CUOffsetMap &CUMap,
                                          const std::list<DWARFUnit *> &CUs) {
+  size_t BufferSize = 0;
+  for (DWARFUnit *CU : CUs) {
+    if (CU->getVersion() != 4)
+      continue;
+    std::optional<uint64_t> DWOId;
+    DWOId = CU->getDWOId();
+    if (DWOId) {
+      if (DIE *Die = UpdatedDIEsByDWO[*DWOId]) {
+        DIEValue DvalGNUBase = Die->findAttribute(dwarf::DW_AT_GNU_ranges_base);
+        DIEBlder.replaceValue(Die, dwarf::DW_AT_GNU_ranges_base,
+                              DvalGNUBase.getForm(), DIEInteger(BufferSize));
+      }
+      if (LegacyRangesWritersByCU[*DWOId]) {
+        std::unique_ptr<DebugBufferVector> RangesWritersContents =
+            LegacyRangesWritersByCU[*DWOId]->releaseBuffer();
+        LegacyRangesSectionWriter->updateRangeBuffer(RangesWritersContents);
+        BufferSize += RangesWritersContents->size();
+      }
+    }
+  }
   DIEBlder.generateAbbrevs();
   DIEBlder.finish();
   // generate debug_info and CUMap
@@ -2263,7 +2296,6 @@ void DWARFRewriter::convertToRangesPatchDebugInfo(
     DWARFUnit &Unit, DIEBuilder &DIEBldr, DIE &Die,
     uint64_t RangesSectionOffset, DIEValue &LowPCAttrInfo,
     DIEValue &HighPCAttrInfo, std::optional<uint64_t> RangesBase) {
-  uint32_t BaseOffset = 0;
   dwarf::Form LowForm = LowPCAttrInfo.getForm();
   dwarf::Attribute RangeBaseAttribute = dwarf::DW_AT_GNU_ranges_base;
   dwarf::Form RangesForm = dwarf::DW_FORM_sec_offset;
@@ -2278,42 +2310,34 @@ void DWARFRewriter::convertToRangesPatchDebugInfo(
                    Die.getTag() == dwarf::DW_TAG_skeleton_unit;
   if (!IsUnitDie)
     DIEBldr.deleteValue(&Die, LowPCAttrInfo.getAttribute());
-  // In DWARF4 for DW_AT_low_pc in binary DW_FORM_addr is used. In the DWO
-  // section DW_FORM_GNU_addr_index is used. So for if we are converting
-  // DW_AT_low_pc/DW_AT_high_pc and see DW_FORM_GNU_addr_index. We are
-  // converting in DWO section, and DW_AT_ranges [DW_FORM_sec_offset] is
-  // relative to DW_AT_GNU_ranges_base.
-  if (LowForm == dwarf::DW_FORM_GNU_addr_index) {
-    // Ranges are relative to DW_AT_GNU_ranges_base.
-    uint64_t CurRangeBase = 0;
-    if (std::optional<uint64_t> DWOId = Unit.getDWOId()) {
-      CurRangeBase = getDwoRangesBase(*DWOId);
-    }
-    BaseOffset = CurRangeBase;
-  } else {
-    // In DWARF 5 we can have DW_AT_low_pc either as DW_FORM_addr, or
-    // DW_FORM_addrx. Former is when DW_AT_rnglists_base is present. Latter is
-    // when it's absent.
-    if (IsUnitDie) {
-      if (LowForm == dwarf::DW_FORM_addrx) {
-        const uint32_t Index = AddrWriter->getIndexFromAddress(0, Unit);
-        DIEBldr.replaceValue(&Die, LowPCAttrInfo.getAttribute(),
-                             LowPCAttrInfo.getForm(), DIEInteger(Index));
-      } else {
-        DIEBldr.replaceValue(&Die, LowPCAttrInfo.getAttribute(),
-                             LowPCAttrInfo.getForm(), DIEInteger(0));
-      }
+
+  // In DWARF 5 we can have DW_AT_low_pc either as DW_FORM_addr, or
+  // DW_FORM_addrx. Former is when DW_AT_rnglists_base is present. Latter is
+  // when it's absent.
+  if (IsUnitDie) {
+    if (LowForm == dwarf::DW_FORM_addrx) {
+      const uint32_t Index = AddrWriter->getIndexFromAddress(0, Unit);
+      DIEBldr.replaceValue(&Die, LowPCAttrInfo.getAttribute(),
+                           LowPCAttrInfo.getForm(), DIEInteger(Index));
+    } else {
+      DIEBldr.replaceValue(&Die, LowPCAttrInfo.getAttribute(),
+                           LowPCAttrInfo.getForm(), DIEInteger(0));
     }
-    // Original CU didn't have DW_AT_*_base. We converted it's children (or
-    // dwo), so need to insert it into CU.
-    if (RangesBase)
+  }
+  // Original CU didn't have DW_AT_*_base. We converted it's children (or
+  // dwo), so need to insert it into CU.
+  if (RangesBase) {
+    if (Unit.getVersion() >= 5) {
       DIEBldr.addValue(&Die, RangeBaseAttribute, dwarf::DW_FORM_sec_offset,
                        DIEInteger(*RangesBase));
+    } else {
+      DIEBldr.addValue(&Die, RangeBaseAttribute, dwarf::DW_FORM_sec_offset,
+                       DIEInteger(INT_MAX));
+      UpdatedDIEsByDWO[*Unit.getDWOId()] = &Die;
+    }
   }
 
-  uint64_t RangeAttrVal = RangesSectionOffset - BaseOffset;
-  if (Unit.getVersion() >= 5)
-    RangeAttrVal = RangesSectionOffset;
+  uint64_t RangeAttrVal = RangesSectionOffset;
   // HighPC was conveted into DW_AT_ranges.
   // For DWARF5 we only access ranges through index.
 
diff --git a/bolt/test/X86/debug-fission-single-convert.s b/bolt/test/X86/debug-fission-single-convert.s
index 28fcb6686e0a2..4cd881740b2f8 100644
--- a/bolt/test/X86/debug-fission-single-convert.s
+++ b/bolt/test/X86/debug-fission-single-convert.s
@@ -31,11 +31,11 @@
 # CHECK-DWO-DWO: 00000010
 # CHECK-DWO-DWO: 00000050
 # CHECK-DWO-DWO: DW_TAG_subprogram
-# CHECK-DWO-DWO-NEXT: DW_AT_ranges [DW_FORM_sec_offset] (0x00000000
+# CHECK-DWO-DWO-NEXT: DW_AT_ranges [DW_FORM_sec_offset] (0x00000010
 # CHECK-DWO-DWO: DW_TAG_subprogram
-# CHECK-DWO-DWO-NEXT: DW_AT_ranges [DW_FORM_sec_offset] (0x00000020
+# CHECK-DWO-DWO-NEXT: DW_AT_ranges [DW_FORM_sec_offset] (0x00000030
 # CHECK-DWO-DWO: DW_TAG_subprogram
-# CHECK-DWO-DWO-NEXT: DW_AT_ranges [DW_FORM_sec_offset] (0x00000040
+# CHECK-DWO-DWO-NEXT: DW_AT_ranges [DW_FORM_sec_offset] (0x00000050
 
 # CHECK-ADDR-SEC: .debug_addr contents:
 # CHECK-ADDR-SEC: 0x00000000: Addrs: [
diff --git a/bolt/test/X86/dwarf4-df-dualcu.test b/bolt/test/X86/dwarf4-df-dualcu.test
index b690623b70d83..87bbe36da645c 100644
--- a/bolt/test/X86/dwarf4-df-dualcu.test
+++ b/bolt/test/X86/dwarf4-df-dualcu.test
@@ -38,35 +38,37 @@
 ; BOLT: .debug_ranges
 ; BOLT-NEXT: 00000000 <End of list>
 ; BOLT-NEXT: 00000010 [[#%.16x,ADDR:]] [[#%.16x,ADDRB:]]
+; BOLT-NEXT: 00000010 [[#%.16x,ADDR1:]] [[#%.16x,ADDR1B:]]
 ; BOLT-NEXT: 00000010 <End of list>
-; BOLT-NEXT: 00000030 [[#%.16x,ADDR1:]] [[#%.16x,ADDR1B:]]
-; BOLT-NEXT: 00000030 <End of list>
+; BOLT-NEXT: 00000040 <End of list>
 ; BOLT-NEXT: 00000050 [[#%.16x,ADDR2:]] [[#%.16x,ADDR2B:]]
-; BOLT-NEXT: 00000050 [[#%.16x,ADDR3:]] [[#%.16x,ADDR3B:]]
 ; BOLT-NEXT: 00000050 <End of list>
-; BOLT-NEXT: 00000080 [[#%.16x,ADDR4:]] [[#%.16x,ADDR4B:]]
-; BOLT-NEXT: 00000080 <End of list>
-; BOLT-NEXT: 000000a0 [[#%.16x,ADDR5:]] [[#%.16x,ADDR5B:]]
-; BOLT-NEXT: 000000a0 <End of list>
+; BOLT-NEXT: 00000070 [[#%.16x,ADDR3:]] [[#%.16x,ADDR3B:]]
+; BOLT-NEXT: 00000070 <End of list>
+; BOLT-NEXT: 00000090 [[#%.16x,ADDR4:]] [[#%.16x,ADDR4B:]]
+; BOLT-NEXT: 00000090 <End of list>
+; BOLT-NEXT: 000000b0 <End of list>
+; BOLT-NEXT: 000000c0 [[#%.16x,ADDR5:]] [[#%.16x,ADDR5B:]]
+; BOLT-NEXT: 000000c0 <End of list>
 
 ; BOLT: DW_TAG_compile_unit
 ; BOLT: DW_AT_GNU_dwo_name [DW_FORM_strp] ( .debug_str[0x00000016] = "main.dwo.dwo")
 ; BOLT-NEXT: DW_AT_GNU_dwo_id
 ; BOLT-NEXT: DW_AT_low_pc [DW_FORM_addr] (0x0000000000000000)
-; BOLT-NEXT: DW_AT_ranges [DW_FORM_sec_offset] (0x00000050
-; BOLT-NEXT: [0x[[#ADDR2]], 0x[[#ADDR2B]])
-; BOLT-NEXT: [0x[[#ADDR3]], 0x[[#ADDR3B]]))
+; BOLT-NEXT: DW_AT_ranges [DW_FORM_sec_offset] (0x00000010
+; BOLT-NEXT: [0x[[#ADDR]], 0x[[#ADDRB]])
+; BOLT-NEXT: [0x[[#ADDR1]], 0x[[#ADDR1B]]))
 ; BOLT-NEXT: DW_AT_GNU_addr_base [DW_FORM_sec_offset]  (0x00000000)
-; BOLT-NEXT: DW_AT_GNU_ranges_base [DW_FORM_sec_offset]  (0x00000010)
+; BOLT-NEXT: DW_AT_GNU_ranges_base [DW_FORM_sec_offset]  (0x00000000)
 ; BOLT-NEXT: Compile
 ; BOLT: DW_TAG_compile_unit
 ; BOLT: DW_AT_GNU_dwo_name [DW_FORM_strp] ( .debug_str[0x00000023] = "helper.dwo.dwo")
 ; BOLT-NEXT: DW_AT_GNU_dwo_id
 ; BOLT-NEXT: DW_AT_low_pc [DW_FORM_addr] (0x0000000000000000)
-; BOLT-NEXT: DW_AT_ranges [DW_FORM_sec_offset] (0x000000a0
+; BOLT-NEXT: DW_AT_ranges [DW_FORM_sec_offset] (0x00000090
 ; BOLT-NEXT: [0x[[#ADDR5]], 0x[[#ADDR5B]])
 ; BOLT-NEXT: DW_AT_GNU_addr_base [DW_FORM_sec_offset]  (0x00000010)
-; BOLT-NEXT: DW_AT_GNU_ranges_base [DW_FORM_sec_offset]  (0x00000080)
+; BOLT-NEXT: DW_AT_GNU_ranges_base [DW_FORM_sec_offset]  (0x00000000)
 
 ; PRE-BOLT-DWO-MAIN: version = 0x0004
 ; PRE-BOLT-DWO-MAIN: DW_TAG_compile_unit
@@ -113,13 +115,13 @@
 ; BOLT-DWO-MAIN-NEXT: DW_AT_decl_line
 ; BOLT-DWO-MAIN-NEXT: DW_AT_location [DW_FORM_exprloc]	(DW_OP_GNU_addr_index 0x1)
 ; BOLT-DWO-MAIN: DW_TAG_subprogram [4]
-; BOLT-DWO-MAIN-NEXT: DW_AT_ranges [DW_FORM_sec_offset]	(0x00000000
+; BOLT-DWO-MAIN-NEXT: DW_AT_ranges [DW_FORM_sec_offset]	(0x00000010
 ; BOLT-DWO-MAIN-NEXT: )
 ; BOLT-DWO-MAIN-NEXT: DW_AT_frame_base
 ; BOLT-DWO-MAIN-NEXT: DW_AT_linkage_name [DW_FORM_GNU_str_index]	(indexed (00000003) string = "_Z3usePiS_")
 ; BOLT-DWO-MAIN-NEXT: DW_AT_name [DW_FORM_GNU_str_index]	(indexed (00000004) string = "use")
 ; BOLT-DWO-MAIN: DW_TAG_subprogram [6]
-; BOLT-DWO-MAIN-NEXT: DW_AT_ranges [DW_FORM_sec_offset]	(0x00000020
+; BOLT-DWO-MAIN-NEXT: DW_AT_ranges [DW_FORM_sec_offset]	(0x00000030
 ; BOLT-DWO-MAIN-NEXT: )
 ; BOLT-DWO-MAIN-NEXT: DW_AT_frame_base [DW_FORM_exprloc]	(DW_OP_reg6 RBP)
 ; BOLT-DWO-MAIN-NEXT: DW_AT_name [DW_FORM_GNU_str_index]	(indexed (00000005) string = "main")
@@ -160,4 +162,4 @@
 ; BOLT-DWO-HELPER-NEXT: DW_AT_decl_line
 ; BOLT-DWO-HELPER-NEXT: DW_AT_location [DW_FORM_exprloc]	(DW_OP_GNU_addr_index 0x1)
 ; BOLT-DWO-HELPER: DW_TAG_subprogram [4]
-; BOLT-DWO-HELPER-NEXT: DW_AT_ranges [DW_FORM_sec_offset]	(0x00000000
+; BOLT-DWO-HELPER-NEXT: DW_AT_ranges [DW_FORM_sec_offset]	(0x00000010
diff --git a/bolt/test/X86/dwarf4-df-input-lowpc-ranges-cus.test b/bolt/test/X86/dwarf4-df-input-lowpc-ranges-cus.test
index c9abd02bbb7d9..8dae05db12d73 100644
--- a/bolt/test/X86/dwarf4-df-input-lowpc-ranges-cus.test
+++ b/bolt/test/X86/dwarf4-df-input-lowpc-ranges-cus.test
@@ -17,45 +17,47 @@
 
 ; BOLT: .debug_ranges
 ; BOLT-NEXT: 00000000 <End of list>
-; BOLT-NEXT: 00000010
-; BOLT-NEXT: 00000010
-; BOLT-NEXT: 00000010
+; BOLT-NEXT: 00000010 [[#%.16x,ADDR1:]] [[#%.16x,ADDRB1:]]
+; BOLT-NEXT: 00000010 [[#%.16x,ADDR2:]] [[#%.16x,ADDRB2:]]
+; BOLT-NEXT: 00000010 [[#%.16x,ADDR3:]] [[#%.16x,ADDRB3:]]
+; BOLT-NEXT: 00000010 [[#%.16x,ADDR4:]] [[#%.16x,ADDRB4:]]
+; BOLT-NEXT: 00000010 [[#%.16x,ADDR5:]] [[#%.16x,ADDRB5:]]
+; BOLT-NEXT: 00000010 [[#%.16x,ADDR6:]] [[#%.16x,ADDRB6:]]
+; BOLT-NEXT: 00000010 [[#%.16x,ADDR7:]] [[#%.16x,ADDRB7:]]
 ; BOLT-NEXT: 00000010 <End of list>
-; BOLT-NEXT: 00000050
-; BOLT-NEXT: 00000050
-; BOLT-NEXT: 00000050
-; BOLT-NEXT: 00000050 <End of list>
-; BOLT-NEXT: 00000090 [[#%.16x,ADDR1:]] [[#%.16x,ADDRB1:]]
-; BOLT-NEXT: 00000090 [[#%.16x,ADDR2:]] [[#%.16x,ADDRB2:]]
-; BOLT-NEXT: 00000090 [[#%.16x,ADDR3:]] [[#%.16x,ADDRB3:]]
-; BOLT-NEXT: 00000090 [[#%.16x,ADDR4:]] [[#%.16x,ADDRB4:]]
-; BOLT-NEXT: 00000090 [[#%.16x,ADDR5:]] [[#%.16x,ADDRB5:]]
-; BOLT-NEXT: 00000090 [[#%.16x,ADDR6:]] [[#%.16x,ADDRB6:]]
-; BOLT-NEXT: 00000090 [[#%.16x,ADDR7:]] [[#%.16x,ADDRB7:]]
 ; BOLT-NEXT: 00000090 <End of list>
-; BOLT-NEXT: 00000110
-; BOLT-NEXT: 00000110
-; BOLT-NEXT: 00000110
-; BOLT-NEXT: 00000110 <End of list>
-; BOLT-NEXT: 00000150
-; BOLT-NEXT: 00000150
-; BOLT-NEXT: 00000150
-; BOLT-NEXT: 00000150 <End of list>
-; BOLT-NEXT: 00000190 [[#%.16x,ADDR8:]] [[#%.16x,ADDRB8:]]
-; BOLT-NEXT: 00000190 [[#%.16x,ADDR9:]] [[#%.16x,ADDRB9:]]
-; BOLT-NEXT: 00000190 [[#%.16x,ADDR10:]] [[#%.16x,ADDRB10:]]
-; BOLT-NEXT: 00000190 [[#%.16x,ADDR11:]] [[#%.16x,ADDRB11:]]
-; BOLT-NEXT: 00000190 [[#%.16x,ADDR12:]] [[#%.16x,ADDRB12:]]
-; BOLT-NEXT: 00000190 [[#%.16x,ADDR13:]] [[#%.16x,ADDRB13:]]
-; BOLT-NEXT: 00000190 [[#%.16x,ADDR14:]] [[#%.16x,ADDRB14:]]
-; BOLT-NEXT: 00000190 <End of list>
+; BOLT-NEXT: 000000a0 [[#%.16x,ADDR1:]] [[#%.16x,ADDRB1:]]
+; BOLT-NEXT: 000000a0 [[#%.16x,ADDR2:]] [[#%.16x,ADDRB2:]]
+; BOLT-NEXT: 000000a0 [[#%.16x,ADDR3:]] [[#%.16x,ADDRB3:]]
+; BOLT-NEXT: 000000a0 <End of list>
+; BOLT-NEXT: 000000e0 [[#%.16x,ADDR5:]] [[#%.16x,ADDRB5:]]
+; BOLT-NEXT: 000000e0 [[#%.16x,ADDR6:]] [[#%.16x,ADDRB6:]]
+; BOLT-NEXT: 000000e0 [[#%.16x,ADDR7:]] [[#%.16x,ADDRB7:]]
+; BOLT-NEXT: 000000e0 <End of list>
+; BOLT-NEXT: 00000120 [[#%.16x,ADDR8:]] [[#%.16x,ADDRB8:]]
+; BOLT-NEXT: 00000120 [[#%.16x,ADDR9:]] [[#%.16x,ADDRB9:]]
+; BOLT-NEXT: 00000120 [[#%.16x,ADDR10:]] [[#%.16x,ADDRB10:]]
+; BOLT-NEXT: 00000120 [[#%.16x,ADDR11:]] [[#%.16x,ADDRB11:]]
+; BOLT-NEXT: 00000120 [[#%.16x,ADDR12:]] [[#%.16x,ADDRB12:]]
+; BOLT-NEXT: 00000120 [[#%.16x,ADDR13:]] [[#%.16x,ADDRB13:]]
+; BOLT-NEXT: 00000120 [[#%.16x,ADDR14:]] [[#%.16x,ADDRB14:]]
+; BOLT-NEXT: 00000120 <End of list>
+; BOLT-NEXT: 000001a0 <End of list>
+; BOLT-NEXT: 000001b0 [[#%.16x,ADDR8:]] [[#%.16x,ADDRB8:]]
+; BOLT-NEXT: 000001b0 [[#%.16x,ADDR9:]] [[#%.16x,ADDRB9:]]
+; BOLT-NEXT: 000001b0 [[#%.16x,ADDR10:]] [[#%.16x,ADDRB10:]]
+; BOLT-NEXT: 000001b0 <End of list>
+; BOLT-NEXT: 000001f0 [[#%.16x,ADDR12:]] [[#%.16x,ADDRB12:]]
+; BOLT-NEXT: 000001f0 [[#%.16x,ADDR13:]] [[#%.16x,ADDRB13:]]
+; BOLT-NEXT: 000001f0 [[#%.16x,ADDR14:]] [[#%.16x,ADDRB14:]]
+; BOLT-NEXT: 000001f0 <End of list>
 
 ; BOLT: DW_TAG_compile_unit
 ; BOLT: DW_AT_GNU_dwo_name [DW_FORM_strp] ( .debug_str[0x{{[0-9a-fA-F]+}}] = "main.dwo.dwo")
 ; BOLT-NEXT: DW_AT_GNU_dwo_id
-; BOLT-NEXT: DW_AT_GNU_ranges_base [DW_FORM_sec_offset]  (0x00000010)
+; BOLT-NEXT: DW_AT_GNU_ranges_base [DW_FORM_sec_offset]  (0x00000000)
 ; BOLT-NEXT: DW_AT_low_pc [DW_FORM_addr] (0x0000000000000000)
-; BOLT-NEXT: DW_AT_ranges [DW_FORM_sec_offset] (0x00000090
+; BOLT-NEXT: DW_AT_ranges [DW_FORM_sec_offset] (0x00000010
 ; BOLT-NEXT: [0x[[#ADDR1]], 0x[[#ADDRB1]])
 ; BOLT-NEXT: [0x[[#ADDR2]], 0x[[#ADDRB2]])
 ; BOLT-NEXT: [0x[[#ADDR3]], 0x[[#ADDRB3]])
@@ -68,9 +70,9 @@
 ; BOLT: DW_TAG_compile_unit
 ; BOLT: DW_AT_GNU_dwo_name [DW_FORM_strp] ( .debug_str[0x{{[0-9a-fA-F]+}}] = "mainOther.dwo.dwo")
 ; BOLT-NEXT: DW_AT_GNU_dwo_id
-; BOLT-NEXT: DW_AT_GNU_ranges_base [DW_FORM_sec_offset]  (0x00000110)
+; BOLT-NEXT: DW_AT_GNU_ranges_base [DW_FORM_sec_offset]  (0x00000000)
 ; BOLT-NEXT: DW_AT_low_pc [DW_FORM_addr] (0x0000000000000000)
-; BOLT-NEXT: DW_AT_ranges [DW_FORM_sec_offset] (0x00000190
+; BOLT-NEXT: DW_AT_ranges [DW_FORM_sec_offset] (0x00000120
 ; BOLT-NEXT: [0x[[#ADDR8]], 0x[[#ADDRB8]])
 ; BOLT-NEXT: [0x[[#ADDR9]], 0x[[#ADDRB9]])
 ; BOLT-NEXT: [0x[[#ADDR10]], 0x[[#ADDRB10]])
@@ -81,17 +83,17 @@
 ; BOLT-NEXT: DW_AT_GNU_addr_base [DW_FORM_sec_offset]  (0x00000018)
 
 ; BOLT-DWO-MAIN:        DW_TAG_subprogram
-; BOLT-DWO-MAIN-NEXT:   DW_AT_ranges [DW_FORM_sec_offset] (0x00000000
+; BOLT-DWO-MAIN-NEXT:   DW_AT_ranges [DW_FORM_sec_offset] (0x00000010
 ; BOLT-DWO-MAIN:        DW_TAG_subprogram
 ; BOLT-DWO-MAIN:        DW_TAG_subprogram
 ; BOLT-DWO-MAIN:        DW_TAG_subprogram
 ; BOLT-DWO-MAIN:        DW_TAG_subprogram
-; BOLT-DWO-MAIN-NEXT:   DW_AT_ranges [DW_FORM_sec_offset] (0x00000040
+; BOL...
[truncated]

@ayermolo
Copy link
Contributor

This isn't an NFC change. You are not only refactoring updateDWARFObjectAddressRanges, you are also introducing multiple instances of RangeWriter for DWARF4.

@sayhaan sayhaan changed the title [BOLT][DWARF][NFC] Refactor updateDWARFObjectAddressRanges [BOLT][DWARF] Refactor updateDWARFObjectAddressRanges Jun 20, 2024
ayermolo
ayermolo requested changes Jun 20, 2024
View reviewed changes
aaupov
aaupov reviewed Jun 20, 2024
View reviewed changes
Copy link
Contributor

@aaupov aaupov left a comment

Choose a reason for hiding this comment

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

Left comments about coding style

ayermolo
ayermolo reviewed Jun 21, 2024
View reviewed changes
@sayhaan sayhaan changed the title [BOLT][DWARF] Refactor updateDWARFObjectAddressRanges [BOLT][DWARF] Refactor legacy ranges writers Jun 21, 2024
ayermolo
ayermolo reviewed Jun 21, 2024
View reviewed changes
@sayhaan sayhaan requested a review from JDevlieghere as a code owner June 21, 2024 19:40
@sayhaan
[BOLT][DWARF][NFC] Refactor updateDWARFObjectAddressRanges
c13de83 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: https://phabricator.intern.facebook.com/D58679290
@ayermolo ayermolo removed the request for review from JDevlieghere June 21, 2024 21:25
@sayhaan sayhaan requested a review from ayermolo June 24, 2024 16:20
@maksfb
Copy link
Contributor

maksfb commented Jun 24, 2024

This isn't an NFC change. You are not only refactoring updateDWARFObjectAddressRanges, you are also introducing multiple instances of RangeWriter for DWARF4.

@sayhaan Could you please add to the summary how this PR changes DWARF4 generation? I.e. what is different and why.

aaupov
aaupov approved these changes Jun 24, 2024
View reviewed changes
Copy link
Contributor

@aaupov aaupov left a comment

Choose a reason for hiding this comment

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

Please avoid force-pushing to preserve review comments. Code-style-wise LGTM.

ayermolo
ayermolo approved these changes Jun 24, 2024
View reviewed changes
Copy link
Contributor

@ayermolo ayermolo left a comment

Choose a reason for hiding this comment

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

LGTM
Address comments by others.

maksfb
maksfb reviewed Jun 24, 2024
View reviewed changes
maksfb
maksfb reviewed Jun 25, 2024
View reviewed changes
ayermolo
ayermolo reviewed Jun 25, 2024
View reviewed changes
@sayhaan sayhaan requested review from maksfb, aaupov and ayermolo June 25, 2024 16:49
ayermolo
ayermolo reviewed Jun 25, 2024
View reviewed changes
ayermolo
ayermolo approved these changes Jun 25, 2024
View reviewed changes
Copy link
Contributor

@ayermolo ayermolo left a comment

Choose a reason for hiding this comment

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

couple small nits, otherwise looks good

maksfb
maksfb approved these changes Jun 25, 2024
View reviewed changes
Copy link
Contributor

@maksfb maksfb left a comment

Choose a reason for hiding this comment

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

LGTM

@ayermolo
Copy link
Contributor

hmm I was thinking, maybe as a followup PR.
DIE in DWARFRagnes writer doesn't make sense for the instance that handles main binary section.
Also since for Rangelistswriter inherits from it, for DWARF5 there is a super slight size penalty.
Maybe worth creating another class that inherits from DWARFRangeswriter. Sticking DIE and access APIs there.
Storing that instance in the legacy map.
@maksfb WDYT?

@maksfb
Copy link
Contributor

maksfb commented Jun 25, 2024

hmm I was thinking, maybe as a followup PR. DIE in DWARFRagnes writer doesn't make sense for the instance that handles main binary section. Also since for Rangelistswriter inherits from it, for DWARF5 there is a super slight size penalty. Maybe worth creating another class that inherits from DWARFRangeswriter. Sticking DIE and access APIs there. Storing that instance in the legacy map. @maksfb WDYT?

Don't think it's worth it for one field in a class without a ridiculously large number of instances.

@sayhaan sayhaan merged commit 5828b04 into llvm:main Jul 3, 2024
kbluck pushed a commit to kbluck/llvm-project that referenced this pull request Jul 6, 2024
@sayhaan @kbluck
[BOLT][DWARF] Refactor legacy ranges writers (llvm#96006)
5603924 
Refactors legacy ranges writers to create a writer for each instance of
a DWO file.

We now write out everything into .debug_ranges after the all the DWO
files are processed. This also changes the order that ranges is written
out in, as before we wrote out while in the main CU processing loop and
we now iterate through the CU buckets created by partitionCUs, after the
main processing loop.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maksfb maksfb maksfb approved these changes

@ayermolo ayermolo ayermolo approved these changes

@rafaelauler rafaelauler Awaiting requested review from rafaelauler rafaelauler is a code owner

@dcci dcci Awaiting requested review from dcci

@aaupov aaupov Awaiting requested review from aaupov aaupov is a code owner

Assignees
No one assigned
Labels
BOLT
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sayhaan @llvmbot @ayermolo @maksfb @aaupov