[PATCH] D126087: [BOLT][DWARF] Fix TU Index handling for DWARF4/5.
Maksim Panchenko via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon May 23 17:24:31 PDT 2022
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
More information about the llvm-commits
mailing list