[LiveDebugValues] Omit entry values for DBG_VALUEs with pre-existing expressions

[dstenb]

Summary

Entry values are currently only supported for register DBG_VALUEs
with empty debug expressions, as the DW_OP_entry_value operation can
only wrap one byte. Creating an entry value for a DBG_VALUE with a
non-empty debug expression would result in an invalid expression.
This occurred for the parameter in the attached test case,
triggering an assert later on.

[vsk]

Thanks for the patch. Could you elaborate on what you mean by OP_entry_value only being able to wrap a single byte? Skimming the implementation, the impression I get is that a full machine-reg-expression can be attached to an OP_entry_value (see: DwarfCompileUnit::addComplexAddress).

llvm/test/DebugInfo/Mips/entry-value-non-empty-expr.ll

25

Not sure if it's worth it here, but in general I find FileCheck's '-implicit-check-not=<pattern>' feature useful in situations like this.

[djtodoro]

Thanks for the patch.
We wanted to avoid any complex DIExpression here, so this looks good. In order to support it here, we need to add fully support within the AsmPrinter (DwarfDebug, DwarfExpression, DwarfCompileUnit) and then enable it here.

llvm/test/DebugInfo/Mips/entry-value-non-empty-expr.ll

50

I think we don't need the attributes.

[dstenb]

In D66746#1645802, @vsk wrote:

Thanks for the patch. Could you elaborate on what you mean by OP_entry_value only being able to wrap a single byte? Skimming the implementation, the impression I get is that a full machine-reg-expression can be attached to an OP_entry_value (see: DwarfCompileUnit::addComplexAddress).

The lang ref describes the DW_OP_entry_value operation in DIExpression as:

If an expression is marked with DW_OP_entry_value all register and memory read operations refer to the respective value at the function entry. The first operand of DW_OP_entry_value is the size of following DWARF expression. DW_OP_entry_value may appear after the LiveDebugValues pass. LLVM only supports entry values for function parameters that are unmodified throughout a function and that are described as simple register location descriptions. DW_OP_entry_value may also appear after the AsmPrinter pass when a call site parameter value (DW_AT_call_site_parameter_value) is represented as entry value of the parameter.

I have interpreted the size as meaning the byte size of the DWARF block that the operation will cover. Assuming that, at the time of running LiveDebugValues I don't think there is a good way to query the size of the block that the entry value will cover; we don't know that until we actually emit the DWARF, as far as I can tell. That is why I have assumed that a hard coded operand of 1 is emitted there, with the assumption that only simple register location descriptions are supported.

However, I now got uncertain when looking at prependOpcodes() which is used to add the operation to the DIExpression:

Ops.push_back(dwarf::DW_OP_entry_value);
// Add size info needed for entry value expression.
// Add plus one for target register operand.
Ops.push_back(Expr->getNumElements() + 1);

As seen, there the number of pre-existing elements plus one is used. I don't think the number of elements does not map one-to-one with the byte size of the DWARF block, so I'm not sure how to interpret that. Can you help me understand what the operand in the DIExpression world indicates, @djtodoro?

As far as I understand, we now emit the operation from the DIExpression as-is in the DWARF. That means that if we have a register that turns into a complex expression we will still say that the size of that expression is one byte. I have seen such cases with our downstream target, but I'll see if I can trigger that behavior with an upstream target with a source level reproducer.

[djtodoro]

I have interpreted the size as meaning the byte size of the DWARF block that the operation will cover. Assuming that, at the time of running LiveDebugValues I don't think there is a good way to query the size of the block that the entry value will cover; we don't know that until we actually emit the DWARF, as far as I can tell. That is why I have assumed that a hard coded operand of 1 is emitted there, with the assumption that only simple register location descriptions are supported.

However, I now got uncertain when looking at prependOpcodes() which is used to add the operation to the DIExpression:

Ops.push_back(dwarf::DW_OP_entry_value);
Add size info needed for entry value expression.
Add plus one for target register operand.
Ops.push_back(Expr->getNumElements() + 1);

As seen, there the number of pre-existing elements plus one is used. I don't think the number of elements does not map one-to-one with the byte size of the DWARF block, so I'm not sure how to interpret that. Can you help me understand what the operand in the DIExpression world indicates, @djtodoro?

I think your point is right. We wanted to have there hard-coded value 1 for the size of following expression. Except if we did not cover all the cases where we should avoid complex expressions, we always generate an entry value expression with an empty pre-existing DIExpression, so we assumed that this code will cover current situation and may be extended to support complex debug expressions as well. But, I also think it is hard to distinguish the size of a complex DIExpression until DWARF being printed, so maybe we can change the code to Ops.push_back(1); and put some kind of assertion there.

As far as I understand, we now emit the operation from the DIExpression as-is in the DWARF. That means that if we have a register that turns into a complex expression we will still say that the size of that expression is one byte. I have seen such cases with our downstream target, but I'll see if I can trigger that behavior with an upstream target with a source level reproducer.

If you can produce such scenario it will be desirable. We enabled the feature in this initial stage only for x86 targets, and tried to cover all the situations found for the target. We meant to cover all the places where we should avoid generation of debug entry values with complex expressions (now). Eventually, we should handle all types of expressions.

[aprantl]

I have interpreted the size as meaning the byte size of the DWARF block that the operation will cover. Assuming that, at the time of running LiveDebugValues I don't think there is a good way to query the size of the block that the entry value will cover; we don't know that until we actually emit the DWARF, as far as I can tell. That is why I have assumed that a hard coded operand of 1 is emitted there, with the assumption that only simple register location descriptions are supported.

I think it would be reasonable to use the number of opcodes in the DIExpression in LLVM IR and only substitute the number of bytes in AsmPrinter.

[probinson]

In D66746#1647732, @aprantl wrote:

I have interpreted the size as meaning the byte size of the DWARF block that the operation will cover. Assuming that, at the time of running LiveDebugValues I don't think there is a good way to query the size of the block that the entry value will cover; we don't know that until we actually emit the DWARF, as far as I can tell. That is why I have assumed that a hard coded operand of 1 is emitted there, with the assumption that only simple register location descriptions are supported.

I think it would be reasonable to use the number of opcodes in the DIExpression in LLVM IR and only substitute the number of bytes in AsmPrinter.

+1. The DIExpression is more of an abstracted expression than a true DWARF expression, and even supports non-DWARF opcodes e.g. DW_OP_LLVM_convert. We can't be treating this as the size of the final expression.

[dstenb]

In D66746#1648864, @probinson wrote:

In D66746#1647732, @aprantl wrote:

I have interpreted the size as meaning the byte size of the DWARF block that the operation will cover. Assuming that, at the time of running LiveDebugValues I don't think there is a good way to query the size of the block that the entry value will cover; we don't know that until we actually emit the DWARF, as far as I can tell. That is why I have assumed that a hard coded operand of 1 is emitted there, with the assumption that only simple register location descriptions are supported.

I think it would be reasonable to use the number of opcodes in the DIExpression in LLVM IR and only substitute the number of bytes in AsmPrinter.

+1. The DIExpression is more of an abstracted expression than a true DWARF expression, and even supports non-DWARF opcodes e.g. DW_OP_LLVM_convert. We can't be treating this as the size of the final expression.

That sounds like a reasonable approach. I can start looking on some patches for that, if no one objects.

Even if the entry value could wrap multiple operations, we would still not want LDV to emit an entry value for the parameter in the attached test case, since the parameter value has been propagated to the callee, but I guess that's another question.

[dstenb]

In D66746#1646665, @djtodoro wrote:

I have interpreted the size as meaning the byte size of the DWARF block that the operation will cover. Assuming that, at the time of running LiveDebugValues I don't think there is a good way to query the size of the block that the entry value will cover; we don't know that until we actually emit the DWARF, as far as I can tell. That is why I have assumed that a hard coded operand of 1 is emitted there, with the assumption that only simple register location descriptions are supported.

However, I now got uncertain when looking at prependOpcodes() which is used to add the operation to the DIExpression:

Ops.push_back(dwarf::DW_OP_entry_value);
Add size info needed for entry value expression.
Add plus one for target register operand.
Ops.push_back(Expr->getNumElements() + 1);

As seen, there the number of pre-existing elements plus one is used. I don't think the number of elements does not map one-to-one with the byte size of the DWARF block, so I'm not sure how to interpret that. Can you help me understand what the operand in the DIExpression world indicates, @djtodoro?

I think your point is right. We wanted to have there hard-coded value 1 for the size of following expression. Except if we did not cover all the cases where we should avoid complex expressions, we always generate an entry value expression with an empty pre-existing DIExpression, so we assumed that this code will cover current situation and may be extended to support complex debug expressions as well. But, I also think it is hard to distinguish the size of a complex DIExpression until DWARF being printed, so maybe we can change the code to Ops.push_back(1); and put some kind of assertion there.

As far as I understand, we now emit the operation from the DIExpression as-is in the DWARF. That means that if we have a register that turns into a complex expression we will still say that the size of that expression is one byte. I have seen such cases with our downstream target, but I'll see if I can trigger that behavior with an upstream target with a source level reproducer.

If you can produce such scenario it will be desirable. We enabled the feature in this initial stage only for x86 targets, and tried to cover all the situations found for the target. We meant to cover all the places where we should avoid generation of debug entry values with complex expressions (now). Eventually, we should handle all types of expressions.

It can be reproduced with the following C file compiled for sparc64:

volatile long double global;
extern void clobber();
int foo(long double p) {
  global = p;
  clobber();
  return 123;
}

compiled using:

clang --target=sparc64 -g -O2 -Xclang -femit-debug-entry-values -c -integrated-as sparc.c

yields the following entry value after LiveDebugValues:

CALL @clobber, csr, implicit $o6, implicit-def $o6, debug-location !24 {
  STDFri killed renamable $i0, 8, renamable $d1, implicit killed $q0, debug-location !19 :: (store 8)
}
DBG_VALUE $q0, $noreg, !17, !DIExpression(DW_OP_entry_value, 1), debug-location !18

which currently results in the following bad location list entry:

[0x0000000000000020,  0x0000000000000028): DW_OP_GNU_entry_value(DW_OP_regx D0), DW_OP_piece 0x8, DW_OP_regx D1, DW_OP_piece 0x8, DW_OP_stack_value)

Please note that you have to apply D66888, and also add sparc to the list of allowed targets in ParseCodeGenArgs(), to get that reproducer working.

[aprantl]

Another comment I made in an earlier review is that it is also reasonable to have completely different semantics for DW_OP_entry_value in LLVM IR; in which case it would be best to introduce an IR-only DW_OP_LLVM_entry_value (like DW_OP_LLVM_fragment) to avoid confusion and lower that to a real DWARF opcode in AsmPrinter.

[djtodoro]

@dstenb Thanks a lot for the test case, I will take a look into that.

Another comment I made in an earlier review is that it is also reasonable to have completely different semantics for DW_OP_entry_value in LLVM IR; in which case it would be best to introduce an IR-only DW_OP_LLVM_entry_value (like DW_OP_LLVM_fragment) to avoid confusion and lower that to a real DWARF opcode in AsmPrinter.

@aprantl I agree with this... That will simplify things a lot, especially future work.

[dstenb]

Sorry for the delay!

The DW_OP_LLVM_entry_value operation has now been introduced, and that allows for DW_OP_entry_value operations with blocks larger than one byte to be expressed. However, the DW_OP_LLVM_entry_value operation can currently only wrap one operation in the DIExpression (there are some things missing in DwarfDebug and DwarfExpression for allowing more), so we'd still see the crash that this patch addresses.

However, we do not really want to emit an entry value here, as the caller's parameter value has been propagated into the callee. I think that "omit entry values if the caller's parameter value has been propagated into the callee" is the condition that I really want check here. I have created a new patch that attempts to address that: D69889. I will abandon this revision, and we can then revisit it later on if needed.

llvm/lib/CodeGen/LiveDebugValues.cpp

bool LiveDebugValues::ExtendRanges(MachineFunction &MF) {
   // representing candidates for production of debug entry values.
   DebugParamMap DebugEntryVals;
 
   MachineBasicBlock &First_MBB = *(MF.begin());
   // Only in the case of entry MBB collect DBG_VALUEs representing
   // function parameters in order to generate debug entry values for them.
   // Currently, we generate debug entry values only for parameters that are
   // unmodified throughout the function and located in a register.
-  // TODO: Add support for parameters that are described as fragments.
+  // TODO: Add support for parameters with non-empty debug expressions (for
+  // example fragments).
   // TODO: Add support for modified arguments that can be expressed
   // by using its entry value.
   // TODO: Add support for local variables that are expressed in terms of
   // parameters entry values.
   for (auto &MI : First_MBB)
     if (MI.isDebugValue() && IsUnmodifiedFuncParam(MI) &&
         !MI.isIndirectDebugValue() && IsRegOtherThanSPAndFP(MI.getOperand(0)) &&
         !DebugEntryVals.count(MI.getDebugVariable()) &&
-        !MI.getDebugExpression()->isFragment())
+        MI.getDebugExpression()->getNumElements() == 0)
       DebugEntryVals[MI.getDebugVariable()] = &MI;
 
   // Initialize every mbb with OutLocs.
   // We are not looking at any spill instructions during the initial pass
   // over the BBs. The LiveDebugVariables pass has already created DBG_VALUE
   // instructions for spills of registers that are known to be user variables
   // within the BB in which the spill occurs.
   for (auto &MBB : MF) {

llvm/test/DebugInfo/Mips/entry-value-non-empty-expr.ll

+; RUN: llc -O0 -debug-entry-values -stop-after=livedebugvalues < %s | FileCheck %s
+
+target datalayout = "E-m:m-p:32:32-i8:8:32-i16:16:32-i64:64-n32-S64"
+target triple = "mips"
+
+; Based on the following C reproducer:
+;
+; int arr[9];
+; extern void ext(int);
+; static void bar(int *p) { ext(*p); }
+; void foo() { bar(&arr[8]); }
+
+; Verify that there is no entry value created for the parameter. Entry values
+; are currently only supported for register DBG_VALUEs with empty debug
+; expressions, since the DW_OP_entry_value can currently only wrap one byte.
+; Creating an entry value for a DBG_VALUE with a non-empty debug expression
+; would result in an invalid expression, triggering an assertion later on.
+;
+; In this case the entry value for the parameter would not have any purpose
+; since the caller's parameter value has been propagated to the static callee,
+; so there is no call site entry for the parameter.
+
+; CHECK-NOT: DBG_VALUE
+; CHECK: DBG_VALUE {{.*}}, $noreg, {{.*}}, !DIExpression(DW_OP_plus_uconst, 32, DW_OP_stack_value)
+; CHECK-NOT: DBG_VALUE
+
+@arr = common global [9 x i32] zeroinitializer, align 4
+
+; Function Attrs: nounwind
+define void @foo() #0 !dbg !11 {
+entry:
+  tail call fastcc void @bar() #2, !dbg !14
+  ret void, !dbg !14
+}
+
+; Function Attrs: nounwind
+define internal fastcc void @bar() #0 !dbg !15 {
+entry:
+  call void @llvm.dbg.value(metadata i32* getelementptr inbounds ([9 x i32], [9 x i32]* @arr, i32 0, i32 8), metadata !20, metadata !DIExpression()), !dbg !21
+  %0 = load i32, i32* getelementptr inbounds ([9 x i32], [9 x i32]* @arr, i32 0, i32 8), align 4, !dbg !22
+  tail call void @ext(i32 signext %0), !dbg !22
+  ret void, !dbg !22
+}
+
+declare !dbg !4 void @ext(i32 signext)
+
+; Function Attrs: nounwind readnone speculatable willreturn
+declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+
+attributes #0 = { nounwind }
+attributes #1 = { nounwind readnone speculatable willreturn }
+attributes #2 = { nobuiltin }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!8, !9}
+!llvm.ident = !{!10}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 10.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !3, globals: !2, nameTableKind: None)
+!1 = !DIFile(filename: "mips.c", directory: "/")
+!2 = !{}
+!3 = !{!4}
+!4 = !DISubprogram(name: "ext", scope: !1, file: !1, line: 3, type: !5, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
+!5 = !DISubroutineType(types: !6)
+!6 = !{null, !7}
+!7 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!8 = !{i32 2, !"Dwarf Version", i32 4}
+!9 = !{i32 2, !"Debug Info Version", i32 3}
+!10 = !{!"clang version 10.0.0"}
+!11 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 7, type: !12, scopeLine: 7, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+!12 = !DISubroutineType(types: !13)
+!13 = !{null}
+!14 = !DILocation(line: 7, scope: !11)
+!15 = distinct !DISubprogram(name: "bar", scope: !1, file: !1, line: 5, type: !16, scopeLine: 5, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !19)
+!16 = !DISubroutineType(types: !17)
+!17 = !{null, !18}
+!18 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !7, size: 32)
+!19 = !{!20}
+!20 = !DILocalVariable(name: "p", arg: 1, scope: !15, file: !1, line: 5, type: !18, flags: DIFlagArgumentNotModified)
+!21 = !DILocation(line: 0, scope: !15)
+!22 = !DILocation(line: 5, scope: !15)