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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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",
----------------
Is it worth saying "name lookup table" here (& in related errors) - seems a bit redundant when the caller will add the specific section name?


================
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) {
----------------
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.


================
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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83050