[DebugInfo] Error if unsupported address size detected in line table

[jhenderson]

Summary

Prior to this patch, if a DW_LNE_set_address opcode was parsed with an address size (i.e. with a length after the opcode) of anything other 1, 2, 4, or 8, an llvm_unreachable would be hit, as the data extractor does not support other values. This patch introduces a new error check that verifies the address size is one of the supported sizes, in common with other places within the DWARF parsing.

This patch also fixes calculation of a generated line table's size in unit tests. One of the tests in this patch highlighted a bug introduced in 1271cde4745, when non-byte operands were used as arguments for extended or standard opcodes.

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

668–670

I'm going to write a follow-up patch for this issue.

[ikudrin]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

668–670

You probably meant Len >= 1, right?

[jhenderson]

Address FIXME by using uint64_t for longer. If the size is greater than uint8_t max, then it is clearly not 4 or 8, so the adddress size check will catch it. Also added a unit test to cover this case, and made a minor change to a struct used in the test to ensure the test could pass.

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

668–670

I'm guessing this comment is related to the now-deleted FIXME?

[probinson]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

680

I just happened to notice D73961 which says AVR usually has 2-byte addresses. Maybe that's a broader issue for a separate patch but there are a lot of 16-bit machines still out there in the world.

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

677–698

I'd generally like to assume that the /first/ time a value/field/length/etc is seen, that is correct and anything later that conflicts with it is incorrect.

In what cases would we reach here anyway? Could we check earlier and ensure the extractor has a meaningful (non-zero) address size?

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

680

I just happened to notice D73961 which says AVR usually has 2-byte addresses. Maybe that's a broader issue for a separate patch but there are a lot of 16-bit machines still out there in the world.

Thanks for flagging that up. I saw the 4/8 byte support check in several other places, so copied those. I'll expand this to 2 and 1 too, since that's what the DataExtractor can support. The other places can be fixed as needed.

I'd generally like to assume that the /first/ time a value/field/length/etc is seen, that is correct and anything later that conflicts with it is incorrect.

I've been following the approach of assuming the length field is correct (in this case, the length of the extended opcode). Indeed, in this case, if we were to follow a different approach, we could read past the end of the instruction.

In what cases would we reach here anyway?

We hit here any time the length field of a DW_LNE_set_address opcode is not either 5 or 9 (i.e. 1 for the opcode, plus 4/8 for the address).

Could we check earlier and ensure the extractor has a meaningful (non-zero) address size?

I'm deliberately not checking that any earlier stated address makes sense, because it's not actually needed until here. If a V5 line table had an address of, say, 7, it wouldn't matter if the table had no DW_LNE_set_address opcodes, so an earlier validation would lead to an error that has no impact. @probinson (I think) previously encouraged me to be lazy in reporting this sort of thing.

Additionally, we don't always have any address size information until this point - in the case where e.g. llvm-dwarfdump is dumping just the whole of a V4 .debug_line section and there is no corresponding debug info unit, the parser knows nothing about the address until it hits this point.

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

677–698

(oops, Phabricator is showing my reply above your one @dblaikie)

[jhenderson]

Address review comments:

1. Add support for 1-byte and 2-byte address sizes.
2. Fold in unit test bug fix from D73901.

[aykevl]

I just noticed this patch from D73961. I am not yet familiar with the DWARF support in LLVM but just wanted to say two things:

• The address size (2) that LLVM uses for AVR might be wrong: avr-gcc uses 4 instead. I don't know why exactly but perhaps because there are AVR devices that have more than 64K words of flash (such as the ATmega2560).
• So far it looks like code compiled with Clang has faulty debug info. dwarfdump complains ERROR: dwarf_srcfiles: DW_DLE_HEADER_LEN_BIGGER_THAN_SECSIZE (342) Corrupt dwarf. while llvm-dwarfdump doesn't even try to print the contents of .debug-info.

To reproduce, code:

int main(void)
{
  while (1) {
    __asm__ __volatile__("nop");
  }
}

Compiled with avr-gcc (the -gdwarf-4 is important because avr-gcc on Debian defaults to stabs for some reason):

avr-gcc -o avr-nop.elf -g -gdwarf-4 avr-nop.c

Compiled with Clang:

clang --target=avr -c -o avr-nop.o -Os -g avr-nop.c
avr-gcc -o avr-nop.elf avr-nop.o

Noting it here in case it is of any use, or you happen to have any hints how I could debug this further (I've already spent hours on this).

[jhenderson]

In D73962#1860331, @aykevl wrote:

I just noticed this patch from D73961. I am not yet familiar with the DWARF support in LLVM but just wanted to say two things:

• The address size (2) that LLVM uses for AVR might be wrong: avr-gcc uses 4 instead. I don't know why exactly but perhaps because there are AVR devices that have more than 64K words of flash (such as the ATmega2560).
• So far it looks like code compiled with Clang has faulty debug info. dwarfdump complains ERROR: dwarf_srcfiles: DW_DLE_HEADER_LEN_BIGGER_THAN_SECSIZE (342) Corrupt dwarf. while llvm-dwarfdump doesn't even try to print the contents of .debug-info.

Noting it here in case it is of any use, or you happen to have any hints how I could debug this further (I've already spent hours on this).

Hi @aykevl,

It might be worth filing a bug against LLVM to report and discuss this further. Also, if you are able to attach the object file to that bug which you are having trouble with, it might be helpful for debugging purposes.

[aykevl]

@jhenderson thanks!
It took me a while but finally I figured out that the problem is with the AVR backend - at least the first bug I was dealing with. For some reason it adjusted absolute relocations, breaking references between DWARF sections.

[jhenderson]

Ping?

[jhenderson]

Fix unit test following rebase.

[dblaikie]

Sounds alright

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

662

A comment on the "addExtendedOpcode" might help make it clearer that that's the "interesting" part of the input & how half+byte == 3 bytes, which is the invalid value being tested for on this line? (what's the "addByte(0xaa)" for? that looks a bit opaque)

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

if (Opcode == 0) {
         // 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.
         {
           uint8_t ExtractorAddressSize = DebugLineData.getAddressSize();
-          if (ExtractorAddressSize != Len - 1 && ExtractorAddressSize != 0)
+          uint64_t OpcodeAddressSize = Len - 1;
+          if (ExtractorAddressSize != OpcodeAddressSize &&
+              ExtractorAddressSize != 0)
             RecoverableErrorHandler(createStringError(
                 errc::invalid_argument,
                 "mismatching address size at offset 0x%8.8" PRIx64
                 " expected 0x%2.2" PRIx8 " found 0x%2.2" PRIx64,
                 ExtOffset, ExtractorAddressSize, Len - 1));
 
           // Assume that the line table is correct and temporarily override the
-          // address size.
-          DebugLineData.setAddressSize(Len - 1);
+          // address size. If the size is unsupported, give up trying to read
+          // the address and continue to the next opcode.
+          if (OpcodeAddressSize != 1 && OpcodeAddressSize != 2 &&
+              OpcodeAddressSize != 4 && OpcodeAddressSize != 8) {
+            RecoverableErrorHandler(createStringError(
+                errc::invalid_argument,
+                "address size 0x%2.2" PRIx64
+                " of DW_LNE_set_address opcode at offset 0x%8.8" PRIx64
+                " is unsupported",
+                OpcodeAddressSize, ExtOffset));
+            *OffsetPtr += OpcodeAddressSize;
+          } else {
+            DebugLineData.setAddressSize(OpcodeAddressSize);
             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

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

checkError(
       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,
+       ErrorForUnsupportedAddressSizeInSetAddressLength) {
+  // Use DWARF v4, and 0 for data extractor address size so that the address
+  // size is derived from the opcode length.
+  if (!setupGenerator(4, 0))
+    return;
+
+  LineTable &LT = Gen->addLineTable();
+  // 4 == length of the extended opcode, i.e. 1 for the opcode itself and 3 for
+  // the Half (2) + Byte (1) operand, representing the unsupported address size.
+  LT.addExtendedOpcode(4, DW_LNE_set_address,
+                       {{0x1234, LineTable::Half}, {0x56, LineTable::Byte}});
+  LT.addStandardOpcode(DW_LNS_copy, {});
+  // Special opcode to ensure the address has changed between the first and last
+  // row in the sequence. Without this, the sequence will not be recorded.
+  LT.addByte(0xaa);
+  LT.addExtendedOpcode(1, DW_LNE_end_sequence, {});
+
+  generate();
+
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+                                                    nullptr, RecordRecoverable);
+  checkError(
+      "address size 0x03 of DW_LNE_set_address opcode at offset 0x00000030 is "
+      "unsupported",
+      std::move(Recoverable));
+  ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+  ASSERT_EQ((*ExpectedLineTable)->Rows.size(), 3u);
+  EXPECT_EQ((*ExpectedLineTable)->Sequences.size(), 1u);
+  // Show that the set address opcode is ignored in this case.
+  EXPECT_EQ((*ExpectedLineTable)->Rows[0].Address.Address, 0);
+}
+
+TEST_F(DebugLineBasicFixture, ErrorForAddressSizeGreaterThanByteSize) {
+  // Use DWARF v4, and 0 for data extractor address size so that the address
+  // size is derived from the opcode length.
+  if (!setupGenerator(4, 0))
+    return;
+
+  LineTable &LT = Gen->addLineTable();
+  // Specifically use an operand size that has a trailing byte of a supported
+  // size (8), so that any potential truncation would result in a valid size.
+  std::vector<LineTable::ValueAndLength> Operands(0x108);
+  LT.addExtendedOpcode(Operands.size() + 1, DW_LNE_set_address, Operands);
+  LT.addExtendedOpcode(1, DW_LNE_end_sequence, {});
+
+  generate();
+
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+                                                    nullptr, RecordRecoverable);
+  checkError(
+      "address size 0x108 of DW_LNE_set_address opcode at offset 0x00000031 is "
+      "unsupported",
+      std::move(Recoverable));
+  ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+}
+
+TEST_F(DebugLineBasicFixture, ErrorForUnsupportedAddressSizeDefinedInHeader) {
+  // Use 0 for data extractor address size so that it does not clash with the
+  // header address size.
+  if (!setupGenerator(5, 0))
+    return;
+
+  LineTable &LT = Gen->addLineTable();
+  // AddressSize + 1 == length of the extended opcode, i.e. 1 for the opcode
+  // itself and 9 for the Quad (8) + Byte (1) operand representing the
+  // unsupported address size.
+  uint8_t AddressSize = 9;
+  LT.addExtendedOpcode(AddressSize + 1, DW_LNE_set_address,
+                       {{0x12345678, LineTable::Quad}, {0, LineTable::Byte}});
+  LT.addStandardOpcode(DW_LNS_copy, {});
+  // Special opcode to ensure the address has changed between the first and last
+  // row in the sequence. Without this, the sequence will not be recorded.
+  LT.addByte(0xaa);
+  LT.addExtendedOpcode(1, DW_LNE_end_sequence, {});
+  DWARFDebugLine::Prologue Prologue = LT.createBasicPrologue();
+  Prologue.FormParams.AddrSize = AddressSize;
+  LT.setPrologue(Prologue);
+
+  generate();
+
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+                                                    nullptr, RecordRecoverable);
+  checkError(
+      "address size 0x09 of DW_LNE_set_address opcode at offset 0x00000038 is "
+      "unsupported",
+      std::move(Recoverable));
+  ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+  ASSERT_EQ((*ExpectedLineTable)->Rows.size(), 3u);
+  EXPECT_EQ((*ExpectedLineTable)->Sequences.size(), 1u);
+  // Show that the set address opcode is ignored in this case.
+  EXPECT_EQ((*ExpectedLineTable)->Rows[0].Address.Address, 0);
+}
+
 TEST_F(DebugLineBasicFixture, CallbackUsedForUnterminatedSequence) {
   if (!setupGenerator())
     return;
 
   LineTable &LT = Gen->addLineTable();
   LT.addExtendedOpcode(9, DW_LNE_set_address,
                        {{0x1122334455667788, LineTable::Quad}});
   LT.addStandardOpcode(DW_LNS_copy, {});

llvm/unittests/DebugInfo/DWARF/DwarfGenerator.h

 /// A DWARF line unit-like class used to generate DWARF line units.
 ///
 /// Instances of this class are created by instances of the Generator class.
 class LineTable {
 public:
   enum ValueLength { Byte = 1, Half = 2, Long = 4, Quad = 8, ULEB, SLEB };
 
   struct ValueAndLength {
-    uint64_t Value;
-    ValueLength Length;
+    uint64_t Value = 0;
+    ValueLength Length = Byte;
   };
 
   LineTable(uint16_t Version, dwarf::DwarfFormat Format, uint8_t AddrSize,
             uint8_t SegSize = 0)
       : Version(Version), Format(Format), AddrSize(AddrSize), SegSize(SegSize) {
     assert(Version >= 2 && Version <= 5 && "unsupported version");
   }
 

 private:
   void writeData(ArrayRef<ValueAndLength> Data, AsmPrinter &Asm) const;
   MCSymbol *writeDefaultPrologue(AsmPrinter &Asm) const;
   void writePrologue(AsmPrinter &Asm) const;
 
   void writeProloguePayload(const DWARFDebugLine::Prologue &Prologue,
                             AsmPrinter &Asm) const;
 
+  // Calculate the number of bytes the Contents will take up.
+  size_t getContentsSize() const;
+
   llvm::Optional<DWARFDebugLine::Prologue> Prologue;
   std::vector<ValueAndLength> CustomPrologue;
   std::vector<ValueAndLength> Contents;
 
   // The Version field is used for determining how to write the Prologue, if a
   // non-custom prologue is used. The version value actually written, will be
   // that specified in the Prologue, if a custom prologue has been passed in.
   // Otherwise, it will be this value.

llvm/unittests/DebugInfo/DWARF/DwarfGenerator.cpp

 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCObjectFileInfo.h"
 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/MCTargetOptionsCommandFlags.inc"
 #include "llvm/PassAnalysisSupport.h"
+#include "llvm/Support/LEB128.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetLoweringObjectFile.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Target/TargetOptions.h"
 
 using namespace llvm;
 using namespace dwarf;
case 5:
     break;
   default:
     llvm_unreachable("unsupported version");
   }
   if (Format == DWARF64) {
     P.TotalLength += 4;
     P.FormParams.Format = DWARF64;
   }
-  P.TotalLength += Contents.size();
+  P.TotalLength += getContentsSize();
   P.FormParams.Version = Version;
   P.MinInstLength = 1;
   P.MaxOpsPerInst = 1;
   P.DefaultIsStmt = 1;
   P.LineBase = -5;
   P.LineRange = 14;
   P.OpcodeBase = 13;
   P.StandardOpcodeLengths = {0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1};
for (auto Entry : Data) {
     case SLEB:
       Asm.emitSLEB128(Entry.Value);
       continue;
     }
     llvm_unreachable("unsupported ValueAndLength Length value");
   }
 }
 
+size_t dwarfgen::LineTable::getContentsSize() const {
+  size_t Size = 0;
+  for (auto Entry : Contents) {
+    switch (Entry.Length) {
+    case ULEB:
+      Size += getULEB128Size(Entry.Value);
+      break;
+    case SLEB:
+      Size += getSLEB128Size(Entry.Value);
+      break;
+    default:
+      Size += Entry.Length;
+      break;
+    }
+  }
+  return Size;
+}
+
 MCSymbol *dwarfgen::LineTable::writeDefaultPrologue(AsmPrinter &Asm) const {
   MCSymbol *UnitStart = Asm.createTempSymbol("line_unit_start");
   MCSymbol *UnitEnd = Asm.createTempSymbol("line_unit_end");
   if (Format == DwarfFormat::DWARF64) {
     Asm.emitInt32((int)dwarf::DW_LENGTH_DWARF64);
     Asm.emitLabelDifference(UnitEnd, UnitStart, 8);
   } else {
     Asm.emitLabelDifference(UnitEnd, UnitStart, 4);