[safestack] Move allocations to support separate stack segment

[mlemay-intel]

Summary

When some option is enabled that necessitates using only local stack
pointers, move allocations to the unsafe stack to help satisfy that
requirement. The only option of this type that is currently supported
is the separate stack segment option for x86-32.

[eugenis]

lib/CodeGen/SafeStack.cpp

61

This is hard to read, please change to something like safe-stack-subroutine-access-unsafe.

[mlemay-intel]

lib/CodeGen/SafeStack.cpp

61

That sounds good. I'll wait for any additional comments you may have on the patches so I can batch this in with other revisions, if you don't mind.

[qcolombet]

Please add ll-commit as a subscriber as well.

Thanks,
-Quentin

[qcolombet]

FWIW, LGTM but please address Eugenis’ comment.

I believe Eugenis is better suited to give the final LGTM.

Cheers,
-Quentin

[pcc]

Regarding the first motivation, the code is already being conservative. As far as I know, the nocapture readnone attributes are stronger than what safestack requires of a pointer. For example, they forbid loading from a pointer at a constant offset (which safestack could conceivably allow). Rather than adding this flag, we should in the long term be adding an attribute that specifically means what safestack requires.

Regarding the second motivation, if we do intend to start using segment selectors on x86 in some cases, we should probably make this behavior gated on the target (e.g. the separate-stack-seg feature you appear to be adding) rather than on a flag.

[mlemay-intel]

In D17094#375794, @pcc wrote:

I apologize for not responding to this until now. Email notifications of new activity on my patches are apparently not making it to my inbox for some reason.

Regarding the first motivation, the code is already being conservative. As far as I know, the nocapture readnone attributes are stronger than what safestack requires of a pointer. For example, they forbid loading from a pointer at a constant offset (which safestack could conceivably allow). Rather than adding this flag, we should in the long term be adding an attribute that specifically means what safestack requires.

Thank you for pointing out my misinterpretation of the FIXME. I will update the commit message accordingly.

Regarding the second motivation, if we do intend to start using segment selectors on x86 in some cases, we should probably make this behavior gated on the target (e.g. the separate-stack-seg feature you appear to be adding) rather than on a flag.

This would result in simpler command line option sequences. I'll try to revise this patch to do what you suggested.

[mlemay-intel]

Check for the separate-stack-seg feature instead of defining a new command line option.

[pcc]

Please update the commit message and add a test case.

[mlemay-intel]

Add test. Do not move allocations to unsafe stack simply due to them being passed to lifetime intrinsics.

[pcc]

LGTM

lib/CodeGen/SafeStack.cpp

static cl::opt<UnsafeStackPtrStorageVal> USPStorage("safe-stack-usp-storage",
     cl::Hidden, cl::init(ThreadLocalUSP),
     cl::desc("Type of storage for the unsafe stack pointer"),
     cl::values(clEnumValN(ThreadLocalUSP, "thread-local",
                           "Thread-local storage"),
                clEnumValN(SingleThreadUSP, "single-thread",
                           "Non-thread-local storage"),
                clEnumValEnd));
 
+static cl::opt<bool> SubroutineAccessAsUnsafe("safe-stack-subr-acc-as-unsafe",
+    cl::Hidden, cl::init(false),
+    cl::desc("Allocate memory on the unsafe stack if a pointer to it is passed "
+             "to a subroutine"));
+
 namespace llvm {
 
 STATISTIC(NumFunctions, "Total number of functions");
 STATISTIC(NumUnsafeStackFunctions, "Number of functions with unsafe stack");
 STATISTIC(NumUnsafeStackRestorePointsFunctions,
           "Number of functions that use setjmp or exceptions");
 
 STATISTIC(NumAllocas, "Total number of allocas");
for (const Use &UI : V->uses()) {
             DEBUG(dbgs() << "[SafeStack] Unsafe alloca: " << *AllocaPtr
                          << "\n            unsafe memintrinsic: " << *I
                          << "\n");
             return false;
           }
           continue;
         }
 
+        if (SubroutineAccessAsUnsafe)
+          return false;
+
         // LLVM 'nocapture' attribute is only set for arguments whose address
         // is not stored, passed around, or used in any other non-trivial way.
         // We assume that passing a pointer to an object as a 'nocapture
         // readnone' argument is safe.
         // FIXME: a more precise solution would require an interprocedural
         // analysis here, which would look at all uses of an argument inside
         // the function being called.
         ImmutableCallSite::arg_iterator B = CS.arg_begin(), E = CS.arg_end();

test/Transforms/SafeStack/call.ll

 ; RUN: opt -safe-stack -S -mtriple=i386-pc-linux-gnu < %s -o - | FileCheck %s
 ; RUN: opt -safe-stack -S -mtriple=x86_64-pc-linux-gnu < %s -o - | FileCheck %s
+; RUN: opt -safe-stack -safe-stack-subr-acc-as-unsafe -S -mtriple=i386-pc-linux-gnu < %s -o - | FileCheck -check-prefix=SUBR-ACC-AS-UNSAFE %s
+; RUN: opt -safe-stack -safe-stack-subr-acc-as-unsafe -S -mtriple=x86_64-pc-linux-gnu < %s -o - | FileCheck -check-prefix=SUBR-ACC-AS-UNSAFE %s
 
 @.str = private unnamed_addr constant [4 x i8] c"%s\0A\00", align 1
 
 ; no arrays / no nested arrays
 ; Requires no protector.
 
 define void @foo(i8* %a) nounwind uwtable safestack {
 entry:
entry:
   ; CHECK-NOT: @__safestack_unsafe_stack_ptr
   ; CHECK: ret void
   %q = alloca [10 x i8], align 1
   %arraydecay = getelementptr inbounds [10 x i8], [10 x i8]* %q, i32 0, i32 0
   call void @readnone(i8* %arraydecay)
   ret void
 }
 
-; Arg0 is readnone, arg1 is not. Pass alloca ptr as arg0 -> safe
+; Arg0 is readnone, arg1 is not. Pass alloca ptr as arg0 -> safe unless -safe-stack-subr-acc-as-unsafe is set.
 define void @call_readnone0_0(i64 %len) safestack {
 entry:
   ; CHECK-LABEL: define void @call_readnone0_0
   ; CHECK-NOT: @__safestack_unsafe_stack_ptr
+  ; SUBR-ACC-AS-UNSAFE: @__safestack_unsafe_stack_ptr
   ; CHECK: ret void
   %q = alloca [10 x i8], align 1
   %arraydecay = getelementptr inbounds [10 x i8], [10 x i8]* %q, i32 0, i32 0
   call void @readnone0(i8* %arraydecay, i8* zeroinitializer)
   ret void
 }
 
 ; Arg0 is readnone, arg1 is not. Pass alloca ptr as arg1 -> unsafe