[PATCH] D83050: [DebugInfo] Add more checks to parsing .debug_pub* sections.

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin marked 3 inline comments as done.
ikudrin added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugPubTable.cpp:57
+          createStringError(errc::invalid_argument,
+                            "name lookup table at offset 0x%" PRIx64
+                            " does not have a complete header: %s",
----------------
dblaikie wrote:
> Is it worth saying "name lookup table" here (& in related errors) - seems a bit redundant when the caller will add the specific section name?
I would be really grateful for the better wording.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugPubTable.cpp:74-75
+
+    // Note: do not drop the last set even if there are parsing errors.
+    // This way we can, at least, dump the header and possibly some name tuples.
+    if (!C) {
----------------
dblaikie wrote:
> Hmm, why only do this on the last one?
> If the goal is to be able to parse/dump things that might be a bit broken (which seems generally good) I think we should parse from the length to however long the length says (unless it extends beyond the section) - terminate early if the list terminates early (& warn about the fact that it terminated before its length) & then parse the next thing at current start + length. Don't think that needs a special case for the last one.
Well, it looks the code does exactly that you say. Maybe I was not clear enough in the comment. I meant the set that was just read. The error handlers before the `while` loop drop the last added set with `Sets.pop_back()` because even header for it can not be parsed. The error handlers after the loop preserve it and the comment was aimed to explain that difference.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugPubTable.cpp:76-89
+    if (!C) {
+      RecoverableErrorHandler(createStringError(
+          errc::invalid_argument,
+          "name lookup table at offset 0x%" PRIx64
+          " terminated prematurely: %s",
+          SetOffset, toString(std::move(C.takeError())).c_str()));
+      continue;
----------------
dblaikie wrote:
> I think phrasing of these two might use some improvement. "terminated prematurely" actually would make me think of the second case - where the list had a terminator before the prefix-encoded length was reached, rather than that the prefix-encoded length was reached before the list ended.
> 
> Perhaps "terminated before the expected length was reached" and "reached the expected length without encountering a terminator"? They're both a bit of a mouthful though... open to ideas.
These wordings are already better than mine. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83050