-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: Add options to select sanitizers in configure.py #2437
base: master
Are you sure you want to change the base?
build: Add options to select sanitizers in configure.py #2437
Conversation
@tchaikov please review |
CMakeLists.txt
Outdated
DEFAULT_BUILD_TYPES "Debug" "Sanitize" | ||
CONDITION sanitizers_enabled) | ||
|
||
if (sanitizers_enabled) |
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.
this does not work when we build seastar with a multi-config generator. in that case, the value of sanitizers_enabled
would be a generator expression, which always evaluates to TRUE
. and its value is supposed to be used like:
target_link_libraries (seastar
PUBLIC
$<${condition}:Sanitizers::address>)
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.
fixed
CMakeLists.txt
Outdated
|
||
if (sanitizers_enabled) | ||
if ((NOT Seastar_SANITIZERS) OR (Seastar_SANITIZERS STREQUAL "")) | ||
set (Seastar_SANITIZERS ${Seastar_DEFAULT_SANITIZERS}) |
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'd suggest making Seastar_SANITIZERS
a CMake option explicitly, like:
set (Seastar_SANITIZERS
"address;undefined_behavior"
CACHE
STRING
"Sanitizers enabled when building Seastar")
and if we build with multi-config generator, this option applies to Debug
and Sanitize
modes, otherwise, this option applies the the current build.
what do you think?
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 about multi-config generator.
It doesn't make sense to use sanitizers in other modes because they (at least address and thread) won't work with seastar allocator. But you still can do it by passing Seastar_SANITIZE=yes
.
I can force to use sanitizers in ./configure.py
if they are specified manually and args.mode != 'all'
. What do you think?
P.S. ./configure.py
does not use multi-config generator
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.
Now I left the logic of sanitizers and build modes unchanged. Enabling sanitizers depends on Seastar_SANITIZE
variable. By default it enables them only for Debug and Sanitize
configure.py
Outdated
@@ -171,15 +173,23 @@ def identify_best_standard(cpp_standards, compiler): | |||
|
|||
MODE_TO_CMAKE_BUILD_TYPE = {'release': 'RelWithDebInfo', 'debug': 'Debug', 'dev': 'Dev', 'sanitize': 'Sanitize' } | |||
|
|||
SANITIZER_TO_CMAKE = {'address': 'address', 'undefined': 'undefined_behavior'} |
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 not just use undefined_behavior
? what's the merit of introducing another layer?
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.
Agree, the same thing happens in FindSanitizers.cmake
. I deleted it
2d840e0
to
7305f93
Compare
sorry for the latency. i will try to review this change in this week. |
CMakeLists.txt
Outdated
$<${condition}:Sanitizers::undefined_behavior>) | ||
set (Seastar_Sanitizers_OPTIONS $<${sanitizers_enabled}:${Sanitizers_COMPILE_OPTIONS}>) | ||
foreach (component ${Seastar_SANITIZERS}) | ||
string (TOUPPER ${component} COMPONENT) |
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.
COMPONENT
is never used.
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.
fixed
PUBLIC | ||
$<${condition}:Sanitizers::address> | ||
$<${condition}:Sanitizers::undefined_behavior>) | ||
set (Seastar_Sanitizers_OPTIONS $<${sanitizers_enabled}:${Sanitizers_COMPILE_OPTIONS}>) |
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.
could you extract this change into a separate commit, and accompany it with rationales in the commit message?
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.
done in this PR
tri_state_option (${Seastar_SANITIZE} | ||
DEFAULT_BUILD_TYPES "Debug" "Sanitize" | ||
CONDITION sanitizers_enabled) |
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 are you moving this command up here?
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.
if sanitizers are disabled for this build type we don't need to find them
cmake/SeastarDependencies.cmake
Outdated
@@ -141,6 +141,9 @@ macro (seastar_find_dependencies) | |||
seastar_set_dep_args (rt REQUIRED) | |||
seastar_set_dep_args (numactl | |||
OPTION ${Seastar_NUMA}) | |||
seastar_set_dep_args (Sanitizers | |||
COMPONENTS | |||
${Seastar_SANITIZERS}) |
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.
please note, this file is installed and redistributed by seastar. it is evaluated when a Seastar application consumes the Seastar library via CMake's find_package(Seastar)
.
there is a potential issue with the current configuration:
-
At the time this file is processed,
Seastar_SANITIZERS
is empty. -
This may result in an incomplete Seastar CMake configuration for the parent project.
-
If the user doesn't have sanitizer libraries installed,
find_package(Seastar)
still succeeds. -
However, Seastar libraries compiled with sanitizers enabled may fail to configure or link due to the missing sanitizer libraries, as what we actually have in this file is
find_package(Sanitizers COMPONENTS)
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 am not sure what is the best way to address this. but there are at least two approaches:
-
create a separate
cmake/SeastarDependencies.cmake.in
, and generate the cmake file usingconfigure_file()
, so we can have a differentSeastarDependencies.cmake
for each mode, and theninstall()
it -
check all the valid combinations:
address
+undefined_behavior
thread
configure.py
Outdated
@@ -142,6 +142,8 @@ def standard_supported(standard, compiler='g++'): | |||
arg_parser.add_argument('--dpdk-machine', default='native', help='Specify the target architecture') | |||
add_tristate(arg_parser, name='deferred-action-require-noexcept', dest='deferred_action_require_noexcept', help='noexcept requirement for deferred actions', default=True) | |||
arg_parser.add_argument('--prefix', dest='install_prefix', default='/usr/local', help='Root installation path of Seastar files') | |||
arg_parser.add_argument('--sanitizer', dest='sanitizers', action='append', default=[], help='Use specified sanitizer') | |||
arg_parser.add_argument('--no-sanitizers', dest='no_sanitizers', action='store_true', default=False, help='Do not use sanitizers') |
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.
after a second thought, i think probably it's simpler to accept a colon-separated or a comma-separated list as the argument as --sanitizers
? like
g_parser.add_argument('--sanitizers', dest='sanitizers', default='address;undefined_behavior', help='Use specified sanitizers')
what do you think?
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.
done
7b30680
to
7b7009b
Compare
We are going to add ThreadSanitizer that can't be used together with AddressSanitizer. We want to choose which sanitizers to use. Sanitizers can be specified as semicolon separated list: ./configure.py --sanitizers address;undefined_behavior Specified sanitizers are passed to Seastar_SANITIZERS list. Enabling sanitizers is consistently with Seastar_SANITIZE option.
7b7009b
to
1b11a38
Compare
@devDDen thank you for addressing the comments. i will try to review your change this week. |
@tchaikov have you had a chance to look through proposed changes? |
We are going to add ThreadSanitizer that can't be used together with
AddressSanitizer. We want to choose which sanitizers to use.
Sanitizers can be specified as semicolon separated list:
Specified sanitizers are passed to Seastar_SANITIZERS list.
Enabling sanitizers is consistently with Seastar_SANITIZE option.