Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

devDDen
Copy link

@devDDen devDDen commented Sep 16, 2024

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
Copy link
Author

devDDen commented Sep 16, 2024

@tchaikov please review

CMakeLists.txt Outdated
DEFAULT_BUILD_TYPES "Debug" "Sanitize"
CONDITION sanitizers_enabled)

if (sanitizers_enabled)
Copy link
Contributor

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

Copy link
Author

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})
Copy link
Contributor

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?

Copy link
Author

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

Copy link
Author

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'}
Copy link
Contributor

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?

Copy link
Author

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

@devDDen 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 devDDen requested a review from tchaikov October 8, 2024 08:28
@tchaikov
Copy link
Contributor

tchaikov commented Oct 9, 2024

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

COMPONENT is never used.

Copy link
Author

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}>)
Copy link
Contributor

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in this PR

Comment on lines +401 to +403
tri_state_option (${Seastar_SANITIZE}
DEFAULT_BUILD_TYPES "Debug" "Sanitize"
CONDITION sanitizers_enabled)
Copy link
Contributor

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?

Copy link
Author

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

@@ -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})
Copy link
Contributor

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:

  1. At the time this file is processed, Seastar_SANITIZERS is empty.

  2. This may result in an incomplete Seastar CMake configuration for the parent project.

  3. If the user doesn't have sanitizer libraries installed, find_package(Seastar) still succeeds.

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

Copy link
Contributor

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

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')
Copy link
Contributor

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@devDDen devDDen force-pushed the select-sanitizers-in-configure-py branch 2 times, most recently from 7b30680 to 7b7009b Compare November 1, 2024 09:40
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 devDDen force-pushed the select-sanitizers-in-configure-py branch from 7b7009b to 1b11a38 Compare November 1, 2024 10:58
@tchaikov
Copy link
Contributor

tchaikov commented Nov 5, 2024

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

@devDDen
Copy link
Author

devDDen commented Nov 12, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants