[DebugInfo] Don't use DW_OP_implicit_value for fragments

[labath]

Summary

Currently using DW_OP_implicit_value in fragments produces invalid DWARF
expressions. (Such a case can occur in complex floats, for example.)

This problem manifests itself as a missing DW_OP_piece operation after
the last fragment. This happens because the function for printing
constant float value skips printing the accompanying DWARF expression,
as that would also print DW_OP_stack_value (which is not desirable in
this case). However, this also results in DW_OP_piece being skipped.

The reason that DW_OP_piece is missing only for the last piece is that
the act of printing the next fragment corrects this. However, it does
that for the wrong reason -- the code emitting this DW_OP_piece thinks
that the previous fragment was missing, and so it thinks that it needs
to skip over it in order to be able to print itself.

In a simple scenario this works out, but it's likely that in a more
complex setup (where some pieces are in fact missing), this logic would
go badly wrong. In a simple setup gdb also seems to not mind the fact
that the DW_OP_piece is missing, but it would also likely not handle
more complex use cases.

For this reason, this patch disables the usage of DW_OP_implicit_value
in the frament scenario (we will use DW_OP_const*** instead), until we
figure out the right way to deal with this. This guarantees that we
produce valid expressions, and gdb can handle both kinds of inputs
anyway.

[labath]

I'd like to also fix this in a more fundamental way, but I think I'd need some guidance from someone more familiar with this code (who would that be?).

One idea I had is to add something like isPureFragment() to the DIExpression class, which would return true if the expression consistent solely of a single DW_OP_LLVM_fragment operand. Then this code could use that to detect when it is safe to do a DwarfExpr.addExpression(std::move(ExprCursor)) to emit a DW_OP_piece. One would probably also need to introduce a new LocationKind constant (ImplicitlyImplicit :P) to indicate that there's no need to emit DW_OP_stack_value. Or it might be possible to reuse one of the existing kinds for this purpose.

Does that sounds reasonable?

[dblaikie]

In D92013#2413734, @labath wrote:

I'd like to also fix this in a more fundamental way, but I think I'd need some guidance from someone more familiar with this code (who would that be?).

Guessing @aprantl - think he implemented the fragment support? (though some archaeology may confirm/deny that/find some other folks with familiarity)

[jmorse]

I'd like to chime in and point out a slightly related scenario [0] where we have to interpret a DIExpression to work out how adding operators is going to change the meaning of the expression. I believe in the long run we need metadata describing "the attributes of this variable location" rather than packing information into DIExpressions and having to interpret it.

For the record this patch looks fine, but DWARF printing is outside of my comfort zone so I'd prefer to hear from others.

[0] https://github.com/llvm/llvm-project/blob/7b1cb4715063679531e51127eee869cd03df88da/llvm/lib/CodeGen/PrologEpilogInserter.cpp#L1221

[aprantl]

In D92013#2413734, @labath wrote:

I'd like to also fix this in a more fundamental way, but I think I'd need some guidance from someone more familiar with this code (who would that be?).

One idea I had is to add something like isPureFragment() to the DIExpression class, which would return true if the expression consistent solely of a single DW_OP_LLVM_fragment operand.

You mean DIExpression(DW_OP_LLVM_fragment 0, 1) is "pure" s opposed to DIExpression(DW_OP_deref, DW_OP_LLVM_fragment 0, 1), which isn't? Or something else?

Then this code could use that to detect when it is safe to do a DwarfExpr.addExpression(std::move(ExprCursor)) to emit a DW_OP_piece. One would probably also need to introduce a new LocationKind constant (ImplicitlyImplicit :P) to indicate that there's no need to emit DW_OP_stack_value. Or it might be possible to reuse one of the existing kinds for this purpose.

Does that sounds reasonable?

Are you saying that the code to emit FP constants is not passing the DW_OP_LLVM_fragment from DIExpr to DwarfExpr (and we happen to emit a skip-piece, which often has the same effect)? Shouldn't we then try to get DwarfExpression to process the fragment operator from DIExpr instead? I'm probably missing something...

[labath]

In D92013#2453364, @aprantl wrote:

In D92013#2413734, @labath wrote:

I'd like to also fix this in a more fundamental way, but I think I'd need some guidance from someone more familiar with this code (who would that be?).

One idea I had is to add something like isPureFragment() to the DIExpression class, which would return true if the expression consistent solely of a single DW_OP_LLVM_fragment operand.

You mean DIExpression(DW_OP_LLVM_fragment 0, 1) is "pure" s opposed to DIExpression(DW_OP_deref, DW_OP_LLVM_fragment 0, 1), which isn't? Or something else?

Yes, that's pretty much it.

Then this code could use that to detect when it is safe to do a DwarfExpr.addExpression(std::move(ExprCursor)) to emit a DW_OP_piece. One would probably also need to introduce a new LocationKind constant (ImplicitlyImplicit :P) to indicate that there's no need to emit DW_OP_stack_value. Or it might be possible to reuse one of the existing kinds for this purpose.

Does that sounds reasonable?

Are you saying that the code to emit FP constants is not passing the DW_OP_LLVM_fragment from DIExpr to DwarfExpr (and we happen to emit a skip-piece, which often has the same effect)?

Affirmative.

Shouldn't we then try to get DwarfExpression to process the fragment operator from DIExpr instead? I'm probably missing something...

Yes, that is what I (too) think we should do. The thing which makes it tricky (and maybe that's the thing you're missing) is that we have two ways of printing a FP constant (controlled by a debugger tuning parameter, among other things), and they have different constraints on what can be placed after them.

• the first method does that via DW_OP_const***. These are regular dwarf operators (pushing a value on the stack) and they can be combined with anything that can be present in the DIExpression (I'm not sure why one would want to e.g. dereference a FP constant, but yeah...)
• the second method uses DW_OP_implicit_value. This operator describes the value directly (it doesn't just push something on the stack). This means we should not place DW_OP_stack_value after it. It also means it cannot be combined with DW_OP_deref, DW_OP_plus, etc. However, it *can* be combined with DW_OP_(bit_)piece (i.e., DW_OP_LLVM_fragment), which is why we need a way to detect whether an expression consists _solely_ of DW_OP_LLVM_fragment. (and yeah, I agree that a dedicated way of representing fragments would be nice)

Another option would be to be conservative, and choose the DW_OP_const method whenever the DIExpr is non-empty, even if it just consists of a DW_OP_LLVM_fragment. That is what this patch, in its current form does. This fixes the problem at hand, but I don't think it's a good long term strategy, as it's not possible to represent floating point values larger than 64 bits in this way (at least not without additional fragmenting).

[aprantl]

Thanks, Pavel! My aspiration for DwarfExpression.cpp was to eventually add some backtracking capability to it to choose the optimal (=shortest) sequence of instructions for each expression. So far we haven't done this, because local decisions were usually sufficient, and more importantly, streaming the output limited our flexibility. If we give DwarfExpression a one-step look-ahead buffer, we could implement addConstantFP() as pushing two alternatives to the buffer:

 [Ops=[DW_OP_const ...], Constraints=[AnyAllowed], // alternative 1
  Ops= [DW_OP_implicit_value, ...], Constraints=[NextMustBeOpPiece] // alternative 2
]

The next time DwarfExpression wants to push an operation to the buffer, it first checks the constraints and picks the one that is compatible. And of course someone could have some computer science fun with generalizing this to an arbitrarily-large buffer guaranteeing an optimal sequence while not having exponential runtime.

I'm fine with this patch as it solves our problem in the short term but I also wanted to point out a route towards solving this properly.

[labath]

Thanks for the review, and for the explanation Adrian. I want to fix a couple more issues in the general vicinity of this code, so I'll keep this design goal in mind.

[labath]

Thinking about this further, I am not sure if the approach with multiple alternatives is a good solution (for this particular problem at least). If we take x87 long double constants (currently not supported here, but one of the things I wanted to look at) for example, they are 10 bytes long. Representing them with DW_OP_implicit_value is trivial. However, if we wanted to represent them with DW_OP_const, we would need to break it down into smaller chunks, something like DW_OP_const low64, DW_OP_stack_value, DW_OP_piece 8, DW_OP_const high16, DW_OP_stack_value, DW_OP_piece 2. If the DIExpression is empty, then that's fairly easy. However, if the expression already contains some operators (DW_OP_LLVM_fragment in particular), then I fear things would get messy. How messy depends on what is actually allowed to be contained in these expressions, which brings me to my next question:

What kind of operands are actually allowed in an expression describing a constant? DW_OP_LLVM_fragment is obviously useful, but I'm not sure about the other operations. For example, the arithmetic operations could simply be performed by applying the desired operation directly to the constant. DW_OP_deref might be interesting in cases where one wants to dereference a constant pointer, but in that case, the constant already needs to be address-sized, so there should be no further fragmenting.

One possibility I considered is to change the code which creates fragments (I don't yet know where this code lives) in such way that it always fragments that don't need any further fragmenting. The complications I see with that are:

• it would create the reverse problem of having to jump through hoops to reconstitute the fragments, if we wanted to emit a DW_OP_implicit_value (maybe we don't have to do that?)
• the fragmenting code would need to take the pointer size into account (one of the bugs I want to solve is that we're always assuming a 64-bit stack, whereas dwarf says it's address-sized). Knowing the address size at some point is unavoidable, so this might not be a big issue.
• it might interfere with subsequent optimizations (the scope of this is unknown to me)

Given that, it seems to me that it would be better (in line what @jmorse mentioned) to give the fragment information more prominence in the DIExpression, and enable emission code to better reason about it. That way it should be pretty easy to re-fragment the expression, if necessary, or just bail out (like now) in case we run into something we don't support. Since DW_OP_LLVM_fragment only makes sense at the end of an expression (does it?), it's not clear to me if it's really useful to have it be a part of that expression in the first place, but it does not really matter, I think -- we could either keep them there, and create an abstraction that would let skip/ignore them, or try move them out completely.

What do you think?

[aprantl]

the fragmenting code would need to take the pointer size into account (one of the bugs I want to solve is that we're always assuming a 64-bit stack, whereas dwarf says it's address-sized). Knowing the address size at some point is unavoidable, so this might not be a big issue.

If we have a way to read the pointer size out of MC, for example, I don't see a problem with that. Perhaps not even if we have to encode it in the compile unit.

Given that, it seems to me that it would be better (in line what @jmorse mentioned) to give the fragment information more prominence in the DIExpression, and enable emission code to better reason about it.

One thing to keep in mind is that DwarfExpression already may create additional fragments in order to piece together super registers for which no DWARF encoding exists from multiple subregisters. I think whatever design we choose it should be able to cope with DwarfExpression's lowering creating more fragments.

Since DW_OP_LLVM_fragment only makes sense at the end of an expression (does it?), it's not clear to me if it's really useful to have it be a part of that expression in the first place, but it does not really matter, I think -- we could either keep them there, and create an abstraction that would let skip/ignore them, or try move them out completely.

Yes, the fact that the fragment info is stored in the expression is orthogonal to how we interpret it. Currently it is stored as operators and also accessible through the DIExpression::getFragmentInfo() interface (https://llvm.org/doxygen/classllvm_1_1DIExpression.html#a8bab2e1fa7810adfd2c2f687838cec5d). We could store it separately, if that is more space-efficient, or simplifies the code, but how we store it shouldn't limit how we use that information.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

void DwarfDebug::emitDebugLocValue(const AsmPrinter &AP, const DIBasicType *BT,
   } else if (Value.isTargetIndexLocation()) {
     TargetIndexLocation Loc = Value.getTargetIndexLocation();
     // TODO TargetIndexLocation is a target-independent. Currently only the WebAssembly-specific
     // encoding is supported.
     DwarfExpr.addWasmLocation(Loc.Index, static_cast<uint64_t>(Loc.Offset));
       DwarfExpr.addExpression(std::move(ExprCursor));
       return;
   } else if (Value.isConstantFP()) {
-    if (AP.getDwarfVersion() >= 4 && !AP.getDwarfDebug()->tuneForSCE()) {
+    if (AP.getDwarfVersion() >= 4 && !AP.getDwarfDebug()->tuneForSCE() &&
+        !ExprCursor) {
       DwarfExpr.addConstantFP(Value.getConstantFP()->getValueAPF(), AP);
       return;
-    } else if (Value.getConstantFP()
-                   ->getValueAPF()
-                   .bitcastToAPInt()
-                   .getBitWidth() <= 64 /*bits*/)
+    }
+    if (Value.getConstantFP()->getValueAPF().bitcastToAPInt().getBitWidth() <=
+        64 /*bits*/)
       DwarfExpr.addUnsignedConstant(
           Value.getConstantFP()->getValueAPF().bitcastToAPInt());
     else
       LLVM_DEBUG(
           dbgs()
           << "Skipped DwarfExpression creation for ConstantFP of size"
           << Value.getConstantFP()->getValueAPF().bitcastToAPInt().getBitWidth()
           << " bits\n");

llvm/test/DebugInfo/X86/implicit_value-double.ll

 ;; This test checks for emission of DW_OP_implicit_value operation
 ;; for double type.
 
-; RUN: llc -debugger-tune=gdb -filetype=obj %s -o -  | llvm-dwarfdump - | FileCheck %s
-; RUN: llc -debugger-tune=lldb -filetype=obj %s -o - | llvm-dwarfdump - | FileCheck %s
+; RUN: llc -O0 -debugger-tune=gdb -filetype=obj %s -o -  | llvm-dwarfdump - | FileCheck %s --check-prefixes=CHECK,BOTH
+; RUN: llc -O0 -debugger-tune=lldb -filetype=obj %s -o - | llvm-dwarfdump - | FileCheck %s --check-prefixes=CHECK,BOTH
 
 ; CHECK: .debug_info contents:
 ; CHECK: DW_TAG_variable
 ; CHECK-NEXT:  DW_AT_location        ({{.*}}
 ; CHECK-NEXT:                     [{{.*}}): DW_OP_implicit_value 0x8 0x1f 0x85 0xeb 0x51 0xb8 0x1e 0x09 0x40)
 ; CHECK-NEXT:  DW_AT_name    ("d")
 
-; RUN: llc -debugger-tune=sce -filetype=obj %s -o -  | llvm-dwarfdump - | FileCheck %s -check-prefix=SCE-CHECK
+; RUN: llc -O0 -debugger-tune=sce -filetype=obj %s -o -  | llvm-dwarfdump - | FileCheck %s -check-prefixes=SCE-CHECK,BOTH
 
 ; SCE-CHECK: .debug_info contents:
 ; SCE-CHECK: DW_TAG_variable
 ; SCE-CHECK-NEXT:  DW_AT_location        ({{.*}}
 ; SCE-CHECK-NEXT:                     [{{.*}}): DW_OP_constu 0x40091eb851eb851f, DW_OP_stack_value)
 ; SCE-CHECK-NEXT:  DW_AT_name    ("d")
 
-;; Generated from: clang -ggdb -O1
-;;int main() {
-;;        double d = 3.14;
-;;        printf("dummy\n");
-;;        d *= d;
-;;        return 0;
-;;}
+;; Using DW_OP_implicit_value for fragments is not currently supported.
+; BOTH: DW_TAG_variable
+; BOTH-NEXT:  DW_AT_location        ({{.*}}
+; BOTH-NEXT:                     [{{.*}}): DW_OP_constu 0x4047800000000000, DW_OP_stack_value, DW_OP_piece 0x8, DW_OP_constu 0x4052800000000000, DW_OP_stack_value, DW_OP_piece 0x8)
+; BOTH-NEXT:  DW_AT_name    ("c")
 
 ; ModuleID = 'implicit_value-double.c'
 source_filename = "implicit_value-double.c"
 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"
 
 @str = private unnamed_addr constant [6 x i8] c"dummy\00", align 1
 
 ; Function Attrs: nofree nounwind uwtable
 define dso_local i32 @main() local_unnamed_addr #0 !dbg !7 {
 entry:
   call void @llvm.dbg.value(metadata double 3.140000e+00, metadata !12, metadata !DIExpression()), !dbg !14
+  call void @llvm.dbg.value(metadata double 4.700000e+01, metadata !17, metadata !DIExpression(DW_OP_LLVM_fragment, 0, 64)), !dbg !14
+  call void @llvm.dbg.value(metadata double 7.400000e+01, metadata !17, metadata !DIExpression(DW_OP_LLVM_fragment, 64, 64)), !dbg !14
   %puts = call i32 @puts(i8* nonnull dereferenceable(1) getelementptr inbounds ([6 x i8], [6 x i8]* @str, i64 0, i64 0)), !dbg !15
   call void @llvm.dbg.value(metadata double undef, metadata !12, metadata !DIExpression()), !dbg !14
   ret i32 0, !dbg !16
 }
 
 ; Function Attrs: nounwind readnone speculatable willreturn
 declare void @llvm.dbg.value(metadata, metadata, metadata) #1
 

 !3 = !{i32 7, !"Dwarf Version", i32 4}
 !4 = !{i32 2, !"Debug Info Version", i32 3}
 !5 = !{i32 1, !"wchar_size", i32 4}
 !6 = !{!"clang version 11.0.0"}
 !7 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 1, type: !8, scopeLine: 1, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !11)
 !8 = !DISubroutineType(types: !9)
 !9 = !{!10}
 !10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
-!11 = !{!12}
+!11 = !{!12, !17}
 !12 = !DILocalVariable(name: "d", scope: !7, file: !1, line: 2, type: !13)
 !13 = !DIBasicType(name: "double", size: 64, encoding: DW_ATE_float)
 !14 = !DILocation(line: 0, scope: !7)
 !15 = !DILocation(line: 3, column: 2, scope: !7)
 !16 = !DILocation(line: 5, column: 2, scope: !7)
+!17 = !DILocalVariable(name: "c", scope: !7, file: !1, line: 2, type: !18)
+!18 = !DIBasicType(name: "complex", size: 128, encoding: DW_ATE_complex_float)