[StackProtector] Do not emit the stack protector on GPU architectures

[jhuber6]

Summary:
This patch changes the code generation to not emit the stack protector
metadata on unsupported architectures. The issue was caused by system
toolchains emitting stack protector option by default which would lead
to errors when compiling for the GPU. I elected to change the code
generation as we may want to update this in the future so we should keep
the clang Driver code common. Although the user can use some
combination of -Xarch-device -fno-stack-protector to override this, it
is very irritating to do when we shouldn't emit this incompatible IR
anyway.

Fixes: #65911

[llvmbot]

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Joseph Huber (jhuber6)

Changes

Summary:
This patch changes the code generation to not emit the stack protector
metadata on unsupported architectures. The issue was caused by system
toolchains emitting stack protector option by default which would lead
to errors when compiling for the GPU. I elected to change the code
generation as we may want to update this in the future so we should keep
the clang Driver code common. Although the user can use some
combination of -Xarch-device -fno-stack-protector to override this, it
is very irritating to do when we shouldn't emit this incompatible IR
anyway.

Fixes: #65911

---

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

1 Files Affected:

• (modified) clang/lib/CodeGen/CodeGenModule.cpp (+11-4)

diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index b1a6683a66bd052..562f421aa2c36e8 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -761,6 +761,13 @@ static void setVisibilityFromDLLStorageClass(const clang::LangOptions &LO,
   }
 }
 
+static bool isStackProtectorOn(const LangOptions &LangOpts, const llvm::Triple &Triple,
+                               clang::LangOptions::StackProtectorMode Mode) {
+  if (Triple.isAMDGPU() || Triple.isNVPTX())
+    return false;
+  return LangOpts.getStackProtector() == Mode;
+}
+
 void CodeGenModule::Release() {
   Module *Primary = getContext().getCurrentNamedModule();
   if (CXX20ModuleInits && Primary && !Primary->isHeaderLikeModule())
@@ -2296,13 +2303,13 @@ void CodeGenModule::SetLLVMFunctionAttributesForDefinition(const Decl *D,
   if (D && D->hasAttr<NoStackProtectorAttr>())
     ; // Do nothing.
   else if (D && D->hasAttr<StrictGuardStackCheckAttr>() &&
-           LangOpts.getStackProtector() == LangOptions::SSPOn)
+           isStackProtectorOn(LangOpts, getTriple(), LangOptions::SSPOn))
     B.addAttribute(llvm::Attribute::StackProtectStrong);
-  else if (LangOpts.getStackProtector() == LangOptions::SSPOn)
+  else if (isStackProtectorOn(LangOpts, getTriple(), LangOptions::SSPOn))
     B.addAttribute(llvm::Attribute::StackProtect);
-  else if (LangOpts.getStackProtector() == LangOptions::SSPStrong)
+  else if (isStackProtectorOn(LangOpts, getTriple(), LangOptions::SSPStrong))
     B.addAttribute(llvm::Attribute::StackProtectStrong);
-  else if (LangOpts.getStackProtector() == LangOptions::SSPReq)
+  else if (isStackProtectorOn(LangOpts, getTriple(), LangOptions::SSPReq))
     B.addAttribute(llvm::Attribute::StackProtectReq);
 
   if (!D) {

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[shiltian]

I think the changes make sense.

[efriedma-quic]

Is there some reason stack protectors don't make sense on GPU targets? Or is the issue just that the GPU targets in question don't have the necessary runtime support?

[jhuber6]

Is there some reason stack protectors don't make sense on GPU targets? Or is the issue just that the GPU targets in question don't have the necessary runtime support?

It's more of the latter as GPUs don't really have much of a runtime. That's why I didn't want to completely filter this out at the driver level and instead made it more of a "TODO". However, I don't know if we'd really have much of a use for stack protectors. Maybe @arsenm could help explain that further.

[MaskRay]

I think this is fine, but we probably should think about the general option compatibility problem #70740 (comment)