-
Notifications
You must be signed in to change notification settings - Fork 981
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
build: do not use stack protector for gpu targets #1705
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.