[ARM] Force LR spill in thumb2 noreturn functions

[bruno]

Summary

In ARM mode we do an opt in noreturn functions that uses branch
instead of calls and explicitly save the pc (r151645):

  mov lr, pc
  b _bar

This isn't possible in thumb2 mode because the mode bit doesnt get set
(r167657). However, we currently emit a branch and link but do not save
the LR, which leads to bad backtraces coming from noreturn functions.

Example:

define void @a() noreturn nounwind ssp {
  tail call void @b()
  unreachable
}
define void @b() noreturn nounwind ssp {
  tail call void @abort() noreturn nounwind
  unreachable
}
declare void @abort() noreturn

Currently generates:

_a:
        bl      _b
_b:
        push    {r7, lr}
        mov     r7, sp
        blx     _abort

Leading to bad backtraces. This patch generates instead:

_a:
        push    {r7, lr}
        mov     r7, sp
        bl      _b
_b:
        push    {r7, lr}
        mov     r7, sp
        blx     _abort

[qcolombet]

Hi Bruno,

I guess that it is general goodness to have the right stack trace when you debug something, but I thought noreturn + nounwind allow the compiler to trash the information for optimization purposes?

I am not saying that your patch is wrong, I am pointing out that it seems we regress some pattern to improve the debug experience, and I am not sure this is something we should do.

My 2c.

Cheers,
-Quentin

[bruno]

In D11243#205972, @qcolombet wrote:

Hi Bruno,

I guess that it is general goodness to have the right stack trace when you debug something, but I thought noreturn + nounwind allow the compiler to trash the information for optimization purposes?

Yep, thanks for pointing that out, I completely missed the unwind meaning here!

I am not saying that your patch is wrong, I am pointing out that it seems we regress some pattern to improve the debug experience, and I am not sure this is something we should do.

Seems reasonable! One can always use "-funwind-tables" to override the behavior and LR will be there.
Thanks,

[rengolin]

Isn't that the point of nounwind to not emit unwind information?

If you omit the "nounwind" from the function declaration you get your stack save.

lib/Target/ARM/ARMFrameLowering.cpp

const ARMBaseRegisterInfo *RegInfo = static_cast<const ARMBaseRegisterInfo *>(
       MF.getSubtarget().getRegisterInfo());
   const ARMBaseInstrInfo &TII =
       *static_cast<const ARMBaseInstrInfo *>(MF.getSubtarget().getInstrInfo());
   ARMFunctionInfo *AFI = MF.getInfo<ARMFunctionInfo>();
   MachineFrameInfo *MFI = MF.getFrameInfo();
   MachineRegisterInfo &MRI = MF.getRegInfo();
   unsigned FramePtr = RegInfo->getFrameRegister(MF);
 
+  if (AFI->isThumb2Function()) {
     // Spill R4 if Thumb2 function requires stack realignment - it will be used as
     // scratch register. Also spill R4 if Thumb2 function has varsized objects,
     // since it's not always possible to restore sp from fp in a single
     // instruction.
     // FIXME: It will be better just to find spare register here.
-  if (AFI->isThumb2Function() &&
-      (MFI->hasVarSizedObjects() || RegInfo->needsStackRealignment(MF)))
+    if (MFI->hasVarSizedObjects() || RegInfo->needsStackRealignment(MF))
       SavedRegs.set(ARM::R4);
 
+    // If the current function is noreturn and has calls it must preserve
+    // ARM::LR in order to guarantee sane backtraces.
+    if (AFI->shouldForceLRSpill())
+      SavedRegs.set(ARM::LR);
+  }
+
   if (AFI->isThumb1OnlyFunction()) {
     // Spill LR if Thumb1 function uses variable length argument lists.
     if (AFI->getArgRegsSaveSize() > 0)
       SavedRegs.set(ARM::LR);
 
     // Spill R4 if Thumb1 epilogue has to restore SP from FP. We don't know
     // for sure what the stack size will be, but for this, an estimate is good
     // enough. If there anything changes it, it'll be a spill, which implies

lib/Target/ARM/ARMISelLowering.cpp

ARMTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
   // FIXME: handle tail calls differently.
   unsigned CallOpc;
   bool HasMinSizeAttr = MF.getFunction()->hasFnAttribute(Attribute::MinSize);
   if (Subtarget->isThumb()) {
     if ((!isDirect || isARMFunc) && !Subtarget->hasV5TOps())
       CallOpc = ARMISD::CALL_NOLINK;
     else
       CallOpc = isARMFunc ? ARMISD::CALL : ARMISD::tCALL;
+    // If the current function is noreturn and has direct calls it must preserve
+    // ARM::LR in order to guarantee sane backtraces.
+    if (!isARMFunc && doesNotRet && isDirect)
+      AFI->setForceLRSpill(true);
   } else {
     if (!isDirect && !Subtarget->hasV5TOps())
       CallOpc = ARMISD::CALL_NOLINK;
     else if (doesNotRet && isDirect && Subtarget->hasRAS() &&
                // Emit regular call when code size is the priority
                !HasMinSizeAttr)
       // "mov lr, pc; b _foo" to avoid confusing the RSP
       CallOpc = ARMISD::CALL_NOLINK;

lib/Target/ARM/ARMMachineFunctionInfo.h

class ARMFunctionInfo : public MachineFunctionInfo {
   /// HasStackFrame - True if this function has a stack frame. Set by
   /// processFunctionBeforeCalleeSavedScan().
   bool HasStackFrame;
 
   /// RestoreSPFromFP - True if epilogue should restore SP from FP. Set by
   /// emitPrologue.
   bool RestoreSPFromFP;
 
+  /// ForceLRSpill - True if the LR register needs to be spilled. This is
+  /// necessary to force LR spill in noreturn function and is used in
+  /// processFunctionBeforeCalleeSavedScan().
+  bool ForceLRSpill;
+
   /// LRSpilledForFarJump - True if the LR register has been for spilled to
   /// enable far jump.
   bool LRSpilledForFarJump;
 
   /// FramePtrSpillOffset - If HasStackFrame, this records the frame pointer
   /// spill stack offset.
   unsigned FramePtrSpillOffset;
 
class ARMFunctionInfo : public MachineFunctionInfo {
   /// coalesced weights.
   DenseMap<const MachineBasicBlock*, unsigned> CoalescedWeights;
 
 public:
   ARMFunctionInfo() :
     isThumb(false),
     hasThumb2(false),
     ArgRegsSaveSize(0), ReturnRegsCount(0), HasStackFrame(false),
-    RestoreSPFromFP(false),
+    RestoreSPFromFP(false), ForceLRSpill(false),
     LRSpilledForFarJump(false),
     FramePtrSpillOffset(0), GPRCS1Offset(0), GPRCS2Offset(0), DPRCSOffset(0),
     GPRCS1Size(0), GPRCS2Size(0), DPRCSAlignGapSize(0), DPRCSSize(0),
     NumAlignedDPRCS2Regs(0), PICLabelUId(0),
     VarArgsFrameIndex(0), HasITBlocks(false), GlobalBaseReg(0) {}
 
   explicit ARMFunctionInfo(MachineFunction &MF);
 
public:
   void setReturnRegsCount(unsigned s) { ReturnRegsCount = s; }
 
   bool hasStackFrame() const { return HasStackFrame; }
   void setHasStackFrame(bool s) { HasStackFrame = s; }
 
   bool shouldRestoreSPFromFP() const { return RestoreSPFromFP; }
   void setShouldRestoreSPFromFP(bool s) { RestoreSPFromFP = s; }
 
+  bool shouldForceLRSpill() const { return ForceLRSpill; }
+  void setForceLRSpill(bool s) { ForceLRSpill = s; }
+
   bool isLRSpilledForFarJump() const { return LRSpilledForFarJump; }
   void setLRIsSpilledForFarJump(bool s) { LRSpilledForFarJump = s; }
 
   unsigned getFramePtrSpillOffset() const { return FramePtrSpillOffset; }
   void setFramePtrSpillOffset(unsigned o) { FramePtrSpillOffset = o; }
 
   unsigned getNumAlignedDPRCS2Regs() const { return NumAlignedDPRCS2Regs; }
   void setNumAlignedDPRCS2Regs(unsigned n) { NumAlignedDPRCS2Regs = n; }

test/CodeGen/Thumb2/call-noret-direct.ll

+;RUN: llc < %s -mtriple=thumbv7-apple-ios | FileCheck %s
+
+define void @a() noreturn nounwind ssp {
+; CHECK-LABEL: _a:
+; CHECK: push {r7, lr}
+; CHECK-NEXT: mov r7, sp
+; CHECK-NEXT: bl _b
+  tail call void @b()
+  unreachable
+}
+
+define void @b() noreturn nounwind ssp {
+; CHECK-LABEL: _b:
+; CHECK: push {r7, lr}
+; CHECK-NEXT: mov r7, sp
+; CHECK-NEXT: blx _abort
+  tail call void @abort() noreturn nounwind
+  unreachable
+}
+
+declare void @abort() noreturn