[PATCH] D72155: [DebugInfo] Make incorrect debug line extended opcode length non-fatal

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

In D72155#1813778 <https://reviews.llvm.org/D72155#1813778>, @jhenderson wrote:

> In D72155#1813355 <https://reviews.llvm.org/D72155#1813355>, @dblaikie wrote:
>
> > Out of curiosity: what's your broader goal with this work? (it'll help understand what's in-scope and out of scope, and better understand the framing when reviewing changes)
>
>
> There's two parts to this:
>
> 1. Making it easier for consumers to continue and try to do something with slightly bad output. One of the problems with using the unrecoverable errors is that it prevents people even trying to iterate over later tables.
> 2. (this one's not specific to this patch, but some other patches in this area I've been doing are related): we have some local code that uses the DebugInfo library to read a line table. In order for this code to be sound, we need to make sure the line table makes sense. The parser in its current state doesn't pick up on a number of bad situations, so we added some local patches to detect these other errors (local because we didn't have time at that point to try to push them into the open source). We'd like to now get these into the opensource library, to avoid merge conflicts.


Ah, OK - thanks for the context! (out of curiosity, though not necessary, who is "we"?)

So you'd like/are working on making libDebugInfoDWARF more pedantic/precise/detect more errors? Specifically only for the line table? Do you have any interest in adding fuzz testing for this, or are your needs less severe than would justify fuzzing?

In D72155#1814130 <https://reviews.llvm.org/D72155#1814130>, @jhenderson wrote:

> Rebased + changed to assume the stated opcode length is always correct.


Looks good - thanks!



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:686-687
       }
       // Make sure the stated and parsed lengths are the same.
-      // Otherwise we have an unparseable line-number program.
-      if (*OffsetPtr - ExtOffset != Len)
-        return createStringError(errc::illegal_byte_sequence,
-                           "unexpected line op length at offset 0x%8.8" PRIx64
-                           " expected 0x%2.2" PRIx64 " found 0x%2.2" PRIx64,
-                           ExtOffset, Len, *OffsetPtr - ExtOffset);
+      // If they aren't, continue from the claimed length.
+      uint64_t End = ExtOffset + Len;
----------------
Might be worth finding some clearer words here - this mentions 3 lengths ("stated", "parsed", and "claimed") - I see "claimed" is the same as "stated", so at least collapsing those two to use the same word would help clarify this. But in theory all the lengths are parsed, so "parsed" isn't completely unambiguous. Not sure what the perfect words would be here by any means.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72155