[DWARF] Allow op-index in line number programs

[dstenb]

Summary

This extends DWARFDebugLine to properly parse line number programs with
maximum_operations_per_instruction > 1 for VLIW targets.

No functions that use that parsed output to retrieve line information
have been extended to support multiple op-indexes. This means that when
retrieving information for an address with multiple op-indexes, e.g.
when using llvm-addr2line, the penultimate row for that address will be
used, which in most cases is the row for the second largest op-index.
This will be addressed in further changes, but this patch at least
allows us to correctly parse such line number programs, with a warning
showing that the line number information may be incorrect (incomplete).

[dstenb]

Rebase.

[dstenb]

Sending a gentle ping on this (and the parent revision).

I assume that making upstream VLIW targets emit line number programs with maximum_operations_per_instruction > 1 will require quite a bit of work, so the immediate gains of these patches are small in that regard, but perhaps this is interesting for some other downstream VLIW target?

[StephenTozer]

Largely LGTM, some comments inline.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

670–671

If I understand correctly this calculation is incorrect, the division by MaxOpsPerInst comes before the multiply by MinInstLength.

For example, if MaxOpsPerInst = 4, MinInstLength = 8, Row.OpIndex = 1, OperationAdvance = 2, then this calculation would give AddrOffset = ((2 + 1) * 8) / 4 = 6, when it should be ((2 + 1) / 4) * 8 = 0 since we're actually advancing to a different OpIndex within the same instruction.

1279–1280

Any idea about the complexity of implementing this? It seems to me that most users either 1) already want a range, in which case this function can just return a pair of {firstIndex, lastIndex} and the caller can determine the range, or 2) want the info for a specific operation, in which case an OpIndex needs to be passed. I've not personally done anything with VLIW architectures but I'm assuming it's possible for different operations in the same instruction to have different source locations and even be in different inline frames, in which case it would be necessary to update a dozen or so callsites to pass an OpIndex through - seems like it would still be fairly simple in terms of code though.

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

1092–1093

It's not technically a part of supporting op-index, but might be a good idea to also include a MinInstLength > 1 in this test - as mentioned above I think the calculation for the address offset is wrong if MinInstLength > 1, which I expect it would be for an actual VLIW line table.

[dstenb]

Address review comments.

[dstenb]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

670–671

Thanks for catching that! I have updated the test fixture with MinInstLength > 1 as you suggest to test this.

1279–1280

I have some downstream WIP patches that starts addressing 1). I suspect it is mainly a lot of mechanical reworking of the code to propagate that information to the tools, but I can't show my working yet. I am not sure in how many cases 2) would be needed, but it seems that that for example could be useful if, whatever you are running a test on, can pinpoint which individual operation in a bundle that triggered a crash/interrupt.

There are some user interface decisions that need to be made, e.g. what the output for llvm-addr2line --inlines should look like when printing an address with multiple op-indexes. The locations for such addresses can, as you say, originate from different inline frames.

My hope is that, following some framework changes, mostly in this file, we can gradually enable proper op-index support in tools. Until we get to that point with a tool/option we should just be able to pick one of the locations if handling an address with multiple op-indexes.

[StephenTozer]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

1279–1280

I have some concerns about returning information that could be misleading (by misattributing the source location of an operation), since that can be worse than getting no information at all. I think it would be a suitable stopgap to disable those tools for line tables with MaxOperationsPerInst > 1, since that ensures that we are no worse off in any case. Open to other options though, or a justification for allowing the "incorrect" output.

[dstenb]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

1279–1280

I don't have any stronger justification than that I personally see value in at least getting some line information for an address belonging to a bundle, rather than none. I know that Binutils can parse line number programs with op-index, but I don't know what e.g. addr2line will emit for such cases. I have unfortunately not been able to try that out with a non-modified GDB on a mainstream target.

But I fully understand your concern, and that would in some sense go against LLVM's philosophy, e.g. as we drop incompatible locations when merging instructions.

If it is preferred, I can add an assert to findRowInSeq() that catches if the op-index is larger than 0? Then we will be able to correctly parse line number programs with maximum_operation_per_instruction > 1, correctly fetch line number information for addresses with a single operation or multiple operations with identical locations in such programs, and we can then "bubble out" the assert towards the tools as we gradually add proper op-index support.

That would change the "line table program at offset [...] contains a special opcode at offset [...], but the prologue maximum_operations_per_instruction value is [...], which is unsupported. Assuming a value of 1 instead" error to an assert (which would only hit when retrieving line information for an address with multiple operations), which maybe is not acceptable?

[dstenb]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

1279–1280

I guess it should be an llvm_unreachable rather than assert though, as we'd otherwise basically run into the same issue in a non-assert build.

[StephenTozer]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

1279–1280

I agree that replacing an error message with an assert may be a problem - perhaps the answer would be to keep the existing error message declaring that maximum_operation_per_instruction > 1 is not supported, but adjust it to indicate that support is incomplete/experimental and may produce incorrect information? The ideal is that we never produce wrong information, but as you point out: we're already producing "wrong" information for VLIW line tables by ignoring maximum_operation_per_instruction, so I don't think the patch should be blocked on not fixing everything.

[dstenb]

• Add recoverable error describing that support for maximum_operations_per_instruction > 1 is experimental, and that the line number information may be incorrect.
• Bring back an error for maximum_operations_per_instruction = 0, and upgrade that error message from "not supported", to "invalid".
• This replaces the MaxOpsPerInstFixture with two explicit test cases for maximum_operations_per_instruction = 0. I could not reuse that fixture for maximum_operations_per_instruction > 1, since the advance would be done with MaxOpsPerInst > 1 so the existing expects in the fixture would fail.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

1279–1280

Thanks, that is definitely preferable! I've done that in the latest diff.

[StephenTozer]

Changes LGTM!

[dstenb]

Thanks for the review! Will push this shortly.

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

 private:
   struct ParsingState {
     ParsingState(struct LineTable *LT, uint64_t TableOffset,
                  function_ref<void(Error)> ErrorHandler);
 
     void resetRowAndSequence();
     void appendRowToMatrix();
 
-    /// Advance the address by the \p OperationAdvance value. \returns the
-    /// amount advanced by.
-    uint64_t advanceAddr(uint64_t OperationAdvance, uint8_t Opcode,
-                         uint64_t OpcodeOffset);
+    struct AddrOpIndexDelta {
+      uint64_t AddrOffset;
+      int16_t OpIndexDelta;
+    };
 
-    struct AddrAndAdjustedOpcode {
+    /// Advance the address and op-index by the \p OperationAdvance value.
+    /// \returns the amount advanced by.
+    AddrOpIndexDelta advanceAddrOpIndex(uint64_t OperationAdvance,
+                                        uint8_t Opcode, uint64_t OpcodeOffset);
+
+    struct OpcodeAdvanceResults {
       uint64_t AddrDelta;
+      int16_t OpIndexDelta;
       uint8_t AdjustedOpcode;
     };
 
-    /// Advance the address as required by the specified \p Opcode.
+    /// Advance the address and op-index as required by the specified \p Opcode.
     /// \returns the amount advanced by and the calculated adjusted opcode.
-    AddrAndAdjustedOpcode advanceAddrForOpcode(uint8_t Opcode,
-                                               uint64_t OpcodeOffset);
+    OpcodeAdvanceResults advanceForOpcode(uint8_t Opcode,
 
-    struct AddrAndLineDelta {
+    struct SpecialOpcodeDelta {
       uint64_t Address;
       int32_t Line;
+      int16_t OpIndex;
     };
 
-    /// Advance the line and address as required by the specified special \p
-    /// Opcode. \returns the address and line delta.
-    AddrAndLineDelta handleSpecialOpcode(uint8_t Opcode, uint64_t OpcodeOffset);
+    /// Advance the line, address and op-index as required by the specified
+    /// special \p Opcode. \returns the address, op-index and line delta.
+    SpecialOpcodeDelta handleSpecialOpcode(uint8_t Opcode,
+                                           uint64_t OpcodeOffset);
 
     /// Line table we're currently parsing.
     struct LineTable *LineTable;
     struct Row Row;
     struct Sequence Sequence;
 
   private:
     uint64_t LineTableOffset;

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

 
 static StringRef getOpcodeName(uint8_t Opcode, uint8_t OpcodeBase) {
   assert(Opcode != 0);
   if (Opcode < OpcodeBase)
     return LNStandardString(Opcode);
   return "special";
 }
 
-uint64_t DWARFDebugLine::ParsingState::advanceAddr(uint64_t OperationAdvance,
-                                                   uint8_t Opcode,
-                                                   uint64_t OpcodeOffset) {
+DWARFDebugLine::ParsingState::AddrOpIndexDelta
+DWARFDebugLine::ParsingState::advanceAddrOpIndex(uint64_t OperationAdvance,
   StringRef OpcodeName = getOpcodeName(Opcode, LineTable->Prologue.OpcodeBase);
   // For versions less than 4, the MaxOpsPerInst member is set to 0, as the
   // maximum_operations_per_instruction field wasn't introduced until DWARFv4.
   // Don't warn about bad values in this situation.
   if (ReportAdvanceAddrProblem && LineTable->Prologue.getVersion() >= 4 &&
-      LineTable->Prologue.MaxOpsPerInst != 1)
+      LineTable->Prologue.MaxOpsPerInst == 0)
+    ErrorHandler(createStringError(
+        errc::invalid_argument,
+        "line table program at offset 0x%8.8" PRIx64
+        " contains a %s opcode at offset 0x%8.8" PRIx64
+        ", but the prologue maximum_operations_per_instruction value is 0"
+        ", which is invalid. Assuming a value of 1 instead",
+        LineTableOffset, OpcodeName.data(), OpcodeOffset));
+  // Although we are able to correctly parse line number programs with
+  // MaxOpsPerInst > 1, the rest of DWARFDebugLine and its
+  // users have not been updated to handle line information for all operations
+  // in a multi-operation instruction, so warn about potentially incorrect
+  // results.
+  if (ReportAdvanceAddrProblem && LineTable->Prologue.MaxOpsPerInst > 1)
     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 maximum_operations_per_instruction value is %" PRId8
-        ", which is unsupported. Assuming a value of 1 instead",
+        ", which is experimentally supported, so line number information "
+        "may be incorrect",
         LineTableOffset, OpcodeName.data(), OpcodeOffset,
         LineTable->Prologue.MaxOpsPerInst));
   if (ReportAdvanceAddrProblem && LineTable->Prologue.MinInstLength == 0)
     ErrorHandler(
         createStringError(errc::invalid_argument,
                           "line table program at offset 0x%8.8" PRIx64
                           " contains a %s opcode at offset 0x%8.8" PRIx64
                           ", but the prologue minimum_instruction_length value "
                           "is 0, which prevents any address advancing",
                           LineTableOffset, OpcodeName.data(), OpcodeOffset));
   ReportAdvanceAddrProblem = false;
-  uint64_t AddrOffset = OperationAdvance * LineTable->Prologue.MinInstLength;
+
+  // Advances the address and op_index according to DWARFv5, section 6.2.5.1:
+  //
+  // new address = address +
+  //   minimum_instruction_length *
+  //   ((op_index + operation advance) / maximum_operations_per_instruction)
+  //
+  // new op_index =
+  //   (op_index + operation advance) % maximum_operations_per_instruction
+
+  // For versions less than 4, the MaxOpsPerInst member is set to 0, as the
+  // maximum_operations_per_instruction field wasn't introduced until DWARFv4.
+  uint8_t MaxOpsPerInst =
+      std::max(LineTable->Prologue.MaxOpsPerInst, uint8_t{1});
+
+  uint64_t AddrOffset = ((Row.OpIndex + OperationAdvance) / MaxOpsPerInst) *
+                        LineTable->Prologue.MinInstLength;
   Row.Address.Address += AddrOffset;
-  return AddrOffset;
+
+  uint8_t PrevOpIndex = Row.OpIndex;
+  Row.OpIndex = (Row.OpIndex + OperationAdvance) % MaxOpsPerInst;
+  int16_t OpIndexDelta = static_cast<int16_t>(Row.OpIndex) - PrevOpIndex;
+
+  return {AddrOffset, OpIndexDelta};
 }
 
-DWARFDebugLine::ParsingState::AddrAndAdjustedOpcode
-DWARFDebugLine::ParsingState::advanceAddrForOpcode(uint8_t Opcode,
-                                                   uint64_t OpcodeOffset) {
+DWARFDebugLine::ParsingState::OpcodeAdvanceResults
+DWARFDebugLine::ParsingState::advanceForOpcode(uint8_t Opcode,
   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 =
       LineTable->Prologue.LineRange != 0
           ? AdjustedOpcode / LineTable->Prologue.LineRange
           : 0;
-  uint64_t AddrOffset = advanceAddr(OperationAdvance, Opcode, OpcodeOffset);
-  return {AddrOffset, AdjustedOpcode};
+  AddrOpIndexDelta Advance =
+      advanceAddrOpIndex(OperationAdvance, Opcode, OpcodeOffset);
+  return {Advance.AddrOffset, Advance.OpIndexDelta, AdjustedOpcode};
 }
 
-DWARFDebugLine::ParsingState::AddrAndLineDelta
+DWARFDebugLine::ParsingState::SpecialOpcodeDelta
 DWARFDebugLine::ParsingState::handleSpecialOpcode(uint8_t Opcode,
                                                   uint64_t OpcodeOffset) {
   // A special opcode value is chosen based on the amount that needs
   // to be added to the line and address registers. The maximum line
   // increment for a special opcode is the value of the line_base
   // field in the header, plus the value of the line_range field,
   // minus 1 (line base + line range - 1). If the desired line
   // increment is greater than the maximum line increment, a standard
DWARFDebugLine::ParsingState::handleSpecialOpcode(uint8_t Opcode,
   //  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)
 
-  DWARFDebugLine::ParsingState::AddrAndAdjustedOpcode AddrAdvanceResult =
-      advanceAddrForOpcode(Opcode, OpcodeOffset);
+  DWARFDebugLine::ParsingState::OpcodeAdvanceResults AddrAdvanceResult =
+      advanceForOpcode(Opcode, OpcodeOffset);
   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};
+  return {AddrAdvanceResult.AddrDelta, LineOffset,
+          AddrAdvanceResult.OpIndexDelta};
 }
 
 /// Parse a ULEB128 using the specified \p Cursor. \returns the parsed value on
 /// success, or std::nullopt if \p Cursor is in a failing state.
 template <typename T>
 static std::optional<T> parseULEB128(DWARFDataExtractor &Data,
                                      DataExtractor::Cursor &Cursor) {
   T Value = Data.getULEB128(Cursor);
if (Opcode == 0) {
         EmitRow();
         State.resetRowAndSequence();
         break;
 
       case DW_LNE_set_address:
         // Takes a single relocatable address as an operand. The size of the
         // 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.
+        // relocatable address and set the op_index register to 0. 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 = TableData.getAddressSize();
           uint64_t OpcodeAddressSize = Len - 1;
           if (ExtractorAddressSize != OpcodeAddressSize &&
               ExtractorAddressSize != 0)
if (Opcode == 0) {
                 " of DW_LNE_set_address opcode at offset 0x%8.8" PRIx64
                 " is unsupported",
                 OpcodeAddressSize, ExtOffset));
             TableData.skip(Cursor, OpcodeAddressSize);
           } else {
             TableData.setAddressSize(OpcodeAddressSize);
             State.Row.Address.Address = TableData.getRelocatedAddress(
                 Cursor, &State.Row.Address.SectionIndex);
+            State.Row.OpIndex = 0;
 
             uint64_t Tombstone =
                 dwarf::computeTombstoneAddress(OpcodeAddressSize);
             TombstonedAddress = State.Row.Address.Address == Tombstone;
 
             // Restore the address size if the extractor already had it.
             if (ExtractorAddressSize != 0)
               TableData.setAddressSize(ExtractorAddressSize);
if (Opcode == 0) {
       // Standard Opcodes
       case DW_LNS_copy:
         // Takes no arguments. Append a row to the matrix using the
         // current values of the state-machine registers.
         EmitRow();
         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.
+        // Takes a single unsigned LEB128 operand as the operation advance
+        // and modifies the address and op_index registers of the state machine
+        // according to that.
         if (std::optional<uint64_t> Operand =
                 parseULEB128<uint64_t>(TableData, Cursor)) {
-          uint64_t AddrOffset =
-              State.advanceAddr(*Operand, Opcode, OpcodeOffset);
+          ParsingState::AddrOpIndexDelta Advance =
+              State.advanceAddrOpIndex(*Operand, Opcode, OpcodeOffset);
           if (Verbose)
-            *OS << " (" << AddrOffset << ")";
+            *OS << " (addr += " << Advance.AddrOffset
+                << ", op-index += " << Advance.OpIndexDelta << ")";
         }
         break;
 
       case DW_LNS_advance_line:
         // Takes a single signed LEB128 operand and adds that value to
         // the line register of the state machine.
         {
           int64_t LineDelta = TableData.getSLEB128(Cursor);
if (Opcode == 0) {
 
       case DW_LNS_set_basic_block:
         // Takes no arguments. Set the basic_block register of the
         // state machine to true
         State.Row.BasicBlock = true;
         break;
 
       case DW_LNS_const_add_pc:
-        // Takes no arguments. Add to the address register of the state
-        // machine the address increment value corresponding to special
+        // Takes no arguments. Advance the address and op_index registers of
+        // the state machine by the increments corresponding to special
         // opcode 255. The motivation for DW_LNS_const_add_pc is this:
         // when the statement program needs to advance the address by a
         // 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.
         {
-          uint64_t AddrOffset =
-              State.advanceAddrForOpcode(Opcode, OpcodeOffset).AddrDelta;
+          ParsingState::OpcodeAdvanceResults Advance =
+              State.advanceForOpcode(Opcode, OpcodeOffset);
           if (Verbose)
-            *OS << format(" (0x%16.16" PRIx64 ")", AddrOffset);
+            *OS << format(" (addr += 0x%16.16" PRIx64 ", op-index += %" PRIu8
+                          ")",
+                          Advance.AddrDelta, Advance.OpIndexDelta);
         }
         break;
 
       case DW_LNS_fixed_advance_pc:
         // Takes a single uhalf operand. Add to the address register of
-        // the state machine the value of the (unencoded) operand. This
-        // is the only extended opcode that takes an argument that is not
-        // a variable length number. The motivation for DW_LNS_fixed_advance_pc
-        // is this: existing assemblers cannot emit DW_LNS_advance_pc or
-        // special opcodes because they cannot encode LEB128 numbers or
-        // judge when the computation of a special opcode overflows and
-        // requires the use of DW_LNS_advance_pc. Such assemblers, however,
-        // can use DW_LNS_fixed_advance_pc instead, sacrificing compression.
+        // the state machine the value of the (unencoded) operand and set
+        // the op_index register to 0. This is the only extended opcode that
+        // takes an argument that is not a variable length number.
+        // The motivation for DW_LNS_fixed_advance_pc is this: existing
+        // assemblers cannot emit DW_LNS_advance_pc or special opcodes because
+        // they cannot encode LEB128 numbers or judge when the computation
+        // of a special opcode overflows and requires the use of
+        // DW_LNS_advance_pc. Such assemblers, however, can use
+        // DW_LNS_fixed_advance_pc instead, sacrificing compression.
         {
           uint16_t PCOffset =
               TableData.getRelocatedValue(Cursor, 2);
           if (Cursor) {
             State.Row.Address.Address += PCOffset;
+            State.Row.OpIndex = 0;
             if (Verbose)
-              *OS << format(" (0x%4.4" PRIx16 ")", PCOffset);
+              *OS << format(" (addr += 0x%4.4" PRIx16 ", op-index = 0)",
+                            PCOffset);
           }
         }
         break;
 
       case DW_LNS_set_prologue_end:
         // Takes no arguments. Set the prologue_end register of the
         // state machine to true
         State.Row.PrologueEnd = true;
if (Opcode == 0) {
           }
         }
         break;
       }
 
       *OffsetPtr = Cursor.tell();
     } else {
       // Special Opcodes.
-      ParsingState::AddrAndLineDelta Delta =
+      ParsingState::SpecialOpcodeDelta Delta =
           State.handleSpecialOpcode(Opcode, OpcodeOffset);
 
       if (Verbose)
-        *OS << "address += " << Delta.Address << ",  line += " << Delta.Line;
+        *OS << "address += " << Delta.Address << ",  line += " << Delta.Line
+            << ",  op-index += " << Delta.OpIndex;
       EmitRow();
       *OffsetPtr = Cursor.tell();
     }
 
     // When a row is added to the matrix, it is also dumped, which includes a
     // new line already, so don't add an extra one.
     if (Verbose && Rows.size() == RowCount)
       *OS << "\n";
uint32_t DWARFDebugLine::LineTable::findRowInSeq(
   if (!Seq.containsPC(Address))
     return UnknownRowIndex;
   assert(Seq.SectionIndex == Address.SectionIndex);
   // In some cases, e.g. first instruction in a function, the compiler generates
   // two entries, both with the same address. We want the last one.
   //
   // In general we want a non-empty range: the last row whose address is less
   // than or equal to Address. This can be computed as upper_bound - 1.
+  //
+  // TODO: This function, and its users, needs to be update to return multiple
+  // rows for bundles with multiple op-indexes.
   DWARFDebugLine::Row Row;
   Row.Address = Address;
   RowIter FirstRow = Rows.begin() + Seq.FirstRowIndex;
   RowIter LastRow = Rows.begin() + Seq.LastRowIndex;
   assert(FirstRow->Address.Address <= Row.Address.Address &&
          Row.Address.Address < LastRow[-1].Address.Address);
   RowIter RowPos = std::upper_bound(FirstRow + 1, LastRow - 1, Row,
                                     DWARFDebugLine::Row::orderByAddress) -

llvm/test/MC/ELF/RISCV/gen-dwarf.s

 # CHECK-NEXT: DW_CFA_offset: X1 -8
 # CHECK-NEXT: DW_CFA_nop:
 
 # CHECK:      DW_LNE_set_address
 # CHECK-NEXT: DW_LNS_advance_line ([[#]])
 # CHECK-NEXT: DW_LNS_copy
 # CHECK-NEXT:                           is_stmt
 # CHECK-NEXT: DW_LNS_advance_line
-# CHECK-NEXT: DW_LNS_fixed_advance_pc (0x0004)
+# CHECK-NEXT: DW_LNS_fixed_advance_pc (addr += 0x0004, op-index = 0)
 # CHECK-NEXT: DW_LNS_copy
 # CHECK-NEXT:                           is_stmt
 # CHECK-NEXT: DW_LNS_advance_line
-# CHECK-NEXT: DW_LNS_fixed_advance_pc (0x0004)
+# CHECK-NEXT: DW_LNS_fixed_advance_pc (addr += 0x0004, op-index = 0)
 # CHECK-NEXT: DW_LNS_copy
 
 # CHECK:      0x00000000: range list header: length = 0x0000001d, format = DWARF32, version = 0x0005
 # CHECK-NEXT: ranges:
 # CHECK-NEXT: 0x0000000c: [DW_RLE_start_length]:  0x0000000000000000, 0x0000000000000034
 # CHECK-NEXT: 0x00000016: [DW_RLE_start_length]:  0x0000000000000000, 0x0000000000000004
 # CHECK-NEXT: 0x00000020: [DW_RLE_end_of_list ]
 

llvm/test/tools/llvm-dwarfdump/X86/tombstone.s

 # CHECK:         DW_AT_low_pc [DW_FORM_addrx]     (indexed (00000001) address = 0x0000000000000042)
 # CHECK:         DW_AT_high_pc [DW_FORM_data4]   (0x00000006)
 
 # CHECK-DAG: .debug_line contents:
 # CHECK:      Address Line
 # CHECK-NEXT: --------------
 # CHECK-NEXT: DW_LNE_set_address (0xffffffff)
 # CHECK-NEXT: DW_LNS_copy
-# CHECK-NEXT: DW_LNS_advance_pc (1)
+# CHECK-NEXT: DW_LNS_advance_pc (addr += 1, op-index += 0)
 # CHECK-NEXT: DW_LNE_end_sequence
 # CHECK-NEXT: DW_LNE_set_address (0x00000042)
 # CHECK-NEXT: DW_LNS_copy
 # CHECK-NEXT:   0x0000000000000042 1
-# CHECK-NEXT: DW_LNS_advance_pc (1)
+# CHECK-NEXT: DW_LNS_advance_pc (addr += 1, op-index += 0)
 # CHECK-NEXT: DW_LNE_end_sequence
 # CHECK:      Address Line
 # CHECK-NEXT: --------------
 # CHECK-NEXT: DW_LNE_set_address (0xffffffffffffffff)
 # CHECK-NEXT: DW_LNS_copy
-# CHECK-NEXT: DW_LNS_advance_pc (1)
+# CHECK-NEXT: DW_LNS_advance_pc (addr += 1, op-index += 0)
 # CHECK-NEXT: DW_LNE_end_sequence
 # CHECK-NEXT: DW_LNE_set_address (0x0000000000000042)
 # CHECK-NEXT: DW_LNS_copy
 # CHECK-NEXT:   0x0000000000000042 1
-# CHECK-NEXT: DW_LNS_advance_pc (1)
+# CHECK-NEXT: DW_LNS_advance_pc (addr += 1, op-index += 0)
 # CHECK-NEXT: DW_LNE_end_sequence
 
 # Dumping of the debug_addr, ranges, and rnglists sections don't do anything
 # different with tombstoned addresses, but dump them just for
 # documentation/comparison with the tombstone-filtered renderings in the
 # debug_info section above
 
 # CHECK-DAG: .debug_addr contents:

llvm/test/tools/llvm-dwarfdump/X86/verbose.test

 # CHECK-EMPTY:
 #  CHECK-NEXT:            Address            Line   Column File   ISA Discriminator OpIndex Flags
 #  CHECK-NEXT:            ------------------ ------ ------ ------ --- ------------- ------- -------------
 #  CHECK-NEXT:0x00000029: 00 DW_LNE_set_address (0x0000000000000000)
 #  CHECK-NEXT:0x00000034: 01 DW_LNS_copy
 #  CHECK-NEXT:            0x0000000000000000      1      0      1   0             0       0  is_stmt
 #  CHECK-NEXT:0x00000035: 05 DW_LNS_set_column (12)
 #  CHECK-NEXT:0x00000037: 0a DW_LNS_set_prologue_end
-#  CHECK-NEXT:0x00000038: 66 address += 6,  line += 0
+#  CHECK-NEXT:0x00000038: 66 address += 6,  line += 0,  op-index += 0
 #  CHECK-NEXT:            0x0000000000000006      1     12      1   0             0       0  is_stmt prologue_end
-#  CHECK-NEXT:0x00000039: 02 DW_LNS_advance_pc (2)
+#  CHECK-NEXT:0x00000039: 02 DW_LNS_advance_pc (addr += 2, op-index += 0)
 #  CHECK-NEXT:0x0000003b: 00 DW_LNE_end_sequence
 #  CHECK-NEXT:            0x0000000000000008      1     12      1   0             0       0  is_stmt end_sequence
 # CHECK-EMPTY:
 
 #   CHECK-NOT:{{.*}} contents:

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

struct AdjustAddressFixtureBase : public CommonFixture {
   uint64_t ConstAddPCOffset;
   uint64_t SpecialOffset;
   uint64_t AdvancePCOffset;
 
   uint16_t Version = 4;
   bool IsErrorExpected;
 };
 
-struct MaxOpsPerInstFixture
-    : TestWithParam<std::tuple<uint16_t, uint8_t, bool>>,
-      AdjustAddressFixtureBase {
-  void SetUp() override {
-    std::tie(Version, MaxOpsPerInst, IsErrorExpected) = GetParam();
-  }
-
-  uint64_t editPrologue(LineTable &LT) override {
+struct OpIndexFixture : Test, CommonFixture {
+  void createPrologue(LineTable &LT, uint8_t MaxOpsPerInst,
+                      uint8_t MinInstLength) {
     DWARFDebugLine::Prologue Prologue = LT.createBasicPrologue();
     Prologue.MaxOpsPerInst = MaxOpsPerInst;
+    Prologue.MinInstLength = MinInstLength;
     LT.setPrologue(Prologue);
-    return Prologue.TotalLength + Prologue.sizeofTotalLength();
   }
+};
+
+#ifdef _AIX
+TEST_F(OpIndexFixture, DISABLED_OpIndexAdvance) {
+#else
+TEST_F(OpIndexFixture, OpIndexAdvance) {
+#endif
+  if (!setupGenerator(4, 4))
+    GTEST_SKIP();
+
+  uint8_t MaxOpsPerInst = 13;
+  uint8_t MinInstLength = 4;
+
+  LineTable &LT = Gen->addLineTable();
+
+  // Row 0-2: Different locations for one bundle set up using special opcodes.
+  LT.addExtendedOpcode(5, DW_LNE_set_address, {{0x20, LineTable::Long}});
+  LT.addByte(0x13); // Special opcode, +1 line.
+  LT.addByte(0x23); // Special opcode, +3 line, +1 op-index.
+  LT.addByte(0x3a); // Special opcode, -2 line, +3 op-index.
+
+  // Row 3: New bundle, set up using DW_LNS_advance pc.
+  // Operation advance 0x84, which gives +40 addr, +2 op-index
+  LT.addStandardOpcode(DW_LNS_advance_line, {{100, LineTable::SLEB}});
+  LT.addStandardOpcode(DW_LNS_advance_pc, {{0x84, LineTable::ULEB}});
+  LT.addStandardOpcode(DW_LNS_copy, {}); // Create new row.
+
+  // Row 4: New bundle, set up using a single special opcode.
+  LT.addByte(0x71); // Special opcode, +4 addr, -3 line, -6 op-index.
+
+  // Row 5: New bundle, set up using using DW_LNS_const_add_pc.
+  // Corresponds to advancing address and op-index using special opcode 255,
+  // which gives +4 addr, +4 op-index.
+  LT.addStandardOpcode(DW_LNS_advance_line, {{10, LineTable::SLEB}});
+  LT.addStandardOpcode(DW_LNS_const_add_pc, {});
+  LT.addStandardOpcode(DW_LNS_copy, {}); // Create new row.
+
+  // Row 6: End sequence to have the input well-formed.
+  LT.addExtendedOpcode(1, DW_LNE_end_sequence, {});
 
-  uint8_t MaxOpsPerInst;
+  createPrologue(LT, MaxOpsPerInst, MinInstLength);
+
+  generate();
+
+  auto VerifyRow = [](const DWARFDebugLine::Row &Row, uint64_t Address,
+                      uint8_t OpIndex, uint32_t Line) {
+    EXPECT_EQ(Row.Address.Address, Address);
+    EXPECT_EQ(Row.OpIndex, OpIndex);
+    EXPECT_EQ(Row.Line, Line);
   };
 
+  auto Table = Line.getOrParseLineTable(LineData, 0, *Context, nullptr,
+                                        RecordRecoverable);
+  EXPECT_THAT_ERROR(
+      std::move(Recoverable),
+      FailedWithMessage("line table program at offset 0x00000000 contains a "
+                        "special opcode at offset 0x00000035, but the prologue "
+                        "maximum_operations_per_instruction value is 13, which "
+                        "is experimentally supported, so line number "
+                        "information may be incorrect"));
+  EXPECT_THAT_ERROR(std::move(Unrecoverable), Succeeded());
+  ASSERT_THAT_EXPECTED(Table, Succeeded());
+
+  ASSERT_EQ((*Table)->Rows.size(), 7u);
+
+  VerifyRow((*Table)->Rows[0], 0x20, 0, 2);
+  VerifyRow((*Table)->Rows[1], 0x20, 1, 5);
+  VerifyRow((*Table)->Rows[2], 0x20, 4, 3);
+  VerifyRow((*Table)->Rows[3], 0x48, 6, 103);
+  VerifyRow((*Table)->Rows[4], 0x4c, 0, 100);
+  VerifyRow((*Table)->Rows[5], 0x50, 4, 110);
+}
+
 #ifdef _AIX
-TEST_P(MaxOpsPerInstFixture, DISABLED_MaxOpsPerInstProblemsReportedCorrectly) {
+TEST_F(OpIndexFixture, DISABLED_OpIndexReset) {
 #else
-TEST_P(MaxOpsPerInstFixture, MaxOpsPerInstProblemsReportedCorrectly) {
+TEST_F(OpIndexFixture, OpIndexReset) {
 #endif
-  runTest(/*CheckAdvancePC=*/true,
-          "but the prologue maximum_operations_per_instruction value is " +
-              Twine(unsigned(MaxOpsPerInst)) +
-              ", which is unsupported. Assuming a value of 1 instead");
+  if (!setupGenerator(4, 4))
+    GTEST_SKIP();
+
+  uint8_t MaxOpsPerInst = 13;
+  uint8_t MinInstLength = 4;
+
+  LineTable &LT = Gen->addLineTable();
+
+  // Row 0: Just set op-index to some value > 0.
+  LT.addExtendedOpcode(5, DW_LNE_set_address, {{0, LineTable::Long}});
+  LT.addByte(0x20); // Special opcode, +1 op-index
+
+  // Row 1: DW_LNE_fixed_advance_pc should set op-index to 0.
+  LT.addStandardOpcode(DW_LNS_fixed_advance_pc, {{10, LineTable::Half}});
+  LT.addStandardOpcode(DW_LNS_copy, {}); // Create new row.
+
+  // Row 2: Just set op-index to some value > 0.
+  LT.addByte(0x66); // Special opcode, +6 op-index
+
+  // Row 3: DW_LNE_set_address should set op-index to 0.
+  LT.addExtendedOpcode(5, DW_LNE_set_address, {{20, LineTable::Long}});
+  LT.addStandardOpcode(DW_LNS_copy, {}); // Create new row.
+
+  // Row 4: Just set op-index to some value > 0.
+  LT.addByte(0xba); // Special opcode, +12 op-index
+
+  // Row 5: End sequence (op-index unchanged for this row)...
+  LT.addExtendedOpcode(1, DW_LNE_end_sequence, {});
+
+  // Row 6: ... but shall be reset after the DW_LNE_end_sequence row.
+  LT.addStandardOpcode(DW_LNS_copy, {}); // Create new row.
+
+  // Row 7: End sequence to have the input well-formed.
+  LT.addExtendedOpcode(1, DW_LNE_end_sequence, {});
+
+  createPrologue(LT, MaxOpsPerInst, MinInstLength);
+
+  generate();
+
+  auto Table = Line.getOrParseLineTable(LineData, 0, *Context, nullptr,
+                                        RecordRecoverable);
+  EXPECT_THAT_ERROR(
+      std::move(Recoverable),
+      FailedWithMessage("line table program at offset 0x00000000 contains a "
+                        "special opcode at offset 0x00000035, but the prologue "
+                        "maximum_operations_per_instruction value is 13, which "
+                        "is experimentally supported, so line number "
+                        "information may be incorrect"));
+  EXPECT_THAT_ERROR(std::move(Unrecoverable), Succeeded());
+  ASSERT_THAT_EXPECTED(Table, Succeeded());
+
+  ASSERT_EQ((*Table)->Rows.size(), 8u);
+  EXPECT_EQ((*Table)->Rows[0].OpIndex, 1u);
+  EXPECT_EQ((*Table)->Rows[1].OpIndex, 0u); // DW_LNS_fixed_advance_pc.
+  EXPECT_EQ((*Table)->Rows[2].OpIndex, 6u);
+  EXPECT_EQ((*Table)->Rows[3].OpIndex, 0u); // DW_LNE_set_address.
+  EXPECT_EQ((*Table)->Rows[4].OpIndex, 12u);
+  EXPECT_EQ((*Table)->Rows[5].OpIndex, 12u);
+  EXPECT_EQ((*Table)->Rows[6].OpIndex, 0u); // row after DW_LNE_end_sequence.
+  EXPECT_EQ((*Table)->Rows[7].OpIndex, 0u);
 }
 
-INSTANTIATE_TEST_SUITE_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).
+#ifdef _AIX
+TEST_F(OpIndexFixture, DISABLED_MaxOpsZeroDwarf3) {
+#else
+TEST_F(OpIndexFixture, MaxOpsZeroDwarf3) {
+#endif
+  if (!setupGenerator(3, 4))
+    GTEST_SKIP();
+
+  LineTable &LT = Gen->addLineTable();
+  LT.addStandardOpcode(DW_LNS_const_add_pc, {}); // Just some opcode to advance.
+  LT.addExtendedOpcode(1, DW_LNE_end_sequence, {}); //
+  createPrologue(LT, /*MaxOpsPerInst=*/0, /*MinInstLength=*/1);
+  generate();
+
+  auto Table = Line.getOrParseLineTable(LineData, 0, *Context, nullptr,
+                                        RecordRecoverable);
+  EXPECT_THAT_ERROR(std::move(Recoverable), Succeeded());
+  EXPECT_THAT_ERROR(std::move(Unrecoverable), Succeeded());
+  ASSERT_THAT_EXPECTED(Table, Succeeded());
+}
+
+#ifdef _AIX
+TEST_F(OpIndexFixture, DISABLED_MaxOpsZeroDwarf4) {
+#else
+TEST_F(OpIndexFixture, MaxOpsZeroDwarf4) {
+#endif
+  if (!setupGenerator(4, 4))
+    GTEST_SKIP();
+
+  LineTable &LT = Gen->addLineTable();
+  LT.addStandardOpcode(DW_LNS_const_add_pc, {}); // Just some opcode to advance.
+  LT.addExtendedOpcode(1, DW_LNE_end_sequence, {}); //
+  createPrologue(LT, /*MaxOpsPerInst=*/0, /*MinInstLength=*/1);
+  generate();
+
+  auto Table = Line.getOrParseLineTable(LineData, 0, *Context, nullptr,
+                                        RecordRecoverable);
+  EXPECT_THAT_ERROR(
+      std::move(Recoverable),
+      FailedWithMessage(
+          "line table program at offset 0x00000000 contains a "
+          "DW_LNS_const_add_pc opcode at offset 0x0000002e, but "
+          "the prologue maximum_operations_per_instruction value "
+          "is 0, which is invalid. Assuming a value of 1 instead"));
+  EXPECT_THAT_ERROR(std::move(Unrecoverable), Succeeded());
+  ASSERT_THAT_EXPECTED(Table, Succeeded());
+}
 
 struct LineRangeFixture : TestWithParam<std::tuple<uint8_t, bool>>,
                           AdjustAddressFixtureBase {
   void SetUp() override { std::tie(LineRange, IsErrorExpected) = GetParam(); }
 
   uint64_t editPrologue(LineTable &LT) override {
     DWARFDebugLine::Prologue Prologue = LT.createBasicPrologue();
     Prologue.LineRange = LineRange;
#endif
   EXPECT_EQ(NextLine(),
             "0x00000042: 00 DW_LNE_set_address (0x0123456789abcdef)");
   EXPECT_EQ(NextLine(), "0x0000004d: 00 DW_LNE_define_file (a, dir=2, "
                         "mod_time=(0x0000000000000003), length=4)");
   EXPECT_EQ(NextLine(), "0x00000055: 00 DW_LNE_set_discriminator (127)");
   EXPECT_EQ(NextLine(), "0x00000059: 01 DW_LNS_copy");
   EXPECT_EQ(NextLine(), "            0x0123456789abcdef      1      0      1   "
                         "0           127       0  is_stmt");
-  EXPECT_EQ(NextLine(), "0x0000005a: 02 DW_LNS_advance_pc (11)");
+  EXPECT_EQ(NextLine(), "0x0000005a: 02 DW_LNS_advance_pc (addr += 11, "
+                        "op-index += 0)");
   EXPECT_EQ(NextLine(), "0x0000005c: 03 DW_LNS_advance_line (23)");
   EXPECT_EQ(NextLine(), "0x0000005e: 04 DW_LNS_set_file (33)");
   EXPECT_EQ(NextLine(), "0x00000060: 05 DW_LNS_set_column (44)");
   EXPECT_EQ(NextLine(), "0x00000062: 06 DW_LNS_negate_stmt");
   EXPECT_EQ(NextLine(), "0x00000063: 07 DW_LNS_set_basic_block");
   EXPECT_EQ(NextLine(),
-            "0x00000064: 08 DW_LNS_const_add_pc (0x0000000000000011)");
-  EXPECT_EQ(NextLine(), "0x00000065: 09 DW_LNS_fixed_advance_pc (0x0037)");
+            "0x00000064: 08 DW_LNS_const_add_pc (addr += 0x0000000000000011, "
+            "op-index += 0)");
+  EXPECT_EQ(NextLine(), "0x00000065: 09 DW_LNS_fixed_advance_pc (addr += 0x0037"
+            ", op-index = 0)");
   EXPECT_EQ(NextLine(), "0x00000068: 0a DW_LNS_set_prologue_end");
   EXPECT_EQ(NextLine(), "0x00000069: 0b DW_LNS_set_epilogue_begin");
   EXPECT_EQ(NextLine(), "0x0000006a: 0c DW_LNS_set_isa (66)");
   EXPECT_EQ(NextLine(), "0x0000006c: 0d Unrecognized standard opcode "
                         "(operands: 0x0000000000000001, 0x0123456789abcdef)");
   EXPECT_EQ(NextLine(), "0x00000077: 0e Unrecognized standard opcode");
-  EXPECT_EQ(NextLine(), "0x00000078: ff address += 17,  line += -3");
+  EXPECT_EQ(NextLine(), "0x00000078: ff address += 17,  line += -3,  "
+                        "op-index += 0");
   EXPECT_EQ(NextLine(),
             "            0x0123456789abce53     20     44     33  66           "
             "  0       0  basic_block prologue_end epilogue_begin");
   EXPECT_EQ(NextLine(), "0x00000079: 00 DW_LNE_end_sequence");
   EXPECT_EQ(NextLine(), "            0x0123456789abce53     20     44     33  "
                         "66             0       0  end_sequence");
   EXPECT_EQ(NextLine(), "");
   EXPECT_EQ(Output.size(), Pos);