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

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin 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:
> 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?


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/debug_pub_tables_error_cases.s:3
+
+## All four public name sections share the same parser, but slightly different
+## code paths are used to reach it. Do a comprehensive check for one of the
----------------
jhenderson wrote:
> I think this is the first time I've heard the term "public name sections" being used. Is this called that in the standard? Otherwise, I might suggest using a different phrasing (though don't necessarily know what).
Well, the standard sometimes uses the term "name lookup tables". Do you think now the comment sound better?


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/debug_pub_tables_error_cases.s:7-8
+
+# RUN: not llvm-dwarfdump -debug-gnu-pubnames %t 2> %t.err | FileCheck %s
+# RUN: FileCheck %s --input-file=%t.err --check-prefix=ERR
+
----------------
jhenderson wrote:
> I don't mind too much either way, especially given the difficulties I recently had with the debug line equivalent test, but is there a particular reason you've kept the two streams separate? By combining them you can show the relative position of output for the common case of the streams being combined.
That is done to improve readability. The error messages are printed during parsing and dumping of all sets in the section comes after that. Thus, if we want to check all the messages at once, the error messages (or dumping messages) have to be separated from the corresponding lines of source code.


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

https://reviews.llvm.org/D83050