[LiveDebugValues] Allow EntryValue with OP_deref expressions

[fdeazeve]

Summary

With D68945, more DBG_VALUEs were created without the indirect operand,
instead relying on OP_deref to accomplish the same effect.

At the time, however, we were not able to handle arbitrary expressions
in combination with OP_LLVM_entry_value, so D71416 prevented the use of
such operation in the presence of expressions.

As per the comment in DIExpression::isValid, "we support only entry
values of a simple register location." As such, a simple deref operation
should be supported. In fact, D80345 added support for indirect
DBG_VALUEs.

Taken the patches above into consideration, this commit relaxes the
restrictions on which expressions are allowed for entry value
candidates: the expression must be either empty or a single dereference
operator.

This patch is useful for D141381, which adds support for storing the
address of ABI-indirect parameters on the stack.

Depends on D142160

[fdeazeve]

I've tagged everyone who was involved one way or another in the previous patches related to this, hopefully I haven't missed anyone

[djtodoro]

Hello. I am wondering, have you checked how both `GDB`` and `LLDB`` act with this example?

[fdeazeve]

In D142654#4085122, @djtodoro wrote:

Hello. I am wondering, have you checked how both `GDB`` and `LLDB`` act with this example?

I believe we don't have much to check in this example, as there are no callers for the function here.
However, the test lldb/test/API/functionalities/param_entry_vals/basic_entry_values/ has a function with this exact behaviour (see func15).
It produces this dwarf for the callee:

0x0000065f:   DW_TAG_subprogram
                DW_AT_low_pc	(0x00000000000001f8)
                DW_AT_high_pc	(0x0000000000000228)
                DW_AT_frame_base	(DW_OP_reg29 W29)
                DW_AT_call_all_calls	(true)
                DW_AT_linkage_name	("_Z6func1537StructPassedViaPointerToTemporaryCopy")
                DW_AT_name	("func15")
                DW_AT_decl_file	("lldb/test/API/functionalities/param_entry_vals/basic_entry_values/main.cpp")
                DW_AT_decl_line	(157)
                DW_AT_external	(true)

0x00000678:     DW_TAG_formal_parameter
                  DW_AT_location	(0x000003ef: 
                     [0x00000000000001f8, 0x0000000000000204): DW_OP_breg0 W0+0
                     [0x0000000000000204, 0x0000000000000228): DW_OP_entry_value(DW_OP_reg0 W0))
                  DW_AT_name	("S")
                  DW_AT_decl_file	("lldb/test/API/functionalities/param_entry_vals/basic_entry_values/main.cpp")
                  DW_AT_decl_line	(157)
                  DW_AT_type	(0x00000749 "StructPassedViaPointerToTemporaryCopy")

And then, on the caller's side (main):

0x0000093d:     DW_TAG_call_site
                  DW_AT_call_origin	(0x0000065f)
                  DW_AT_call_return_pc	(0x00000000000002dc)

0x0000094a:       DW_TAG_call_site_parameter
                    DW_AT_location	(DW_OP_reg0 W0)
                    DW_AT_call_value	(DW_OP_breg31 WSP+8)

LLDB is able -- in fact this is what the test is testing -- to recover the parameter with those two pieces of information.

I have not checked whether GDB has support for this dwarf OP.

[djtodoro]

Yeah, GDB had implemented it as GNU extension even before LLDB.
All in all, I don't see any obstacle to use the entry_value for this case. GCC also produces the same location entry for the variable b.

Have you checked the InstrRefBased implementation of the LiveDebugValues Pass?

[jmorse]

LGTM, but I'll leave to @djtodoro to approve. Support in the other flavour of LiveDebugValues would indeed be appreciated.

[fdeazeve]

In D142654#4085821, @djtodoro wrote:

Have you checked the InstrRefBased implementation of the LiveDebugValues Pass?

Oops, I wrote that down as something I had to do prior to posting this but forgot to :(
Will update the patch shortly!

[fdeazeve]

Added support in InstrRefBased. Updated the existing test to test both
implementations.

[aprantl]

LGTM, but I agree with Djordje that we should also check that at least LLDB can support this. Maybe someone with access to GDB can comment with another data point.
It will be a fun project to mock up an unwinder in lldb/unittests/Expression/DWARFExpression test for this ;-)

llvm/test/DebugInfo/MIR/X86/dbgcall-site-reference.mir

20

This DWARF expression looked suspicious to me, but I think it's actually correct. IIUC, this will unwind the call stack push the value of reg4 onto the DWARF stack. Because the DWARF expression isn't a DW_OP_reg and also doesn't end with DW_OP_stack_value this is still a memory location.

[fdeazeve]

In D142654#4087149, @aprantl wrote:

LGTM, but I agree with Djordje that we should also check that at least LLDB can support this. Maybe someone with access to GDB can comment with another data point.
It will be a fun project to mock up an unwinder in lldb/unittests/Expression/DWARFExpression test for this ;-)

Just to make sure we're on the same page, this test is producing virtually the same dwarf as the one I posted previously, which I confirmed to work with LLDB, or is there something fundamentally different that I'm missing here?

[aprantl]

In D142654#4087249, @fdeazeve wrote:

In D142654#4087149, @aprantl wrote:

LGTM, but I agree with Djordje that we should also check that at least LLDB can support this. Maybe someone with access to GDB can comment with another data point.
It will be a fun project to mock up an unwinder in lldb/unittests/Expression/DWARFExpression test for this ;-)

Just to make sure we're on the same page, this test is producing virtually the same dwarf as the one I posted previously, which I confirmed to work with LLDB, or is there something fundamentally different that I'm missing here?

If you already confirmed that LLDB can deal with this correctly, that's great!

[djtodoro]

LGTM.

[jmorse]

LGTM3

[fdeazeve]

Rebased

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

public:
   bool isEntryValueVariable(const DebugVariable &Var,
                             const DIExpression *Expr) const {
     if (!Var.getVariable()->isParameter())
       return false;
 
     if (Var.getInlinedAt())
       return false;
 
-    if (Expr->getNumElements() > 0)
+    if (Expr->getNumElements() > 0 && !Expr->isDeref())
       return false;
 
     return true;
   }
 
   bool isEntryValueValue(const ValueIDNum &Val) const {
     // Must be in entry block (block number zero), and be a PHI / live-in value.
     if (Val.getBlock() || !Val.isPHI())

llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp

bool VarLocBasedLDV::isEntryValueCandidate(
   // parameter's DBG_VALUE may be described using a register defined by some
   // instruction in the entry block, in which case we shouldn't create an
   // entry value.
   if (DefinedRegs.count(MI.getDebugOperand(0).getReg()))
     return false;
 
   // TODO: Add support for parameters that have a pre-existing debug expressions
   // (e.g. fragments).
-  if (MI.getDebugExpression()->getNumElements() > 0)
+  // A simple deref expression is equivalent to an indirect debug value.
+  const DIExpression *Expr = MI.getDebugExpression();
+  if (Expr->getNumElements() > 0 && !Expr->isDeref())
     return false;
 
   return true;
 }
 
 /// Collect all register defines (including aliases) for the given instruction.
 static void collectRegDefs(const MachineInstr &MI, DefinedRegsSet &Regs,
                            const TargetRegisterInfo *TRI) {

llvm/test/DebugInfo/MIR/X86/dbgcall-site-reference.mir

 # RUN: llc -start-before=livedebugvalues -filetype=obj -o - %s | llvm-dwarfdump - | FileCheck %s
+# RUN: llc --force-instr-ref-livedebugvalues -start-before=livedebugvalues -filetype=obj -o - %s | llvm-dwarfdump - | FileCheck %s
 
 # Based on the following C++ code:
 # struct A { A(A &) {} };
 # struct B { A a; };
 # struct C { C(B); };
 # struct D : C { D(B); };
 # D::D(B b) : C(b) {}
 
-# Reproducer for PR44275. Ideally we should get an entry value location list
-# entry for the reference parameter b, but we are currently not able to do that
-# due to limitations in the DWARF emission code, which puts restrictions on the
-# DIExpression. For now verify that we don't crash when trying to add such an
-# entry value.
+# Reproducer for PR44275.
 
 # CHECK: DW_AT_location
 # CHECK-NEXT: [{{0x[0-9a-f]+}}, {{0x[0-9a-f]+}}): DW_OP_reg5 RDI
 # CHECK-NEXT: [{{0x[0-9a-f]+}}, {{0x[0-9a-f]+}}): DW_OP_GNU_entry_value(DW_OP_reg5 RDI), DW_OP_stack_value)
 # CHECK-NEXT: DW_AT_name    ("this")
 
 # CHECK: DW_AT_location
-# CHECK-NEXT: [0x0000000000000000, 0x0000000000000004): DW_OP_breg4 RSI+0)
-# TODO: Here we should ideally get a location list entry using an entry value.
+# CHECK-NEXT: [0x0000000000000000, 0x0000000000000004): DW_OP_breg4 RSI+0
+# CHECK-NEXT: [0x0000000000000004, 0x000000000000000b): DW_OP_GNU_entry_value(DW_OP_reg4 RSI))
 # CHECK-NEXT: DW_AT_name    ("b")
 
 --- |
   target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
   target triple = "x86_64-unknown-linux-gnu"
 
   %struct.D = type { i8 }
   %struct.B = type { %struct.A }