[UR] Fix cfi initialization #17472
[UR] Fix cfi initialization #17472
Conversation
RossBrunton
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
unified-runtime/cmake/helpers.cmake
Outdated
| @@ -154,7 +154,7 @@ function(add_ur_target_link_options name) | |||
| if(NOT MSVC) | |||
| if (NOT APPLE) | |||
| target_link_options(${name} PRIVATE | |||
| "LINKER:-z,relro,-z,now,-z,noexecstack ${CFI_FLAGS}" | |||
| "LINKER:-z,relro,-z,now,-z,noexecstack,${CFI_FLAGS}" | |||
There was a problem hiding this comment.
wait actually CFI_FLAGS is space separated so we're going to hit the same problem after the first flag, should we have a different comma separated one for this?
There was a problem hiding this comment.
Actually these aren't link options at all, lld doesn't recognize them. Think we just need to remove this
There was a problem hiding this comment.
So I've looked into this further, I don't think the CFI stuff works at all, it should be:
set(CFI_FLAGS "-flto;-fsanitize=cfi;-fno-sanitize=cfi-icall;-fsanitize-ignorelist=${PROJECT_SOURCE_DIR}/sanitizer-ignorelist.txt")
Otherwise it gets parsed as one big argument even in target_compile_options(${name} PRIVATE ${CFI_FLAGS}).
I also don't think CFI_FLAGS should be included in the LINKER: section, since they aren't linker options. What was the motivation behind moving it here https://github.com/intel/llvm/pull/17452/files#diff-bb0766d43b4a66d03975ff909dbc08a14f03b104ac5acd72b2e670cecfe6e30fL154 ?
There was a problem hiding this comment.
I was seeing similar quote-related problems here as with the compiler flags, where cmake was keeping the quotes in the final command because the link_options arg wasn't one big string. Figured the order didn't matter as long as they were still space separated as they (presumably) should have been before
There was a problem hiding this comment.
fwiw I do get sensible looking compile commands with spaces rather than ;:
-O3 -DNDEBUG -std=gnu++17 -fPIC -Wall -Wpedantic -Wempty-body -Wformat -Wformat-security -Wunused-parameter -fPIC -fstack-protector-strong -fvisibility=hidden -fcf-protection=full -fstack-clash-protection -fcolor-diagnostics -flto -fsanitize=cfi -fno-sanitize=cfi-icall -fsanitize-ignorelist=/path/to/sanitizer-ignorelist.txt -MD -MT
There was a problem hiding this comment.
I also don't think CFI_FLAGS should be included in the
LINKER:section, since they aren't linker options. What was the motivation behind moving it here https://github.com/intel/llvm/pull/17452/files#diff-bb0766d43b4a66d03975ff909dbc08a14f03b104ac5acd72b2e670cecfe6e30fL154 ?
Those are all inside target_link_options() so will always be treated as linkger options, if they are not linker options they should be in a target_compile_options() instead.
fwiw I do get sensible looking compile commands with spaces rather than
;:-O3 -DNDEBUG -std=gnu++17 -fPIC -Wall -Wpedantic -Wempty-body -Wformat -Wformat-security -Wunused-parameter -fPIC -fstack-protector-strong -fvisibility=hidden -fcf-protection=full -fstack-clash-protection -fcolor-diagnostics -flto -fsanitize=cfi -fno-sanitize=cfi-icall -fsanitize-ignorelist=/path/to/sanitizer-ignorelist.txt -MD -MT
If you don't surround a CMake list variable with quotes it will be treated as-if it's space separated when being parse in the context of the target_link_options().
There was a problem hiding this comment.
By "linker options" I mean options passed to the linker itself via https://cmake.org/cmake/help/latest/command/target_link_options.html#handling-compiler-driver-differences .
The CFI flags should be passed to c++ directly, not via -Xlinker or so.
There was a problem hiding this comment.
You don't pass flags to c++ with target_link_options(), is my point.
There was a problem hiding this comment.
It is used to pass flags to c++ as part of the link step.
If they aren't passed, linking will fail because c++ isn't able to link the object files (since -flto doesn't use elf as its object file format).
RossBrunton
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
RossBrunton
requested a deployment
to
WindowsCILock
GitHub Actions
In progress
RossBrunton
force-pushed
the
ross/noexecstack
branch
from
df8bd8c to
3728c39
Compare
RossBrunton
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
| @@ -132,8 +132,6 @@ struct MsanShadowMemoryGPU : public MsanShadowMemory { | |||
| ur_mutex VirtualMemMapsMutex; | |||
|
|
|||
| uptr LocalShadowOffset = 0; | |||
|
|
|||
| uptr PrivateShadowOffset = 0; | |||
There was a problem hiding this comment.
Is this change intentional?
There was a problem hiding this comment.
Yes, otherwise building with clang fails because it is unused.
There were a number of problems preventing CFI from being enabled, this hopefully fixes them. As part of this, an unused variable compile error in `clang` was fixed.
RossBrunton
force-pushed
the
ross/noexecstack
branch
from
3728c39 to
7afc470
Compare
RossBrunton
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
RossBrunton
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
RossBrunton
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
There were a number of problems preventing CFI from being enabled, this hopefully fixes them.