Support for the mno-stack-arg-probe flag

[nruslan]

Summary

Adds support for this flag. There is also another piece for llvm (separate review). More info: https://bugs.llvm.org/show_bug.cgi?id=36221

Related: D43107

[nruslan]

@craig.topper, @MatzeB, @hans: Can someone take a look at this change?

[hans]

I see in the PR that matches a MinGW flag, but I'm curious to the motivation here. In what scenario would the user want to use this, i.e. how do they know it's safe to drop the probes?

lib/CodeGen/TargetInfo.cpp

2358

I'd suggest perhaps "addStackProbeTargetAttributes" as a name instead, since I'm not sure what Params is for.

2361

This could be written as

if (llvm::Function *Fn = dyn_cast_or_null<llvm::Function>(GV)) {

lib/Driver/ToolChains/Clang.cpp

4038

I think you can just do

Args.AddLastArg(CmdArgs, options::OPT_mno_stack_arg_probe)

which avoids the if-statement and having to spell out the flag.

[MatzeB]

• No test.
• What about -mstack-arg-probe, shouldn't we have that for consistency?
• I'd prefer not to review clang changes myself as I don't know that part of the code too well.

[nruslan]

@hans: One real-world example is when it is used to compile UEFI code using PE/COFF targets natively. Obviously, UEFI uses ABI which is basically almost the same as MS ABI, except that chkstk is not used. It mostly works (I actually was able to get it running) except the cases when the code contains variable-sized arrays allocated on stacks. Unfortunately, stack-probe-size will only help with fixed sized array but will not help to solve the problem described in the bug description since stack usage is unknown at compile time. MinGW does not have this problem because it provides this flag.

@MatzeB : there is a test on LLVM side (related review). Do you think the test is needed for clang side? If so, please let me know, what kind of test it is supposed to be.

[hans]

In D43108#1005904, @nruslan wrote:

@hans: One real-world example is when it is used to compile UEFI code using PE/COFF targets natively. Obviously, UEFI uses ABI which is basically almost the same as MS ABI, except that chkstk is not used. It mostly works (I actually was able to get it running) except the cases when the code contains variable-sized arrays allocated on stacks. Unfortunately, stack-probe-size will only help with fixed sized array but will not help to solve the problem described in the bug description since stack usage is unknown at compile time. MinGW does not have this problem because it provides this flag.

I see, interesting. Might be worth mentioning in the commit message for others wondering what the flag is useful for.

@MatzeB : there is a test on LLVM side (related review). Do you think the test is needed for clang side? If so, please let me know, what kind of test it is supposed to be.

Yes please, I think think there should be on in test/Driver/ to check that forwarding the flag to cc1 (and if we have a -mno-foo, there should maybe be a -mfoo variant too?), and a test in test/CodeGen/ to check that the attribute gets put on the functions correctly. Perhaps r321992 is a good example to look at.

[nruslan]

Added mstack-arg-probe, tests, and changes related to other comments of the reviewers

[nruslan]

lib/Driver/ToolChains/Clang.cpp

4038

I added mstack_arg_probe also, and this check now contains two options. I am not sure how AddLastArg behaves in this case (especially because only one attribute needs to be defined: mno-stack-arg-probe, and a positive case is the default). I found some other examples similar to this one, and they used hasArg with two options.

[nruslan]

@hans: I responded to the comments and also updated diff for clang with new changes.

[hans]

This looks good to me.

[nruslan]

@hans, @craig.topper, @MatzeB: Can someone commit D43108 (this) and D43107 on my behalf? I do not think, I have commit access.

[aemerson]

Can we clarify the meaning of this option a bit. The doc you've added here is saying that -mno-stack-arg-probe disables stack probes. Then what does -mstack-arg-probe mean specifically? Does it mean that only stack probes for ABI required reasons are enabled, or probes are done even in cases where the ABI doesn't require them? Either way, the doc needs to be clearer on the exact purpose.

I'm currently working on enabling stack probes for reasons other than ABI, and so if the answer is that this option is only concerned with ABI, we will need another option like -fstack-check.

[nruslan]

@aemerson : I am not 100% sure, but I think you are talking about different flag. This commit is for supporting a flag to disable stack probes (identical flag can be found in MinGW), it basically only applies to PE/COFF (Windows) targets. It can be useful to compile UEFI code. By default, Windows will use check probes of 4K in x86. A related stack-probe-size allows to vary the size of stack probes. This one completely disables them.

By default, stack probes are enabled (i.e., -mstack-arg-probe is the default behavior) and have the size of 4K in x86.

[aemerson]

In D43108#1023300, @nruslan wrote:

By default, stack probes are enabled (i.e., -mstack-arg-probe is the default behavior) and have the size of 4K in x86.

This part what I wanted to clarify, -mstack-probe-arg is enabling stack probes if the ABI requires it only, not for other reasons like security.

[nruslan]

@aemerson : yes, it is just part of MS ABI

docs/ClangCommandLineReference.rst

 .. option:: -mstack-alignment=<arg>
 
 Set the stack alignment
 
 .. option:: -mstack-probe-size=<arg>
 
 Set the stack probe size
 
+.. option:: -mstack-arg-probe, -mno-stack-arg-probe
+
+Disable stack probes
+
 .. option:: -mstackrealign, -mno-stackrealign
 
 Force realign the stack at entry to every function
 
 .. option:: -mthread-model <arg>
 
 The thread model to use, e.g. posix, single (posix by default)
 

include/clang/Driver/Options.td

 def mno_ms_bitfields : Flag<["-"], "mno-ms-bitfields">, Group<m_Group>,
   HelpText<"Do not 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 mstack_arg_probe : Flag<["-"], "mstack-arg-probe">, Group<m_Group>,
+  HelpText<"Enable stack probes">;
+def mno_stack_arg_probe : Flag<["-"], "mno-stack-arg-probe">, Group<m_Group>, Flags<[CC1Option]>,
+  HelpText<"Disable stack probes which are enabled by default">;
 def mthread_model : Separate<["-"], "mthread-model">, Group<m_Group>, Flags<[CC1Option]>,
   HelpText<"The thread model to use, e.g. posix, single (posix by default)">, Values<"posix,single">;
 def meabi : Separate<["-"], "meabi">, Group<m_Group>, Flags<[CC1Option]>,
   HelpText<"Set EABI type, e.g. 4, 5 or gnu (default depends on triple)">, Values<"default,4,5,gnu">;
 
 def mno_constant_cfstrings : Flag<["-"], "mno-constant-cfstrings">, Group<m_Group>;
 def mno_global_merge : Flag<["-"], "mno-global-merge">, Group<m_Group>, Flags<[CC1Option]>,
   HelpText<"Disable merging of globals">;

include/clang/Frontend/CodeGenOptions.def

 CODEGENOPT(StackRealignment  , 1, 0) ///< Control whether to force 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(NoStackArgProbe, 1, 0) ///< Set when -mno-stack-arg-probe is used
 CODEGENOPT(DebugColumnInfo, 1, 0) ///< Whether or not to use column information
                                   ///< in debug info.
 
 CODEGENOPT(DebugTypeExtRefs, 1, 0) ///< Whether or not debug info should contain
                                    ///< external references to a PCH or module.
 
 CODEGENOPT(DebugExplicitImport, 1, 0)  ///< Whether or not debug info should
                                        ///< contain explicit imports for

lib/CodeGen/TargetInfo.cpp

public:
 
   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 (D && isa<FunctionDecl>(D)) {
-    if (CGM.getCodeGenOpts().StackProbeSize != 4096) {
-      llvm::Function *Fn = cast<llvm::Function>(GV);
+static void addStackProbeTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
+  if (llvm::Function *Fn = dyn_cast_or_null<llvm::Function>(GV)) {
 
+    if (CGM.getCodeGenOpts().StackProbeSize != 4096)
       Fn->addFnAttr("stack-probe-size",
                     llvm::utostr(CGM.getCodeGenOpts().StackProbeSize));
-    }
+    if (CGM.getCodeGenOpts().NoStackArgProbe)
+      Fn->addFnAttr("no-stack-arg-probe");
   }
 }
 
 void WinX86_32TargetCodeGenInfo::setTargetAttributes(
     const Decl *D, llvm::GlobalValue *GV, CodeGen::CodeGenModule &CGM,
     ForDefinition_t IsForDefinition) const {
   X86_32TargetCodeGenInfo::setTargetAttributes(D, GV, CGM, IsForDefinition);
   if (!IsForDefinition)
     return;
-  addStackProbeSizeTargetAttribute(D, GV, CGM);
+  addStackProbeTargetAttributes(D, GV, CGM);
 }
 
 class WinX86_64TargetCodeGenInfo : public TargetCodeGenInfo {
 public:
   WinX86_64TargetCodeGenInfo(CodeGen::CodeGenTypes &CGT,
                              X86AVXABILevel AVXLevel)
       : TargetCodeGenInfo(new WinX86_64ABIInfo(CGT)) {}
 
if (FD->hasAttr<X86ForceAlignArgPointerAttr>()) {
       Fn->addAttributes(llvm::AttributeList::FunctionIndex, B);
     }
     if (FD->hasAttr<AnyX86InterruptAttr>()) {
       llvm::Function *Fn = cast<llvm::Function>(GV);
       Fn->setCallingConv(llvm::CallingConv::X86_INTR);
     }
   }
 
-  addStackProbeSizeTargetAttribute(D, GV, CGM);
+  addStackProbeTargetAttributes(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

 };
 
 void WindowsARMTargetCodeGenInfo::setTargetAttributes(
     const Decl *D, llvm::GlobalValue *GV, CodeGen::CodeGenModule &CGM,
     ForDefinition_t IsForDefinition) const {
   ARMTargetCodeGenInfo::setTargetAttributes(D, GV, CGM, IsForDefinition);
   if (!IsForDefinition)
     return;
-  addStackProbeSizeTargetAttribute(D, GV, CGM);
+  addStackProbeTargetAttributes(D, GV, CGM);
 }
 }
 
 void ARMABIInfo::computeInfo(CGFunctionInfo &FI) const {
   if (!getCXXABI().classifyReturnType(FI))
     FI.getReturnInfo() =
         classifyReturnType(FI.getReturnType(), FI.isVariadic());
 

lib/Driver/ToolChains/Clang.cpp

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 (!Args.hasFlag(options::OPT_mstack_arg_probe,
+                    options::OPT_mno_stack_arg_probe, true))
+    CmdArgs.push_back(Args.MakeArgString("-mno-stack-arg-probe"));
+
   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");
       CmdArgs.push_back("-arm-restrict-it");
     } else {
       CmdArgs.push_back("-backend-option");
       CmdArgs.push_back("-arm-no-restrict-it");

lib/Frontend/CompilerInvocation.cpp

static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
 
   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.NoStackArgProbe = Args.hasArg(OPT_mno_stack_arg_probe);
+
   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) {

test/CodeGen/stack-arg-probe.c

+// RUN: %clang_cc1 %s -triple=i686-windows-msvc -emit-llvm -o - -mno-stack-arg-probe | FileCheck %s -check-prefix=NO-STACKPROBE
+// RUN: %clang_cc1 %s -triple=i686-windows-msvc -emit-llvm -o - | FileCheck %s -check-prefix=STACKPROBE
+
+// NO-STACKPROBE: attributes #{{[0-9]+}} = {{{.*}} "no-stack-arg-probe"
+// STACKPROBE-NOT: attributes #{{[0-9]+}} = {{{.*}} "no-stack-arg-probe"
+
+void test1() {
+}

test/Driver/stack-arg-probe.c

+// RUN: %clang -### %s 2>&1 | FileCheck %s -check-prefix=STACKPROBE
+// RUN: %clang -### -mno-stack-arg-probe -mstack-arg-probe %s 2>&1 | FileCheck %s -check-prefix=STACKPROBE
+// RUN: %clang -### -mstack-arg-probe -mno-stack-arg-probe %s 2>&1 | FileCheck %s -check-prefix=NO-STACKPROBE
+// REQUIRES: clang-driver
+
+// NO-STACKPROBE: -mno-stack-arg-probe
+// STACKPROBE-NOT: -mno-stack-arg-probe