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

[Petr Hosek via Phabricator via llvm-commits]

phosek accepted this revision.
phosek added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:758
+    MIB.addImm(0U);
+  else
+    MIB.addReg(0U, RegState::Debug);
----------------
leonardchan wrote:
> 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.
We've tried reverting D68945, but since that one landed some time ago, it doesn't revert cleanly and it seems like we would need to revert multiple changes that landed after. Given that, and the fact that D69178 is not ready yet, I'd really prefer to go ahead and land this change to unblock SafeStack users, and land D69178 when ready as a proper solution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73526