[DebugInfo] Unify Cursor usage for all debug line opcodes

[jhenderson]

Summary

This is a natural extension of the previous changes to use the Cursor class independently in the standard and extended opcode paths, and in turn allows delaying error handling until the entire line has been printed in verbose mode, removing interleaved output in some cases.

Depends on D81470.

[MaskRay]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

767

What about hoisting Cursor and refactoring more *OffsetPtr to use Cursor?

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

767

I considered that, but there are a few reasons not to:

1. We handle errors differently depending on context. For extended opcodes, we continue parsing, which means we want to reset the Cursor to a good state after handling them. Recreating it at the start of the loop is easier.
2. We'd have to do a no-op error check of the Cursor, to avoid an unchecked error situation in the event of an empty body.
3. The only parsing outside the loop is the Prologue parsing, which has its own Cursor and is self-contained (not all clients that parse the prologue parse the body), meaning it doesn't make much sense to pass in the Cursor from outside. There is therefore no need to share said Cursor with anything else. The only benefit in hoisting it is therefore to avoid needing to reinitialise the Cursor on each iteration (but see above why this isn't that helpful).

[jhenderson]

@MaskRay, are you happy for me to land this as-is?

[MaskRay]

Looks great!

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

Error DWARFDebugLine::LineTable::parse(
   ParsingState State(this, DebugLineOffset, RecoverableErrorHandler);
 
   *OffsetPtr = DebugLineOffset + Prologue.getLength();
   if (OS && *OffsetPtr < EndOffset) {
     *OS << '\n';
     Row::dumpTableHeader(*OS, /*Indent=*/Verbose ? 12 : 0);
   }
   while (*OffsetPtr < EndOffset) {
+    DataExtractor::Cursor Cursor(*OffsetPtr);
+
     if (Verbose)
       *OS << format("0x%08.08" PRIx64 ": ", *OffsetPtr);
 
     uint64_t OpcodeOffset = *OffsetPtr;
-    uint8_t Opcode = TableData.getU8(OffsetPtr);
+    uint8_t Opcode = TableData.getU8(Cursor);
     size_t RowCount = Rows.size();
 
-    if (Verbose)
+    if (Cursor && Verbose)
       *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
-      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 (Verbose)
           *OS << "Badly formed extended line op (length 0)\n";
         if (!Cursor)
if (Opcode == 0) {
         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)
-        RecoverableErrorHandler(Cursor.takeError());
-      else if (Cursor.tell() != End)
+      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) {
-      DataExtractor::Cursor Cursor(*OffsetPtr);
       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) {
if (Opcode == 0) {
             if (Verbose)
               *OS << ')';
           }
         }
         break;
       }
 
       *OffsetPtr = Cursor.tell();
-
-      // Most standard opcode failures are due to failures to read ULEBs. Bail
-      // out of parsing, since we don't know where to continue reading from as
-      // there is no stated length for such byte sequences.
-      if (!Cursor) {
-        if (Verbose)
-          *OS << "\n\n";
-        return Cursor.takeError();
-      }
     } else {
       // Special Opcodes.
       ParsingState::AddrAndLineDelta Delta =
           State.handleSpecialOpcode(Opcode, OpcodeOffset);
 
       if (Verbose) {
         *OS << "address += " << Delta.Address << ",  line += " << Delta.Line
             << "\n";
         OS->indent(12);
       }
       if (OS)
         State.Row.dump(*OS);
 
       State.appendRowToMatrix();
+      *OffsetPtr = Cursor.tell();
     }
 
     // When a row is added to the matrix, it is also dumped, which includes a
     // new line already, so don't add an extra one.
     if (Verbose && Rows.size() == RowCount)
       *OS << "\n";
+
+    // Most parse failures other than when parsing extended opcodes are due to
+    // failures to read ULEBs. Bail out of parsing, since we don't know where to
+    // continue reading from as there is no stated length for such byte
+    // sequences. Print the final trailing new line if needed before doing so.
+    if (!Cursor && Opcode != 0) {
+      if (Verbose)
+        *OS << "\n";
+      return Cursor.takeError();
+    }
+
+    if (!Cursor)
+      RecoverableErrorHandler(Cursor.takeError());
   }
 
   if (!State.Sequence.Empty)
     RecoverableErrorHandler(createStringError(
         errc::illegal_byte_sequence,
         "last sequence in debug line table at offset 0x%8.8" PRIx64
         " is not terminated",
         DebugLineOffset));

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

 # NONFATAL-NEXT:          length: 0x00000003
 # NONFATAL-NOT:  file_names
 # VERBOSE:       DW_LNE_set_address (0xababcdcdefef0909)
 # VERBOSE-NEXT:  DW_LNE_end_sequence
 
 ## Table with extended opcode that overruns table end.
 # NONFATAL:      debug_line[0x0000039d]
 # NONFATAL-NEXT: Line table prologue
-# VERBOSE:       DW_LNE_set_address (0x00000000feedfeed)
+# NONFATAL:      Address
+# NONFATAL-NEXT: ------------------
+# VERBOSE-NEXT:  DW_LNE_set_address (0x00000000feedfeed)
 # VERBOSE-NEXT:  DW_LNS_copy
-# VERBOSE:       DW_LNE_set_address (0x0000000000000000)
-# MORE-ERR:      warning: unexpected end of data at offset 0x3ed while reading [0x3e6, 0x3ee)
-# MORE-ERR:      warning: last sequence in debug line table at offset 0x0000039d is not terminated
+# NONFATAL-NEXT: 0x00000000feedfeed
+# VERBOSE-NEXT:  DW_LNE_set_address (0x0000000000000000)
+# MORE-ERR-NEXT: warning: unexpected end of data at offset 0x3ed while reading [0x3e6, 0x3ee)
+# MORE-ERR-NEXT: warning: last sequence in debug line table at offset 0x0000039d is not terminated
 
 # LAST:          debug_line[0x000003ed]
 # VERBOSE:       DW_LNE_set_address (0x00000000cafebabe)
 # VERBOSE-NEXT:  DW_LNE_end_sequence