Page MenuHomePhabricator

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

Authored by emaste on Dec 24 2015, 10:56 AM.

Details

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.

Diff Detail

Event Timeline

chatur01 retitled this revision from to [PEI] Calculate stack protector frame offset before local stack allocation..
chatur01 updated this object.
chatur01 set the repository for this revision to rL LLVM.
chatur01 updated this object.
chatur01 added a subscriber: llvm-commits.
emaste added a subscriber: emaste.Dec 24 2015, 11:08 AM

Ping after holiday season :)

rengolin added subscribers: t.p.northover, 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 edited edge metadata.Feb 9 2016, 10:48 AM

With this the failing FreeBSD ssp test now passes.

gberry edited edge metadata.Feb 10 2016, 11:49 AM

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 commandeered this revision.Feb 25 2016, 10:28 AM
emaste added a reviewer: chatur01.

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

emaste updated this revision to Diff 49091.Feb 25 2016, 10:30 AM
emaste edited edge metadata.
emaste removed rL LLVM as the repository for this revision.

rebase

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

gberry resigned from this revision.May 23 2016, 1:42 PM
gberry removed a reviewer: gberry.

Removing myself as this seems to be dead

void removed a reviewer: void.Apr 17 2018, 2:24 PM

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