[Offload] Do not pass -fcf-protection= for offloading

[jhuber6]

Summary:
This patch prevents the -fcf-protection= flag from being passed to the
device compilation during offloading. This is not supported on CUDA and
AMD devices, but if the user is compiling with fcf protection for the
host it will fail to compile.

We have a lot of these cases with various hacked together solutions, it
would be nice to have a single solution to detect from the driver if a
feature like this can be used for offloading, but for now this should
resolve the issue.

Fixes: #86450

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Joseph Huber (jhuber6)

Changes

Summary:
This patch prevents the -fcf-protection= flag from being passed to the
device compilation during offloading. This is not supported on CUDA and
AMD devices, but if the user is compiling with fcf protection for the
host it will fail to compile.

We have a lot of these cases with various hacked together solutions, it
would be nice to have a single solution to detect from the driver if a
feature like this can be used for offloading, but for now this should
resolve the issue.

Fixes: #86450

---

Full diff: https://github.com/llvm/llvm-project/pull/88402.diff

2 Files Affected:

• (modified) clang/lib/Driver/ToolChains/Clang.cpp (+8-2)
• (added) clang/test/Driver/offload-no-fcf-protection.c (+39)

diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 766a9b91e3c0ad..1264ffa1ef7c8e 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -6867,8 +6867,14 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
     CmdArgs.push_back("-nogpulib");
 
   if (Arg *A = Args.getLastArg(options::OPT_fcf_protection_EQ)) {
-    CmdArgs.push_back(
-        Args.MakeArgString(Twine("-fcf-protection=") + A->getValue()));
+    // Do not pass this argument to the offloading device if the target does not
+    // support it.
+    // TODO: We need a better way to detect incompatible options for offloading.
+    if (JA.getOffloadingDeviceKind() == Action::OFK_None ||
+        (!TC.getTriple().isAMDGPU() && !TC.getTriple().isNVPTX() &&
+         !TC.getTriple().isSPIRV()))
+      CmdArgs.push_back(
+          Args.MakeArgString(Twine("-fcf-protection=") + A->getValue()));
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_mfunction_return_EQ))
diff --git a/clang/test/Driver/offload-no-fcf-protection.c b/clang/test/Driver/offload-no-fcf-protection.c
new file mode 100644
index 00000000000000..ef275881edbaf1
--- /dev/null
+++ b/clang/test/Driver/offload-no-fcf-protection.c
@@ -0,0 +1,39 @@
+// Check that -fcf-protection does not get passed to the device-side
+// compilation.
+
+// RUN: %clang -### -x cuda --target=x86_64-unknown-linux-gnu -nogpulib \
+// RUN:   -nogpuinc --offload-arch=sm_52 -fcf-protection=full -c %s 2>&1 \
+// RUN: | FileCheck %s --check-prefix=CUDA
+
+// CUDA: "-cc1" "-triple" "nvptx64-nvidia-cuda"
+// CUDA-NOT: "-fcf-protection=full"
+// CUDA: "-cc1" "-triple" "x86_64-unknown-linux-gnu"
+// CUDA: "-fcf-protection=full"
+
+// RUN: %clang -### -x hip --target=x86_64-unknown-linux-gnu -nogpulib \
+// RUN:   -nogpuinc --offload-arch=gfx90a -fcf-protection=full -c %s 2>&1 \
+// RUN: | FileCheck %s --check-prefix=HIP
+
+// HIP: "-cc1" "-triple" "amdgcn-amd-amdhsa"
+// HIP-NOT: "-fcf-protection=full"
+// HIP: "-cc1" "-triple" "x86_64-unknown-linux-gnu"
+// HIP: "-fcf-protection=full"
+
+// RUN: %clang -### -x c --target=x86_64-unknown-linux-gnu -nogpulib -fopenmp=libomp \
+// RUN:   -nogpuinc --offload-arch=gfx90a -fcf-protection=full -c %s 2>&1 \
+// RUN: | FileCheck %s --check-prefix=OMP
+
+// OMP: "-cc1" "-triple" "x86_64-unknown-linux-gnu"
+// OMP: "-fcf-protection=full"
+// OMP: "-cc1" "-triple" "amdgcn-amd-amdhsa"
+// OMP-NOT: "-fcf-protection=full"
+// OMP: "-cc1" "-triple" "x86_64-unknown-linux-gnu"
+// OMP: "-fcf-protection=full"
+
+// RUN: %clang -### -x c --target=nvptx64-nvidia-cuda -nogpulib -nogpuinc \
+// RUN:   -march=sm_52 -fcf-protection=full -c %s 2>&1 \
+// RUN: | FileCheck %s --check-prefix=DIRECT
+// RUN: %clang -### -x c --target=amdgcn-amd-amdhsa -nogpulib -nogpuinc \
+// RUN:   -mcpu=gfx90a -fcf-protection=full -c %s 2>&1 \
+// RUN: | FileCheck %s --check-prefix=DIRECT
+// DIRECT: "-fcf-protection=full"

[arsenm]

Can we avoid adding yet another one of these arbitrary architecture list cases

[jhuber6]

I mean we could potentially just have some toolchain check like isToolchainGPU, but I don't know if that's exactly any better.

[arsenm]

It is 100% better

[Artem-B]

+1. We have grown too many offloading cases all over the place over time. It was fine when there was only CUDA/NVPTX, was sort of OK when AMDGPU got added, now it gets to be a bit too much.

[arsenm]

Can you check explicitly passing this through to the device?

[jhuber6]

I don't know how that would be possible. We have options like -Xarch_device, but those are stripped by the time we create the synthetic arguments that make it to the tool builder. Potentially we could do this logic where we build the args and just prevent the offloading toolchain from seeing it at all?

[arsenm]

Missing spirv run lines

[Artem-B]

LGTM in principle.

[Artem-B]

Nit: I'd collapse negations into one:

!(TC.getTriple().isAMDGPU() || TC.getTriple().isNVPTX() || TC.getTriple().isSPIRV())

[Artem-B]

+1. We have grown too many offloading cases all over the place over time. It was fine when there was only CUDA/NVPTX, was sort of OK when AMDGPU got added, now it gets to be a bit too much.

[jhuber6]

I'm wondering if it would be better to make this something that the toolchains handle in TranslateArgs. That way we can prevent it from being added at all. Although I don't know how that would allow us to override stuff. There's not really a way to "override" flags like this if we ignore it at all. We could "assume" that if someone does -Xarch_device -foo they want the device to use it, but that's not really what the flag means.

[MaskRay]

We also have unsupported-option-gpu.c to test various ignored options for GPU.