[PEI] Calculate stack protector frame offset before local stack allocation.

[emaste]

Summary

In cases where virtual frame index registers are used, make sure the canary is
calculated at an offset above those, otherwise it's placed in the wrong part of
the stack.

This was only causing problems with variable length arguments lists.

Fixes PR25610.

[• chatur01]

Ping after holiday season :)

[rengolin]

This looks like a simple patch, but I don't claim to understand the original problem, so I'm not sure what we're dealing with here.

@andrew, @t.p.northover, could you guys have a look?

lib/CodeGen/PrologEpilogInserter.cpp

636

This seems to imply that the move down of the code above is good.

[andrew]

With this the failing FreeBSD ssp test now passes.

[gberry]

This change seems reasonable to me. I don't think the test is quite right, specifically the CHECK line checking the store of the canary value. I don't think the issue is whether it is being stored relative to the frame pointer or stack pointer, but rather where on the stack it is being stored in relation to the local variables. The below test is simpler and hopefully captures more of the constraints we'd like to verify, though it may be more sensitive to codegen changes.

; RUN: llc -mtriple aarch64--linux < %s | FileCheck %s

; CHECK-LABEL: test:
; Make sure the canary is placed just above the alloca on the stack.
; CHECK: stp {{x[0-9]+}}, {{x[0-9]+}}, [sp, #-32]!
; CHECK: add x29, sp, #16
; CHECK-NEXT: sub sp, sp, #8, lsl #12
; CHECK: ldr [[GUARD:x[0-9]+]]{{.*}}:lo12:__stack_chk_guard
; CHECK: sub [[GUARDADDR:x[0-9]+]], x29, #32
; CHECK-NEXT: str [[GUARD]], {{\[}}[[GUARDADDR]]]
; CHECK-NEXT: str x0, [sp, #-16]!

; Function Attrs: ssp
define void @test(i64 %x) #0 {
entry:
  %buf = alloca i64, i32 4096, align 8
  %idx = getelementptr inbounds i64, i64* %buf, i64 0
  store i64 %x, i64* %idx, align 8
  ret void
}

attributes #0 = { ssp }

[emaste]

Grab this: I believe @chatur01 will not be able to pursue it

[emaste]

rebase

[andrew]

I think something else has changed as I always get the expected SIGABRT either with or without this patch.

[gberry]

Removing myself as this seems to be dead

[thegameg]

A fix for this got in: rL366371. Can we close this revision?

lib/CodeGen/PrologEpilogInserter.cpp

void PEI::calculateFrameObjectOffsets(MachineFunction &Fn) {
   if (RS && EarlyScavengingSlots) {
     SmallVector<int, 2> SFIs;
     RS->getScavengingFrameIndices(SFIs);
     for (SmallVectorImpl<int>::iterator I = SFIs.begin(),
            IE = SFIs.end(); I != IE; ++I)
       AdjustStackOffset(MFI, *I, StackGrowsDown, Offset, MaxAlign, Skew);
   }
 
-  // FIXME: Once this is working, then enable flag will change to a target
-  // check for whether the frame is large enough to want to use virtual
-  // frame index registers. Functions which don't want/need this optimization
-  // will continue to use the existing code path.
-  if (MFI->getUseLocalStackAllocationBlock()) {
-    unsigned Align = MFI->getLocalFrameMaxAlign();
-
-    // Adjust to alignment boundary.
-    Offset = alignTo(Offset, Align, Skew);
-
-    DEBUG(dbgs() << "Local frame base offset: " << Offset << "\n");
-
-    // Resolve offsets for objects in the local block.
-    for (unsigned i = 0, e = MFI->getLocalFrameObjectCount(); i != e; ++i) {
-      std::pair<int, int64_t> Entry = MFI->getLocalFrameObjectMap(i);
-      int64_t FIOffset = (StackGrowsDown ? -Offset : Offset) + Entry.second;
-      DEBUG(dbgs() << "alloc FI(" << Entry.first << ") at SP[" <<
-            FIOffset << "]\n");
-      MFI->setObjectOffset(Entry.first, FIOffset);
-    }
-    // Allocate the local block
-    Offset += MFI->getLocalFrameSize();
-
-    MaxAlign = std::max(Align, MaxAlign);
-  }
-
   // Make sure that the stack protector comes before the local variables on the
   // stack.
   SmallSet<int, 16> ProtectedObjs;
   if (MFI->getStackProtectorIndex() >= 0) {
     StackObjSet LargeArrayObjs;
     StackObjSet SmallArrayObjs;
     StackObjSet AddrOfObjs;
 
     AdjustStackOffset(MFI, MFI->getStackProtectorIndex(), StackGrowsDown,
                       Offset, MaxAlign, Skew);
if (MFI->getStackProtectorIndex() >= 0) {
     AssignProtectedObjSet(LargeArrayObjs, ProtectedObjs, MFI, StackGrowsDown,
                           Offset, MaxAlign, Skew);
     AssignProtectedObjSet(SmallArrayObjs, ProtectedObjs, MFI, StackGrowsDown,
                           Offset, MaxAlign, Skew);
     AssignProtectedObjSet(AddrOfObjs, ProtectedObjs, MFI, StackGrowsDown,
                           Offset, MaxAlign, Skew);
   }
 
+  // FIXME: Once this is working, then enable flag will change to a target
+  // check for whether the frame is large enough to want to use virtual
+  // frame index registers. Functions which don't want/need this optimization
+  // will continue to use the existing code path.
+  if (MFI->getUseLocalStackAllocationBlock()) {
+    unsigned Align = MFI->getLocalFrameMaxAlign();
+
+    // Adjust to alignment boundary.
+    Offset = alignTo(Offset, Align, Skew);
+
+    DEBUG(dbgs() << "Local frame base offset: " << Offset << "\n");
+
+    // Resolve offsets for objects in the local block.
+    for (unsigned i = 0, e = MFI->getLocalFrameObjectCount(); i != e; ++i) {
+      std::pair<int, int64_t> Entry = MFI->getLocalFrameObjectMap(i);
+      int64_t FIOffset = (StackGrowsDown ? -Offset : Offset) + Entry.second;
+      DEBUG(dbgs() << "alloc FI(" << Entry.first << ") at SP[" <<
+            FIOffset << "]\n");
+      MFI->setObjectOffset(Entry.first, FIOffset);
+    }
+    // Allocate the local block
+    Offset += MFI->getLocalFrameSize();
+
+    MaxAlign = std::max(Align, MaxAlign);
+  }
+
   SmallVector<int, 8> ObjectsToAllocate;
 
   // Then prepare to assign frame offsets to stack objects that are not used to
   // spill callee saved registers.
   for (unsigned i = 0, e = MFI->getObjectIndexEnd(); i != e; ++i) {
     if (MFI->isObjectPreAllocated(i) &&
         MFI->getUseLocalStackAllocationBlock())
       continue;

test/CodeGen/AArch64/stack-guard-vaarg.ll

+; RUN: llc < %s | FileCheck %s
+target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64--linux"
+
+%struct.__va_list = type { i8*, i8*, i8*, i32, i32 }
+
+; CHECK-LABEL: test
+; CHECK: ldr [[GUARD:x[0-9]+]]{{.*}}:lo12:__stack_chk_guard
+; Make sure the canary is placed relative to the frame pointer, not
+; the stack pointer.
+; CHECK: stur [[GUARD]], [x29,
+
+; Function Attrs: nounwind ssp
+define void @test(i8* %i, ...) #0 {
+entry:
+  %buf = alloca [10 x i8], align 1
+  %ap = alloca %struct.__va_list, align 8
+  %tmp = alloca %struct.__va_list, align 8
+  %0 = getelementptr inbounds [10 x i8], [10 x i8]* %buf, i64 0, i64 0
+  call void @llvm.lifetime.start(i64 10, i8* %0)
+  %1 = bitcast %struct.__va_list* %ap to i8*
+  call void @llvm.lifetime.start(i64 32, i8* %1)
+  call void @llvm.va_start(i8* %1)
+  %2 = bitcast %struct.__va_list* %tmp to i8*
+  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %2, i8* %1, i64 32, i32 8, i1 false)
+  call void @baz(i8* %i, %struct.__va_list* nonnull %tmp)
+  call void @bar(i8* %0)
+  call void @llvm.va_end(i8* %1)
+  call void @llvm.lifetime.end(i64 32, i8* %1)
+  call void @llvm.lifetime.end(i64 10, i8* %0)
+  ret void
+}
+
+; Function Attrs: argmemonly nounwind
+declare void @llvm.lifetime.start(i64, i8* nocapture)
+
+; Function Attrs: nounwind
+declare void @llvm.va_start(i8*)
+
+declare void @baz(i8*, %struct.__va_list*)
+
+; Function Attrs: argmemonly nounwind
+declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture, i8* nocapture readonly, i64, i32, i1)
+
+declare void @bar(i8*)
+
+; Function Attrs: nounwind
+declare void @llvm.va_end(i8*)
+
+; Function Attrs: argmemonly nounwind
+declare void @llvm.lifetime.end(i64, i8* nocapture)
+
+attributes #0 = { ssp }