[DWARFDebugLine] Use new DWARFDataExtractor::getInitialLength

[labath]

Summary

The error messages change somewhat, but I believe the overall
informational value remains unchanged.

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

347–350

If I understand this correctly, this will now report cases where the unit length couldn't be read due to data running out. Should this change include some testing for that too?

Also, in that case, if I've followed the code correctly, the error message will be "parsing line table prologue at offset 0x00000000: unexpected end of data", which doesn't give particularly great precision as to where the unexpected end of data was. How about adding something like "parsing line table prologue at offset 0x00000000: unable to read unit_length: unexpected end of data"?

1218

Idle thought: If wonder if this should be unit tested (probably at a slightly higher level)? I could easily see a bug where a length equal to DW_LENGTH_lo_reserved in a DWARF64 file was not treated as valid.

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

815

This and the following case might want renaming slightly, since the offset is no longer at the end. Perhaps "ParserMarkedAsDoneFor..."?

[labath]

I haven't managed to get around to updating this patch today. I'll try to do it tomorrow.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

347–350

Yes, I think I can add a test for the "data running out" case.

I also believe you're right about the error message. I can add the extra string since it is convenient here, but in general I wouldn't want to subscribe to this level of detail in error messages. My goal is to make error handling to only take up a fraction of the function doing the parsing. Ideally I'd just carry the Error (or Cursor) object around, and only check it once or twice per function -- kind of like what the debug_names patch (D75119) does now. If I wanted report the exact point at which the data "ran out", I'd have to check for errors after every get() operation.

1218

Yes, I've noticed this too, and I am pretty sure there was a bug here. I'll see if I can construct some meanigful test here without creating a 4GB buffer.

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

815

will do

[jhenderson]

I had another thought about the "unexpected end of data" message: would it be possible to include the offset at which the data ended? That would still give sufficient context to the message without the need to check in lots of different places.

[labath]

In D75116#1895025, @jhenderson wrote:

I had another thought about the "unexpected end of data" message: would it be possible to include the offset at which the data ended? That would still give sufficient context to the message without the need to check in lots of different places.

Yes, that is definitely possible. In fact I was considering that myself but I wasn't sure whether this extra information would be useful.

Now that I think about, I think this is a good idea, particularly once we have truncated data extractors, as then the value will not just be the end of the section.

[labath]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

347–350

Test case added. Instead of adding the error prefix, I've created D75265.

1218

The most interesting thing to test here would be to check that the parser correctly advances to the next unit for a contribution with length=DW_LENGTH_lo_reserved. However, that is not possible, since the very next thing it does after checking totalLengthIsValid() (which would have returned false before, but returns true now) is to check that the next unit offset fits into the data extractor.

However, I have created a dwarfdump test that ensures that we can dump a prologue of an incomplete contribution with this length (my other patch has introduced a bug where we would skip dumping of prologues with these lengths. That seems to be the next best thing.

[labath]

• add more tests

[jhenderson]

LGTM, thanks!

llvm/test/tools/llvm-dwarfdump/X86/debug_line_dwarf64_large_table.s

2–4

This isn't a hard policy or anything, but I often find it more readable for comments in lit tests to use '##' instead of just '#', as it helps them stand out from lit and FileCheck directives.

17

Nit: too many blank lines here.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

 
 Error DWARFDebugLine::Prologue::parse(
     const DWARFDataExtractor &DebugLineData, uint64_t *OffsetPtr,
     function_ref<void(Error)> RecoverableErrorHandler, const DWARFContext &Ctx,
     const DWARFUnit *U) {
   const uint64_t PrologueOffset = *OffsetPtr;
 
   clear();
-  TotalLength = DebugLineData.getRelocatedValue(4, OffsetPtr);
-  if (TotalLength == dwarf::DW_LENGTH_DWARF64) {
-    FormParams.Format = dwarf::DWARF64;
-    TotalLength = DebugLineData.getU64(OffsetPtr);
-  } else if (TotalLength >= dwarf::DW_LENGTH_lo_reserved) {
-    // 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);
-  }
+  Error Err = Error::success();
+  std::tie(TotalLength, FormParams.Format) =
+      DebugLineData.getInitialLength(OffsetPtr, &Err);
+  if (Err)
+    return createStringError(
+        errc::invalid_argument,
+        "parsing line table prologue at offset 0x%8.8" PRIx64 ": %s",
+        PrologueOffset, toString(std::move(Err)).c_str());
+
   FormParams.Version = DebugLineData.getU16(OffsetPtr);
   if (!versionIsSupported(getVersion()))
     // 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 %" PRIu16,
+        ": unsupported version %" PRIu16,
         PrologueOffset, getVersion());
 
   if (getVersion() >= 5) {
     FormParams.AddrSize = DebugLineData.getU8(OffsetPtr);
     assert((DebugLineData.getAddressSize() == 0 ||
             DebugLineData.getAddressSize() == getAddressSize()) &&
            "Line table header and data extractor disagree");
     SegSelectorSize = DebugLineData.getU8(OffsetPtr);
DWARFDebugLine::SectionParser::SectionParser(DWARFDataExtractor &Data,
                                              cu_range CUs, tu_range TUs)
     : DebugLineData(Data), Context(C) {
   LineToUnit = buildLineToUnitMap(CUs, TUs);
   if (!DebugLineData.isValidOffset(Offset))
     Done = true;
 }
 
 bool DWARFDebugLine::Prologue::totalLengthIsValid() const {
-  return TotalLength == dwarf::DW_LENGTH_DWARF64 ||
-         TotalLength < dwarf::DW_LENGTH_lo_reserved;
+  return TotalLength != 0u;
 }
 
 DWARFDebugLine::LineTable DWARFDebugLine::SectionParser::parseNext(
     function_ref<void(Error)> RecoverableErrorHandler,
     function_ref<void(Error)> UnrecoverableErrorHandler, raw_ostream *OS) {
   assert(DebugLineData.isValidOffset(Offset) &&
          "parsing should have terminated");
   DWARFUnit *U = prepareToParse(Offset);

llvm/test/tools/llvm-dwarfdump/X86/debug_line_dwarf64_large_table.s

+## Test that we can dump the (intact) prologue of a large table which was
+## truncated. Also, make sure we don't get confused by a DWARF64 length which
+## matches one of the reserved initial length values.
+
+# RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux %s >%t
+# RUN: llvm-dwarfdump %t -debug-line 2>&1 | FileCheck %s
+
+# CHECK: debug_line[0x00000000]
+# CHECK-NEXT: warning: line table program with offset 0x00000000 has length 0xfffffffc but only 0x0000003a bytes are available
+# CHECK-NEXT: Line table prologue:
+# CHECK-NEXT:     total_length: 0xfffffff0
+# CHECK-NEXT:          version: 4
+# CHECK-NEXT:  prologue_length: 0x00000016
+
+# CHECK:      0x000000000badbeef      1      0      1   0             0  is_stmt end_sequence
+
+.section .debug_line,"",@progbits
+.long   0xffffffff      # Length of Unit (DWARF-64 format)
+.quad   0xfffffff0
+.short  4               # DWARF version number
+.quad   .Lprologue1_end-.Lprologue1_start # Length of Prologue
+.Lprologue1_start:
+.byte   1               # Minimum Instruction Length
+.byte   1               # Maximum Operations per Instruction
+.byte   1               # Default is_stmt
+.byte   -5              # Line Base
+.byte   14              # Line Range
+.byte   1               # Opcode Base
+.asciz "dir1"           # Include table
+.byte   0
+.asciz "file1"          # File table
+.byte   0, 0, 0
+.byte   0
+.Lprologue1_end:
+.byte   0, 9, 2         # DW_LNE_set_address
+.quad   0x0badbeef
+.byte   0, 1, 1         # DW_LNE_end_sequence

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

 # NOFIRST-NOT: debug_line[0x00000000]
 # NOFIRST-NOT: 0x000000000badbeef {{.*}} end_sequence
 # NOLATER-NOT: debug_line[{{.*}}]
 # NOLATER-NOT: end_sequence
 
 ## For fatal issues, the following table(s) should not be dumped:
 # FATAL:      debug_line[0x00000048]
 # RESERVED-NOT: prologue
-# RESERVED: warning: parsing line table prologue at offset 0x00000048 unsupported reserved unit length found of value 0xfffffffe
+# RESERVED: warning: parsing line table prologue at offset 0x00000048: unsupported reserved unit length of value 0xfffffffe
 # RESERVED-NOT: prologue
 # FATAL-NOT:  debug_line
 
 ## For non-fatal issues, the table data should be dumped:
 
 ## Version 0 table.
 # NONFATAL:      debug_line[0x00000048]
 # NONFATAL-NEXT: Line table prologue

 # NONFATAL-NEXT:          length: 0x00000003
 # NONFATAL-NOT:  file_names
 # NONFATAL:      0xababcdcdefef0909 {{.*}} is_stmt end_sequence
 
 # LAST:          debug_line[0x000003c9]
 # LAST:          0x00000000cafebabe {{.*}} end_sequence
 
 # ALL-NOT:  warning:
-# ALL:      warning: parsing line table prologue at offset 0x00000048 found unsupported version 0
-# ALL-NEXT: warning: parsing line table prologue at offset 0x0000004e found unsupported version 1
+# ALL:      warning: parsing line table prologue at offset 0x00000048: unsupported version 0
+# ALL-NEXT: warning: parsing line table prologue at offset 0x0000004e: unsupported version 1
 # ALL-NEXT: warning: parsing line table prologue at 0x00000054 found an invalid directory or file table description at 0x00000073
 # ALL-NEXT: warning: failed to parse entry content descriptions because no path was found
 # ALL-NEXT: warning: parsing line table prologue at 0x00000081 found an invalid directory or file table description at 0x000000ba
 # ALL-NEXT: warning: file names table was not null terminated before the end of the prologue
 # 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

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

if (!setupGenerator())
     return;
 
   LineTable &LT = Gen->addLineTable();
   LT.setCustomPrologue({{0, LineTable::Byte}});
 
   generate();
 
   EXPECT_THAT_EXPECTED(
+      getOrParseLineTableFatalErrors(0),
+      FailedWithMessage("parsing line table prologue at offset 0x00000000: "
+                        "unexpected end of data at offset 0x0"));
+
+  EXPECT_THAT_EXPECTED(
       getOrParseLineTableFatalErrors(1),
       FailedWithMessage(
           "offset 0x00000001 is not a valid debug line section offset"));
 }
 
 TEST_P(DebugLineParameterisedFixture, PrologueGetLength) {
   if (!setupGenerator(Version))
     return;
TEST_F(DebugLineBasicFixture, ErrorForReservedLength) {
   LineTable &LT = Gen->addLineTable();
   LT.setCustomPrologue({{0xfffffff0, LineTable::Long}});
 
   generate();
 
   EXPECT_THAT_EXPECTED(
       getOrParseLineTableFatalErrors(),
       FailedWithMessage(
-          "parsing line table prologue at offset 0x00000000 unsupported "
-          "reserved unit length found of value 0xfffffff0"));
+          "parsing line table prologue at offset 0x00000000: unsupported "
+          "reserved unit length of value 0xfffffff0"));
 }
 
 struct DebugLineUnsupportedVersionFixture : public TestWithParam<uint16_t>,
                                             public CommonFixture {
   void SetUp() { Version = GetParam(); }
 
   uint16_t Version;
 };
 
 TEST_P(DebugLineUnsupportedVersionFixture, ErrorForUnsupportedVersion) {
   if (!setupGenerator())
     return;
 
   LineTable &LT = Gen->addLineTable();
   LT.setCustomPrologue(
       {{LineTable::Half, LineTable::Long}, {Version, LineTable::Half}});
 
   generate();
 
   EXPECT_THAT_EXPECTED(
       getOrParseLineTableFatalErrors(),
-      FailedWithMessage("parsing line table prologue at offset 0x00000000 "
-                        "found unsupported version " +
+      FailedWithMessage("parsing line table prologue at offset 0x00000000: "
+                        "unsupported version " +
                         std::to_string(Version)));
 }
 
 INSTANTIATE_TEST_CASE_P(UnsupportedVersionTestParams,
                         DebugLineUnsupportedVersionFixture,
                         Values(/*1 below min */ 1, /* 1 above max */ 6,
                                /* Maximum possible */ 0xffff), );
 
if (!setupGenerator())
     return;
 
   generate();
   DWARFDebugLine::SectionParser Parser(LineData, *Context, CUs, TUs);
 
   EXPECT_TRUE(Parser.done());
 }
 
-TEST_F(DebugLineBasicFixture, ParserMovesToEndForBadLengthWhenParsing) {
+TEST_F(DebugLineBasicFixture, ParserMarkedAsDoneForBadLengthWhenParsing) {
   if (!setupGenerator())
     return;
 
   LineTable &LT = Gen->addLineTable();
   LT.setCustomPrologue({{0xfffffff0, LineTable::Long}});
   Gen->addLineTable();
   generate();
 
   DWARFDebugLine::SectionParser Parser(LineData, *Context, CUs, TUs);
   Parser.parseNext(RecordRecoverable, RecordUnrecoverable);
 
-  EXPECT_EQ(Parser.getOffset(), 4u);
+  EXPECT_EQ(Parser.getOffset(), 0u);
   EXPECT_TRUE(Parser.done());
   EXPECT_FALSE(Recoverable);
 
   EXPECT_THAT_ERROR(
       std::move(Unrecoverable),
       FailedWithMessage(
-          "parsing line table prologue at offset 0x00000000 unsupported "
-          "reserved unit length found of value 0xfffffff0"));
+          "parsing line table prologue at offset 0x00000000: unsupported "
+          "reserved unit length of value 0xfffffff0"));
 }
 
-TEST_F(DebugLineBasicFixture, ParserMovesToEndForBadLengthWhenSkipping) {
+TEST_F(DebugLineBasicFixture, ParserMarkedAsDoneForBadLengthWhenSkipping) {
   if (!setupGenerator())
     return;
 
   LineTable &LT = Gen->addLineTable();
   LT.setCustomPrologue({{0xfffffff0, LineTable::Long}});
   Gen->addLineTable();
   generate();
 
   DWARFDebugLine::SectionParser Parser(LineData, *Context, CUs, TUs);
   Parser.skip(RecordRecoverable, RecordUnrecoverable);
 
-  EXPECT_EQ(Parser.getOffset(), 4u);
+  EXPECT_EQ(Parser.getOffset(), 0u);
   EXPECT_TRUE(Parser.done());
   EXPECT_FALSE(Recoverable);
 
   EXPECT_THAT_ERROR(
       std::move(Unrecoverable),
       FailedWithMessage(
-          "parsing line table prologue at offset 0x00000000 unsupported "
-          "reserved unit length found of value 0xfffffff0"));
+          "parsing line table prologue at offset 0x00000000: unsupported "
+          "reserved unit length of value 0xfffffff0"));
 }
 
 TEST_F(DebugLineBasicFixture, ParserReportsFirstErrorInEachTableWhenParsing) {
   if (!setupGenerator())
     return;
 
   LineTable &LT = Gen->addLineTable(DWARF32);
   LT.setCustomPrologue({{2, LineTable::Long}, {0, LineTable::Half}});
   LineTable &LT2 = Gen->addLineTable(DWARF32);
   LT2.setCustomPrologue({{2, LineTable::Long}, {1, LineTable::Half}});
   generate();
 
   DWARFDebugLine::SectionParser Parser(LineData, *Context, CUs, TUs);
   Parser.parseNext(RecordRecoverable, RecordUnrecoverable);
   ASSERT_FALSE(Parser.done());
   Parser.parseNext(RecordRecoverable, RecordUnrecoverable);
 
   EXPECT_TRUE(Parser.done());
   EXPECT_THAT_ERROR(std::move(Recoverable), Succeeded());
 
   EXPECT_THAT_ERROR(
       std::move(Unrecoverable),
-      FailedWithMessage("parsing line table prologue at offset 0x00000000 "
-                        "found unsupported version 0",
-                        "parsing line table prologue at offset 0x00000006 "
-                        "found unsupported version 1"));
+      FailedWithMessage("parsing line table prologue at offset 0x00000000: "
+                        "unsupported version 0",
+                        "parsing line table prologue at offset 0x00000006: "
+                        "unsupported version 1"));
 }
 
 TEST_F(DebugLineBasicFixture, ParserReportsNonPrologueProblemsWhenParsing) {
   if (!setupGenerator())
     return;
 
   LineTable &LT = Gen->addLineTable(DWARF32);
   LT.addExtendedOpcode(0x42, DW_LNE_end_sequence, {});
TEST_F(DebugLineBasicFixture,
   ASSERT_FALSE(Parser.done());
   Parser.skip(RecordRecoverable, RecordUnrecoverable);
 
   EXPECT_TRUE(Parser.done());
   EXPECT_FALSE(Recoverable);
 
   EXPECT_THAT_ERROR(
       std::move(Unrecoverable),
-      FailedWithMessage("parsing line table prologue at offset 0x00000000 "
-                        "found unsupported version 0",
-                        "parsing line table prologue at offset 0x00000006 "
-                        "found unsupported version 1"));
+      FailedWithMessage("parsing line table prologue at offset 0x00000000: "
+                        "unsupported version 0",
+                        "parsing line table prologue at offset 0x00000006: "
+                        "unsupported version 1"));
 }
 
 TEST_F(DebugLineBasicFixture, ParserIgnoresNonPrologueErrorsWhenSkipping) {
   if (!setupGenerator())
     return;
 
   LineTable &LT = Gen->addLineTable(DWARF32);
   LT.addExtendedOpcode(42, DW_LNE_end_sequence, {});