[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

64

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

[mlemay-intel]

lib/CodeGen/SafeStack.cpp

64

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

include/llvm/Target/TargetLowering.h

protected:
   /// \brief Initialize all of the actions to default values.
   void initActions();
 
 public:
   const TargetMachine &getTargetMachine() const { return TM; }
 
   virtual bool useSoftFloat() const { return false; }
 
+  /// A target may require that no pointer passed from one function to another
+  /// points to the main stack.  Such a target must override this function to
+  /// return true.  SafeStack must always be used for such a target, to move
+  /// any stack allocation to the unsafe stack that is the target of a pointer
+  /// passed to a subroutine.
+  virtual bool useOnlyLocalStackPointers() const { return false; }
+
   /// Return the pointer type for the given address space, defaults to
   /// the pointer type from the data layout.
   /// FIXME: The default needs to be removed once all the code is updated.
   MVT getPointerTy(const DataLayout &DL, uint32_t AS = 0) const {
     return MVT::getIntegerVT(DL.getPointerSizeInBits(AS));
   }
 
   /// EVT is not used in-tree, but is used by out-of-tree target.

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));
 
 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");
 STATISTIC(NumUnsafeStaticAllocas, "Number of unsafe static allocas");
for (const Use &UI : V->uses()) {
         ImmutableCallSite CS(I);
 
         if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(I)) {
           if (II->getIntrinsicID() == Intrinsic::lifetime_start ||
               II->getIntrinsicID() == Intrinsic::lifetime_end)
             continue;
         }
 
+        // FIXME: A more precise solution may consider whether particular types
+        // of intrinsics necessitate moving allocations to the unsafe stack.
+        if (TL->useOnlyLocalStackPointers())
+          return false;
+
         if (const MemIntrinsic *MI = dyn_cast<MemIntrinsic>(I)) {
           if (!IsMemIntrinsicSafe(MI, UI, AllocaPtr, AllocaSize)) {
             DEBUG(dbgs() << "[SafeStack] Unsafe alloca: " << *AllocaPtr
                          << "\n            unsafe memintrinsic: " << *I
                          << "\n");
             return false;
           }
           continue;

lib/Target/X86/X86ISelLowering.h

namespace llvm {
   //  X86 Implementation of the TargetLowering interface
   class X86TargetLowering final : public TargetLowering {
   public:
     explicit X86TargetLowering(const X86TargetMachine &TM,
                                const X86Subtarget &STI);
 
     unsigned getJumpTableEncoding() const override;
     bool useSoftFloat() const override;
+    bool useOnlyLocalStackPointers() const override;
 
     MVT getScalarShiftAmountTy(const DataLayout &, EVT) const override {
       return MVT::i8;
     }
 
     const MCExpr *
     LowerCustomJumpTableEntry(const MachineJumpTableInfo *MJTI,
                               const MachineBasicBlock *MBB, unsigned uid,

lib/Target/X86/X86ISelLowering.cpp

unsigned X86TargetLowering::getJumpTableEncoding() const {
   // Otherwise, use the normal jump table encoding heuristics.
   return TargetLowering::getJumpTableEncoding();
 }
 
 bool X86TargetLowering::useSoftFloat() const {
   return Subtarget.useSoftFloat();
 }
 
+bool X86TargetLowering::useOnlyLocalStackPointers() const {
+  return Subtarget.useSeparateStackSeg();
+}
+
 const MCExpr *
 X86TargetLowering::LowerCustomJumpTableEntry(const MachineJumpTableInfo *MJTI,
                                              const MachineBasicBlock *MBB,
                                              unsigned uid,MCContext &Ctx) const{
   assert(MBB->getParent()->getTarget().getRelocationModel() == Reloc::PIC_ &&
          Subtarget.isPICStyleGOT());
   // In 32-bit ELF systems, our jump table entries are formed with @GOTOFF
   // entries.

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 -S -mtriple=i386-pc-linux-gnu -mattr=+separate-stack-seg < %s -o - | FileCheck --check-prefix SEP-STK %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:
   ; CHECK-LABEL: define void @foo(
   ; CHECK-NOT: __safestack_unsafe_stack_ptr
   ; CHECK: ret void
+  ; SEP-STK-LABEL: define void @foo(
+  ; SEP-STK-NOT: __safestack_unsafe_stack_ptr
+  ; SEP-STK: ret void
   %a.addr = alloca i8*, align 8
   store i8* %a, i8** %a.addr, align 8
   %0 = load i8*, i8** %a.addr, align 8
   %call = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* %0)
   ret void
 }
 
 declare i32 @printf(i8*, ...)
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 define void @call_memset(i64 %len) safestack {
 entry:
   ; CHECK-LABEL: define void @call_memset
   ; CHECK: @__safestack_unsafe_stack_ptr
   ; CHECK: ret void
+  ; SEP-STK-LABEL: define void @call_memset
+  ; SEP-STK: @__safestack_unsafe_stack_ptr
+  ; SEP-STK: 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 @llvm.memset.p0i8.i64(i8* %arraydecay, i8 1, i64 %len, i32 1, i1 false)
   ret void
 }
 
 define void @call_constant_memset() safestack {
 entry:
   ; CHECK-LABEL: define void @call_constant_memset
   ; CHECK-NOT: @__safestack_unsafe_stack_ptr
   ; CHECK: ret void
+  ; SEP-STK-LABEL: define void @call_constant_memset
+  ; SEP-STK: @__safestack_unsafe_stack_ptr
+  ; SEP-STK: ret void
   %q = alloca [10 x i8], align 1
   %arraydecay = getelementptr inbounds [10 x i8], [10 x i8]* %q, i32 0, i32 2
   call void @llvm.memset.p0i8.i64(i8* %arraydecay, i8 1, i64 7, i32 1, i1 false)
   ret void
 }
 
 define void @call_constant_overflow_memset() safestack {
 entry:
   ; CHECK-LABEL: define void @call_constant_overflow_memset
   ; CHECK: @__safestack_unsafe_stack_ptr
   ; CHECK: ret void
+  ; SEP-STK-LABEL: define void @call_constant_overflow_memset
+  ; SEP-STK: @__safestack_unsafe_stack_ptr
+  ; SEP-STK: ret void
   %q = alloca [10 x i8], align 1
   %arraydecay = getelementptr inbounds [10 x i8], [10 x i8]* %q, i32 0, i32 7
   call void @llvm.memset.p0i8.i64(i8* %arraydecay, i8 1, i64 5, i32 1, i1 false)
   ret void
 }
 
 define void @call_constant_underflow_memset() safestack {
 entry:
   ; CHECK-LABEL: define void @call_constant_underflow_memset
   ; CHECK: @__safestack_unsafe_stack_ptr
   ; CHECK: ret void
+  ; SEP-STK-LABEL: define void @call_constant_underflow_memset
+  ; SEP-STK: @__safestack_unsafe_stack_ptr
+  ; SEP-STK: ret void
   %q = alloca [10 x i8], align 1
   %arraydecay = getelementptr [10 x i8], [10 x i8]* %q, i32 0, i32 -1
   call void @llvm.memset.p0i8.i64(i8* %arraydecay, i8 1, i64 3, i32 1, i1 false)
   ret void
 }
 
 ; Readnone nocapture -> safe
 define void @call_readnone(i64 %len) safestack {
 entry:
   ; CHECK-LABEL: define void @call_readnone
   ; CHECK-NOT: @__safestack_unsafe_stack_ptr
   ; CHECK: ret void
+  ; SEP-STK-LABEL: define void @call_readnone
+  ; SEP-STK: @__safestack_unsafe_stack_ptr
+  ; SEP-STK: 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
 define void @call_readnone0_0(i64 %len) safestack {
 entry:
   ; CHECK-LABEL: define void @call_readnone0_0
   ; CHECK-NOT: @__safestack_unsafe_stack_ptr
   ; CHECK: ret void
+  ; SEP-STK-LABEL: define void @call_readnone0_0
+  ; SEP-STK: @__safestack_unsafe_stack_ptr
+  ; SEP-STK: 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
 define void @call_readnone0_1(i64 %len) safestack {
 entry:
   ; CHECK-LABEL: define void @call_readnone0_1
   ; CHECK: @__safestack_unsafe_stack_ptr
   ; CHECK: ret void
+  ; SEP-STK-LABEL: define void @call_readnone0_1
+  ; SEP-STK: @__safestack_unsafe_stack_ptr
+  ; SEP-STK: 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 *zeroinitializer, i8* %arraydecay)
   ret void
 }
 
 ; Readonly nocapture -> unsafe
 define void @call_readonly(i64 %len) safestack {
 entry:
   ; CHECK-LABEL: define void @call_readonly
   ; CHECK: @__safestack_unsafe_stack_ptr
   ; CHECK: ret void
+  ; SEP-STK-LABEL: define void @call_readonly
+  ; SEP-STK: @__safestack_unsafe_stack_ptr
+  ; SEP-STK: 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 @readonly(i8* %arraydecay)
   ret void
 }
 
 ; Readonly nocapture -> unsafe
 define void @call_arg_readonly(i64 %len) safestack {
 entry:
   ; CHECK-LABEL: define void @call_arg_readonly
   ; CHECK: @__safestack_unsafe_stack_ptr
   ; CHECK: ret void
+  ; SEP-STK-LABEL: define void @call_arg_readonly
+  ; SEP-STK: @__safestack_unsafe_stack_ptr
+  ; SEP-STK: 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 @arg_readonly(i8* %arraydecay)
   ret void
 }
 
 ; Readwrite nocapture -> unsafe
 define void @call_readwrite(i64 %len) safestack {
 entry:
   ; CHECK-LABEL: define void @call_readwrite
   ; CHECK: @__safestack_unsafe_stack_ptr
   ; CHECK: ret void
+  ; SEP-STK-LABEL: define void @call_readwrite
+  ; SEP-STK: @__safestack_unsafe_stack_ptr
+  ; SEP-STK: 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 @readwrite(i8* %arraydecay)
   ret void
 }
 
 ; Captures the argument -> unsafe
 define void @call_capture(i64 %len) safestack {
 entry:
   ; CHECK-LABEL: define void @call_capture
   ; CHECK: @__safestack_unsafe_stack_ptr
   ; CHECK: ret void
+  ; SEP-STK-LABEL: define void @call_capture
+  ; SEP-STK: @__safestack_unsafe_stack_ptr
+  ; SEP-STK: 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 @capture(i8* %arraydecay)
   ret void
 }
 
 ; Lifetime intrinsics are always safe.
 define void @call_lifetime(i32* %p) {
   ; CHECK-LABEL: define void @call_lifetime
   ; CHECK-NOT: @__safestack_unsafe_stack_ptr
   ; CHECK: ret void
+  ; SEP-STK-LABEL: define void @call_lifetime
+  ; SEP-STK-NOT: @__safestack_unsafe_stack_ptr
+  ; SEP-STK: ret void
 entry:
   %q = alloca [100 x i8], align 16
   %0 = bitcast [100 x i8]* %q to i8*
   call void @llvm.lifetime.start(i64 100, i8* %0)
   call void @llvm.lifetime.end(i64 100, i8* %0)
   ret void
 }