[PATCH] D122988: [BOLT][DWARF] Add version 5 split dwarf support

[Alexander Yermolovich via Phabricator via llvm-commits]

ayermolo added a comment.

Comments from @maksfb



================
Comment at: bolt/include/bolt/Core/DebugData.h:219
+
+  RangesWriterKind getKind() const {
+    return DebugRangesSectionWriter::getKind();
----------------
delete


================
Comment at: bolt/include/bolt/Core/DebugData.h:230
+  /// Used to find unique CU ID.
+  DWARFUnit *CU;
   /// Current relative offset of range list entry within this CUs rangelist
----------------
make reference


================
Comment at: bolt/include/bolt/Core/DebugData.h:348
+
+  virtual uint64_t getCUId(DWARFUnit &Unit) {
+    assert(Unit.getDWOId() && "Unit is not Skeleton CU.");
----------------
add documentation.


================
Comment at: bolt/include/bolt/Core/DebugData.h:386
+  DebugStrOffsetsWriter() = delete;
+  DebugStrOffsetsWriter(BinaryContext *Bc) : BC(Bc) { create(); }
+
----------------
Pass BC by reference


================
Comment at: bolt/include/bolt/Core/DebugData.h:399
+  /// Returns False if no strings were added to .debug_str.
+  bool isInitialized() const { return !StrOffsetsBuffer->empty(); }
+
----------------
isFinalized


================
Comment at: bolt/include/bolt/Core/DebugData.h:402
+  /// Returns buffer containing .debug_str_offsets.
+  std::unique_ptr<DebugStrOffsetsBufferVector> finalize() {
+    return std::move(StrOffsetsBuffer);
----------------
releaseBuffer()


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


================
Comment at: bolt/lib/Core/DebugData.cpp:358
       continue;
-    auto AM = AddressMaps.find(*DWOId);
+    uint64_t CUID = getCUId(*CU.get());
+    auto AM = AddressMaps.find(CUID);
----------------
const


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


================
Comment at: bolt/lib/Core/DebugData.cpp:483
 uint64_t DebugAddrWriter::getOffset(DWARFUnit &Unit) {
-  Optional<uint64_t> DWOId = Unit.getDWOId();
-  assert(DWOId && "Can't get offset, not a skeleton CU.");
-  auto Iter = DWOIdToOffsetMap.find(*DWOId);
+  uint64_t CUID = getCUId(Unit);
+  assert(CUID && "Can't get offset, not a skeleton CU.");
----------------
const
getCUId -->getCUID


================
Comment at: bolt/lib/Core/DebugData.cpp:1058
 
+void DebugStrOffsetsWriter::create() {
+  StrOffsetsBuffer = std::make_unique<DebugStrOffsetsBufferVector>();
----------------
remove method, move to constructor


================
Comment at: bolt/lib/Core/DebugData.cpp:1069
+
+  uint8_t DwarfOffsetByteSize = Contr->getDwarfOffsetByteSize();
+  assert(DwarfOffsetByteSize == 4 &&
----------------
const


================
Comment at: bolt/lib/Core/DebugData.cpp:1071
+  assert(DwarfOffsetByteSize == 4 &&
+         "Darf String Offsets Byte Size is not supported.");
+  uint32_t Index = 0;
----------------
Darf --> Dwarf


================
Comment at: bolt/lib/Core/DebugData.cpp:1075
+    IndexToAddressMap[Index++] = *(
+        const uint32_t *)(StrOffsetsSection.Data.data() + Contr->Base + Offset);
+}
----------------
use static_cast


================
Comment at: bolt/lib/Core/DebugData.cpp:1093
+                         support::little);
+  for (const auto &Entry : IndexToAddressMap) {
+    support::endian::write(*StrOffsetsStream, Entry.second, support::little);
----------------
remove curly braces


================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:166
+void DWARFRewriter::addStringHelper(DebugInfoBinaryPatcher &DebugInfoPatcher,
+                                    DWARFUnit &Unit, AttrInfo &AttrInfoVal,
+                                    StringRef Str) {
----------------
const Unit, AttrInfoVal


================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:168
+                                    StringRef Str) {
+  uint32_t NewOffset = StrWriter->addString(Str);
+  if (Unit.getVersion() == 5) {
----------------
const


================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:208
   for (std::unique_ptr<DWARFUnit> &CU : BC.DwCtx->compile_units()) {
-    if (CU->getVersion() == 5) {
+    uint16_t Dwarfversion = CU->getVersion();
+    if (Dwarfversion == 5) {
----------------
const DwarfVersion


================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:304
+        TempRangesSectionWriter = RangeListsWritersByCU[*DWOId].get();
+      } else {
+        RangesBase = RangesSectionWriter->getSectionOffset();
----------------
add else if for DWARF < 5 else case llvm_unreachable


================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:322
+            TempRangesSectionWriter)
+            ->finalizeSection();
     }
----------------
make finalizeSection virtual and do nothing for dwarf4.


================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:335
           RangesSectionWriter.get())
-          ->initSection(Unit->getOffset());
+          ->initSection(*Unit);
+      StrOffstsWriter->finalizeSection();
----------------
Make initSection virtual and do nothing for DWARF4


================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:1175
+    if (!SectionName.equals("debug_str.dwo"))
+      errs() << "BOLT-WARNING: Unsupported Debug section: " << SectionName
+             << "\n";
----------------



================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:1719
     if (LowForm == dwarf::DW_FORM_addrx) {
-      uint32_t Index =
-          AddrWriter->getIndexFromAddress(0, DIE.getDwarfUnit()->getOffset());
+      uint32_t Index = AddrWriter->getIndexFromAddress(0, *DIE.getDwarfUnit());
       DebugInfoPatcher.addUDataPatch(LowPCOffset, Index, LowPCVal->Size);
----------------
const


================
Comment at: bolt/test/X86/dwarf4-df-dualcu.test:23
+
+; Testing dwarf5 split dwarf for two CUs. Making sure DW_AT_low_pc/DW_AT_high_pc are converted correctly in the binary and in dwo.
+; Checking that DW_AT_location [DW_FORM_exprloc]	(DW_OP_addrx ##) are updated correctly.
----------------
dwarf4


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122988