[DebugInfo] Don't print extended opcode operands if invalid

[jhenderson]

Summary

Previously, if there was an error whilst parsing the operands of an extended opcode, the operands would be treated as zero and printed. This could potentially be slightly confusing. This patch changes the behaviour to print the raw bytes for the opcode instead.

Depends on D81562.

[ikudrin]

Would not it be more useful if we dump a sequence of raw bytes, as we do for DWARF expressions? See DWARFExpression::print() and test llvm/test/tools/llvm-dwarfdump/X86/verify_broken_exprloc.s as an example.

[jhenderson]

In D81570#2095372, @ikudrin wrote:

Would not it be more useful if we dump a sequence of raw bytes, as we do for DWARF expressions? See DWARFExpression::print() and test llvm/test/tools/llvm-dwarfdump/X86/verify_broken_exprloc.s as an example.

Yes, it's probably possible somehow. I'll take a look. It would lead to slightly inconsistent behaviour with standard opcodes, where we don't have a known length for the operands, of course.

[jhenderson]

Print raw bytes of extended opcode on failure instead of nothing.

[ikudrin]

You may also want to add a couple of tests with an unrecognized opcode and premature termination, like these:

std::make_tuple(
    3, 9, 99,
    ValueAndLengths{{0x12345678, LineTable::Quad}}, ...
std::make_tuple(
    4, 9, 99,
    ValueAndLengths{{0x12345678, LineTable::Quad}}, ...

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

808

It seems it is not possible to get here with an invalidated Cursor, no?

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

1477

Maybe it is better to change DW_LNE_set_address to something different to emphasize that this test case goes to the default branch of switch (SubOpcode)?

1478

Should we add this if there are no raw bytes to report, especially when even opcode is missing? That space at the beginning looks quite untidy.

[jhenderson]

Address review comments, rewrite the raw byte printing, and added a few additional comments.

[ikudrin]

Overall looks great.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

808

Maybe it's worth to drop here a note about that Cursor is always valid here? So that a programmer coming across these lines would not be surprised why Cursor is checked in other branches, but not here?

931–940

I suppose that this might be simplified a bit using a do-while loop. Something like this:

uint8_t Byte = TableData.getU8(ByteCursor);
if (ByteCursor) {
  *OS << " (<parsing error>";
  do {
    *OS << format(" %2.2" PRIx8, Byte);
    Byte = TableData.getU8(ByteCursor);
  } while (ByteCursor);
  *OS << ")";
}

WDYT?

[ikudrin]

Oh, and there are still no tests with unknown opcodes and truncated operands.

[jhenderson]

In D81570#2106512, @ikudrin wrote:

Oh, and there are still no tests with unknown opcodes and truncated operands.

Oops, missed that bit of your earlier comment. Thanks.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

931–940

I think that makes sense. I previously didn't do that, since it didn't seem to interact well with the ByteCursor.tell() < End, but looking at it again, I realise that bit is irrelevant, since we can just keep parsing until the end, and then ignore the error like we're already doing.

[jhenderson]

Add comment, use do/while, and add missing test case.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

while (*OffsetPtr < EndOffset) {
     if (Opcode == 0) {
       // Extended Opcodes always start with a zero opcode followed by
       // a uleb128 length so you can skip ones you don't know about
       uint64_t Len = TableData.getULEB128(Cursor);
       uint64_t ExtOffset = Cursor.tell();
 
       // Tolerate zero-length; assume length is correct and soldier on.
       if (Len == 0) {
-        if (Verbose)
+        if (Cursor && Verbose)
           *OS << "Badly formed extended line op (length 0)\n";
-        if (!Cursor)
+        if (!Cursor) {
+          if (Verbose)
+            *OS << "\n";
           RecoverableErrorHandler(Cursor.takeError());
+        }
         *OffsetPtr = Cursor.tell();
         continue;
       }
 
       uint8_t SubOpcode = TableData.getU8(Cursor);
       if (Verbose)
         *OS << LNExtendedString(SubOpcode);
       switch (SubOpcode) {
       case DW_LNE_end_sequence:
         // Set the end_sequence register of the state machine to true and
         // append a row to the matrix using the current values of the
         // state-machine registers. Then reset the registers to the initial
         // values specified above. Every statement program sequence must end
         // with a DW_LNE_end_sequence instruction which creates a row whose
         // address is that of the byte after the last target machine instruction
         // of the sequence.
         State.Row.EndSequence = true;
-        if (Verbose) {
+        if (Cursor && Verbose) {
           *OS << "\n";
           OS->indent(12);
         }
-        if (OS)
+        if (Cursor && OS)
           State.Row.dump(*OS);
         State.appendRowToMatrix();
         State.resetRowAndSequence();
         break;
 
       case DW_LNE_set_address:
         // Takes a single relocatable address as an operand. The size of the
         // operand is the size appropriate to hold an address on the target
if (Opcode == 0) {
             State.Row.Address.Address = TableData.getRelocatedAddress(
                 Cursor, &State.Row.Address.SectionIndex);
 
             // Restore the address size if the extractor already had it.
             if (ExtractorAddressSize != 0)
               TableData.setAddressSize(ExtractorAddressSize);
           }
 
-          if (Verbose)
+          if (Cursor && Verbose)
             *OS << format(" (0x%16.16" PRIx64 ")", State.Row.Address.Address);
         }
         break;
 
       case DW_LNE_define_file:
         // Takes 4 arguments. The first is a null terminated string containing
         // a source file name. The second is an unsigned LEB128 number
         // representing the directory index of the directory in which the file
if (Opcode == 0) {
           FileNameEntry FileEntry;
           const char *Name = TableData.getCStr(Cursor);
           FileEntry.Name =
               DWARFFormValue::createFromPValue(dwarf::DW_FORM_string, Name);
           FileEntry.DirIdx = TableData.getULEB128(Cursor);
           FileEntry.ModTime = TableData.getULEB128(Cursor);
           FileEntry.Length = TableData.getULEB128(Cursor);
           Prologue.FileNames.push_back(FileEntry);
-          if (Verbose)
+          if (Cursor && Verbose)
             *OS << " (" << Name << ", dir=" << FileEntry.DirIdx << ", mod_time="
                 << format("(0x%16.16" PRIx64 ")", FileEntry.ModTime)
                 << ", length=" << FileEntry.Length << ")";
         }
         break;
 
       case DW_LNE_set_discriminator:
         State.Row.Discriminator = TableData.getULEB128(Cursor);
-        if (Verbose)
+        if (Cursor && Verbose)
           *OS << " (" << State.Row.Discriminator << ")";
         break;
 
       default:
-        if (Verbose)
+        if (Cursor && Verbose)
           *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.
         TableData.skip(Cursor, Len - 1);
         break;
       }
       // 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. Similarly, continue from the claimed end in the event of
       // a parsing error.
       uint64_t End = ExtOffset + Len;
       if (Cursor && Cursor.tell() != End)
         RecoverableErrorHandler(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, Cursor.tell() - ExtOffset));
       *OffsetPtr = End;
     } else if (Opcode < Prologue.OpcodeBase) {
       if (Verbose)
         *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.
         if (Verbose) {
           *OS << "\n";
           OS->indent(12);
         }
         if (OS)
           State.Row.dump(*OS);
         State.appendRowToMatrix();
         break;
 
       case DW_LNS_advance_pc:
         // Takes a single unsigned LEB128 operand, multiplies it by the

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

 ## Table with extended opcode that overruns table end.
 # NONFATAL:      debug_line[0x000003c9]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      Address
 # NONFATAL-NEXT: ------------------
 # VERBOSE-NEXT:  DW_LNE_set_address (0x00000000feedfeed)
 # VERBOSE-NEXT:  DW_LNS_copy
 # NONFATAL-NEXT: 0x00000000feedfeed
-# VERBOSE-NEXT:  DW_LNE_set_address (0x0000000000000000)
+# VERBOSE-NEXT:  DW_LNE_set_address{{$}}
 # MORE-ERR-NEXT: warning: unexpected end of data at offset 0x419 while reading [0x412, 0x41a)
 # MORE-ERR-NEXT: warning: last sequence in debug line table at offset 0x000003c9 is not terminated
 
 # LAST:          debug_line[0x00000419]
 # VERBOSE:       DW_LNE_set_address (0x00000000cafebabe)
 # VERBOSE-NEXT:  DW_LNE_end_sequence

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

TEST_P(TruncatedExtendedOpcodeFixture, ErrorForTruncatedExtendedOpcode) {
   EXPECT_THAT_ERROR(std::move(Recoverable),
                     FailedWithMessage(ExpectedErr.str()));
 }
 
 INSTANTIATE_TEST_CASE_P(
     TruncatedExtendedOpcodeParams, TruncatedExtendedOpcodeFixture,
     Values(
         std::make_tuple(1, 1, DW_LNE_end_sequence, ValueAndLengths(),
-                        "Badly formed extended line op (length 0)",
+                        "",
                         "unable to decode LEB128 at offset 0x00000030: "
                         "malformed uleb128, extends past end"),
         std::make_tuple(
             2, 9, DW_LNE_set_address,
-            ValueAndLengths{{0x12345678, LineTable::Quad}},
-            "Unrecognized extended op 0x00 length 9",
+            ValueAndLengths{{0x12345678, LineTable::Quad}}, "",
             "unexpected end of data at offset 0x31 while reading [0x31, 0x32)"),
         std::make_tuple(
             3, 9, DW_LNE_set_address,
             ValueAndLengths{{0x12345678, LineTable::Quad}},
-            "DW_LNE_set_address (0x0000000000000000)",
+            "DW_LNE_set_address",
             "unexpected end of data at offset 0x32 while reading [0x32, 0x3a)"),
         std::make_tuple(3, 5, DW_LNE_define_file,
                         ValueAndLengths{{'a', LineTable::Byte},
                                         {'\0', LineTable::Byte},
                                         {1, LineTable::ULEB},
                                         {1, LineTable::ULEB},
                                         {1, LineTable::ULEB}},
-                        "DW_LNE_define_file (, dir=0, "
-                        "mod_time=(0x0000000000000000), length=0)",
+                        "DW_LNE_define_file",
                         "no null terminated string at offset 0x32"),
         std::make_tuple(5, 5, DW_LNE_define_file,
                         ValueAndLengths{{'a', LineTable::Byte},
                                         {'\0', LineTable::Byte},
                                         {1, LineTable::ULEB},
                                         {1, LineTable::ULEB},
                                         {1, LineTable::ULEB}},
-                        "DW_LNE_define_file (a, dir=0, "
-                        "mod_time=(0x0000000000000000), length=0)",
+                        "DW_LNE_define_file",
                         "unable to decode LEB128 at offset 0x00000034: "
                         "malformed uleb128, extends past end"),
         std::make_tuple(6, 5, DW_LNE_define_file,
                         ValueAndLengths{{'a', LineTable::Byte},
                                         {'\0', LineTable::Byte},
                                         {1, LineTable::ULEB},
                                         {1, LineTable::ULEB},
                                         {1, LineTable::ULEB}},
-                        "DW_LNE_define_file (a, dir=1, "
-                        "mod_time=(0x0000000000000000), length=0)",
+                        "DW_LNE_define_file",
                         "unable to decode LEB128 at offset 0x00000035: "
                         "malformed uleb128, extends past end"),
         std::make_tuple(7, 5, DW_LNE_define_file,
                         ValueAndLengths{{'a', LineTable::Byte},
                                         {'\0', LineTable::Byte},
                                         {1, LineTable::ULEB},
                                         {1, LineTable::ULEB},
                                         {1, LineTable::ULEB}},
-                        "DW_LNE_define_file (a, dir=1, "
-                        "mod_time=(0x0000000000000001), length=0)",
+                        "DW_LNE_define_file",
                         "unable to decode LEB128 at offset 0x00000036: "
                         "malformed uleb128, extends past end"),
         std::make_tuple(3, 2, DW_LNE_set_discriminator,
                         ValueAndLengths{{1, LineTable::ULEB}},
-                        "DW_LNE_set_discriminator (0)",
+                        "DW_LNE_set_discriminator",
                         "unable to decode LEB128 at offset 0x00000032: "
                         "malformed uleb128, extends past end")), );
 
 TEST_P(TruncatedStandardOpcodeFixture, ErrorForTruncatedStandardOpcode) {
   if (!setupGenerator())
     return;
   LineTable &LT = setupTable();
   LT.addStandardOpcode(Opcode, Operands);