[DebugInfo] Report unsupported maximum_operations_per_instruction values

[jhenderson]

Summary

This patch adds a check which reports an unsupported value of the maximum_operations_per_instruction field in a debug line table header. This is reported once per line table, at most, and only if the table would otherwise need to use it (i.e. never for tables with version 3 or less, or for tables which don't use DW_LNS_const_add_pc or special opcodes). Unsupported values are currently any apart from 1.

See also D43470, which takes a similar approach for bad line_range values (i.e. 0). I may refactor the pair of them to avoid the code duplication.

Depends on D75188.

[jhenderson]

There are a couple of issues in this change, and I also want to refactor it and D43470 to share some code.

[probinson]

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

9

Should not have moved this; it's not the header for this module.

[jhenderson]

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

9

Weird. I don't know how that happened. I certainly wouldn't have deliberately moved it.

[jhenderson]

Rewrite to allow for other similar checks to be added in subsequent changes. See D43470 and another patch TBD. See the full patch series for why the test unit test is written the way it is (the other two patches reuse the fixture).

[ikudrin]

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

766

clang-9 shows a warning here:

DWARFDebugLineTest.cpp:752:8: warning: '(anonymous namespace)::AdjustAddressFixtureBase' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]

942

Yet another warning:

DWARFDebugLineTest.cpp:928:8: warning: 'SetUp' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]

The similar warnings are also shown for SetUp methods in the dependent patches.

[jhenderson]

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

942

Does this SetUp warning also apply for the DebugLineParameterisedFixture or DebugLineUnsupportedVersionFixture classes? They aren't new, but are missing overrides on their SetUp functions.

[ikudrin]

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

942

Nope. The warning appears if some methods in a class are missing override.

[jhenderson]

Rebase and move some of the parameters to the ParsingState class to reduce the number of arguments required in the functions. Also fix warnings mentioned by @ikudrin.

[probinson]

DWARFDebugLine.h has a pre-merge clang-format note, did you miss that?

[jhenderson]

In D74819#1903147, @probinson wrote:

DWARFDebugLine.h has a pre-merge clang-format note, did you miss that?

Oops, my bad (a result of a bad rebase/merge that was). I'll fix it.

[jhenderson]

Fix formatting.

[probinson]

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

374 ↗

(On Diff #248136)

As a future API simplification, think about using dwarf::LNStandardName() to compute the names as needed, instead of passing around strings. LNStandardName() would need an opcode_base parameter to know which ones are special for a particular line-number program, but it's used in only one place that I can find, so that should be an easy update.
I know there's the weird case of DW_LNS_const_add_pc where you're basing the advance on 255 but want to use the real opcode for reporting; but the weird cases are why we pay you the big bucks.

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

9

Did it move again? If you ran clang-format, I wonder if there's a bug in it (it might think it's moving the "module's" header to the top, but have incorrect module-header detection).

[jhenderson]

Fix formatting again. I blame Visual Studio...!

[jhenderson]

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

374 ↗

(On Diff #248136)

I had a play around with this, and LNStandardName isnt't suitable for passing in opcode_base with directly, as it needs to share the interface with the equivalent functions for other DWARF enum types. I can workaround this with a little extra logic though, I think.

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

9

Weird...

[jhenderson]

Redo name handling to remove the need for specifying it explicitly.

[probinson]

Thanks for simplifying the API. One more bit of code tidying and I think I will be happy.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

573

Sink this under the if as you don't need the name unless you're reporting the warning.

587

After sinking the getOpcodeName call, you have a block under the if instead of just a single statement, in which case you might as well move ReportAdvanceAddrProblem = false into that block as well.
These moves help distinguish the error-reporting stuff from the functional stuff as well.

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

573

It's used in the second error added in D75189, so I don't think sinking it inside the if makes much sense.

587

Same response as above. Given the additional error message in D75189, I can't put it inside the if.

[jhenderson]

Fix formatting.

[probinson]

LGTM

[MaskRay]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

507

struct can be omitted (and conventional in C++).

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

942

public can be omitted for struct.

[jhenderson]

Get rid of some unnecessary public usage.

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

507

I took a look at this, but LineTable is both a member name and struct name here. We'd have to explicitly qualify the type name to be DWARFDebugLine::LineTable. There are several similar maybe unnecessary struct specifiers in the header too, but these are all because various members have the same name as their type. I'd recommend leaving this until the variable case changes are made in this code, after which there should be no ambiguity.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

 void DWARFDebugLine::LineTable::clear() {
   Prologue.clear();
   Rows.clear();
   Sequences.clear();
 }
 
 DWARFDebugLine::ParsingState::ParsingState(struct LineTable *LT)
     : LineTable(LT) {
   resetRowAndSequence();
 }
 
 void DWARFDebugLine::ParsingState::resetRowAndSequence() {
   Row.reset(LineTable->Prologue.DefaultIsStmt);
   Sequence.reset();
 }
 
 void DWARFDebugLine::ParsingState::appendRowToMatrix() {
Error DWARFDebugLine::LineTable::parse(
     DWARFDataExtractor &DebugLineData, uint64_t *OffsetPtr,
     const DWARFContext &Ctx, const DWARFUnit *U,
     function_ref<void(Error)> RecoverableErrorHandler, raw_ostream *OS) {
   const uint64_t DebugLineOffset = *OffsetPtr;
 
   clear();
 
   Error PrologueErr =
       Prologue.parse(DebugLineData, OffsetPtr, RecoverableErrorHandler, Ctx, U);
 
   if (OS) {
     // The presence of OS signals verbose dumping.
     DIDumpOptions DumpOptions;
     DumpOptions.Verbose = true;
     Prologue.dump(*OS, DumpOptions);
   }
 
   if (PrologueErr)
     return PrologueErr;
 
   uint64_t ProgramLength = Prologue.TotalLength + Prologue.sizeofTotalLength();
   if (!DebugLineData.isValidOffsetForDataOfSize(DebugLineOffset,
                                                 ProgramLength)) {
     assert(DebugLineData.size() > DebugLineOffset &&
            "prologue parsing should handle invalid offset");
     uint64_t BytesRemaining = DebugLineData.size() - DebugLineOffset;
     RecoverableErrorHandler(
         createStringError(errc::invalid_argument,
                           "line table program with offset 0x%8.8" PRIx64
                           " has length 0x%8.8" PRIx64 " but only 0x%8.8" PRIx64
                           " bytes are available",
                           DebugLineOffset, ProgramLength, BytesRemaining));
     // Continue by capping the length at the number of remaining bytes.
     ProgramLength = BytesRemaining;
   }
 
   const uint64_t EndOffset = DebugLineOffset + ProgramLength;
 
   // See if we should tell the data extractor the address size.
   if (DebugLineData.getAddressSize() == 0)
     DebugLineData.setAddressSize(Prologue.getAddressSize());
   else
     assert(Prologue.getAddressSize() == 0 ||
            Prologue.getAddressSize() == DebugLineData.getAddressSize());
 
   ParsingState State(this);
 
+  // For versions less than 4, the MaxOpsPerInst member is set to 0, as it is
+  // not as the maximum_operations_per_instruction field wasn't introduced until
+  // DWARFv4. Don't warn about bad values in this situation.
+  bool IgnoreBadMaxOps = Prologue.getVersion() < 4;
+  auto CheckMaxOpsPerInst = [&IgnoreBadMaxOps, DebugLineOffset,
+                             RecoverableErrorHandler](uint64_t Offset,
+                                                      uint8_t MaxOpsPerInst) {
+    if (IgnoreBadMaxOps || MaxOpsPerInst == 1)
+      return;
+    IgnoreBadMaxOps = true;
+    RecoverableErrorHandler(createStringError(
+        errc::not_supported,
+        "line table program at offset 0x%8.8" PRIx64
+        " contains an opcode at offset 0x%8.8" PRIx64
+        " that advances the address, but the prologue "
+        "maximum_operations_per_instruction value is %" PRId8
+        ", which is unsupported. Assuming a value of 1 instead",
+        DebugLineOffset, Offset, MaxOpsPerInst));
+  };
+
   while (*OffsetPtr < EndOffset) {
     if (OS)
       *OS << format("0x%08.08" PRIx64 ": ", *OffsetPtr);
 
     uint8_t Opcode = DebugLineData.getU8(OffsetPtr);
 
     if (OS)
       *OS << format("%02.02" PRIx8 " ", Opcode);
if (Opcode == 0) {
         State.appendRowToMatrix();
         break;
 
       case DW_LNS_advance_pc:
         // Takes a single unsigned LEB128 operand, multiplies it by the
         // min_inst_length field of the prologue, and adds the
         // result to the address register of the state machine.
         {
+          CheckMaxOpsPerInst(*OffsetPtr - 1, Prologue.MaxOpsPerInst);
           uint64_t AddrOffset =
               DebugLineData.getULEB128(OffsetPtr) * Prologue.MinInstLength;
           State.Row.Address.Address += AddrOffset;
           if (OS)
             *OS << " (" << AddrOffset << ")";
         }
         break;
 
if (Opcode == 0) {
         // small amount, it can use a single special opcode, which occupies
         // a single byte. When it needs to advance the address by up to
         // twice the range of the last special opcode, it can use
         // DW_LNS_const_add_pc followed by a special opcode, for a total
         // of two bytes. Only if it needs to advance the address by more
         // than twice that range will it need to use both DW_LNS_advance_pc
         // and a special opcode, requiring three or more bytes.
         {
+          CheckMaxOpsPerInst(*OffsetPtr - 1, Prologue.MaxOpsPerInst);
           uint8_t AdjustOpcode = 255 - Prologue.OpcodeBase;
           uint64_t AddrOffset =
               (AdjustOpcode / Prologue.LineRange) * Prologue.MinInstLength;
           State.Row.Address.Address += AddrOffset;
           if (OS)
             *OS
                 << format(" (0x%16.16" PRIx64 ")", AddrOffset);
         }
if (Opcode == 0) {
       //  address increment = (adjusted opcode / line_range) *
       //                      minimum_instruction_length
       //
       // The amount to increment the line register is the line_base plus
       // the result of the adjusted opcode modulo the line_range. That is:
       //
       // line increment = line_base + (adjusted opcode % line_range)
 
+      CheckMaxOpsPerInst(*OffsetPtr - 1, Prologue.MaxOpsPerInst);
+
       uint8_t AdjustOpcode = Opcode - Prologue.OpcodeBase;
       uint64_t AddrOffset =
           (AdjustOpcode / Prologue.LineRange) * Prologue.MinInstLength;
       int32_t LineOffset =
           Prologue.LineBase + (AdjustOpcode % Prologue.LineRange);
       State.Row.Line += LineOffset;
       State.Row.Address.Address += AddrOffset;
 

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

 //===- DWARFDebugLineTest.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"
 #include "DwarfGenerator.h"
 #include "DwarfUtils.h"
 #include "llvm/DebugInfo/DWARF/DWARFContext.h"
-#include "llvm/DebugInfo/DWARF/DWARFDebugLine.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 using namespace dwarf;
 using namespace dwarfgen;
 using namespace object;
checkError("last sequence in debug line table at offset 0x00000000 is not "
              "terminated",
              std::move(Recoverable));
   ASSERT_TRUE(ExpectedLineTable.operator bool());
   EXPECT_EQ((*ExpectedLineTable)->Rows.size(), 6u);
   // The unterminated sequence is not added to the sequence list.
   EXPECT_EQ((*ExpectedLineTable)->Sequences.size(), 1u);
 }
 
+struct MaxOpsPerInstFixture
+    : public TestWithParam<std::tuple<uint16_t, uint8_t, bool>>,
+      public CommonFixture {
+  void SetUp() { std::tie(Version, MaxOpsPerInst, ExpectedError) = GetParam(); }
+
+  uint16_t Version;
+  uint8_t MaxOpsPerInst;
+  bool ExpectedError;
+};
+
+TEST_P(MaxOpsPerInstFixture, MaxOpsPerInstProblemsReportedCorrectly) {
+  if (!setupGenerator(Version))
+    return;
+
+  LineTable &Padding = Gen->addLineTable();
+  Padding.setCustomPrologue({{0, LineTable::Byte}});
+
+  // Show that no warning is generated for the case where no
+  // DW_LNS_const_add_pc or special opcode is used.
+  LineTable &NoProblem = Gen->addLineTable();
+  NoProblem.addExtendedOpcode(9, DW_LNE_set_address,
+                              {{0xabcd, LineTable::Quad}});
+  NoProblem.addStandardOpcode(DW_LNS_copy, {});
+  // Change the address, so that we have a valid sequence.
+  NoProblem.addExtendedOpcode(9, DW_LNE_set_address,
+                              {{0xabce, LineTable::Quad}});
+  NoProblem.addExtendedOpcode(1, DW_LNE_end_sequence, {});
+  DWARFDebugLine::Prologue NoProblemPrologue = NoProblem.createBasicPrologue();
+  NoProblemPrologue.MaxOpsPerInst = MaxOpsPerInst;
+  NoProblem.setPrologue(NoProblemPrologue);
+
+  // Show that the warning is emitted for the first DW_LNS_const_add_pc opcode
+  // and then not again.
+  LineTable &ConstAddPCFirst = Gen->addLineTable();
+  uint64_t Addr1 = 0x1234;
+  uint64_t ConstIncr = 0x11;
+  ConstAddPCFirst.addExtendedOpcode(9, DW_LNE_set_address,
+                                    {{Addr1, LineTable::Quad}});
+  ConstAddPCFirst.addStandardOpcode(DW_LNS_const_add_pc, {});
+  ConstAddPCFirst.addStandardOpcode(DW_LNS_const_add_pc, {});
+  ConstAddPCFirst.addStandardOpcode(DW_LNS_advance_pc,
+                                    {{0x10, LineTable::Quad}});
+  ConstAddPCFirst.addByte(0x21); // Special opcode, +1 op, +1 line.
+  ConstAddPCFirst.addExtendedOpcode(1, DW_LNE_end_sequence, {});
+  DWARFDebugLine::Prologue ConstAddPCPrologue =
+      ConstAddPCFirst.createBasicPrologue();
+  ConstAddPCPrologue.MaxOpsPerInst = MaxOpsPerInst;
+  ConstAddPCFirst.setPrologue(ConstAddPCPrologue);
+
+  // Show that the warning is emitted for the first DW_LNS_const_add_pc opcode
+  // and then not again.
+  LineTable &AdvancePCFirst = Gen->addLineTable();
+  uint64_t Addr1 = 0x1111;
+  AdvancePCFirst.addExtendedOpcode(9, DW_LNE_set_address,
+                                   {{Addr1, LineTable::Quad}});
+  AdvancePCFirst.addStandardOpcode(DW_LNS_advance_pc,
+                                   {{0x20, LineTable::Quad}});
+  AdvancePCFirst.addStandardOpcode(DW_LNS_const_add_pc, {});
+  AdvancePCFirst.addStandardOpcode(DW_LNS_advance_pc,
+                                   {{0x30, LineTable::Quad}});
+  AdvancePCFirst.addByte(0x22); // Special opcode, +1 op, +2 line.
+  AdvancePCFirst.addExtendedOpcode(1, DW_LNE_end_sequence, {});
+  DWARFDebugLine::Prologue AdvancePCPrologue =
+      AdvancePCFirst.createBasicPrologue();
+  AdvancePCPrologue.MaxOpsPerInst = MaxOpsPerInst;
+  AdvancePCFirst.setPrologue(AdvancePCPrologue);
+
+  // Show that the warning is emitted for the first special opcode and then
+  // not again.
+  LineTable &SpecialFirst = Gen->addLineTable();
+  uint64_t Addr2 = 0x5678;
+  SpecialFirst.addExtendedOpcode(9, DW_LNE_set_address,
+                                 {{Addr2, LineTable::Quad}});
+  SpecialFirst.addByte(0x23); // Special opcode, +1 op, +3 line.
+  SpecialFirst.addStandardOpcode(DW_LNS_const_add_pc, {});
+  AdvancePCFirst.addStandardOpcode(DW_LNS_advance_pc,
+                                   {{0x40, LineTable::Quad}});
+  SpecialFirst.addByte(0x24); // Special opcode, +1 op, +4 line.
+  SpecialFirst.addExtendedOpcode(1, DW_LNE_end_sequence, {});
+  DWARFDebugLine::Prologue SpecialPrologue = SpecialFirst.createBasicPrologue();
+  SpecialPrologue.MaxOpsPerInst = MaxOpsPerInst;
+  SpecialFirst.setPrologue(SpecialPrologue);
+
+  generate();
+
+  auto ExpectedNoProblem = Line.getOrParseLineTable(LineData, 1, *Context,
+                                                    nullptr, RecordRecoverable);
+  EXPECT_THAT_ERROR(std::move(Recoverable), Succeeded());
+  EXPECT_THAT_ERROR(std::move(Unrecoverable), Succeeded());
+  ASSERT_THAT_EXPECTED(ExpectedNoProblem, Succeeded());
+
+  uint64_t NextOffset = 1 + (*ExpectedNoProblem)->Prologue.TotalLength +
+                        (*ExpectedNoProblem)->Prologue.sizeofTotalLength();
+  auto ExpectedConstAddPC = Line.getOrParseLineTable(
+      LineData, NextOffset, *Context, nullptr, RecordRecoverable);
+  ASSERT_THAT_EXPECTED(ExpectedConstAddPC, Succeeded());
+  if (!ExpectedError) {
+    EXPECT_THAT_ERROR(std::move(Recoverable), Succeeded());
+  } else {
+    uint64_t ExpectedOffset = NextOffset +
+                              (*ExpectedConstAddPC)->Prologue.getLength() +
+                              11; // 11 == size of DW_LNE_set_address.
+    checkError(("line table program with offset 0x000000" +
+                Twine::utohexstr(NextOffset) +
+                " contains an opcode at offset 0x000000" +
+                Twine::utohexstr(ExpectedOffset) +
+                " that advances the address, but the prologue "
+                "maximum_operations_per_instruction value is " +
+                Twine(unsigned(MaxOpsPerInst)) +
+                ", which is unsupported. Assuming a value of 1 instead")
+                   .str(),
+               std::move(Recoverable));
+  }
+  EXPECT_THAT_ERROR(std::move(Unrecoverable), Succeeded());
+  ASSERT_EQ((*ExpectedConstAddPC)->Rows.size(), 2u);
+  EXPECT_EQ((*ExpectedConstAddPC)->Rows[0].Address.Address,
+            Addr1 + ConstIncr * 2 + 0x11);
+  EXPECT_EQ((*ExpectedConstAddPC)->Rows[0].Line, 2u);
+
+  NextOffset = NextOffset + (*ExpectedConstAddPC)->Prologue.TotalLength +
+               (*ExpectedConstAddPC)->Prologue.sizeofTotalLength();
+  auto ExpectedAdvancePC = Line.getOrParseLineTable(
+      LineData, NextOffset, *Context, nullptr, RecordRecoverable);
+  ASSERT_THAT_EXPECTED(ExpectedAdvancePC, Succeeded());
+  if (!ExpectedError) {
+    EXPECT_THAT_ERROR(std::move(Recoverable), Succeeded());
+  } else {
+    uint64_t ExpectedOffset = NextOffset +
+                              (*ExpectedAdvancePC)->Prologue.getLength() +
+                              11; // 11 == size of DW_LNE_set_address.
+    checkError(("line table program with offset 0x000000" +
+                Twine::utohexstr(NextOffset) +
+                " contains an opcode at offset 0x000000" +
+                Twine::utohexstr(ExpectedOffset) +
+                " that advances the address, but the prologue "
+                "maximum_operations_per_instruction value is " +
+                Twine(unsigned(MaxOpsPerInst)) +
+                ", which is unsupported. Assuming a value of 1 instead")
+                   .str(),
+               std::move(Recoverable));
+  }
+  EXPECT_THAT_ERROR(std::move(Unrecoverable), Succeeded());
+  ASSERT_EQ((*ExpectedAdvancePC)->Rows.size(), 2u);
+  EXPECT_EQ((*ExpectedAdvancePC)->Rows[0].Address.Address,
+            Addr1 + ConstIncr * 2 + 0x51);
+  EXPECT_EQ((*ExpectedAdvancePC)->Rows[0].Line, 3u);
+
+  NextOffset = NextOffset + (*ExpectedAdvancePC)->Prologue.TotalLength +
+               (*ExpectedAdvancePC)->Prologue.sizeofTotalLength();
+  auto ExpectedSpecial = Line.getOrParseLineTable(
+      LineData, NextOffset, *Context, nullptr, RecordRecoverable);
+  ASSERT_THAT_EXPECTED(ExpectedSpecial, Succeeded());
+  if (!ExpectedError) {
+    EXPECT_THAT_ERROR(std::move(Recoverable), Succeeded());
+  } else {
+    uint64_t ExpectedOffset = NextOffset +
+                              (*ExpectedSpecial)->Prologue.getLength() +
+                              11; // 11 == size of DW_LNE_set_address.
+    checkError(
+        ("line table program with offset 0x000000" +
+         Twine::utohexstr(NextOffset) +
+         " contains a special opcode or DW_LNS_const_add_pc opcode at "
+         "offset 0x000000" +
+         Twine::utohexstr(ExpectedOffset) +
+         ", but the prologue maximum_operations_per_instruction value is " +
+         Twine(unsigned(MaxOpsPerInst)) +
+         ", which is unsupported. Assuming a value of 1 instead")
+            .str(),
+        std::move(Recoverable));
+  }
+  ASSERT_EQ((*ExpectedSpecial)->Rows.size(), 3u);
+  EXPECT_EQ((*ExpectedSpecial)->Rows[0].Address.Address, Addr2 + 1);
+  EXPECT_EQ((*ExpectedSpecial)->Rows[0].Line, 4u);
+  EXPECT_EQ((*ExpectedSpecial)->Rows[1].Address.Address,
+            Addr2 + 0x72 + ConstIncr);
+  EXPECT_EQ((*ExpectedSpecial)->Rows[1].Line, 8u);
+}
+
+INSTANTIATE_TEST_CASE_P(
+    MaxOpsPerInstParams, MaxOpsPerInstFixture,
+    Values(std::make_tuple(3, 0, false), // Test for version < 4 (no error).
+           std::make_tuple(4, 0, true),  // Test zero value for V4 (error).
+           std::make_tuple(4, 1, false), // Test good value for V4 (no error).
+           std::make_tuple(
+               4, 2, true)), ); // Test one higher than permitted V4 (error).
+
 TEST_F(DebugLineBasicFixture, ParserParsesCorrectly) {
   if (!setupGenerator())
     return;
 
   DWARFDebugLine::SectionParser Parser = setupParser();
 
   EXPECT_EQ(Parser.getOffset(), 0u);
   ASSERT_FALSE(Parser.done());