Fix PR44001: assert failure in getFunctionLocalOffsetAfterInsn

[thopre]

Summary

Assert in getFunctionLocalOffsetAfterInsn() fails when processing a call
MachineInstr inside a bundle and compiling with debug info. This is
because labels are added by DwarfDebug::beginInstruction() which is
called for each top-level MI by EmitFunctionBody()'s for-loop iteration
but constructCallSiteEntryDIEs() which calls
getFunctionLocalOffsetAfterInsn() iterates over all MIs.

This commit modifies constructCallSiteEntryDIEs() to get the associated
bundle MI for call MIs inside a bundle and use that to when calling
getFunctionLocalOffsetAfterInsn() and getLabelAfterInsn(). It also skips
loop iterations for bundle MIs since the loop statements are concerned
with debug info for each physical instructions and bundles represent a
group of instructions.

[djtodoro]

@thopre Thanks for the patch!

Can you please add another test case, by testing the Dwarf output produced by the AsmPrinter in this case (e.g. test the DW_TAG_call_sites and its attributes)?

llvm/test/DebugInfo/Hexagon/bundled-call-pr44001.ll

23 ↗

(On Diff #229453)

Usually, we don't need the attributes.

32 ↗

(On Diff #229453)

It is enough only producer: "clang version 10.0.0".

33 ↗

(On Diff #229453)

Better directory: "/"?

43 ↗

(On Diff #229453)

Same as above. (It is enough only producer: "clang version 10.0.0".)

[thopre]

Address comments about existing testcase.

[thopre]

In D70293#1747054, @djtodoro wrote:

@thopre Thanks for the patch!

Can you please add another test case, by testing the Dwarf output produced by the AsmPrinter in this case (e.g. test the DW_TAG_call_sites and its attributes)?

Will do, but will have to wait Monday I'm afraid.

[vsk]

This looks good to me, modulo prior reviewer comments. Thanks!

[thopre]

Check DWARF call site info in testcase

[thopre]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

771–779

BTW this seems wrong to me, it should probably be getLabelBeforeInsn(MI) but would need labels to be added before call MIs inside a bundle rather than after bundle MIs with a call MI. On the testcase below it gives a DW_AT_low_pc of 0x8 for the following code:

Disassembly of section .text:

00000000 foo:

0:       00 40 00 5a     5a004000 {      call 0x0
4:       00 c0 9d a0     a09dc000        allocframe(#0) } 
8:       40 3f 00 51     51003f40 {      r0 = add(r0,#1);        dealloc_return }

[dstenb]

This seems good!

As @djtodoro mentions, a test case which verifies the DWARF output would be good.

Downstream we have had some unrelated issues with requesting labels for bundled MIs, which we solved with making the requestLabel*() and getLabel*() functions do getBundleStart() for bundled instructions. We can probably look towards using the same approach as here for the issue(s) which caused us to add that bundle handling in the first case, and see if it's possible/valuable to upstream that. If so, the requestLabel*() and getLabel*() functions should probably have !isInsideBundle() assertions.

[djtodoro]

Thanks for the test, but do you mind creating a MIR test case? That will show more precisely what we are checking.

[dstenb]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

771–779

It seems that the DW_AT_low_pc attribute is the return address in the GNU extension, so I think that's correct.

The call site entry has a DW_AT_low_pc attribute which is the return address after the call. This corresponds to the return address computed by CFI in the called function (6.4).

http://lists.dwarfstd.org/pipermail/dwarf-discuss-dwarfstd.org/2010-August/002318.html

[thopre]

In D70293#1749752, @dstenb wrote:

This seems good!

As @djtodoro mentions, a test case which verifies the DWARF output would be good.

Downstream we have had some unrelated issues with requesting labels for bundled MIs, which we solved with making the requestLabel*() and getLabel*() functions do getBundleStart() for bundled instructions. We can probably look towards using the same approach as here for the issue(s) which caused us to add that bundle handling in the first case, and see if it's possible/valuable to upstream that. If so, the requestLabel*() and getLabel*() functions should probably have !isInsideBundle() assertions.

I believe labels get inserted in other places as well which might add some labels on actual MIs rather than bundle MIs so I'm not sure assert in those function would be appropriate.

[thopre]

Make it a MIR testcase

[thopre]

Fix comment for PCAddr

[thopre]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

771–779

Alright, fixed the comment accordingly then.

[thopre]

Note about the testcase: I had to remove the CFI directives from the MIR testcase because it wouldn't parse it. i.e. -stop-after=patchable-function followed by -start-after=patchable-function would not work unless I edit the CFI directives out.

[djtodoro]

This looks good to me (with minor nit included).

llvm/test/DebugInfo/Hexagon/bundled-call-pr44001.mir

28 ↗

(On Diff #229825)

You can delete this one, since it is not being used in the foo()function.

[thopre]

Simplify testcase further

[djtodoro]

Thanks!

[thopre]

• Fix RUN line for llc to detect MIR
• make CHECK for call return address more specific since MIR can safely be mapped to instructions

[djtodoro]

Sorry, but one more thing. The MIR test should go into llvm/test/DebugInfo/MIR/Hexagon.

[thopre]

Move testcase to llvm/test/DebugInfo/MIR/Hexagon

[djtodoro]

I am OK with the change (with the last nit included).

Please wait for someone else of the reviewers to confirm it as well. Thanks a lot for this!

llvm/test/DebugInfo/MIR/Hexagon/bundled-call-pr44001.mir

11

This should not contain your local path.

[dstenb]

LGTM. Thanks!

[thopre]

Remove local path from ModuleID field.

[thopre]

Committed as a89ca4ae179def7fac1a35e53d7b28d4c9d531be

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

void DwarfDebug::constructCallSiteEntryDIEs(const DISubprogram &SP,
 
   const TargetInstrInfo *TII = MF.getSubtarget().getInstrInfo();
   assert(TII && "TargetInstrInfo not found: cannot label tail calls");
   bool ApplyGNUExtensions = getDwarfVersion() == 4 && tuneForGDB();
 
   // Emit call site entries for each call or tail call in the function.
   for (const MachineBasicBlock &MBB : MF) {
     for (const MachineInstr &MI : MBB.instrs()) {
+      // Bundles with call in them will pass the isCall() test below but do not
+      // have callee operand information so skip them here. Iterator will
+      // eventually reach the call MI.
+      if (MI.isBundle())
+        continue;
+
       // Skip instructions which aren't calls. Both calls and tail-calling jump
       // instructions (e.g TAILJMPd64) are classified correctly here.
       if (!MI.isCall())
         continue;
 
       // TODO: Add support for targets with delay slots (see: beginInstruction).
       if (MI.hasDelaySlot())
         return;
for (const MachineInstr &MI : MBB.instrs()) {
           continue;
         CalleeSP = CalleeDecl->getSubprogram();
       }
 
       // TODO: Omit call site entries for runtime calls (objc_msgSend, etc).
 
       bool IsTail = TII->isTailCall(MI);
 
+      // If MI is in a bundle, the label was created after the bundle since
+      // EmitFunctionBody iterates over top-level MIs. Get that top-level MI
+      // to search for that label below.
+      const MachineInstr *TopLevelCallMI =
+          MI.isInsideBundle() ? &*getBundleStart(MI.getIterator()) : &MI;
+
       // For tail calls, for non-gdb tuning, no return PC information is needed.
       // For regular calls (and tail calls in GDB tuning), the return PC
       // is needed to disambiguate paths in the call graph which could lead to
       // some target function.
       const MCExpr *PCOffset =
-          (IsTail && !tuneForGDB()) ? nullptr
-                                    : getFunctionLocalOffsetAfterInsn(&MI);
-
-      // Address of a call-like instruction for a normal call or a jump-like
-      // instruction for a tail call. This is needed for GDB + DWARF 4 tuning.
+          (IsTail && !tuneForGDB())
+              ? nullptr
+              : getFunctionLocalOffsetAfterInsn(TopLevelCallMI);
+
+      // Return address of a call-like instruction for a normal call or a
+      // jump-like instruction for a tail call. This is needed for
+      // GDB + DWARF 4 tuning.
       const MCSymbol *PCAddr =
-          ApplyGNUExtensions ? const_cast<MCSymbol*>(getLabelAfterInsn(&MI))
-                             : nullptr;
+          ApplyGNUExtensions
+              ? const_cast<MCSymbol *>(getLabelAfterInsn(TopLevelCallMI))
 
       assert((IsTail || PCOffset || PCAddr) &&
              "Call without return PC information");
 
       LLVM_DEBUG(dbgs() << "CallSiteEntry: " << MF.getName() << " -> "
                         << (CalleeDecl ? CalleeDecl->getName()
                                        : StringRef(MF.getSubtarget()
                                                        .getRegisterInfo()
                                                        ->getName(CallReg)))

llvm/test/DebugInfo/MIR/Hexagon/bundled-call-pr44001.mir

+# RUN: llc -filetype=obj -start-after=patchable-function -O3 %s -o - | llvm-dwarfdump -v - | FileCheck %s
+
+# CHECK-LABEL: .debug_info contents:
+
+# CHECK-LABEL: DW_TAG_GNU_call_site
+# CHECK-NEXT: DW_AT_abstract_origin [DW_FORM_ref4] (cu + 0x[[BAR_ADDR:[0-9a-f]+]] => {0x{{0*}}[[BAR_ADDR]]} "bar")
+# CHECK-NEXT: DW_AT_low_pc [DW_FORM_addr] (0x0000000000000008 ".text")
+
+--- |
+  ; ModuleID = 'bundled-call-pr44001.ll'
+  source_filename = "bundled-call-pr44001.c"
+  target datalayout = "e-m:e-p:32:32:32-a:0-n16:32-i64:64:64-i32:32:32-i16:16:16-i1:8:8-f32:32:32-f64:64:64-v32:32:32-v64:64:64-v512:512:512-v1024:1024:1024-v2048:2048:2048"
+  target triple = "hexagon-unknown-unknown-elf"
+  
+  define dso_local i32 @foo(i32 %cond) local_unnamed_addr !dbg !12 {
+  entry:
+    call void @llvm.dbg.value(metadata i32 %cond, metadata !14, metadata !DIExpression()), !dbg !15
+    %call = tail call i32 @bar(i32 %cond), !dbg !16
+    %add = add nsw i32 %call, 1, !dbg !17
+    ret i32 %add, !dbg !18
+  }
+  
+  declare !dbg !4 dso_local i32 @bar(i32) local_unnamed_addr
+  
+  ; Function Attrs: nounwind readnone speculatable willreturn
+  declare void @llvm.dbg.value(metadata, metadata, metadata)
+  
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!8, !9, !10}
+  !llvm.ident = !{!11}
+  
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 10.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !3, nameTableKind: None)
+  !1 = !DIFile(filename: "bundled-call-pr44001.ll", directory: "/")
+  !2 = !{}
+  !3 = !{!4}
+  !4 = !DISubprogram(name: "bar", scope: !1, file: !1, line: 1, type: !5, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
+  !5 = !DISubroutineType(types: !6)
+  !6 = !{!7, !7}
+  !7 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !8 = !{i32 2, !"Dwarf Version", i32 4}
+  !9 = !{i32 2, !"Debug Info Version", i32 3}
+  !10 = !{i32 1, !"wchar_size", i32 4}
+  !11 = !{!"clang version 10.0.0"}
+  !12 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 4, type: !5, scopeLine: 4, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !13)
+  !13 = !{!14}
+  !14 = !DILocalVariable(name: "cond", arg: 1, scope: !12, file: !1, line: 4, type: !7)
+  !15 = !DILocation(line: 0, scope: !12)
+  !16 = !DILocation(line: 5, column: 10, scope: !12)
+  !17 = !DILocation(line: 5, column: 20, scope: !12)
+  !18 = !DILocation(line: 5, column: 3, scope: !12)
+
+...
+---
+name:            foo
+alignment:       16
+tracksRegLiveness: true
+frameInfo:
+  maxAlignment:    1
+  adjustsStack:    true
+  hasCalls:        true
+  maxCallFrameSize: 0
+machineFunctionInfo: {}
+body:             |
+  bb.0.entry:
+    liveins: $r0
+  
+    DBG_VALUE $r0, $noreg, !14, !DIExpression(), debug-location !15
+    DBG_VALUE $r0, $noreg, !14, !DIExpression(), debug-location !15
+    DBG_VALUE $r0, $noreg, !14, !DIExpression(), debug-location !15
+    BUNDLE implicit-def $r29, implicit-def $r30, implicit-def dead $pc, implicit-def dead $r31, implicit-def $r0, implicit $r29, implicit killed $framekey, implicit killed $framelimit, implicit killed $r30, implicit killed $r31, implicit $r0, debug-location !16 {
+      $r29 = S2_allocframe $r29, 0, implicit-def $r30, implicit killed $framekey, implicit killed $framelimit, implicit killed $r30, implicit killed $r31, debug-location !16 :: (store 4 into stack)
+      J2_call @bar, hexagoncsr, implicit-def dead $pc, implicit-def dead $r31, implicit internal killed $r29, implicit $r0, implicit-def $r29, implicit-def $r0, debug-location !16
+    }
+    BUNDLE implicit-def dead $r0, implicit-def $d15, implicit-def $r30, implicit-def $r31, implicit-def $pc, implicit-def $r29, implicit $r0, implicit killed $r30, implicit killed $framekey, debug-location !17 {
+      renamable $r0 = nsw A2_addi renamable $r0, 1, debug-location !17
+      $d15 = L4_return killed $r30, implicit-def $pc, implicit-def $r29, implicit killed $framekey, implicit-def dead $pc, implicit internal killed $r0, debug-location !18
+    }
+
+...