[PATCH] D142046: [BPF][clang] Ignore stack protector options for BPF target

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142046/new/

https://reviews.llvm.org/D142046

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142046: [BPF][clang] Ignore stack protector options for BPF target

[Douglas Yung via Phabricator via cfe-commits]

dyung added a comment.

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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142046/new/

https://reviews.llvm.org/D142046

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142046: [BPF][clang] Ignore stack protector options for BPF target

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

@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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142046/new/

https://reviews.llvm.org/D142046

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142046: [BPF][clang] Ignore stack protector options for BPF target

[Douglas Yung via Phabricator via cfe-commits]

dyung added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142046/new/

https://reviews.llvm.org/D142046

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142046: [BPF][clang] Ignore stack protector options for BPF target

[Yonghong Song via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG56b038f887f3: [BPF][clang] Ignore stack protector options 
for BPF target (authored by eddyz87, committed by yonghong-song).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142046/new/

https://reviews.llvm.org/D142046

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/bpf-stack-protector.c


Index: clang/test/CodeGen/bpf-stack-protector.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-stack-protector.c
@@ -0,0 +1,34 @@
+// 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
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3239,6 +3239,12 @@
   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;
   }


Index: clang/test/CodeGen/bpf-stack-protector.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-stack-protector.c
@@ -0,0 +1,34 @@
+// 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
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3239,6 +3239,12 @@
   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;
   }
___
cfe-commits 

[PATCH] D142046: [BPF][clang] Ignore stack protector options for BPF target

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142046/new/

https://reviews.llvm.org/D142046

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142046: [BPF][clang] Ignore stack protector options for BPF target

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142046/new/

https://reviews.llvm.org/D142046

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142046: [BPF][clang] Ignore stack protector options for BPF target

[Alexei Starovoitov via Phabricator via cfe-commits]

ast added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142046/new/

https://reviews.llvm.org/D142046

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142046: [BPF][clang] Ignore stack protector options for BPF target

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

@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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142046/new/

https://reviews.llvm.org/D142046

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142046: [BPF][clang] Ignore stack protector options for BPF target

[Alexei Starovoitov via Phabricator via cfe-commits]

ast accepted this revision.
ast added a comment.
This revision is now accepted and ready to land.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142046/new/

https://reviews.llvm.org/D142046

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142046: [BPF][clang] Ignore stack protector options for BPF target

[Jose E. Marchesi via Phabricator via cfe-commits]

jemarch added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142046/new/

https://reviews.llvm.org/D142046

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142046: [BPF][clang] Ignore stack protector options for BPF target

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

@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-=c+hdk4ntjjfbhsf4r-g7pdjtrbat6mu...@mail.gmail.com/

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?

WDYT?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142046/new/

https://reviews.llvm.org/D142046

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142046: [BPF][clang] Ignore stack protector options for BPF target

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

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

  static void RenderSSPOptions(const Driver , const ToolChain ,
   const ArgList , ArgStringList ,
   bool KernelOrKext) {
const llvm::Triple  = 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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142046/new/

https://reviews.llvm.org/D142046

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142046: [BPF][clang] Ignore stack protector options for BPF target

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 created this revision.
Herald added a project: All.
eddyz87 requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

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]


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142046

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/bpf-stack-protector.c


Index: clang/test/CodeGen/bpf-stack-protector.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-stack-protector.c
@@ -0,0 +1,34 @@
+// 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
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3239,6 +3239,12 @@
   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;
   }


Index: clang/test/CodeGen/bpf-stack-protector.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-stack-protector.c
@@ -0,0 +1,34 @@
+// 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
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3239,6 +3239,12 @@
   StackProtectorLevel = LangOptions::SSPStrong;
 else if