[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: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;
----------------
jhenderson wrote:
> ikudrin wrote:
> > 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!
> How about the first one be just generic, allowing the cursor's error to provide the context (something like "name lookup table at offset 0x12345678 parsing failed: ..."). I'm actually okay with @ikudrin's current wording for the second one, since @dblaikie's suggestion is as much of a mouthful when you add in the other context.
The suggestion wasn't for brevity, but clarity. I found the original messages unclear & was hoping to clarify them.

What are the two messages in total (with all the added context, for both too short and too long) & how clear are they?


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

https://reviews.llvm.org/D83050