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

[jhenderson]

Summary

It is possible to try to keep parsing a debug line program even when the length of an extended opcode does not match what is expected for that opcode. This patch changes what was previously a fatal error to be non-fatal. The parser now continues by assuming the claimed length is correct and adjusting the offset if required.

[jhenderson]

Early ping - I'd like to get this and the other related reviews in before the release branch is created if possible.

[dblaikie]

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)

The parser now continues by assuming the larger of the claimed length and parsed length is correct.

This seems like a bit of a stretch to guess at which length is the one that's correct. I don't think there's a solid basis to choose either, and if the wrong one is chosen you could start reading from the middle of other opcodes & things, I would imagine, resulting in many follow-on error messages due to apparently further malformed input?

[jhenderson]

In 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.

The parser now continues by assuming the larger of the claimed length and parsed length is correct.

This seems like a bit of a stretch to guess at which length is the one that's correct. I don't think there's a solid basis to choose either, and if the wrong one is chosen you could start reading from the middle of other opcodes & things, I would imagine, resulting in many follow-on error messages due to apparently further malformed input?

I was unsure what to do here, if I'm honest. The counter-argument to your point is that you might not start reading from the middle of things, and that it was just the one field that was bad for whatever reason. Essentially, I think you have fpur options in this sort of situation: a) treat the error as unrecoverable - this makes it impossible to parse later tables, regardless of the state of things, but would prevent other, potentially spurious, errors from coming out; b) use a "stated length wins" approach; c) use a "parsed length wins" approach; d) use a "largest length wins" approach. The right decision is going to differ in any given situation, but without attempting to look ahead and see what makes the most sense if parsing were to continue (which seems like it would be unnecessarily complicated), we have to just pick one. Previously, when I implemented these errors, I took approach a) on the basis of "you can't know what to do, so better not allow continuation, since it could lead to spurious information", but it was pointed out more recently to me that if we do this, it becomes impossible for people to see any later information at all.

I don't really have a strong opinion on this or the similar "bigger wins" patches I've been making. I'm happy to go with whatever the consensus is.

[jhenderson]

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.

I misremembered the rest of the code. An unrecoverable error doesn't prevent the SectionParser from continuing to the next line table. It just means that tools like llvm-dwarfdump will treat them as an error. The only difference between this and the recoverable error is that parsing of the CURRENT table will stop.

I took a step back and re-discussed the situation with a colleague offline. The errors are passed to callbacks so that clients can decide what to do when they see an error. For example, a consumer who needs to rely on the output being definitely correct can choose the callback to say "ignore this line table, and also ignore any further errors in that table". The only real difference between the unrecoverable and recoverable callbacks are that we stop parsing at the point the former is hit because we have no way of continuing, meaning our information is incomplete, whereas the latter will try to continue and give some information, but the information might be inaccurate. Given this, I don't think causing producing spurious information is wrong, as long as we have reported at least one error.

That being said, I do think "largest one wins" is the wrong approach, having considered some of the other cases. In particular, the SectionParser treats the unit length field as sacrosanct, and will always follow it (assuming it can possibly make sense - see totalLengthIsValid) when moving to the next table, evn if it means that the offset moves backwards from the point it got to after reading the table. I therefore think for consistency, we should treat all length fields as sacrosanct, i.e. jump to the position the offset should be according to the length (whether it is bigger or not) and continue from there. Yes, this might cause the parsing to produce bogus information, but that's okay (see my above point).

I'll update this and the other similar diffs accordingly.

[jhenderson]

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

[jhenderson]

Ping!

[dblaikie]

In D72155#1813778, @jhenderson wrote:

In 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, @jhenderson wrote:

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

Looks good - thanks!

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

687–699

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.

[probinson]

In D72155#1827856, @dblaikie wrote:

In D72155#1813778, @jhenderson wrote:

In 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"?)

"We" is Sony, here. James is part of our binutils/linker team. We have local linker patches to edit the line table in response to GC'ing functions. Also doing other fixups to alert the debugger to GC'd functions, but the part where we actually parse something is for the line table.

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?

Mostly we care about dealing with a possibly malformed/corrupt line-number program for one CU, and soldiering on to the next one. The middle of a user's edit/build/debug cycle is not really the optimal time to abort and whine about a malformed lump of debug info.
I don't know if the linker team has contemplated fuzzing in this area.

[jhenderson]

Sorry for the delayed response. Last week was pretty crazy, so I didn't get a chance to continue with this.

In D72155#1833571, @probinson wrote:

In D72155#1827856, @dblaikie wrote:(out of curiosity, though not necessary, who is "we"?)

"We" is Sony, here. James is part of our binutils/linker team. We have local linker patches to edit the line table in response to GC'ing functions. Also doing other fixups to alert the debugger to GC'd functions, but the part where we actually parse something is for the line table.

Thanks for clarifying @probinson!

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?

Mostly we care about dealing with a possibly malformed/corrupt line-number program for one CU, and soldiering on to the next one. The middle of a user's edit/build/debug cycle is not really the optimal time to abort and whine about a malformed lump of debug info.
I don't know if the linker team has contemplated fuzzing in this area.

Like @probinson said, much of our work has been focused on the line table, because we mess about with it in relation to GC-sections work in the linker, so hence why I've been focusing on that area. We need to detect dodgy output to avoid us doing bad stuff when doing this manipulation of the line tables, and soldiering on if we do find bad things is better than failing the link completely. For other DWARF sections, we currently have other strategies which don't require parsing the sections, so we're not so interested in the other areas, at least within our team, at this point (although making llvm-dwarfdump and llvm-symbolizer more robust/useful on bad inputs etc is always beneficial, just not such high priorities). Fuzzing might eventually be useful, but we're mostly interested in the low-hanging fruit as shown by some older testing we had, so haven't looked at it at this point.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

687–699

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"?

[jhenderson]

Make comment clearer

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

687–699

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*

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

687–699

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!

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

687–699

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?)

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

if (Opcode == 0) {
       default:
         if (OS)
           *OS << format("Unrecognized extended op 0x%02.02" PRIx8, SubOpcode)
               << format(" length %" PRIx64, Len);
         // Len doesn't include the zero opcode byte or the length itself, but
         // it does include the sub_opcode, so we have to adjust for that.
         (*OffsetPtr) += Len - 1;
         break;
       }
-      // 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);
+      // 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.
+      uint64_t End = ExtOffset + Len;
+      if (*OffsetPtr != End) {
+        RecoverableErrorCallback(createStringError(
+            errc::illegal_byte_sequence,
+            ExtOffset, Len, *OffsetPtr - ExtOffset));
+        *OffsetPtr = End;
+      }
     } else if (Opcode < Prologue.OpcodeBase) {
       if (OS)
         *OS << LNStandardString(Opcode);
       switch (Opcode) {
       // Standard Opcodes
       case DW_LNS_copy:
         // Takes no arguments. Append a row to the matrix using the
         // current values of the state-machine registers.

llvm/test/tools/llvm-dwarfdump/X86/Inputs/debug_line_malformed.s

 .byte   1, 2, 3
 .byte   0
 .Lprologue_long_prologue_end:
 .byte   0, 9, 2        # DW_LNE_set_address
 .quad   0x1111222233334444
 .byte   0, 1, 1        # DW_LNE_end_sequence
 .Lunit_long_prologue_end:
 
-# Over-long extended opcode.
+# Incorrect length extended opcodes.
 .long   .Lunit_long_opcode_end - .Lunit_long_opcode_start # unit length
 .Lunit_long_opcode_start:
 .short  4               # version
 .long   .Lprologue_long_opcode_end-.Lprologue_long_opcode_start # Length of Prologue
 .Lprologue_long_opcode_start:
 .byte   1               # Minimum Instruction Length
 .byte   1               # Maximum Operations per Instruction
 .byte   1               # Default is_stmt
 .byte   -5              # Line Base
 .byte   14              # Line Range
 .byte   13              # Opcode Base
 .byte   0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1 # Standard Opcode Lengths
 .asciz "dir1"           # Include table
 .asciz "dir2"
 .byte   0
 .asciz "file1"          # File table
 .byte   0, 0, 0
 .asciz "file2"
 .byte   1, 0, 0
 .byte   0
 .Lprologue_long_opcode_end:
 .byte   0, 9, 2         # DW_LNE_set_address
 .quad   0xabbadaba
 .byte   0, 2, 1         # DW_LNE_end_sequence (too long)
+.byte   6               # DW_LNS_negate_stmt (but will be consumed with the end sequence above).
+.byte   0, 1, 4         # DW_LNE_set_discriminator (too short)
+.byte   0xa             # Parsed as argument for set_discriminator and also DW_LNS_set_prologue_end.
 .byte   0, 9, 2         # DW_LNE_set_address
 .quad   0xbabb1e45
 .byte   0, 1, 1         # DW_LNE_end_sequence
 .Lunit_long_opcode_end:
 
 # No end of sequence.
 .long   .Lunit_no_eos_end - .Lunit_no_eos_start # unit length
 .Lunit_no_eos_start:

llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test

 # RUN: FileCheck %s --input-file=%t-malformed-verbose.err --check-prefixes=ALL,OTHER
 
 ## We should still produce warnings for malformed tables after the specified unit.
 # RUN: llvm-dwarfdump -debug-line=0 %t-malformed.o 2> %t-malformed-off-first.err \
 # RUN:   | FileCheck %s --check-prefixes=FIRST,NOLATER
 # RUN: FileCheck %s --input-file=%t-malformed-off-first.err --check-prefix=ALL
 
 ## Don't stop looking for the later unit if non-fatal issues are found.
-# RUN: llvm-dwarfdump -debug-line=0x2af %t-malformed.o 2> %t-malformed-off-last.err \
+# RUN: llvm-dwarfdump -debug-line=0x2b4 %t-malformed.o 2> %t-malformed-off-last.err \
 # RUN:   | FileCheck %s --check-prefix=LAST --implicit-check-not='debug_line[{{.*}}]'
 # RUN: FileCheck %s --input-file=%t-malformed-off-last.err --check-prefix=ALL
 
 # FIRST:       debug_line[0x00000000]
 # FIRST:       0x000000000badbeef {{.*}} end_sequence
 # NOFIRST-NOT: debug_line[0x00000000]
 # NOFIRST-NOT: 0x000000000badbeef {{.*}} end_sequence
 # NOLATER-NOT: debug_line[{{.*}}]

 # NONFATAL-NEXT:       dir_index: 1
 # NONFATAL-NEXT:        mod_time: 0x00000002
 # NONFATAL-NEXT:          length: 0x00000003
 # NONFATAL-NOT:  file_names
 # NONFATAL-NOT:  Address
 
 ## Case 6: Extended opcode with incorrect length versus expected.
 # NONFATAL:      debug_line[0x00000111]
-## Dumping prints the line table prologue and any valid operations up to the
-## point causing the problem.
 # NONFATAL-NEXT: Line table prologue
-# NONFATAL:      0x00000000abbadaba {{.*}} end_sequence
-# NONFATAL-NOT:  is_stmt
+# NONFATAL: 0x00000000abbadaba {{.*}} end_sequence
+# NONFATAL: 0x00000000babb1e45 {{.*}} 10 is_stmt prologue_end end_sequence{{$}}
 
-## For minor issues, we can dump the whole table.
 ## Case 7: No end of sequence.
-# NONFATAL:      debug_line[0x00000167]
+# NONFATAL:      debug_line[0x0000016c]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      0x00000000deadfade {{.*}} is_stmt
 # NONFATAL-NOT:  end_sequence
 
 ## Case 8: Very short prologue length for V5 (ends during parameters).
-# NONFATAL:      debug_line[0x000001ad]
+# NONFATAL:      debug_line[0x000001b2]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      standard_opcode_lengths[DW_LNS_set_isa] = 1
 # NONFATAL-NEXT: include_directories[  0] = "/tmp"
 # NONFATAL-NEXT: file_names[  0]:
 # NONFATAL-NEXT:            name: "a.c"
 # NONFATAL-NEXT:       dir_index: 1
 # NONFATAL-NOT:  Address
 
 ## Case 9: V5 prologue ends during file table.
-# NONFATAL:      debug_line[0x000001ed]
+# NONFATAL:      debug_line[0x000001f2]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      include_directories[  0] = "/tmp"
 # NONFATAL-NEXT: file_names[  0]:
 # NONFATAL-NEXT:            name: "a.c"
 # NONFATAL-NEXT:       dir_index: 1
 # NONFATAL-NOT:  Address
 
 ## Case 10: V5 prologue ends during directory table.
-# NONFATAL:      debug_line[0x0000022d]
+# NONFATAL:      debug_line[0x00000232]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      include_directories[  0] = "/tmp"
 # NONFATAL-NEXT: file_names[  0]:
 # NONFATAL-NEXT:            name: "a.c"
 # NONFATAL-NEXT:       dir_index: 1
 # NONFATAL-NOT:  Address
 
 ## Case 11: V5 invalid MD5 hash form.
-# NONFATAL:      debug_line[0x0000026d]
+# NONFATAL:      debug_line[0x00000272]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      include_directories[  0] = "/tmp"
 # NONFATAL-NOT:  file_names
 # NONFATAL-NOT:  Address
 
-# LAST:          debug_line[0x000002af]
+# LAST:          debug_line[0x000002b4]
 # LAST:          0x00000000cafebabe {{.*}} end_sequence
 
 # RESERVED: warning: parsing line table prologue at offset 0x00000048 unsupported reserved unit length found of value 0xfffffffe
 
 # ALL-NOT:  warning:
 # ALL:      warning: parsing line table prologue at offset 0x00000048 found unsupported version 0x00
 # ALL-NEXT: warning: parsing line table prologue at offset 0x0000004e found unsupported version 0x01
 # ALL-NEXT: warning: parsing line table prologue at 0x00000054 found an invalid directory or file table description at 0x00000073
 # ALL-NEXT: warning: failed to parse entry content descriptions because no path was found
 # FIXME - The latter offset in the next line should be 0xad. The filename parsing code does not notice a missing terminating byte.
 # ALL-NEXT: warning: parsing line table prologue at 0x00000081 should have ended at 0x000000b9 but it ended at 0x000000ba
 # ALL-NEXT: warning: parsing line table prologue at 0x000000c9 should have ended at 0x00000104 but it ended at 0x00000103
 # OTHER-NEXT: warning: unexpected line op length at offset 0x00000158 expected 0x02 found 0x01
-# OTHER-NEXT: warning: last sequence in debug line table at offset 0x00000167 is not terminated
-# ALL-NEXT: warning: parsing line table prologue at 0x000001ad should have ended at 0x000001c8 but it ended at 0x000001df
-# ALL-NEXT: warning: parsing line table prologue at 0x000001ed should have ended at 0x00000218 but it ended at 0x0000021f
-# ALL-NEXT: warning: parsing line table prologue at 0x0000022d should have ended at 0x0000024f but it ended at 0x0000025f
-# ALL-NEXT: warning: parsing line table prologue at 0x0000026d found an invalid directory or file table description at 0x000002a2
+# OTHER-NEXT: warning: unexpected line op length at offset 0x0000015c expected 0x01 found 0x02
+# OTHER-NEXT: warning: last sequence in debug line table at offset 0x0000016c is not terminated
+# ALL-NEXT: warning: parsing line table prologue at 0x000001b2 should have ended at 0x000001cd but it ended at 0x000001e4
+# ALL-NEXT: warning: parsing line table prologue at 0x000001f2 should have ended at 0x0000021d but it ended at 0x00000224
+# ALL-NEXT: warning: parsing line table prologue at 0x00000232 should have ended at 0x00000254 but it ended at 0x00000264
+# ALL-NEXT: warning: parsing line table prologue at 0x00000272 found an invalid directory or file table description at 0x000002a7
 # ALL-NEXT: warning: failed to parse file entry because the MD5 hash is invalid
 # ALL-NOT:  warning:

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

 INSTANTIATE_TEST_CASE_P(
     LineTableTestParams, DebugLineParameterisedFixture,
     Values(std::make_pair(
                2, DWARF32), // Test lower-bound of v2-3 fields and DWARF32.
            std::make_pair(3, DWARF32), // Test upper-bound of v2-3 fields.
            std::make_pair(4, DWARF64), // Test v4 fields and DWARF64.
            std::make_pair(5, DWARF32), std::make_pair(5, DWARF64)), );
 
-TEST_F(DebugLineBasicFixture, ErrorForInvalidExtendedOpcodeLength) {
+TEST_F(DebugLineBasicFixture, ErrorForExtendedOpcodeLengthSmallerThanExpected) {
   if (!setupGenerator())
     return;
 
   LineTable &LT = Gen->addLineTable();
+  LT.addByte(0xaa);
+  // The Length should be 1 + sizeof(ULEB) for a set discriminator opcode.
+  // The operand will be read for both the discriminator opcode and then parsed
+  // again as DW_LNS_negate_stmt, to respect the claimed length.
+  LT.addExtendedOpcode(1, DW_LNE_set_discriminator,
+                       {{DW_LNS_negate_stmt, LineTable::ULEB}});
+  LT.addByte(0xbb);
+  LT.addStandardOpcode(DW_LNS_const_add_pc, {});
+  LT.addExtendedOpcode(1, DW_LNE_end_sequence, {});
+
+  generate();
+
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+                                                    nullptr, RecordRecoverable);
+  checkError(
+      "unexpected line op length at offset 0x00000031 expected 0x01 found 0x02",
+      std::move(Recoverable));
+  ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+  ASSERT_EQ((*ExpectedLineTable)->Rows.size(), 3u);
+  EXPECT_EQ((*ExpectedLineTable)->Sequences.size(), 1u);
+  EXPECT_EQ((*ExpectedLineTable)->Rows[1].IsStmt, 0u);
+  EXPECT_EQ((*ExpectedLineTable)->Rows[1].Discriminator, DW_LNS_negate_stmt);
+}
+
+TEST_F(DebugLineBasicFixture, ErrorForExtendedOpcodeLengthLargerThanExpected) {
+  if (!setupGenerator())
+    return;
+
+  LineTable &LT = Gen->addLineTable();
+  LT.addByte(0xaa);
+  LT.addStandardOpcode(DW_LNS_const_add_pc, {});
   // The Length should be 1 for an end sequence opcode.
   LT.addExtendedOpcode(2, DW_LNE_end_sequence, {});
+  // The negate statement opcode will be skipped.
+  LT.addStandardOpcode(DW_LNS_negate_stmt, {});
+  LT.addByte(0xbb);
+  LT.addStandardOpcode(DW_LNS_const_add_pc, {});
+  LT.addExtendedOpcode(1, DW_LNE_end_sequence, {});
 
   generate();
 
-  checkGetOrParseLineTableEmitsFatalError(
-      "unexpected line op length at offset "
-      "0x00000030 expected 0x02 found 0x01");
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+                                                    nullptr, RecordRecoverable);
+  checkError(
+      "unexpected line op length at offset 0x00000032 expected 0x02 found 0x01",
+      std::move(Recoverable));
+  ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+  ASSERT_EQ((*ExpectedLineTable)->Rows.size(), 4u);
+  EXPECT_EQ((*ExpectedLineTable)->Sequences.size(), 2u);
+  ASSERT_EQ((*ExpectedLineTable)->Sequences[1].FirstRowIndex, 2u);
+  EXPECT_EQ((*ExpectedLineTable)->Rows[2].IsStmt, 1u);
 }
 
 TEST_F(DebugLineBasicFixture, ErrorForUnitLengthTooLarge) {
   if (!setupGenerator())
     return;
 
   LineTable &Padding = Gen->addLineTable();
   // Add some padding to show that a non-zero offset is handled correctly.
TEST_F(DebugLineBasicFixture, ParserReportsNonPrologueProblemsWhenParsing) {
   LT2.addExtendedOpcode(9, DW_LNE_set_address,
                         {{0x1234567890abcdef, LineTable::Quad}});
   LT2.addStandardOpcode(DW_LNS_copy, {});
   LT2.addByte(0xbb);
   generate();
 
   DWARFDebugLine::SectionParser Parser(LineData, *Context, CUs, TUs);
   Parser.parseNext(RecordRecoverable, RecordUnrecoverable);
-  EXPECT_FALSE(Recoverable);
+  EXPECT_FALSE(Unrecoverable);
   ASSERT_FALSE(Parser.done());
   checkError(
       "unexpected line op length at offset 0x00000030 expected 0x42 found 0x01",
-      std::move(Unrecoverable));
+      std::move(Recoverable));
 
   // Reset the error state so that it does not confuse the next set of checks.
   Unrecoverable = Error::success();
   Parser.parseNext(RecordRecoverable, RecordUnrecoverable);
 
   EXPECT_TRUE(Parser.done());
   checkError("last sequence in debug line table at offset 0x00000031 is not "
              "terminated",