[DebugInfo][InstrRef] Handle llvm.frameaddress intrinsics gracefully

[jmorse]

Summary

When working out which instruction defines a value, the instruction-referencing variable location code has a few special cases for physical registers:

• Arguments are never defined by instructions,
• Constant physical registers always read the same value and are never def'd.

This patch adds a third case for the llvm.frameaddress intrinsics: you can read the framepointer in any block if you so choose, and use it as a variable location, as shown in the attached patch.

This rather violates one of the assumptions behind instruction referencing, that llvm-ir shouldn't be able to read from an arbitrary register at some arbitrary point in the program. The solution for now is to just emit a DBG_PHI that reads the register value: this works, but if we wanted to do something clever with DBG_PHIs in the future then this would probably get in the way. As it stands, this patch avoids a crash.

[Orlando]

LGTM

llvm/lib/CodeGen/MachineFunction.cpp

1149

The empty if body looks a little odd but makes sense to me. I don't think you need the ; though, unless this is to match prior art? No strong opinion.

llvm/test/DebugInfo/X86/instr-ref-framereg-read.ll

8–9

Why is it important to check outside of the entry block (it might be worth adding to the comment in the test too, ymmv)?

26

nit: there seems to be some wobbly indentation above

[jmorse]

llvm/test/DebugInfo/X86/instr-ref-framereg-read.ll

8–9

If the frame pointer is read in the entry block, there's a chance that it'll be interpreted as a function argument (i.e. a register not otherwise defined in the block). Depending on how the frame setup works, there might be instructions that def the frame register anyway in the entry block -- which wouldn't expose the problem this patch fixes.

(Folding this explanation into landing patch)

llvm/lib/CodeGen/MachineFunction.cpp

auto MachineFunction::salvageCopySSA(MachineInstr &MI)
   MachineBasicBlock &InsertBB = *CurInst->getParent();
 
   // We reached the start of the block before finding a defining instruction.
   // It could be from a constant register, otherwise it must be an argument.
   if (TRI.isConstantPhysReg(State.first)) {
     // We can produce a DBG_PHI that identifies the constant physreg. Doesn't
     // matter where we put it, as it's constant valued.
     assert(CurInst->isCopy());
+  } else if (State.first == TRI.getFrameRegister(*this)) {
+    // LLVM IR is allowed to read the framepointer by calling a
+    // llvm.frameaddress.* intrinsic. We can support this by emitting a
+    // DBG_PHI $fp. This isn't ideal, because it extends the behaviours /
+    // position that DBG_PHIs appear at, limiting what can be done later.
+    // TODO: see if there's a better way of expressing these variable
+    // locations.
+    ;
   } else {
     // Assert that this is the entry block. If it isn't, then there is some
     // code construct we don't recognise that deals with physregs across
     // blocks.
     assert(!State.first.isVirtual());
     assert(&*InsertBB.getParent()->begin() == &InsertBB);
   }
 

llvm/test/DebugInfo/X86/instr-ref-framereg-read.ll

+; RUN: llc %s -o - -start-after=codegenprepare -stop-before=finalize-isel \
+; RUN:     -mtriple=x86_64-unknown-unknown -experimental-debug-variable-locations \
+; RUN: | FileCheck %s
+
+; Test for a crash / weird behaviour when llvm.frameaddress.* is called.
+; Today, as a concession, we emit a DBG_PHI allowing the frame register value
+; to be read, but it could be expressed in other ways. Check that this works
+; outside of the entry block, to avoid the frame register being recognised
+; as def'd by frame-setup code or as a function argument.
+
+; CHECK-LABEL: bb.1.notentry:
+; CHECK: DBG_PHI $rbp
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+declare i8 *@llvm.frameaddress.p0i8(i32)
+
+ ; Function Attrs: mustprogress nofree nosync nounwind sspstrong uwtable
+define hidden i8 * @foo() !dbg !7 {
+entry:
+  br label  %notentry
+
+notentry:
+  %0 = tail call i8* @llvm.frameaddress.p0i8(i32 0), !dbg !12
+  call void @llvm.dbg.value(metadata i8* %0, metadata !11, metadata !DIExpression()), !dbg !12
+  ret i8 *%0
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !6}
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+!1 = !DIFile(filename: "foo.cpp", directory: ".")
+!2 = !DIBasicType(name: "int", size: 8, encoding: DW_ATE_signed)
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 2}
+!6 = !{i32 7, !"PIC Level", i32 2}
+!7 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: !1, file: !1, line: 6, type: !8, scopeLine: 6, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !10)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!2, !2}
+!10 = !{!11}
+!11 = !DILocalVariable(name: "baz", scope: !7, file: !1, line: 7, type: !2)
+!12 = !DILocation(line: 10, scope: !7)