[DebugInfo] Allow parsing line tables aligned to 4 or 8-byte boundaries

[benmxwl-arm]

Summary

This allows the DWARFDebugLine::SectionParser to try parsing line tables
at 4 or 8-byte boundaries if the unaligned offset appears invalid. If
aligning the offset does not reduce errors the offset is used unchanged.

This is needed for llvm-dwarfdump to be able to extract the line tables
(with --debug-lines) from binaries produced by certain compilers that
like to align each line table in the .debug_line section. Note that this
alignment does not seem to be invalid since the units do point to the
correct line table offsets via the DW_AT_stmt_list attribute.

[benmxwl-arm]

Currently this patch is using a heuristic due to the following comment, and because the unit tests currently don't create the associated units for the line tables.

// We want to supply the Unit associated with a .debug_line[.dwo] table when
// we dump it, if possible, but still dump the table even if there isn't a Unit.
// Therefore, collect up handles on all the Units that point into the
// line-table section.

[benmxwl-arm]

Corrected error handling

[benmxwl-arm]

Any thoughts on this or suggestions for reviewers?

[probinson]

+debug-info tag.

It's true that DWARF doesn't require the per-unit .debug_line chunks to be adjacent, which makes life difficult for dumpers who want to read just the .debug_line section (without parsing .debug_info headers). Debuggers with an edit-and-continue feature can dynamically replace bits of the line table, though, and in those cases padding is handy. That would usually be a bigger lump than just alignment, though. Just curious, what compiler(s) are aligning line-tables?

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

1546

[benmxwl-arm]

Just curious, what compiler(s) are aligning line-tables?

This was something noticed with binaries produced by the ARM C/C++ compiler. It aligns each line table entry and pads out the section size, which currently confuses llvm-dwarfdump.

[benmxwl-arm]

Typo fix

[probinson]

Sorry for letting this sit so long! Code looks okay, still needs a test. For something like this you probably need an assembler source with a hand-made .debug_line section.

[benmxwl-arm]

Added test case for aligned line tables (took a little while to get round to!)

[probinson]

Tidy up the test and LGTM.

llvm/test/tools/llvm-dwarfdump/ARM/aligned_line_tables.s

14

This CHECK-NOT is unnecessary. If you've matched the 28 offset, that shows you've found the correct place.

17

Again this CHECK-NOT is unnecessary. You could modify the previous CHECK line to end with 3a{{$}} to emphasize that nothing follows it.

20

Same thing here; make the previous check 2{{$}} (so it won't match something like 256) and remove the CHECK-NOT.

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

1508

We know we're in the DWARFDebugLine class, so we can simplify this function name somewhat. Perhaps to headerHasValidVersion or even simply hasValidVersion.

1514–1517

I'm not convinced you should be throwing away the error here. At the very least, there should be a comment explaining why it is reasonable to do so.

1539–1540

1549–1550

Can you use alignTo to do this alignment?

1555

Does this return imply that the PossibleAlignments must be in ascending order? That's fine for now, but I wonder if it's a bit brittle (I guess you could argue that if there's not enough space for alignment, it's not going to be big enough for a line table header, so it's moot, but if you want to rely on that, it should at least be explained in the comment, in case something changes in the future).

llvm/test/tools/llvm-dwarfdump/ARM/aligned_line_tables.s

5–7

It seems a little odd to be mixing comment marker styles (# and //). In newer tests, I tend to go with # for lit directives (like you have done) and ## for real comments.

23

This CHECK-NOT will be quite fragile - if somebody changes the warning wording for whatever reason, it will fail to catch the warning, should it be emitted. Instead, I'd just do CHECK-NOT: warning:.

[benmxwl-arm]

Address various review comments.

(I'm still using // in the test since llvm-mc with the arm-none-eabi triple does not accept
\# or \#\# as a comment).

[probinson]

In D143513#4178528, @benmxwl-arm wrote:

(I'm still using // in the test since llvm-mc with the arm-none-eabi triple does not accept
\# or \#\# as a comment).

I'm confused. The RUN lines and CHECK lines are using # as a comment introducer; if llvm-mc won't accept that, why does it work?

[benmxwl-arm]

It seems like they are removed by the test runner before passing the file to llvm-mc (but it only removes lines that start with a directive it recognises), though I can't point to precisely where this happens.
I'll just change the test to use // for the checks too for consistency.

[MaskRay]

# is a universal comment marker at the line beginning (ignoring whitespace). If there are non-space tokens before the comment, use // (for arm-* @ works as well but I don't think it is common).
// is a universal comment marker in GNU assembler.

% clang --target=arm-linux-gnueabi -c a.s  # aarch64 is similar
a.s:1:10: error: unexpected token
.long 5  # Unit

[MaskRay]

llvm/test/tools/llvm-dwarfdump/ARM/aligned_line_tables.s

25

Replace the magic number with a label difference.

You can define two temporary labels .L and use a subtraction. See how Clang generates the section.

27

Use a label difference.

[MaskRay]

llvm/test/tools/llvm-dwarfdump/ARM/aligned_line_tables.s

55

.long 58 is at offset 40, which is a multiple of 8 (also a multiple of 4).
I suggest that you create at least 3 RUN lines to cover the multiple-of-4, multiple-of-8, not-multiple-of-4 cases.

.ifdef XXX
.asciz "test.c"
.else
.asciz "testxxxx.c"  // appropriate number of padding bytes
.endif

If you do this, my earlier comment about using a label difference will show its advantage...

[MaskRay]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

1548

Certain compilers => ARM C/C++ Compiler.

Be more specific when available information is useful. It helps decide whether the code is a workaround and when it can be removed.

You can adjust the previous comment or this comment to say that we are testing 4 and 8 as possible alignments.

[jhenderson]

In D143513#4179083, @benmxwl-arm wrote:

It seems like they are removed by the test runner before passing the file to llvm-mc (but it only removes lines that start with a directive it recognises), though I can't point to precisely where this happens.

I am 99% certain that this is incorrect. The %s that is used for the llvm-mc test input is simply a lit substitution which expands to the current file's path. In other words, llvm-mc sees the entire test file verbatim, with no modification.

I'll just change the test to use // for the checks too for consistency.

This is fine from my point of view.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

1539–1540

You only did half my suggestion. The "Otherwise" should be the start of a new sentence.

[benmxwl-arm]

• Tidied up FileChecks
• Use labels for unit/header lengths
• Test 8 byte multiple offset, 4 (not 8) byte multiple offset, and invalid padding
• Applied various suggestions

[MaskRay]

llvm/test/tools/llvm-dwarfdump/ARM/aligned_line_tables.s

86

The number of padding will be fragile if the above directives are changed.

You may use llvm-mc ... --save-temp-labels to retain .L* symbols, then add // RUN: llvm-nm %t.o | FileCheck ... to test the st_value of .Ltable0_end. This will clearly show the needed padding bytes.

[benmxwl-arm]

• Verify values of .Ltable_end0 using llvm-nm

[jhenderson]

llvm/test/tools/llvm-dwarfdump/ARM/aligned_line_tables.s

13–15

FWIW, I'd use //// here, much like I'd use ## to distinguish it from test code. Same goes with other test comments later on.

19

Here and throughout, can you use CHECK-NEXT (well, MULT4-NEXT etc)?

29

Why are you specifically testing that there is no warning at this specific point in the prologue printing?

49

I recommend you match the full warning message here. Otherwise, it might match another warning that isn't relevant. Also, it helps readers to know what warning you are actually testing for.

[benmxwl-arm]

Updated test to use CHECK-NEXT and clearer comment styles.
(There was no reason the CHECK-NOT: warning: was where it was other than a misconception on my part)

P.s. Thanks for all the feedback on this test here; I've not written many manual file checks before,
so it's been very helpful in learning the system.

[jhenderson]

llvm/test/tools/llvm-dwarfdump/ARM/aligned_line_tables.s

19

So, in case it wasn't clear, this will only check that there is no warning between the two CHECK lines that are used either side of it. If you want to check that no warning appears in the output at all, you're better off adding --implicit-check-not='warning:' to the FileCheck command-line.

20

Not required, but as FileCheck by default doesn't treat whitespace as significant, when I have a line like this followed by -NEXT lines, I add a few extra spaces between the end of the CHECK and the actual to-be-matched data, so that everything lines up. In this case, I'd do:

// MULT4:      Address            Line   Column File   ISA Discriminator Flags

[benmxwl-arm]

Make use of --implicit-check-not='warning:' and some minor formatting changes.

[jhenderson]

Looks good.

[MaskRay]

llvm/test/tools/llvm-dwarfdump/ARM/aligned_line_tables.s

13–15

/// to distinguish non-CHECK-non-RUN comments should be fine :)

[MatzeB]

llvm/test/tools/llvm-dwarfdump/ARM/aligned_line_tables.s

2

This test needs something like // REQUIRES: arm-registered-target

[MaskRay]

llvm/test/tools/llvm-dwarfdump/ARM/aligned_line_tables.s

2

Thanks. I added lit.local.cfg to exclude the test for non-ARM builds instead.

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

public:
     bool done() const { return Done; }
 
     /// Get the offset the parser has reached.
     uint64_t getOffset() const { return Offset; }
 
   private:
     DWARFUnit *prepareToParse(uint64_t Offset);
     void moveToNextTable(uint64_t OldOffset, const Prologue &P);
+    bool hasValidVersion(uint64_t Offset);
 
     LineToUnitMap LineToUnit;
 
     DWARFDataExtractor &DebugLineData;
     const DWARFContext &Context;
     uint64_t Offset = 0;
     bool Done = false;
   };

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

DWARFUnit *DWARFDebugLine::SectionParser::prepareToParse(uint64_t Offset) {
   DWARFUnit *U = nullptr;
   auto It = LineToUnit.find(Offset);
   if (It != LineToUnit.end())
     U = It->second;
   DebugLineData.setAddressSize(U ? U->getAddressByteSize() : 0);
   return U;
 }
 
+bool DWARFDebugLine::SectionParser::hasValidVersion(uint64_t Offset) {
+  DataExtractor::Cursor Cursor(Offset);
+  auto [TotalLength, _] = DebugLineData.getInitialLength(Cursor);
+  DWARFDataExtractor HeaderData(DebugLineData, Cursor.tell() + TotalLength);
+  uint16_t Version = HeaderData.getU16(Cursor);
+  if (!Cursor) {
+    // Ignore any error here.
+    // If this is not the end of the section parseNext() will still be
+    // attempted, where this error will occur again (and can be handled).
+    consumeError(Cursor.takeError());
+    return false;
+  }
+  return versionIsSupported(Version);
+}
+
 void DWARFDebugLine::SectionParser::moveToNextTable(uint64_t OldOffset,
                                                     const Prologue &P) {
   // If the length field is not valid, we don't know where the next table is, so
   // cannot continue to parse. Mark the parser as done, and leave the Offset
   // value as it currently is. This will be the end of the bad length field.
   if (!P.totalLengthIsValid()) {
     Done = true;
     return;
   }
 
   Offset = OldOffset + P.TotalLength + P.sizeofTotalLength();
   if (!DebugLineData.isValidOffset(Offset)) {
     Done = true;
+    return;
+  }
+
+  // Heuristic: If the version is valid, then this is probably a line table.
+  // Otherwise, the offset might need alignment (to a 4 or 8 byte boundary).
+  if (hasValidVersion(Offset))
+    return;
+
+  // ARM C/C++ Compiler aligns each line table to word boundaries and pads out
+  // the .debug_line section to a word multiple. Note that in the specification
+  // this does not seem forbidden since each unit has a DW_AT_stmt_list.
+  for (unsigned Align : {4, 8}) {
+    uint64_t AlignedOffset = alignTo(Offset, Align);
+    if (!DebugLineData.isValidOffset(AlignedOffset)) {
+      // This is almost certainly not another line table but some alignment
+      // padding. This assumes the alignments tested are ordered, and are
+      // smaller than the header size (which is true for 4 and 8).
+      Done = true;
+      return;
+    }
+    if (hasValidVersion(AlignedOffset)) {
+      Offset = AlignedOffset;
+      break;
+    }
   }
 }

llvm/test/tools/llvm-dwarfdump/ARM/aligned_line_tables.s

+// RUN: llvm-mc %s -defsym ALIGN_4=1 -save-temp-labels -filetype obj -triple arm-none-eabi -o %t.o
+// RUN: llvm-nm %t.o | FileCheck %s --check-prefix=L4
+// RUN: llvm-dwarfdump -debug-line %t.o 2>&1 | FileCheck %s --check-prefix=MULT4
+
+// RUN: llvm-mc %s -defsym ALIGN_8=1 -save-temp-labels -filetype obj -triple arm-none-eabi -o %t.o
+// RUN: llvm-nm %t.o | FileCheck %s --check-prefix=L8
+// RUN: llvm-dwarfdump -debug-line %t.o 2>&1 | FileCheck %s --check-prefix=MULT8
+
+// RUN: llvm-mc %s -defsym UNALIGNED_PADDING=1 -save-temp-labels -filetype obj -triple arm-none-eabi -o %t.o
+// RUN: llvm-nm %t.o | FileCheck %s --check-prefix=LUNALIGN
+// RUN: llvm-dwarfdump -debug-line %t.o 2>&1 | FileCheck %s --check-prefix=UNALIGN
+
+// This test is based on a real example from ARM C/C++ Compiler.
+// It verifies llvm-dwarfdump is able to dump line tables even if they've been
+// placed at aligned offsets.
+
+// L4: 0000002b N .Ltable0_end
+// MULT4: Address            Line   Column File   ISA Discriminator Flags
+// MULT4: ------------------ ------ ------ ------ --- ------------- -------------
+// MULT4: 0x0000000000000000      1      0      1   0             0  is_stmt end_sequence
+// MULT4-EMPTY:
+// MULT4: debug_line[0x0000002c]
+// MULT4: Line table prologue:
+// MULT4:    total_length: 0x0000003a{{$}}
+// MULT4:          format: DWARF32
+// MULT4:         version: 2{{$}}
+// MULT4: prologue_length: 0x0000001a
+// MULT4: min_inst_length: 2
+// MULT4-NOT: warning:
+// MULT4: default_is_stmt: 1
+
+// L8: 00000027 N .Ltable0_end
+// MULT8: Address            Line   Column File   ISA Discriminator Flags
+// MULT8: ------------------ ------ ------ ------ --- ------------- -------------
+// MULT8: 0x0000000000000000      1      0      1   0             0  is_stmt end_sequence
+// MULT8-EMPTY:
+// MULT8: debug_line[0x00000028]
+// MULT8: Line table prologue:
+// MULT8:    total_length: 0x0000003a{{$}}
+// MULT8:          format: DWARF32
+// MULT8:         version: 2{{$}}
+// MULT8: prologue_length: 0x0000001a
+// MULT8: min_inst_length: 2
+// MULT8-NOT: warning:
+// MULT8: default_is_stmt: 1
+
+// This should fail to dump:
+// LUNALIGN: 00000027 N .Ltable0_end
+// UNALIGN: warning:
+
+.section .debug_line
+// First line table
+// Unit total length:
+.long .Ltable0_end - .Ltable0_start
+.Ltable0_start:
+.short 2        // Version
+// Header length:
+.long .Ltable0_header_end - .Ltable0_header_start
+.Ltable0_header_start:
+.byte 4         // Min instruction length
+.byte 1         // Max operations per instruction
+.byte 0         // Default is statement
+.byte 6         // Line range
+.byte 10        // Opcode base
+.byte 0         // standard_opcode_lengths[DW_LNS_copy] = 0
+.byte 1         // standard_opcode_lengths[DW_LNS_advance_pc] = 1
+.byte 1         // standard_opcode_lengths[DW_LNS_advance_line] = 1
+.byte 1         // standard_opcode_lengths[DW_LNS_set_file] = 1
+.byte 1         // standard_opcode_lengths[DW_LNS_set_column] = 1
+.byte 0         // standard_opcode_lengths[DW_LNS_negate_stmt] = 0
+.byte 0         // standard_opcode_lengths[DW_LNS_set_basic_block] = 0
+.byte 0         // standard_opcode_lengths[DW_LNS_const_add_pc] = 0
+.byte 0         // standard_opcode_lengths[DW_LNS_fixed_advance_pc] = 0
+.byte 0         // No include directories
+// File name:
+.ifdef ALIGN_4
+// Pad out filename so next 4 byte aligned offset is a multiple of 4 and not 8.
+.asciz "foobar.cpp"
+.else
+.asciz "test.c"
+.endif
+.byte 0         // Dir idx
+.byte 0         // Mod time
+.byte 0         // Length
+.byte 0         // End files
+.Ltable0_header_end:
+// Line table operations
+.byte 0         // Extended opcode
+.byte 1         // Length 1
+.byte 1         // DW_LNE_end_sequence
+.Ltable0_end:
+// End first line table
+// Padding:
+.ifdef UNALIGNED_PADDING
+.short 0
+.else
+.byte 0
+.endif
+// Second line table
+// Unit total length:
+.long .Ltable1_end - .Ltable1_start
+.Ltable1_start:
+.short 2        // Version
+// Header length:
+.long .Ltable1_header_end - .Ltable1_header_start
+.Ltable1_header_start:
+.byte 2         // Min instruction length
+.byte 1         // Max operations per instruction
+.byte 0         // Default is statement
+.byte 6         // Line range
+.byte 10        // Opcode base
+.byte 0         // standard_opcode_lengths[DW_LNS_copy] = 0
+.byte 1         // standard_opcode_lengths[DW_LNS_advance_pc] = 1
+.byte 1         // standard_opcode_lengths[DW_LNS_advance_line] = 1
+.byte 1         // standard_opcode_lengths[DW_LNS_set_file] = 1
+.byte 1         // standard_opcode_lengths[DW_LNS_set_column] = 1
+.byte 0         // standard_opcode_lengths[DW_LNS_negate_stmt] = 0
+.byte 0         // standard_opcode_lengths[DW_LNS_set_basic_block] = 0
+.byte 0         // standard_opcode_lengths[DW_LNS_const_add_pc] = 0
+.byte 0         // standard_opcode_lengths[DW_LNS_fixed_advance_pc] = 0
+.byte 0         // No include directories
+.asciz "test.c" // File name
+.byte 0         // Dir idx
+.byte 0         // Mod time
+.byte 0         // Length
+.byte 0         // End files
+.Ltable1_header_end:
+// Line table operations
+.byte 4         // DW_LNS_set_file
+.byte 1         // File 1
+.byte 5         // DW_LNS_set_column
+.byte 1         // Column 1
+.byte 0         // Extended opcode
+.byte 5         // Length 5
+.byte 2         // DW_LNE_set_address
+.long 32896     // Address = 0x00008080
+.byte 3         // DW_LNS_advance_line
+.byte 6         // Line += 6
+.byte 1         // DW_LNS_copy
+.byte 5         // DW_LNS_set_column
+.byte 2         // Column 2
+.byte 12        // Special opcode (address += 0,  line += 2)
+.byte 30        // Special opcode (address += 6,  line += 2)
+.byte 5         // DW_LNS_set_column
+.byte 1         // Column 1
+.byte 17        // Special opcode (address += 2,  line += 1)
+.byte 2         // DW_LNS_advance_pc
+.byte 4         // += (4 * min instruction length)
+.byte 0         // Extended opcode
+.byte 1         // Length 1
+.byte 1         // DW_LNE_end_sequence
+.Ltable1_end:
+// End second line table
+.short 0        // Padding (to make section a word multiple)