Implement command line options for stack probe space

[AndrewH]

Summary

This code adds the -mstack-probe-size command line option and implements the /Gs compiler switch for clang-cl.

This should fix http://llvm.org/bugs/show_bug.cgi?id=21896

[AndrewH]

Like the bug this bug depends on: I took the liberty to add some reviewers based on the commit history. Is anything blocking this? I would be happy to help.

[hans]

Thanks! I think this looks good. Please also add a test for the /Gs alias in test/Driver/cl-options.c, and remove it from the "Unsupported but parsed" section.

[AndrewH]

Added some management code to make the special case -mstack-probe-size=0 work. Current behavior is that when -mstack-probe-size is omitted, the X86 target will choose the default stack probe size (which is currently 4096 bytes, but I did not want to expose such internal technicalities to the driver). When -mstack-probe-size=n is used, the X86 target will use a stack probing size of n bytes. Note that in contrast to the stack alignment parameter, n=0 is a valid value for the stack probing size.

Added the test cases to cl-options.c.

[AndrewH]

Update revision due to changes in dependency D6684.

[AndrewH]

Again: Update revision due to changes in dependency D6684.

[AndrewH]

Fixed handling in case of x86_64-pc-windows-msvc target.

[AndrewH]

Again: Update revision due to changes in dependency D6685.

[majnemer]

include/clang/Frontend/CodeGenOptions.def

144–145 ↗

(On Diff #17852)

Why not just make StackProbeSize default to 4096 and get rid of IsStackProbeSizeOverridden. Then you can get rid of IsStackProbeSizeOverridden.

After all that, only add the "stack-probe-size" attribute to the function if the StackProbeSize is not equal to 4096.

lib/CodeGen/TargetInfo.cpp

1637 ↗

(On Diff #17852)

This should start with a lower case letter and be marked as static.

1638–1640 ↗

(On Diff #17852)

This doesn't look correctly formatted for the coding style.

1644 ↗

(On Diff #17852)

This looks unused.

1646 ↗

(On Diff #17852)

I don't think we can use std::to_string. IIRC, some platforms don't have it yet.

Please use llvm::utostr instead. It is in "llvm/ADT/StringExtras.h"

1654 ↗

(On Diff #17852)

Please do the same for the X86_64 side so nothing mysterious happens if somebody adds an implementation of SetTargetAttributes.

lib/Driver/Tools.cpp

3723 ↗

(On Diff #17852)

Size should be capitalized.

3725 ↗

(On Diff #17852)

Space between the if and the (

lib/Frontend/CompilerInvocation.cpp

518–520 ↗

(On Diff #17852)

Braces should be right next to the else.

[AndrewH]

Why not just make StackProbeSize default to 4096 and get rid of IsStackProbeSizeOverridden. Then you can get rid of IsStackProbeSizeOverridden.
After all that, only add the "stack-probe-size" attribute to the function if the StackProbeSize is not equal to 4096.

I could see the problem that this would expose a (somewhat) magic constant to the frontend, whereas only the backend knows what sane default size should be used. StackAlignment has the same problem, but it can use 0 to indicate the usage of the default value, thus it does not need an Is...Overridden code gen option.

So the question is whether we should duplicate that magic value of 4096 for both frontend and backend. You can make that decision; I have no preference and I am very pragmatic on that issue.

Please do the same for the X86_64 side so nothing mysterious happens if somebody adds an implementation of SetTargetAttributes.

Please be aware that WinX86_32TargetCodeGenInfo inherits from X86_32TargetCodeGenInfo (which in turn inherits from TargetCodeGenInfo), whereas WinX86_64TargetCodeGenInfo directly inherits from TargetCodeGenInfo. Thus I have added a call to TargetCodeGenInfo::SetTargetAttributes.

[AndrewH]

Ping.

[AndrewH]

And this is the version without IsStackProbeSizeOverridden but with 4096 as a somewhat magic number in clang.

[AndrewH]

The two revisions that can be considered for submission are:
Diff 17925: No 4096, but with IsStackProbeSizeOverridden.
Diff 18254: With 4096, but no IsStackProbeSizeOverridden.

Could please some look over these revisions and decide which one could be applied to the trunk?

[majnemer]

LGTM with nits fixed.

include/clang/Driver/CLCompatOptions.td

65 ↗

(On Diff #18254)

Please remove trailing whitespace here.

include/clang/Frontend/CodeGenOptions.def

141 ↗

(On Diff #18254)

Please remove trailing whitespace here.

lib/CodeGen/TargetInfo.cpp

1639 ↗

(On Diff #18254)

You don't actually use the result of the dyn_cast, please use isa instead.

[hans]

lgtm too

[AndrewH]

Fixed.

[AndrewH]

To majnemer or hans: Could you push the last revision to trunk? I do not have any commit rights. Thanks!

cfe/trunk/include/clang/Driver/CLCompatOptions.td

 def _SLASH_c : CLFlag<"c">, HelpText<"Compile only">, Alias<c>;
 def _SLASH_D : CLJoinedOrSeparate<"D">, HelpText<"Define macro">,
   MetaVarName<"<macro[=value]>">, Alias<D>;
 def _SLASH_E : CLFlag<"E">, HelpText<"Preprocess to stdout">, Alias<E>;
 def _SLASH_GR : CLFlag<"GR">, HelpText<"Enable emission of RTTI data">;
 def _SLASH_GR_ : CLFlag<"GR-">, HelpText<"Disable emission of RTTI data">;
 def _SLASH_GF_ : CLFlag<"GF-">, HelpText<"Disable string pooling">,
   Alias<fwritable_strings>;
+def _SLASH_Gs : CLJoined<"Gs">, HelpText<"Set stack probe size">,
+  Alias<mstack_probe_size>;
 def _SLASH_Gy : CLFlag<"Gy">, HelpText<"Put each function in its own section">,
   Alias<ffunction_sections>;
 def _SLASH_Gy_ : CLFlag<"Gy-">, HelpText<"Don't put each function in its own section">,
   Alias<fno_function_sections>;
 def _SLASH_Gw : CLFlag<"Gw">, HelpText<"Put each data item in its own section">,
   Alias<fdata_sections>;
 def _SLASH_Gw_ : CLFlag<"Gw-">, HelpText<"Don't put each data item in its own section">,
   Alias<fno_data_sections>;

 def _SLASH_Gh : CLFlag<"Gh">;
 def _SLASH_GH : CLFlag<"GH">;
 def _SLASH_GL : CLFlag<"GL">;
 def _SLASH_GL_ : CLFlag<"GL-">;
 def _SLASH_Gm : CLFlag<"Gm">;
 def _SLASH_Gm_ : CLFlag<"Gm-">;
 def _SLASH_Gr : CLFlag<"Gr">;
 def _SLASH_GS : CLFlag<"GS">;
-def _SLASH_Gs : CLJoined<"Gs">;
 def _SLASH_GT : CLFlag<"GT">;
 def _SLASH_GX : CLFlag<"GX">;
 def _SLASH_Gv : CLFlag<"Gv">;
 def _SLASH_Gz : CLFlag<"Gz">;
 def _SLASH_GZ : CLFlag<"GZ">;
 def _SLASH_H : CLFlag<"H">;
 def _SLASH_homeparams : CLFlag<"homeparams">;
 def _SLASH_hotpatch : CLFlag<"hotpatch">;

cfe/trunk/include/clang/Driver/Options.td

 def mmacosx_version_min_EQ : Joined<["-"], "mmacosx-version-min=">,
   Group<m_Group>, HelpText<"Set Mac OS X deployment target">;
 def mms_bitfields : Flag<["-"], "mms-bitfields">, Group<m_Group>, Flags<[CC1Option]>,
   HelpText<"Set the default structure layout to be compatible with the Microsoft compiler standard">;
 def mstackrealign : Flag<["-"], "mstackrealign">, Group<m_Group>, Flags<[CC1Option]>,
   HelpText<"Force realign the stack at entry to every function">;
 def mstack_alignment : Joined<["-"], "mstack-alignment=">, Group<m_Group>, Flags<[CC1Option]>,
   HelpText<"Set the stack alignment">;
+def mstack_probe_size : Joined<["-"], "mstack-probe-size=">, Group<m_Group>, Flags<[CC1Option]>,
+  HelpText<"Set the stack probe size">;
 def mthread_model : Separate<["-"], "mthread-model">, Group<m_Group>, Flags<[CC1Option]>,
   HelpText<"The thread model to use, e.g. posix, single (posix by default)">;
 
 def mmmx : Flag<["-"], "mmmx">, Group<m_x86_Features_Group>;
 def mno_3dnowa : Flag<["-"], "mno-3dnowa">, Group<m_x86_Features_Group>;
 def mno_3dnow : Flag<["-"], "mno-3dnow">, Group<m_x86_Features_Group>;
 def mno_constant_cfstrings : Flag<["-"], "mno-constant-cfstrings">, Group<m_Group>;
 def mno_global_merge : Flag<["-"], "mno-global-merge">, Group<m_Group>, Flags<[CC1Option]>,

cfe/trunk/include/clang/Frontend/CodeGenOptions.def

CODEGENOPT(VerifyModule      , 1, 1) ///< Control whether the module should be run
                                      ///< through the LLVM Verifier.
 
 CODEGENOPT(StackRealignment  , 1, 0) ///< Control whether to permit stack
                                      ///< realignment.
 CODEGENOPT(UseInitArray      , 1, 0) ///< Control whether to use .init_array or
                                      ///< .ctors.
 VALUE_CODEGENOPT(StackAlignment    , 32, 0) ///< Overrides default stack 
                                             ///< alignment, if not 0.
+VALUE_CODEGENOPT(StackProbeSize    , 32, 4096) ///< Overrides default stack
+                                               ///< probe size, even if 0.
 CODEGENOPT(DebugColumnInfo, 1, 0) ///< Whether or not to use column information
                                   ///< in debug info.
 
 /// The user specified number of registers to be used for integral arguments,
 /// or 0 if unspecified.
 VALUE_CODEGENOPT(NumRegisterParameters, 32, 0)
 
 /// The lower bound for a buffer to be considered for stack protection.

cfe/trunk/lib/CodeGen/TargetInfo.cpp

 }
 
 class WinX86_32TargetCodeGenInfo : public X86_32TargetCodeGenInfo {
 public:
   WinX86_32TargetCodeGenInfo(CodeGen::CodeGenTypes &CGT,
         bool d, bool p, bool w, unsigned RegParms)
     : X86_32TargetCodeGenInfo(CGT, d, p, w, RegParms) {}
 
+  void SetTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
+                           CodeGen::CodeGenModule &CGM) const override;
+
   void getDependentLibraryOption(llvm::StringRef Lib,
                                  llvm::SmallString<24> &Opt) const override {
     Opt = "/DEFAULTLIB:";
     Opt += qualifyWindowsLibrary(Lib);
   }
 
   void getDetectMismatchOption(llvm::StringRef Name,
                                llvm::StringRef Value,
                                llvm::SmallString<32> &Opt) const override {
     Opt = "/FAILIFMISMATCH:\"" + Name.str() + "=" + Value.str() + "\"";
   }
 };
 
+static void addStackProbeSizeTargetAttribute(const Decl *D,
+                                             llvm::GlobalValue *GV,
+                                             CodeGen::CodeGenModule &CGM) {
+  if (isa<FunctionDecl>(D)) {
+    if (CGM.getCodeGenOpts().StackProbeSize != 4096) {
+      llvm::Function *Fn = cast<llvm::Function>(GV);
+
+      Fn->addFnAttr("stack-probe-size", llvm::utostr(CGM.getCodeGenOpts().StackProbeSize));
+    }
+  }
+}
+
+void WinX86_32TargetCodeGenInfo::SetTargetAttributes(const Decl *D,
+                                                     llvm::GlobalValue *GV,
+                                            CodeGen::CodeGenModule &CGM) const {
+  X86_32TargetCodeGenInfo::SetTargetAttributes(D, GV, CGM);
+
+  addStackProbeSizeTargetAttribute(D, GV, CGM);
+}
+
 class WinX86_64TargetCodeGenInfo : public TargetCodeGenInfo {
   bool HasAVX;
 public:
   WinX86_64TargetCodeGenInfo(CodeGen::CodeGenTypes &CGT, bool HasAVX)
     : TargetCodeGenInfo(new WinX86_64ABIInfo(CGT)), HasAVX(HasAVX) {}
 
+  void SetTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
+                           CodeGen::CodeGenModule &CGM) const override;
+
   int getDwarfEHStackPointer(CodeGen::CodeGenModule &CGM) const override {
     return 7;
   }
 
   bool initDwarfEHRegSizeTable(CodeGen::CodeGenFunction &CGF,
                                llvm::Value *Address) const override {
     llvm::Value *Eight8 = llvm::ConstantInt::get(CGF.Int8Ty, 8);
 
void getDetectMismatchOption(llvm::StringRef Name,
     Opt = "/FAILIFMISMATCH:\"" + Name.str() + "=" + Value.str() + "\"";
   }
 
   unsigned getOpenMPSimdDefaultAlignment(QualType) const override {
     return HasAVX ? 32 : 16;
   }
 };
 
+void WinX86_64TargetCodeGenInfo::SetTargetAttributes(const Decl *D,
+                                                     llvm::GlobalValue *GV,
+                                            CodeGen::CodeGenModule &CGM) const {
+  TargetCodeGenInfo::SetTargetAttributes(D, GV, CGM);
+
+  addStackProbeSizeTargetAttribute(D, GV, CGM);
+}
 }
 
 void X86_64ABIInfo::postMerge(unsigned AggregateSize, Class &Lo,
                               Class &Hi) const {
   // AMD64-ABI 3.2.3p2: Rule 5. Then a post merger cleanup is done:
   //
   // (a) If one of the classes is Memory, the whole argument is passed in
   //     memory.

cfe/trunk/lib/Driver/Tools.cpp

if (!Args.hasFlag(options::OPT_mno_stackrealign, options::OPT_mstackrealign,
     CmdArgs.push_back(Args.MakeArgString("-mstackrealign"));
   }
 
   if (Args.hasArg(options::OPT_mstack_alignment)) {
     StringRef alignment = Args.getLastArgValue(options::OPT_mstack_alignment);
     CmdArgs.push_back(Args.MakeArgString("-mstack-alignment=" + alignment));
   }
 
+  if (Args.hasArg(options::OPT_mstack_probe_size)) {
+    StringRef Size = Args.getLastArgValue(options::OPT_mstack_probe_size);
+
+    if (!Size.empty())
+      CmdArgs.push_back(Args.MakeArgString("-mstack-probe-size=" + Size));
+    else
+      CmdArgs.push_back("-mstack-probe-size=0");
+  }
+
   if (getToolChain().getTriple().getArch() == llvm::Triple::aarch64 ||
       getToolChain().getTriple().getArch() == llvm::Triple::aarch64_be)
     CmdArgs.push_back("-fallow-half-arguments-and-returns");
 
   if (Arg *A = Args.getLastArg(options::OPT_mrestrict_it,
                                options::OPT_mno_restrict_it)) {
     if (A->getOption().matches(options::OPT_mrestrict_it)) {
       CmdArgs.push_back("-backend-option");

cfe/trunk/lib/Frontend/CompilerInvocation.cpp

static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
   Opts.StackRealignment = Args.hasArg(OPT_mstackrealign);
   if (Arg *A = Args.getLastArg(OPT_mstack_alignment)) {
     StringRef Val = A->getValue();
     unsigned StackAlignment = Opts.StackAlignment;
     Val.getAsInteger(10, StackAlignment);
     Opts.StackAlignment = StackAlignment;
   }
 
+  if (Arg *A = Args.getLastArg(OPT_mstack_probe_size)) {
+    StringRef Val = A->getValue();
+    unsigned StackProbeSize = Opts.StackProbeSize;
+    Val.getAsInteger(0, StackProbeSize);
+    Opts.StackProbeSize = StackProbeSize;
+  }
+
   if (Arg *A = Args.getLastArg(OPT_fobjc_dispatch_method_EQ)) {
     StringRef Name = A->getValue();
     unsigned Method = llvm::StringSwitch<unsigned>(Name)
       .Case("legacy", CodeGenOptions::Legacy)
       .Case("non-legacy", CodeGenOptions::NonLegacy)
       .Case("mixed", CodeGenOptions::Mixed)
       .Default(~0U);
     if (Method == ~0U) {

cfe/trunk/test/Driver/cl-options.c

 // GR_: -fno-rtti
 
 // RUN: %clang_cl /Gy -### -- %s 2>&1 | FileCheck -check-prefix=Gy %s
 // Gy: -ffunction-sections
 
 // RUN: %clang_cl /Gy /Gy- -### -- %s 2>&1 | FileCheck -check-prefix=Gy_ %s
 // Gy_-NOT: -ffunction-sections
 
+// RUN: %clang_cl /Gs -### -- %s 2>&1 | FileCheck -check-prefix=Gs %s
+// Gs: "-mstack-probe-size=0"
+// RUN: %clang_cl /Gs0 -### -- %s 2>&1 | FileCheck -check-prefix=Gs0 %s
+// Gs0: "-mstack-probe-size=0"
+// RUN: %clang_cl /Gs4096 -### -- %s 2>&1 | FileCheck -check-prefix=Gs4096 %s
+// Gs4096: "-mstack-probe-size=4096"
+
 // RUN: %clang_cl /Gw -### -- %s 2>&1 | FileCheck -check-prefix=Gw %s
 // Gw: -fdata-sections
 
 // RUN: %clang_cl /Gw /Gw- -### -- %s 2>&1 | FileCheck -check-prefix=Gw_ %s
 // Gw_-NOT: -fdata-sections
 
 // RUN: %clang_cl /Imyincludedir -### -- %s 2>&1 | FileCheck -check-prefix=SLASH_I %s
 // RUN: %clang_cl /I myincludedir -### -- %s 2>&1 | FileCheck -check-prefix=SLASH_I %s

 // RUN:     /Gh \
 // RUN:     /GH \
 // RUN:     /GL \
 // RUN:     /GL- \
 // RUN:     /Gm \
 // RUN:     /Gm- \
 // RUN:     /Gr \
 // RUN:     /GS \
-// RUN:     /Gs1000 \
 // RUN:     /GT \
 // RUN:     /GX \
 // RUN:     /Gv \
 // RUN:     /Gz \
 // RUN:     /GZ \
 // RUN:     /H \
 // RUN:     /homeparams \
 // RUN:     /hotpatch \