[COFF, ARM64] Implement Intrinsic.sponentry for AArch64

[yinma]

Summary

This patch adds Intrinsic.sponentry. This intrinsic is required to correctly support setjmp for AArch64 Windows platform.

[mstorsjo]

How is the corresponding case handled for x86_64, where you afaik also need the original stack pointer?

[mgrang]

lib/Target/AArch64/AArch64FastISel.cpp

3466

Period at end of comments.

[yinma]

I debugged MS ARM64 setjmp/longjmp functions and ARM64 EH runtime. If you looks like assembly output from CL for ARM64, it is using SP instead of FP.

[mstorsjo]

In D53673#1275103, @yinma wrote:

I debugged MS ARM64 setjmp/longjmp functions and ARM64 EH runtime. If you looks like assembly output from CL for ARM64, it is using SP instead of FP.

Hmm, in my test it seems like it doesn't even use the SP as it was on on entry, it uses SP on entry + 16:

$ cat frame.c 
#include <setjmp.h>
void other(char* buf);
jmp_buf jb;
void func(int a) {
    char buf[100];
    setjmp(jb);
    other(buf);
}
$ cl -c frame.c -O2
Microsoft (R) C/C++ Optimizing Compiler Version 19.15.26726 for ARM64
Copyright (C) Microsoft Corporation.  All rights reserved.

frame.c
$ dumpbin -nologo -disasm frame.obj

Dump of file frame.obj

File Type: COFF OBJECT

func:
  0000000000000000: F81F0FFE  str         lr,[sp,#-0x10]!
  0000000000000004: 94000000  bl          __security_push_cookie
  0000000000000008: D10183FF  sub         sp,sp,#0x60
  000000000000000C: 90000008  adrp        x8,jb
  0000000000000010: 91000100  add         x0,x8,jb
  0000000000000014: 910203E1  add         x1,sp,#0x80
  0000000000000018: 94000000  bl          _setjmpex
  000000000000001C: 910003E0  mov         x0,sp
  0000000000000020: 94000000  bl          other
  0000000000000024: 910183FF  add         sp,sp,#0x60
  0000000000000028: 94000000  bl          __security_pop_cookie
  000000000000002C: F84107FE  ldr         lr,[sp],#0x10
  0000000000000030: D65F03C0  ret

In this case, we first subtract sp by 0x10 and later by 0x60, but in the parameter to _setjmpex we pass sp + 0x80.

test/CodeGen/AArch64/sponentry.ll

1

This test should test both with fastisel and dagisel, and should test cases both with and without fixed objects.

[yinma]

Because __security_push_cookie has extra 0x10 subtraction on SP, if you removes buf[100], you will see no security_push_cookie is used. it will be sp on entry.

__security_push_cookie:

000000014000D680: D10043FF  sub         sp,sp,#0x10
000000014000D684: F0000471  adrp        xip1,__scrt_native_dllmain_reason
000000014000D688: 91002231  add         xip1,xip1,#8
000000014000D68C: F9400231  ldr         xip1,[xip1]
000000014000D690: CB3163F1  sub         xip1,sp,xip1
000000014000D694: F90007F1  str         xip1,[sp,#8]
000000014000D698: D65F03C0  ret

__security_pop_cookie:

000000014000D69C: F0000471  adrp        xip1,__scrt_native_dllmain_reason
000000014000D6A0: 91002231  add         xip1,xip1,#8
000000014000D6A4: F94007F0  ldr         xip0,[sp,#8]
000000014000D6A8: F9400231  ldr         xip1,[xip1]
000000014000D6AC: CB3063F0  sub         xip0,sp,xip0
000000014000D6B0: EB11021F  cmp         xip0,xip1
000000014000D6B4: 54000061  bne         000000014000D6C0
000000014000D6B8: 910043FF  add         sp,sp,#0x10
000000014000D6BC: D65F03C0  ret
000000014000D6C0: AA1003E0  mov         x0,xip0

[mstorsjo]

In D53673#1276123, @yinma wrote:

Because __security_push_cookie has extra 0x10 subtraction on SP, if you removes buf[100], you will see no security_push_cookie is used. it will be sp on entry.

Ah, I see. Ok, then I guess this aspect makes sense.

[yinma]

Please review again

[mstorsjo]

Except for the concern about the test, this looks sensible to me. As this adds a new intrinsic in the public IR, I'd prefer if someone else with a bit more experience with these things to approve it as well though - @efriedma?

test/CodeGen/AArch64/sponentry.ll

23

Are you sure this actually triggers the fixed object codepath? (This looks a lot like the test example I posted earlier, and that wasn't at least designed to trigger fixed objects.) I think at least a function with varargs has fixed objects.

[mstorsjo]

test/CodeGen/AArch64/sponentry.ll

23

Indeed, I tested this, and both functions show numFixed = 0. So is the other codepath actually tested with some other code, or still untested?

[efriedma]

All new target-independent intrinsics need to be documented in LangRef.

Thinking about it a bit, this seems like a reasonable intrinsic to support across all (or at least most) targets; the meaning is well-defined, and it can't be computed from any of the other frame-related intrinsics, so I think it makes sense to support cross-platform.

That said, maybe instead of adding a new intrinsic, we should rename llvm.addressofreturnaddress? It's the same thing on x86, and addressofreturnaddress doesn't really make sense on other targets anyway.

[rnk]

lib/Target/AArch64/AArch64ISelLowering.cpp

5167–5171

What invariant are you relying on for fixed objects here? This seems like it could be fragile. Stack protectors and exception handling schemes often introduce new fixed objects. You might want to scan for the first or last parameter or CSR or whatever it is that you're really trying to be relative to.

[yinma]

add var_arg test case

[efriedma]

That said, maybe instead of adding a new intrinsic, we should rename llvm.addressofreturnaddress? It's the same thing on x86, and addressofreturnaddress doesn't really make sense on other targets anyway.

Nevermind, ignore this; apparently we actually need a separate llvm.addressofreturnaddress() to return the address of the return address on non-x86 targets, to implement the MSVC _AddressOfReturnAddress().

[efriedma]

lib/Target/AArch64/AArch64ISelLowering.cpp

5167–5171

You can also get fixed objects by just having a function with too many arguments to fit in registers. (Please add a testcase.)

There isn't really any existing fixed object with the right offset, in general. But I think you can unconditionally compute the correct offset with something like DAG.getFrameIndex(MFI.CreateFixedObject(1, 0, true), VT). Or I guess you could use a pseudo-instruction, and expand it after frame lowering, but that doesn't seem obviously better.

[efriedma]

lib/Target/AArch64/AArch64ISelLowering.cpp

5165

This might become unnecessary. But if it doesn't, please use setFrameAddressIsTaken to force a frame pointer to be emitted, instead of checking whether frame pointer elimination is enabled.

5170

Do you need the "if" statement here? CreateFixedObject should work whether or not there are other fixed objects, I think.

[efriedma]

LGTM

include/llvm/CodeGen/ISDOpcodes.h

enum NodeType {
     /// The address of the GOT
     GLOBAL_OFFSET_TABLE,
 
     /// FRAMEADDR, RETURNADDR - These nodes represent llvm.frameaddress and
     /// llvm.returnaddress on the DAG.  These nodes take one operand, the index
     /// of the frame or return address to return.  An index of zero corresponds
     /// to the current function's frame or return address, an index of one to
     /// the parent's frame or return address, and so on.
-    FRAMEADDR, RETURNADDR, ADDROFRETURNADDR,
+    FRAMEADDR, RETURNADDR, ADDROFRETURNADDR, SPONENTRY,
 
     /// LOCAL_RECOVER - Represents the llvm.localrecover intrinsic.
     /// Materializes the offset from the local object pointer of another
     /// function to a particular local object passed to llvm.localescape. The
     /// operand is the MCSymbol label used to represent this offset, since
     /// typically the offset is not known until after code generation of the
     /// parent.
     LOCAL_RECOVER,

include/llvm/IR/Intrinsics.td

def int_gcwrite : Intrinsic<[],
                             [llvm_ptr_ty, llvm_ptr_ty, llvm_ptrptr_ty],
                             [IntrArgMemOnly, NoCapture<1>, NoCapture<2>]>;
 
 //===--------------------- Code Generator Intrinsics ----------------------===//
 //
 def int_returnaddress : Intrinsic<[llvm_ptr_ty], [llvm_i32_ty], [IntrNoMem]>;
 def int_addressofreturnaddress : Intrinsic<[llvm_ptr_ty], [], [IntrNoMem]>;
 def int_frameaddress  : Intrinsic<[llvm_ptr_ty], [llvm_i32_ty], [IntrNoMem]>;
+def int_sponentry  : Intrinsic<[llvm_ptr_ty], [], [IntrNoMem]>;
 def int_read_register  : Intrinsic<[llvm_anyint_ty], [llvm_metadata_ty],
                                    [IntrReadMem], "llvm.read_register">;
 def int_write_register : Intrinsic<[], [llvm_metadata_ty, llvm_anyint_ty],
                                    [], "llvm.write_register">;
 
 // Gets the address of the local variable area. This is typically a copy of the
 // stack, frame, or base pointer depending on the type of prologue.
 def int_localaddress : Intrinsic<[llvm_ptr_ty], [], [IntrNoMem]>;

lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

case ISD::EH_SJLJ_SETUP_DISPATCH:
     if (Action == TargetLowering::Legal)
       Action = TargetLowering::Expand;
     break;
   case ISD::INIT_TRAMPOLINE:
   case ISD::ADJUST_TRAMPOLINE:
   case ISD::FRAMEADDR:
   case ISD::RETURNADDR:
   case ISD::ADDROFRETURNADDR:
+  case ISD::SPONENTRY:
     // These operations lie about being legal: when they claim to be legal,
     // they should actually be custom-lowered.
     Action = TLI.getOperationAction(Node->getOpcode(), Node->getValueType(0));
     if (Action == TargetLowering::Legal)
       Action = TargetLowering::Custom;
     break;
   case ISD::READCYCLECOUNTER:
     // READCYCLECOUNTER returns an i64, even if type legalization might have

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

case Intrinsic::returnaddress:
     setValue(&I, DAG.getNode(ISD::RETURNADDR, sdl,
                              TLI.getPointerTy(DAG.getDataLayout()),
                              getValue(I.getArgOperand(0))));
     return nullptr;
   case Intrinsic::addressofreturnaddress:
     setValue(&I, DAG.getNode(ISD::ADDROFRETURNADDR, sdl,
                              TLI.getPointerTy(DAG.getDataLayout())));
     return nullptr;
+  case Intrinsic::sponentry:
+    setValue(&I, DAG.getNode(ISD::SPONENTRY, sdl,
+                             TLI.getPointerTy(DAG.getDataLayout())));
+    return nullptr;
   case Intrinsic::frameaddress:
     setValue(&I, DAG.getNode(ISD::FRAMEADDR, sdl,
                              TLI.getPointerTy(DAG.getDataLayout()),
                              getValue(I.getArgOperand(0))));
     return nullptr;
   case Intrinsic::read_register: {
     Value *Reg = I.getArgOperand(0);
     SDValue Chain = getRoot();

lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp

#endif
   case ISD::GlobalAddress:              return "GlobalAddress";
   case ISD::GlobalTLSAddress:           return "GlobalTLSAddress";
   case ISD::FrameIndex:                 return "FrameIndex";
   case ISD::JumpTable:                  return "JumpTable";
   case ISD::GLOBAL_OFFSET_TABLE:        return "GLOBAL_OFFSET_TABLE";
   case ISD::RETURNADDR:                 return "RETURNADDR";
   case ISD::ADDROFRETURNADDR:           return "ADDROFRETURNADDR";
   case ISD::FRAMEADDR:                  return "FRAMEADDR";
+  case ISD::SPONENTRY:                  return "SPONENTRY";
   case ISD::LOCAL_RECOVER:              return "LOCAL_RECOVER";
   case ISD::READ_REGISTER:              return "READ_REGISTER";
   case ISD::WRITE_REGISTER:             return "WRITE_REGISTER";
   case ISD::FRAME_TO_ARGS_OFFSET:       return "FRAME_TO_ARGS_OFFSET";
   case ISD::EH_DWARF_CFA:               return "EH_DWARF_CFA";
   case ISD::EH_RETURN:                  return "EH_RETURN";
   case ISD::EH_SJLJ_SETJMP:             return "EH_SJLJ_SETJMP";
   case ISD::EH_SJLJ_LONGJMP:            return "EH_SJLJ_LONGJMP";

lib/Target/AArch64/AArch64FastISel.cpp

while (Depth--) {
                                 SrcReg, /*IsKill=*/true, 0);
       assert(DestReg && "Unexpected LDR instruction emission failure.");
       SrcReg = DestReg;
     }
 
     updateValueMap(II, SrcReg);
     return true;
   }
+  case Intrinsic::sponentry: {
+    MachineFrameInfo &MFI = FuncInfo.MF->getFrameInfo();
+    MFI.setFrameAddressIsTaken(true);
+
+    // Make sure the computation is correct.
+    if (!Subtarget->getFrameLowering()->noFramePointerElim(*FuncInfo.MF))
+      report_fatal_error("Intrinsic::sponentry requires frame pointer.");
+
+    // FP + 16 without Fixed Object.
+    // Fixed Object + 16
+    unsigned numFixed = MFI.getNumFixedObjects();
+    MVT VT = TLI.getPointerTy(DL);
+    if (numFixed) {
+      // First fixed objected
+      unsigned ResultReg = createResultReg(TLI.getRegClassFor(VT));
+      BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
+              TII.get(AArch64::ADDXri), ResultReg)
+              .addFrameIndex(-numFixed)
+              .addImm(0)
+              .addImm(0);
+      updateValueMap(II, ResultReg);
+    } else {
+      // FP + 16
+      const AArch64RegisterInfo *RegInfo = Subtarget->getRegisterInfo();
+      unsigned FramePtr = RegInfo->getFrameRegister(*(FuncInfo.MF));
+      unsigned SrcReg = MRI.createVirtualRegister(&AArch64::GPR64RegClass);
+      BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
+              TII.get(TargetOpcode::COPY), SrcReg).addReg(FramePtr);
+
+      unsigned AddReg = emitAdd_ri_(VT, SrcReg, /*IsKill=*/false, 16);
+      SrcReg = AddReg;
+
+      updateValueMap(II, SrcReg);
+    }
+    return true;
+  }
   case Intrinsic::memcpy:
   case Intrinsic::memmove: {
     const auto *MTI = cast<MemTransferInst>(II);
     // Don't handle volatile.
     if (MTI->isVolatile())
       return false;
 
     // Disable inlining for memmove before calls to ComputeAddress.  Otherwise,

lib/Target/AArch64/AArch64ISelLowering.h

private:
   SDValue LowerBlockAddress(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerAAPCS_VASTART(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerDarwin_VASTART(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerWin64_VASTART(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerVASTART(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerVACOPY(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerVAARG(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerFRAMEADDR(SDValue Op, SelectionDAG &DAG) const;
+  SDValue LowerSPONENTRY(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerRETURNADDR(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerFLT_ROUNDS_(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerINSERT_VECTOR_ELT(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerEXTRACT_VECTOR_ELT(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerSCALAR_TO_VECTOR(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerBUILD_VECTOR(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerVECTOR_SHUFFLE(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerEXTRACT_SUBVECTOR(SDValue Op, SelectionDAG &DAG) const;

lib/Target/AArch64/AArch64ISelLowering.cpp

SDValue AArch64TargetLowering::LowerOperation(SDValue Op,
   case ISD::FDIV:
     return LowerF128Call(Op, DAG, RTLIB::DIV_F128);
   case ISD::FP_ROUND:
     return LowerFP_ROUND(Op, DAG);
   case ISD::FP_EXTEND:
     return LowerFP_EXTEND(Op, DAG);
   case ISD::FRAMEADDR:
     return LowerFRAMEADDR(Op, DAG);
+  case ISD::SPONENTRY:
+    return LowerSPONENTRY(Op, DAG);
   case ISD::RETURNADDR:
     return LowerRETURNADDR(Op, DAG);
   case ISD::INSERT_VECTOR_ELT:
     return LowerINSERT_VECTOR_ELT(Op, DAG);
   case ISD::EXTRACT_VECTOR_ELT:
     return LowerEXTRACT_VECTOR_ELT(Op, DAG);
   case ISD::BUILD_VECTOR:
     return LowerBUILD_VECTOR(Op, DAG);
SDValue AArch64TargetLowering::LowerFRAMEADDR(SDValue Op,
   SDValue FrameAddr =
       DAG.getCopyFromReg(DAG.getEntryNode(), DL, AArch64::FP, VT);
   while (Depth--)
     FrameAddr = DAG.getLoad(VT, DL, DAG.getEntryNode(), FrameAddr,
                             MachinePointerInfo());
   return FrameAddr;
 }
 
+SDValue AArch64TargetLowering::LowerSPONENTRY(SDValue Op,
+                                              SelectionDAG &DAG) const {
+  MachineFrameInfo &MFI = DAG.getMachineFunction().getFrameInfo();
+  MFI.setFrameAddressIsTaken(true);
+
+  // Make sure the computation is correct.
+  if (!Subtarget->getFrameLowering()->noFramePointerElim(
+                                                      DAG.getMachineFunction()))
+      report_fatal_error("Intrinsic::sponentry requires frame pointer.");
+
+  unsigned numFixed = MFI.getNumFixedObjects();
+  EVT VT = getPointerTy(DAG.getDataLayout());
+  SDLoc DL(Op);
+  if (numFixed) {
+    return DAG.getFrameIndex(-numFixed, VT);
+  } else {
+    SDValue Offset = DAG.getConstant(16, DL, MVT::i64);
+    SDValue FrameAddr =
+        DAG.getCopyFromReg(DAG.getEntryNode(), DL, AArch64::FP, VT);
+    return DAG.getNode(ISD::ADD, DL, VT, FrameAddr, Offset);
+  }
+}
+
 // FIXME? Maybe this could be a TableGen attribute on some registers and
 // this table could be generated automatically from RegInfo.
 unsigned AArch64TargetLowering::getRegisterByName(const char* RegName, EVT VT,
                                                   SelectionDAG &DAG) const {
   unsigned Reg = StringSwitch<unsigned>(RegName)
                        .Case("sp", AArch64::SP)
                        .Case("x1", AArch64::X1)
                        .Case("w1", AArch64::W1)

test/CodeGen/AArch64/sponentry.ll

+; RUN: llc -mtriple=aarch64-windows-msvc -disable-fp-elim %s -o - | FileCheck %s
+; RUN: not llc -mtriple=aarch64-windows-msvc %s -o - 2>&1 | FileCheck %s --check-prefix=NOTWORK
+
+define dso_local void @bar([24 x i64]*) {
+  %2 = alloca [24 x i64]*, align 8
+  %3 = alloca i32, align 4
+  store [24 x i64]* %0, [24 x i64]** %2, align 8
+  %4 = call i8* @llvm.sponentry()
+  %5 = load [24 x i64]*, [24 x i64]** %2, align 8
+  %6 = getelementptr inbounds [24 x i64], [24 x i64]* %5, i32 0, i32 0
+  %7 = bitcast i64* %6 to i8*
+  %8 = call i32 @_setjmpex(i8* %7, i8* %4) #2
+  store i32 %8, i32* %3, align 4
+  ret void
+}
+
+; Function Attrs: nounwind readnone
+declare i8* @llvm.sponentry()
+
+; Function Attrs: returns_twice
+declare dso_local i32 @_setjmpex(i8*, i8*)
+
+; CHECK: add     x29, sp, #16
+; CHECK: add     x1, x29, #16
+; CEHCK: bl      _setjmpex
+
+; NOTWORK: Intrinsic::sponentry requires frame pointer