Fix an assertion error when emitting call site info that combines two DW_OP_stack_values.

[vsk]

Summary

When compiling

struct S {
  float w;
};
void f(long w, long b);
void g(struct S s) {
  int w = s.w;
  f(w, w*4);
}

I get Assertion failed: ((!CombinedExpr || CombinedExpr->isValid()) && "Combined debug expression is invalid").

That's because we combine two epxressions that both end in DW_OP_stack_value:

(lldb) p Expr->dump()
!DIExpression(DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_LLVM_convert, 64, DW_ATE_signed, DW_OP_stack_value)
(lldb) p Param.Expr->dump()
!DIExpression(DW_OP_constu, 4, DW_OP_mul, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_LLVM_convert, 64, DW_ATE_signed, DW_OP_stack_value)
(lldb) p CombinedExpr->isValid()
(bool) $0 = false
(lldb) p CombinedExpr->dump()
!DIExpression(4097, 32, 5, 4097, 64, 5, 16, 4, 30, 4097, 32, 5, 4097, 64, 5, 159, 159)

I believe that in this particular case combining two stack values is safe, but I didn't want to sink the special handlingi into DIExpression::append() because I do want everyone to think about what they are doing.

rdar://problem/60383095

[aprantl]

I believe that that's the same assertion failure that @thakis found in Chromium today.

[manojgupta]

Can you also test on the reduced test cases in https://bugs.llvm.org/show_bug.cgi?id=45181 ?

[dstenb]

In D76164#1922504, @manojgupta wrote:

Can you also test on the reduced test cases in https://bugs.llvm.org/show_bug.cgi?id=45181 ?

Sorry for taking some extra time with that PR. This patch should fix that assertion, but we would emit invalid debug information, related to D76146, for that case.

[dstenb]

In D76164#1922542, @dstenb wrote:

Sorry for taking some extra time with that PR. This patch should fix that assertion, but we would emit invalid debug information, related to D76146, for that case.

But we already emit such debug information in other cases, so it's probably not a big issue.

[djtodoro]

llvm/test/DebugInfo/MIR/X86/callsite-stack-value.mir

3

Since the D73534 was reverted, this should be -debug-entry-values instead of -emit-debug-entry-values.

[djtodoro]

llvm/test/DebugInfo/MIR/X86/callsite-stack-value.mir

3

And I guess we can add --implicit-check-not=DW_TAG_GNU_call_site_parameter, since we are avoiding the production of call_site_params in this case.

[djtodoro]

Basically, looks good to me, but please post the patch with -U999999.

llvm/test/DebugInfo/MIR/X86/callsite-stack-value.mir

16

since we deleted the attributes, we can delete #0

20

as well as the #1

27

we can get rid of the ([e-mail]:llvm/llvm-project 32e90cbcd19a83e20a86bfc1cbf7cec9729e9077) part

[djtodoro]

In D76164#1922552, @dstenb wrote:

In D76164#1922542, @dstenb wrote:

Sorry for taking some extra time with that PR. This patch should fix that assertion, but we would emit invalid debug information, related to D76146, for that case.

But we already emit such debug information in other cases, so it's probably not a big issue.

Since the patch (D76146) will solve the debug info issue in general, I guess this can go anyway?

[vsk]

@djtodoro Perhaps I'm missing something, but it doesn't look like D76146 would address the issue of there being two DW_OP_stack_values in the combined expression. So I think something along the lines of this patch will be needed. I'll commandeer this, per Adrian's request.

[aprantl]

Thanks for shepherding this patch, Vedant!

[vsk]

Address review feedback.

[djtodoro]

In D76164#1925521, @vsk wrote:

@djtodoro Perhaps I'm missing something, but it doesn't look like D76146 would address the issue of there being two DW_OP_stack_values in the combined expression. So I think something along the lines of this patch will be needed.

Sorry for not being clear enough, but I think we need this. I just wanted to confirm that fact, with the latest comment. :)

[djtodoro]

In D76164#1925985, @djtodoro wrote:

In D76164#1925521, @vsk wrote:

@djtodoro Perhaps I'm missing something, but it doesn't look like D76146 would address the issue of there being two DW_OP_stack_values in the combined expression. So I think something along the lines of this patch will be needed.

Sorry for not being clear enough, but I think we need this. I just wanted to confirm that fact, with the latest comment. :)

And "the debug info issue" was referring to the issue regarding sign/zero exts debug info (David mentioned above), I did not want to point on this problem, so we are on the same page.

[dstenb]

llvm/test/DebugInfo/MIR/X86/callsite-stack-value.mir

6

Should this be a CHECK-NOT:?

[djtodoro]

llvm/test/DebugInfo/MIR/X86/callsite-stack-value.mir

6

+1

I would also prefer adding a check for not having a DW_TAG_call_site_parameter generated.

[aprantl]

llvm/lib/IR/DebugInfoMetadata.cpp

1116

typo: "concatenated" or "appended"?

[vsk]

Fix typo, tighten up test.

[aprantl]

Thanks!

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

assert(none_of(ParamsForFwdReg,
                      return D.ParamReg == Param.ParamReg;
                    }) &&
            "Same parameter described twice by forwarding reg");
 
     // If a parameter's call site value is produced by a chain of
     // instructions we may have already created an expression for the
     // parameter when walking through the instructions. Append that to the
     // new expression.
+    std::vector<uint64_t> ParamElts = Param.Expr->getElements().vec();
+    // Avoid multiple DW_OP_stack_values.
+    if (Expr->isImplicit() && Param.Expr->isImplicit())
+      erase_if(ParamElts,
+               [](uint64_t Op) { return Op == dwarf::DW_OP_stack_value; });
     const DIExpression *CombinedExpr =
         (Param.Expr->getNumElements() > 0)
-            ? DIExpression::append(Expr, Param.Expr->getElements())
+            ? DIExpression::append(Expr, ParamElts)
             : Expr;
-    assert(CombinedExpr->isValid() && "Combined debug expression is invalid");
-
     ParamsForFwdReg.push_back({Param.ParamReg, CombinedExpr});
   }
 }
 
 /// Try to interpret values loaded into registers that forward parameters
 /// for \p CallMI. Store parameters with interpreted value into \p Params.
 static void collectCallSiteParameters(const MachineInstr *CallMI,
                                       ParamSet &Params) {

llvm/lib/IR/DebugInfoMetadata.cpp

if (Op.getOp() == dwarf::DW_OP_stack_value ||
 
       // Ensure that the new opcodes are only appended once.
       Ops = None;
     }
     Op.appendToVector(NewOps);
   }
 
   NewOps.append(Ops.begin(), Ops.end());
-  return DIExpression::get(Expr->getContext(), NewOps);
+  auto *result = DIExpression::get(Expr->getContext(), NewOps);
+  assert(result->isValid() && "concatenated expression is not valid");
+  return result;
 }
 
 DIExpression *DIExpression::appendToStack(const DIExpression *Expr,
                                           ArrayRef<uint64_t> Ops) {
   assert(Expr && !Ops.empty() && "Can't append ops to this expression");
   assert(none_of(Ops,
                  [](uint64_t Op) {
                    return Op == dwarf::DW_OP_stack_value ||

llvm/test/DebugInfo/MIR/X86/callsite-stack-value.mir

+# RUN: llc -start-after=livedebugvalues -mtriple=x86_64-apple-darwin -o - %s -filetype=obj \
+# RUN:   -emit-call-site-info -debug-entry-values | llvm-dwarfdump - | FileCheck %s -implicit-check-not=call_site_parameter
+
+# CHECK: DW_TAG_formal_parameter
+# CHECK-NEXT: DW_AT_location
+# CHECK-NEXT: DW_OP_reg17
+
+# struct S {
+#   float w;
+# };
+# void f(long w, long b);
+# void g(struct S s) {
+#   int w = s.w;
+#   f(w, w*4);
+# }
+
+--- |
+  define void @g(float %s.coerce) local_unnamed_addr !dbg !20 {
+  entry:
+    ret void, !dbg !31
+  }
+  declare !dbg !14 void @f(i64, i64) local_unnamed_addr
+  !llvm.module.flags = !{!6, !7, !8, !9}
+  !llvm.dbg.cu = !{!10}
+  !6 = !{i32 7, !"Dwarf Version", i32 4}
+  !7 = !{i32 2, !"Debug Info Version", i32 3}
+  !8 = !{i32 1, !"wchar_size", i32 4}
+  !9 = !{i32 7, !"PIC Level", i32 2}
+  !10 = distinct !DICompileUnit(language: DW_LANG_ObjC, file: !15, producer: "", isOptimized: true, runtimeVersion: 2, emissionKind: FullDebug, enums: !12, retainedTypes: !13, nameTableKind: None, sysroot: "/")
+  !12 = !{}
+  !13 = !{!14}
+  !14 = !DISubprogram(name: "f", scope: !15, file: !15, line: 4, type: !16, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !12)
+  !15 = !DIFile(filename: "test.m", directory: "/")
+  !16 = !DISubroutineType(types: !17)
+  !17 = !{null, !18, !18}
+  !18 = !DIBasicType(name: "long int", size: 64, encoding: DW_ATE_signed)
+  !20 = distinct !DISubprogram(name: "g", scope: !15, file: !15, line: 5, type: !21, scopeLine: 5, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !10, retainedNodes: !27)
+  !21 = !DISubroutineType(types: !22)
+  !22 = !{null, !23}
+  !23 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "S", file: !15, line: 1, size: 32, elements: !24)
+  !24 = !{!25}
+  !25 = !DIDerivedType(tag: DW_TAG_member, name: "w", scope: !23, file: !15, line: 2, baseType: !26, size: 32)
+  !26 = !DIBasicType(name: "float", size: 32, encoding: DW_ATE_float)
+  !27 = !{!28, !29}
+  !28 = !DILocalVariable(name: "s", arg: 1, scope: !20, file: !15, line: 5, type: !23)
+  !29 = !DILocalVariable(name: "w", scope: !20, file: !15, line: 6, type: !30)
+  !30 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !31 = !DILocation(line: 0, scope: !20)
+name:            g
+callSites:
+  - { bb: 0, offset: 13, fwdArgRegs: 
+      - { arg: 1, reg: '$rsi' } }
+body:             |
+  bb.0.entry:
+    DBG_VALUE $xmm0, $noreg, !28, !DIExpression(), debug-location !31
+    DBG_VALUE $xmm0, $noreg, !28, !DIExpression(), debug-location !31
+    frame-setup PUSH64r killed $rbp, implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 16
+    CFI_INSTRUCTION offset $rbp, -16
+    $rbp = frame-setup MOV64rr $rsp
+    CFI_INSTRUCTION def_cfa_register $rbp
+    renamable $eax = nofpexcept CVTTSS2SIrr killed renamable $xmm0, implicit $mxcsr, debug-location !31
+    DBG_VALUE $eax, $noreg, !29, !DIExpression(), debug-location !31
+    renamable $rdi = MOVSX64rr32 killed renamable $eax, debug-location !31
+    renamable $eax = LEA64_32r $noreg, 4, renamable $rdi, 0, $noreg, debug-location !31
+    renamable $rsi = MOVSX64rr32 killed renamable $eax, debug-location !31
+    $rbp = frame-destroy POP64r implicit-def $rsp, implicit $rsp, debug-location !31
+    TAILJMPd64 @f, csr_64, implicit $rsp, implicit $ssp, implicit $rsp, implicit $ssp, implicit $rdi, implicit $rsi, debug-location !31