Page MenuHomePhabricator

Support emitting complex expressions that include entry values
ClosedPublic

Authored by aprantl on Feb 11 2021, 4:10 PM.

Details

Summary

This patch enables AsmPrinter support for complex expression with entry values. It shouldn't AsmPrinter's call whether these are safe or not but the pass who introduces the DW_OP_LLVM_entry_value. This patch on its own has no effect on clang.

Diff Detail

Event Timeline

aprantl created this revision.Feb 11 2021, 4:10 PM
aprantl requested review of this revision.Feb 11 2021, 4:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2021, 4:10 PM
vsk added a reviewer: dstenb.Feb 11 2021, 4:21 PM

By no effect on clang, do you mean on the debug info for a stage2 clang binary?

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
300

Stray +?

llvm/test/DebugInfo/MIR/X86/complex-entryvalue.mir
86

Interesting, is 1 code for %rdi? That might be worth a note.

dstenb added inline comments.Feb 12 2021, 2:27 AM
llvm/test/DebugInfo/MIR/X86/complex-entryvalue.mir
86

The 1 is for the number of expression operands (including the DBG_VALUE value operand, i.e. in this case $edi) that the DW_OP_LLVM_entry_value covers. The actual byte size operand of the to-be-emitted DW_OP_entry_value operation is then calculated by getTemporaryBufferSize() in finalizeEntryValue().

This has no effect to llvm/clang, since we support simple register locations as entry values only. D87233 and D87357 introduce complex-entry-values.

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
300

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.

llvm/test/DebugInfo/MIR/X86/complex-entryvalue.mir
47–83

NIt: we can get rid of these

djtodoro added inline comments.Feb 12 2021, 4:28 AM
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
300

mistake: we should not be

djtodoro added inline comments.Feb 12 2021, 4:53 AM
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
300

Oh I overlooked it, it is still checking !isParameterValue(). This looks good to me...

djtodoro added inline comments.Feb 12 2021, 7:56 AM
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
300

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;
aprantl marked 8 inline comments as done.Feb 12 2021, 4:07 PM
aprantl added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
300

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.

aprantl updated this revision to Diff 323495.Feb 12 2021, 4:08 PM
aprantl marked an inline comment as done.

Cleaned up the attributes and default values in the testcase.

vsk added a comment.Feb 12 2021, 4:20 PM

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?

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
287–288

Maybe expand the comment with:
+ // Also handle the start of an entry value expression, even if it's complex.

llvm/test/DebugInfo/MIR/X86/complex-entryvalue.mir
86

Sorry about that, I inexplicably skipped over the $edi operand on my first pass through.

In 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.

Cleaned up the attributes and default values in the testcase.

I assume you pushed a wrong patch? :)

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
300

Sure. That sounds good to me.

aprantl updated this revision to Diff 323502.Feb 12 2021, 4:58 PM

That would not be the first time.

djtodoro accepted this revision.Feb 12 2021, 5:11 PM

thanks! Lgtm

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
287–288

+1

This revision is now accepted and ready to land.Feb 12 2021, 5:11 PM