This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Allow non-stack_value variadic expressions and use in DBG_INSTR_REF
ClosedPublic

Authored by StephenTozer on Sep 15 2022, 3:01 AM.

Details

Summary

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.

Diff Detail

Event Timeline

StephenTozer created this revision.Sep 15 2022, 3:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2022, 3:01 AM
StephenTozer requested review of this revision.Sep 15 2022, 3:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2022, 3:01 AM

Move test changes ahead into this patch.

jmorse added a comment.Oct 6 2022, 5:08 AM

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
1140–1145

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
1530–1538

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.

jmorse added a comment.Oct 6 2022, 5:09 AM

(Lo and behold, I already asked that last question in D133929, never mind about that)

StephenTozer added inline comments.Oct 6 2022, 5:37 AM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1140–1145

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
1530–1538

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.

StephenTozer added inline comments.Nov 7 2022, 9:39 AM
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.

jmorse accepted this revision.Dec 20 2022, 7:18 AM

Looks good (leaning largely on my prior review).

This revision is now accepted and ready to land.Dec 20 2022, 7:18 AM

Rebased onto latest master, added unit tests for convertToVariadicExpression.

This revision was landed with ongoing or failed builds.Jan 6 2023, 11:32 AM
This revision was automatically updated to reflect the committed changes.

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

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.

fdeazeve added a comment.EditedAug 16 2023, 10:26 AM

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.

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?

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)

Could you give this a try? It repros for me with a llc test.ll.

Repro is good, review opened: D158185