[DebugInfo] Avoid adding too much indirection to pointer-valued variables

[jmorse]

Summary

This patch addresses PR41675 [0], where a stack-pointer variable is dereferenced too many times by its location expression, presenting a value on the stack as the pointer to the stack.

The difference between a stack *pointer* DBG_VALUE and one that refers to a value on the stack, is currently the indirect flag. In the below, the former is the address of, the latter the value in %stack_loc:

DBG_VALUE %stack_loc, $noreg, !1, !DIExpression()
DBG_VALUE %stack_loc, 0, !1, !DIExpression()

When the prologepilog pass runs, the stack references are replaced with complex expression opcodes:

DBG_VALUE $rsp, $noreg, !1, !DIExpression(DW_OP_plus_uconst, 4)
DBG_VALUE $rsp, 0, !1, !DIExpression(DW_OP_plus_uconst, 4)

Unfortunately, the DWARF backend interprets both of these as being memory locations. For the second DBG_VALUE this is because it's explicitly indirect, but for the first it's because there's a complex expression without DW_OP_stack_value.

Get around this by detecting empty-to-non-empty DIExpression transitions in prologepilog, and adding DW_OP_stack_value to them. This ensures that pointers-to-stack-locs are interpreted as implicit locations, which is what they are.

For the edge case where the offset is zero and the location could be a register location, DIExpression::prepend will still generate opcodes, and thus DW_OP_stack_value must still be added.

[0] https://bugs.llvm.org/show_bug.cgi?id=41675

[aprantl]

lib/CodeGen/PrologEpilogInserter.cpp

1191

The only thing I'm concerned about is the DIExpr->getNumElements() == 0. This sounds like it would also reject other cases where we'd want this to apply, such as an expression that only contains a DW_OP_LLVM_fragment. Is there a better condition? Perhaps by inverting it? What's an example of an expression where we don't want this to happen?

[jmorse]

Add an isComplex() method to DIExpression that detects when an expression could be interpreted as a memory location.

[jmorse]

lib/CodeGen/PrologEpilogInserter.cpp

1191

Oooofff, indeed, that definitely would cause problems. The current test that DwarfExpression makes [0] to see whether something is a simple register location or not, is whether the expression is:

• Empty, or
• Only a fragment

Anything else involves computing values on the expression stack: that makes it a memory location unless DW_OP_stack_value is added. I've added a method to perform that check in isComplex, and use that in PrologEpilog. I can't replace the DwarfExpression test with that method unfortunately, as it's passing around expr_operand iterators rather than DIExpressions.

[0] https://github.com/llvm/llvm-project/blob/fb9ce100d19be130d004d03088ccd4af295f3435/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp#L229

[jmorse]

lib/CodeGen/PrologEpilogInserter.cpp

1191

Hmmn, isComplex will also have to account for DW_OP_LLVM_tag_offset that's just been added. I'll need some time to pull that in and rebuild, will have that patch up shortly.

[jmorse]

Account for DW_OP_LLVM_tag_offset when working out whether a DIExpression is "complex" or not.

[vsk]

This looks reasonable to me. Mind adding test coverage for simple tag_offset/fragment expressions?

[jmorse]

Vedant wrote:

This looks reasonable to me. Mind adding test coverage for simple tag_offset/fragment expressions?

Updated with two new variables / DBG_VALUEs to cover that -- as it turns out I was using the wrong kind of iterator in isComplex :(. Iterating elements seems to iterate over all expression storage, iterating over operands is what was needed. Great catch!

[vsk]

Thanks!

lib/CodeGen/PrologEpilogInserter.cpp

for (unsigned i = 0, e = MI.getNumOperands(); i != e; ++i) {
         unsigned Size = MF.getFrameInfo().getObjectSize(FrameIdx);
 
         int64_t Offset =
             TFI->getFrameIndexReference(MF, FrameIdx, Reg);
         MI.getOperand(0).ChangeToRegister(Reg, false /*isDef*/);
         MI.getOperand(0).setIsDebug();
 
         const DIExpression *DIExpr = MI.getDebugExpression();
+
+        // If we have a direct DBG_VALUE, and its complex expr is currently
+        // empty, then adding an offset will morph it into a memory location,
+        // when it is currently equivalent to a register location. This changes
+        // a pointer-valued variable to dereference that pointer, which is
+        // incorrect.
+        unsigned PrependFlags = DIExpression::ApplyOffset;
+        if (!MI.isIndirectDebugValue() && DIExpr->getNumElements() == 0)
+          PrependFlags |= DIExpression::StackValue;
+
         // If we have DBG_VALUE that is indirect and has a Implicit location
         // expression need to insert a deref before prepending a Memory
         // location expression. Also after doing this we change the DBG_VALUE
         // to be direct.
         if (MI.isIndirectDebugValue() && DIExpr->isImplicit()) {
           SmallVector<uint64_t, 2> Ops = {dwarf::DW_OP_deref_size, Size};
           bool WithStackValue = true;
           DIExpr = DIExpression::prependOpcodes(DIExpr, Ops, WithStackValue);
           // Make the DBG_VALUE direct.
           MI.getOperand(1).ChangeToRegister(0, false);
         }
-        DIExpr =
-            DIExpression::prepend(DIExpr, DIExpression::ApplyOffset, Offset);
+        DIExpr = DIExpression::prepend(DIExpr, PrependFlags, Offset);
         MI.getOperand(3).setMetadata(DIExpr);
         continue;
       }
 
       // TODO: This code should be commoned with the code for
       // PATCHPOINT. There's no good reason for the difference in
       // implementation other than historical accident.  The only
       // remaining difference is the unconditional use of the stack

test/DebugInfo/MIR/X86/prolog-epilog-indirection.mir

+# RUN: llc %s -x mir -o - -mtriple=x86_64-unknown-unknown -run-pass=prologepilog | FileCheck %s
+#
+# Check when the DBG_VALUE on a stack slot below (for var "c") has its  stack
+# slot replaced with $rsp and a complex expression, it has DW_OP_stack_value
+# added. A direct reference to the stack slot is considered to be the _address_
+# of that stack slot, wheras its contents would be an indirect DBG_VALUE.
+#
+# Check too that for the same DBG_VALUE inst, with an indirect reference to
+# the stack slot, we do _not_ get DW_OP_plus_uconst added. This expression
+# should remain indirect, referring to the contents of the stack slot.
+#
+# CHECK:      ![[VAR:[0-9]+]] = !DILocalVariable(name: "c"
+#
+# CHECK:      LEA64r $rsp
+# CHECK-NEXT: DBG_VALUE $rsp, $noreg, ![[VAR]], !DIExpression(DW_OP_plus_uconst, {{[0-9]+}}, DW_OP_stack_value)
+# CHECK-NEXT: DBG_VALUE 1834104526
+# CHECK-NEXT: MOV64mr
+# CHECK-NEXT: DBG_VALUE $rsp, 0, ![[VAR]], !DIExpression(DW_OP_plus_uconst, {{[0-9]+}})
+
+--- |
+  ; ModuleID = 'out.ll'
+  source_filename = "abc.c"
+  target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+  target triple = "x86_64-unknown-linux-gnu"
+  
+  @b = common dso_local local_unnamed_addr global i32* null, align 8, !dbg !0
+  @a = common dso_local local_unnamed_addr global i32 0, align 4, !dbg !6
+  
+  ; Function Attrs: nounwind uwtable
+  define dso_local i32 @main() local_unnamed_addr !dbg !14 {
+  entry:
+    %l_1081 = alloca i32, align 4
+    %0 = bitcast i32* %l_1081 to i8*, !dbg !20
+    call void @llvm.lifetime.start.p0i8(i64 4, i8* nonnull %0), !dbg !20
+    call void @llvm.dbg.value(metadata i32 1834104526, metadata !18, metadata !DIExpression()), !dbg !21
+    call void @llvm.dbg.value(metadata i32* %l_1081, metadata !19, metadata !DIExpression()), !dbg !21
+    store i32* %l_1081, i32** @b, align 8, !dbg !22, !tbaa !23
+    store i32 9, i32* @a, align 4, !dbg !27, !tbaa !28
+    store i32 9, i32* %l_1081, align 4, !dbg !30, !tbaa !28
+    %call = call i32 (...) @optimize_me_not(), !dbg !31
+    call void @llvm.lifetime.end.p0i8(i64 4, i8* nonnull %0), !dbg !32
+    ret i32 0, !dbg !32
+  }
+  
+  ; Function Attrs: argmemonly nounwind
+  declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture)
+  
+  declare dso_local i32 @optimize_me_not(...) local_unnamed_addr
+  
+  ; Function Attrs: argmemonly nounwind
+  declare void @llvm.lifetime.end.p0i8(i64 immarg, i8* nocapture)
+  
+  ; Function Attrs: nounwind readnone speculatable
+  declare void @llvm.dbg.value(metadata, metadata, metadata)
+  
+  !llvm.dbg.cu = !{!2}
+  !llvm.module.flags = !{!10, !11, !12}
+  !llvm.ident = !{!13}
+  
+  !0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+  !1 = distinct !DIGlobalVariable(name: "b", scope: !2, file: !3, line: 2, type: !9, isLocal: false, isDefinition: true)
+  !2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !4, globals: !5, nameTableKind: None)
+  !3 = !DIFile(filename: "abc.c", directory: ".")
+  !4 = !{}
+  !5 = !{!6, !0}
+  !6 = !DIGlobalVariableExpression(var: !7, expr: !DIExpression())
+  !7 = distinct !DIGlobalVariable(name: "a", scope: !2, file: !3, line: 1, type: !8, isLocal: false, isDefinition: true)
+  !8 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !9 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !8, size: 64)
+  !10 = !{i32 2, !"Dwarf Version", i32 4}
+  !11 = !{i32 2, !"Debug Info Version", i32 3}
+  !12 = !{i32 1, !"wchar_size", i32 4}
+  !13 = !{!"clang"}
+  !14 = distinct !DISubprogram(name: "main", scope: !3, file: !3, line: 3, type: !15, scopeLine: 3, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !2, retainedNodes: !17)
+  !15 = !DISubroutineType(types: !16)
+  !16 = !{!8}
+  !17 = !{!18, !19}
+  !18 = !DILocalVariable(name: "l_1081", scope: !14, file: !3, line: 4, type: !8)
+  !19 = !DILocalVariable(name: "c", scope: !14, file: !3, line: 5, type: !9)
+  !20 = !DILocation(line: 4, column: 3, scope: !14)
+  !21 = !DILocation(line: 0, scope: !14)
+  !22 = !DILocation(line: 6, column: 5, scope: !14)
+  !23 = !{!24, !24, i64 0}
+  !24 = !{!"any pointer", !25, i64 0}
+  !25 = !{!"omnipotent char", !26, i64 0}
+  !26 = !{!"Simple C/C++ TBAA"}
+  !27 = !DILocation(line: 7, column: 10, scope: !14)
+  !28 = !{!29, !29, i64 0}
+  !29 = !{!"int", !25, i64 0}
+  !30 = !DILocation(line: 7, column: 6, scope: !14)
+  !31 = !DILocation(line: 8, column: 3, scope: !14)
+  !32 = !DILocation(line: 9, column: 1, scope: !14)
+
+...
+---
+name:            main
+alignment:       4
+tracksRegLiveness: true
+frameInfo:       
+  maxAlignment:    4
+  hasCalls:        true
+stack:           
+  - { id: 0, name: l_1081, type: default, offset: 0, size: 4, alignment: 4, 
+      stack-id: 0, callee-saved-register: '', callee-saved-restored: true, 
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+body:             |
+  bb.0.entry:
+    renamable $rax = LEA64r %stack.0.l_1081, 1, $noreg, 0, $noreg
+    DBG_VALUE %stack.0.l_1081, $noreg, !19, !DIExpression(), debug-location !21
+    DBG_VALUE 1834104526, $noreg, !18, !DIExpression(), debug-location !21
+    MOV64mr $rip, 1, $noreg, @b, $noreg, killed renamable $rax, debug-location !22 :: (store 8 into @b, !tbaa !23)
+    DBG_VALUE %stack.0.l_1081, 0, !19, !DIExpression(), debug-location !21
+    MOV32mi $rip, 1, $noreg, @a, $noreg, 9, debug-location !27 :: (store 4 into @a, !tbaa !28)
+    MOV32mi %stack.0.l_1081, 1, $noreg, 0, $noreg, 9, debug-location !30 :: (store 4 into %ir.l_1081, !tbaa !28)
+    ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp, debug-location !31
+    dead $eax = MOV32r0 implicit-def dead $eflags, implicit-def $al, debug-location !31
+    CALL64pcrel32 @optimize_me_not, csr_64, implicit $rsp, implicit $ssp, implicit $al, implicit-def $rsp, implicit-def $ssp, implicit-def dead $eax, debug-location !31
+    ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp, debug-location !31
+    $eax = MOV32r0 implicit-def dead $eflags, debug-location !32
+    RET 0, $eax, debug-location !32
+
+...