build: Add options to select sanitizers in configure.py #2437

devDDen · 2024-09-16T11:51:23Z

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.

devDDen · 2024-09-16T11:54:35Z

@tchaikov please review

tchaikov · 2024-09-17T11:53:57Z

CMakeLists.txt

+  DEFAULT_BUILD_TYPES "Debug" "Sanitize"
+  CONDITION sanitizers_enabled)
+
+if (sanitizers_enabled)


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>)

tchaikov · 2024-09-17T12:00:23Z

CMakeLists.txt

+
+if (sanitizers_enabled)
+  if ((NOT Seastar_SANITIZERS) OR (Seastar_SANITIZERS STREQUAL ""))
+    set (Seastar_SANITIZERS ${Seastar_DEFAULT_SANITIZERS})


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?

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

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

forgot to explain, we (scylladb) wanted to use multi-config generator in our building system to build seastar, but it turned out it was complicated to build both shared and static libraries in a single pass without hurting the performance of the shared libraries.

tchaikov · 2024-09-17T12:02:30Z

configure.py

@@ -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'}


why not just use undefined_behavior? what's the merit of introducing another layer?

Agree, the same thing happens in FindSanitizers.cmake. I deleted it

tchaikov · 2024-10-09T03:25:21Z

sorry for the latency. i will try to review this change in this week.

tchaikov · 2024-10-13T13:50:56Z

CMakeLists.txt

-      $<${condition}:Sanitizers::undefined_behavior>)
+  set (Seastar_Sanitizers_OPTIONS $<${sanitizers_enabled}:${Sanitizers_COMPILE_OPTIONS}>)
+  foreach (component ${Seastar_SANITIZERS})
+    string (TOUPPER ${component} COMPONENT)


COMPONENT is never used.

tchaikov · 2024-10-13T13:52:00Z

CMakeLists.txt

-    PUBLIC
-      $<${condition}:Sanitizers::address>
-      $<${condition}:Sanitizers::undefined_behavior>)
+  set (Seastar_Sanitizers_OPTIONS $<${sanitizers_enabled}:${Sanitizers_COMPILE_OPTIONS}>)


could you extract this change into a separate commit, and accompany it with rationales in the commit message?

done in this PR

tchaikov · 2024-10-13T13:52:47Z

CMakeLists.txt

+tri_state_option (${Seastar_SANITIZE}
+  DEFAULT_BUILD_TYPES "Debug" "Sanitize"
+  CONDITION sanitizers_enabled)


why are you moving this command up here?

if sanitizers are disabled for this build type we don't need to find them

tchaikov · 2024-10-13T14:06:27Z

cmake/SeastarDependencies.cmake

@@ -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})


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)

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 using configure_file(), so we can have a different SeastarDependencies.cmake for each mode, and then install() it

check all the valid combinations:

address + undefined_behavior

thread

tchaikov · 2024-10-13T14:18:21Z

configure.py

@@ -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')


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?

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.

tchaikov · 2024-11-05T02:27:59Z

@devDDen thank you for addressing the comments. i will try to review your change this week.

devDDen · 2024-11-12T11:25:13Z

@tchaikov have you had a chance to look through proposed changes?

devDDen · 2024-11-26T11:40:55Z

@tchaikov ping

tchaikov · 2024-11-26T11:59:13Z

sorry for the latency. will review your pull request early tomorrow.

tchaikov · 2024-12-02T14:20:02Z

i am still reviewing this change.

tchaikov reviewed Sep 17, 2024

View reviewed changes

devDDen force-pushed the select-sanitizers-in-configure-py branch 2 times, most recently from 2d840e0 to 7305f93 Compare October 4, 2024 19:42

devDDen requested a review from tchaikov October 8, 2024 08:28

tchaikov reviewed Oct 13, 2024

View reviewed changes

devDDen force-pushed the select-sanitizers-in-configure-py branch 2 times, most recently from 7b30680 to 7b7009b Compare November 1, 2024 09:40

devDDen added 2 commits November 1, 2024 13:49

build: Use correct San options in pkgconfig when using multi-config gen

b56f6f4

devDDen force-pushed the select-sanitizers-in-configure-py branch from 7b7009b to 1b11a38 Compare November 1, 2024 10:58

devDDen requested review from tchaikov November 1, 2024 11:06

tchaikov self-assigned this Nov 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: Add options to select sanitizers in configure.py #2437

build: Add options to select sanitizers in configure.py #2437

devDDen commented Sep 16, 2024 •

edited

Loading

devDDen commented Sep 16, 2024

tchaikov Sep 17, 2024

devDDen Oct 4, 2024

tchaikov Sep 17, 2024

devDDen Sep 20, 2024

devDDen Oct 4, 2024

tchaikov Dec 2, 2024

tchaikov Sep 17, 2024

devDDen Oct 4, 2024

tchaikov commented Oct 9, 2024

tchaikov Oct 13, 2024

devDDen Nov 1, 2024

tchaikov Oct 13, 2024

devDDen Nov 1, 2024

tchaikov Oct 13, 2024

devDDen Nov 1, 2024

tchaikov Oct 13, 2024

tchaikov Oct 13, 2024

tchaikov Oct 13, 2024

devDDen Nov 1, 2024

tchaikov commented Nov 5, 2024

devDDen commented Nov 12, 2024

devDDen commented Nov 26, 2024

tchaikov commented Nov 26, 2024

tchaikov commented Dec 2, 2024

		@@ -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'}

build: Add options to select sanitizers in configure.py #2437

Are you sure you want to change the base?

build: Add options to select sanitizers in configure.py #2437

Conversation

devDDen commented Sep 16, 2024 • edited Loading

devDDen commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Nov 5, 2024

devDDen commented Nov 12, 2024

devDDen commented Nov 26, 2024

tchaikov commented Nov 26, 2024

tchaikov commented Dec 2, 2024

devDDen commented Sep 16, 2024 •

edited

Loading