-fstack-clash-protection: Return an actual error when used on unsupported OS

[sylvestre.ledru]

Summary

$ clang-12: error: -fstack-clash-protection is not supported on Windows or Mac OS X

[sylvestre.ledru]

clang-format

[kristof.beyls]

clang/include/clang/Basic/DiagnosticDriverKinds.td

276–277

There are more OSes than Linux, Windows or OSX.
Maybe it's somewhat better to say "-fstack-clash-protection is not supported on %0", with the targeted OS being fed in.
If that is not easily possible, maybe just say "-fstack-clash-protection is not supported on the targeted OS"?

[serge-sans-paille]

clang/include/clang/Basic/DiagnosticDriverKinds.td

276–277

Maybe it's somewhat better to say "-fstack-clash-protection is not supported on %0", with the targeted OS being fed in.

I second that one.

clang/lib/Driver/ToolChains/Clang.cpp

3074–3076

In that case you should probably allow explicitly the different Linux flavors here

[sylvestre.ledru]

Fix the error message

[emaste]

Can we add a test that the feature can be enabled on an OS other than Linux / Windows / Darwin?

[sylvestre.ledru]

extend the test (thanks serge)

[rnk]

Windows has effectively always had stack clash protection: we've always emitted those little chkstk probe calls for stack frames larger than a page. Would it make more sense to ignore this flag on Windows, since it opts into always-on behavior? If so, this doesn't seem like the right place to ignore it.

I see your comment here, but I don't really understand it:
https://reviews.llvm.org/D92100#2422729
What goes wrong on Windows? Can it be made to just work instead? It should be simple.

How do things go wrong on Darwin? I was under the impression that this was implemented in LLVM as strictly inline code, no runtime support required.

[emaste]

How do things go wrong on Darwin? I was under the impression that this was implemented in LLVM as strictly inline code, no runtime support required.

That is my impression as well (although it seems that an earlier version might have emitted calls to a stack probe routine?)

[serge-sans-paille]

In D92245#2425817, @emaste wrote:

How do things go wrong on Darwin? I was under the impression that this was implemented in LLVM as strictly inline code, no runtime support required.

That is my impression as well (although it seems that an earlier version might have emitted calls to a stack probe routine?)

The original security advisory doesn't mention Darwin, but there's nothing specific to Darwin in the stack clash protection implementation, so I'm fine with allowing it for Darwin too.

[sylvestre.ledru]

@serge-sans-paille ok to push this? :)
merci

[serge-sans-paille]

@sylvestre.ledru I: double checked and there's nothing in the original advisory against Darwin, but nothing that clearly states it's protected either (unlike Windows-based system). And there's also nothing specific to Darwin in the stack clash protection implementation, I think it's ok to only warn on Windows.

[emaste]

I think it's ok to only warn on Windows.

IMO that's sensible

[sylvestre.ledru]

Error only on Windows

[serge-sans-paille]

LGTM, thanks!

[thakis]

This seems to break all tests on all platforms: http://45.33.8.238/linux/35854/step_7.txt

As far as I can tell, RenderSCPOptions is called unconditinoally and the error message is emitted at its start, before any Args.hasFlag checks -- so it's emitted every time a windows triple is passed.

[thakis]

Also, rnk's comment above wasn't addressed as far as I can tell.

[thakis]

reverted in 00065d5cbd02b0f3fccb34881b58bcd0852b3970

[sylvestre.ledru]

Thanks for the revert!

clang/include/clang/Basic/DiagnosticDriverKinds.td

def warn_drv_omp_offload_target_duplicate : Warning<
   InGroup<OpenMPTarget>;
 def warn_drv_omp_offload_target_missingbcruntime : Warning<
   "No library '%0' found in the default clang lib directory or in LIBRARY_PATH. Expect degraded performance due to no inlining of runtime functions on target devices.">,
   InGroup<OpenMPTarget>;
 def err_drv_unsupported_embed_bitcode
     : Error<"%0 is not supported with -fembed-bitcode">;
 def err_drv_bitcode_unsupported_on_toolchain : Error<
   "-fembed-bitcode is not supported on versions of iOS prior to 6.0">;
+def err_drv_stack_clash_protection_unsupported_on_toolchain : Error<
+  "-fstack-clash-protection is not supported on %0">;
 
 def err_drv_invalid_malign_branch_EQ : Error<
   "invalid argument '%0' to -malign-branch=; each element must be one of: %1">;
 
 def warn_O4_is_O3 : Warning<"-O4 is equivalent to -O3">, InGroup<Deprecated>;
 def warn_drv_optimization_value : Warning<"optimization level '%0' is not supported; using '%1%2' instead">,
   InGroup<InvalidCommandLineArgument>;
 def warn_ignored_gcc_optimization : Warning<"optimization flag '%0' is not supported">,

clang/lib/Driver/ToolChains/Clang.cpp

if (EffectiveTriple.isX86() && (Value != "fs" && Value != "gs")) {
       << A->getOption().getName() << Value
       << "for X86, valid arguments to '-mstack-protector-guard-reg=' are:fs gs";
       return;
     }
     A->render(Args, CmdArgs);
   }
 }
 
-static void RenderSCPOptions(const ToolChain &TC, const ArgList &Args,
-                             ArgStringList &CmdArgs) {
+static void RenderSCPOptions(const Driver &D, const ToolChain &TC,
+                             const ArgList &Args, ArgStringList &CmdArgs) {
   const llvm::Triple &EffectiveTriple = TC.getEffectiveTriple();
 
-  if (!EffectiveTriple.isOSLinux())
+  if (EffectiveTriple.isOSWindows()) {
+    D.Diag(diag::err_drv_stack_clash_protection_unsupported_on_toolchain)
+        << EffectiveTriple.getOSName();
     return;
+  }
 
   if (!EffectiveTriple.isX86() && !EffectiveTriple.isSystemZ() &&
       !EffectiveTriple.isPPC64())
     return;
 
   if (Args.hasFlag(options::OPT_fstack_clash_protection,
                    options::OPT_fno_stack_clash_protection, false))
     CmdArgs.push_back("-fstack-clash-protection");

 
   Args.AddLastArg(CmdArgs, options::OPT_pthread);
 
   if (Args.hasFlag(options::OPT_mspeculative_load_hardening,
                    options::OPT_mno_speculative_load_hardening, false))
     CmdArgs.push_back(Args.MakeArgString("-mspeculative-load-hardening"));
 
   RenderSSPOptions(D, TC, Args, CmdArgs, KernelOrKext);
-  RenderSCPOptions(TC, Args, CmdArgs);
+  RenderSCPOptions(D, TC, Args, CmdArgs);
   RenderTrivialAutoVarInitOptions(D, TC, Args, CmdArgs);
 
   // Translate -mstackrealign
   if (Args.hasFlag(options::OPT_mstackrealign, options::OPT_mno_stackrealign,
                    false))
     CmdArgs.push_back(Args.MakeArgString("-mstackrealign"));
 
   if (Args.hasArg(options::OPT_mstack_alignment)) {

llvm/test/CodeGen/X86/stack-clash-large.ll

 ; RUN: llc -mtriple=x86_64-linux-android < %s | FileCheck -check-prefix=CHECK-X86-64 %s 
 ; RUN: llc -mtriple=i686-linux-android < %s | FileCheck -check-prefix=CHECK-X86-32 %s 
+; RUN: llc -mtriple=x86_64-unknown-freebsd < %s | FileCheck -check-prefix=CHECK-X86-64 %s
+; RUN: llc -mtriple=x86_64-pc-linux-gnu < %s | FileCheck -check-prefix=CHECK-X86-64 %s
 
 define i32 @foo() local_unnamed_addr #0 {
 
   %a = alloca i32, i64 18000, align 16
   %b0 = getelementptr inbounds i32, i32* %a, i64 98
   %b1 = getelementptr inbounds i32, i32* %a, i64 7198
   store volatile i32 1, i32* %b0
   store volatile i32 1, i32* %b1