[PATCH] D73526: [SafeStack][DebugInfo] Insert DW_OP_deref in correct location

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:758
+    MIB.addImm(0U);
+  else
+    MIB.addReg(0U, RegState::Debug);
----------------
aprantl wrote:
> leonardchan wrote:
> > aprantl wrote:
> > > This is surprising to me. Can you walk me through this as a sanity check to make sure this isn't papering over some other issue?
> > > 
> > > Both the before and after state should be equivalent:
> > > Before your patch we emit a direct register location + DW_OP_deref afterwards we emit an indirect register location.
> > > 
> > > What am I missing?
> > I'll admit I'm not an expert in this area of LLVM, but my understanding is that this would affect the order of the expression stack.
> > 
> > Before my patch, if this value was an indirection and the expression was `DW_OP_constu, 8, DW_OP_minus`, then we would append `DW_OP_deref` to the end of the stack and we would not actually deref the value immediately.
> > 
> > With this patch (which is essentially a revert of https://reviews.llvm.org/D68945#change-VaZel0Y6QQdl), the indirection happens first, then the expressions on the stack.
> > 
> > This would essentially turn something like:
> > 
> > ```
> > DW_OP_breg6 RBP-40, DW_OP_constu 0x20, DW_OP_minus, DW_OP_deref
> > ```
> > 
> > into
> > 
> > ```
> > DW_OP_breg6 RBP-40, DW_OP_deref, DW_OP_constu 0x20, DW_OP_minus
> > ```
> I see. So this should have been DIExpression::prepend instead of append? Could we implement it that way instead?
Looking at pre-emission DBG_VALUEs, I see without this patch:

    DIExpression(DW_OP_constu, 40, DW_OP_minus, DW_OP_constu, 32, DW_OP_minus, DW_OP_deref)

And with this patch:

    DIExpression(DW_OP_constu, 40, DW_OP_minus, DW_OP_deref, DW_OP_constu, 32, DW_OP_minus)

Which correctly gets the deref in the middle of the minuses. I think with prepend we would end up with:

    DIExpression(DW_OP_deref, DW_OP_constu, 40, DW_OP_minus, DW_OP_constu, 32, DW_OP_minus)

Which would still be broken.

More generally: D68945 was supposed to eliminate pre-regalloc uses of IsIndirect, and D69178 repurpose the field. With both applied this fault doesn't occur, however I didn't get around to landing D69178. Seeing how D68945 is in the release-10 branch and is unexpectedly broken, it might be better to:
 * Fully revert D68945 and add the regression test
 * cherry-pick the revert into release-10
 * I'll re-land D68945 when D69178 is ready (there were some increased sizes of location lists that need looking at).
Opinions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73526