[Driver] Add -fno-split-stack

[ChuanqiXu]

Summary

I found that clang has -fsplit-stack but doesn't support -fno-split-stack. So I think it is natural to support it.

[jansvoboda11]

Adding the negative option to the driver makes sense to me. I think we could also simplify the option definitions, see my inline comment.

clang/include/clang/Driver/Options.td

2381

Typo.

I think it would also make sense to merge the option definitions for driver (fsplit_stack and fno_split_stack) and CC1 (split_stacks) into one. (We can rename the CC1 option from -split-stacks to -fsplit-stack.)

Other options already do this via BoolFOption, for example signed_char above. If you are not sure about how that works, take a look here.

[ChuanqiXu]

clang/include/clang/Driver/Options.td

2381

Thanks for reviewing.

For the renaming part, I worry about if it involves problems for compatibility.

If we mark fsplit_stack as BoolFOption, the user seems couldn't specify the opposite value by -fno-split-stack which seems to be one traditional usage style.

[ChuanqiXu]

Edit as suggestion.

[jansvoboda11]

clang/include/clang/Driver/Options.td

2381

Why not? BoolFOption would create both -fsplit-stack and -fno-split-stack in the driver. Marking the former with CC1Option would make it available to CC1 as well and we get the same result as with your current changes (but unifying the wording and tablegen definitions).

[ChuanqiXu]

clang/include/clang/Driver/Options.td

2381

Thanks for clarifying. I am not clear about the driver part. And I worry about if it is a problem if we rename the CC1 option from -split-stacks to -fsplit-stack.

[jansvoboda11]

clang/include/clang/Driver/Options.td

2381

What are you not clear about on the driver part?

Renaming CC1 options is fine, as they are only intended to be used by Clang developers and we don't need to keep them backward compatible. Fixing potential build failures should be all there is to it.

[ChuanqiXu]

Merge -fsplit-stack and -fno-split-stack into one in options.td. And rename -split-stacks into -fsplit-stack.

[ChuanqiXu]

clang/include/clang/Driver/Options.td

2381

Thanks for clarifying!

[MaskRay]

If we want to improve test coverage, we can test -fno-split-stack -fsplit-stack passes -fsplit-stack. But doing this for every option may be excessive...
I have filed the libgcc morestack.S .init_array.0 feature request: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99759

clang/include/clang/Driver/Options.td

2383

s/Would use/Use/

[ChuanqiXu]

In D99245#2649645, @MaskRay wrote:

If we want to improve test coverage, we can test -fno-split-stack -fsplit-stack passes -fsplit-stack. But doing this for every option may be excessive...
I have filed the libgcc morestack.S .init_array.0 feature request: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99759

Thanks for reviewing! I would try to submit a patch to gcc if possible.

clang/include/clang/Driver/Options.td

defm spell_checking : BoolFOption<"spell-checking",
   LangOpts<"SpellChecking">, DefaultTrue,
   NegFlag<SetFalse, [CC1Option], "Disable spell-checking">, PosFlag<SetTrue>>;
 def fspell_checking_limit_EQ : Joined<["-"], "fspell-checking-limit=">, Group<f_Group>;
 def fsigned_bitfields : Flag<["-"], "fsigned-bitfields">, Group<f_Group>;
 defm signed_char : BoolFOption<"signed-char",
   LangOpts<"CharIsSigned">, DefaultTrue,
   NegFlag<SetFalse, [CC1Option], "char is unsigned">, PosFlag<SetTrue, [], "char is signed">>,
   ShouldParseIf<!strconcat("!", open_cl.KeyPath)>;
-def fsplit_stack : Flag<["-"], "fsplit-stack">, Group<f_Group>;
+defm split_stack : BoolFOption<"split-stack",
+  CodeGenOpts<"EnableSegmentedStacks">, DefaultFalse,
+  NegFlag<SetFalse, [], "Wouldn't use segmented stack">, 
+  PosFlag<SetTrue, [CC1Option], "Use segmented stack">>;
 def fstack_protector_all : Flag<["-"], "fstack-protector-all">, Group<f_Group>,
   HelpText<"Enable stack protectors for all functions">;
 defm stack_clash_protection : BoolFOption<"stack-clash-protection",
   CodeGenOpts<"StackClashProtector">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Enable">, NegFlag<SetFalse, [], "Disable">,
   BothFlags<[], " stack clash protection">>;
 def fstack_protector_strong : Flag<["-"], "fstack-protector-strong">, Group<f_Group>,
   HelpText<"Enable stack protectors for some functions vulnerable to stack smashing. "

 def mfloat_abi : Separate<["-"], "mfloat-abi">,
   HelpText<"The float ABI to use">,
   MarshallingInfoString<CodeGenOpts<"FloatABI">>;
 def mtp : Separate<["-"], "mtp">,
   HelpText<"Mode for reading thread pointer">;
 def mlimit_float_precision : Separate<["-"], "mlimit-float-precision">,
   HelpText<"Limit float precision to the given value">,
   MarshallingInfoString<CodeGenOpts<"LimitFloatPrecision">>;
-def split_stacks : Flag<["-"], "split-stacks">,
-  HelpText<"Try to use a split stack if possible.">,
-  MarshallingInfoFlag<CodeGenOpts<"EnableSegmentedStacks">>;
 def mregparm : Separate<["-"], "mregparm">,
   HelpText<"Limit the number of registers available for integer arguments">,
   MarshallingInfoInt<CodeGenOpts<"NumRegisterParameters">>;
 def msmall_data_limit : Separate<["-"], "msmall-data-limit">,
   HelpText<"Put global and static data smaller than the limit into a special section">,
   MarshallingInfoInt<CodeGenOpts<"SmallDataLimit">>;
 def munwind_tables : Flag<["-"], "munwind-tables">,
   HelpText<"Generate unwinding tables for all functions">,

clang/lib/Driver/ToolChains/Clang.cpp

#endif
 
   Args.AddLastArg(CmdArgs, options::OPT_ffine_grained_bitfield_accesses,
                   options::OPT_fno_fine_grained_bitfield_accesses);
 
   Args.AddLastArg(CmdArgs, options::OPT_fexperimental_relative_cxx_abi_vtables,
                   options::OPT_fno_experimental_relative_cxx_abi_vtables);
 
   // Handle segmented stacks.
-  if (Args.hasArg(options::OPT_fsplit_stack))
-    CmdArgs.push_back("-split-stacks");
+  if (Args.hasFlag(options::OPT_fsplit_stack, options::OPT_fno_split_stack,
+                   false))
+    CmdArgs.push_back("-fsplit-stack");
 
   RenderFloatingPointOptions(TC, D, OFastEnabled, Args, CmdArgs, JA);
 
   if (Arg *A = Args.getLastArg(options::OPT_mdouble_EQ)) {
     if (TC.getArch() == llvm::Triple::avr)
       A->render(Args, CmdArgs);
     else
       D.Diag(diag::err_drv_unsupported_opt_for_target)

clang/test/Driver/clang_f_opts.c

 // REQUIRES: clang-driver
 
 // RUN: %clang -### -S -fasm -fblocks -fbuiltin -fno-math-errno -fcommon -fpascal-strings -fno-blocks -fno-builtin -fmath-errno -fno-common -fno-pascal-strings -fblocks -fbuiltin -fmath-errno -fcommon -fpascal-strings -fsplit-stack %s 2>&1 | FileCheck -check-prefix=CHECK-OPTIONS1 %s
 // RUN: %clang -### -S -fasm -fblocks -fbuiltin -fno-math-errno -fcommon -fpascal-strings -fno-asm -fno-blocks -fno-builtin -fmath-errno -fno-common -fno-pascal-strings -fno-show-source-location -fshort-enums %s 2>&1 | FileCheck -check-prefix=CHECK-OPTIONS2 %s
 
-// CHECK-OPTIONS1: -split-stacks
+// CHECK-OPTIONS1: -fsplit-stack
 // CHECK-OPTIONS1: -fgnu-keywords
 // CHECK-OPTIONS1: -fblocks
 // CHECK-OPTIONS1: -fpascal-strings
 
 // CHECK-OPTIONS2: -fmath-errno
 // CHECK-OPTIONS2: -fno-gnu-keywords
 // CHECK-OPTIONS2: -fno-builtin
 // CHECK-OPTIONS2: -fshort-enums