[PATCH] D96559: Support emitting complex expressions that include entry values

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added a comment.

In D96559#2561263 <https://reviews.llvm.org/D96559#2561263>, @vsk wrote:

> It seems reasonable to me to split out AsmPrinter support for complex entry values from the change(s) to start emitting them. @djtodoro any concerns?

I completely agree :) And it is safe.

In D96559#2561228 <https://reviews.llvm.org/D96559#2561228>, @aprantl wrote:

> Cleaned up the attributes and default values in the testcase.

I assume you pushed a wrong patch? :)



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp:300
+      if (!isIndirect() && !isParameterValue() && !HasComplexExpression &&
+          +DwarfVersion >= 4)
+        emitOp(dwarf::DW_OP_stack_value);
----------------
aprantl wrote:
> djtodoro wrote:
> > djtodoro wrote:
> > > djtodoro wrote:
> > > > djtodoro wrote:
> > > > > vsk wrote:
> > > > > > Stray +?
> > > > > `DW_OP_entry_value` could be used in the expressions representing `DW_AT_call_value` within `DW_TAG_call_site_parameter`, and I think we should be printing the `DW_OP_stack_value` in these.
> > > > mistake: we **should not** be
> > > Oh I overlooked it, it is still checking `!isParameterValue()`. This looks good to me...
> > I am just wondering, do we need the changes from D87357 regarding the `DwarfExpression::addExpression()`?
> > e.g.:
> > 
> >     case dwarf::DW_OP_plus_uconst:
> >     -  assert(!isRegisterLocation());
> >     +  assert(!isRegisterLocation() || isEntryValue());
> >       emitOp(dwarf::DW_OP_plus_uconst);
> >       emitUnsigned(Op->getArg(0));
> >       break;
> For the very narrow use-case I am interested in (https://reviews.llvm.org/D96549) this doesn't have effect, but it won't hurt when that patch lands either.
Sure. That sounds good to me.


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

https://reviews.llvm.org/D96559