[PATCH] D121876: [BOLT][DWARF] Implementation of monolithic DWARF5.

[Maksim Panchenko via Phabricator via llvm-commits]

maksfb added inline comments.


================
Comment at: bolt/include/bolt/Core/DebugData.h:187
+class DebugAddrWriter;
+class DebugRangelistsSectionWriter : public DebugRangesSectionWriter {
+public:
----------------
nit


================
Comment at: bolt/include/bolt/Core/DebugData.h:214
+
+  /// Returns a buffer containing Ranges.
+  void finalizeSection();
----------------
The comment is out of sync.


================
Comment at: bolt/include/bolt/Core/DebugData.h:217
+
+  RangesWriterKind getKind() const {
+    return DebugRangesSectionWriter::getKind();
----------------
Why do you need to redefine `getKind()`?


================
Comment at: bolt/include/bolt/Core/DebugData.h:231
+  /// body.
+  uint32_t CurrentOffset{0};
+  /// Contains relative offset of each range list entry.
----------------
Is `uint32_t` enough to support DWARF-64 in the future?


================
Comment at: bolt/include/bolt/Core/DebugData.h:290
 
   /// Given DWOID returns offset of this CU in to .debug_addr section.
+  virtual uint64_t getOffset(DWARFUnit &Unit);
----------------
Update the comment.


================
Comment at: bolt/include/bolt/Core/DebugData.h:492
+
+  static uint32_t InvalidIndex;
+  static uint32_t InvalidLocListsBaseAttrOffset;
----------------
Could this be `constexpr` and defined here?


================
Comment at: bolt/include/bolt/Core/DebugData.h:493
+  static uint32_t InvalidIndex;
+  static uint32_t InvalidLocListsBaseAttrOffset;
+
----------------
Likewise.


================
Comment at: bolt/include/bolt/Core/DebugData.h:1099
+  // Returns DWARF Version for this line table.
+  uint16_t getDwarfVersion() { return DwarfVersion; }
 };
----------------
nit


================
Comment at: bolt/lib/Core/BinaryContext.cpp:1535
     }
+    uint32_t Offset = DwarfVersion < 5 ? 1 : 0;
     for (size_t I = 0, Size = FileNames.size(); I != Size; ++I) {
----------------
nit


================
Comment at: bolt/lib/Core/DebugData.cpp:168
+
+struct LoclistsRangelistsHeader {
+  UnitLengthType UnitLength; // Size of loclist entris section, not including
----------------
nit


================
Comment at: bolt/lib/Core/DebugData.cpp:186
+  // the length field itself
+  uint32_t HeaderSize =
+      getDWARF5RngListLocListHeaderSize() - sizeof(UnitLengthType);
----------------



================
Comment at: bolt/lib/Core/DebugData.cpp:209
+                           support::little);
+    uint32_t Index = AddrWriter->getIndexFromAddress(Range.LowPC, CUId);
+    encodeULEB128(Index, *CUBodyStream);
----------------



================
Comment at: bolt/lib/Core/DebugData.cpp:226
+  constexpr uint32_t SizeOfArrayEntry = 4;
+  uint32_t SizeOfArraySection = RangeEntries.size() * SizeOfArrayEntry;
+  for (uint32_t Offset : RangeEntries)
----------------



================
Comment at: bolt/lib/Core/DebugData.cpp:404
+  raw_svector_ostream AddressStream(Buffer);
+  endianness Endian =
+      BC->DwCtx->isLittleEndian() ? support::little : support::big;
----------------



================
Comment at: bolt/lib/Core/DebugData.cpp:414
+    uint8_t AddrSize = CU->getAddressByteSize();
+    auto AM = AddressMaps.find(CUId);
+    // A case where CU has entry in .debug_addr, but we don't modify addresses
----------------
nit


================
Comment at: bolt/lib/Core/DebugData.cpp:446
+    // Writing out Header
+    uint32_t Length = SortedMap.size() * AddrSize + 4;
+    support::endian::write(AddressStream, Length, Endian);
----------------



================
Comment at: bolt/lib/Core/DebugData.cpp:458
+      default:
+        assert(false && "Address Size is invalid.");
+        break;
----------------



================
Comment at: bolt/lib/Core/DebugData.cpp:481
+uint64_t DebugAddrWriter::getOffset(DWARFUnit &Unit) {
+  auto DWOId = Unit.getDWOId();
+  assert(DWOId && "Can't get offset, not a skeleton CU.");
----------------
Expand `auto`.


================
Comment at: bolt/lib/Core/DebugData.cpp:577
+
+  uint32_t SizeOfArraySection = Patches.size() * sizeof(uint32_t);
+  std::sort(Patches.begin(), Patches.end(),
----------------



================
Comment at: bolt/lib/Core/DebugData.cpp:607
+                             support::little);
+      uint32_t Index = AddrWriter->getIndexFromAddress(Entry.LowPC, CUId);
+      encodeULEB128(Index, *LocBodyStream);
----------------



================
Comment at: bolt/lib/Core/DebugData.cpp:653
                              support::little);
-      uint32_t Index = AddrWriter->getIndexFromAddress(Entry.LowPC, DWOId);
+      uint32_t Index = AddrWriter->getIndexFromAddress(Entry.LowPC, CUId);
       encodeULEB128(Index, *LocStream);
----------------



================
Comment at: bolt/lib/Core/DebugData.cpp:1525-1526
+    if (Err) {
+      llvm::errs()
+          << "BOLT-ERROR: could not extract string from .debug_line_str.";
+      continue;
----------------



================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:83-87
+  for (dwarf::Attribute &Attr : Attrs) {
+    if (Optional<AttrInfo> Info = findAttributeInfo(DIE, Attr)) {
+      return Info;
+    }
+  }
----------------



================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:495
                   return false;
-                case dwarf::DW_LLE_base_address:
+                case dwarf::DW_LLE_base_address: {
                   assert(Entry.SectionIndex == SectionedAddress::UndefSection &&
----------------
No need to introduce the new scope unless you need it.


================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:565-566
+            } else {
+              // I have seen this building CLANG-15 where subprogram was removed
+              // and has address of 0. Adding this entry to output to preserve
+              // debug information.
----------------



================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:1588
+  // low_pc/high_pc into ranges. The CU originally didn't have DW_AT_*_base, so
+  // we re adding it here.
+  if (RangesBase)
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121876/new/

https://reviews.llvm.org/D121876