[DebugInfo] Emit locations for large constant integers

[labath]

Summary

This adds support for emissing of integers larger than 8 bytes (e.g
__int128_t). It reuses/extends the existing code for emitting floating
point constant, using DW_OP_implicit_value (if debugger tuning permits
it), or glueing the value together using DW_OP_const and DW_OP_piece.

[SouraVX]

Thanks for the patch! Overall this looks nice. We can also represent long double(128 bits) using DW_OP_implicit_value, any plans for supporting that too ? long double is non-trivial like it has different representation on Power. It's pending on me since a while :(

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

206 ↗

(On Diff #303377)

GDB also supports this. When I introduced this representation, I kept it exclusively GDB. Looking at DwarfDebug.cpp:2477 looks like someone changed it along the way.
Do you mind adding GDB also ?

[SouraVX]

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

245 ↗

(On Diff #303377)

NIT: Trival braces ?
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

[dblaikie]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

2477–2480

What's this change doing? It looks new & not sure what the old code would've been doing previously under this condition (isConstantInt)

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

202 ↗

(On Diff #303377)

This change looks like it'd have two effects:

1. existing callers of addUnsignedConstant now might get DW_OP_implicit_values where they didn't before
2. the new call from addConstantFP might lead to that use case getting DW_OP_const values it didn't get before

Could those two situations be separated into distinct patches with distinct tests? (I mean, could be the same test file, but making it clear how adding this new functionality to addUnsignedConstant impacts existing callers, then separately how changing addConstantFP to use addUnsignedConstant changes that)

233 ↗

(On Diff #303377)

Is this a separate bugfix? Could go in a separate patch/test/etc?

[labath]

In D90916#2378622, @SouraVX wrote:

Thanks for the patch! Overall this looks nice. We can also represent long double(128 bits) using DW_OP_implicit_value, any plans for supporting that too ? long double is non-trivial like it has different representation on Power. It's pending on me since a while :(

Um... maybe? What's the deal with long double and PPC ? Does bitcastToAPInt return a different representation than what's actually used on the machine?

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

2477–2480

This is the part which actually implements this feature. :)
I don't understand why, but integers <= 64 bit are stored differently than integers with larger width. So, smaller integers would be caught by the isInt check on line 2451, but the code wouldn't do anything with larger integers (AFAICT).

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

202 ↗

(On Diff #303377)

I elaborate on (1) below. You're right about (2). I did check that gdb&lldb can actually handle the expanded usage of DW_OP_const, but I was too lazy to turn it into a test. I'll try to split that off into a separate patch.

206 ↗

(On Diff #303377)

This already covers gdb (note the negation). The condition was changed from GDB to not SCE when lldb support for this was added.

233 ↗

(On Diff #303377)

I'm not sure I would call it a bugfix, as the function had only one caller (and a pretty strange one at that) and was only being called for size <= 64. However, it is required to reflect the new realities of how this function is used now.

Not emitting DW_OP_stack_value here was kind of correct, as its only caller (DwarfDebug.cpp:2484) was falling through (after ensuring that the size is <= 64) to DwarfExpr.addExpression(std::move(ExprCursor));, which would end up adding DW_OP_stack_value.

I'm not really sure what addExpression does (I am quite new to this area), but in the examples I've tried, it only appends DW_OP_stack_value. I suppose it might be possible to modify the value of the float constant via some dwarf expressions, but that would be fairly tricky as the dwarf operands have integral semantics.

In the new setup, I don't even bother calling addExpression, as hooking that up would be tricky (and of dubious value), as that function could be called on single-piece non-implicit_value expressions. That means this function always needs to emit DW_OP_stack_value itself.

245 ↗

(On Diff #303377)

In my mind, the FIXME made the body non-trivial. But I guess it is possible to move that outside the body...

[labath]

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

202 ↗

(On Diff #303377)

Created a separate refactoring patch here: https://reviews.llvm.org/D91058.

On second though, I don't think there's anything additional to test (until we support long double constants that is). The function was only being called for values smaller than 64 bits, and the existing implict_value FP tests have an SCE test which confirms that DW_OP_stack value is emitted in the same way as before.

[labath]

Rebase on top of the other patch.

[dblaikie]

Looks good!

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;
   }
+  if (Value.isConstantInt()) {
+    DwarfExpr.addUnsignedConstant(Value.getConstantInt()->getValue(), AP);
+    return;
+  }
   if (Value.isConstantFP()) {
     DwarfExpr.addConstantFP(Value.getConstantFP()->getValueAPF(), AP);
     return;
   }
   DwarfExpr.addExpression(std::move(ExprCursor));
 }
 
 void DebugLocEntry::finalize(const AsmPrinter &AP,

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

+; RUN: llc -debugger-tune=gdb -filetype=obj %s -o - | llvm-dwarfdump - \
+; RUN:   | FileCheck %s --check-prefixes=GDB,BOTH
+; RUN: llc -debugger-tune=sce -filetype=obj %s -o - | llvm-dwarfdump - \
+; RUN:   | FileCheck %s --check-prefixes=SCE,BOTH
+
+; BOTH: DW_TAG_variable
+; BOTH-NEXT:  DW_AT_location
+; GDB-NEXT:      {{.*}}: DW_OP_implicit_value 0x10 0x0f 0x0e 0x0d 0x0c 0x0b 0x0a 0x09 0x08 0x07 0x06 0x05 0x04 0x03 0x02 0x01 0x00
+; SCE-NEXT:      {{.*}}: DW_OP_constu 0x8090a0b0c0d0e0f, DW_OP_stack_value, DW_OP_piece 0x8, DW_OP_constu 0x1020304050607, DW_OP_stack_value, DW_OP_bit_piece 0x40 0x40
+; GDB-NEXT:      {{.*}}: DW_OP_implicit_value 0x10 0x00 0x10 0x20 0x30 0x40 0x50 0x60 0x70 0x80 0x90 0xa0 0xb0 0xc0 0xd0 0xe0 0xf0)
+; SCE-NEXT:      {{.*}}: DW_OP_constu 0x7060504030201000, DW_OP_stack_value, DW_OP_piece 0x8, DW_OP_constu 0xf0e0d0c0b0a09080, DW_OP_stack_value, DW_OP_bit_piece 0x40 0x40
+; BOTH-NEXT:  DW_AT_name    ("a")
+
+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"
+
+define dso_local void @f() local_unnamed_addr !dbg !7 {
+entry:
+  call void @llvm.dbg.value(metadata i128 5233100606242806050955395731361295, metadata !12, metadata !DIExpression()), !dbg !15
+  tail call void @g(), !dbg !15
+  call void @llvm.dbg.value(metadata i128 320182027492359845421654932427609477120, metadata !12, metadata !DIExpression()), !dbg !15
+  tail call void @g(), !dbg !15
+  ret void, !dbg !15
+}
+
+declare dso_local void @g() local_unnamed_addr
+
+declare void @llvm.dbg.value(metadata, metadata, metadata) #2
+
+attributes #2 = { nofree nosync nounwind readnone speculatable willreturn }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "-", directory: "/")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!7 = distinct !DISubprogram(name: "f", scope: !1, file: !1, type: !9, unit: !0)
+!9 = !DISubroutineType(types: !10)
+!10 = !{null}
+!12 = !DILocalVariable(name: "a", scope: !7, file: !1, type: !14)
+!14 = !DIBasicType(name: "__int128", size: 128, encoding: DW_ATE_signed)
+!15 = !DILocation(line: 0, scope: !7)