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

[James Henderson via Phabricator via llvm-commits]

jhenderson marked an inline comment as done.
jhenderson added inline comments.


================
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;
----------------
dblaikie wrote:
> jhenderson wrote:
> > dblaikie wrote:
> > > 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.
> > I'll change this to something better. How about "Make sure the length as recorded in the table and the standard length for the opcode match. If they don't, continue from the end as claimed by the table"?
> Abstract concern: If the table length is too short for the standard length, that would be a problem - the thing would get truncated. The other way around seems relatively benign (there's some excess padding, perhaps the producer had some reason to structure it that way). If we were trying to make this thing maximally general, I'd imagine "too short" would be a warning/error, but "too long" would at best be a linter/verifier issue, let alone the same error that'd be relatively indistinguishable (at an API level, if you were trying to decide what things to show to the user, etc).
> 
> But I doubt this'll come up in practice enough to matter/need that sort of nuance anyway. *shrug*
> If the table length is too short for the standard length, that would be a problem - the thing would get truncated.

Actually, this change means the parser reads the data the standard expects, then goes back and continues from the length as recorded in the table. This means that we don't truncate the opcode (how would we interpret it otherwise?) whilst still (loosely) respecting the length. Of course, chances are either the operand is bogus, or the following opcodes will be incorrect, but in all likelihood, one at least will be right.

> But I doubt this'll come up in practice enough to matter/need that sort of nuance anyway. *shrug*

That's probably true!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72155