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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Probably worth changing the `== 5` version checks to `>= 5` since most things stay the same between DWARF versions, and the things that don't are the exceptions/things we'll need to touch on the next version (& nice to not have to touch/update code for things that stay the same)



================
Comment at: bolt/include/bolt/Core/DebugData.h:375-377
+    if (Optional<uint64_t> DWOId = Unit.getDWOId())
+      return *DWOId;
+    return Unit.getOffset();
----------------
That seems like a somewhat problematic API - returning two very different kinds of data (the DWO_ID or the unit offset) seems quite easy to misuse this?


================
Comment at: bolt/include/bolt/Core/DebugData.h:386
+  DebugStrOffsetsWriter() = delete;
+  DebugStrOffsetsWriter(BinaryContext *Bc) : BC(Bc) { create(); }
+
----------------
Probably name the parameter the same as the member, rather than one character different/lower case.


================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:208-209
   for (std::unique_ptr<DWARFUnit> &CU : BC.DwCtx->compile_units()) {
-    if (CU->getVersion() == 5) {
+    uint16_t Dwarfversion = CU->getVersion();
+    if (Dwarfversion == 5) {
       uint32_t AttrInfoOffset =
----------------
Probably an uppercase `V` since that's a separate word within the name?


================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:306
+        RangesBase = RangesSectionWriter->getSectionOffset();
+        // For DWARF5 there is now .debug_rangelists.dwo, so don't need to
+        // update rnglists base.
----------------



================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:320
+      if (Unit->getVersion() == 5)
+        reinterpret_cast<DebugRangeListsSectionWriter *>(
+            TempRangesSectionWriter)
----------------
reinterpret_cast seems a bit scary here - perhaps `TempRangesSectionWriter` should be a `void*` if it could be a pointer to two unrelated types (if it's a pointer to a base class that covers both ranges and rnglists, then a static_cast should suffice for the downcasting from base pointer to derived pointer)


================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:327-328
       std::lock_guard<std::mutex> Lock(AccessMutex);
-      DebugLocWriter = LocListWritersByCU[CUIndex].get();
+      if (LocListWritersByCU.count(CUIndex))
+        DebugLocWriter = LocListWritersByCU[CUIndex].get();
     }
----------------
to avoid two lookups (one for count, the other for op[]) perhaps this code should use find?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122988