build: do not use stack protector for gpu targets #1705
Conversation
cmake/SDL.cmake
Outdated
| @@ -71,7 +71,8 @@ if(UNIX) | |||
| elseif("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang") | |||
| get_filename_component(CXX_CMD_NAME ${CMAKE_CXX_COMPILER} NAME) | |||
| # Fujitsu CXX compiler does not support "-fstack-protector-all". | |||
| if(NOT CXX_CMD_NAME STREQUAL "FCC") | |||
| # The same applies to GPU targets (CUDA/HIP). | |||
| if(NOT CXX_CMD_NAME STREQUAL "FCC" AND NOT(DNNL_SYCL_CUDA OR DNNL_SYCL_HIP)) | |||
There was a problem hiding this comment.
It seems to me that CPU backend stack protection will be disabled as well with this change. Not sure this is the intention.
There was a problem hiding this comment.
How do you mean, previously it would skip adding the flag if not using FCC, now it check if we are not using FCC and are not compiling for either CUDA or HIP.
Perhaps a cleaner way would be to rewrite it to:
if (NOT (CXX_CMD_NAME STREQUAL "FCC" OR DNNL_SYCL_CUDA OR DNN_SYCL_HIP))
There was a problem hiding this comment.
Why is this set in the default build config at all? Isn't it to the downstream user to decide whether to slow down their build for the extra security e.g. what Linux distributions do with setting these in their package build scripts et al
I personally wouldn't set this by default and leave it for folk to configure with CXXFLAGS= or similar mechanisms. You might see a few cycles shaved off too ;)
There was a problem hiding this comment.
I agree, but it seems like a discussion for another ticket. I'd like to fix the build for people targeting AMD as soon as possible, as it manifest through a hard compiler crash that might not be trivial for everyone to trace down.
I've simplified the boolean logic in the if clause.
There was a problem hiding this comment.
Why is this set in the default build config at all? Isn't it to the downstream user to decide whether to slow down their build for the extra security e.g. what Linux distributions do with setting these in their package build scripts et al
It's enabled by default to meet security requirements. Having the right flags by default helps to fortify downstream applications.
jchlanda
force-pushed
the
jakub/stack_protector_fix
branch
from
e8188d1
to
ba76ca5
Compare
| @@ -71,7 +71,8 @@ if(UNIX) | |||
| elseif("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang") | |||
| get_filename_component(CXX_CMD_NAME ${CMAKE_CXX_COMPILER} NAME) | |||
| # Fujitsu CXX compiler does not support "-fstack-protector-all". | |||
| if(NOT CXX_CMD_NAME STREQUAL "FCC") | |||
| # The same applies to GPU targets (CUDA/HIP). | |||
| if(NOT (CXX_CMD_NAME STREQUAL "FCC" OR DNNL_SYCL_CUDA OR DNN_SYCL_HIP)) | |||
There was a problem hiding this comment.
We don't see any issues with CUDA backend when using -fstack-protector-all. Does it mean it's supported for CUDA and not supported for HIP?
Also, is there any way to keep the option for the host compiler while disabling it for the device compiler?
There was a problem hiding this comment.
Does it mean it's supported for CUDA and not supported for HIP?
As far as I know, it's only because compiler will decide to ignore the option and warn during compilation, i.e: https://godbolt.org/z/nKeq5PaWP, but that's only my guess so take it with a pinch of salt.
Also, is there any way to keep the option for the host compiler while disabling it for the device compiler?
As for your second question perhaps sycl-target-backend option could be of help here, see: https://intel.github.io/llvm-docs/UsersManual.html#target-toolchain-options
It would be something along the lines of -fsycl-target-backend=amdgcn-amd-amdhsa -fno-stack-protector. Again take it with a pinch of salt, I've not tested it myself.
In general feel free to reject/close this PR, I was trying to unblock compilation for HIP backend, as we've seen similar issues as discussed in intel/llvm#7269 on our side, but as I don't know this codebase well enough, I'm happy to describe the problem (and potential solution) in an issue against https://github.com/oneapi-src/oneDNN.
GPU backends are not geared up to use stack protectors. Using the flag will result in miscompilation.
This has been discovered in intel/llvm#7269.