[PATCH] D73264: [DWARF] Do not pass Version to DWARFExpression.

[Paul Robinson via Phabricator via llvm-commits]

probinson accepted this revision.
probinson added a comment.
This revision is now accepted and ready to land.

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



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp:158-160
     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.
+      Operands[Operand] = Data.getU32(&Offset);
----------------
dblaikie wrote:
> 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?
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.


================
Comment at: llvm/test/DebugInfo/X86/DW_OP_call_ref_ver2.s:4
+# RUN:   FileCheck %s
+
+# CHECK: DW_TAG_variable
----------------
Please add a comment saying what this is for.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73264/new/

https://reviews.llvm.org/D73264