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

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added inline comments.


================
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);
----------------
probinson wrote:
> 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.
> 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.


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

https://reviews.llvm.org/D73264