[safestack] Use non-thread-local unsafe stack pointer for Contiki OS

[mlemay-intel]

Summary

This patch causes a non-thread-local unsafe stack pointer to be used
with SafeStack for Contiki OS. To avoid generating a redundant copy
of the code for looking up or creating the unsafe stack pointer
variable, this patch moves that code from SafeStack to
TargetLoweringBase.

This patch removes the -mllvm -safe-stack-usp-storage option, since it
is no longer needed.

[mlemay-intel]

This patch and D19854 are useful independent of my segmentation-related patches. I also have a Contiki OS patch that is blocked on these. So, I would appreciate having these reviewed separately from my other patches. Thanks!

[eugenis]

include/llvm/ADT/Triple.h

166 ↗

(On Diff #55942)

Please add to the end of the list.

lib/Target/X86/X86ISelLowering.cpp

1940 ↗

(On Diff #55942)

Why duplicate all this code instead of just checking for isTargetContiki is SafeStack.cpp?

[mlemay-intel]

include/llvm/ADT/Triple.h

166 ↗

(On Diff #55942)

Will do.

lib/Target/X86/X86ISelLowering.cpp

1940 ↗

(On Diff #55942)

@pcc commented on D19483:

You should be using -target x86-unknown-contiki or similar. That should tune the behaviour to what is required for that OS. See what we have in TargetLoweringBase::getSafeStackPointerLocation to provide Android-specific behaviour for example.

My Contiki OS patches only add SafeStack support on X86, so that's another reason to put this code here.

[mlemay-intel]

Moved new OS type definition to end of list.

[mlemay-intel]

ping

[mlemay-intel]

ping. Please keep in mind that this is independent of my patches for supporting a separate stack segment. This patch is needed to enable SafeStack for Contiki OS.

[pcc]

lib/Target/X86/X86ISelLowering.cpp

2025 ↗

(On Diff #70072)

You might also consider moving the SafeStack::getOrCreateUnsafeStackPtr implementation into a utility function in TargetLoweringBase that takes a bool UseTLS. Then guarantee that getSafeStackPointerLocation always returns non-null and you can remove SafeStack::getOrCreateUnsafeStackPtr altogehter.

[mlemay-intel]

lib/Target/X86/X86ISelLowering.cpp

2025 ↗

(On Diff #70072)

That would be a nice way to eliminate this redundancy. However, the SafeStack pass is written to support TL being null. How would your suggested approach handle that case?

[pcc]

lib/Target/X86/X86ISelLowering.cpp

2025 ↗

(On Diff #70072)

What happens if you assert(TL);? Do any tests fail?

[mlemay-intel]

lib/Target/X86/X86ISelLowering.cpp

2025 ↗

(On Diff #70072)

I added assert(TL) right after the initialization of TL in runOnFunction and ran check-llvm and check-clang. There were no unexpected failures.

[pcc]

lib/Target/X86/X86ISelLowering.cpp

2025 ↗

(On Diff #70072)

In that case, I would suggest that you change the pass to require TL to be available. There's precedent for this in the CodeGen pipeline -- see the StackProtector pass.

[mlemay-intel]

Refactored to avoid duplicating code.

[pcc]

LGTM

include/llvm/Target/TargetLowering.h

1115 ↗

(On Diff #72401)

You might want to reword this more simply as something like "Returns the target-specific address of the unsafe stack pointer."

[mlemay-intel]

Reworded a comment.

[echristo]

Seems reasonable.

lib/Target/X86/X86ISelLowering.cpp

2027 ↗

(On Diff #72600)

Not sure that adding isTargetContiki to the subtarget is worth it here, but ok.

[mlemay-intel]

lib/Target/X86/X86ISelLowering.cpp

2027 ↗

(On Diff #72600)

TM is private in TargetLoweringBase, so I can't call TM.getTargetTriple().isOSContiki() from here.

[echristo]

lib/Target/X86/X86ISelLowering.cpp

2027 ↗

(On Diff #72600)

Subtarget.getTargetTriple().isOSContiki()?

[pcc]

include/llvm/Target/TargetLowering.h

1111 ↗

(On Diff #72600)

Doesn't need to be virtual.

[mlemay-intel]

Use Subtarget.getTargetTriple().isOSContiki instead of Subtarget.isTargetContiki.
Make TargetLoweringBase::getDefaultSafeStackPointerLocation non-virtual.

[echristo]

At this point if Peter and Evgenii are happy so am I.

llvm/trunk/include/llvm/Target/TargetLowering.h

public:
   virtual Value *getSDagStackGuard(const Module &M) const;
 
   /// If the target has a standard stack protection check function that
   /// performs validation and error handling, returns the function. Otherwise,
   /// returns nullptr. Must be previously inserted by insertSSPDeclarations.
   /// Should be used only when getIRStackGuard returns nullptr.
   virtual Value *getSSPStackGuardCheck(const Module &M) const;
 
-  /// If the target has a standard location for the unsafe stack pointer,
-  /// returns the address of that location. Otherwise, returns nullptr.
+protected:
+  Value *getDefaultSafeStackPointerLocation(IRBuilder<> &IRB,
+                                            bool UseTLS) const;
+
+public:
+  /// Returns the target-specific address of the unsafe stack pointer.
   virtual Value *getSafeStackPointerLocation(IRBuilder<> &IRB) const;
 
   /// Returns true if a cast between SrcAS and DestAS is a noop.
   virtual bool isNoopAddrSpaceCast(unsigned SrcAS, unsigned DestAS) const {
     return false;
   }
 
   /// Return true if the pointer arguments to CI should be aligned by aligning

llvm/trunk/lib/CodeGen/SafeStack.cpp

 #include "llvm/Transforms/Utils/Local.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
 
 using namespace llvm;
 using namespace llvm::safestack;
 
 #define DEBUG_TYPE "safestack"
 
-enum UnsafeStackPtrStorageVal { ThreadLocalUSP, SingleThreadUSP };
-
-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")));
-
 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");
class SafeStack : public FunctionPass {
   /// Unsafe stack alignment. Each stack frame must ensure that the stack is
   /// aligned to this value. We need to re-align the unsafe stack if the
   /// alignment of any object on the stack exceeds this value.
   ///
   /// 16 seems like a reasonable upper bound on the alignment of objects that we
   /// might expect to appear on the stack on most common targets.
   enum { StackAlignment = 16 };
 
-  /// \brief Build a value representing a pointer to the unsafe stack pointer.
-  Value *getOrCreateUnsafeStackPtr(IRBuilder<> &IRB, Function &F);
-
   /// \brief Return the value of the stack canary.
   Value *getStackGuard(IRBuilder<> &IRB, Function &F);
 
   /// \brief Load stack guard from the frame and check if it has changed.
   void checkStackGuard(IRBuilder<> &IRB, Function &F, ReturnInst &RI,
                        AllocaInst *StackGuardSlot, Value *StackGuard);
 
   /// \brief Find all static allocas, dynamic allocas, return instructions and
for (const Use &UI : V->uses()) {
       }
     }
   }
 
   // All uses of the alloca are safe, we can place it on the safe stack.
   return true;
 }
 
-Value *SafeStack::getOrCreateUnsafeStackPtr(IRBuilder<> &IRB, Function &F) {
-  // Check if there is a target-specific location for the unsafe stack pointer.
-  if (Value *V = TL->getSafeStackPointerLocation(IRB))
-    return V;
-
-  // Otherwise, assume the target links with compiler-rt, which provides a
-  // thread-local variable with a magic name.
-  Module &M = *F.getParent();
-  const char *UnsafeStackPtrVar = "__safestack_unsafe_stack_ptr";
-  auto UnsafeStackPtr =
-      dyn_cast_or_null<GlobalVariable>(M.getNamedValue(UnsafeStackPtrVar));
-
-  bool UseTLS = USPStorage == ThreadLocalUSP;
-
-  if (!UnsafeStackPtr) {
-    auto TLSModel = UseTLS ?
-        GlobalValue::InitialExecTLSModel :
-        GlobalValue::NotThreadLocal;
-    // The global variable is not defined yet, define it ourselves.
-    // We use the initial-exec TLS model because we do not support the
-    // variable living anywhere other than in the main executable.
-    UnsafeStackPtr = new GlobalVariable(
-        M, StackPtrTy, false, GlobalValue::ExternalLinkage, nullptr,
-        UnsafeStackPtrVar, nullptr, TLSModel);
-  } else {
-    // The variable exists, check its type and attributes.
-    if (UnsafeStackPtr->getValueType() != StackPtrTy)
-      report_fatal_error(Twine(UnsafeStackPtrVar) + " must have void* type");
-    if (UseTLS != UnsafeStackPtr->isThreadLocal())
-      report_fatal_error(Twine(UnsafeStackPtrVar) + " must " +
-                         (UseTLS ? "" : "not ") + "be thread-local");
-  }
-  return UnsafeStackPtr;
-}
-
 Value *SafeStack::getStackGuard(IRBuilder<> &IRB, Function &F) {
   Value *StackGuardVar = TL->getIRStackGuard(IRB);
   if (!StackGuardVar)
     StackGuardVar =
         F.getParent()->getOrInsertGlobal("__stack_chk_guard", StackPtrTy);
   return IRB.CreateLoad(StackGuardVar, "StackGuard");
 }
 
bool SafeStack::runOnFunction(Function &F) {
   if (!StaticAllocas.empty() || !DynamicAllocas.empty() ||
       !ByValArguments.empty())
     ++NumUnsafeStackFunctions; // This function has the unsafe stack.
 
   if (!StackRestorePoints.empty())
     ++NumUnsafeStackRestorePointsFunctions;
 
   IRBuilder<> IRB(&F.front(), F.begin()->getFirstInsertionPt());
-  UnsafeStackPtr = getOrCreateUnsafeStackPtr(IRB, F);
+  UnsafeStackPtr = TL->getSafeStackPointerLocation(IRB);
 
   // Load the current stack pointer (we'll also use it as a base pointer).
   // FIXME: use a dedicated register for it ?
   Instruction *BasePointer =
       IRB.CreateLoad(UnsafeStackPtr, false, "unsafe_stack_ptr");
   assert(BasePointer->getType() == StackPtrTy);
 
   AllocaInst *StackGuardSlot = nullptr;

llvm/trunk/lib/CodeGen/TargetLoweringBase.cpp

while (true) {
     if (MTy == LK.second)
       return std::make_pair(Cost, MTy.getSimpleVT());
 
     // Keep legalizing the type.
     MTy = LK.second;
   }
 }
 
+Value *TargetLoweringBase::getDefaultSafeStackPointerLocation(IRBuilder<> &IRB,
+                                                              bool UseTLS) const {
+  // compiler-rt provides a variable with a magic name.  Targets that do not
+  // link with compiler-rt may also provide such a variable.
+  Module *M = IRB.GetInsertBlock()->getParent()->getParent();
+  const char *UnsafeStackPtrVar = "__safestack_unsafe_stack_ptr";
+  auto UnsafeStackPtr =
+      dyn_cast_or_null<GlobalVariable>(M->getNamedValue(UnsafeStackPtrVar));
+
+  Type *StackPtrTy = Type::getInt8PtrTy(M->getContext());
+
+  if (!UnsafeStackPtr) {
+    auto TLSModel = UseTLS ?
+        GlobalValue::InitialExecTLSModel :
+        GlobalValue::NotThreadLocal;
+    // The global variable is not defined yet, define it ourselves.
+    // We use the initial-exec TLS model because we do not support the
+    // variable living anywhere other than in the main executable.
+    UnsafeStackPtr = new GlobalVariable(
+        *M, StackPtrTy, false, GlobalValue::ExternalLinkage, nullptr,
+        UnsafeStackPtrVar, nullptr, TLSModel);
+  } else {
+    // The variable exists, check its type and attributes.
+    if (UnsafeStackPtr->getValueType() != StackPtrTy)
+      report_fatal_error(Twine(UnsafeStackPtrVar) + " must have void* type");
+    if (UseTLS != UnsafeStackPtr->isThreadLocal())
+      report_fatal_error(Twine(UnsafeStackPtrVar) + " must " +
+                         (UseTLS ? "" : "not ") + "be thread-local");
+  }
+  return UnsafeStackPtr;
+}
+
 Value *TargetLoweringBase::getSafeStackPointerLocation(IRBuilder<> &IRB) const {
   if (!TM.getTargetTriple().isAndroid())
-    return nullptr;
+    return getDefaultSafeStackPointerLocation(IRB, true);
 
   // Android provides a libc function to retrieve the address of the current
   // thread's unsafe stack pointer.
   Module *M = IRB.GetInsertBlock()->getParent()->getParent();
   Type *StackPtrTy = Type::getInt8PtrTy(M->getContext());
   Value *Fn = M->getOrInsertFunction("__safestack_pointer_address",
                                      StackPtrTy->getPointerTo(0), nullptr);
   return IRB.CreateCall(Fn);

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

 Value *X86TargetLowering::getSSPStackGuardCheck(const Module &M) const {
   // MSVC CRT has a function to validate security cookie.
   if (Subtarget.getTargetTriple().isOSMSVCRT())
     return M.getFunction("__security_check_cookie");
   return TargetLowering::getSSPStackGuardCheck(M);
 }
 
 Value *X86TargetLowering::getSafeStackPointerLocation(IRBuilder<> &IRB) const {
+  if (Subtarget.getTargetTriple().isOSContiki())
+    return getDefaultSafeStackPointerLocation(IRB, false);
+
   if (!Subtarget.isTargetAndroid())
     return TargetLowering::getSafeStackPointerLocation(IRB);
 
   // Android provides a fixed TLS slot for the SafeStack pointer. See the
   // definition of TLS_SLOT_SAFESTACK in
   // https://android.googlesource.com/platform/bionic/+/master/libc/private/bionic_tls.h
   unsigned AddressSpace, Offset;
 

llvm/trunk/test/Transforms/SafeStack/X86/array.ll

 ; RUN: opt -safe-stack -S -mtriple=i386-pc-linux-gnu < %s -o - | FileCheck %s
-; RUN: opt -safe-stack -safe-stack-usp-storage=single-thread -S -mtriple=i386-pc-linux-gnu < %s -o - | FileCheck -check-prefix=SINGLE-THREAD %s
+; RUN: opt -safe-stack -S -mtriple=i386-pc-contiki-unknown < %s -o - | FileCheck -check-prefix=SINGLE-THREAD %s
 ; RUN: opt -safe-stack -S -mtriple=x86_64-pc-linux-gnu < %s -o - | FileCheck %s
-; RUN: opt -safe-stack -safe-stack-usp-storage=single-thread -S -mtriple=x86_64-pc-linux-gnu < %s -o - | FileCheck -check-prefix=SINGLE-THREAD %s
 
 ; array [4 x i8]
 ; Requires protector.
 
 ; CHECK: @__safestack_unsafe_stack_ptr = external thread_local(initialexec) global i8*
 ; SINGLE-THREAD: @__safestack_unsafe_stack_ptr = external global i8*
 
 define void @foo(i8* %a) nounwind uwtable safestack {