[DWARF] Do not pass Version to DWARFExpression.

[ikudrin]

Summary

The Version was used only to determine the size of an operand of DW_OP_call_ref. The size was 4 for all versions apart from 2, but the DW_OP_call_ref operation was introduced only in DWARF3. Thus, the code may be simplified and using of Version may be eliminated.

[probinson]

Reviewing the DWARF spec for DW_OP_call_ref:
This operator does not exist in DWARF v2. In v3 and onward, its operand is specified as 4- or 8-byte depending on the DWARF-32/64 format. There is non-normative commentary that says this is the same interpretation as DW_FORM_ref_addr, which is true for v3 and onward.
While the interpretation of DW_FORM_ref_addr did change from v2 to v3, I think that's not relevant to DW_OP_call_ref, because the normative text for its operand has always explicitly specified that it is format-dependent.
So, I think it is correct to remove getRefAddrSize() from DWARFExpression, and have the SizeRefAddr case be strictly format-dependent.

Removing the Version field also seems appropriate, even though it's a bit of churn. Version would be most helpful for something like strict verification; on the other hand, all the operator descriptions carry the version-when-first-defined, and the verify() call takes a DWARFUnit* which will specify the version to verify against.

One thing is missing: There should be a test to verify we are handling DW_OP_call_ref in a V2 unit according to the format rather than the address size.

[ikudrin]

• Rebase, Update;
• Add a test.

@probinson, did you mean a test like this? I am a bit in doubt because it checks an unrealistic case.

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp

158–161

Looks like the old code handled 64 and 32 - so is this not a regression? (unlikely we'd want to regress functionality) or was this dead code previously? Or some other explanation?

[probinson]

LGTM but give @dblaikie a chance to say whether he accepts my explanation.

llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp

158–161

The old code would use getU64() only if you had Version == 2 and AddressSize == 64. For Version > 2 it was hard-coded to return size 4, i.e. it did not correctly look at the 32/64 *format*. Doing that correctly will, I expect, be a follow-up (because @ikudrin has been doing DWARF-64 stuff in general).

IMO this is not a regression. Pedantically you should never see a SizeRefAddr op in DWARF v2, it was introduced in v3.

llvm/test/DebugInfo/X86/DW_OP_call_ref_ver2.s

5

Please add a comment saying what this is for.

[ikudrin]

• Add an explanatory comment to the test.

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp

158–161

Pedantically you should never see a SizeRefAddr op in DWARF v2, it was introduced in v3.

That suggests this was dead code, then? That this code path wasn't reachable (no SizeRefAddr op in DWARFv2) with anything other than a ref addr size of 4, so the other two code paths here were dead?

Or that they are reachable (one could craft an input that would execute the code), but the behavior isn't important/meaningful to change?

It looks like it's literally unreachable/dead code (the only SizeRefAddr in Descriptions table is for DWARFv3 and above), so I'm fine with that.

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

private:
     uint64_t OperandEndOffsets[2];
 
   public:
     Description &getDescription() { return Desc; }
     uint8_t getCode() { return Opcode; }
     uint64_t getRawOperand(unsigned Idx) { return Operands[Idx]; }
     uint64_t getOperandEndOffset(unsigned Idx) { return OperandEndOffsets[Idx]; }
     uint64_t getEndOffset() { return EndOffset; }
-    bool extract(DataExtractor Data, uint16_t Version, uint8_t AddressSize,
-                 uint64_t Offset);
+    bool extract(DataExtractor Data, uint8_t AddressSize, uint64_t Offset);
     bool isError() { return Error; }
     bool print(raw_ostream &OS, const DWARFExpression *Expr,
                const MCRegisterInfo *RegInfo, DWARFUnit *U, bool isEH);
     bool verify(DWARFUnit *U);
   };
 
   /// An iterator to go through the expression operations.
   class iterator
       : public iterator_facade_base<iterator, std::forward_iterator_tag,
                                     Operation> {
     friend class DWARFExpression;
     const DWARFExpression *Expr;
     uint64_t Offset;
     Operation Op;
     iterator(const DWARFExpression *Expr, uint64_t Offset)
         : Expr(Expr), Offset(Offset) {
       Op.Error =
           Offset >= Expr->Data.getData().size() ||
-          !Op.extract(Expr->Data, Expr->Version, Expr->AddressSize, Offset);
+          !Op.extract(Expr->Data, Expr->AddressSize, Offset);
     }
 
   public:
     class Operation &operator++() {
       Offset = Op.isError() ? Expr->Data.getData().size() : Op.EndOffset;
       Op.Error =
           Offset >= Expr->Data.getData().size() ||
-          !Op.extract(Expr->Data, Expr->Version, Expr->AddressSize, Offset);
+          !Op.extract(Expr->Data, Expr->AddressSize, Offset);
       return Op;
     }
 
     class Operation &operator*() {
       return Op;
     }
 
     // Comparison operators are provided out of line.
     friend bool operator==(const iterator &, const iterator &);
   };
 
-  DWARFExpression(DataExtractor Data, uint16_t Version, uint8_t AddressSize)
-      : Data(Data), Version(Version), AddressSize(AddressSize) {
+  DWARFExpression(DataExtractor Data, uint8_t AddressSize)
+      : Data(Data), AddressSize(AddressSize) {
     assert(AddressSize == 8 || AddressSize == 4 || AddressSize == 2);
   }
 
   iterator begin() const { return iterator(this, 0); }
   iterator end() const { return iterator(this, Data.getData().size()); }
 
   void print(raw_ostream &OS, const MCRegisterInfo *RegInfo, DWARFUnit *U,
              bool IsEH = false) const;
 
   bool verify(DWARFUnit *U);
 
 private:
   DataExtractor Data;
-  uint16_t Version;
   uint8_t AddressSize;
 };
 
 inline bool operator==(const DWARFExpression::iterator &LHS,
                        const DWARFExpression::iterator &RHS) {
   return LHS.Expr == RHS.Expr && LHS.Offset == RHS.Offset;
 }
 
 inline bool operator!=(const DWARFExpression::iterator &LHS,
                        const DWARFExpression::iterator &RHS) {
   return !(LHS == RHS);
 }
 }
 #endif

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

void DwarfDebug::emitDebugLocEntry(ByteStreamer &Streamer,
   // DwarfExpression::addExpression) so for those ops (e.g. DW_OP_convert) that
   // need to reference a base_type DIE the offset of that DIE is not yet known.
   // To deal with this we instead insert a placeholder early and then extract
   // it here and replace it with the real reference.
   unsigned PtrSize = Asm->MAI->getCodePointerSize();
   DWARFDataExtractor Data(StringRef(DebugLocs.getBytes(Entry).data(),
                                     DebugLocs.getBytes(Entry).size()),
                           Asm->getDataLayout().isLittleEndian(), PtrSize);
-  DWARFExpression Expr(Data, getDwarfVersion(), PtrSize);
+  DWARFExpression Expr(Data, PtrSize);
 
   using Encoding = DWARFExpression::Operation::Encoding;
   uint64_t Offset = 0;
   for (auto &Op : Expr) {
     assert(Op.getCode() != dwarf::DW_OP_const_type &&
            "3 operand ops not yet supported");
     Streamer.EmitInt8(Op.getCode(), Comment != End ? *(Comment++) : "");
     Offset++;

llvm/lib/DWARFLinker/DWARFLinker.cpp

unsigned DWARFLinker::DIECloner::cloneBlockAttribute(
   SmallVector<uint8_t, 32> Buffer;
   ArrayRef<uint8_t> Bytes = *Val.getAsBlock();
   if (DWARFAttribute::mayHaveLocationDescription(AttrSpec.Attr) &&
       (Val.isFormClass(DWARFFormValue::FC_Block) ||
        Val.isFormClass(DWARFFormValue::FC_Exprloc))) {
     DWARFUnit &OrigUnit = Unit.getOrigUnit();
     DataExtractor Data(StringRef((const char *)Bytes.data(), Bytes.size()),
                        IsLittleEndian, OrigUnit.getAddressByteSize());
-    DWARFExpression Expr(Data, OrigUnit.getVersion(),
-                         OrigUnit.getAddressByteSize());
+    DWARFExpression Expr(Data, OrigUnit.getAddressByteSize());
     cloneExpression(Data, Expr, OF, Unit, Buffer);
     Bytes = Buffer;
   }
   for (auto Byte : Bytes)
     Attr->addValue(DIEAlloc, static_cast<dwarf::Attribute>(0),
                    dwarf::DW_FORM_data1, DIEInteger(Byte));
 
   // FIXME: If DIEBlock and DIELoc just reuses the Size field of
if (!Linker.Options.NoOutput) {
 
       Linker.patchRangesForUnit(*CurrentUnit, DwarfContext, OF);
       auto ProcessExpr = [&](StringRef Bytes,
                              SmallVectorImpl<uint8_t> &Buffer) {
         DWARFUnit &OrigUnit = CurrentUnit->getOrigUnit();
         DataExtractor Data(Bytes, IsLittleEndian,
                            OrigUnit.getAddressByteSize());
         cloneExpression(Data,
-                        DWARFExpression(Data, OrigUnit.getVersion(),
-                                        OrigUnit.getAddressByteSize()),
+                        DWARFExpression(Data, OrigUnit.getAddressByteSize()),
                         OF, *CurrentUnit, Buffer);
       };
       Emitter->emitLocationsForUnit(*CurrentUnit, DwarfContext, ProcessExpr);
     }
   }
 
   if (!Linker.Options.NoOutput) {
     assert(Emitter);

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp

if (Primary) {
           break;
         }
         case DW_CFA_def_cfa_expression: {
           uint32_t ExprLength = Data.getULEB128(Offset);
           addInstruction(Opcode, 0);
           DataExtractor Extractor(
               Data.getData().slice(*Offset, *Offset + ExprLength),
               Data.isLittleEndian(), Data.getAddressSize());
-          Instructions.back().Expression = DWARFExpression(
-              Extractor, Data.getAddressSize(), dwarf::DWARF_VERSION);
+          Instructions.back().Expression =
+              DWARFExpression(Extractor, Data.getAddressSize());
           *Offset += ExprLength;
           break;
         }
         case DW_CFA_expression:
         case DW_CFA_val_expression: {
           auto RegNum = Data.getULEB128(Offset);
           auto BlockLength = Data.getULEB128(Offset);
           addInstruction(Opcode, RegNum, 0);
           DataExtractor Extractor(
               Data.getData().slice(*Offset, *Offset + BlockLength),
               Data.isLittleEndian(), Data.getAddressSize());
-          Instructions.back().Expression = DWARFExpression(
-              Extractor, Data.getAddressSize(), dwarf::DWARF_VERSION);
+          Instructions.back().Expression =
+              DWARFExpression(Extractor, Data.getAddressSize());
           *Offset += BlockLength;
           break;
         }
       }
     }
   }
 
   return Error::success();

llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp

case dwarf::DW_LLE_start_length:
     return DWARFLocationExpression{
         DWARFAddressRange{E.Value0, E.Value0 + E.Value1, E.SectionIndex},
         E.Loc};
   default:
     llvm_unreachable("unreachable locations list kind");
   }
 }
 
-// When directly dumping the .debug_loc without a compile unit, we have to guess
-// at the DWARF version. This only affects DW_OP_call_ref, which is a rare
-// expression that LLVM doesn't produce. Guessing the wrong version means we
-// won't be able to pretty print expressions in DWARF2 binaries produced by
-// non-LLVM tools.
 static void dumpExpression(raw_ostream &OS, ArrayRef<uint8_t> Data,
                            bool IsLittleEndian, unsigned AddressSize,
                            const MCRegisterInfo *MRI, DWARFUnit *U) {
   DWARFDataExtractor Extractor(toStringRef(Data), IsLittleEndian, AddressSize);
-  DWARFExpression(Extractor, dwarf::DWARF_VERSION, AddressSize).print(OS, MRI, U);
+  DWARFExpression(Extractor, AddressSize).print(OS, MRI, U);
 }
 
 bool DWARFLocationTable::dumpLocationList(uint64_t *Offset, raw_ostream &OS,
                                           Optional<SectionedAddress> BaseAddr,
                                           const MCRegisterInfo *MRI,
                                           const DWARFObject &Obj, DWARFUnit *U,
                                           DIDumpOptions DumpOpts,
                                           unsigned Indent) const {

llvm/lib/DebugInfo/DWARF/DWARFDie.cpp

static void dumpLocation(raw_ostream &OS, DWARFFormValue &FormValue,
                          DIDumpOptions DumpOpts) {
   DWARFContext &Ctx = U->getContext();
   const MCRegisterInfo *MRI = Ctx.getRegisterInfo();
   if (FormValue.isFormClass(DWARFFormValue::FC_Block) ||
       FormValue.isFormClass(DWARFFormValue::FC_Exprloc)) {
     ArrayRef<uint8_t> Expr = *FormValue.getAsBlock();
     DataExtractor Data(StringRef((const char *)Expr.data(), Expr.size()),
                        Ctx.isLittleEndian(), 0);
-    DWARFExpression(Data, U->getVersion(), U->getAddressByteSize())
-        .print(OS, MRI, U);
+    DWARFExpression(Data, U->getAddressByteSize()).print(OS, MRI, U);
     return;
   }
 
   if (FormValue.isFormClass(DWARFFormValue::FC_SectionOffset)) {
     uint64_t Offset = *FormValue.getAsSectionOffset();
 
     if (FormValue.getForm() == DW_FORM_loclistx) {
       FormValue.dump(OS, DumpOpts);

llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp

static DWARFExpression::Operation::Description getOpDesc(unsigned OpCode) {
   // FIXME: Make this constexpr once all compilers are smart enough to do it.
   static DescVector Descriptions = getDescriptions();
   // Handle possible corrupted or unsupported operation.
   if (OpCode >= Descriptions.size())
     return {};
   return Descriptions[OpCode];
 }
 
-static uint8_t getRefAddrSize(uint8_t AddrSize, uint16_t Version) {
-  return (Version == 2) ? AddrSize : 4;
-}
-
-bool DWARFExpression::Operation::extract(DataExtractor Data, uint16_t Version,
+bool DWARFExpression::Operation::extract(DataExtractor Data,
                                          uint8_t AddressSize, uint64_t Offset) {
   Opcode = Data.getU8(&Offset);
 
   Desc = getOpDesc(Opcode);
   if (Desc.Version == Operation::DwarfNA) {
     EndOffset = Offset;
     return false;
   }
case Operation::Size4:
         Operands[Operand] = (int32_t)Operands[Operand];
       break;
     case Operation::Size8:
       Operands[Operand] = Data.getU64(&Offset);
       break;
     case Operation::SizeAddr:
       Operands[Operand] = Data.getUnsigned(&Offset, AddressSize);
       break;
     case Operation::SizeRefAddr:
-      if (getRefAddrSize(AddressSize, Version) == 8) {
-        Operands[Operand] = Data.getU64(&Offset);
-      } else if (getRefAddrSize(AddressSize, Version) == 4) {
-        Operands[Operand] = Data.getU32(&Offset);
-      } else {
-        assert(getRefAddrSize(AddressSize, Version) == 2);
-        Operands[Operand] = Data.getU16(&Offset);
-      }
+      // TODO: Add support for 64-bit DWARF format.
       break;
     case Operation::SizeLEB:
       if (Signed)
         Operands[Operand] = Data.getSLEB128(&Offset);
       else
         Operands[Operand] = Data.getULEB128(&Offset);
       break;
     case Operation::BaseTypeRef:
       Operands[Operand] = Data.getULEB128(&Offset);

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

case DW_AT_stmt_list:
     ReportError("DIE has invalid DW_AT_stmt_list encoding:");
     break;
   case DW_AT_location: {
     if (Expected<std::vector<DWARFLocationExpression>> Loc =
             Die.getLocations(DW_AT_location)) {
       DWARFUnit *U = Die.getDwarfUnit();
       for (const auto &Entry : *Loc) {
         DataExtractor Data(toStringRef(Entry.Expr), DCtx.isLittleEndian(), 0);
-        DWARFExpression Expression(Data, U->getVersion(),
-                                   U->getAddressByteSize());
+        DWARFExpression Expression(Data, U->getAddressByteSize());
         bool Error = any_of(Expression, [](DWARFExpression::Operation &Op) {
           return Op.isError();
         });
         if (Error || !Expression.verify(U))
           ReportError("DIE contains invalid DWARF expression:");
       }
     } else
       ReportError(toString(Loc.takeError()));
static bool isVariableIndexable(const DWARFDie &Die, DWARFContext &DCtx) {
   if (!Loc) {
     consumeError(Loc.takeError());
     return false;
   }
   DWARFUnit *U = Die.getDwarfUnit();
   for (const auto &Entry : *Loc) {
     DataExtractor Data(toStringRef(Entry.Expr), DCtx.isLittleEndian(),
                        U->getAddressByteSize());
-    DWARFExpression Expression(Data, U->getVersion(), U->getAddressByteSize());
+    DWARFExpression Expression(Data, U->getAddressByteSize());
     bool IsInteresting = any_of(Expression, [](DWARFExpression::Operation &Op) {
       return !Op.isError() && (Op.getCode() == DW_OP_addr ||
                                Op.getCode() == DW_OP_form_tls_address ||
                                Op.getCode() == DW_OP_GNU_push_tls_address);
     });
     if (IsInteresting)
       return true;
   }

llvm/test/DebugInfo/X86/DW_OP_call_ref_ver2.s

+# RUN: llvm-mc -triple x86_64-pc-linux %s -filetype=obj | \
+# RUN:   llvm-dwarfdump - | \
+# RUN:   FileCheck %s
+
+# This checks that the operand of DW_OP_call_ref is always parsed corresponding
+# to the DWARF format of CU. Our code used to have an exception for verson == 2,
+# where it treated the operand like it had the size of address, but since
+# DW_OP_call_ref was introduced only in DWARF3, the code could be simplified.
+
+# CHECK: DW_TAG_variable
+# CHECK-NEXT: DW_AT_location (DW_OP_call_ref 0x11223344)
+
+  .section  .debug_abbrev,"",@progbits
+  .byte 1                       # Abbreviation Code
+  .byte 17                      # DW_TAG_compile_unit
+  .byte 1                       # DW_CHILDREN_yes
+  .byte 0                       # EOM(1)
+  .byte 0                       # EOM(2)
+  .byte 5                       # Abbreviation Code
+  .byte 52                      # DW_TAG_variable
+  .byte 0                       # DW_CHILDREN_no
+  .byte 2                       # DW_AT_location
+  .byte 10                      # DW_FORM_block1
+  .byte 0                       # EOM(1)
+  .byte 0                       # EOM(2)
+  .byte 0                       # EOM(3)
+
+  .section  .debug_info,"",@progbits
+  .long .Lcu_end-.Lcu_start     # Length of Unit
+.Lcu_start:
+  .short  2                       # DWARF version number
+  .long .debug_abbrev           # Offset Into Abbrev. Section
+  .byte 8                       # Address Size (in bytes)
+  .byte 1                       # Abbrev [1] DW_TAG_compile_unit
+  .byte 5                       # Abbrev [5] DW_TAG_variable
+  .byte .Lloc_end-.Lloc_begin   # DW_AT_location
+.Lloc_begin:
+  .byte 154                     # DW_OP_call_ref
+  .long 0x11223344              # Offset
+.Lloc_end:
+  .byte 0                       # End Of Children Mark
+.Lcu_end:

llvm/tools/llvm-dwarfdump/Statistics.cpp

static void collectStatsForDie(DWARFDie Die, std::string FnPrefix,
 
   if (Die.find(dwarf::DW_AT_artificial))
     IsArtificial = true;
 
   auto IsEntryValue = [&](ArrayRef<uint8_t> D) -> bool {
     DWARFUnit *U = Die.getDwarfUnit();
     DataExtractor Data(toStringRef(D),
                        Die.getDwarfUnit()->getContext().isLittleEndian(), 0);
-    DWARFExpression Expression(Data, U->getVersion(), U->getAddressByteSize());
+    DWARFExpression Expression(Data, U->getAddressByteSize());
     // Consider the expression containing the DW_OP_entry_value as
     // an entry value.
     return llvm::any_of(Expression, [](DWARFExpression::Operation &Op) {
       return Op.getCode() == dwarf::DW_OP_entry_value ||
              Op.getCode() == dwarf::DW_OP_GNU_entry_value;
     });
   };