Support emitting complex expressions that include entry values

[aprantl]

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.

[vsk]

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]

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().

[djtodoro]

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]

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

300

mistake: we should not be

[djtodoro]

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

300

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

[djtodoro]

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]

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]

Cleaned up the attributes and default values in the testcase.

[vsk]

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.

[djtodoro]

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.

In D96559#2561228, @aprantl wrote:

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]

That would not be the first time.

[djtodoro]

thanks! Lgtm

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

287–288

+1

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

if ((HasComplexExpression || IsEmittingEntryValue) && DwarfRegs.size() > 1) {
     DwarfRegs.clear();
     LocationKind = Unknown;
     return false;
   }
 
   // Handle simple register locations. If we are supposed to emit
   // a call site parameter expression and if that expression is just a register
   // location, emit it with addBReg and offset 0, because we should emit a DWARF
   // expression representing a value, rather than a location.
-  if (!isMemoryLocation() && !HasComplexExpression &&
-      (!isParameterValue() || isEntryValue())) {
+  if ((!isParameterValue() && !isMemoryLocation() && !HasComplexExpression) ||
+      isEntryValue()) {
     for (auto &Reg : DwarfRegs) {
       if (Reg.DwarfRegNo >= 0)
         addReg(Reg.DwarfRegNo, Reg.Comment);
       addOpPiece(Reg.SubRegSize);
     }
 
-    if (isEntryValue())
+    if (isEntryValue()) {
       finalizeEntryValue();
 
-    if (isEntryValue() && !isIndirect() && !isParameterValue() &&
+      if (!isIndirect() && !isParameterValue() && !HasComplexExpression &&
           DwarfVersion >= 4)
         emitOp(dwarf::DW_OP_stack_value);
+    }
 
     DwarfRegs.clear();
     return true;
   }
 
   // Don't emit locations that cannot be expressed without DW_OP_stack_value.
   if (DwarfVersion < 4)
     if (any_of(ExprCursor, [](DIExpression::ExprOperand Op) -> bool {

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

+# RUN: llc -filetype=obj -start-after=livedebugvalues -verify-machineinstrs -march=x86-64 -o - %s | llvm-dwarfdump - | FileCheck %s
+# CHECK: DW_AT_location (DW_OP_entry_value(DW_OP_reg5 RDI), DW_OP_constu 0xffffffff, DW_OP_and, DW_OP_lit0, DW_OP_plus)
+--- |
+  ; ModuleID = '/tmp/e.c'
+  source_filename = "/tmp/e.c"
+  target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+  target triple = "x86_64-apple-macosx11.0.0"
+  
+  ; Function Attrs: norecurse nounwind readnone ssp uwtable willreturn
+  define dso_local void @f(i32 %i) local_unnamed_addr #0 !dbg !8 {
+  entry:
+    call void @llvm.dbg.value(metadata i32 %i, metadata !14, metadata !DIExpression()), !dbg !15
+    ret void, !dbg !16
+  }
+  
+  ; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
+  declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+  
+  attributes #0 = { norecurse nounwind readnone ssp uwtable willreturn }
+  attributes #1 = { nofree nosync nounwind readnone speculatable willreturn }
+  
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!3, !4, !5, !6}
+  
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, emissionKind: FullDebug)
+  !1 = !DIFile(filename: "/tmp/e.c", directory: "/")
+  !2 = !{}
+  !3 = !{i32 7, !"Dwarf Version", i32 4}
+  !4 = !{i32 2, !"Debug Info Version", i32 3}
+  !5 = !{i32 1, !"wchar_size", i32 4}
+  !6 = !{i32 7, !"PIC Level", i32 2}
+  !8 = distinct !DISubprogram(name: "f", scope: !9, file: !9, line: 1, type: !10, scopeLine: 1, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !13)
+  !9 = !DIFile(filename: "/tmp/e.c", directory: "")
+  !10 = !DISubroutineType(types: !11)
+  !11 = !{null, !12}
+  !12 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !13 = !{!14}
+  !14 = !DILocalVariable(name: "i", arg: 1, scope: !8, file: !9, line: 1, type: !12)
+  !15 = !DILocation(line: 0, scope: !8)
+  !16 = !DILocation(line: 1, column: 16, scope: !8)
+
+...
+---
+name:            f
+body:             |
+  bb.0.entry:
+    DBG_VALUE $edi, $noreg, !14, !DIExpression(DW_OP_LLVM_entry_value, 1, DW_OP_lit0, DW_OP_plus), debug-location !15
+    frame-setup PUSH64r killed $rbp, implicit-def $rsp, implicit $rsp
+    $rbp = frame-setup MOV64rr $rsp
+    $rbp = frame-destroy POP64r implicit-def $rsp, implicit $rsp, debug-location !16
+    RETQ debug-location !16
+
+...