[PATCH] D126087: [BOLT][DWARF] Fix TU Index handling for DWARF4/5.

[Maksim Panchenko via Phabricator via llvm-commits]

maksfb added a comment.

> Added support for handling it when DWP output or DWP is output for DWARF4. For DWARF5 it just handles it as an input for now.

Could you please rephrase?



================
Comment at: bolt/include/bolt/Rewrite/DWARFRewriter.h:31
   DWARFRewriter() = delete;
+  using DebugTypesSignaturesPerCU =
+      std::unordered_map<uint64_t, std::unordered_set<uint64_t>>;
----------------
Or `DebugTypesSignaturesPerCUMap`.


================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:738
+        else
+          llvm_unreachable("DW_AT_signature form is not supported.");
       }
----------------
If you are validating the input here, then it's better to issue an error as `llvm_unreachable()` has no effect in no-assert builds.


================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:1259
+};
+using TUContributionVector = std::vector<TUContribution>;
+/// Extracts TU entries per CU from DWP.
----------------



================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:1261
+/// Extracts TU entries per CU from DWP.
+static StringRef extractDWOTUFromDWP(
+    DWARFRewriter::DebugTypesSignaturesPerCU &TypeSignaturesPerCU,
----------------
Could you add detailed comments? E.g., it's unclear which parameters are input and which are output.


================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:1289
+  }
+  return StringRef(reinterpret_cast<const char *>(DWOTUSection.c_str()),
+                   DWOTUSection.size());
----------------
Is `reinterpret_cast<>` necessary here? Does `StringRef(DWOTUSection)` not work? Do you actually need to return `StringRef` from the function, as it's always a reference to `DWOTUSection` which is a parameter to the function?


================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:1402
+    } else if (Version != CU->getVersion()) {
+      llvm_unreachable("Incompatible DWARF compile unit versions.");
+    }
----------------
Should this be an error instead?


================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:1505
+    if (!TUContributionsToCU.empty()) {
+      auto Index = getContributionIndex(DW_SECT_EXT_TYPES, IndexVersion);
+      for (const TUContribution &TUC : TUContributionsToCU) {
----------------



================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:1615
+      if (IsDWP && SectionName == "debug_types.dwo") {
+        assert(TUIndex &&
+               "DWP Input with .debug_types.dwo section with TU Index.");
----------------
Likewise, should this be an error?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126087