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

[Maksim Panchenko via Phabricator via llvm-commits]

maksfb added inline comments.


================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:738
+        else
+          llvm_unreachable("DW_AT_signature form is not supported.");
       }
----------------
ayermolo wrote:
> 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: " ?
What's the desired action? Do you want to terminate the processing? Alternatively, you can return an error and handle it from a caller.


================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:1289
+  }
+  return StringRef(reinterpret_cast<const char *>(DWOTUSection.c_str()),
+                   DWOTUSection.size());
----------------
ayermolo wrote:
> 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.
Sorry, I don't follow. Where is null-byte bug manifesting itself? Both, `std::string` and `StringRef` allow null characters, right?


================
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.");
----------------
ayermolo wrote:
> 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.
If you are validating an internal assumption, then it's fine to leave the `assert()` as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126087