[PATCH] D156545: [DebugInfo][InstrRef] Don't produce over-sized DW_OP_deref_size expressions for very wide stack spills

[Jeremy Morse via Phabricator via llvm-commits]

jmorse planned changes to this revision.
jmorse added a comment.

Sorry for the month delay, I was wandering up some mountains,

Coming back to this with fresh eyes, I missed something in the reproducer from GH ticket -- I thought it was spilling an entire vector register to the stack, however...

  push    rbp
  mov     rbp, rsp
  sub     rsp, 16
  movsd   qword ptr [rbp - 8], xmm0
  call    c()@PLT
  movsd   xmm0, qword ptr [rbp - 8]       # xmm0 = mem[0],zero
  call    b(double)@PLT
  add     rsp, 16
  pop     rbp
  ret

... it's actually performing a partial spill of the lower parts of xmm0, a fact that is encoded in the opcode (movsd not mov) but missed by InstrRefBasedLDV. So there's an extra factor of "where does that information go missing" to build into a different patch.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1163
+  // size of value and the size of variable portion being read, on big endian
+  // systems. Consider:
+  //
----------------
Orlando wrote:
> Does a test for this part already exist? I assume so, since this patch is just narrowing the use cases rather than adding the functionally, but wanted to check to be sure.
Yup, it's `llvm/test/DebugInfo/MIR/InstrRef/deref-spills-with-size.mir` and tests pretty much every combination of sizes and derefs.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1175
+  unsigned ValueSizeInBits = getLocSizeInBits(MLoc);
+  unsigned DerefSizeInBytes = ValueSizeInBits / 8;
+  if (DerefSizeInBytes > PointerSizeBytes) {
----------------
aprantl wrote:
> Orlando wrote:
> > Can `ValueSizeInBits` ever be a value that doesn't divide cleanly by 8? e.g. 1 from an `i1` type (I haven't looked at how `getLocSizeInBits` works). 
> > 
> > If so then I think we might need to use `divideCeil` from `MathExtras.h` here. Otherwise, with the code as is, a `1` would result in `DerefSizeInBytes` of `0`.
> Would using a PointerSizeInBits instead help?
I suppose switching to using bits instead of bytes will remove a category of weirdness that can occur, will implement that,


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1177-1178
+  if (DerefSizeInBytes > PointerSizeBytes) {
+    // This would be illegal DWARF. Perhaps the output will identify a
+    // flawed location, but it'll at least be legal output.
+    return false;
----------------
dblaikie wrote:
> Could you clarify/rephrase the "output will identify a flawed location" - I don't understand what you mean by that & I'd have expected the failure here to be "absence of a location" rather than possibly incorrect locations ("flawed" I assume means "incorrect"?)
Improved wording would be "There's a risk this is incorrect on big endian systems", however I'm going to back off this for a moment, see main comment.


================
Comment at: llvm/test/DebugInfo/MIR/InstrRef/livedebugvalues_deref_size_too_big.mir:6
+# exceeds the size of an address (64 bits). We should produce a plain
+# DW_OP_deref instead, as that's a legal output.
+#
----------------
dblaikie wrote:
> How would a plain DW_OP_deref work here - it'd still only be word-wide, right - so 64 bits, but the low 64 bit of a 128bit floating point value presumably isn't a valid floating point value, right?
Hrrm. It's technically valid for x86 as the 128-bit-and-higher registers are all vectors, but baking that assumption in here guarantees it'll break something else. I'm going to back off from this for a moment (see main comment) and come back with a re-rationalised approach.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156545