[DebugInfo] Make debug line address size mismatch non-fatal to parsing

[jhenderson]

Summary

Reasonable assumptions can be made when a parsed address length does not match the expected length, so there's no need for this to be fatal.

[jhenderson]

Early ping - I'd like to get this and the other related reviews in before the release branch is created if possible.

[ikudrin]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

9–1

I find these lines a bit hard to follow, without prior understanding of the whole idea. I would suggest reorganize them like this:

uint8_t ExtractorAddressSize = DebugLineData.getAddressSize();
if (ExtractorAddressSize != Len - 1) {
  RecoverableErrorCallback(...);
}
// Assuming that the line table is correct, temporarily override the address size.
DebugLineData.setAddressSize(Len - 1);
State.Row.Address.Address = ...
// Restore the address size if the extractor already had it.
if (ExtractorAddressSize != 0)
  DebugLineData.setAddressSize(ExtractorAddressSize);

What do you think?

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

9–1

Yes, I think that should work, with one minor change to the error check - a value of zero from the extractor shouldn't cause an error. I'll try that.

[jhenderson]

Address review comment.

[jhenderson]

Rebase.

[ikudrin]

LGTM. Sorry for the delay.

[jhenderson]

In D72154#1817101, @ikudrin wrote:

LGTM. Sorry for the delay.

No problem. Thanks for the review!

[ikudrin]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

621

@jhenderson, it looks like that an incorrect Len value, i. e. not 1, 2, 4 or 8, eventually leads to a crash in DataExtractor::getUnsigned(). Please add a patch to check that case.

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

621

Will do!

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

621

Patch put up for review in D73962.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

 //===- DWARFDebugLine.cpp -------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/DebugInfo/DWARF/DWARFDebugLine.h"
if (Opcode == 0) {
         // operand is the size appropriate to hold an address on the target
         // machine. Set the address register to the value given by the
         // relocatable address. All of the other statement program opcodes
         // that affect the address register add a delta to it. This instruction
         // stores a relocatable value into it instead.
         //
         // Make sure the extractor knows the address size.  If not, infer it
         // from the size of the operand.
-        if (DebugLineData.getAddressSize() == 0)
-          DebugLineData.setAddressSize(Len - 1);
-        else if (DebugLineData.getAddressSize() != Len - 1) {
-          return createStringError(errc::invalid_argument,
-                             "mismatching address size at offset 0x%8.8" PRIx64
-                             " expected 0x%2.2" PRIx8 " found 0x%2.2" PRIx64,
-                             ExtOffset, DebugLineData.getAddressSize(),
-                             Len - 1);
-        }
+        {
+          uint8_t ExtractorAddressSize = DebugLineData.getAddressSize();
+          if (ExtractorAddressSize != Len - 1 && ExtractorAddressSize != 0)
+            RecoverableErrorCallback(createStringError(
+                errc::invalid_argument,
+                ExtOffset, ExtractorAddressSize, Len - 1));
+
+          // Assume that the line table is correct and temporarily override the
+          // address size.
+          DebugLineData.setAddressSize(Len - 1);
           State.Row.Address.Address = DebugLineData.getRelocatedAddress(
               OffsetPtr, &State.Row.Address.SectionIndex);
+
+          // Restore the address size if the extractor already had it.
+          if (ExtractorAddressSize != 0)
+            DebugLineData.setAddressSize(ExtractorAddressSize);
+
           if (OS)
             *OS << format(" (0x%16.16" PRIx64 ")", State.Row.Address.Address);
+        }
         break;
 
       case DW_LNE_define_file:
         // Takes 4 arguments. The first is a null terminated string containing
         // a source file name. The second is an unsigned LEB128 number
         // representing the directory index of the directory in which the file
         // was found. The third is an unsigned LEB128 number representing the
         // time of last modification of the file. The fourth is an unsigned

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

CommonFixture()
         RecordUnrecoverable(std::bind(&CommonFixture::recordUnrecoverable, this,
                                       std::placeholders::_1)){};
 
   ~CommonFixture() {
     EXPECT_FALSE(Recoverable);
     EXPECT_FALSE(Unrecoverable);
   }
 
-  bool setupGenerator(uint16_t Version = 4) {
-    Triple T = getDefaultTargetTripleForAddrSize(8);
+  bool setupGenerator(uint16_t Version = 4, uint8_t AddrSize = 8) {
+    AddressSize = AddrSize;
+    Triple T =
+        getDefaultTargetTripleForAddrSize(AddressSize == 0 ? 8 : AddressSize);
     if (!isConfigurationSupported(T))
       return false;
     auto ExpectedGenerator = Generator::create(T, Version);
     if (ExpectedGenerator)
       Gen.reset(ExpectedGenerator->release());
     return true;
   }
 
   void generate() {
     Context = createContext();
     assert(Context != nullptr && "test state is not valid");
     const DWARFObject &Obj = Context->getDWARFObj();
+    uint8_t TargetAddrSize = AddressSize == 0 ? 8 : AddressSize;
     LineData = DWARFDataExtractor(
         Obj, Obj.getLineSection(),
-        getDefaultTargetTripleForAddrSize(8).isLittleEndian(), 8);
+        getDefaultTargetTripleForAddrSize(TargetAddrSize).isLittleEndian(),
+        AddressSize);
   }
 
   std::unique_ptr<DWARFContext> createContext() {
     if (!Gen)
       return nullptr;
     StringRef FileBytes = Gen->generate();
     MemoryBufferRef FileBuffer(FileBytes, "dwarf");
     auto Obj = object::ObjectFile::createObjectFile(FileBuffer);
void checkGetOrParseLineTableEmitsFatalError(ArrayRef<StringRef> ExpectedMsgs,
     auto ExpectedLineTable = Line.getOrParseLineTable(
         LineData, Offset, *Context, nullptr, RecordRecoverable);
     EXPECT_FALSE(ExpectedLineTable);
     EXPECT_FALSE(Recoverable);
 
     checkError(ExpectedMsgs, ExpectedLineTable.takeError());
   }
 
+  uint8_t AddressSize;
   std::unique_ptr<Generator> Gen;
   std::unique_ptr<DWARFContext> Context;
   DWARFDataExtractor LineData;
   DWARFDebugLine Line;
   Error Recoverable;
   std::function<void(Error)> RecordRecoverable;
   Error Unrecoverable;
   std::function<void(Error)> RecordUnrecoverable;
checkError("line table program with offset 0x00000001 has length 0x00000034 "
              "but only 0x00000033 bytes are available",
              std::move(Recoverable));
   ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
   EXPECT_EQ((*ExpectedLineTable)->Rows.size(), 2u);
   EXPECT_EQ((*ExpectedLineTable)->Sequences.size(), 1u);
 }
 
 TEST_F(DebugLineBasicFixture, ErrorForMismatchedAddressSize) {
-  if (!setupGenerator())
+  if (!setupGenerator(4, 8))
     return;
 
   LineTable &LT = Gen->addLineTable();
   // The line data extractor expects size 8 (Quad) addresses.
-  LT.addExtendedOpcode(5, DW_LNE_set_address, {{0x11223344, LineTable::Long}});
+  uint64_t Addr1 = 0x11223344;
+  LT.addExtendedOpcode(5, DW_LNE_set_address, {{Addr1, LineTable::Long}});
   LT.addStandardOpcode(DW_LNS_copy, {});
-  LT.addByte(0xaa);
+  // Show that the expected address size is unchanged, so later valid lines
+  // don't cause a problem.
+  uint64_t Addr2 = 0x1122334455667788;
+  LT.addExtendedOpcode(9, DW_LNE_set_address, {{Addr2, LineTable::Quad}});
   LT.addExtendedOpcode(1, DW_LNE_end_sequence, {});
 
   generate();
 
-  checkGetOrParseLineTableEmitsFatalError(
-      "mismatching address size at offset 0x00000030 expected 0x08 found 0x04");
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+                                                    nullptr, RecordRecoverable);
+  checkError(
+      "mismatching address size at offset 0x00000030 expected 0x08 found 0x04",
+      std::move(Recoverable));
+  ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+  ASSERT_EQ((*ExpectedLineTable)->Rows.size(), 2u);
+  EXPECT_EQ((*ExpectedLineTable)->Sequences.size(), 1u);
+  EXPECT_EQ((*ExpectedLineTable)->Rows[0].Address.Address, Addr1);
+  EXPECT_EQ((*ExpectedLineTable)->Rows[1].Address.Address, Addr2);
+}
+
+TEST_F(DebugLineBasicFixture,
+       ErrorForMismatchedAddressSizeUnsetInitialAddress) {
+  if (!setupGenerator(4, 0))
+    return;
+
+  LineTable &LT = Gen->addLineTable();
+  uint64_t Addr1 = 0x11223344;
+  LT.addExtendedOpcode(5, DW_LNE_set_address, {{Addr1, LineTable::Long}});
+  LT.addStandardOpcode(DW_LNS_copy, {});
+  uint64_t Addr2 = 0x1122334455667788;
+  LT.addExtendedOpcode(9, DW_LNE_set_address, {{Addr2, LineTable::Quad}});
+  LT.addExtendedOpcode(1, DW_LNE_end_sequence, {});
+
+  generate();
+
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+                                                    nullptr, RecordRecoverable);
+  checkError(
+      "mismatching address size at offset 0x00000038 expected 0x04 found 0x08",
+      std::move(Recoverable));
+  ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+  ASSERT_EQ((*ExpectedLineTable)->Rows.size(), 2u);
+  EXPECT_EQ((*ExpectedLineTable)->Sequences.size(), 1u);
+  EXPECT_EQ((*ExpectedLineTable)->Rows[0].Address.Address, Addr1);
+  EXPECT_EQ((*ExpectedLineTable)->Rows[1].Address.Address, Addr2);
 }
 
 TEST_F(DebugLineBasicFixture, CallbackUsedForUnterminatedSequence) {
   if (!setupGenerator())
     return;
 
   LineTable &LT = Gen->addLineTable();
   LT.addExtendedOpcode(9, DW_LNE_set_address,