DebugInfo: Stop modifying Operation::Error inside of verify()

[dexonsmith]

Summary

The only caller of Operation::verify() is DWARFExpression::verify(),
which iterates past the (ephemeral) Operation immediately after.

• Stop setting Operation::Error because the mutation will never be observed.
• Change verify() to a static function to be sure all callers are updated.

This is motivated by a follow-up patch, which will make
DWARFExpression::iterator iterate over const Operation.

[dexonsmith]

Oh, a couple of comments on alternatives:

• I considered moving the mutable keyword from iterator::Operation to Operation::Error, just limiting its scope. But since the change is completely ignored and it's reasonable to have a const-qualified verify() function this seems simpler.
• Another approach would be to make Operation::verify() a static local function private to DWARFExpression.cpp. It accesses private fields, but it'd be easy enough to rewrite it to use accessors.

[krasimir]

Thank you! This and the follow-up https://reviews.llvm.org/D113958 look reasonable to me.
Letting the people more familiar with the DebugInfo parts of the codebase have a chance to review.

[dblaikie]

Sounds good!

llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h

public:
     uint64_t getOperandEndOffset(unsigned Idx) const {
       return OperandEndOffsets[Idx];
     }
     uint64_t getEndOffset() const { return EndOffset; }
     bool isError() const { return Error; }
     bool print(raw_ostream &OS, DIDumpOptions DumpOpts,
                const DWARFExpression *Expr, const MCRegisterInfo *RegInfo,
                DWARFUnit *U, bool isEH) const;
-    bool verify(DWARFUnit *U);
+
+    /// Verify \p Op. Does not affect the return of \a isError().
+    static bool verify(const Operation &Op, DWARFUnit *U);
 
   private:
     bool extract(DataExtractor Data, uint8_t AddressSize, uint64_t Offset,
                  Optional<dwarf::DwarfFormat> Format);
   };
 
   /// An iterator to go through the expression operations.
   class iterator

llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp

if (EntryValExprSize) {
         OS << ")";
     }
 
     if (Op.getEndOffset() < Data.getData().size())
       OS << ", ";
   }
 }
 
-bool DWARFExpression::Operation::verify(DWARFUnit *U) {
-
+bool DWARFExpression::Operation::verify(const Operation &Op, DWARFUnit *U) {
   for (unsigned Operand = 0; Operand < 2; ++Operand) {
-    unsigned Size = Desc.Op[Operand];
+    unsigned Size = Op.Desc.Op[Operand];
 
     if (Size == Operation::SizeNA)
       break;
 
     if (Size == Operation::BaseTypeRef) {
       // For DW_OP_convert the operand may be 0 to indicate that conversion to
       // the generic type should be done, so don't look up a base type in that
       // case. The same holds for DW_OP_reinterpret, which is currently not
       // supported.
-      if (Opcode == DW_OP_convert && Operands[Operand] == 0)
+      if (Op.Opcode == DW_OP_convert && Op.Operands[Operand] == 0)
         continue;
-      auto Die = U->getDIEForOffset(U->getOffset() + Operands[Operand]);
-      if (!Die || Die.getTag() != dwarf::DW_TAG_base_type) {
-        Error = true;
+      auto Die = U->getDIEForOffset(U->getOffset() + Op.Operands[Operand]);
+      if (!Die || Die.getTag() != dwarf::DW_TAG_base_type)
         return false;
-      }
-    }
-  }
 
   return true;
 }
 
 bool DWARFExpression::verify(DWARFUnit *U) {
   for (auto &Op : *this)
-    if (!Op.verify(U))
+    if (!Operation::verify(Op, U))
       return false;
 
   return true;
 }
 
 /// A user-facing string representation of a DWARF expression. This might be an
 /// Address expression, in which case it will be implicitly dereferenced, or a
 /// Value expression.