[DebugInfo] Do not emit entry values for composite locations

[dstenb]

Summary

This is a fix for PR45009.

When working on D67492 I made DwarfExpression emit a single
DW_OP_entry_value operation covering the whole composite location
description that is produced if a register does not have a valid DWARF
number, and is instead composed of multiple register pieces. Looking
closer at the standard, I realized that that is not valid DWARF. A
DW_OP_entry_value operation's block can only be a DWARF expression or a
register location description, so it is not valid for it to hold a
composite location description like that.

See DWARFv5 sec. 2.5.1.7:

"The DW_OP_entry_value operation pushes the value that the described
location held upon entering the current subprogram. It has two
operands: an unsigned LEB128 length, followed by a block containing a
DWARF expression or a register location description (see Section
2.6.1.1.3 on page 39)."

Perhaps we instead want to emit a entry value operation per each
DW_OP_reg* operation, e.g.:

• DW_OP_entry_value(DW_OP_regx sub_reg0), DW_OP_stack_value, DW_OP_piece 8,
• DW_OP_entry_value(DW_OP_regx sub_reg1), DW_OP_stack_value, DW_OP_piece 8, [...]

The question then becomes how the call site should look; should a
composite location description be emitted there, and we then leave it up
to the debugger to match those two composite location descriptions?
Another alternative could be to emit a call site parameter entry for
each sub-register, but firstly I'm unsure if that is even valid DWARF,
and secondly it seems like that would complicate the collection of call
site values quite a bit. As far as I can tell GCC does not emit any
entry values / call sites in these cases, so we do not have something to
compare with, but the former seems like the more reasonable approach.

Currently when trying to emit a call site entry for a parameter composed
of multiple DWARF registers a (DwarfRegs.size() == 1) assert is
triggered in addMachineRegExpression(). Until the call site
representation is figured out, and until there is use for these entry
values in practice, this commit simply stops the invalid DWARF from
being emitted.

[vsk]

Fixing the assertion failure in this way sgtm. Re:

DW_OP_entry_value(DW_OP_regx sub_reg0), DW_OP_stack_value, DW_OP_piece 8, DW_OP_entry_value(DW_OP_regx sub_reg1), DW_OP_stack_value, DW_OP_piece 8, ...

It sounds like you expect debuggers to have some difficulty handling this expression. How come? I didn't understand this part.

My lack of familiarity with DWARF is showing now, but would DW_OP_entry_value(DW_OP_regx sub_reg0, DW_OP_piece 8, DW_OP_regx sub_reg1, DW_OP_piece 8) be valid (sorry if I've left out any necessary OP_stack_values)?

The question then becomes how the call site should look; should a
composite location description be emitted there, and we then leave it up
to the debugger to match those two composite location descriptions?

This seems reasonable to me, but I feel like I'm missing something here..

Another alternative could be to emit a call site parameter entry for
each sub-register,

I believe this would complicate the debugger support significantly, hopefully there's a simpler way to do this. Right now lldb just does a 'memcmp' to figure out whether to use an entry value. If there were a separate call site parameter entry for each subregister in the caller that forms a register used in the callee, that would have to change into a more complicated merging operation.

[djtodoro]

In D75270#1897102, @vsk wrote:

Another alternative could be to emit a call site parameter entry for
each sub-register,

I believe this would complicate the debugger support significantly, hopefully there's a simpler way to do this.

+1

In D75270, @dstenb wrote:

As far as I can tell GCC does not emit any
entry values / call sites in these cases, so we do not have something to
compare with, but the former seems like the more reasonable approach.

I think as well. What about GDB side? Did GDB expect something like this? I though just running GDB over such case could be interesting to see if it accepts such scenario or not (I should find some time to play with that for sure).

[dstenb]

In D75270#1897396, @djtodoro wrote:

As far as I can tell GCC does not emit any
entry values / call sites in these cases, so we do not have something to
compare with, but the former seems like the more reasonable approach.

I think as well. What about GDB side? Did GDB expect something like this? I though just running GDB over such case could be interesting to see if it accepts such scenario or not (I should find some time to play with that for sure).

GDB currently imposes the following restrictions for call site locations:

Only single DW_OP_reg or DW_OP_fbreg is supported for DW_FORM_block* DW_AT_location is supported for DW_TAG_call_site child DIE

and the following for entry value operations:

DWARF-2 expression error: DW_OP_entry_value is supported only for single DW_OP_reg* or for DW_OP_breg*(0)+DW_OP_deref*

[djtodoro]

In D75270#1897552, @dstenb wrote:

In D75270#1897396, @djtodoro wrote:

As far as I can tell GCC does not emit any
entry values / call sites in these cases, so we do not have something to
compare with, but the former seems like the more reasonable approach.

I think as well. What about GDB side? Did GDB expect something like this? I though just running GDB over such case could be interesting to see if it accepts such scenario or not (I should find some time to play with that for sure).

GDB currently imposes the following restrictions for call site locations:

Only single DW_OP_reg or DW_OP_fbreg is supported for DW_FORM_block* DW_AT_location is supported for DW_TAG_call_site child DIE

and the following for entry value operations:

DWARF-2 expression error: DW_OP_entry_value is supported only for single DW_OP_reg* or for DW_OP_breg*(0)+DW_OP_deref*

Thanks for this! I've expected that... I've confirmed this output with an older version of GDB 7.1* as well. We will need to discuss about adding such support to debuggers.

[dstenb]

In D75270#1897102, @vsk wrote:

Fixing the assertion failure in this way sgtm. Re:

Sorry, coming back to this now after the similar discussion in D75326.

I just want to clarify that this patch does not fix any assertion failures. The entry value operation is emitted without crashing; it's just that I think that it's invalid DWARF. The assertion I mentioned in the description was for the DW_AT_location at the call site parameter entry (but see my correction about that at the end of this comment).

DW_OP_entry_value(DW_OP_regx sub_reg0), DW_OP_stack_value, DW_OP_piece 8, DW_OP_entry_value(DW_OP_regx sub_reg1), DW_OP_stack_value, DW_OP_piece 8, ...

It sounds like you expect debuggers to have some difficulty handling this expression. How come? I didn't understand this part.

I don't know how much about how the call site and entry value matching works and is implemented in the debuggers, but I thought there could be some difficulties to understand that the each sub-register's entry value at the callee's location:

DW_OP_entry_value(DW_OP_regx sub_reg0), DW_OP_stack_value, DW_OP_piece 8, DW_OP_entry_value(DW_OP_regx sub_reg1), DW_OP_stack_value, DW_OP_piece 8, ...

should be matched to the corresponding piece of the DW_AT_location for the call site's single parameter entry:

DW_OP_regx sub_reg0, DW_OP_piece 8, DW_OP_regx sub_reg1, DW_OP_stack_value, DW_OP_piece 8, ...

Then, if we e.g. have a DW_OP_constu as the DW_AT_call_value for that call site parameter entry, I assume that the debugger has to split up that value into different parts according to the DW_AT_location composition, and push those to the DWARF stacks for the entry values in the different pieces?

Again, as I don't know much about the debuggers' implementations, maybe nothing of that is an issue really?

My lack of familiarity with DWARF is showing now, but would DW_OP_entry_value(DW_OP_regx sub_reg0, DW_OP_piece 8, DW_OP_regx sub_reg1, DW_OP_piece 8) be valid (sorry if I've left out any necessary OP_stack_values)?

That is what we currently emit (with a stack value after the DW_OP_entry_value). I think that's invalid DWARF since the standard specifies that the DW_OP_entry_value operation's block is "containing a DWARF expression or a register location description", so I don't think it's valid to emit a composite location description like that one.

And sorry, I noticed something incorrect in my patch description:

In D75270, @dstenb wrote:

Currently when trying to emit a call site entry for a parameter composed
of multiple DWARF registers a (DwarfRegs.size() == 1) assert is
triggered in addMachineRegExpression(). Until the call site
representation is figured out, and until there is use for these entry
values in practice, this commit simply stops the invalid DWARF from
being emitted.

The call site entry's DW_AT_location is emitted fine for composite locations, so what I'm describing there does not happen for those cases. However, if you were to describe the call site value using a preserved register, which is composed of multiple DWARF register pieces, you'll currently hit that assertion (code here: [0]) since isParameterValue() is true.

[0] https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp#L279

[djtodoro]

Hi @dstenb,

What is the status with this?

[dstenb]

In D75270#2012177, @djtodoro wrote:

Hi @dstenb,

What is the status with this?

Sorry for leaving this!

This related a bit to D75326, and there I sent a mail to the Dwarf-Discuss mailing thread (http://lists.dwarfstd.org/pipermail/dwarf-discuss-dwarfstd.org/2020-March/004610.html) about whether or not composite locations are allowed in DWARF Expressions.

@vsk, do you think it would be okay for you if we land this patch based on the similar reasoning as in you wrote in D75326? I mean this:

Reading through the discussion, I do see some amount of consensus about the first sentence of section 2.5 being misleading (or at least confusing) -- this is the bit I cited in argument for allowing composite locations wherever a value is expected. I think we'd need a strong reason to disregard that consensus.

[djtodoro]

ping :)

[vsk]

@dstenb thanks for kicking off the thread on dwarf-discuss, and sorry for the delay here. Please go ahead, it looks like there's strong consensus that a composite location description can't be emitted where a DWARF expression or simple register location is expected.

[dstenb]

Thanks, and sorry for letting this patch lay dormant. I'll land this tomorrow when I have time for the build bots.

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h

protected:
   /// heuristics to disambiguate the value vs. location status of the
   /// expression.  See PR21176 for more details.
   void addStackValue();
 
   /// Finalize an entry value by emitting its size operand, and committing the
   /// DWARF block which has been emitted to the temporary buffer.
   void finalizeEntryValue();
 
+  /// Cancel the emission of an entry value.
+  void cancelEntryValue();
+
   ~DwarfExpression() = default;
 
 public:
   DwarfExpression(unsigned DwarfVersion, DwarfCompileUnit &CU)
       : CU(CU), SubRegisterSizeInBits(0), SubRegisterOffsetInBits(0),
         LocationKind(Unknown), LocationFlags(Unknown),
         DwarfVersion(DwarfVersion) {}
 

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

bool DwarfExpression::addMachineRegExpression(const TargetRegisterInfo &TRI,
   bool HasComplexExpression = false;
   auto Op = ExprCursor.peek();
   if (Op && Op->getOp() != dwarf::DW_OP_LLVM_fragment)
     HasComplexExpression = true;
 
   // If the register can only be described by a complex expression (i.e.,
   // multiple subregisters) it doesn't safely compose with another complex
   // expression. For example, it is not possible to apply a DW_OP_deref
-  // operation to multiple DW_OP_pieces.
-  if (HasComplexExpression && DwarfRegs.size() > 1) {
+  // operation to multiple DW_OP_pieces, since composite location descriptions
+  // do not push anything on the DWARF stack.
+  //
+  // DW_OP_entry_value operations can only hold a DWARF expression or a
+  // register location description, so we can't emit a single entry value
+  // covering a composite location description. In the future we may want to
+  // emit entry value operations for each register location in the composite
+  // location, but until that is supported do not emit anything.
+  if ((HasComplexExpression || IsEmittingEntryValue) && DwarfRegs.size() > 1) {
+    if (IsEmittingEntryValue)
+      cancelEntryValue();
     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
void DwarfExpression::beginEntryValueExpression(
   (void)Op;
   assert(Op && Op->getOp() == dwarf::DW_OP_LLVM_entry_value);
   assert(!isMemoryLocation() &&
          "We don't support entry values of memory locations yet");
   assert(!IsEmittingEntryValue && "Already emitting entry value?");
   assert(Op->getArg(0) == 1 &&
          "Can currently only emit entry values covering a single operation");
 
-  emitOp(CU.getDwarf5OrGNULocationAtom(dwarf::DW_OP_entry_value));
   IsEmittingEntryValue = true;
   enableTemporaryBuffer();
 }
 
 void DwarfExpression::finalizeEntryValue() {
   assert(IsEmittingEntryValue && "Entry value not open?");
   disableTemporaryBuffer();
 
+  emitOp(CU.getDwarf5OrGNULocationAtom(dwarf::DW_OP_entry_value));
+
   // Emit the entry value's size operand.
   unsigned Size = getTemporaryBufferSize();
   emitUnsigned(Size);
 
   // Emit the entry value's DWARF block operand.
   commitTemporaryBuffer();
 
   IsEmittingEntryValue = false;
 }
 
+void DwarfExpression::cancelEntryValue() {
+  assert(IsEmittingEntryValue && "Entry value not open?");
+  disableTemporaryBuffer();
+
+  // The temporary buffer can't be emptied, so for now just assert that nothing
+  // has been emitted to it.
+  assert(getTemporaryBufferSize() == 0 &&
+         "Began emitting entry value block before cancelling entry value");
+
+  IsEmittingEntryValue = false;
+}
+
 unsigned DwarfExpression::getOrCreateBaseType(unsigned BitSize,
                                               dwarf::TypeKind Encoding) {
   // Reuse the base_type if we already have one in this CU otherwise we
   // create a new one.
   unsigned I = 0, E = CU.ExprRefedBaseTypes.size();
   for (; I != E; ++I)
     if (CU.ExprRefedBaseTypes[I].BitSize == BitSize &&
         CU.ExprRefedBaseTypes[I].Encoding == Encoding)
default:
       return false;
     }
   }
   return true;
 }
 
 void DwarfExpression::addExpression(DIExpressionCursor &&ExprCursor,
                                     unsigned FragmentOffsetInBits) {
+  // Entry values can currently only cover the initial register location,
+  // and not any other parts of the following DWARF expression.
+  assert(!IsEmittingEntryValue && "Can't emit entry value around expression");
+
   // If we need to mask out a subregister, do it now, unless the next
   // operation would emit an OpPiece anyway.
   auto N = ExprCursor.peek();
   if (SubRegisterSizeInBits && N && (N->getOp() != dwarf::DW_OP_LLVM_fragment))
     maskSubRegister();
 
   Optional<DIExpression::ExprOperand> PrevConvertOp = None;
 

llvm/test/DebugInfo/Sparc/entry-value-complex-reg-expr.ll

 ; RUN: llc -debug-entry-values -filetype=asm -o - %s | FileCheck %s
 
-; Verify that the entry value covers both of the DW_OP_regx pieces. Previously
-; the size operand of the entry value would be hardcoded to one.
+; The q0 register does not have a DWARF register number, and is instead emitted
+; as a composite location description with two sub-registers. Previously we
+; emitted a single DW_OP_entry_value wrapping that whole composite location
+; description, but that is not valid DWARF; DW_OP_entry_value operations can
+; only hold DWARF expressions and register location descriptions.
 ;
-; XXX: Is this really what should be emitted, or should we instead emit one
-; entry value operation per DW_OP_regx? GDB can currently not understand
-; entry values containing complex expressions like this.
+; In the future we may want to emit a composite location description where each
+; DW_OP_regx operation is wrapped in an entry value operation, but for now
+; just verify that no invalid DWARF is emitted.
 
 target datalayout = "E-m:e-i64:64-n32:64-S128"
 target triple = "sparc64"
 
 ; Based on the following C reproducer:
 ;
 ; volatile long double global;
 ; extern void clobber();
 ; int foo(long double p) {
 ;   global = p;
 ;   clobber();
 ;   return 123;
 ; }
 
-; CHECK:      .byte   243       ! DW_OP_GNU_entry_value
-; CHECK-NEXT: .byte   8         ! 8
+; Verify that we got an entry value in the DIExpression...
+; CHECK: DEBUG_VALUE: foo:p <- [DW_OP_LLVM_entry_value 1] $q0
+
+; ... but that no entry value location was emitted:
+; CHECK:      .half   8         ! Loc expr size
 ; CHECK-NEXT: .byte   144       ! sub-register DW_OP_regx
 ; CHECK-NEXT: .byte   72        ! 72
 ; CHECK-NEXT: .byte   147       ! DW_OP_piece
 ; CHECK-NEXT: .byte   8         ! 8
 ; CHECK-NEXT: .byte   144       ! sub-register DW_OP_regx
 ; CHECK-NEXT: .byte   73        ! 73
 ; CHECK-NEXT: .byte   147       ! DW_OP_piece
 ; CHECK-NEXT: .byte   8         ! 8
-; CHECK-NEXT: .byte   159       ! DW_OP_stack_value
+; CHECK-NEXT: .xword  0
+; CHECK-NEXT: .xword  0
 
 @global = common global fp128 0xL00000000000000000000000000000000, align 16, !dbg !0
 
 ; Function Attrs: nounwind
 define signext i32 @foo(fp128 %p) #0 !dbg !12 {
 entry:
   call void @llvm.dbg.value(metadata fp128 %p, metadata !17, metadata !DIExpression()), !dbg !18
   store volatile fp128 %p, fp128* @global, align 16, !dbg !19