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

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

3732

Size should be capitalized.

3734

Space between the if and the (

lib/Frontend/CompilerInvocation.cpp

530–532

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

Please remove trailing whitespace here.

include/clang/Frontend/CodeGenOptions.def

141

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!

include/clang/Driver/CLCompatOptions.td

 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">,

 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">;

include/clang/Driver/Options.td

   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)">;
 

include/clang/Frontend/CodeGenOptions.def

                                      ///< .ctors.
 VALUE_CODEGENOPT(StackAlignment    , 32, 0) ///< Overrides default stack 
                                             ///< alignment, if not 0.
+CODEGENOPT(IsStackProbeSizeOverridden, 1, 0) ///< Controls whether to use the
+                                             ///< overridden or the default
+                                             ///< stack probe size.
+VALUE_CODEGENOPT(StackProbeSize    , 32, 0) ///< Overrides default stack 
+                                            ///< probe size, even if 0.
 CODEGENOPT(DebugColumnInfo, 1, 0) ///< Whether or not to use column information
                                   ///< in debug info.
 

lib/CodeGen/BackendUtil.cpp

   Options.UnsafeFPMath = CodeGenOpts.UnsafeFPMath;
   Options.UseSoftFloat = CodeGenOpts.SoftFloat;
   Options.StackAlignmentOverride = CodeGenOpts.StackAlignment;
+  Options.IsStackProbeSizeOverridden = CodeGenOpts.IsStackProbeSizeOverridden;
+  Options.StackProbeSizeOverride = CodeGenOpts.StackProbeSize;
   Options.DisableTailCalls = CodeGenOpts.DisableTailCalls;
   Options.TrapFuncName = CodeGenOpts.TrapFuncName;
   Options.ABIName = TargetOpts.ABI;

lib/Driver/Tools.cpp

     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");

lib/Frontend/CompilerInvocation.cpp

     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;
+    Opts.IsStackProbeSizeOverridden = true;
+  }
+  else
+  {
+    Opts.IsStackProbeSizeOverridden = false;
+  }
+
   if (Arg *A = Args.getLastArg(OPT_fobjc_dispatch_method_EQ)) {
     StringRef Name = A->getValue();
     unsigned Method = llvm::StringSwitch<unsigned>(Name)

test/Driver/cl-options.c

 // 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:     /Gm- \
 // RUN:     /Gr \
 // RUN:     /GS \
-// RUN:     /Gs1000 \
 // RUN:     /GT \
 // RUN:     /GX \
 // RUN:     /Gv \