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

[Alexander Yermolovich via Phabricator via llvm-commits]

ayermolo added inline comments.


================
Comment at: bolt/include/bolt/Rewrite/DWARFRewriter.h:31
   DWARFRewriter() = delete;
+  using DebugTypesSignaturesPerCU =
+      std::unordered_map<uint64_t, std::unordered_set<uint64_t>>;
----------------
maksfb wrote:
> Or `DebugTypesSignaturesPerCUMap`.
oh yeah, that's better. thx


================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:738
+        else
+          llvm_unreachable("DW_AT_signature form is not supported.");
       }
----------------
maksfb wrote:
> 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.
Ah right forgot about that. K, will change to llvm::errs() << "BOLT-ERROR: " ?


================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:1289
+  }
+  return StringRef(reinterpret_cast<const char *>(DWOTUSection.c_str()),
+                   DWOTUSection.size());
----------------
maksfb wrote:
> 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?
This avoids a bug where a null byte is interpreted as the end of string.
I thought returning StringRef would be cleaner then putting this logic on the callee side.


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


================
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.");
----------------
maksfb wrote:
> Likewise, should this be an error?
Because we are validating input? Way I thought about it, is that it's not a common path, and more of a sanity check so assert would be more appropriate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126087