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

[Leonard Chan via Phabricator via llvm-commits]

leonardchan marked an inline comment as done.
leonardchan added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:758
+    MIB.addImm(0U);
+  else
+    MIB.addReg(0U, RegState::Debug);
----------------
jmorse wrote:
> 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?
Thanks for checking. For our purposes, we don't mind just landing this patch, or landing D69178 on top of D68945, as long as the regression is addressed relatively quickly upstream.

If this is urgent for other people or others need this fix in the the next release, then reverting, cherry-picking, and relanding with the regression test sounds appropriate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73526