[DWARF] Fix the reserved values for unit length in DWARFDebugLine.

[ikudrin]

Summary

The DWARF3 documentation had inconsistency concerning the reserved range for unit length values. The issue was fixed in DWARF4.
See http://lists.dwarfstd.org/pipermail/dwarf-discuss-dwarfstd.org/2008-June/003343.html

[probinson]

This magic number appears in many places; it should have a symbolic name in llvm/include/llvm/BinaryFormat/Dwarf.h, in the LLVMConstants enumeration.

[ikudrin]

Right, but maybe that worths another patch?

[probinson]

In D64622#1583163, @ikudrin wrote:

Right, but maybe that worths another patch?

A quick grep suggests there are not very many more places that you would have to change. You can limit this patch to DebugInfo/DWARF if you like; I expect there are more places in CodeGen/AsmPrinter, which could be done separately.

[ikudrin]

I mean, the purposes for the patches are different. This one fixes an existing flaw while your suggestion improves code quality. It is usually better not to intermix such different aims in one patch, no?

[probinson]

In D64622#1584208, @ikudrin wrote:

I mean, the purposes for the patches are different. This one fixes an existing flaw while your suggestion improves code quality. It is usually better not to intermix such different aims in one patch, no?

There is no hard-and-fast rule about this; the project does prefer incremental changes, but the determination of what "size" increment to perform is not strictly defined. Personally I would do it all at once, with the stated purpose being to clean up the use of magic numbers (some of which happen to be incorrect). If you want to work in smaller increments, that's okay too.

I see that there are no tests verifying that DWARFDebugLine handles the invalid length correctly (assuming you ran the tests and none failed), so please add one.

[ikudrin]

In D64622#1585577, @probinson wrote:

I see that there are no tests verifying that DWARFDebugLine handles the invalid length correctly (assuming you ran the tests and none failed), so please add one.

There were three tests in unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp which failed, so they are also updated in this patch. Or do you mean something different?

[probinson]

If you found the unittests cases because they failed, that's good enough for me.
LGTM

[ikudrin]

Thanks! I'll prepare a patch to symbolize these magic numbers a bit later.

llvm/trunk/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

Error DWARFDebugLine::Prologue::parse(const DWARFDataExtractor &DebugLineData,
                                       const DWARFUnit *U) {
   const uint64_t PrologueOffset = *OffsetPtr;
 
   clear();
   TotalLength = DebugLineData.getU32(OffsetPtr);
   if (TotalLength == UINT32_MAX) {
     FormParams.Format = dwarf::DWARF64;
     TotalLength = DebugLineData.getU64(OffsetPtr);
-  } else if (TotalLength >= 0xffffff00) {
+  } else if (TotalLength >= 0xfffffff0) {
     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)
     return createStringError(errc::not_supported,
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 == 0xffffffff || TotalLength < 0xffffff00;
+  return TotalLength == 0xffffffff || TotalLength < 0xfffffff0;
 }
 
 DWARFDebugLine::LineTable DWARFDebugLine::SectionParser::parseNext(
     function_ref<void(Error)> RecoverableErrorCallback,
     function_ref<void(Error)> UnrecoverableErrorCallback, raw_ostream *OS) {
   assert(DebugLineData.isValidOffset(Offset) &&
          "parsing should have terminated");
   DWARFUnit *U = prepareToParse(Offset);

llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

TEST_P(DebugLineParameterisedFixture, GetOrParseLineTableValidTable) {
   // correctly.
 }
 
 TEST_F(DebugLineBasicFixture, ErrorForReservedLength) {
   if (!setupGenerator())
     return;
 
   LineTable &LT = Gen->addLineTable();
-  LT.setCustomPrologue({{0xffffff00, LineTable::Long}});
+  LT.setCustomPrologue({{0xfffffff0, LineTable::Long}});
 
   generate();
 
   checkGetOrParseLineTableEmitsError(
       "parsing line table prologue at offset 0x00000000 unsupported reserved "
-      "unit length found of value 0xffffff00");
+      "unit length found of value 0xfffffff0");
 }
 
 TEST_F(DebugLineBasicFixture, ErrorForLowVersion) {
   if (!setupGenerator())
     return;
 
   LineTable &LT = Gen->addLineTable();
   LT.setCustomPrologue(
TEST_F(DebugLineBasicFixture, ParserAlwaysDoneForEmptySection) {
   EXPECT_TRUE(Parser.done());
 }
 
 TEST_F(DebugLineBasicFixture, ParserMovesToEndForBadLengthWhenParsing) {
   if (!setupGenerator())
     return;
 
   LineTable &LT = Gen->addLineTable();
-  LT.setCustomPrologue({{0xffffff00, LineTable::Long}});
+  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_TRUE(Parser.done());
   EXPECT_FALSE(Recoverable);
 
   checkError("parsing line table prologue at offset 0x00000000 unsupported "
-             "reserved unit length found of value 0xffffff00",
+             "reserved unit length found of value 0xfffffff0",
              std::move(Unrecoverable));
 }
 
 TEST_F(DebugLineBasicFixture, ParserMovesToEndForBadLengthWhenSkipping) {
   if (!setupGenerator())
     return;
 
   LineTable &LT = Gen->addLineTable();
-  LT.setCustomPrologue({{0xffffff00, LineTable::Long}});
+  LT.setCustomPrologue({{0xfffffff0, LineTable::Long}});
   Gen->addLineTable();
   generate();
 
   DWARFDebugLine::SectionParser Parser(LineData, *Context, CUs, TUs);
   Parser.skip(RecordUnrecoverable);
 
   EXPECT_EQ(Parser.getOffset(), 4u);
   EXPECT_TRUE(Parser.done());
 
   checkError("parsing line table prologue at offset 0x00000000 unsupported "
-             "reserved unit length found of value 0xffffff00",
+             "reserved unit length found of value 0xfffffff0",
              std::move(Unrecoverable));
 }
 
 TEST_F(DebugLineBasicFixture, ParserReportsFirstErrorInEachTableWhenParsing) {
   if (!setupGenerator())
     return;
 
   LineTable &LT = Gen->addLineTable(DWARF32);