Ensure proper call frame information metadata is set

[asb]

The current frame handling code doesn't do this. Although auditing debug info correctness on general could be more involved, I'm marking this bug as "easy" as just handling and testing call frame information shouldn't be much hassle.

[dvc94ch]

Seems like a hack to me, but at least I'm getting some bt's...

>>> bt
#0  riscv_rt::lang_items::panic_fmt () at /home/dvc/repos/riscv-rust/crates/riscv-rt/src/lang_items.rs:10
#1  0x20400560 in core::panicking::panic_fmt (fmt=..., file_line_col=<optimized out>) at /home/dvc/repos/riscv-rust/rust/src/libcore/panicking.rs:71
#2  0x20400500 in core::panicking::panic (expr_file_line_col=<optimized out>) at /home/dvc/repos/riscv-rust/rust/src/libcore/panicking.rs:51
#3  0x20400164 in test_nested::three () at examples/test_nested.rs:11
#4  0x20400184 in test_nested::two () at examples/test_nested.rs:16
#5  0x204001a4 in test_nested::one () at examples/test_nested.rs:22
#6  0x204001c4 in test_nested::main () at examples/test_nested.rs:27

From ee2706711fe739dc5244097ede68372a65d70692 Mon Sep 17 00:00:00 2001
From: David Craven <[email protected]>
Date: Fri, 25 Aug 2017 16:04:50 +0200
Subject: [PATCH] Emit DWARF CFI.

---
 lib/CodeGen/AsmPrinter/DwarfCFIException.cpp       |  4 +-
 lib/Target/RISCV/MCTargetDesc/RISCVMCAsmInfo.cpp   |  2 +
 .../RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp       | 10 ++++-
 lib/Target/RISCV/RISCVFrameLowering.cpp            | 27 +++++++++++++
 test/CodeGen/RISCV/debug-frame.ll                  | 47 ++++++++++++++++++++++
 5 files changed, 88 insertions(+), 2 deletions(-)
 create mode 100644 test/CodeGen/RISCV/debug-frame.ll

diff --git a/lib/CodeGen/AsmPrinter/DwarfCFIException.cpp b/lib/CodeGen/AsmPrinter/DwarfCFIException.cpp
index dd7f7931b06..ce02b17f04e 100644
--- a/lib/CodeGen/AsmPrinter/DwarfCFIException.cpp
+++ b/lib/CodeGen/AsmPrinter/DwarfCFIException.cpp
@@ -137,7 +137,9 @@ void DwarfCFIException::beginFragment(const MachineBasicBlock *MBB,
     return;
 
   if (!hasEmittedCFISections) {
-    if (Asm->needsOnlyDebugCFIMoves())
+    if (!Asm->needsOnlyDebugCFIMoves())
+      Asm->OutStreamer->EmitCFISections(true, true);
+    else
       Asm->OutStreamer->EmitCFISections(false, true);
     hasEmittedCFISections = true;
   }
diff --git a/lib/Target/RISCV/MCTargetDesc/RISCVMCAsmInfo.cpp b/lib/Target/RISCV/MCTargetDesc/RISCVMCAsmInfo.cpp
index d622911e92c..9f806dc1a49 100644
--- a/lib/Target/RISCV/MCTargetDesc/RISCVMCAsmInfo.cpp
+++ b/lib/Target/RISCV/MCTargetDesc/RISCVMCAsmInfo.cpp
@@ -22,4 +22,6 @@ RISCVMCAsmInfo::RISCVMCAsmInfo(const Triple &TT) {
   CommentString = "#";
   AlignmentIsInBytes = false;
   SupportsDebugInformation = true;
+  ExceptionsType = ExceptionHandling::DwarfCFI;
+  DwarfRegNumForCFI = true;
 }
diff --git a/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp b/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp
index 2b35eab577b..e4a9c624ae8 100644
--- a/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp
+++ b/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp
@@ -48,7 +48,15 @@ static MCRegisterInfo *createRISCVMCRegisterInfo(const Triple &TT) {
 
 static MCAsmInfo *createRISCVMCAsmInfo(const MCRegisterInfo &MRI,
                                        const Triple &TT) {
-  return new RISCVMCAsmInfo(TT);
+
+  MCAsmInfo *MAI = new RISCVMCAsmInfo(TT);
+
+  unsigned Reg = RISCV::X2_32;
+  MCCFIInstruction Inst =
+    MCCFIInstruction::createDefCfa(nullptr, MRI.getDwarfRegNum(Reg, true), 0);
+  MAI->addInitialFrameState(Inst);
+
+  return MAI;
 }
 
 static MCInstPrinter *createRISCVMCInstPrinter(const Triple &T,
diff --git a/lib/Target/RISCV/RISCVFrameLowering.cpp b/lib/Target/RISCV/RISCVFrameLowering.cpp
index bc7b0dda836..035e4352348 100644
--- a/lib/Target/RISCV/RISCVFrameLowering.cpp
+++ b/lib/Target/RISCV/RISCVFrameLowering.cpp
@@ -17,7 +17,9 @@
 #include "llvm/CodeGen/MachineFrameInfo.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
+#include "llvm/CodeGen/MachineModuleInfo.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
+#include "llvm/MC/MCDwarf.h"
 
 using namespace llvm;
 
@@ -146,6 +148,31 @@ void RISCVFrameLowering::emitPrologue(MachineFunction &MF,
   // Generate new FP
   adjustReg(MBB, MBBI, DL, FPReg, SPReg, StackSize - RVFI->getVarArgsSaveSize(),
             MachineInstr::FrameSetup);
+
+
+  // emit ".cfi_def_cfa_offset StackSize"
+  MachineModuleInfo &MMI = MF.getMMI();
+  const MCRegisterInfo *MRI = MMI.getContext().getRegisterInfo();
+  const RISCVInstrInfo *TII = STI.getInstrInfo();
+
+  MCCFIInstruction ICFI = MCCFIInstruction::createDefCfaOffset(nullptr, -StackSize);
+  unsigned CFIIndex = MF.addFrameInst(ICFI);
+  BuildMI(MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
+    .addCFIIndex(CFIIndex);
+
+  const std::vector<CalleeSavedInfo> &CSI = MFI.getCalleeSavedInfo();
+  if (!CSI.empty()) {
+    for (std::vector<CalleeSavedInfo>::const_iterator I = CSI.begin(),
+           E = CSI.end(); I != E; ++I) {
+      int64_t Offset = MFI.getObjectOffset(I->getFrameIdx());
+      unsigned Reg = I->getReg();
+      unsigned DReg = MRI->getDwarfRegNum(Reg, true);
+      MCCFIInstruction CFIOffset = MCCFIInstruction::createOffset(nullptr, DReg, Offset);
+      unsigned CFIIndex = MF.addFrameInst(CFIOffset);
+      BuildMI(MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
+        .addCFIIndex(CFIIndex);
+    }
+  }
 }
 
 void RISCVFrameLowering::emitEpilogue(MachineFunction &MF,
diff --git a/test/CodeGen/RISCV/debug-frame.ll b/test/CodeGen/RISCV/debug-frame.ll
new file mode 100644
index 00000000000..91ebb0221dd
--- /dev/null
+++ b/test/CodeGen/RISCV/debug-frame.ll
@@ -0,0 +1,47 @@
+; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s | FileCheck %s
+
+!llvm.dbg.cu = !{!1}
+!1 = distinct !DICompileUnit(language: DW_LANG_C99, file: !2,
+                             emissionKind: FullDebug)
+!2 = !DIFile(filename: "/dev/stdin",  directory: "/homeless-shelter")
+
+!llvm.module.flags = !{!3, !4}
+;; Dwarf version to output.
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+;; Debug info schema version.
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+
+define void @test_sections() nounwind {
+; CHECK: test_sections:
+; CHECK: .cfi_sections .eh_frame, .debug_frame
+; CHECK: .cfi_startproc
+; CHECK: .cfi_endproc
+entry:
+  ret void
+}
+
+declare void @extern();
+
+define void @test_nounwind(i32 %a) nounwind {
+; CHECK: test_nounwind:
+; CHECK: .cfi_startproc
+; CHECK: .cfi_def_cfa_offset 8
+; CHECK: .cfi_offset 1, -4
+; CHECK: .cfi_offset 8, -8
+; CHECK: .cfi_endproc
+entry:
+  call void @extern()
+  ret void
+}
+
+define void @test_unwind(i32 %a) {
+; CHECK: test_unwind:
+; CHECK: .cfi_startproc
+; CHECK: .cfi_def_cfa_offset 8
+; CHECK: .cfi_offset 1, -4
+; CHECK: .cfi_offset 8, -8
+; CHECK: .cfi_endproc
+entry:
+  call void @extern()
+  ret void
+}
\ No newline at end of file
-- 
2.11.1

[asb]

The emitPrologue changes are along the lines I would expect,thanks for sharing. Why do you need to patch DwarfCFIException?

[dvc94ch]

There's only three places from where EmitCFISections get's called. DwarfCFIException, ARMException and AsmParser. I didn't find a better way to force a .debug_frame section. Otherwise there's only a .eh_frame section.

[dvc94ch]

The emitPrologue changes are along the lines I would expect

I'd expect that too, but can't find the .cfi-restore directive [0] used in any backend and the emitCFIInstruction function doesn't think emitting restores is necessary, which is confusing :/

void AsmPrinter::emitCFIInstruction(const MCCFIInstruction &Inst) const {
  switch (Inst.getOperation()) {
  default:
    llvm_unreachable("Unexpected instruction");
  case MCCFIInstruction::OpDefCfaOffset:
    OutStreamer->EmitCFIDefCfaOffset(Inst.getOffset());
    break;
  case MCCFIInstruction::OpAdjustCfaOffset:
    OutStreamer->EmitCFIAdjustCfaOffset(Inst.getOffset());
    break;
  case MCCFIInstruction::OpDefCfa:
    OutStreamer->EmitCFIDefCfa(Inst.getRegister(), Inst.getOffset());
    break;
  case MCCFIInstruction::OpDefCfaRegister:
    OutStreamer->EmitCFIDefCfaRegister(Inst.getRegister());
    break;
  case MCCFIInstruction::OpOffset:
    OutStreamer->EmitCFIOffset(Inst.getRegister(), Inst.getOffset());
    break;
  case MCCFIInstruction::OpRegister:
    OutStreamer->EmitCFIRegister(Inst.getRegister(), Inst.getRegister2());
    break;
  case MCCFIInstruction::OpWindowSave:
    OutStreamer->EmitCFIWindowSave();
    break;
  case MCCFIInstruction::OpSameValue:
    OutStreamer->EmitCFISameValue(Inst.getRegister());
    break;
  case MCCFIInstruction::OpGnuArgsSize:
    OutStreamer->EmitCFIGnuArgsSize(Inst.getOffset());
    break;
  case MCCFIInstruction::OpEscape:
    OutStreamer->EmitCFIEscape(Inst.getValues());
    break;
  }
}

[0] https://sourceware.org/binutils/docs/as/CFI-directives.html#g_t_002ecfi_005frestore-register

[dvc94ch]

From looking at what llvm outputs on x86 and Mips, it indeed does not make use of .cfi_restore, but the riscv gcc port does.

The DWARF4 specification only talks about a .debug_frame and to be standards compliant it is my opinion that we should always emit a .debug_frame section (it seems riscv gcc does this), even if all other archs don't (they abuse .eh-frame).

Since C does not use exception handling I think that clang and rustc should mark all functions with @nounwind when emitting c stuff.

ExceptionsType = ExceptionHandling::DwarfCFI;

This should not be needed in my opinion for C or Rust panic=abort code, since debug information and exception handling are two separate features, even if .eh-frame often gets abused.
This would be needed for C++ or Rust panic=unwind code.

But someone who understands the issues and implications involved needs to comment on this, and it should be documented in an official place somewhere.

FYI: There is also a behavioral difference between .eh-frame and .debug-frame in gdb. If there isn't a .debug-frame it will trace all the way back to _start, else only to the language boundary main.

[dvc94ch]

Needed to add this to get CFI working (don't ask me why):

diff --git a/lib/CodeGen/AsmPrinter/DwarfCFIException.cpp b/lib/CodeGen/AsmPrinter/DwarfCFIException.cpp
index dd7f7931b06..ce02b17f04e 100644
--- a/lib/CodeGen/AsmPrinter/DwarfCFIException.cpp
+++ b/lib/CodeGen/AsmPrinter/DwarfCFIException.cpp
@@ -137,7 +137,9 @@ void DwarfCFIException::beginFragment(const MachineBasicBlock *MBB,
     return;
 
   if (!hasEmittedCFISections) {
-    if (Asm->needsOnlyDebugCFIMoves())
+    if (!Asm->needsOnlyDebugCFIMoves())
+      Asm->OutStreamer->EmitCFISections(true, true);
+    else
       Asm->OutStreamer->EmitCFISections(false, true);
     hasEmittedCFISections = true;
   }

All changes that are required to use rust are here (most of them obsolete now):
https://github.com/dvc94ch/riscv-rust-toolchain

Thank you!

[dvc94ch]

There also seems to be something else wrong - the stack view displays the wrong values for variables. Any variable I try to print is scrambled somehow.

[asb]

When did you last update? I just pushed to changes to frameindex lowering (implementing getFrameIndexReference). It's this patch: https://reviews.llvm.org/D39848

Depending on the version you're using, it's possible I either just fixed things or just broke them.

[dvc94ch]

Unfortunately it's not that simple to test since llvm master breaks things for me (maybe in a few days it will be "fixed"), and I don't have the time to look into this. I'll just live with the debugger not working 100% for now... (the llvm commit that worked for me is to old and requires backporting the patches which I'm to lazy to do right now =P)

rustc --print sysroot
LLVM ERROR: out of memory
[1]    32044 abort      rustc --print sysroot

[dvc94ch]

Works now sorry for the noise. The remaining CFI problem is probably unrelated to llvm.