[BPF][clang] Ignore stack protector options for BPF target

[eddyz87]

Summary

Stack protector builtin functions are not implemented for BPF target,
thus compiling programs with one of the following options would result
in an error:

-fstack-protector
-fstack-protector-all
-fstack-protector-strong

This commit adds logic to ignore these options for BPF target.
Searching through DiagnosticDriverKinds.td shows that all messages for
such kind of behavior are implemented as warnings, this commit follows
the suit.

Here is an example of the diagnostic message:

clang-16: warning: ignoring '-fstack-protector' option as it is not currently supported for target 'bpf' [-Woption-ignored]

[eddyz87]

Slightly above my edit there is a similar logic for NVPTX target:

static void RenderSSPOptions(const Driver &D, const ToolChain &TC,
                             const ArgList &Args, ArgStringList &CmdArgs,
                             bool KernelOrKext) {
  const llvm::Triple &EffectiveTriple = TC.getEffectiveTriple();

  // NVPTX doesn't support stack protectors; from the compiler's perspective, it
  // doesn't even have a stack!
  if (EffectiveTriple.isNVPTX())
    return;
  ...

I don't like it because it produces rather vague error message:

warning: argument unused during compilation: '-fstack-protector' [-Wunused-command-line-argument]

However I'm hesitant to merge the NVPTX check with BPF check to avoid any potential changes in the NVPTX target behavior (e.g. different warning).

[yonghong-song]

@compnerd could you also take a look at this patch?

First, some background about this patch. The reason of this patch is due to:

https://lore.kernel.org/bpf/CAOFdcFPnHEc2qd-=[e-mail]/

Further the following link has details about hardened gentoo and modified clang default flags:

https://wiki.gentoo.org/wiki/Hardened_Gentoo

Unfortunately, we don't want -fstack-protector for bpf target as bpf kernel verifier will do stack verification.
This patch tries to ignore -fstack-protector. NVPTX arch has the same need of ignoring -fstack-protector.
This patch presented a different message than NVPTX. This patch:

clang-16: warning: ignoring '-fstack-protector' option as it is not currently supported for target 'bpf' [-Woption-ignored]

NVPTX

clang-16: warning: argument unused during compilation: '-fstack-protector' [-Wunused-command-line-argument]

Does clang community has a preference for the above two formats?

Let us say we do implement this patch and merged into clang, then using gentoo clang compiler, we will hit
the warning:

clang-16: warning: ignoring '-fstack-protector' option as it is not currently supported for target 'bpf' [-Woption-ignored]

I suspect gentoo community might complain. I don't think we should add -Wno-option-ignored since this may cause
unintended issues with compiler command line arguments.
Without -Wno-option-ignored, gentoo community might wants to append -fno-stack-protector to disable stack-protector,
but with the current implementation, we might actually trigger another warning.

So we have to two choices here for bpf target:

(1). ignore any stack-protector option and do not issue any warning, or
(2). append -fno-stack-protector by clang to compilation flags.

We can document this behavior in related clang (and kernel) docs.

WDYT?

[jemarch]

Note that in GCC we are now emitting a note if the stack protector is requested by the user in the BPF backend:

note: -fstack-protector does not work on this architecture

which is less intrusive than a warning, still informative.
Then the stack protector is disabled.

[ast]

This approach looks better to me than NVPTX warn and I agree with Ed that it's better to leave NVPTX as-is to avoid any regression.

[yonghong-song]

@ast With this patch, gentoo clang compilation will hit the warning even if people appends -fno-stack-protector. Is this okay? In general, if the option is '-fstack-protector -fno-stack-protector', we should not issue warning, right?

[ast]

In D142046#4066758, @yonghong-song wrote:

@ast With this patch, gentoo clang compilation will hit the warning even if people appends -fno-stack-protector. Is this okay? In general, if the option is '-fstack-protector -fno-stack-protector', we should not issue warning, right?

ahh. yeah. -fno-stack-protector should silence the warning.

[yonghong-song]

Sorry, I double checked. '-fstack-protector -fno-stack-protector' will not result in warnings. So the patch LGTM. So gentoo people can add -fno-stack-protector to suppress warnings.

[eddyz87]

I can relax the warning to note to be on the same page with GCC, the reason I didn't is that similar things in DiagnosticDriverKinds.td are warnings.

[dyung]

Hi, our internal release build buildbot seems to have issues with this test. It seems you are expected the following function declaration in the emitted IR:

define dso_local void @_Z5test1PKc(ptr noundef %msg) #0 !dbg !19 {

But in a release build, it becomes:

define dso_local void @_Z5test1PKc(ptr noundef %0) #0 !dbg !19 {

Note that %msg became %0. Can you fix the test so that it works in release builds as well?

Godbolt link: https://godbolt.org/z/E5zbM4chc

[yonghong-song]

@dyung I just pushed the fix to the 'main' branch (https://github.com/llvm/llvm-project/commit/183d075055c591dedead7ece972f1bdea611aa3b). Please check it out. Thanks for reporting!

[dyung]

In D142046#4071594, @yonghong-song wrote:

@dyung I just pushed the fix to the 'main' branch (https://github.com/llvm/llvm-project/commit/183d075055c591dedead7ece972f1bdea611aa3b). Please check it out. Thanks for reporting!

That fixed it, thanks!

[eddyz87]

Hi Yonghong,

Thank you for taking care of this issue.
This was sloppy on my side as the parameter name was completely
irrelevant for the test.

Best regards,
Eduard

clang/lib/Driver/ToolChains/Clang.cpp

if (Arg *A = Args.getLastArg(options::OPT_fno_stack_protector,
                                options::OPT_fstack_protector)) {
     if (A->getOption().matches(options::OPT_fstack_protector))
       StackProtectorLevel =
           std::max<>(LangOptions::SSPOn, DefaultStackProtectorLevel);
     else if (A->getOption().matches(options::OPT_fstack_protector_strong))
       StackProtectorLevel = LangOptions::SSPStrong;
     else if (A->getOption().matches(options::OPT_fstack_protector_all))
       StackProtectorLevel = LangOptions::SSPReq;
+
+    if (EffectiveTriple.isBPF() && StackProtectorLevel != LangOptions::SSPOff) {
+      D.Diag(diag::warn_drv_unsupported_option_for_target)
+          << A->getSpelling() << EffectiveTriple.getTriple();
+      StackProtectorLevel = DefaultStackProtectorLevel;
+    }
   } else {
     StackProtectorLevel = DefaultStackProtectorLevel;
   }
 
   if (StackProtectorLevel) {
     CmdArgs.push_back("-stack-protector");
     CmdArgs.push_back(Args.MakeArgString(Twine(StackProtectorLevel)));
   }

clang/test/CodeGen/bpf-stack-protector.c

+// REQUIRES: bpf-registered-target
+
+// RUN %clang -target bpf -S -emit-llvm -o - %s -fno-stack-protector 2>&1 \
+// RUN        | FileCheck -check-prefix=OFF -check-prefix=COMMON %s
+
+// RUN: %clang -target bpf -S -emit-llvm -o - %s -fstack-protector 2>&1 \
+// RUN:        | FileCheck -check-prefix=ON -check-prefix=COMMON %s
+
+// RUN: %clang -target bpf -S -emit-llvm -o - %s -fstack-protector-all 2>&1 \
+// RUN:        | FileCheck -check-prefix=ALL -check-prefix=COMMON %s
+
+// RUN: %clang -target bpf -S -emit-llvm -o - %s -fstack-protector-strong 2>&1 \
+// RUN:        | FileCheck -check-prefix=STRONG -check-prefix=COMMON %s
+
+typedef __SIZE_TYPE__ size_t;
+
+int printf(const char * _Format, ...);
+size_t strlen(const char *s);
+char *strcpy(char *s1, const char *s2);
+
+//     OFF-NOT: warning
+//          ON: warning: ignoring '-fstack-protector'
+//         ALL: warning: ignoring '-fstack-protector-all'
+//      STRONG: warning: ignoring '-fstack-protector-strong'
+// COMMON-SAME: option as it is not currently supported for target 'bpf'
+
+// COMMON: define {{.*}}void @test1(ptr noundef %msg) #[[A:.*]] {
+void test1(const char *msg) {
+  char a[strlen(msg) + 1];
+  strcpy(a, msg);
+  printf("%s\n", a);
+}
+
+// COMMON-NOT: attributes #[[A]] = {{.*}} ssp