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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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;
----------------
dblaikie wrote:
> ikudrin wrote:
> > jhenderson wrote:
> > > dblaikie wrote:
> > > > 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?
> > > Taken from the test case:
> > > 
> > > ```
> > > error: name lookup table at offset 0x5f parsing failed: no null terminated string at offset 0x72
> > > ```
> > > (the "no null teminated" bit might differ depending on the exact failure, e.g. "unexpected end of data at offset 0x4c while reading [0x4c, 0x4d)")
> > > 
> > > ```
> > > error: name lookup table at offset 0x75 has an unexpected terminator at offset 0x8c
> > > ```
> > Thanks, @jhenderson! @dblaikie are you OK with these messages or going to suggest a better alternative?
> This one sounds OK (guess it could be more precise in this case "bounds reached without finding expected null terminator" perhaps - but I realize that's fairly orthogonal to this patch & could be improved in the general DataExtractor infrastructure) - honestly the verbosity of these messages doesn't seem like a problem to me. They should be pretty rare & when they do come up, the more explicit/precise the better, it seems to me.
> ```
> error: name lookup table at offset 0x5f parsing failed: no null terminated string at offset 0x72
> ```
> This one
> ```
> error: name lookup table at offset 0x75 has an unexpected terminator at offset 0x8c
> ```
> Still seems like it could be more precise - exactly why was the terminator unexpected? "has a terminator at 0x8c before the expected end at 0x??" perhaps.
> "has a terminator at 0x8c before the expected end at 0x??" perhaps.

Sounds good to me.


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

https://reviews.llvm.org/D83050