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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a subscriber: labath.
dblaikie 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;
----------------
jhenderson wrote:
> 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!
>> 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. 

I don't think that's good behavior - any length read should, in my opinion, restrict what's read after that to only that length.

> This means that we don't truncate the opcode (how would we interpret it otherwise?) 

The same as we would if the file ended early & there weren't enough bytes to read, I think.

As per the design discussion on llvm-dev (DWARF debug line error handling changes) - perhaps these sort of things could benefit from the ability to create a more constrained DWARFDataExtractor (with a shorter length - specified by some length prefix provided such as contribution lengths, etc).

(@labath - Pavel, for another use case for a DWARFDataExtractor with a constrained length - once that DWARFDataExtractor feature is in place could one of you (Pavel or James) revisit this patch & port it to such a tool?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72155