[DebugInfo] Reject line tables of version > 5

[jhenderson]

Summary

If a debug line section with version of greater than 5 is encountered, prior to this change the parser would accept it and treat it as version 5. This might work to some extent, but then it might not at all, as it really depends on the format of the unspecified future version, which will be different (otherwise there would be no point in changing the version number). Any information we could provide has a good chance of being invalid, so we should just refuse to parse such tables.

Depends on D74202.

[MaskRay]

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

67

max -> the max?

[dblaikie]

Might be worth a different strategy (either add to the end, and/or use stable names rather than numbers). But otherwise seems fine.

Other thoughts: Probably print the unknown version number in decimal rather than hex? Probably doesn't need both an assembly+dumper test and a unit test. One or the other (which ever is most convenient - the unit test looks simpler/more self contained & targeted) is fine by me - assuming the general codepath for error handling is already tested for the dumper, for instance. (ie: the changed code is exercised in the new test, doesn't mean all paths that reach that changed code need to be retested).

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

158

Perhaps numbering cases isn't a good idea if new cases are going to be added in the middle & shift all the numbers? Perhaps a short textual name would be better, then? (like test cases in a GUnit test, for instance)

[jhenderson]

In D74204#1864663, @dblaikie wrote:

Might be worth a different strategy (either add to the end, and/or use stable names rather than numbers). But otherwise seems fine.

I think I'm going to do a follow-up review to remove the numbers. Previously, all my new cases were added to the end, but this test case is strongly related to the other version ones, so I thought it should belong with it.

Other thoughts: Probably print the unknown version number in decimal rather than hex?

Yeah, printing in decimal makes sense. I'd like to do that in a follow-up, as it's a change to a pre-existing message.

Probably doesn't need both an assembly+dumper test and a unit test. One or the other (which ever is most convenient - the unit test looks simpler/more self contained & targeted) is fine by me - assuming the general codepath for error handling is already tested for the dumper, for instance. (ie: the changed code is exercised in the new test, doesn't mean all paths that reach that changed code need to be retested).

Seems okay to me (I'd be inclined to keep the unit test here). I've been uncertain where to draw the distinction between the two, if I'm honest, and a number of my other recent changes have added both. On a previous code base I worked on, it was generally expected that there were both unit tests and system (i.e. lit-equivalent) testing for any change, so I've tended to err on the side of over-testing rather than under-testing.

[dblaikie]

In D74204#1866664, @jhenderson wrote:

In D74204#1864663, @dblaikie wrote:

Might be worth a different strategy (either add to the end, and/or use stable names rather than numbers). But otherwise seems fine.

I think I'm going to do a follow-up review to remove the numbers. Previously, all my new cases were added to the end, but this test case is strongly related to the other version ones, so I thought it should belong with it.

Sounds good to me.

Other thoughts: Probably print the unknown version number in decimal rather than hex?

Yeah, printing in decimal makes sense. I'd like to do that in a follow-up, as it's a change to a pre-existing message.

Sure sure.

Probably doesn't need both an assembly+dumper test and a unit test. One or the other (which ever is most convenient - the unit test looks simpler/more self contained & targeted) is fine by me - assuming the general codepath for error handling is already tested for the dumper, for instance. (ie: the changed code is exercised in the new test, doesn't mean all paths that reach that changed code need to be retested).

Seems okay to me (I'd be inclined to keep the unit test here). I've been uncertain where to draw the distinction between the two, if I'm honest, and a number of my other recent changes have added both. On a previous code base I worked on, it was generally expected that there were both unit tests and system (i.e. lit-equivalent) testing for any change, so I've tended to err on the side of over-testing rather than under-testing.

I'll admit I'm a bit fussy about testing - both too much and too little can be maintenance problems & LLVM's choices aren't necessarily the best, nor applicable to all other projects/types of code.

My basic rule of thumb is: "If it's unlikly/impossible to introduce a bug that will cause only this test to fail and no others, then this test is probably unnecessary/excessive". And while LLVM historically tends towards end-to-end testing (where the end-to-end might be quite small, using some targeted tool like llvm-dwarfdump, opt, etc) with relatively little unit testing - the latter can be the right tool for the job in some situations & this may be one of them. Where the end-to-end error handling path is tested, but a smaller/easier to read/etc targeted unit test can then exercise the various sources of errors, without having to test all the way through the end-to-end every for every one.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

if (TotalLength == dwarf::DW_LENGTH_DWARF64) {
     // Treat this error as unrecoverable - we have no way of knowing where the
     // table ends.
     return createStringError(errc::invalid_argument,
         "parsing line table prologue at offset 0x%8.8" PRIx64
         " unsupported reserved unit length found of value 0x%8.8" PRIx64,
         PrologueOffset, TotalLength);
   }
   FormParams.Version = DebugLineData.getU16(OffsetPtr);
-  if (getVersion() < 2)
+  if (getVersion() < 2 || getVersion() > 5)
     // Treat this error as unrecoverable - we cannot be sure what any of
     // the data represents including the length field, so cannot skip it or make
     // any reasonable assumptions.
     return createStringError(errc::not_supported,
                        "parsing line table prologue at offset 0x%8.8" PRIx64
                        " found unsupported version 0x%2.2" PRIx16,
                        PrologueOffset, getVersion());
 

llvm/test/tools/llvm-dwarfdump/X86/Inputs/debug_line_malformed.s

 .Lunit_v0_end:
 
 # Version 1.
 .long   .Lunit_v1_end - .Lunit_v1_start # unit length
 .Lunit_v1_start:
 .short  1               # version
 .Lunit_v1_end:
 
+# Version greater than the max supported.
+.long   .Lunit_v6_end - .Lunit_v6_start # unit length
+.Lunit_v6_start:
+.short  6               # version
+.Lunit_v6_end:
+
 # Version 5 malformed line/include table.
 .long   .Lunit_v5_end - .Lunit_v5_start # unit length
 .Lunit_v5_start:
 .short  5               # version
 .byte   8               # address size
 .byte   8               # segment selector
 .long   .Lprologue_v5_end-.Lprologue_v5_start # Length of Prologue
 .Lprologue_v5_start:

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

 # RUN: FileCheck %s --input-file=%t-malformed-verbose.err --check-prefixes=ALL,OTHER
 
 ## We should still produce warnings for malformed tables after the specified unit.
 # RUN: llvm-dwarfdump -debug-line=0 %t-malformed.o 2> %t-malformed-off-first.err \
 # RUN:   | FileCheck %s --check-prefixes=FIRST,NOLATER
 # RUN: FileCheck %s --input-file=%t-malformed-off-first.err --check-prefix=ALL
 
 ## Don't stop looking for the later unit if non-fatal issues are found.
-# RUN: llvm-dwarfdump -debug-line=0x332 %t-malformed.o 2> %t-malformed-off-last.err \
+# RUN: llvm-dwarfdump -debug-line=0x338 %t-malformed.o 2> %t-malformed-off-last.err \
 # RUN:   | FileCheck %s --check-prefix=LAST --implicit-check-not='debug_line[{{.*}}]'
 # RUN: FileCheck %s --input-file=%t-malformed-off-last.err --check-prefix=ALL
 
 # FIRST:       debug_line[0x00000000]
 # FIRST:       0x000000000badbeef {{.*}} end_sequence
 # NOFIRST-NOT: debug_line[0x00000000]
 # NOFIRST-NOT: 0x000000000badbeef {{.*}} end_sequence
 # NOLATER-NOT: debug_line[{{.*}}]

 # NONFATAL-NEXT: Line table prologue
 # NONFATAL-NOT:  Address
 
 ## Case 2: Version 1 table.
 # NONFATAL:      debug_line[0x0000004e]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL-NOT:  Address
 
-## Case 3: Malformed directory format with no path component.
+## Case 3: Version greater than max supported.
 # NONFATAL:      debug_line[0x00000054]
 # NONFATAL-NEXT: Line table prologue
+# NONFATAL-NOT:  Address
+
+## Case 4: Malformed directory format with no path component.
+# NONFATAL:      debug_line[0x0000005a]
+# NONFATAL-NEXT: Line table prologue
 # NONFATAL-NOT:  include_directories
 # NONFATAL-NOT:  file_names
 # NONFATAL:      0x8877665544332211 {{.*}} end_sequence
 
-## Case 4: Prologue with length shorter than parsed.
-# NONFATAL:      debug_line[0x00000081]
+## Case 5: Prologue with length shorter than parsed.
+# NONFATAL:      debug_line[0x00000087]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      file_names[  2]:
 # NONFATAL-NEXT:            name: "file2"
 # NONFATAL-NEXT:       dir_index: 1
 # NONFATAL-NEXT:        mod_time: 0x00000002
 # NONFATAL-NEXT:          length: 0x00000006
 # NONFATAL:      0x1122334455667788 {{.*}} 0 end_sequence{{$}}
 
-## Case 5: Prologue with length longer than parsed.
-# NONFATAL:      debug_line[0x000000c8]
+## Case 6: Prologue with length longer than parsed.
+# NONFATAL:      debug_line[0x000000ce]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      file_names[  2]:
 # NONFATAL-NEXT:            name: "file2"
 # NONFATAL-NEXT:       dir_index: 1
 # NONFATAL-NEXT:        mod_time: 0x00000002
 # NONFATAL-NEXT:          length: 0x00000003
 # NONFATAL-NOT:  file_names
 # NONFATAL:      0x1111222233334444 {{.*}} is_stmt end_sequence
 
-## Case 6: Extended opcode with incorrect length versus expected.
-# NONFATAL:      debug_line[0x00000111]
+## Case 7: Extended opcode with incorrect length versus expected.
+# NONFATAL:      debug_line[0x00000117]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL: 0x00000000abbadaba {{.*}} end_sequence
 # NONFATAL: 0x00000000babb1e45 {{.*}} 10 is_stmt prologue_end end_sequence{{$}}
 
-## Case 7: No end of sequence.
-# NONFATAL:      debug_line[0x0000016c]
+## Case 8: No end of sequence.
+# NONFATAL:      debug_line[0x00000172]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      0x00000000deadfade {{.*}} is_stmt
 # NONFATAL-NOT:  end_sequence
 
-## Case 8: Very short prologue length for V5 (ends during parameters).
-# NONFATAL:      debug_line[0x000001b2]
+## Case 9: Very short prologue length for V5 (ends during parameters).
+# NONFATAL:      debug_line[0x000001b8]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      standard_opcode_lengths[DW_LNS_set_isa] = 1
 # NONFATAL-NEXT: include_directories[  0] = "/tmp"
 # NONFATAL-NEXT: file_names[  0]:
 # NONFATAL-NEXT:            name: "xyz"
 # NONFATAL:      0x0000000000000000 1 0 1 0 0 is_stmt end_sequence
 # NONFATAL:      0x0000babb1ebabb1e {{.*}} end_sequence
 
-## Case 9: V5 prologue ends during file table.
-# NONFATAL:      debug_line[0x000001ee]
+## Case 10: V5 prologue ends during file table.
+# NONFATAL:      debug_line[0x000001f4]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      include_directories[  0] = "/tmp"
 # NONFATAL-NEXT: file_names[  0]:
 # NONFATAL-NEXT:            name: "xyz"
 # NONFATAL-NEXT:       dir_index: 1
 # NONFATAL:      0x0000000000000000 {{.*}} epilogue_begin
 # NONFATAL:      0x00000ab4acadab4a {{.*}} end_sequence
 
-## Case 10: V5 prologue ends during directory table.
-# NONFATAL:      debug_line[0x0000022f]
+## Case 11: V5 prologue ends during directory table.
+# NONFATAL:      debug_line[0x00000235]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      include_directories[  0] = "/tmp"
 # NONFATAL-NEXT: file_names[  0]:
 # NONFATAL-NEXT:            name: "xyz"
 # NONFATAL:      0x0000000000000002 2 0 1 0 0 is_stmt{{$}}
 # NONFATAL:      0x4444333322221111 {{.*}} end_sequence
 
-## Case 11: V5 invalid MD5 hash form when there is still data to be read.
-# NONFATAL:      debug_line[0x0000026b]
+## Case 12: V5 invalid MD5 hash form when there is still data to be read.
+# NONFATAL:      debug_line[0x00000271]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      include_directories[  0] = "/tmp"
 # NONFATAL-NOT:  file_names
 # NONFATAL-NOT:  is_stmt
 # NONFATAL:      0x1234123412341234 {{.*}} end_sequence
 
-## Case 12: V5 invalid MD5 hash form when data beyond the prologue length has
+## Case 13: V5 invalid MD5 hash form when data beyond the prologue length has
 ## been read before the MD5 problem is identified.
-# NONFATAL:      debug_line[0x000002ae]
+# NONFATAL:      debug_line[0x000002b4]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      include_directories[  0] = "/tmp"
 # NONFATAL-NOT:  file_names
 # NONFATAL:      0x0000000000000000 {{.*}} epilogue_begin
 # NONFATAL:      0x4321432143214321 {{.*}} is_stmt end_sequence
 
-## Case 13: V5 invalid directory content description has unsupported form.
-# NONFATAL:      debug_line[0x000002ec]
+## Case 14: V5 invalid directory content description has unsupported form.
+# NONFATAL:      debug_line[0x000002f2]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      include_directories[  0] = "/foo"
 # NONFATAL-NOT:  include_directories
 # NONFATAL-NOT:  file_names
 # NONFATAL:      0xaaaabbbbccccdddd {{.*}} is_stmt end_sequence
 
-# LAST:          debug_line[0x00000332]
+# LAST:          debug_line[0x00000338]
 # LAST:          0x00000000cafebabe {{.*}} end_sequence
 
 # RESERVED: warning: parsing line table prologue at offset 0x00000048 unsupported reserved unit length found of value 0xfffffffe
 
 # ALL-NOT:  warning:
 # ALL:      warning: parsing line table prologue at offset 0x00000048 found unsupported version 0x00
 # ALL-NEXT: warning: parsing line table prologue at offset 0x0000004e found unsupported version 0x01
-# ALL-NEXT: warning: parsing line table prologue at 0x00000054 found an invalid directory or file table description at 0x00000073
+# ALL-NEXT: warning: parsing line table prologue at offset 0x00000054 found unsupported version 0x06
+# ALL-NEXT: warning: parsing line table prologue at 0x0000005a found an invalid directory or file table description at 0x00000079
 # ALL-NEXT: warning: failed to parse entry content descriptions because no path was found
-# ALL-NEXT: warning: parsing line table prologue at 0x00000081 should have ended at 0x000000b9 but it ended at 0x000000ba
-# ALL-NEXT: warning: parsing line table prologue at 0x000000c8 should have ended at 0x00000103 but it ended at 0x00000102
-# OTHER-NEXT: warning: unexpected line op length at offset 0x00000158 expected 0x02 found 0x01
-# OTHER-NEXT: warning: unexpected line op length at offset 0x0000015c expected 0x01 found 0x02
-# OTHER-NEXT: warning: last sequence in debug line table at offset 0x0000016c is not terminated
-# ALL-NEXT: warning: parsing line table prologue at 0x000001b2 should have ended at 0x000001ce but it ended at 0x000001e1
-# ALL-NEXT: warning: parsing line table prologue at 0x000001ee should have ended at 0x00000219 but it ended at 0x00000220
-# ALL-NEXT: warning: parsing line table prologue at 0x0000022f should have ended at 0x00000251 but it ended at 0x0000025e
-# ALL-NEXT: warning: parsing line table prologue at 0x0000026b found an invalid directory or file table description at 0x0000029f
+# ALL-NEXT: warning: parsing line table prologue at 0x00000087 should have ended at 0x000000bf but it ended at 0x000000c0
+# ALL-NEXT: warning: parsing line table prologue at 0x000000ce should have ended at 0x00000109 but it ended at 0x00000108
+# OTHER-NEXT: warning: unexpected line op length at offset 0x0000015e expected 0x02 found 0x01
+# OTHER-NEXT: warning: unexpected line op length at offset 0x00000162 expected 0x01 found 0x02
+# OTHER-NEXT: warning: last sequence in debug line table at offset 0x00000172 is not terminated
+# ALL-NEXT: warning: parsing line table prologue at 0x000001b8 should have ended at 0x000001d4 but it ended at 0x000001e7
+# ALL-NEXT: warning: parsing line table prologue at 0x000001f4 should have ended at 0x0000021f but it ended at 0x00000226
+# ALL-NEXT: warning: parsing line table prologue at 0x00000235 should have ended at 0x00000257 but it ended at 0x00000264
+# ALL-NEXT: warning: parsing line table prologue at 0x00000271 found an invalid directory or file table description at 0x000002a5
 # ALL-NEXT: warning: failed to parse file entry because the MD5 hash is invalid
-# ALL-NEXT: warning: parsing line table prologue at 0x000002ae found an invalid directory or file table description at 0x000002e0
+# ALL-NEXT: warning: parsing line table prologue at 0x000002b4 found an invalid directory or file table description at 0x000002e6
 # ALL-NEXT: warning: failed to parse file entry because the MD5 hash is invalid
-# ALL-NEXT: warning: parsing line table prologue at 0x000002ae should have ended at 0x000002d9 but it ended at 0x000002e0
-# ALL-NEXT: warning: parsing line table prologue at 0x000002ec found an invalid directory or file table description at 0x00000315
+# ALL-NEXT: warning: parsing line table prologue at 0x000002b4 should have ended at 0x000002df but it ended at 0x000002e6
+# ALL-NEXT: warning: parsing line table prologue at 0x000002f2 found an invalid directory or file table description at 0x0000031b
 # ALL-NEXT: warning: failed to parse directory entry because skipping the form value failed.
 # ALL-NOT:  warning:

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

TEST_F(DebugLineBasicFixture, ErrorForLowVersion) {
   generate();
 
   checkGetOrParseLineTableEmitsFatalError(
       "parsing line table prologue at offset "
       "0x00000000 found unsupported version "
       "0x01");
 }
 
+TEST_F(DebugLineBasicFixture, ErrorForHighVersion) {
+  if (!setupGenerator())
+    return;
+
+  LineTable &LT = Gen->addLineTable();
+  LT.setCustomPrologue(
+      {{LineTable::Half, LineTable::Long}, {6, LineTable::Half}});
+
+  generate();
+
+  checkGetOrParseLineTableEmitsFatalError(
+      "parsing line table prologue at offset 0x00000000 found unsupported "
+      "version 0x06");
+}
+
 TEST_F(DebugLineBasicFixture, ErrorForInvalidV5IncludeDirTable) {
   if (!setupGenerator(5))
     return;
 
   LineTable &LT = Gen->addLineTable();
   LT.setCustomPrologue({
       {19, LineTable::Long}, // unit length
       {5, LineTable::Half},  // version