[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 ↗

(On Diff #243112)

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 ↗

(On Diff #243112)

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/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