[DebugInfo] Check for errors when reading data for extended opcode

[jhenderson]

Summary

Previously, if an extended opcode was truncated, it would manifest as an "unexpected line op length error" which wasn't quite accurate. This change checks for errors any time data is read whilst parsing an extended opcode, and reports any errors detected.

Depends on D80796.

[aprantl]

llvm/include/llvm/DebugInfo/DWARF/DWARFDataExtractor.h

74 ↗

(On Diff #267221)

I assume this never turns an error into a success?

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

878

This seems oddly stricter than before. Is the !Err condition necessary?

[MaskRay]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

857

Does it help clarity using DataExtractor::Cursor and StringRef getCStrRef(Cursor &C) const?

[labath]

llvm/include/llvm/DebugInfo/DWARF/DWARFDataExtractor.h

74 ↗

(On Diff #267221)

Nope. The idea is that if the error is already set, then the function will be a no-op, and the error will remain in the erroneous (and unchecked) state.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

878

I'm guessing this is because in the erroneous state the SubOpcode and Len won't contain real data, but only zeroes, and so a better course of action is to print the error about reaching the end-of-file (contribution) message.

Though if that's the case, then it does bring up the question of why is this the only statement guarded by !Err -- this argument should apply to other print statements too...

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

857

Maybe. I'm not particularly familiar with the Cursor usage yet, so I'll spend some time experimenting with that to see if it makes sense here.

878

@labath is right in that if there's an error, the "extended opcode" won't be a known value (it'll be another zero), so printing its value doesn't make much sense. Although I guess printing the length might not be a terrible idea. I'll give that some thought.

Re. consistency: I ran out of time to finish uploading a series of patches. One of my later patches does the check for the othe opcodes to stop them printing their arguments. I'll get back to that in the next day or two, and it should make everything consistent. Although now you point that out, maybe I should move this bit into that patch. I'll take a look to see if it makes sense.

[jhenderson]

Switch to using Cursor instead of an explicit offset and Error when parsing extended opcodes. Also removed suppression of output in event of error. A later patch will address this.

[jhenderson]

The unit tests need fixing up. Not sure why there weren't failing for me locally before.

[jhenderson]

Fixed the unit tests by adding an additional check which I'd previously dodged. Also added additional testing as unit testing, to cover all the cases which use the cursor.

[jhenderson]

Ping?

[labath]

Looks good to me.

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

1330–1331 ↗

(On Diff #268115)

Maybe OutputRef.split(LinePrefix).second

[jhenderson]

Rebased tests for recent changes to error messages. Also addressed @labath's review comments.

If there are no objections overnight, I'll put this in tomorrow UK time.

[jhenderson]

For reference, the pre merge bot tests are failing due to a change in the error message which I haven't rebased on yet. I'll fix that prior to committing.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

while (*OffsetPtr < EndOffset) {
     uint8_t Opcode = TableData.getU8(OffsetPtr);
 
     if (OS)
       *OS << format("%02.02" PRIx8 " ", Opcode);
 
     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(OffsetPtr);
-      uint64_t ExtOffset = *OffsetPtr;
+      DataExtractor::Cursor Cursor(*OffsetPtr);
+      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 (OS)
           *OS << "Badly formed extended line op (length 0)\n";
+        *OffsetPtr = Cursor.tell();
         continue;
       }
 
-      uint8_t SubOpcode = TableData.getU8(OffsetPtr);
+      uint8_t SubOpcode = TableData.getU8(Cursor);
       if (OS)
         *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
if (Opcode == 0) {
           if (OpcodeAddressSize != 1 && OpcodeAddressSize != 2 &&
               OpcodeAddressSize != 4 && OpcodeAddressSize != 8) {
             RecoverableErrorHandler(createStringError(
                 errc::invalid_argument,
                 "address size 0x%2.2" PRIx64
                 " of DW_LNE_set_address opcode at offset 0x%8.8" PRIx64
                 " is unsupported",
                 OpcodeAddressSize, ExtOffset));
-            *OffsetPtr += OpcodeAddressSize;
+            TableData.skip(Cursor, OpcodeAddressSize);
           } else {
             TableData.setAddressSize(OpcodeAddressSize);
             State.Row.Address.Address = TableData.getRelocatedAddress(
-                OffsetPtr, &State.Row.Address.SectionIndex);
+                Cursor, &State.Row.Address.SectionIndex);
 
             // Restore the address size if the extractor already had it.
             if (ExtractorAddressSize != 0)
               TableData.setAddressSize(ExtractorAddressSize);
           }
 
           if (OS)
             *OS << format(" (0x%16.16" PRIx64 ")", State.Row.Address.Address);
if (Opcode == 0) {
         // ignored for file names that represent full path names.
         //
         // The files are numbered, starting at 1, in the order in which they
         // appear; the names in the prologue come before names defined by
         // the DW_LNE_define_file instruction. These numbers are used in the
         // the file register of the state machine.
         {
           FileNameEntry FileEntry;
-          const char *Name = TableData.getCStr(OffsetPtr);
+          const char *Name = TableData.getCStr(Cursor);
           FileEntry.Name =
               DWARFFormValue::createFromPValue(dwarf::DW_FORM_string, Name);
-          FileEntry.DirIdx = TableData.getULEB128(OffsetPtr);
-          FileEntry.ModTime = TableData.getULEB128(OffsetPtr);
-          FileEntry.Length = TableData.getULEB128(OffsetPtr);
+          FileEntry.DirIdx = TableData.getULEB128(Cursor);
+          FileEntry.ModTime = TableData.getULEB128(Cursor);
+          FileEntry.Length = TableData.getULEB128(Cursor);
           Prologue.FileNames.push_back(FileEntry);
           if (OS)
             *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(OffsetPtr);
+        State.Row.Discriminator = TableData.getULEB128(Cursor);
         if (OS)
           *OS << " (" << State.Row.Discriminator << ")";
         break;
 
       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;
+        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.
+      // by the table. Similarly, continue from the claimed end in the event of
+      // a parsing error.
       uint64_t End = ExtOffset + Len;
-      if (*OffsetPtr != End) {
+      if (!Cursor)
+        RecoverableErrorHandler(Cursor.takeError());
+      else if (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, *OffsetPtr - ExtOffset));
-        *OffsetPtr = End;
-      }
+            ExtOffset, Len, Cursor.tell() - ExtOffset));
     } 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/debug_line_invalid.test

 # ALL-NEXT: warning: failed to parse file entry because the MD5 hash is invalid
 # ALL-NEXT: warning: parsing line table prologue at 0x000002ec found an invalid directory or file table description at 0x00000315
 # ALL-NEXT: warning: failed to parse directory entry because skipping the form value failed.
 # ALL-NEXT: warning: parsing line table prologue at offset 0x00000332 found opcode base of 0. Assuming no standard opcodes
 # ALL-NEXT: warning: parsing line table prologue at 0x00000361 found an invalid directory or file table description at 0x00000383
 # ALL-NEXT: warning: include directories table was not null terminated before the end of the prologue
 # ALL-NEXT: warning: parsing line table prologue at 0x00000390 found an invalid directory or file table description at 0x000003bf
 # ALL-NEXT: warning: file names table was not null terminated before the end of the prologue
-# OTHER-NEXT: warning: unexpected line op length at offset 0x00000411 expected 0x09 found 0x01
+# OTHER-NEXT: warning: unexpected end of data at offset 0x412
 # OTHER-NEXT: warning: last sequence in debug line table at offset 0x000003c9 is not terminated
 # ALL-NOT:  warning: