[DebugInfo] Call site entries cannot be generated for FrameSetup calls

[lewis-revill]

Summary

Instructions marked as FrameSetup do not cause requestLabelAfterInsn to be called and so no such label is generated. Call instructions which require call site entries to be generated require this label to be present in order to calculate the return PC offset/address, but the check for whether the call instruction is marked as FrameSetup was not present.

Therefore in the case where a call instruction is marked as FrameSetup, an assertion failure occurs if a call site entry is to be generated. This is the case with RISC-V's implementation of save/restore via library calls (see https://reviews.llvm.org/D62686).

[shiva0217]

LGTM, Labels required to describe call site scope will be generated for the calls after FrameSetup instructions. When the SaveLibcall mark as FrameSetup, the Label will not be generated, so constructing call site entry for SaveLibcall will trigger an assertion. Given that SaveLibcall should be part of the prologue, so I think generating call site begin after FrameSetup instructions should be reasonable. But I hope there could be a second look.

[dblaikie]

What about going the other way, and making sure instructions in frame setup that need labels get them?

[djtodoro]

Please add a test case.

[lewis-revill]

In D71593#1810372, @dblaikie wrote:

What about going the other way, and making sure instructions in frame setup that need labels get them?

I guess it depends if people think there is an advantage to having compiler generated calls within frame setup labelled with DW_AT_return_pc. I don't know debug information well enough to say if it makes sense or not.

[lewis-revill]

In D71593#1816377, @djtodoro wrote:

Please add a test case.

I think adding a test case would make this patch and save/restore codependent, unless there is another way to generate a call marked as frame-setup.

[dblaikie]

In D71593#1816687, @lewis-revill wrote:

In D71593#1810372, @dblaikie wrote:

What about going the other way, and making sure instructions in frame setup that need labels get them?

I guess it depends if people think there is an advantage to having compiler generated calls within frame setup labelled with DW_AT_return_pc. I don't know debug information well enough to say if it makes sense or not.

Would be good to rope someone who is familiar to chime in on this review, then.

In D71593#1816698, @lewis-revill wrote:

In D71593#1816377, @djtodoro wrote:

Please add a test case.

I think adding a test case would make this patch and save/restore codependent, unless there is another way to generate a call marked as frame-setup.

It's not uncommon that a test in one feature might need to depend on certain functionality in another feature (most abundantly - LLVM IR parsing/emission is exercised in lots of other tests). Is the codependence you're referring to likely to be especially brittle? (are there no simple, unambiguous cases that would trigger this codepath for testing that aren't likely to be purturbed by improvements/changes to save/restore?)

[djtodoro]

If this depends on another patch please make a proper stack.
Anyhow, when we have the stack of patches (we would know that this goes after the feature you are mentioning), we could have the testcase as well.

[probinson]

It's hard for me to imagine a case where a frame-setup prologue requires a call that really wants debug info. For the save/restore example in D62686, the calls are to system/library helpers to support a size optimization, and as such seem like an implementation detail (along the lines of, say, implementing integer divide as a lib call when you don't have a hardware instruction). So, skipping FrameSetup instructions seems like the right tactic, although the justification in the comment isn't really the right reason.

I can see that a test here could depend on save/restore landing first, but I don't see them as co-dependent. The other one introduces the calls that become an issue for debug info, this one fixes the debug-info issue.

[vsk]

In D71593#1851876, @probinson wrote:

It's hard for me to imagine a case where a frame-setup prologue requires a call that really wants debug info.

+ 1, I think what this patch is trying to do makes sense.

For the save/restore example in D62686, the calls are to system/library helpers to support a size optimization, and as such seem like an implementation detail (along the lines of, say, implementing integer divide as a lib call when you don't have a hardware instruction). So, skipping FrameSetup instructions seems like the right tactic, although the justification in the comment isn't really the right reason.

I can see that a test here could depend on save/restore landing first, but I don't see them as co-dependent. The other one introduces the calls that become an issue for debug info, this one fixes the debug-info issue.

Yeah, unless the issue is that the MIR parser won't accept frame-setup <call instruction> right now, but it might be possible to split that part out of the save/restore patch.

[lewis-revill]

Thanks all for the feedback, I'll make this patch depend upon the save/restore feature so that I can add a testcase.

In D71593#1851876, @probinson wrote:

It's hard for me to imagine a case where a frame-setup prologue requires a call that really wants debug info. For the save/restore example in D62686, the calls are to system/library helpers to support a size optimization, and as such seem like an implementation detail (along the lines of, say, implementing integer divide as a lib call when you don't have a hardware instruction). So, skipping FrameSetup instructions seems like the right tactic, although the justification in the comment isn't really the right reason.

What should the comment say?

[probinson]

In D71593#1857072, @lewis-revill wrote:

In D71593#1851876, @probinson wrote:

It's hard for me to imagine a case where a frame-setup prologue requires a call that really wants debug info. For the save/restore example in D62686, the calls are to system/library helpers to support a size optimization, and as such seem like an implementation detail (along the lines of, say, implementing integer divide as a lib call when you don't have a hardware instruction). So, skipping FrameSetup instructions seems like the right tactic, although the justification in the comment isn't really the right reason.

What should the comment say?

Currently, the comment implies that we *would* want to handle these calls, except that we don't have a piece of infrastructure in place yet (i.e., the missing labels). That is, it reads like a FIXME remark. IMO we don't want to handle these calls, even if we did have those labels; that is, skipping these calls is always the right thing to do. I would write the comment as something like:
// Skip instructions marked as frame setup, as they are not interesting to the user.

[lewis-revill]

Updated comment and added brief testcase, which now depends on D62686

[vsk]

LGTM.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

for (const MachineInstr &MI : MBB.instrs()) {
       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.isCandidateForCallSiteEntry())
         continue;
 
+      // Skip instructions marked as frame setup, as they are not interesting to
+      // the user.
+      if (MI.getFlag(MachineInstr::FrameSetup))
+        continue;
+
       // TODO: Add support for targets with delay slots (see: beginInstruction).
       if (MI.hasDelaySlot())
         return;
 
       // If this is a direct call, find the callee's subprogram.
       // In the case of an indirect call find the register that holds
       // the callee.
       const MachineOperand &CalleeOp = MI.getOperand(0);

llvm/test/DebugInfo/RISCV/saverestore.ll

+; RUN: llc -mtriple=riscv32 -mattr=+save-restore < %s
+
+; Ensure that the addition of framesetup instructions which call save/restore
+; libcalls do not cause a crash when DIFlagAllCallsDescribed is set.
+
+define i32 @main() noreturn nounwind !dbg !7 {
+entry:
+  tail call void @exit(i32 0)
+  unreachable
+}
+
+declare void @exit(i32) noreturn
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "saverestore.c", directory: ".")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 5}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang"}
+!7 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 1, type: !8, scopeLine: 1, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)