[X86] Add -mseparate-stack-seg

[mlemay-intel]

Summary

This patch adds the -mseparate-stack-seg option to enable restricting accesses to help prevent stack corruption.

[delena]

lib/CodeGen/TargetInfo.cpp

1569

Hi,
I'm not clang reviewer at all and you can ignore my comment.

Searching string looks strange for me. I suppose that this string should be defined in another form. Something like
options::OPT_separate_stack_seg.

1577

You should not use 258 as a constant. It should be defined somewhere as address space enum.

1578

This line alignment does not match LLVM style.

1580

Again, not sure that I'm right. You are trying to create addressspacecast. Is it the right way to create ptrtoint + inttoptr?

[mlemay-intel]

Thank you for your feedback!

lib/CodeGen/TargetInfo.cpp

1569

I haven't quite found an example of how an option like this should be handled. String comparisons of this form are performed in clang/lib/Basic/Targets.cpp, but there they are used to initialize variables. Furthermore, all of those tests are for CPU features, whereas this one is for a particular segment configuration.

1577

I agree with this principle. However, the address space numbers for FS and GS are listed in the Clang documentation [1] and are used as literals in LLVM code. Thus, I think this patch is consistent with existing usage of address space numbers.

[1] http://clang.llvm.org/docs/LanguageExtensions.html#memory-references-off-the-gs-segment

1578

I'll fix it.

1580

I first attempted to create an address space cast, but LLVM disallows that. I think it is necessary to perform the conversion through an int, but I am not entirely sure of that.

[qcolombet]

Hi Craig,

Could you have a look? I am not confident enough with clang to give more feedback than Elena.

Cheers,
-Quentin

[mlemay-intel]

Revise patch to fix line alignment.

[mkuper]

I really don't know enough about this part of clang either.

[mlemay-intel]

Removed the portions that are specific to 32-bit segmentation. I plan to resubmit those later as a separate patch.

include/clang/Driver/Options.td

 def mno_adx : Flag<["-"], "mno-adx">, Group<m_x86_Features_Group>;
 def mno_sha : Flag<["-"], "mno-sha">, Group<m_x86_Features_Group>;
 def mno_fxsr : Flag<["-"], "mno-fxsr">, Group<m_x86_Features_Group>;
 def mno_xsave : Flag<["-"], "mno-xsave">, Group<m_x86_Features_Group>;
 def mno_xsaveopt : Flag<["-"], "mno-xsaveopt">, Group<m_x86_Features_Group>;
 def mno_xsavec : Flag<["-"], "mno-xsavec">, Group<m_x86_Features_Group>;
 def mno_xsaves : Flag<["-"], "mno-xsaves">, Group<m_x86_Features_Group>;
 def mno_pku : Flag<["-"], "mno-pku">, Group<m_x86_Features_Group>;
+def mseparate_stack_seg : Flag<["-"], "mseparate-stack-seg">, Group<m_x86_Features_Group>;
 
 def munaligned_access : Flag<["-"], "munaligned-access">, Group<m_arm_Features_Group>,
   HelpText<"Allow memory accesses to be unaligned (AArch32/AArch64 only)">;
 def mno_unaligned_access : Flag<["-"], "mno-unaligned-access">, Group<m_arm_Features_Group>,
   HelpText<"Force all memory accesses to be aligned (AArch32/AArch64 only)">;
 def mstrict_align : Flag<["-"], "mstrict-align">, Alias<mno_unaligned_access>, Flags<[CC1Option,HelpHidden]>,
   HelpText<"Force all memory accesses to be aligned (same as mno-unaligned-access)">;
 def mno_thumb : Flag<["-"], "mno-thumb">, Group<m_arm_Features_Group>;

lib/CodeGen/TargetInfo.cpp

Address X86_32ABIInfo::EmitVAArg(CodeGenFunction &CGF,
 
   // x86-32 changes the alignment of certain arguments on the stack.
   //
   // Just messing with TypeInfo like this works because we never pass
   // anything indirectly.
   TypeInfo.second = CharUnits::fromQuantity(
                 getTypeStackAlignInBytes(Ty, TypeInfo.second.getQuantity()));
 
-  return emitVoidPtrVAArg(CGF, VAListAddr, Ty, /*Indirect*/ false,
+  const Address Addr = emitVoidPtrVAArg(CGF, VAListAddr, Ty, /*Indirect*/ false,
                                         TypeInfo, CharUnits::fromQuantity(4),
                                         /*AllowHigherAlign*/ true);
+
+  const std::vector<std::string> &TargetFeatures =
+    CGF.getTarget().getTargetOpts().Features;
+  if (std::find(TargetFeatures.begin(), TargetFeatures.end(),
+                "+separate-stack-seg") != TargetFeatures.end()) {
+    // Cast the pointer into the address space for the stack segment.
+    // This is to help support multi-segment memory models in which DS and SS
+    // may differ from each other.
+    llvm::Type *DirectTy = CGF.ConvertTypeForMem(Ty);
+    llvm::Value *PtrAsInt =
+      CGF.Builder.CreatePtrToInt(Addr.getPointer(), CGF.IntPtrTy);
+    llvm::Value *PtrInStackSeg = CGF.Builder.CreateIntToPtr(PtrAsInt,
+                                                   DirectTy->getPointerTo(258));
+    return Address(PtrInStackSeg, Addr.getAlignment());
+  }
+
+  return Addr;
 }
 
 bool X86_32TargetCodeGenInfo::isStructReturnInRegABI(
     const llvm::Triple &Triple, const CodeGenOptions &Opts) {
   assert(Triple.getArch() == llvm::Triple::x86);
 
   switch (Opts.getStructReturnConvention()) {
   case CodeGenOptions::SRCK_Default:

test/CodeGen/varargs.c

 // RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple i386-unknown-unknown -target-feature +separate-stack-seg -emit-llvm -o - %s | FileCheck -check-prefix=SEPARATE-SS %s
 
 // PR6433 - Don't crash on va_arg(typedef).
 typedef double gdouble;
 void focus_changed_cb () {
     __builtin_va_list pa;
     double mfloat;
     mfloat = __builtin_va_arg((pa), gdouble);
 }
 
 void vararg(int, ...);
 void function_as_vararg() {
   // CHECK: define {{.*}}function_as_vararg
   // CHECK-NOT: llvm.trap
   vararg(0, focus_changed_cb);
 }
 
 void vla(int n, ...)
 {
   __builtin_va_list ap;
   void *p;
   p = __builtin_va_arg(ap, typeof (int (*)[++n])); // CHECK: add nsw i32 {{.*}}, 1
+  // SEPARATE-SS: load i32*, i32* addrspace(258)* {{.*}}
 }