[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:
> 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
```


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


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


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/debug_pub_tables_error_cases.s:17
+
+## The next few sets does not contain all required fields in the header.
+# CHECK-NEXT: length = 0x00000001, format = DWARF32, version = 0x0000, unit_offset = 0x00000000, unit_size = 0x00000000
----------------
does not -> do not


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/debug_pub_tables_error_cases.s:44-45
+    .short 2                    # Version
+    .long 0x32                  # Debug Info offset
+    .byte 1, 2, 3               # Debug Info Length (truncated)
+.LSet2End:
----------------
For consistency, either offset -> Offset or Length -> length (here and below).


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

https://reviews.llvm.org/D83050