[llvm-dwarfdump] Add a table header for -debug-line -verbose output

[MaskRay]

Summary

Like non-verbose output, so that it is easy to recognize the Line,Column,File,ISA,Discriminator column values.

[jhenderson]

Column headers probably make some sense. This might clash with my ongoing patches in the area, but I think that's okay - we can both adapt patches as needed without any real dififculty. You should probably update the primary verbose test (i.e. verbose.test) rather than (or possibly in addition to) the test you have modified though. If you do it in addition, you probably don't need the --strict-whitespace/--match-full-lines options in the debug-line-dw-lne-end-sequence.s test.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

706

Will 12 be the right number if using DWARF64 (i.e. with 64-bit section offsets)?

(probably not, but maybe okay for now with a FIXME/TODO for DWARF64, as it looks like we aren't currently printing the offsets as fixed width for DWARF64).

[ikudrin]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

706

I'd prefer to keep the current convention of printing offsets. They are not something that has a real size in the file, like fields that we started printing with extended widths in D79997. They are just positions in sections and in most cases they will not exceed 4G boundary, even for the DWARF64 case. Printing all offsets in a unified way makes the report tidier.

[MaskRay]

Update verbose.test instead

[JDevlieghere]

LGTM

[jhenderson]

Sorry, thought I posted this comment yesterday, but I guess not.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

705–706

Another thought: maybe we should only print the column headers if there is no PrologueErr. I'm also not convinced we need the extra new line before the header. I'd ideally like to see a test for that if it is to be included.

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

21–22

By the way, I have a patch ready to put up for review to fix all these extra blank lines, so no need for others to fix them themselves.

[MaskRay]

Remove empty line. Don't dump table header if PrologueErr

[jhenderson]

LGTM, with one remaining request.

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

41–42 ↗

(On Diff #267903)

Since the rest of this test doesn't line up with the header here, I'd just get rid of the extra spacing. The test looks odd to me with it.

[jhenderson]

@MaskRay - are you okay to land this today? I have an update for D80989 that relies on it, and it'll be easier to work with this patch if it's already in master!

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

712–713

Actually, just realised this is the wrong place for this - this will cause, in verbose output, the header to be printed, even if there are no rows, unlike non-verbose output. We should try to match the two up. Also, there's a new line in non-verbose output between the prologue and this header, please add it here too.

I think the best thing to do might be to move this code to just inside the while loop, executed on the first run only. This would mean the header is printed before any verbose opcodes are printed, although it would also mean it was printed even if there are no rows emitted to the table, due to a malformed table, which is slightly inconsistent. I'm open to alternatives. For now, I'm going to use my suggestion in my update to D80989. That will also cause the difference between non-verbose and verbose output to disappear, but not between those outputs and the LineTable::dump().

[MaskRay]

Move the header within while (*OffsetPtr < EndOffset)

[MaskRay]

Simplify

[jhenderson]

Thanks for the update. LGTM, with nit.

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

41–42 ↗

(On Diff #267903)

Looks like this hasn't been addressed still?

[MaskRay]

Delete excess indentation

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

public:
   struct Row {
     explicit Row(bool DefaultIsStmt = false);
 
     /// Called after a row is appended to the matrix.
     void postAppend();
     void reset(bool DefaultIsStmt);
     void dump(raw_ostream &OS) const;
 
-    static void dumpTableHeader(raw_ostream &OS);
+    static void dumpTableHeader(raw_ostream &OS, unsigned Indent);
 
     static bool orderByAddress(const Row &LHS, const Row &RHS) {
       return std::tie(LHS.Address.SectionIndex, LHS.Address.Address) <
              std::tie(RHS.Address.SectionIndex, RHS.Address.Address);
     }
 
     /// The program-counter value corresponding to a machine instruction
     /// generated by the compiler and section index pointing to the section

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

void DWARFDebugLine::Row::reset(bool DefaultIsStmt) {
   Discriminator = 0;
   IsStmt = DefaultIsStmt;
   BasicBlock = false;
   EndSequence = false;
   PrologueEnd = false;
   EpilogueBegin = false;
 }
 
-void DWARFDebugLine::Row::dumpTableHeader(raw_ostream &OS) {
-  OS << "Address            Line   Column File   ISA Discriminator Flags\n"
+void DWARFDebugLine::Row::dumpTableHeader(raw_ostream &OS, unsigned Indent) {
+  OS.indent(Indent)
+      << "Address            Line   Column File   ISA Discriminator Flags\n";
+  OS.indent(Indent)
       << "------------------ ------ ------ ------ --- ------------- "
          "-------------\n";
 }
 
 void DWARFDebugLine::Row::dump(raw_ostream &OS) const {
   OS << format("0x%16.16" PRIx64 " %6u %6u", Address.Address, Line, Column)
      << format(" %6u %3u %13u ", File, Isa, Discriminator)
      << (IsStmt ? " is_stmt" : "") << (BasicBlock ? " basic_block" : "")
      << (PrologueEnd ? " prologue_end" : "")
      << (EpilogueBegin ? " epilogue_begin" : "")

 DWARFDebugLine::LineTable::LineTable() { clear(); }
 
 void DWARFDebugLine::LineTable::dump(raw_ostream &OS,
                                      DIDumpOptions DumpOptions) const {
   Prologue.dump(OS, DumpOptions);
 
   if (!Rows.empty()) {
     OS << '\n';
-    Row::dumpTableHeader(OS);
+    Row::dumpTableHeader(OS, 0);
     for (const Row &R : Rows) {
       R.dump(OS);
     }
   }
 
   // Terminate the table with a final blank line to clearly delineate it from
   // later dumps.
   OS << '\n';
Error DWARFDebugLine::LineTable::parse(
   Error PrologueErr =
       Prologue.parse(DebugLineData, OffsetPtr, RecoverableErrorHandler, Ctx, U);
 
   if (OS) {
     // The presence of OS signals verbose dumping.
     DIDumpOptions DumpOptions;
     DumpOptions.Verbose = true;
     Prologue.dump(*OS, DumpOptions);
+    *OS << '\n';
+    Row::dumpTableHeader(*OS, 12);
   }
 
   if (PrologueErr)
     return PrologueErr;
 
   uint64_t ProgramLength = Prologue.TotalLength + Prologue.sizeofTotalLength();
   if (!DebugLineData.isValidOffsetForDataOfSize(DebugLineOffset,
                                                 ProgramLength)) {
     assert(DebugLineData.size() > DebugLineOffset &&
            "prologue parsing should handle invalid offset");
     uint64_t BytesRemaining = DebugLineData.size() - DebugLineOffset;
     RecoverableErrorHandler(
         createStringError(errc::invalid_argument,
                           "line table program with offset 0x%8.8" PRIx64
                           " has length 0x%8.8" PRIx64 " but only 0x%8.8" PRIx64

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

for (const auto &Row : LineTable->Rows) {
       if (Row.Address.Address < PrevAddress) {
         ++NumDebugLineErrors;
         error() << ".debug_line["
                 << format("0x%08" PRIx64,
                           *toSectionOffset(Die.find(DW_AT_stmt_list)))
                 << "] row[" << RowIndex
                 << "] decreases in address from previous row:\n";
 
-        DWARFDebugLine::Row::dumpTableHeader(OS);
+        DWARFDebugLine::Row::dumpTableHeader(OS, 0);
         if (RowIndex > 0)
           LineTable->Rows[RowIndex - 1].dump(OS);
         Row.dump(OS);
         OS << '\n';
       }
 
       // Verify file index.
       if (!LineTable->hasFileAtIndex(Row.File)) {
         ++NumDebugLineErrors;
         bool isDWARF5 = LineTable->Prologue.getVersion() >= 5;
         error() << ".debug_line["
                 << format("0x%08" PRIx64,
                           *toSectionOffset(Die.find(DW_AT_stmt_list)))
                 << "][" << RowIndex << "] has invalid file index " << Row.File
                 << " (valid values are [" << (isDWARF5 ? "0," : "1,")
                 << LineTable->Prologue.FileNames.size()
                 << (isDWARF5 ? ")" : "]") << "):\n";
-        DWARFDebugLine::Row::dumpTableHeader(OS);
+        DWARFDebugLine::Row::dumpTableHeader(OS, 0);
         Row.dump(OS);
         OS << '\n';
       }
       if (Row.EndSequence)
         PrevAddress = 0;
       else
         PrevAddress = Row.Address.Address;
       ++RowIndex;

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

-# RUN: llvm-mc %S/brief.s -filetype obj -triple x86_64-apple-darwin -o - \
-# RUN: | llvm-dwarfdump -v --debug-info --debug-line - \
-# RUN: | FileCheck %s
+# RUN: llvm-mc %S/brief.s -filetype obj -triple x86_64-apple-darwin -o %t.o
+# RUN: llvm-dwarfdump -v --debug-info --debug-line %t.o | \
+# RUN:   FileCheck %s --match-full-lines --strict-whitespace
 
-# CHECK-NOT: .debug_abbrev contents:
+#   CHECK-NOT:{{.*}} contents:
 
-# CHECK: .debug_info contents:
-# CHECK: DW_TAG_compile_unit [1]
-# CHECK: DW_AT_name [DW_FORM_strp] ( .debug_str[0x{{.*}}] = "brief.c")
-# CHECK: DW_AT_name [DW_FORM_strp]       ( .debug_str[0x{{.*}}] = "main")
-# CHECK: DW_AT_name [DW_FORM_strp]       ( .debug_str[0x{{.*}}] = "int")
+#       CHECK:.debug_info contents:
+#       CHECK:0x0000000b: DW_TAG_compile_unit [1] *
+#       CHECK:              DW_AT_name [DW_FORM_strp] ( .debug_str[0x{{.*}}] = "brief.c")
+#       CHECK:0x0000002a:   DW_TAG_subprogram [2]  
+#       CHECK:                DW_AT_name [DW_FORM_strp] ( .debug_str[0x{{.*}}] = "main")
+#       CHECK:0x00000043:   DW_TAG_base_type [3]  
+#  CHECK-NEXT:                DW_AT_name [DW_FORM_strp] ( .debug_str[0x{{.*}}] = "int")
 
-# CHECK: .debug_line contents:
-# CHECK: 00 DW_LNE_set_address (0x0000000000000000)
-# CHECK: 01 DW_LNS_copy
-# CHECK: 0x0000000000000000      1      0      1   0             0  is_stmt
-# CHECK: 05 DW_LNS_set_column (12)
-# CHECK: 0a DW_LNS_set_prologue_end
-# CHECK: 66 address += 6,  line += 0
-# CHECK: 0x0000000000000006      1     12      1   0             0  is_stmt prologue_end
-# CHECK: 02 DW_LNS_advance_pc (2)
-# CHECK: 00 DW_LNE_end_sequence
-# CHECK: 0x0000000000000008      1     12      1   0             0  is_stmt end_sequence
-#
-# CHECK-NOT: {{.*}} contents:
+#       CHECK:.debug_line contents:
+#       CHECK:            Address            Line   Column File   ISA Discriminator Flags
+#  CHECK-NEXT:            ------------------ ------ ------ ------ --- ------------- -------------
+#  CHECK-NEXT:0x00000029: 00 DW_LNE_set_address (0x0000000000000000)
+#  CHECK-NEXT:0x00000034: 01 DW_LNS_copy
+#  CHECK-NEXT:            0x0000000000000000      1      0      1   0             0  is_stmt
+# CHECK-EMPTY:
+# CHECK-EMPTY:
+#       CHECK:0x00000035: 05 DW_LNS_set_column (12)
+#  CHECK-NEXT:0x00000037: 0a DW_LNS_set_prologue_end
+#  CHECK-NEXT:0x00000038: 66 address += 6,  line += 0
+#  CHECK-NEXT:            0x0000000000000006      1     12      1   0             0  is_stmt prologue_end
+# CHECK-EMPTY:
+#  CHECK-NEXT:0x00000039: 02 DW_LNS_advance_pc (2)
+#  CHECK-NEXT:0x0000003b: 00 DW_LNE_end_sequence
+#  CHECK-NEXT:            0x0000000000000008      1     12      1   0             0  is_stmt end_sequence
+
+#   CHECK-NOT:{{.*}} contents: