Prior to this patch, variadic DIExpressions (i.e. ones that contain DW_OP_LLVM_arg) could only be created by salvaging debug values to create stack value expressions, resulting in a DBG_VALUE_LIST being created. As of the previous patch in this patch stack, DBG_INSTR_REF's syntax has been changed to match DBG_VALUE_LIST in preparation for supporting variadic expressions. This patch adds some minor changes needed to allow variadic expressions that aren't stack values to exist, and allows variadic expressions that are trivially reduceable to non-variadic expressions to be handled similarly to non-variadic expressions.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Generally looks good, I've got a couple of questions inline.
Are there any serious performance implications for, in the direction these patches are moving in, there being lots of non-empty DIExpressions compared to today when they're mostly empty? (I'm thinking we might need a static, pre-allocated one in LLVMContext, if every variable is going to allocate one).
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp | ||
---|---|---|
1135–1140 | Just to check -- this is purely for printing the informative comments on assembly output, it isn't actually functional, yes? | |
llvm/lib/IR/DebugInfoMetadata.cpp | ||
1633–1641 | Just to confirm, the reason why this wasn't present before was because all variadic expressions had to be stack_value, yes? | |
llvm/test/DebugInfo/MIR/InstrRef/dbg-phi-subregister-location.mir | ||
13 | Does this patch actually make some DBG_VALUEs become DBG_VALUE_LIST? I've missed that part, and see it's happening in various tests. |
(Lo and behold, I already asked that last question in D133929, never mind about that)
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp | ||
---|---|---|
1135–1140 | Correct, this is just removing noise from the patch - even if it was an expression being used somewhere, it'd still have the same meaning either way, just formatted slightly differently. | |
llvm/lib/IR/DebugInfoMetadata.cpp | ||
1633–1641 | Correct - worth noting the only reason they "had" to be stack_value was because they're only naturally produced by salvaging operations that create stack_value expressions, not because there's some inherent incompatibility. |
Updated ISel and LDV to have very basic handling of "variadic" DBG_INSTR_REFs, and updated tests to use DW_OP_LLVM_arg.
llvm/test/DebugInfo/MIR/InstrRef/dbg-phi-subregister-location.mir | ||
---|---|---|
13 | Missed this question - this patch changes DBG_INSTR_REF to have the same syntax as a DBG_VALUE_LIST, which includes having the variadic flag set, hence LDV will output the debug value produced from a DBG_INSTR_REF as a DBG_VALUE_LIST. |
Move some changes from an earlier patch into this one: Make entry value expressions containing only a leading DW_OP_LLVM_arg, 0 valid, as the changes in this patch enable such expressions. Also add the convertToVariadicExpression function, as this is the first patch in which it is used.
Sorry to bump this review, but I'm currently investigating a couple of bugs which I believe are a result of this patch.
One thing that seems off here is that now the definition of isEntryValue is incorrect:
bool DIExpression::isEntryValue() const { return getNumElements() > 0 && getElement(0) == dwarf::DW_OP_LLVM_entry_value; }
Also note that the CodeGen/AsmPrinter/DwarfExpression.cpp code for DwarfExpression::addExpression doesn't expect an EntryValue as anything but the first operand of the op list.
Still investigating how to handle those, but figured I'd give you a heads up in case you have ideas
I'll take a quick look - off the top of my head, there should be an interface to treat !DIExpression(DW_OP_LLVM_arg, 0, <ops>) identically to !DIExpression(<ops>), and if it either doesn't exist or is only applicable to specific cases then it should be created/generalized.
I'll take a quick look - off the top of my head, there should be an interface to treat !DIExpression(DW_OP_LLVM_arg, 0, <ops>) identically to !DIExpression(<ops>), and if it either doesn't exist or is only applicable to specific cases then it should be created/generalized.
Thank you!
Also note that the CodeGen/AsmPrinter/DwarfExpression.cpp code for DwarfExpression::addExpression doesn't expect an EntryValue as anything but the first operand of the op list.
I think the problem here is that, inside DwarfCompileUnit::constructVariableDIEImpl, we are now taking the *variadic path* which ends up calling addExpression.
More concretely, inside that function we have:
if (auto *DVal = DV.getValueLoc()) { if (!DVal->isVariadic()) {
We are taking the else path of that second if, i.e., we are treating things as variadic where we before we wouldn't?
(lldb) p DV.getValueLoc()->isVariadic() (bool) $2 = true (lldb) p DV.getValueLoc()->dump() Loc = { reg=134 } !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_entry_value, 1, DW_OP_plus_uconst, 16, DW_OP_plus_uconst, 16, DW_OP_deref, DW_OP_deref)
Because of this, we end up calling DwarfExpression::addExpression, which crashes for that example because it doesn't expect an entry value.
Thanks for the further details - in summary, as part of the changes to DBG_INSTR_REF, we started using the variadic syntax for debug values that would normally have been non-variadic - there's no fundamental problem with this, but clearly some edges weren't fully ironed out. I'll submit a patch shortly with a fix - do you have a reproducer for the issue?
Yup, that makes perfect sense. Let me see if I can conjure a min repro and I'll come back here shortly.
Could you give this a try? It repros for me with a llc test.ll.
target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64-apple-macosx12.1.0" declare void @llvm.dbg.value(metadata, metadata, metadata) #0 define swifttailcc void @"blah"(ptr swiftasync %0) !dbg !26 { %use = getelementptr i8, ptr %0, i64 9 call void @llvm.dbg.value(metadata ptr %0, metadata !30, metadata !DIExpression(DW_OP_LLVM_entry_value, 1, DW_OP_plus_uconst, 16, DW_OP_plus_uconst, 16, DW_OP_deref, DW_OP_deref)), !dbg !34 %use1 = load i32, ptr null, !dbg !45 %use2 = sext i32 %use1 to i64 %use3 = getelementptr i8, ptr null, i64 %use2 store ptr %use3, ptr %0 ret void } attributes #0 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) } !llvm.module.flags = !{!0} !llvm.dbg.cu = !{!1} !0 = !{i32 2, !"Debug Info Version", i32 3} !1 = distinct !DICompileUnit(language: DW_LANG_Swift, file: !7, producer: "blah", isOptimized: true, flags: "blah", runtimeVersion: 5, emissionKind: FullDebug, globals: !3, imports: !10, sysroot: "blah", sdk: "blah") !3 = !{!4, !11} !4 = !DIGlobalVariableExpression(var: !5, expr: !DIExpression()) !5 = distinct !DIGlobalVariable(name: "blah", linkageName: "blah", scope: !6, file: !7, line: 49, type: !8, isLocal: true, isDefinition: true) !6 = !DIModule(scope: null, name: "blah", includePath: "blah") !7 = !DIFile(filename: "blah", directory: "blah") !8 = !DIDerivedType(tag: DW_TAG_const_type, baseType: !9) !9 = !DICompositeType(tag: DW_TAG_structure_type, name: "blah", scope: !6, file: !7, size: 64, elements: !10, runtimeLang: DW_LANG_Swift, identifier: "blah") !10 = !{} !11 = !DIGlobalVariableExpression(var: !12, expr: !DIExpression()) !12 = distinct !DIGlobalVariable(name: "blah", linkageName: "blah", scope: !6, file: !7, line: 44, type: !14, isLocal: false, isDefinition: true) !14 = !DIDerivedType(tag: DW_TAG_const_type, baseType: !15) !15 = !DICompositeType(tag: DW_TAG_structure_type, name: "blah", scope: !17, file: !7, size: 64, elements: !10, runtimeLang: DW_LANG_Swift, identifier: "blah") !17 = !DIModule(scope: null, name: "blah", configMacros: "blah", includePath: "blah") !26 = distinct !DISubprogram(name: "blah", linkageName: "blah", scope: !28, file: !7, line: 115, type: !29, scopeLine: 117, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !1, retainedNodes: !10, thrownTypes: !10) !28 = !DICompositeType(tag: DW_TAG_structure_type, name: "blah", scope: !6, file: !7, elements: !10, runtimeLang: DW_LANG_Swift, identifier: "blah") !29 = !DISubroutineType(types: !10) !30 = !DILocalVariable(name: "blah", arg: 1, scope: !31, file: !7, line: 95, type: !33, flags: DIFlagArtificial) !31 = distinct !DISubprogram(name: "blah", linkageName: "blah", scope: !28, file: !7, line: 95, type: !32, scopeLine: 95, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !1, retainedNodes: !10) !32 = distinct !DISubroutineType(types: !10) !33 = !DIDerivedType(tag: DW_TAG_const_type, baseType: !28) !34 = !DILocation(line: 95, column: 9, scope: !31, inlinedAt: !35) !35 = distinct !DILocation(line: 0, scope: !36, inlinedAt: !38) !36 = distinct !DISubprogram(name: "blah", linkageName: "blah", scope: !28, file: !7, type: !32, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !1) !38 = distinct !DILocation(line: 121, column: 36, scope: !44) !44 = distinct !DILexicalBlock(scope: !26, file: !7, line: 116, column: 7) !45 = !DILocation(line: 0, scope: !46, inlinedAt: !35) !46 = !DILexicalBlockFile(scope: !31, file: !7, discriminator: 0)
Just to check -- this is purely for printing the informative comments on assembly output, it isn't actually functional, yes?