Fix AArch64 AAPCS frame record chain

[logan]

Summary

After the commit r368987 (rG643adb55769e) was landed, the frame record (FP and LR register) may be placed in the middle of a stack frame if a function has both callee-saved general-purpose registers and floating point registers. This will break the stack unwinders that simply walk through the frame records (based on the guarantee from AAPCS64 "The Frame Pointer" section). This commit fixes the problem by adding the frame record offset.

[sdesmalen]

Thanks for creating a fix for this @logan!

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

2339

There is a condition here that is not yet tested. If the frame-record is saved that is both LR and FP, not just FP, so is this case needed?

[logan]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

2339

I think we don't have to check LR here because AAPCS64 guarantees FP and LR will be spilled to consecutive words. Besides, we only care about the address of the spilled FP.

[sdesmalen]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

2339

If LR is also spilled, shouldn't isPaired() be true? This is testing two layouts (LR, FP) and (FP, <something else>), where the latter is not a frame-record.

This makes me think that this code also needs a condition for hasFP(MF), because that guarantees the existence of a frame-record (as opposed to ordinary spills of FP/LR that don't necessarily constitute the framerecord)

[logan]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

2339

I am concerned about the case that FP is the second register in the pair (next one). Since the EmitMI function only gets a RegisterPairInfo. I cannot see the next RegisterPairInfo. Let me think about how to revise this tomorrow.

hasFP(MF) makes sense to me. I'll update the code in the next revision.

[logan]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

2339

Revised. Please take a look. Thanks.

[sdesmalen]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

1046

Having calculated the offset to the FrameRecord explicitly, can we now replace this with:

int FPOffset = AFI->getFrameRecordOffset();

?

llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h

129

Is this description correct? The current meaning of FrameRecordOffset seems to be the offset from SP _after_ allocating the callee-save area.

[logan]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

1046

Yes. Replaced in the latest Diff.

llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h

129

Thanks. Reworded.

[sdesmalen]

Functionally the patch is looking good, nearly there!

llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h

129

Thanks! Is it worth renaming the variable to something like OffsetToFrameRecordFromCalleeSaveBase to make the meaning of the variable easier to understand in the places it is used? (I couldn't think of something shorter :))

llvm/test/CodeGen/AArch64/framelayout-frame-record.ll

12 ↗

(On Diff #231828)

This has some implicit knowledge that d9 and d8 will be used to keep d0 and d1.
This is probably better tested with a MIR test like:

#RUN: llc -mtriple=aarch64-- -start-before prologepilog %s -o - | FileCheck %s
---
name: TestFrameRecordLocation
tracksRegLiveness: true
frameInfo:
  isFrameAddressTaken: true
body: |
  bb.0:
    $d8 = IMPLICIT_DEF
    $d9 = IMPLICIT_DEF
    RET_ReallyLR
...

which explicitly marks d8 and d9 as callee-saved. The isFrameAddressTaken: true will trigger the use of the frame-pointer, and thus storing of the frame-record. You only need to add a few check lines.

13 ↗

(On Diff #231828)

nit: instead of checking [[FP_OFFSET:16]] as a variable, you can just check 16 directly.

[logan]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

2114

Just noticed that Windows have different ordering.

llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h

129

The naming convention in the file seems to be {get,set,}XXXOffset, thus I renamed this to CalleeSaveBaseToFrameRecordOffset.

[sdesmalen]

Thanks @logan , LGTM!

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

2114

Yes, good spot!

[logan]

Run git clang-format and resolve rebase conflicts.

I am currently converting my SVN commit access to GitHub access. I'll land this as soon as possible.

[logan]

I reverted the CL because I encountered the following assertion:

assert((DestReg != AArch64::SP || Bytes % 16 == 0) &&
       "SP increment/decrement not 16-byte aligned");

I will upload a revision soon.

[logan]

Rebase to latest LLVM master branch

[sdesmalen]

In D70800#1982722, @logan wrote:

Rebase to latest LLVM master branch

Did you make any other changes other than the rebase?

[logan]

In D70800#1982889, @sdesmalen wrote:

In D70800#1982722, @logan wrote:

Rebase to latest LLVM master branch

Did you make any other changes other than the rebase?

Nope. I am still debugging the assertion failure. One of the emitFrameOffset in emitEpilogue get non-zero NumBytes that is not multiple of 16. But from the limited backtrace, I have some difficulty to spot the problem.

BTW, I cannot locally reproduce it with the same command. On my side, the command can run without problems.

[paulwalker-arm]

I've had to revert this patch because it caused runtime failures when building spec2k/eon with -march=armv8-a+sve -mllvm -aarch64-sve-vector-bits-min=256.

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

3445–3446 ↗

(On Diff #288386)

I don't believe this is a safe fix for the issue mentioned when the patch was previously reverted by https://reviews.llvm.org/rG04879086b44348cad600a0a1ccbe1f7776cc3cf9.

The stack always being 16-byte align is a requirement for the AAPCS.

[paulwalker-arm]

But doesn't that mean the assert is now protecting less than it was? Without the knowledge of FP's displacement, you cannot know if Bytes % 8 == 0 is safe or not. Is the knowledge of FP's displacement recorded anyway so the original intent of the assert is not compromised? From an SVE point of view, FP displacement will either need to be prevented or undone if it's used to access SVE stack slots. This is because SVE offsets are implicitly scaled and thus any byte based displacement cannot be encoded into its instructions.

It's my believe that at a minimum the assert should be tightened up to pre-D70800 levels before reapplying the patch.

Paul!!!

On 01/09/2020, 16:28, "Owen Anderson" <[e-mail]> wrote:

[paulwalker-arm]

Sure, but that means there need to be a way to detect when FP is displaced (hence my original question). If such data is available (and it needs to be for SVE code generation to function correctly) then I don't see why the assert cannot use it to assert Bytes is correctly aligned and displaced.

On 01/09/2020, 18:29, "Owen Anderson" <[e-mail]> wrote:

The assertion is protecting less than it was, because the original was overly strictly.  Please refer to the test case I mentioned, which provides an example where AAPCS-compliant code generation (frame record layout is part of AAPCS!) is impossible under the original assertion.  As such, I don’t see a path to returning the assertion to its original form.  Again, correct AAPCS-compliant code generation for this test is impossible with the assertion as it was.

I suspect you will find that if you use that test case as a starting point and add an SVE stack object to it, you will find that the SVE frame layout code needs to handle the case of an unaligned frame pointer.  That’s most likely the underlying cause of the failure you were seeing.

[paulwalker-arm]

Fair enough, thanks for the information Owen.

Clearly my preference would be to not fix one AArch64 bug by introducing another. You sound more knowledgeable in this area than myself so perhaps could create your suggested SVE test cases to ensure this isn't the case. That said, if you prefer to just reapply the patch as is, at least we've recorded the issue for others working on SVE.

Paul!!!

On 01/09/2020, 19:12, "Owen Anderson" <[e-mail]> wrote:

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

 }
 
 static bool needsWinCFI(const MachineFunction &MF) {
   const Function &F = MF.getFunction();
   return MF.getTarget().getMCAsmInfo()->usesWindowsCFI() &&
          F.needsUnwindTableEntry();
 }
 
-static bool isTargetDarwin(const MachineFunction &MF) {
-  return MF.getSubtarget<AArch64Subtarget>().isTargetDarwin();
-}
-
 static bool isTargetWindows(const MachineFunction &MF) {
   return MF.getSubtarget<AArch64Subtarget>().isTargetWindows();
 }
 
 // Convenience function to determine whether I is an SVE callee save.
 static bool IsSVECalleeSave(MachineBasicBlock::iterator I) {
   switch (I->getOpcode()) {
   default:
if (CombineSPBump)
       fixupCalleeSaveRestoreStackOffset(*MBBI, AFI->getLocalStackSize(),
                                         NeedsWinCFI, &HasWinCFI);
     ++MBBI;
   }
 
   // For funclets the FP belongs to the containing function.
   if (!IsFunclet && HasFP) {
     // Only set up FP if we actually need to.
-    int64_t FPOffset = isTargetDarwin(MF) ? (AFI->getCalleeSavedStackSize() - 16) : 0;
+    int64_t FPOffset = AFI->getCalleeSaveBaseToFrameRecordOffset();
 
     if (CombineSPBump)
       FPOffset += AFI->getLocalStackSize();
 
     // Issue    sub fp, sp, FPOffset or
     //          mov fp,sp          when FPOffset is zero.
     // Note: All stores of callee-saved registers are marked as "FrameSetup".
     // This code marks the instruction(s) that set the FP also.
if (isAsynchronousEHPersonality(Per)) {
       BuildMI(MBB, MBBI, DL, TII->get(TargetOpcode::COPY), AArch64::FP)
           .addReg(AArch64::X1)
           .setMIFlag(MachineInstr::FrameSetup);
       MBB.addLiveIn(AArch64::X1);
     }
   }
 
   if (needsFrameMoves) {
-    const DataLayout &TD = MF.getDataLayout();
-    const int StackGrowth = isTargetDarwin(MF)
-                                ? (2 * -TD.getPointerSize(0))
-                                : -AFI->getCalleeSavedStackSize();
-    Register FramePtr = RegInfo->getFrameRegister(MF);
     // An example of the prologue:
     //
     //     .globl __foo
     //     .align 2
     //  __foo:
     // Ltmp0:
     //     .cfi_startproc
     //     .cfi_personality 155, ___gxx_personality_v0
if (needsFrameMoves) {
     //  Ltmp3:
     //     .cfi_offset w29, -16
     //  Ltmp4:
     //     .cfi_offset w27, -24
     //  Ltmp5:
     //     .cfi_offset w28, -32
 
     if (HasFP) {
+      const int OffsetToFirstCalleeSaveFromFP =
+          AFI->getCalleeSaveBaseToFrameRecordOffset() -
+          AFI->getCalleeSavedStackSize();
+      Register FramePtr = RegInfo->getFrameRegister(MF);
+
       // Define the current CFA rule to use the provided FP.
       unsigned Reg = RegInfo->getDwarfRegNum(FramePtr, true);
       unsigned CFIIndex = MF.addFrameInst(MCCFIInstruction::createDefCfa(
-          nullptr, Reg, StackGrowth - FixedObject));
+          nullptr, Reg, OffsetToFirstCalleeSaveFromFP - FixedObject));
       BuildMI(MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
           .addCFIIndex(CFIIndex)
           .setMIFlags(MachineInstr::FrameSetup);
     } else {
       // Encode the stack size of the leaf function.
       unsigned CFIIndex = MF.addFrameInst(
           MCCFIInstruction::createDefCfaOffset(nullptr, -MFI.getStackSize()));
       BuildMI(MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
if (!hasFP(MF)) {
     NumBytes = 0;
   }
 
   // Restore the original stack pointer.
   // FIXME: Rather than doing the math here, we should instead just use
   // non-post-indexed loads for the restores if we aren't actually going to
   // be able to save any instructions.
   if (!IsFunclet && (MFI.hasVarSizedObjects() || AFI->isStackRealigned())) {
-    int64_t OffsetToFrameRecord =
-        isTargetDarwin(MF) ? (-(int64_t)AFI->getCalleeSavedStackSize() + 16) : 0;
     emitFrameOffset(MBB, LastPopI, DL, AArch64::SP, AArch64::FP,
-                    {OffsetToFrameRecord, MVT::i8},
+                    {-AFI->getCalleeSaveBaseToFrameRecordOffset(), MVT::i8},
                     TII, MachineInstr::FrameDestroy, false, NeedsWinCFI);
   } else if (NumBytes)
     emitFrameOffset(MBB, LastPopI, DL, AArch64::SP, AArch64::SP,
                     {NumBytes, MVT::i8}, TII, MachineInstr::FrameDestroy, false,
                     NeedsWinCFI);
 
   // This must be placed after the callee-save restore code because that code
   // assumes the SP is at the same location as it was after the callee-save save
int AArch64FrameLowering::getNonLocalFrameIndexReference(
   return getSEHFrameIndexOffset(MF, FI);
 }
 
 static StackOffset getFPOffset(const MachineFunction &MF, int64_t ObjectOffset) {
   const auto *AFI = MF.getInfo<AArch64FunctionInfo>();
   const auto &Subtarget = MF.getSubtarget<AArch64Subtarget>();
   bool IsWin64 =
       Subtarget.isCallingConvWin64(MF.getFunction().getCallingConv());
-
   unsigned FixedObject =
       getFixedObjectSize(MF, AFI, IsWin64, /*IsFunclet=*/false);
-  unsigned FPAdjust = isTargetDarwin(MF)
-                        ? 16 : AFI->getCalleeSavedStackSize(MF.getFrameInfo());
+  int64_t CalleeSaveSize = AFI->getCalleeSavedStackSize(MF.getFrameInfo());
+  int64_t FPAdjust =
+      CalleeSaveSize - AFI->getCalleeSaveBaseToFrameRecordOffset();
   return {ObjectOffset + FixedObject + FPAdjust, MVT::i8};
 }
 
 static StackOffset getStackOffset(const MachineFunction &MF, int64_t ObjectOffset) {
   const auto &MFI = MF.getFrameInfo();
   return {ObjectOffset + (int64_t)MFI.getStackSize(), MVT::i8};
 }
 
for (unsigned i = 0; i < Count; ++i) {
     int Offset = RPI.isScalable() ? ScalableByteOffset : ByteOffset;
     assert(Offset % Scale == 0);
     RPI.Offset = Offset / Scale;
 
     assert(((!RPI.isScalable() && RPI.Offset >= -64 && RPI.Offset <= 63) ||
             (RPI.isScalable() && RPI.Offset >= -256 && RPI.Offset <= 255)) &&
            "Offset out of bounds for LDP/STP immediate");
 
+    // Save the offset to frame record so that the FP register can point to the
+    // innermost frame record (spilled FP and LR registers).
+    if (NeedsFrameRecord && ((!IsWindows && RPI.Reg1 == AArch64::LR &&
+                              RPI.Reg2 == AArch64::FP) ||
+                             (IsWindows && RPI.Reg1 == AArch64::FP &&
+                              RPI.Reg2 == AArch64::LR)))
+      AFI->setCalleeSaveBaseToFrameRecordOffset(Offset);
+
     RegPairs.push_back(RPI);
     if (RPI.isPaired())
       ++i;
   }
 }
 
 bool AArch64FrameLowering::spillCalleeSavedRegisters(
     MachineBasicBlock &MBB, MachineBasicBlock::iterator MI,
auto EmitMI = [&](const RegPairInfo &RPI) {
     unsigned FrameIdxReg1 = RPI.FrameIdx;
     unsigned FrameIdxReg2 = RPI.FrameIdx + 1;
     if (NeedsWinCFI && RPI.isPaired()) {
       std::swap(Reg1, Reg2);
       std::swap(FrameIdxReg1, FrameIdxReg2);
     }
     MachineInstrBuilder MIB = BuildMI(MBB, MI, DL, TII.get(LdrOpc));
     if (RPI.isPaired()) {
       MIB.addReg(Reg2, getDefRegState(true));
       MIB.addMemOperand(MF.getMachineMemOperand(
           MachinePointerInfo::getFixedStack(MF, FrameIdxReg2),
           MachineMemOperand::MOLoad, Size, Alignment));
     }
     MIB.addReg(Reg1, getDefRegState(true))
         .addReg(AArch64::SP)
         .addImm(RPI.Offset) // [sp, #offset*scale]
                             // where factor*scale is implicit

llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h

class AArch64FunctionInfo final : public MachineFunctionInfo {
   /// that must be forwarded to every musttail call.
   SmallVector<ForwardedRegister, 1> ForwardedMustTailRegParms;
 
   // Offset from SP-at-entry to the tagged base pointer.
   // Tagged base pointer is set up to point to the first (lowest address) tagged
   // stack slot.
   unsigned TaggedBasePointerOffset = 0;
 
+  // Offset from SP-after-callee-saved-spills (i.e. SP-at-entry minus
+  // CalleeSavedStackSize) to the address of the frame record.
+  int CalleeSaveBaseToFrameRecordOffset = 0;
+
 public:
   AArch64FunctionInfo() = default;
 
   explicit AArch64FunctionInfo(MachineFunction &MF) {
     (void)MF;
 
     // If we already know that the function doesn't have a redzone, set
     // HasRedZone here.
#endif
 
   unsigned getTaggedBasePointerOffset() const {
     return TaggedBasePointerOffset;
   }
   void setTaggedBasePointerOffset(unsigned Offset) {
     TaggedBasePointerOffset = Offset;
   }
 
+  int getCalleeSaveBaseToFrameRecordOffset() const {
+    return CalleeSaveBaseToFrameRecordOffset;
+  }
+  void setCalleeSaveBaseToFrameRecordOffset(int Offset) {
+    CalleeSaveBaseToFrameRecordOffset = Offset;
+  }
+
 private:
   // Hold the lists of LOHs.
   MILOHContainer LOHContainerSet;
   SetOfInstructions LOHRelated;
 
   DenseMap<int, std::pair<unsigned, MCSymbol *>> JumpTableEntryInfo;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_LIB_TARGET_AARCH64_AARCH64MACHINEFUNCTIONINFO_H

llvm/test/CodeGen/AArch64/framelayout-frame-record.mir

+# RUN: llc -mtriple=aarch64-linux-gnu -start-before prologepilog %s -o - | FileCheck %s
+
+---
+name: TestFrameRecordLocation
+tracksRegLiveness: true
+frameInfo:
+  isFrameAddressTaken: true
+body: |
+  bb.0:
+    $d8 = IMPLICIT_DEF
+    $d9 = IMPLICIT_DEF
+    $x19 = IMPLICIT_DEF
+    RET_ReallyLR
+
+# CHECK-LABEL: TestFrameRecordLocation
+
+# CHECK: stp d9, d8, [sp, #-48]!
+# CHECK: stp x29, x30, [sp, #16]
+# CHECK: str x19, [sp, #32]
+
+# CHECK: add x29, sp, #16
+
+# CHECK: .cfi_def_cfa w29, 32
+# CHECK: .cfi_offset w19, -16
+# CHECK: .cfi_offset w30, -24
+# CHECK: .cfi_offset w29, -32
+# CHECK: .cfi_offset b8, -40
+# CHECK: .cfi_offset b9, -48
+...