[ARM] implement LOAD_STACK_GUARD for remaining targets

[ardb]

Summary

Currently, LOAD_STACK_GUARD on ARM is only implemented for Mach-O targets, and
other targets rely on the generic support which may result in spilling of the
stack canary value or address, or may cause it to be kept in a callee save
register across function calls, which means they essentially get spilled as
well, only by the callee when it wants to free up this register.

So let's implement LOAD_STACK GUARD for other targets as well. This ensures
that the load of the stack canary is rematerialized fully in the epilogue.

This code was split off from

D112768: [ARM] implement support for TLS register based stack protector

for which it is a prerequisite.

[nickdesaulniers]

Changes LGTM; let's wait for @mstorsjo to triple check the COFF related changes, since I'm almost completely unaware of the details of that object file format. Thanks for the patch (and splitting it up, making it easier to review)!

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp

4904–4905

Other uses of ARMII::MO_COFFSTUB in llvm/lib/Target/ARM/ARMISelLowering.cpp seem to check TargetMachine::shouldAssumeDSOLocal returns false (and GlobalValue::hasDLLImportStorageClass() returns false, too) before setting this flag. Should we be doing so here as well?

To get a reference to the TargetMachine instance, I think we can do:

MachineFunction *MF = MBB.getParent();
ARMSubtarget &Subtarget = MF->getSubtarget<ARMSubtarget>();
TargetMachine *TM = SubTarget.getTargetLowering()->getTargetMachine();

@peter.smith anyone with more windows on ARM experience that might be able to help triple check this? Probably @mstorsjo ?

[nickdesaulniers]

llvm/lib/Target/ARM/Thumb2InstrInfo.cpp

250–259

Does Thumb1InstrInfo::expandLoadStackGuard need a similar change?

If so, then all the callers of ARMBaseInstrInfo::expandLoadStackGuardBase seem to also compute the same value of GV that is rematerialized in ARMBaseInstrInfo::expandLoadStackGuardBase. At that point, I'd say let's change the function signature of ARMBaseInstrInfo::expandLoadStackGuardBase to accept the GlobalValue* we calculated in the caller. Basically, let's DRY up const GlobalValue *GV = cast<GlobalValue>((*MI->memoperands_begin())->getValue()); which seems repeated in callers and callee.

Though if we don't need this change for thumb[1] then perhaps we don't need to do that (or could simply pass nullptr for that interface change.

[ardb]

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp

4904–4905

Other uses of ARMII::MO_COFFSTUB in llvm/lib/Target/ARM/ARMISelLowering.cpp seem to check TargetMachine::shouldAssumeDSOLocal returns false (and GlobalValue::hasDLLImportStorageClass() returns false, too) before setting this flag. Should we be doing so here as well?

!TargetMachine::shouldAssumeDSOLocal() is implied by isGVIndirectSymbol() so we already check this.

To get a reference to the TargetMachine instance, I think we can do:

MachineFunction *MF = MBB.getParent();
ARMSubtarget &Subtarget = MF->getSubtarget<ARMSubtarget>();
TargetMachine *TM = SubTarget.getTargetLowering()->getTargetMachine();

@peter.smith anyone with more windows on ARM experience that might be able to help triple check this? Probably @mstorsjo ?

llvm/lib/Target/ARM/Thumb2InstrInfo.cpp

250–259

No it does not.

[peter.smith]

Adding ostannard as a reviewer, I'm on vacation this week and Oliver knows this area better than I do.

[mstorsjo]

In D112811#3097762, @nickdesaulniers wrote:

Changes LGTM; let's wait for @mstorsjo to triple check the COFF related changes, since I'm almost completely unaware of the details of that object file format. Thanks for the patch (and splitting it up, making it easier to review)!

The change to the testcase looks correct given the description of the commit, and the reasoning around isGVIndirectSymbol seems correct too. Thanks!

[hans]

We bisected some Chromium test failures on Android, which points to this change (https://crbug.com/1270361). We'll investigate further, just wanted to give a heads up.

[hans]

In D112811#3144510, @hans wrote:

We bisected some Chromium test failures on Android, which points to this change (https://crbug.com/1270361). We'll investigate further, just wanted to give a heads up.

What we're hitting seems to be that the LOAD_STACK_GUARD pseudo-instruction can be register allocated to one of the high Thumb registers (in our case r12), but gets expanded using the tLDRLIT_ga_pcrel pseudo, which cannot take a high Thumb register as output.

It tries anyway, and generates the following textual asm:

ldr     r12, .LCPI0_0

but what it actually encodes is this:

4: 11 4c         ldr     r4, [pc, #68]

(Note the different register.)

I've uploaded a reproducer here: https://bugs.chromium.org/p/chromium/issues/detail?id=1270361#c30

I'm not sure what the best fix is. Maybe the register class for LOAD_STACK_GUARD could be restricted, or maybe tLDRLIT_ga_pcrel could be made to work with the high registers.

In any case, unless you can think of an immediate fix, I'd like to revert this in the meantime.

[nickdesaulniers]

In D112811#3159051, @hans wrote:

In D112811#3144510, @hans wrote:

We bisected some Chromium test failures on Android, which points to this change (https://crbug.com/1270361). We'll investigate further, just wanted to give a heads up.

What we're hitting seems to be that the LOAD_STACK_GUARD pseudo-instruction can be register allocated to one of the high Thumb registers (in our case r12), but gets expanded using the tLDRLIT_ga_pcrel pseudo, which cannot take a high Thumb register as output.

It tries anyway, and generates the following textual asm:

ldr     r12, .LCPI0_0

but what it actually encodes is this:

4: 11 4c         ldr     r4, [pc, #68]

(Note the different register.)

I've uploaded a reproducer here: https://bugs.chromium.org/p/chromium/issues/detail?id=1270361#c30

Seems like LLVM is not choosing the wide encoding, whereas GAS is:

$ arm-linux-gnueabi-as foo.s
$ arm-linux-gnueabi-objdump -dr a.out
...
   4:   f8df c038       ldr.w   ip, [pc, #56]   ; 40 <_ZN2v88internal4wasm16LiftoffAssembler13emit_i32_addiENS0_8RegisterES3_i+0x40>
...

$ llc -filetype=obj -relocation-model=pic -o a.out a.ll
$ arm-linux-gnueabi-objdump -dr a.out
...
   4:   4c0d            ldr     r4, [pc, #52]   ; (3c <_ZN2v88internal4wasm16LiftoffAssembler13emit_i32_addiENS0_8RegisterES3_i+0x3c>)
...

I don't see anything in F5.1.73 LDR (literal) of the ARM ARM regarding the use of of ip/r12. GAS even errors if I try to force the narrow encoding with a .n suffix:

foo.s:33: Error: cannot honor width suffix -- `ldr.n r12,.LCPI0_0'

It looks like GAS doesn't support a dest/Rt greater than r7 for the narrow encoding? Do we not have a way to emit the wide encoding?

In any case, unless you can think of an immediate fix, I'd like to revert this in the meantime.

This patch has a child patch as well; D112768.

Follow up: https://reviews.llvm.org/D107872.

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp

for (const auto &Reg : ScratchRegs) {
     STM.addReg(Reg, RegState::Kill);
   }
 
   BB->erase(MI);
 }
 
 bool ARMBaseInstrInfo::expandPostRAPseudo(MachineInstr &MI) const {
   if (MI.getOpcode() == TargetOpcode::LOAD_STACK_GUARD) {
-    assert(getSubtarget().getTargetTriple().isOSBinFormatMachO() &&
-           "LOAD_STACK_GUARD currently supported only for MachO.");
     expandLoadStackGuard(MI);
     MI.getParent()->erase(MI);
     return true;
   }
 
   if (MI.getOpcode() == ARM::MEMCPY) {
     expandMEMCPY(MI);
     return true;
if ((MI.getOperand(4).getImm() != 2 && MI.getOperand(4).getImm() != 3) ||
         MI.getOperand(4).getImm() != MI.getOperand(5).getImm() + 2) {
       ErrInfo = "Incorrect array index for MVE_VMOV_q_rr";
       return false;
     }
   }
   return true;
 }
 
-// LoadStackGuard has so far only been implemented for MachO. Different code
-// sequence is needed for other targets.
 void ARMBaseInstrInfo::expandLoadStackGuardBase(MachineBasicBlock::iterator MI,
                                                 unsigned LoadImmOpc,
                                                 unsigned LoadOpc) const {
   assert(!Subtarget.isROPI() && !Subtarget.isRWPI() &&
          "ROPI/RWPI not currently supported with stack guard");
 
   MachineBasicBlock &MBB = *MI->getParent();
   DebugLoc DL = MI->getDebugLoc();
   Register Reg = MI->getOperand(0).getReg();
   const GlobalValue *GV =
       cast<GlobalValue>((*MI->memoperands_begin())->getValue());
+  bool IsIndirect = Subtarget.isGVIndirectSymbol(GV);
   MachineInstrBuilder MIB;
 
+  unsigned TargetFlags = ARMII::MO_NO_FLAG;
+  if (Subtarget.isTargetMachO()) {
+    TargetFlags |= ARMII::MO_NONLAZY;
+  } else if (Subtarget.isTargetCOFF()) {
+    if (GV->hasDLLImportStorageClass())
+      TargetFlags |= ARMII::MO_DLLIMPORT;
+    else if (IsIndirect)
+      TargetFlags |= ARMII::MO_COFFSTUB;
+  } else if (Subtarget.isGVInGOT(GV)) {
+    TargetFlags |= ARMII::MO_GOT;
+  }
+
   BuildMI(MBB, MI, DL, get(LoadImmOpc), Reg)
-      .addGlobalAddress(GV, 0, ARMII::MO_NONLAZY);
+      .addGlobalAddress(GV, 0, TargetFlags);
 
-  if (Subtarget.isGVIndirectSymbol(GV)) {
+  if (IsIndirect) {
     MIB = BuildMI(MBB, MI, DL, get(LoadOpc), Reg);
     MIB.addReg(Reg, RegState::Kill).addImm(0);
     auto Flags = MachineMemOperand::MOLoad |
                  MachineMemOperand::MODereferenceable |
                  MachineMemOperand::MOInvariant;
     MachineMemOperand *MMO = MBB.getParent()->getMachineMemOperand(
         MachinePointerInfo::getGOT(*MBB.getParent()), Flags, 4, Align(4));
     MIB.addMemOperand(MMO).add(predOps(ARMCC::AL));

llvm/lib/Target/ARM/ARMISelLowering.cpp

ARMTargetLowering::shouldExpandAtomicCmpXchgInIR(AtomicCmpXchgInst *AI) const {
   return AtomicExpansionKind::None;
 }
 
 bool ARMTargetLowering::shouldInsertFencesForAtomic(
     const Instruction *I) const {
   return InsertFencesForAtomic;
 }
 
-// This has so far only been implemented for MachO.
-bool ARMTargetLowering::useLoadStackGuardNode() const {
-  return Subtarget->isTargetMachO();
-}
+bool ARMTargetLowering::useLoadStackGuardNode() const { return true; }
 
 void ARMTargetLowering::insertSSPDeclarations(Module &M) const {
   if (!Subtarget->getTargetTriple().isWindowsMSVCEnvironment())
     return TargetLowering::insertSSPDeclarations(M);
 
   // MSVC CRT has a global variable holding security cookie.
   M.getOrInsertGlobal("__security_cookie",
                       Type::getInt8PtrTy(M.getContext()));

llvm/lib/Target/ARM/ARMInstrInfo.cpp

unsigned ARMInstrInfo::getUnindexedOpcode(unsigned Opc) const {
   return 0;
 }
 
 void ARMInstrInfo::expandLoadStackGuard(MachineBasicBlock::iterator MI) const {
   MachineFunction &MF = *MI->getParent()->getParent();
   const ARMSubtarget &Subtarget = MF.getSubtarget<ARMSubtarget>();
   const TargetMachine &TM = MF.getTarget();
 
-  if (!Subtarget.useMovt()) {
+  const GlobalValue *GV =
+      cast<GlobalValue>((*MI->memoperands_begin())->getValue());
+
+  if (!Subtarget.useMovt() || Subtarget.isGVInGOT(GV)) {
     if (TM.isPositionIndependent())
       expandLoadStackGuardBase(MI, ARM::LDRLIT_ga_pcrel, ARM::LDRi12);
     else
       expandLoadStackGuardBase(MI, ARM::LDRLIT_ga_abs, ARM::LDRi12);
     return;
   }
 
   if (!TM.isPositionIndependent()) {
     expandLoadStackGuardBase(MI, ARM::MOVi32imm, ARM::LDRi12);
     return;
   }
 
-  const GlobalValue *GV =
-      cast<GlobalValue>((*MI->memoperands_begin())->getValue());
-
   if (!Subtarget.isGVIndirectSymbol(GV)) {
     expandLoadStackGuardBase(MI, ARM::MOV_ga_pcrel, ARM::LDRi12);
     return;
   }
 
   MachineBasicBlock &MBB = *MI->getParent();
   DebugLoc DL = MI->getDebugLoc();
   Register Reg = MI->getOperand(0).getReg();

llvm/lib/Target/ARM/Thumb2InstrInfo.cpp

if (ARM::GPRPairRegClass.hasSubClassEq(RC)) {
     if (Register::isPhysicalRegister(DestReg))
       MIB.addReg(DestReg, RegState::ImplicitDefine);
     return;
   }
 
   ARMBaseInstrInfo::loadRegFromStackSlot(MBB, I, DestReg, FI, RC, TRI);
 }
 
 void Thumb2InstrInfo::expandLoadStackGuard(
     MachineBasicBlock::iterator MI) const {
   MachineFunction &MF = *MI->getParent()->getParent();
-  if (MF.getTarget().isPositionIndependent())
+  const GlobalValue *GV =
+      cast<GlobalValue>((*MI->memoperands_begin())->getValue());
+
+  if (MF.getSubtarget<ARMSubtarget>().isGVInGOT(GV))
+    expandLoadStackGuardBase(MI, ARM::tLDRLIT_ga_pcrel, ARM::t2LDRi12);
+  else if (MF.getTarget().isPositionIndependent())
     expandLoadStackGuardBase(MI, ARM::t2MOV_ga_pcrel, ARM::t2LDRi12);
   else
     expandLoadStackGuardBase(MI, ARM::t2MOVi32imm, ARM::t2LDRi12);
 }
 
 MachineInstr *Thumb2InstrInfo::commuteInstructionImpl(MachineInstr &MI,
                                                       bool NewMI,
                                                       unsigned OpIdx1,
                                                       unsigned OpIdx2) const {

llvm/test/CodeGen/ARM/ssp-data-layout.ll

 %struct.struct_large_char2 = type { [2 x i8], [8 x i8] }
 %struct.struct_small_char = type { [2 x i8] }
 %struct.struct_large_nonchar = type { [8 x i32] }
 %struct.struct_small_nonchar = type { [2 x i16] }
 
 define void @layout_ssp() ssp {
 entry:
 ; Expected stack layout for ssp is
-;  176 large_char          . Group 1, nested arrays, arrays >= ssp-buffer-size
-;  168 struct_large_char   .
-;  164 scalar1             | Everything else
-;  160 scalar2
-;  156 scalar3
-;  152 addr-of
-;  148 small_nonchar
+;  180 large_char          . Group 1, nested arrays, arrays >= ssp-buffer-size
+;  172 struct_large_char   .
+;  168 scalar1             | Everything else
+;  164 scalar2
+;  160 scalar3
+;  156 addr-of
+;  152 small_nonchar
 ;  112 large_nonchar
 ;  110 small_char
 ;  108 struct_small_char
 ;   72 struct_large_nonchar
 ;   68 struct_small_nonchar
 
 ; CHECK: layout_ssp:
 
 ; CHECK: bl get_scalar1
-; CHECK: str r0, [sp, #164]
+; CHECK: str r0, [sp, #168]
 ; CHECK: bl end_scalar1
 
 ; CHECK: bl get_scalar2
-; CHECK: str r0, [sp, #160]
-; CHECK: bl end_scalar2
+; CHECK: str r0, [sp, #164]
+; CHECK: bl end_scalar
 
 ; CHECK: bl get_scalar3
-; CHECK: str r0, [sp, #156]
+; CHECK: str r0, [sp, #160]
 ; CHECK: bl end_scalar3
 
 ; CHECK: bl get_addrof
-; CHECK: str r0, [sp, #152]
+; CHECK: str r0, [sp, #156]
 ; CHECK: bl end_addrof
 
 ; CHECK: get_small_nonchar
-; CHECK: strh r0, [sp, #148]
+; CHECK: strh r0, [sp, #152]
 ; CHECK: bl end_small_nonchar
 
 ; CHECK: bl get_large_nonchar
 ; CHECK: str r0, [sp, #112]
 ; CHECK: bl end_large_nonchar
 
 ; CHECK: bl get_small_char
 ; CHECK: strb r0, [sp, #110]
 ; CHECK: bl end_small_char
 
 ; CHECK: bl get_large_char
-; CHECK: strb r0, [sp, #176]
+; CHECK: strb r0, [sp, #180]
 ; CHECK: bl end_large_char
 
 ; CHECK: bl get_struct_large_char
-; CHECK: strb r0, [sp, #168]
+; CHECK: strb r0, [sp, #172]
 ; CHECK: bl end_struct_large_char
 
 ; CHECK: bl get_struct_small_char
 ; CHECK: strb r0, [sp, #108]
 ; CHECK: bl end_struct_small_char
 
 ; CHECK: bl get_struct_large_nonchar
 ; CHECK:str r0, [sp, #72]

llvm/test/CodeGen/ARM/stack-guard-reassign.ll

 ; RUN: llc -O0 --frame-pointer=none -mtriple=arm-- -o - %S/../Inputs/stack-guard-reassign.ll | FileCheck %s
 
 ; Verify that the offset assigned to the stack protector is at the top of the
 ; frame, covering the locals.
 ; CHECK-LABEL: fn:
-; CHECK:      sub sp, sp, #24
+; CHECK:      sub sp, sp, #16
 ; CHECK-NEXT: sub sp, sp, #65536
 ; CHECK-NEXT: ldr r1, .LCPI0_0
-; CHECK-NEXT: str r1, [sp, #8]
 ; CHECK-NEXT: ldr r1, [r1]
 ; CHECK-NEXT: add lr, sp, #65536
-; CHECK-NEXT: str r1, [lr, #20]
+; CHECK-NEXT: str r1, [lr, #12]
 ; CHECK: .LCPI0_0:
 ; CHECK-NEXT: .long __stack_chk_guard

llvm/test/CodeGen/ARM/struct_byval.ll

 ; CHECK-LABEL: g:
 ; CHECK: ldr
 ; CHECK: sub
 ; CHECK: str
 ; CHECK: bne
 ; NACL-LABEL: g:
 ; Ensure that use movw instead of constpool for the loop trip count. But don't
 ; match the __stack_chk_guard movw
-; NACL: movw r{{[1-9]}}, #
+; NACL: movw {{r[0-9]+|lr}}, #
 ; NACL: ldr
 ; NACL: sub
 ; NACL: str
 ; NACL: bne
   %st = alloca %struct.LargeStruct, align 4
   %call = call i32 @e2(%struct.LargeStruct* byval(%struct.LargeStruct) %st)
   ret i32 0
 }
 
 ; Generate a loop using NEON instructions
 define i32 @h() nounwind ssp {
 entry:
 ; CHECK-LABEL: h:
 ; CHECK: vld1
 ; CHECK: sub
 ; CHECK: vst1
 ; CHECK: bne
-; NACL: movw r{{[1-9]}}, #
+; NACL: movw {{r[0-9]+|lr}}, #
 ; NACL: vld1
 ; NACL: sub
 ; NACL: vst1
 ; NACL: bne
   %st = alloca %struct.LargeStruct, align 16
   %call = call i32 @e3(%struct.LargeStruct* byval(%struct.LargeStruct) align 16 %st)
   ret i32 0
 }

llvm/test/CodeGen/ARM/tail-call-scheduling.ll

 ; RUN: llc < %s | FileCheck %s
 target triple = "armv6kz-unknown-unknown-gnueabihf"
 
 ; Make sure this doesn't crash, and we actually emit a tail call.
 ; Unfortunately, this test is sort of fragile... the original issue only
 ; shows up if scheduling happens in a very specific order. But including
 ; it anyway just to demonstrate the issue.
-; CHECK: pop {r4, lr}
+; CHECK: pop {r{{[0-9]+}}, lr}
 
 @e = external local_unnamed_addr constant [0 x i32 (i32, i32)*], align 4
 
 ; Function Attrs: nounwind sspstrong
 define i32 @AVI_ChunkRead_p_chk(i32 %g) nounwind sspstrong "target-cpu"="arm1176jzf-s" {
 entry:
   %b = alloca i8, align 1
   %tobool = icmp eq i32 %g, 0

llvm/test/CodeGen/ARM/win32-ssp.ll

 ; RUN: llc -mtriple=thumbv7-w64-mingw32 < %s -o - | FileCheck --check-prefix=MINGW %s
 
 declare void @llvm.lifetime.start.p0i8(i64, i8* nocapture)
 declare dso_local void @other(i8*)
 declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture)
 
 define dso_local void @func() sspstrong {
 entry:
 ; MINGW-LABEL: func:
 ; MINGW: movw [[REG:r[0-9]+]], :lower16:.refptr.__stack_chk_guard
 ; MINGW: movt [[REG]], :upper16:.refptr.__stack_chk_guard
 ; MINGW: ldr [[REG2:r[0-9]+]], {{\[}}[[REG]]]
 ; MINGW: ldr {{r[0-9]+}}, {{\[}}[[REG2]]]
 ; MINGW: bl other
-; MINGW: ldr {{r[0-9]+}}, {{\[}}[[REG2]]]
+; MINGW: movw [[REG3:r[0-9]+]], :lower16:.refptr.__stack_chk_guard
+; MINGW: movt [[REG3]], :upper16:.refptr.__stack_chk_guard
+; MINGW: ldr [[REG4:r[0-9]+]], {{\[}}[[REG3]]]
+; MINGW: ldr {{r[0-9]+}}, {{\[}}[[REG4]]]
 ; MINGW: bl __stack_chk_fail
 
   %c = alloca i8, align 1
   call void @llvm.lifetime.start.p0i8(i64 1, i8* nonnull %c)
   call void @other(i8* nonnull %c)
   call void @llvm.lifetime.end.p0i8(i64 1, i8* nonnull %c)
   ret void
 }