[NFC][DebugInfo] Refactor address advancing operations to share code

[jhenderson]

Summary

On its own, this change won't achieve much, but it provides a good place to do extra error checks that I plan on adding in D43470, D74819 and D75189. See the full patch series for context.

[labath]

This isn't really my area, so I don't want to get too involved here, but it seems to me the code readability is going to suffer with all of the extra arguments being added to these (getOperationAdvance + advanceAddr) functions. They're just a bunch of locals from the LineTable::parse function -- maybe if these were lambdas inside that function, then they could capture most of these arguments implicitly? Or even create some sort of an object that would carry the state of the line program parsing machinery (like all the flags about reported warnings), and all of these things could be methods on it? That way, we might even be able to break down the parse function somehow, which is getting a bit unwieldy...

[jhenderson]

In D75188#1895417, @labath wrote:

This isn't really my area, so I don't want to get too involved here, but it seems to me the code readability is going to suffer with all of the extra arguments being added to these (getOperationAdvance + advanceAddr) functions. They're just a bunch of locals from the LineTable::parse function -- maybe if these were lambdas inside that function, then they could capture most of these arguments implicitly? Or even create some sort of an object that would carry the state of the line program parsing machinery (like all the flags about reported warnings), and all of these things could be methods on it? That way, we might even be able to break down the parse function somehow, which is getting a bit unwieldy...

Yeah, I and a colleague who I discussed this with did have similar concerns, although we hadn't come up with a better solution yet. I did consider making these lambdas, although I'm not a massive fan of having big lambdas (no specific reason for that, just my personal feeling). There is a ParsingState struct already being passed around. Perhaps some of the local variables could be put in that (i.e. the error handler, the warning booleans and the table offset, perhaps even the opcode offset)? That would reduce advanceAddr's parameter count to 4 or 5 from 7, and getOperationAdvance from 8 to 6 or 5, I believe.

[labath]

In D75188#1895521, @jhenderson wrote:

In D75188#1895417, @labath wrote:

This isn't really my area, so I don't want to get too involved here, but it seems to me the code readability is going to suffer with all of the extra arguments being added to these (getOperationAdvance + advanceAddr) functions. They're just a bunch of locals from the LineTable::parse function -- maybe if these were lambdas inside that function, then they could capture most of these arguments implicitly? Or even create some sort of an object that would carry the state of the line program parsing machinery (like all the flags about reported warnings), and all of these things could be methods on it? That way, we might even be able to break down the parse function somehow, which is getting a bit unwieldy...

Yeah, I and a colleague who I discussed this with did have similar concerns, although we hadn't come up with a better solution yet. I did consider making these lambdas, although I'm not a massive fan of having big lambdas (no specific reason for that, just my personal feeling).

I don't particularly like care for big lambdas either. If we really wanted to we could keep this function, and create a lambda just to "fix" some parameters. However, I like the next idea better.

There is a ParsingState struct already being passed around. Perhaps some of the local variables could be put in that (i.e. the error handler, the warning booleans and the table offset, perhaps even the opcode offset)? That would reduce advanceAddr's parameter count to 4 or 5 from 7, and getOperationAdvance from 8 to 6 or 5, I believe.

Yes, the ParsingState is exactly what I was looking for. I think that adding these locals there is a great idea.

But I also like my idea of making advanceAddr a member function of this struct. I think that would be similar to the existing appendRowToMatrix. Just like the implementation of DW_LNS_copy consists of calling that method, DW_LNS_advance_pc could be implemented by calling State.advanceAddr. (The function could still return the offset which it used to increment the address to enable the verbose dump.)

[jhenderson]

Rebase and further refactor things to reduce number of required arguments for the new functions in subsequent changes.

[labath]

I think this looks a lot better, but someone else ought to give the final approval here..

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

380

calculated

[jhenderson]

Fix grammar.

[probinson]

LGTM

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

912

It seems like there ought to be a symbol for opcode 255, but you inherited the code so I won't insist.

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

912

I can add that in a separate change if you'd like, although I'm not entirely sure what I'd label it as, since the DWARF spec (as quoted by the comment above) simply calls it the "special opcode 255".

[probinson]

One final question and reiterate LGTM.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

912

Not necessary.
We do assert that maximum_operations_per_instruction is 1, right? The advance operations are not factoring that in, but if it's always 1 then the simplified operations are correct.

[probinson]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

912

Duh, that's D74819. Never mind, all good here.

[jhenderson]

Get rid of getOperationAdvance function, as it was only being used in one place, so was unnecessarily complicated.

[probinson]

I agree this looks cleaner.

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

 
 private:
   struct ParsingState {
     ParsingState(struct LineTable *LT);
 
     void resetRowAndSequence();
     void appendRowToMatrix();
 
+    /// Advance the address by the \p OperationAdvance value. \returns the
+    /// amount advanced by.
+    uint64_t advanceAddr(uint64_t OperationAdvance);
+
+    struct AddrAndAdjustedOpcode {
+      uint64_t AddrDelta;
+      uint8_t AdjustedOpcode;
+    };
+
+    /// Advance the address as required by the specified \p Opcode.
+    /// \returns the amount advanced by and the calculated adjusted opcode.
+    AddrAndAdjustedOpcode advanceAddrForOpcode(uint8_t Opcode);
+
+    struct AddrAndLineDelta {
+      uint64_t Address;
+      int32_t Line;
+    };
+
+    /// Advance the line and address as required by the specified special \p
+    /// Opcode. \returns the address and line delta.
+    AddrAndLineDelta handleSpecialOpcode(uint8_t Opcode);
+
     /// Line table we're currently parsing.
     struct LineTable *LineTable;
     struct Row Row;
     struct Sequence Sequence;
   };
 
   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

if (Pos.second) {
     if (Error Err =
             LT->parse(DebugLineData, &Offset, Ctx, U, RecoverableErrorHandler))
       return std::move(Err);
     return LT;
   }
   return LT;
 }
 
+uint64_t DWARFDebugLine::ParsingState::advanceAddr(uint64_t OperationAdvance) {
+  uint64_t AddrOffset = OperationAdvance * LineTable->Prologue.MinInstLength;
+  Row.Address.Address += AddrOffset;
+  return AddrOffset;
+}
+
+DWARFDebugLine::ParsingState::AddrAndAdjustedOpcode
+DWARFDebugLine::ParsingState::advanceAddrForOpcode(uint8_t Opcode) {
+  assert(Opcode == DW_LNS_const_add_pc ||
+         Opcode >= LineTable->Prologue.OpcodeBase);
+  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 AddrOffset = advanceAddr(OperationAdvance);
+  return {AddrOffset, AdjustedOpcode};
+}
+
+DWARFDebugLine::ParsingState::AddrAndLineDelta
+DWARFDebugLine::ParsingState::handleSpecialOpcode(uint8_t Opcode) {
+  // 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
+  // opcode must be used instead of a special opcode. The "address
+  // advance" is calculated by dividing the desired address increment
+  // by the minimum_instruction_length field from the header. The
+  // special opcode is then calculated using the following formula:
+  //
+  //  opcode = (desired line increment - line_base) +
+  //           (line_range * address advance) + opcode_base
+  //
+  // If the resulting opcode is greater than 255, a standard opcode
+  // must be used instead.
+  //
+  // To decode a special opcode, subtract the opcode_base from the
+  // opcode itself to give the adjusted opcode. The amount to
+  // increment the address register is the result of the adjusted
+  // opcode divided by the line_range multiplied by the
+  // minimum_instruction_length field from the header. That is:
+  //
+  //  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);
+  int32_t 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) {
   const uint64_t DebugLineOffset = *OffsetPtr;
 
   clear();
 
if (Opcode == 0) {
         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.
         {
           uint64_t AddrOffset =
-              DebugLineData.getULEB128(OffsetPtr) * Prologue.MinInstLength;
-          State.Row.Address.Address += AddrOffset;
+              State.advanceAddr(DebugLineData.getULEB128(OffsetPtr));
           if (OS)
             *OS << " (" << AddrOffset << ")";
         }
         break;
 
       case DW_LNS_advance_line:
         // Takes a single signed LEB128 operand and adds that value to
         // the line register of the state machine.
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.
         {
-          uint8_t AdjustOpcode = 255 - Prologue.OpcodeBase;
-          uint64_t AddrOffset =
-              (AdjustOpcode / Prologue.LineRange) * Prologue.MinInstLength;
-          State.Row.Address.Address += AddrOffset;
+          uint64_t AddrOffset = State.advanceAddrForOpcode(Opcode).AddrDelta;
           if (OS)
-            *OS
-                << format(" (0x%16.16" PRIx64 ")", AddrOffset);
+            *OS << format(" (0x%16.16" PRIx64 ")", AddrOffset);
         }
         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
if (Opcode == 0) {
             if (OS)
               *OS << format("Skipping ULEB128 value: 0x%16.16" PRIx64 ")\n",
                             Value);
           }
         }
         break;
       }
     } else {
-      // Special Opcodes
-
-      // 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
-      // opcode must be used instead of a special opcode. The "address
-      // advance" is calculated by dividing the desired address increment
-      // by the minimum_instruction_length field from the header. The
-      // special opcode is then calculated using the following formula:
-      //
-      //  opcode = (desired line increment - line_base) +
-      //           (line_range * address advance) + opcode_base
-      //
-      // If the resulting opcode is greater than 255, a standard opcode
-      // must be used instead.
-      //
-      // To decode a special opcode, subtract the opcode_base from the
-      // opcode itself to give the adjusted opcode. The amount to
-      // increment the address register is the result of the adjusted
-      // opcode divided by the line_range multiplied by the
-      // minimum_instruction_length field from the header. That is:
-      //
-      //  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)
-
-      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;
+      // Special Opcodes.
+      ParsingState::AddrAndLineDelta Delta = State.handleSpecialOpcode(Opcode);
 
       if (OS) {
-        *OS << "address += " << AddrOffset << ",  line += " << LineOffset
+        *OS << "address += " << Delta.Address << ",  line += " << Delta.Line
             << "\n";
         OS->indent(12);
         State.Row.dump(*OS);
       }
 
       State.appendRowToMatrix();
     }
     if(OS)