[DebugInfo] Prevent crash when .debug_line line_range is zero

[jhenderson]

Summary

This change adds a check for a .debug_line line_range value of 0 to prevent the program crashing when a special opcode or DW_LNS_const_add_pc is detected. The diagnostic is only issued once per table, and for this case the opcodes do not adjust the address or line.

Depends on D74819.

[aprantl]

lib/DebugInfo/DWARF/DWARFDebugLine.cpp

309 ↗

(On Diff #134927)

Can you also make it so that llvm-dwarfdump --verify reports this as an error?

[aprantl]

lib/DebugInfo/DWARF/DWARFDebugLine.cpp

309 ↗

(On Diff #134927)

Nevermind. That's what you did!

[aprantl]

lib/DebugInfo/DWARF/DWARFDebugLine.cpp

302 ↗

(On Diff #134927)

Bonus points for fixing this one, too :-)

309 ↗

(On Diff #134927)

Can you use DWARFVerifier::warn() here?

[jhenderson]

lib/DebugInfo/DWARF/DWARFDebugLine.cpp

290 ↗

(On Diff #134927)

And presumably this one too @aprantl?

309 ↗

(On Diff #134927)

I'll take a look for this and the other usages here.

llvm-dwarfdump --verify reports an error automatically if false is returned here, so that's how we get an error, as demonstrated in the test.

[aprantl]

lib/DebugInfo/DWARF/DWARFDebugLine.cpp

290 ↗

(On Diff #134927)

That would be great. Can be a separate commit, and you don't need to open a review for that one.

[jhenderson]

lib/DebugInfo/DWARF/DWARFDebugLine.cpp

309 ↗

(On Diff #134927)

Can you use DWARFVerifier::warn() here?

Just quickly looking at this before I leave the office. This function is currently private, and relies on an instance of DWARFVerifier, which I don't think is available. Do you mean to make it do the same thing as what DWARFVerifier::warn() does?

[JDevlieghere]

IMHO, since these "warnings" effectively abort parsing, the right thing to do here is returning an llvm::error which can then be printed by the verifier.

[jhenderson]

In D43470#1012578, @JDevlieghere wrote:

IMHO, since these "warnings" effectively abort parsing, the right thing to do here is returning an llvm::error which can then be printed by the verifier.

Having thought about this a bit and played around, I'm inclined to agree that this is the correct approach. It'll be a bigger change, but it will make the code more amenable to being used as a library, where the consumers can decide what to do with the errors/warnings on a case-by-case basis. I'll experiment with it a bit more, and then upload an updated patch.

[JDevlieghere]

In D43470#1013034, @jhenderson wrote:

In D43470#1012578, @JDevlieghere wrote:

IMHO, since these "warnings" effectively abort parsing, the right thing to do here is returning an llvm::error which can then be printed by the verifier.

Having thought about this a bit and played around, I'm inclined to agree that this is the correct approach. It'll be a bigger change, but it will make the code more amenable to being used as a library, where the consumers can decide what to do with the errors/warnings on a case-by-case basis. I'll experiment with it a bit more, and then upload an updated patch.

Great, thanks!

[jhenderson]

Resurrecting this change, by rewriting the fix on the latest HEAD. In this version, the check is done when a special opcode or DW_LNS_const_add_pc is encountered. An Error is reported if the line_range is 0 for the first such instance of these. Testing is now done via unit tests.

[jhenderson]

Please could this be re-reviewed, given the significant changes in the area since the last review.

[jhenderson]

I missed a case that needs checking. Will revisit.

[jhenderson]

Rewrite based on D75188 and D74819.

[probinson]

It looks like getOperationAdvance() and advanceAddr() now share a lot of parameters. Is it feasible to combine them? I see that they're not *always* called together, but it seems like it would improve clarity not to repeat 3 lines of parameters.

[jhenderson]

Rebase, to adopt interface change as suggested by @probinson. Also fix a warning in the test.

[probinson]

I see a pre-merge clang-format alert in DWARFDebugLine.h that you should take care of.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

670

This seems a bit much for a ternary operator.

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

981

While you _can_ write this in one line, I think it's clearer to write

if (LineRange == 0)
  return Base + AdvanceIncr;
return AdjustAddressFixtureBase::getAdjustedAddr( ... );

[jhenderson]

Fix formatting. Simplify ternaries.

[probinson]

LGTM.

[jhenderson]

Rebase to take account of opcode name handling changes.

[probinson]

One more small bit of cleanup.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

606

This could go under the if as you don't need the name unless you are reporting the warning.

615

Setting the flag to false can go under the if to help distinguish error handling from functional code.

[jhenderson]

Rebase, and put bits inside the if.

[jhenderson]

Remove unnecessary public.

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

struct ParsingState {
     struct LineTable *LineTable;
     struct Row Row;
     struct Sequence Sequence;
 
   private:
     uint64_t LineTableOffset;
 
     bool ReportAdvanceAddrProblem = true;
+    bool ReportBadLineRange = true;
     function_ref<void(Error)> ErrorHandler;
   };
 
   using LineTableMapTy = std::map<uint64_t, LineTable>;
   using LineTableIter = LineTableMapTy::iterator;
   using LineTableConstIter = LineTableMapTy::const_iterator;
 
   LineTableMapTy LineTableMap;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARFDEBUGLINE_H

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

uint64_t DWARFDebugLine::ParsingState::advanceAddr(uint64_t OperationAdvance,
   return AddrOffset;
 }
 
 DWARFDebugLine::ParsingState::AddrAndAdjustedOpcode
 DWARFDebugLine::ParsingState::advanceAddrForOpcode(uint8_t Opcode,
                                                    uint64_t OpcodeOffset) {
   assert(Opcode == DW_LNS_const_add_pc ||
          Opcode >= LineTable->Prologue.OpcodeBase);
+  if (ReportBadLineRange && LineTable->Prologue.LineRange == 0) {
+    StringRef OpcodeName =
+        getOpcodeName(Opcode, LineTable->Prologue.OpcodeBase);
+    ErrorHandler(
+        createStringError(errc::not_supported,
+                          "line table program at offset 0x%8.8" PRIx64
+                          " contains a %s opcode at offset 0x%8.8" PRIx64
+                          ", but the prologue line_range value is 0. The "
+                          "address and line will not be adjusted",
+                          LineTableOffset, OpcodeName.data(), OpcodeOffset));
+    ReportBadLineRange = false;
+  }
+
   uint8_t OpcodeValue = Opcode;
   if (Opcode == DW_LNS_const_add_pc)
     OpcodeValue = 255;
   uint8_t AdjustedOpcode = OpcodeValue - LineTable->Prologue.OpcodeBase;
-  uint64_t OperationAdvance = AdjustedOpcode / LineTable->Prologue.LineRange;
+  uint64_t OperationAdvance =
+      LineTable->Prologue.LineRange != 0
+          ? AdjustedOpcode / LineTable->Prologue.LineRange
+          : 0;
   uint64_t AddrOffset = advanceAddr(OperationAdvance, Opcode, OpcodeOffset);
   return {AddrOffset, AdjustedOpcode};
 }
 
 DWARFDebugLine::ParsingState::AddrAndLineDelta
 DWARFDebugLine::ParsingState::handleSpecialOpcode(uint8_t Opcode,
                                                   uint64_t OpcodeOffset) {
   // A special opcode value is chosen based on the amount that needs
DWARFDebugLine::ParsingState::handleSpecialOpcode(uint8_t Opcode,
   //
   // 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)
 
   DWARFDebugLine::ParsingState::AddrAndAdjustedOpcode AddrAdvanceResult =
       advanceAddrForOpcode(Opcode, OpcodeOffset);
-  int32_t LineOffset =
+  int32_t LineOffset = 0;
+  if (LineTable->Prologue.LineRange != 0)
+    LineOffset =
         LineTable->Prologue.LineBase +
         (AddrAdvanceResult.AdjustedOpcode % LineTable->Prologue.LineRange);
   Row.Line += LineOffset;
   return {AddrAdvanceResult.AddrDelta, LineOffset};
 }
 
 Error DWARFDebugLine::LineTable::parse(
     DWARFDataExtractor &DebugLineData, uint64_t *OffsetPtr,
     const DWARFContext &Ctx, const DWARFUnit *U,
     function_ref<void(Error)> RecoverableErrorHandler, raw_ostream *OS) {

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

 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).
 
+struct LineRangeFixture : public TestWithParam<std::tuple<uint8_t, bool>>,
+                          public AdjustAddressFixtureBase {
+  void SetUp() override { std::tie(LineRange, IsErrorExpected) = GetParam(); }
+
+  uint64_t editPrologue(LineTable &LT) override {
+    DWARFDebugLine::Prologue Prologue = LT.createBasicPrologue();
+    Prologue.LineRange = LineRange;
+    LT.setPrologue(Prologue);
+    return Prologue.TotalLength + Prologue.sizeofTotalLength();
+  }
+
+  uint64_t getAdjustedAddr(uint64_t Base, uint64_t ConstIncr,
+                           uint64_t SpecialIncr,
+                           uint64_t AdvanceIncr) override {
+    if (LineRange == 0)
+      return Base + AdvanceIncr;
+    return AdjustAddressFixtureBase::getAdjustedAddr(Base, ConstIncr,
+                                                     SpecialIncr, AdvanceIncr);
+  }
+
+  uint64_t getAdjustedLine(uint64_t Base, uint64_t Incr) override {
+    return LineRange != 0
+               ? AdjustAddressFixtureBase::getAdjustedLine(Base, Incr)
+               : Base;
+  }
+
+  uint8_t LineRange;
+};
+
+TEST_P(LineRangeFixture, LineRangeProblemsReportedCorrectly) {
+  runTest(/*CheckAdvancePC=*/false,
+          "but the prologue line_range value is 0. The address and line will "
+          "not be adjusted");
+}
+
+INSTANTIATE_TEST_CASE_P(
+    LineRangeParams, LineRangeFixture,
+    Values(std::make_tuple(0, true),       // Test zero value (error).
+           std::make_tuple(14, false)), ); // Test non-zero value (no error).
+
 TEST_F(DebugLineBasicFixture, ParserParsesCorrectly) {
   if (!setupGenerator())
     return;
 
   DWARFDebugLine::SectionParser Parser = setupParser();
 
   EXPECT_EQ(Parser.getOffset(), 0u);
   ASSERT_FALSE(Parser.done());