[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/lib/Rewrite/DWARFRewriter.cpp:738
+        else
+          llvm_unreachable("DW_AT_signature form is not supported.");
       }
----------------
maksfb wrote:
> 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.
yes, I think we should terminate process.


================
Comment at: bolt/lib/Rewrite/DWARFRewriter.cpp:1289
+  }
+  return StringRef(reinterpret_cast<const char *>(DWOTUSection.c_str()),
+                   DWOTUSection.size());
----------------
maksfb wrote:
> 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?
I ran into this issue in updateDebugData. Don't quite remember all the details, but when doing conversion from std::string to StringRef using StringRef(std::string) semantics if there were null bytes in std::string it will think that's the end of the string, so StringRef will get the length wrong. Thus StringRef will only capture part of the string.


================
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:
> 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.
Yep, exactly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126087