build: check the combination of Sanitizers #2408
Conversation
tchaikov
force-pushed
the
find-sanitizer
branch
2 times, most recently
from
2c2136e
to
fa285ae
Compare
tchaikov
force-pushed
the
find-sanitizer
branch
2 times, most recently
from
232aa02
to
c50d312
Compare
tchaikov
changed the title
tchaikov
force-pushed
the
find-sanitizer
branch
from
c50d312
to
0bb1f62
Compare
| set(Sanitizers_FIND_COMPONENTS | ||
| address | ||
| undefined_behavior) | ||
| endif() |
There was a problem hiding this comment.
this preserves the backward compatibility if some parent projects use find_package(Sanitizers) without specifying the COMPONENTS parameter.
| elseif (component STREQUAL "undefined_behavior") | ||
| list (APPEND ${compile_options} -fsanitize=undefined) | ||
| # Disable vptr because of https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88684 | ||
| list (APPEND ${compile_options} -fno-sanitize=vptr) |
There was a problem hiding this comment.
Note: this was fixed in gcc 10 (or perhaps 9?) - gcc-mirror/gcc@c24847a so we can drop it later.
There was a problem hiding this comment.
sure. will do in a separate PR.
cmake/SeastarDependencies.cmake
Outdated
| seastar_set_dep_args (Sanitizers | ||
| COMPONENTS | ||
| address | ||
| undefined_behavior) | ||
| seastar_set_dep_args (ucontext REQUIRED) |
There was a problem hiding this comment.
Is this complexity typical for projects that use sanitizers? I don't mind merging it, but isn't cmake supposed to do this for us?
The code is incomprehensible to me.
There was a problem hiding this comment.
i wanted to make it explicit. yes, it's taken care of. see #2408 (comment) . but with #2406, we will need to specify it anyway. probably not in this file.
There was a problem hiding this comment.
anyway, i dropped this change in the latest revision.
we are going to add a build with ThreadSanitizer enabled, but ThreadSanitizer cannot be used along with AddressSanitizer. the existing sanitizer detection checks each sanitizer separately, so even if the sanitizers combination does not work, the detection still passes. also, we hardwire two sanitizers in `FindSanitizers.cmake`, this is not extensible to the use case where we only need to selectively find a certain (combination) of sanitizers. in order to address these problems, in this change * find specified component(s) in FindSanitizers.cmake, to prepare for the change which selectively specifies a subset of sanitizers * check if the compiler supports the combination of compile options required by all specified sanitizers Signed-off-by: Kefu Chai <[email protected]>
tchaikov
force-pushed
the
find-sanitizer
branch
from
0bb1f62
to
d6dc2ac
Compare
|
@avikivity mind taking another look? |
|
v2:
|
|
@scylladb/seastar-maint and @avikivity could you help review this change? |
we are going to add a build with ThreadSanitizer enabled, but ThreadSanitizer cannot be used along with AddressSanitizer. the existing sanitizer detection checks each sanitizer separately, so even if the sanitizers combination does not work, the detection still passes.
also, we hardwire two sanitizers in
FindSanitizers.cmake, this is not extensible to the use case where we only need to selectively find a certain (combination) of sanitizers.in order to address these problems, in this change
SeastarDependencies.cmake.