[AMDGPU] -fcf-protection=full should not be applied to GPU targets in heterogenous code

[AngryLoki]

In Gentoo 23.0 (upcoming) and hardened profile -fcf-protection=full is added automatically via /etc/clang/x86_64-pc-linux-gnu-clang++.cfg (as well as other flags). However this flag does not work well with heterogeneous hip code:

cd /tmp && wget https://raw.githubusercontent.com/ROCm-Developer-Tools/HIP-CPU/master/examples/vadd_hip/vadd_hip.cpp

# -fcf-protection=full is added manually for demonstration
/usr/lib/llvm/18/bin/clang++ --offload-arch=native -x hip vadd_hip.cpp -o vadd_hip \
-fno-stack-protector --hip-link -fcf-protection=full -nogpulib

error: option 'cf-protection=return' cannot be specified on this target

Although it is possible to use -fcf-protection=full -Xarch_device -fcf-protection=none to override this, it is irritating, as it can not be added to all files, as it produces warning: argument unused during compilation: '-Xarch_device -fcf-protection=none' for non-hip files.

In https://github.com/llvm/llvm-project/pull/70799 you added code to "not emit the stack protector metadata on unsupported architectures". Can you do the same for -fcf-protection=..., to apply CET only for host code? Thanks!

[AngryLoki]

CC @jhuber6 as an author of stack-protector PR.

[llvmbot]

@llvm/issue-subscribers-backend-amdgpu

Author: None (AngryLoki)

In Gentoo 23.0 (upcoming) and hardened profile `-fcf-protection=full` is added automatically via `/etc/clang/x86_64-pc-linux-gnu-clang++.cfg` (as well as [other flags](https://wiki.gentoo.org/wiki/Hardened/Toolchain#Changes)). However this flag does not work well with heterogeneous hip code: ``` cd /tmp && wget https://raw.githubusercontent.com/ROCm-Developer-Tools/HIP-CPU/master/examples/vadd_hip/vadd_hip.cpp # -fcf-protection=full is added manually for demonstration /usr/lib/llvm/18/bin/clang++ --offload-arch=native -x hip vadd_hip.cpp -o vadd_hip -fno-stack-protector --hip-link -fcf-protection=full -nogpulib error: option 'cf-protection=return' cannot be specified on this target ``` Although it is possible to use `-fcf-protection=full -Xarch_device -fcf-protection=none`, to override this, but it is very irritating and it can not be added to all files, as it produces `warning: argument unused during compilation: '-Xarch_device -fcf-protection=none'` for non-hip files. In https://github.com/llvm/llvm-project/pull/70799 you added code to "not emit the stack protector metadata on unsupported architectures". Can you do the same for `-fcf-protection=...`, to apply CET only for host code? Thanks!

[jhuber6]

The stack protector stuff was kind of hacky, I remember some similar case that @yxsamliu and @MaskRay looked into but forget which flag it was.

[AngryLoki]

Huh, actually, after submitting this bug, I remembered flag -Xarch_host. So I reported additional bug to Gentoo to consider rewriting /etc/clang/gentoo-hardened.cfg with:

-Xarch_host -fstack-clash-protection
-Xarch_host -fstack-protector-strong
-Xarch_host -fPIE
-include "/usr/include/gentoo/fortify.h"
-Xarch_host -fcf-protection=full

However I still think the best solution is to solve this in Clang. As far as I know, there are already many flags that are not passed to GPU codegen (even turning -O2 into -O3) and -fcf-protection is just one of them.

(edit: fix copy&paste mistake)

[jhuber6]

Huh, actually, after submitting this bug, I remembered flag -Xarch_device. So I reported additional bug to Gentoo to consider rewriting /etc/clang/gentoo-hardened.cfg with:

-Xarch_device -fstack-clash-protection
-Xarch_device -fstack-protector-strong
-Xarch_device -fPIE
-include "/usr/include/gentoo/fortify.h"
-Xarch_device -fcf-protection=full

I'm assuming you mean -Xarch_host? That will apply all of those to the device build.

However I still think the best solution is to solve this in Clang. As far as I know, there are already many flags that are not passed to GPU codegen (even turning -O2 into -O3) and -fcf-protection is just one of them.

Currently we kind of just treat this on a case-by-case basis. There's a lot of pain that come from these single source languages where we try to mash two separate targets into a single compiler invocation. Same problem with all of our numerous hacks around the glibc headers when included from the GPU.

[AngryLoki]

I'm assuming you mean -Xarch_host?

Yes, copy&paste mistake.

[stalkerg]

The same issue with dev-libs/libclc package.

[stalkerg]

https://bugs.gentoo.org/928961

[jhuber6]

These things are permanent issues caused by pretending that two separate compilations for two separate architectures is a single compiler invocation. The easiest solution would be the following that simply prevents the option from being forwarded to the offloading device, essentially making it behave like -Xarch_host was always added.

diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 766a9b91e3c0..0d19c67778e0 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -6866,9 +6866,11 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
   if (Args.hasArg(options::OPT_nogpulib))
     CmdArgs.push_back("-nogpulib");

-  if (Arg *A = Args.getLastArg(options::OPT_fcf_protection_EQ)) {
-    CmdArgs.push_back(
-        Args.MakeArgString(Twine("-fcf-protection=") + A->getValue()));
+  if (JA.getOffloadingDeviceKind() == Action::OFK_None) {
+    if (Arg *A = Args.getLastArg(options::OPT_fcf_protection_EQ)) {
+      CmdArgs.push_back(
+          Args.MakeArgString(Twine("-fcf-protection=") + A->getValue()));
+    }
   }

   if (Arg *A = Args.getLastArg(options::OPT_mfunction_return_EQ))

However, this would prevent users from enabling it intentionally or overriding that behavior.

[yxsamliu]

These things are permanent issues caused by pretending that two separate compilations for two separate architectures is a single compiler invocation. The easiest solution would be the following that simply prevents the option from being forwarded to the offloading device, essentially making it behave like -Xarch_host was always added.

diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 766a9b91e3c0..0d19c67778e0 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -6866,9 +6866,11 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
   if (Args.hasArg(options::OPT_nogpulib))
     CmdArgs.push_back("-nogpulib");

-  if (Arg *A = Args.getLastArg(options::OPT_fcf_protection_EQ)) {
-    CmdArgs.push_back(
-        Args.MakeArgString(Twine("-fcf-protection=") + A->getValue()));
+  if (JA.getOffloadingDeviceKind() == Action::OFK_None) {
+    if (Arg *A = Args.getLastArg(options::OPT_fcf_protection_EQ)) {
+      CmdArgs.push_back(
+          Args.MakeArgString(Twine("-fcf-protection=") + A->getValue()));
+    }
   }

   if (Arg *A = Args.getLastArg(options::OPT_mfunction_return_EQ))

However, this would prevent users from enabling it intentionally or overriding that behavior.

It seems currently we do not have a better way to handle this. If a target starts to support these options we could re-enable them for that target.

[arsenm]

It seems currently we do not have a better way to handle this. If a target starts to support these options we could re-enable them for that target.

We could just implement this, but it's probably not what anyone wants by default. I think we should interpret the flag as host-only, but I think it would be good to allow enabling it specifically for the device