[PATCH master v1 1/3] build: Add the leak sanitizer

[Jesse Haber-Kucharsky]

This sanitizer was included in the old build-system, and its removal was
not intentional.

Signed-off-by: Jesse Haber-Kucharsky <jhab...@scylladb.com>
---
CMakeLists.txt | 1 +
cmake/FindSanitizers.cmake | 18 ++++++++++++++++++
2 files changed, 19 insertions(+)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 292a022d..fe8c59c4 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -589,6 +589,7 @@ endif()

set (Seastar_DEVELOPMENT_LIBS
debug Sanitizers::address
+ debug Sanitizers::leak
debug Sanitizers::undefined_behavior)

if (CMAKE_BUILD_TYPE STREQUAL "Debug")
diff --git a/cmake/FindSanitizers.cmake b/cmake/FindSanitizers.cmake
index 316f88ea..bff694b7 100644
--- a/cmake/FindSanitizers.cmake
+++ b/cmake/FindSanitizers.cmake
@@ -29,6 +29,13 @@ if (Sanitizers_ADDRESS_FOUND)
set (Sanitizers_ADDRESS_COMPILER_OPTIONS -fsanitize=address)
endif ()

+set (CMAKE_REQUIRED_FLAGS -fsanitize=leak)
+check_cxx_source_compiles ("int main() {}" Sanitizers_LEAK_FOUND)
+
+if (Sanitizers_LEAK_FOUND)
+ set (Sanitizers_LEAK_COMPILER_OPTIONS -fsanitize=leak)
+endif ()
+
set (CMAKE_REQUIRED_FLAGS -fsanitize=undefined)
check_cxx_source_compiles ("int main() {}" Sanitizers_UNDEFINED_BEHAVIOR_FOUND)

@@ -39,6 +46,7 @@ endif ()

set (Sanitizers_COMPILER_OPTIONS
${Sanitizers_ADDRESS_COMPILER_OPTIONS}
+ ${Sanitizers_LEAK_COMPILER_OPTIONS}
${Sanitizers_UNDEFINED_BEHAVIOR_COMPILER_OPTIONS})

file (READ ${CMAKE_CURRENT_LIST_DIR}/code_tests/Sanitizers_fiber_test.cc _sanitizers_fiber_test_code)
@@ -50,6 +58,7 @@ include (FindPackageHandleStandardArgs)
find_package_handle_standard_args (Sanitizers
REQUIRED_VARS
Sanitizers_ADDRESS_COMPILER_OPTIONS
+ Sanitizers_LEAK_COMPILER_OPTIONS
Sanitizers_UNDEFINED_BEHAVIOR_COMPILER_OPTIONS)

if (Sanitizers_FOUND)
@@ -62,6 +71,15 @@ if (Sanitizers_FOUND)
INTERFACE_LINK_LIBRARIES ${Sanitizers_ADDRESS_COMPILER_OPTIONS})
endif ()

+ if (NOT (TARGET Sanitizers::leak))
+ add_library (Sanitizers::leak INTERFACE IMPORTED)
+
+ set_target_properties (Sanitizers::leak
+ PROPERTIES
+ INTERFACE_COMPILE_OPTIONS ${Sanitizers_LEAK_COMPILER_OPTIONS}
+ INTERFACE_LINK_LIBRARIES ${Sanitizers_LEAK_COMPILER_OPTIONS})
+ endif ()
+
if (NOT (TARGET Sanitizers::undefined_behavior))
add_library (Sanitizers::undefined_behavior INTERFACE IMPORTED)

--
2.20.1

[Jesse Haber-Kucharsky]

These changes should be self-explanatory.

After the third patch is applied, the `futures` test fails AddresssSanitizer. I'm not sure if this is a separate Seastar bug, or a bug in this series.

The output is below.

Also available at https://github.com/hakuch/seastar.git on the jhk/sanitizers_stuff/v1 branch.


$ ./test.py --mode=debug --verbose --name=futures
['cmake', '/home/jhaberku/src/seastar/build/debug', '-DSeastar_TEST_TIMEOUT=300', '-DSeastar_EXECUTE_ONLY_FAST_TESTS=no', '-DSeastar_UNIT_TEST_SMP=2', '-DSeastar_JENKINS=']
-- Boost version: 1.66.0
-- Found the following Boost libraries:
-- filesystem
-- program_options
-- thread
-- unit_test_framework
-- system
-- chrono
-- date_time
-- atomic
-- Could NOT find dpdk (missing: dpdk_INCLUDE_DIR dpdk_PMD_VMXNET3_UIO_LIBRARY dpdk_PMD_I40E_LIBRARY dpdk_PMD_IXGBE_LIBRARY dpdk_PMD_E1000_LIBRARY dpdk_PMD_BNXT_LIBRARY dpdk_PMD_RING_LIBRARY dpdk_PMD_CXGBE_LIBRARY dpdk_PMD_ENA_LIBRARY dpdk_PMD_ENIC_LIBRARY dpdk_PMD_FM10K_LIBRARY dpdk_PMD_NFP_LIBRARY dpdk_PMD_QEDE_LIBRARY dpdk_RING_LIBRARY dpdk_KVARGS_LIBRARY dpdk_MEMPOOL_LIBRARY dpdk_MEMPOOL_RING_LIBRARY dpdk_PMD_SFC_EFX_LIBRARY dpdk_HASH_LIBRARY dpdk_CMDLINE_LIBRARY dpdk_MBUF_LIBRARY dpdk_CFGFILE_LIBRARY dpdk_EAL_LIBRARY dpdk_ETHDEV_LIBRARY)
-- Configuring done
-- Generating done
-- Build files have been written to: /home/jhaberku/src/seastar/build/debug
['ctest', '/home/jhaberku/src/seastar/build/debug', '-E', 'Seastar.dist', '--verbose', '-R', 'futures']
UpdateCTestConfiguration from :/home/jhaberku/src/seastar/build/debug/DartConfiguration.tcl
UpdateCTestConfiguration from :/home/jhaberku/src/seastar/build/debug/DartConfiguration.tcl
Test project /home/jhaberku/src/seastar/build/debug
Constructing a list of tests
Done constructing a list of tests
Updating test list for fixtures
Added 0 tests to meet fixture requirements
Checking test dependency graph...
Checking test dependency graph end
test 21
Start 21: Seastar.unit.futures

21: Test command: /usr/bin/cmake "--build" "/home/jhaberku/src/seastar/build/debug" "--target" "test_unit_futures_run"
21: Test timeout computed to be: 300
21: [0/1] cd /home/jhaberku/src/seastar/build/debug/tests/unit && /home/jhaberku/src/seastar/build/debug/tests/unit/futures -- -c 2
21: Reactor stalled for 2609 ms on shard 0.
21: Backtrace:
21: /lib64/libasan.so.5+0x000000000009d87b
21: 0x00000000010bbe18
21: 0x0000000001085fd9
21: 0x0000000000c2db66
21: 0x0000000000c40125
21: 0x0000000000c3cbbe
21: 0x0000000000c3cc78
21: 0x0000000000c3fd12
21: 0x00007f80083cd02f
21: 0x000000000094e37f
21: 0x00000000011cac92
21: 0x0000000001150336
21: 0x00000000010e0ce6
21: 0x000000000110fb1b
21: 0x00000000010b073a
21: 0x000000000106f040
21: 0x0000000000cc9e68
21: 0x00000000005d8ce1
21: 0x0000000000525f76
21: 0x0000000000442c82
21: 0x0000000000af20f8
21: 0x0000000000af31f6
21: 0x0000000000b1111a
21: 0x0000000000afb93a
21: 0x0000000000b04715
21: 0x0000000000b1111a
21: 0x0000000000af8f93
21: 0x0000000000b0c4d3
21: 0x0000000000cace1c
21: 0x0000000000cb24c5
21: 0x0000000000cba5e9
21: 0x0000000000b2a8a4
21: 0x0000000000afa266
21: 0x0000000000b060a0
21: 0x0000000000b73d96
21: 0x0000000000c182aa
21: /lib64/libpthread.so.0+0x000000000000858d
21: /lib64/libc.so.6+0x00000000000fd6a2
21: ==8871==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7f8002dfe880; bottom 0x7f7ffa671000; size: 0x00000878d880 (142137472)
21: False positive error reports may follow
21: For details see https://github.com/google/sanitizers/issues/189
21: =================================================================
21: ==8871==ERROR: AddressSanitizer: stack-buffer-underflow on address 0x7f7ffa6728c0 at pc 0x00000082d469 bp 0x7f7ffa672760 sp 0x7f7ffa672750
21: WRITE of size 8 at 0x7f7ffa6728c0 thread T1
21: #0 0x82d468 in seastar::future_state<>::any::any() ../../include/seastar/core/future.hh:298
21: #1 0x82d528 in seastar::future_state<>::future_state() ../../include/seastar/core/future.hh:303
21: #2 0x8aa4d1 in seastar::continuation_base<>::continuation_base() ../../include/seastar/core/future.hh:397
21: #3 0x90954e in seastar::future<>::thread_wake_task::thread_wake_task(seastar::thread_context*, seastar::future<>*) ../../include/seastar/core/future.hh:892
21: #4 0x89f4de in seastar::future<>::do_wait() ../../include/seastar/core/future.hh:904
21: #5 0x46ac86 in seastar::future<>::get() ../../include/seastar/core/future.hh:852
21: #6 0x46ac86 in operator() ../../tests/unit/futures_test.cc:442
21: #7 0x6f2ffc in apply ../../include/seastar/core/apply.hh:35
21: #8 0x6f3076 in apply<test_parallel_for_each::run_test_case()::<lambda()> > ../../include/seastar/core/apply.hh:43
21: #9 0x6f3187 in apply_tuple<test_parallel_for_each::run_test_case()::<lambda()> > ../../include/seastar/core/future.hh:1343
21: #10 0x685c05 in apply<test_parallel_for_each::run_test_case()::<lambda()> > ../../include/seastar/core/future.hh:1377
21: #11 0x5f7246 in operator() ../../include/seastar/core/thread.hh:315
21: #12 0x7725c4 in _M_invoke /usr/include/c++/8/bits/std_function.h:297
21: #13 0xb73d96 in std::function<void ()>::operator()() const /usr/include/c++/8/bits/std_function.h:687
21: #14 0x14daa26 in seastar::thread_context::main() ../../src/core/thread.cc:317
21:
21: 0x7f7ffa6728c0 is located 129216 bytes inside of 135168-byte region [0x7f7ffa653000,0x7f7ffa674000)
21: allocated by thread T1 here:
21: #0 0x7f8008676420 in aligned_alloc (/lib64/libasan.so.5+0xf0420)
21: #1 0x14d5b8d in seastar::thread_context::make_stack() ../../src/core/thread.cc:173
21: #2 0x14d42b2 in seastar::thread_context::thread_context(seastar::thread_attributes, std::function<void ()>) ../../src/core/thread.cc:157
21: #3 0x6f351e in make_unique<seastar::thread_context, seastar::thread_attributes, seastar::async(seastar::thread_attributes, Func&&, Args&& ...) [with Func = test_parallel_for_each::run_test_case()::<lambda()>; Args = {}]::<lambda(seastar::async(seastar::thread_attributes, Func&&, Args&& ...) [with Func = test_parallel_for_each::run_test_case()::<lambda()>; Args = {}]::work&)> mutable::<lambda()>&> /usr/include/c++/8/bits/unique_ptr.h:831
21: #4 0x685d73 in thread<seastar::async(seastar::thread_attributes, Func&&, Args&& ...) [with Func = test_parallel_for_each::run_test_case()::<lambda()>; Args = {}]::<lambda(seastar::async(seastar::thread_attributes, Func&&, Args&& ...) [with Func = test_parallel_for_each::run_test_case()::<lambda()>; Args = {}]::work&)> mutable::<lambda()> > ../../include/seastar/core/thread.hh:261
21: #5 0x5f87cc in operator() ../../include/seastar/core/thread.hh:314
21: #6 0x5fa18e in do_with<seastar::async(seastar::thread_attributes, Func&&, Args&& ...) [with Func = test_parallel_for_each::run_test_case()::<lambda()>; Args = {}]::work, seastar::async(seastar::thread_attributes, Func&&, Args&& ...) [with Func = test_parallel_for_each::run_test_case()::<lambda()>; Args = {}]::<lambda(seastar::async(seastar::thread_attributes, Func&&, Args&& ...) [with Func = test_parallel_for_each::run_test_case()::<lambda()>; Args = {}]::work&)> > ../../include/seastar/core/do_with.hh:94
21: #7 0x5fc4d9 in async<test_parallel_for_each::run_test_case()::<lambda()> > ../../include/seastar/core/thread.hh:320
21: #8 0x54e4f1 in async<test_parallel_for_each::run_test_case()::<lambda()> > ../../include/seastar/core/thread.hh:336
21: #9 0x46e971 in test_parallel_for_each::run_test_case() ../../tests/unit/futures_test.cc:447
21: #10 0xaf20f8 in operator() ../../src/testing/seastar_test.cc:43
21: #11 0xaf31f6 in _M_invoke /usr/include/c++/8/bits/std_function.h:283
21: #12 0xb1111a in std::function<seastar::future<> ()>::operator()() const /usr/include/c++/8/bits/std_function.h:687
21: #13 0xafb93a in operator() ../../src/testing/test_runner.cc:88
21: #14 0xb04715 in _M_invoke /usr/include/c++/8/bits/std_function.h:283
21: #15 0xb1111a in std::function<seastar::future<> ()>::operator()() const /usr/include/c++/8/bits/std_function.h:687
21: #16 0xaf8f93 in operator() ../../src/testing/test_runner.cc:68
21: #17 0xb0c4d3 in run_and_dispose ../../include/seastar/core/future-util.hh:462
21: #18 0xcace1c in seastar::reactor::run_tasks(seastar::reactor::task_queue&) ../../src/core/reactor.cc:3524
21: #19 0xcb24c5 in seastar::reactor::run_some_tasks() ../../src/core/reactor.cc:3949
21: #20 0xcba5e9 in seastar::reactor::run() ../../src/core/reactor.cc:4092
21: #21 0xb2a8a4 in seastar::app_template::run_deprecated(int, char**, std::function<void ()>&&) ../../src/core/app-template.cc:185
21: #22 0xafa266 in operator() ../../src/testing/test_runner.cc:63
21: #23 0xb060a0 in _M_invoke /usr/include/c++/8/bits/std_function.h:297
21: #24 0xb73d96 in std::function<void ()>::operator()() const /usr/include/c++/8/bits/std_function.h:687
21: #25 0xc182aa in seastar::posix_thread::start_routine(void*) ../../src/core/posix.cc:52
21: #26 0x7f80083c258d in start_thread (/lib64/libpthread.so.0+0x858d)
21:
21: Thread T1 created by T0 here:
21: #0 0x7f80085d2043 in __interceptor_pthread_create (/lib64/libasan.so.5+0x4c043)
21: #1 0xc18c30 in seastar::posix_thread::posix_thread(seastar::posix_thread::attr, std::function<void ()>) ../../src/core/posix.cc:83
21: #2 0xc1845a in seastar::posix_thread::posix_thread(std::function<void ()>) ../../src/core/posix.cc:57
21: #3 0xb0004e in make_unique<seastar::posix_thread, seastar::testing::test_runner::start(int, char**)::<lambda()> > /usr/include/c++/8/bits/unique_ptr.h:831
21: #4 0xafa8f6 in seastar::testing::test_runner::start(int, char**) ../../src/testing/test_runner.cc:61
21: #5 0xaedb8f in init_unit_test_suite ../../src/testing/entry_point.cc:45
21: #6 0x7f800850fc4b (/lib64/libboost_unit_test_framework.so.1.66.0+0x54c4b)
21:
21: SUMMARY: AddressSanitizer: stack-buffer-underflow ../../include/seastar/core/future.hh:298 in seastar::future_state<>::any::any()
21: Shadow bytes around the buggy address:
21: 0x0ff07f4c64c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
21: 0x0ff07f4c64d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
21: 0x0ff07f4c64e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
21: 0x0ff07f4c64f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
21: 0x0ff07f4c6500: 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00 f2
21: =>0x0ff07f4c6510: f2 f2 f2 f2 f2 f2 00 00[f1]f1 00 f2 f2 f2 f3 f3
21: 0x0ff07f4c6520: f3 f3 00 00 00 00 00 00 00 00 00 00 00 00 00 00
21: 0x0ff07f4c6530: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
21: 0x0ff07f4c6540: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
21: 0x0ff07f4c6550: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
21: 0x0ff07f4c6560: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
21: Shadow byte legend (one shadow byte represents 8 application bytes):
21: Addressable: 00
21: Partially addressable: 01 02 03 04 05 06 07
21: Heap left redzone: fa
21: Freed heap region: fd
21: Stack left redzone: f1
21: Stack mid redzone: f2
21: Stack right redzone: f3
21: Stack after return: f5
21: Stack use after scope: f8
21: Global redzone: f9
21: Global init order: f6
21: Poisoned by user: f7
21: Container overflow: fc
21: Array cookie: ac
21: Intra object redzone: bb
21: ASan internal: fe
21: Left alloca redzone: ca
21: Right alloca redzone: cb
21: ==8871==ABORTING
21: FAILED: tests/unit/CMakeFiles/test_unit_futures_run
21: cd /home/jhaberku/src/seastar/build/debug/tests/unit && /home/jhaberku/src/seastar/build/debug/tests/unit/futures -- -c 2
21: ninja: build stopped: subcommand failed.
1/1 Test #21: Seastar.unit.futures .............***Failed 5.85 sec

0% tests passed, 1 tests failed out of 1

Total Test time (real) = 5.85 sec

The following tests FAILED:
21 - Seastar.unit.futures (Failed)
Errors while running CTest
Traceback (most recent call last):
File "./test.py", line 68, in <module>
run_tests(mode)
File "./test.py", line 65, in run_tests
subprocess.check_call(CTEST_ARGS, shell=False, cwd=BUILD_PATH)
File "/usr/lib64/python3.7/subprocess.py", line 347, in check_call
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['ctest', '/home/jhaberku/src/seastar/build/debug', '-E', 'Seastar.dist', '--verbose', '-R', 'futures']' returned non-zero exit status 8.

Compilation exited abnormally with code 1 at Mon Feb 4 12:22:33

Jesse Haber-Kucharsky (3):
build: Add the leak sanitizer
build: Make the use of sanitizers transitive
build: Allow sanitizers to be disabled

CMakeLists.txt | 48 ++++++++++++++++++++------------------
cmake/FindSanitizers.cmake | 18 ++++++++++++++
configure.py | 6 +++++
pkgconfig/seastar.pc.in | 3 +--
4 files changed, 50 insertions(+), 25 deletions(-)

--
2.20.1

[Jesse Haber-Kucharsky]

While some sanitizers do not have to be enabled for programs which link
against libraries with sanitizers enabled, we can be conservative and
ensure that sanitizers are enabled transitively all the time.

This was already the way that pkg-config worked, and this change ensures
that consumers of Seastar using CMake behave the same way.

We can eliminate the `sanitizers_cflags` pkg-config variable because the
sanitizers are part of the expansion of `INTERFACE_COMPILE_OPTIONS`.

Signed-off-by: Jesse Haber-Kucharsky <jhab...@scylladb.com>
---

CMakeLists.txt | 18 +++++-------------
pkgconfig/seastar.pc.in | 3 +--
2 files changed, 6 insertions(+), 15 deletions(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index fe8c59c4..d2680d1e 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -587,11 +587,6 @@ if (Seastar_COMPRESS_DEBUG)
list (APPEND Seastar_DEVELOPMENT_FLAGS -gz)
endif()

-set (Seastar_DEVELOPMENT_LIBS
- debug Sanitizers::address
- debug Sanitizers::leak
- debug Sanitizers::undefined_behavior)
-

if (CMAKE_BUILD_TYPE STREQUAL "Debug")

if (NOT Sanitizers_FOUND)
message (FATAL_ERROR "Sanitizers are required for debug mode!")
@@ -600,6 +595,9 @@ endif ()

target_link_libraries (seastar
PUBLIC
+ debug Sanitizers::address
+ debug Sanitizers::leak
+ debug Sanitizers::undefined_behavior
Boost::boost
Boost::program_options
Boost::thread
@@ -747,9 +745,6 @@ target_compile_definitions (seastar
target_compile_options (seastar
PRIVATE ${Seastar_DEVELOPMENT_FLAGS})

-target_link_libraries (seastar
- PRIVATE ${Seastar_DEVELOPMENT_LIBS})
-
add_library (seastar_with_flags INTERFACE)

target_compile_definitions (seastar_with_flags
@@ -759,9 +754,7 @@ target_compile_options (seastar_with_flags
INTERFACE ${Seastar_DEVELOPMENT_FLAGS})

target_link_libraries (seastar_with_flags
- INTERFACE
- seastar
- ${Seastar_DEVELOPMENT_LIBS})
+ INTERFACE seastar)

#
# The testing library.
@@ -791,8 +784,7 @@ if (Seastar_INSTALL OR Seastar_TESTING)
target_link_libraries (seastar_testing
PUBLIC
Boost::unit_test_framework
- seastar
- PRIVATE ${Seastar_DEVELOPMENT_LIBS})
+ seastar)
endif ()

#
diff --git a/pkgconfig/seastar.pc.in b/pkgconfig/seastar.pc.in
index 35d076f1..f5f8294c 100644
--- a/pkgconfig/seastar.pc.in
+++ b/pkgconfig/seastar.pc.in
@@ -33,7 +33,6 @@ lksctp_tools_cflags=-I$<JOIN:@lksctp-tools_INCLUDE_DIRS@, -I>
lksctp_tools_libs=$<JOIN:@lksctp-tools_LIBRARIES@, >
numactl_cflags=-I$<JOIN:@numactl_INCLUDE_DIRS@, -I>
numactl_libs=$<JOIN:@numactl_LIBRARIES@, >
-sanitizers_cflags=$<JOIN:@Sanitizers_PKGCONFIG_COMPILER_OPTIONS@, >

# Us.
seastar_cflags=${seastar_include_flags} $<JOIN:$<TARGET_PROPERTY:seastar,INTERFACE_COMPILE_OPTIONS>, > -D$<JOIN:$<TARGET_PROPERTY:seastar,INTERFACE_COMPILE_DEFINITIONS>, -D>
@@ -42,6 +41,6 @@ seastar_libs=${libdir}/$<TARGET_FILE_NAME:seastar> @Seastar_SPLIT_DWARF_FLAG@
Requires: liblz4 >= 1.8.0
Requires.private: gnutls >= 3.5.18, protobuf >= 3.3.0, hwloc >= 1.11.5, yaml-cpp >= 0.5.3
Conflicts:
-Cflags: ${boost_cflags} ${c_ares_cflags} ${cryptopp_cflags} ${fmt_cflags} ${lksctp_tools_cflags} ${numactl_cflags} ${sanitizers_cflags} ${seastar_cflags}
+Cflags: ${boost_cflags} ${c_ares_cflags} ${cryptopp_cflags} ${fmt_cflags} ${lksctp_tools_cflags} ${numactl_cflags} ${seastar_cflags}
Libs: ${seastar_libs} ${boost_program_options_libs} ${boost_thread_libs} ${c_ares_libs} ${cryptopp_libs} ${fmt_libs}
Libs.private: ${dl_libs} ${rt_libs} ${boost_filesystem_libs} ${boost_thread_libs} ${dpdk_libs} ${lksctp_tools_libs} ${numactl_libs} ${stdatomic_libs} ${stdfilesystem_libs}
--
2.20.1

[Jesse Haber-Kucharsky]

Sanitizers are enabled by default for debug builds, but can be disabled
via the `Seastar_SANITIZERS` variable or by passing
`--disable-santizers` to `configure.py`.

Signed-off-by: Jesse Haber-Kucharsky <jhab...@scylladb.com>
---

CMakeLists.txt | 37 +++++++++++++++++++++++--------------
configure.py | 6 ++++++
2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index d2680d1e..6befa240 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -167,6 +167,10 @@ option (Seastar_NUMA
"Enable NUMA support."
ON)

+option (Seastar_SANITIZERS
+ "Enable code sanitizers. Sanitizers are only included for debug builds, even when enabled."
+ ON)
+
option (Seastar_STD_OPTIONAL_VARIANT_STRINGVIEW
"Use the non-experimental versions of `optional`, `variant`, and `string_view`. Requires C++17."
OFF)
@@ -587,17 +591,8 @@ if (Seastar_COMPRESS_DEBUG)

list (APPEND Seastar_DEVELOPMENT_FLAGS -gz)
endif()

-if (CMAKE_BUILD_TYPE STREQUAL "Debug")
- if (NOT Sanitizers_FOUND)
- message (FATAL_ERROR "Sanitizers are required for debug mode!")
- endif ()
-endif ()
-
target_link_libraries (seastar
PUBLIC

- debug Sanitizers::address
- debug Sanitizers::leak

- debug Sanitizers::undefined_behavior

Boost::boost
Boost::program_options
Boost::thread

@@ -657,10 +652,6 @@ if (Seastar_STD_OPTIONAL_VARIANT_STRINGVIEW)
PUBLIC SEASTAR_USE_STD_OPTIONAL_VARIANT_STRINGVIEW)
endif ()

-if (Sanitizers_FIBER_SUPPORT)
- list (APPEND Seastar_DEVELOPMENT_DEFINES SEASTAR_HAVE_ASAN_FIBER_SUPPORT)
-endif ()
-
if (Seastar_ALLOC_PAGE_SIZE)
target_compile_definitions (seastar
PUBLIC SEASTAR_OVERRIDE_ALLOCATOR_PAGE_SIZE=${Seastar_ALLOC_PAGE_SIZE})
@@ -725,6 +716,25 @@ if (Seastar_NUMA)
PRIVATE numactl::numactl)
endif ()

+if (Seastar_SANITIZERS AND (CMAKE_BUILD_TYPE STREQUAL "Debug"))
+ if (NOT Sanitizers_FOUND)
+ message (FATAL_ERROR "Sanitizers are enabled but are not available!")
+ endif ()
+
+ target_link_libraries (seastar
+ PUBLIC
+ Sanitizers::address
+ Sanitizers::leak
+ Sanitizers::undefined_behavior)
+
+ target_compile_definitions (seastar
+ PUBLIC SEASTAR_ASAN_EANBLED)
+
+ if (Sanitizers_FIBER_SUPPORT)
+ list (APPEND Seastar_DEVELOPMENT_DEFINES SEASTAR_HAVE_ASAN_FIBER_SUPPORT)
+ endif ()
+endif ()
+
if (lz4_HAVE_COMPRESS_DEFAULT)
list (APPEND Seastar_DEVELOPMENT_DEFINES SEASTAR_HAVE_LZ4_COMPRESS_DEFAULT)
endif ()
@@ -735,7 +745,6 @@ if (CMAKE_BUILD_TYPE STREQUAL "Debug")
SEASTAR_DEBUG
SEASTAR_DEFAULT_ALLOCATOR
SEASTAR_DEBUG_SHARED_PTR
- SEASTAR_ASAN_ENABLED
SEASTAR_THREAD_STACK_GUARDS)
endif ()

diff --git a/configure.py b/configure.py
index 9b70ef46..90c68e37 100755
--- a/configure.py
+++ b/configure.py
@@ -96,6 +96,11 @@ add_tristate(
name = 'exception-scalability-workaround',
dest = 'exception_workaround',
help = 'a workaround for C++ exception scalability issues by overriding the definition of `dl_iterate_phdr`')
+add_tristate(
+ arg_parser,
+ name = 'sanitizers',
+ dest = 'sanitizers',
+ help = 'sanitizers for debug builds')
arg_parser.add_argument('--allocator-page-size', dest='allocator_page_size', type=int, help='override allocator page size')
arg_parser.add_argument('--without-tests', dest='exclude_tests', action='store_true', help='Do not build tests by default')
arg_parser.add_argument('--without-apps', dest='exclude_apps', action='store_true', help='Do not build applications by default')
@@ -158,6 +163,7 @@ def configure_mode(mode):
tr(args.exception_workaround, 'EXCEPTION_SCALABILITY_WORKAROUND', value_when_none='yes'),
tr(args.allocator_page_size, 'ALLOCATOR_PAGE_SIZE'),
tr(args.cpp17_goodies, 'STD_OPTIONAL_VARIANT_STRINGVIEW'),
+ tr(args.sanitizers, 'SANITIZERS', value_when_none='yes'),
]

# Generate a new build by pointing to the source directory.
--
2.20.1

[Avi Kivity]

On 04/02/2019 19.31, Jesse Haber-Kucharsky wrote:
> These changes should be self-explanatory.
>
> After the third patch is applied, the `futures` test fails AddresssSanitizer. I'm not sure if this is a separate Seastar bug, or a bug in this series.

Either way, we can't apply something that causes the tests to fail.

[Rafael Avila de Espindola]

This failed for me:

-- Performing Test Sanitizers_LEAK_FOUND
-- Performing Test Sanitizers_LEAK_FOUND - Failed

The reason is that liblsan was not installed, so I guess this patch
should add it to install-dependencies.sh (probably on top of Avi's
refactoring patch).

Cheers,
Rafael

> --
> You received this message because you are subscribed to the Google Groups "seastar-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to seastar-dev...@googlegroups.com.
> To post to this group, send email to seast...@googlegroups.com.
> Visit this group at https://groups.google.com/group/seastar-dev.
> To view this discussion on the web visit https://groups.google.com/d/msgid/seastar-dev/8791e0a5b8a5b02cd45deae3028c92333da10a46.1549300868.git.jhaberku%40scylladb.com.
> For more options, visit https://groups.google.com/d/optout.

[Rafael Avila de Espindola]

Any particular reason for the option?

Cheers,
Rafael

Jesse Haber-Kucharsky <jhab...@scylladb.com> writes:

> --
> You received this message because you are subscribed to the Google Groups "seastar-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to seastar-dev...@googlegroups.com.
> To post to this group, send email to seast...@googlegroups.com.
> Visit this group at https://groups.google.com/group/seastar-dev.

> To view this discussion on the web visit https://groups.google.com/d/msgid/seastar-dev/fec3ea95ee4779574e739c1f111a73599deb6abb.1549300868.git.jhaberku%40scylladb.com.

[Jesse Haber-Kucharsky]

On Mon, Feb 4, 2019 at 2:01 PM Rafael Avila de Espindola <espi...@scylladb.com> wrote:

This failed for me:

-- Performing Test Sanitizers_LEAK_FOUND
-- Performing Test Sanitizers_LEAK_FOUND - Failed

The reason is that liblsan was not installed, so I guess this patch
should add it to install-dependencies.sh (probably on top of Avi's
refactoring patch).

Good point! Thanks.

[Jesse Haber-Kucharsky]

On Mon, Feb 4, 2019 at 2:08 PM Rafael Avila de Espindola <espi...@scylladb.com> wrote:

Any particular reason for the option?

This way, we can support older systems without sanitizers and also users who do not wish to install the lib*san packages.

[Rafael Avila de Espindola]

Jesse Haber-Kucharsky <jhab...@scylladb.com> writes:

> + target_compile_definitions (seastar
> + PUBLIC SEASTAR_ASAN_EANBLED)
> +

Typo, should be SEASTAR_ASAN_ENABLED.

Cheers,
Rafael

[Jesse Haber-Kucharsky]

Blah. That's probably the source of the test failure.

Thanks! 

 

Cheers,
Rafael

[Rafael Avila de Espindola]

Jesse Haber-Kucharsky <jhab...@scylladb.com> writes:

> On Mon, Feb 4, 2019 at 2:08 PM Rafael Avila de Espindola <
> espi...@scylladb.com> wrote:
>
>> Any particular reason for the option?
>>
>
> This way, we can support older systems without sanitizers and also users
> who do not wish to install the lib*san packages.

But this is basically Dev + -g, no?

Maybe make the option more generic. Right not there is no way of
enabling sanitizers in a release build for example, but that can be
convenient for running tests.

Also, the option is a boolean at the cmake level but a tristate in
configure.py. It should probably be a tristate in cmake too.

-DSeastar_SANITIZERS=ON -> on on all CMAKE_BUILD_TYPE
-DSeastar_SANITIZERS=OFF -> off on all CMAKE_BUILD_TYPE
-> on only on CMAKE_BUILD_TYPE=Debug

Cheers,
Rafael

[Jesse Haber-Kucharsky]

On Mon, Feb 4, 2019 at 2:35 PM Rafael Avila de Espindola <espi...@scylladb.com> wrote:

Jesse Haber-Kucharsky <jhab...@scylladb.com> writes:

> On Mon, Feb 4, 2019 at 2:08 PM Rafael Avila de Espindola <
> espi...@scylladb.com> wrote:
>
>> Any particular reason for the option?
>>
>
> This way, we can support older systems without sanitizers and also users
> who do not wish to install the lib*san packages.

But this is basically Dev + -g, no?

Maybe make the option more generic. Right not there is no way of
enabling sanitizers in a release build for example, but that can be
convenient for running tests.

It makes sense to allow the sanitizers on release builds too.

 

Also, the option is a boolean at the cmake level but a tristate in
configure.py. It should probably be a tristate in cmake too.

-DSeastar_SANITIZERS=ON  -> on on all CMAKE_BUILD_TYPE
-DSeastar_SANITIZERS=OFF -> off on all CMAKE_BUILD_TYPE
                         -> on only on CMAKE_BUILD_TYPE=Debug

This is true, but it's a problem with all of the existing CMake options.

Let's put this patch on hold for now. I don't think it's a pressing issue, and the redundancy in `configure.py` and `CMakeLists.txt` is indeed error prone and more worthy of immediate attention.

I'll fix the tests and republish the series without the last patch.

 

Cheers,
Rafael

[Jesse Haber-Kucharsky]

This series is rebased against the series titled "Modernize dependency management".

Also available at https://github.com/hakuch/seastar.git on the jhk/sanitizers_stuff/v2 branch.

Changes since v1:

- Rebase against "Modernize dependency management"

- Add `lsan` to the list of required dependencies

- Drop patch which makes sanitizers optional

Jesse Haber-Kucharsky (2):

build: Add the leak sanitizer
build: Make the use of sanitizers transitive

CMakeLists.txt | 23 +++++------------------
cmake/FindSanitizers.cmake | 18 ++++++++++++++++++
install-dependencies.sh | 2 ++
pkgconfig/seastar.pc.in | 3 +--
4 files changed, 26 insertions(+), 20 deletions(-)

--
2.20.1

[Jesse Haber-Kucharsky]

This sanitizer was included in the old build-system, and its removal was
not intentional.

Signed-off-by: Jesse Haber-Kucharsky <jhab...@scylladb.com>
---

CMakeLists.txt | 1 +

cmake/FindSanitizers.cmake | 18 ++++++++++++++++++
install-dependencies.sh | 2 ++

3 files changed, 21 insertions(+)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 4d948e8f..787c5b2d 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -575,6 +575,7 @@ set (Seastar_DEVELOPMENT_FLAGS

set (Seastar_DEVELOPMENT_LIBS
debug Sanitizers::address
+ debug Sanitizers::leak
debug Sanitizers::undefined_behavior)

if (CMAKE_BUILD_TYPE STREQUAL "Debug")

+ INTERFACE_LINK_LIBRARIES ${Sanitizers_LEAK_COMPILER_OPTIONS})
+ endif ()
+

if (NOT (TARGET Sanitizers::undefined_behavior))
add_library (Sanitizers::undefined_behavior INTERFACE IMPORTED)

diff --git a/install-dependencies.sh b/install-dependencies.sh
index 66069c53..e415dab5 100755
--- a/install-dependencies.sh
+++ b/install-dependencies.sh
@@ -84,6 +84,7 @@ fedora_packages=(
boost-devel
libubsan
libasan
+ liblsan
libatomic
)

@@ -95,6 +96,7 @@ centos_packages=(
devtoolset-8-gcc-c++
devtoolset-8-libubsan
devtoolset-8-libasan
+ devtoolset-8-liblsan
devtoolset-8-libatomic
)

--
2.20.1

[Jesse Haber-Kucharsky]

While some sanitizers do not have to be enabled for programs which link
against libraries with sanitizers enabled, we can be conservative and
ensure that sanitizers are enabled transitively all the time.

This was already the way that pkg-config worked, and this change ensures
that consumers of Seastar using CMake behave the same way.

We can eliminate the `sanitizers_cflags` pkg-config variable because the
sanitizers are part of the expansion of `INTERFACE_COMPILE_OPTIONS`.

Signed-off-by: Jesse Haber-Kucharsky <jhab...@scylladb.com>
---

CMakeLists.txt | 24 +++++-------------------
pkgconfig/seastar.pc.in | 3 +--
2 files changed, 6 insertions(+), 21 deletions(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 787c5b2d..aa6c216e 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -573,11 +573,6 @@ set (Seastar_DEVELOPMENT_FLAGS
-Wall
-Werror)

-set (Seastar_DEVELOPMENT_LIBS

- debug Sanitizers::address
- debug Sanitizers::leak

- debug Sanitizers::undefined_behavior)
-

if (CMAKE_BUILD_TYPE STREQUAL "Debug")

if (NOT Sanitizers_FOUND)

message (FATAL_ERROR "Sanitizers are required for debug mode!")

@@ -586,6 +581,9 @@ endif ()

target_link_libraries (seastar
PUBLIC
+ debug Sanitizers::address
+ debug Sanitizers::leak
+ debug Sanitizers::undefined_behavior

Boost::boost
Boost::program_options
Boost::thread

@@ -722,9 +720,6 @@ target_compile_definitions (seastar

target_compile_options (seastar
PRIVATE ${Seastar_DEVELOPMENT_FLAGS})

-target_link_libraries (seastar
- PRIVATE ${Seastar_DEVELOPMENT_LIBS})
-
add_library (seastar_with_flags INTERFACE)

target_compile_definitions (seastar_with_flags

@@ -734,9 +729,7 @@ target_compile_options (seastar_with_flags

INTERFACE ${Seastar_DEVELOPMENT_FLAGS})

target_link_libraries (seastar_with_flags
- INTERFACE
- seastar
- ${Seastar_DEVELOPMENT_LIBS})
+ INTERFACE seastar)

#
# The testing library.

@@ -766,8 +759,7 @@ if (Seastar_INSTALL OR Seastar_TESTING)

target_link_libraries (seastar_testing
PUBLIC
Boost::unit_test_framework
- seastar
- PRIVATE ${Seastar_DEVELOPMENT_LIBS})
+ seastar)
endif ()

#

@@ -837,12 +829,6 @@ if (Seastar_INSTALL)
# Necessary here for pkg-config.
include (GNUInstallDirs)

- if (NOT ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug"))
- set (Sanitizers_PKGCONFIG_COMPILER_OPTIONS "")
- else ()
- set (Sanitizers_PKGCONFIG_COMPILER_OPTIONS ${Sanitizers_COMPILER_OPTIONS})
- endif ()
-
# Set paths in pkg-config files for installation.
set (Seastar_PKG_CONFIG_PREFIX ${CMAKE_INSTALL_PREFIX})
set (Seastar_PKG_CONFIG_LIBDIR ${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR})
diff --git a/pkgconfig/seastar.pc.in b/pkgconfig/seastar.pc.in
index 0e165c7e..c278ad1d 100644

--- a/pkgconfig/seastar.pc.in
+++ b/pkgconfig/seastar.pc.in
@@ -33,7 +33,6 @@ lksctp_tools_cflags=-I$<JOIN:@lksctp-tools_INCLUDE_DIRS@, -I>
lksctp_tools_libs=$<JOIN:@lksctp-tools_LIBRARIES@, >
numactl_cflags=-I$<JOIN:@numactl_INCLUDE_DIRS@, -I>
numactl_libs=$<JOIN:@numactl_LIBRARIES@, >
-sanitizers_cflags=$<JOIN:@Sanitizers_PKGCONFIG_COMPILER_OPTIONS@, >

# Us.
seastar_cflags=${seastar_include_flags} $<JOIN:$<TARGET_PROPERTY:seastar,INTERFACE_COMPILE_OPTIONS>, > -D$<JOIN:$<TARGET_PROPERTY:seastar,INTERFACE_COMPILE_DEFINITIONS>, -D>
@@ -42,6 +41,6 @@ seastar_libs=${libdir}/$<TARGET_FILE_NAME:seastar>

[Paweł Dziepak]

On Tue, 5 Feb 2019 at 17:01, Jesse Haber-Kucharsky <jhab...@scylladb.com> wrote:

This sanitizer was included in the old build-system, and its removal was
not intentional.

ASan includes LeakSanitizer and on Linux it's enabled by default. I don't think we need this patch.

 

--
You received this message because you are subscribed to the Google Groups "seastar-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to seastar-dev...@googlegroups.com.
To post to this group, send email to seast...@googlegroups.com.
Visit this group at https://groups.google.com/group/seastar-dev.

To view this discussion on the web visit https://groups.google.com/d/msgid/seastar-dev/068884f2fe7de80d0b2a54114c1d954eaf968b6c.1549385799.git.jhaberku%40scylladb.com.

[Jesse Haber-Kucharsky]

On Wed, Feb 6, 2019 at 6:54 AM Paweł Dziepak <pdzi...@scylladb.com> wrote:

On Tue, 5 Feb 2019 at 17:01, Jesse Haber-Kucharsky <jhab...@scylladb.com> wrote:

This sanitizer was included in the old build-system, and its removal was
not intentional.

ASan includes LeakSanitizer and on Linux it's enabled by default. I don't think we need this patch.

Is there a test that we can use to verify? Why does `lsan` exist as a separate package?

[Paweł Dziepak]

On Wed, 6 Feb 2019 at 14:15, Jesse Haber-Kucharsky <jhab...@scylladb.com> wrote:

On Wed, Feb 6, 2019 at 6:54 AM Paweł Dziepak <pdzi...@scylladb.com> wrote:

On Tue, 5 Feb 2019 at 17:01, Jesse Haber-Kucharsky <jhab...@scylladb.com> wrote:

This sanitizer was included in the old build-system, and its removal was
not intentional.

ASan includes LeakSanitizer and on Linux it's enabled by default. I don't think we need this patch.

Is there a test that we can use to verify? Why does `lsan` exist as a separate package?

There is documentation:

https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer

https://clang.llvm.org/docs/LeakSanitizer.html

-fsanitize=lsan exists for the cases when you want only the LeakSanitizer, without the rest of the ASan which has much larger performance penalty.

[Jesse Haber-Kucharsky]

On Wed, Feb 6, 2019 at 9:25 AM Paweł Dziepak <pdzi...@scylladb.com> wrote:

On Wed, 6 Feb 2019 at 14:15, Jesse Haber-Kucharsky <jhab...@scylladb.com> wrote:

On Wed, Feb 6, 2019 at 6:54 AM Paweł Dziepak <pdzi...@scylladb.com> wrote:

On Tue, 5 Feb 2019 at 17:01, Jesse Haber-Kucharsky <jhab...@scylladb.com> wrote:

This sanitizer was included in the old build-system, and its removal was
not intentional.

ASan includes LeakSanitizer and on Linux it's enabled by default. I don't think we need this patch.

Is there a test that we can use to verify? Why does `lsan` exist as a separate package?

There is documentation:

Imagine that!

 

https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer

https://clang.llvm.org/docs/LeakSanitizer.html 

-fsanitize=lsan exists for the cases when you want only the LeakSanitizer, without the rest of the ASan which has much larger performance penalty.

I appreciate the information. Thanks! I'll remove this patch too.

[Jesse Haber-Kucharsky]

While some sanitizers do not have to be enabled for programs which link
against libraries with sanitizers enabled, we can be conservative and
ensure that sanitizers are enabled transitively all the time.

This was already the way that pkg-config worked, and this change ensures
that consumers of Seastar using CMake behave the same way.

We can eliminate the `sanitizers_cflags` pkg-config variable because the
sanitizers are part of the expansion of `INTERFACE_COMPILE_OPTIONS`.

Signed-off-by: Jesse Haber-Kucharsky <jhab...@scylladb.com>
---

CMakeLists.txt | 18 ++++--------------
pkgconfig/seastar.pc.in | 3 +--
2 files changed, 5 insertions(+), 16 deletions(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 094dc5ae..97ba1446 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -608,6 +608,8 @@ endif ()

target_link_libraries (seastar
PUBLIC
+ debug Sanitizers::address

+ debug Sanitizers::undefined_behavior
Boost::boost
Boost::program_options
Boost::thread

@@ -760,9 +762,6 @@ target_compile_definitions (seastar

target_compile_options (seastar
PRIVATE ${Seastar_DEVELOPMENT_FLAGS})

-target_link_libraries (seastar
- PRIVATE ${Seastar_DEVELOPMENT_LIBS})
-
add_library (seastar_with_flags INTERFACE)

target_compile_definitions (seastar_with_flags

@@ -772,9 +771,7 @@ target_compile_options (seastar_with_flags

INTERFACE ${Seastar_DEVELOPMENT_FLAGS})

target_link_libraries (seastar_with_flags
- INTERFACE
- seastar
- ${Seastar_DEVELOPMENT_LIBS})
+ INTERFACE seastar)

#
# The testing library.

@@ -804,8 +801,7 @@ if (Seastar_INSTALL OR Seastar_TESTING)

target_link_libraries (seastar_testing
PUBLIC
Boost::unit_test_framework
- seastar
- PRIVATE ${Seastar_DEVELOPMENT_LIBS})
+ seastar)
endif ()

#

@@ -875,12 +871,6 @@ if (Seastar_INSTALL)

# Necessary here for pkg-config.
include (GNUInstallDirs)

- if (NOT ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug"))
- set (Sanitizers_PKGCONFIG_COMPILER_OPTIONS "")
- else ()
- set (Sanitizers_PKGCONFIG_COMPILER_OPTIONS ${Sanitizers_COMPILER_OPTIONS})
- endif ()
-
# Set paths in pkg-config files for installation.
set (Seastar_PKG_CONFIG_PREFIX ${CMAKE_INSTALL_PREFIX})
set (Seastar_PKG_CONFIG_LIBDIR ${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR})
diff --git a/pkgconfig/seastar.pc.in b/pkgconfig/seastar.pc.in

index 35d076f1..f5f8294c 100644

--- a/pkgconfig/seastar.pc.in
+++ b/pkgconfig/seastar.pc.in
@@ -33,7 +33,6 @@ lksctp_tools_cflags=-I$<JOIN:@lksctp-tools_INCLUDE_DIRS@, -I>
lksctp_tools_libs=$<JOIN:@lksctp-tools_LIBRARIES@, >
numactl_cflags=-I$<JOIN:@numactl_INCLUDE_DIRS@, -I>
numactl_libs=$<JOIN:@numactl_LIBRARIES@, >
-sanitizers_cflags=$<JOIN:@Sanitizers_PKGCONFIG_COMPILER_OPTIONS@, >

# Us.
seastar_cflags=${seastar_include_flags} $<JOIN:$<TARGET_PROPERTY:seastar,INTERFACE_COMPILE_OPTIONS>, > -D$<JOIN:$<TARGET_PROPERTY:seastar,INTERFACE_COMPILE_DEFINITIONS>, -D>

@@ -42,6 +41,6 @@ seastar_libs=${libdir}/$<TARGET_FILE_NAME:seastar> @Seastar_SPLIT_DWARF_FLAG@

[Rafael Avila de Espindola]

Why not delete Seastar_DEVELOPMENT_LIBS completely?

> --
> You received this message because you are subscribed to the Google Groups "seastar-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to seastar-dev...@googlegroups.com.
> To post to this group, send email to seast...@googlegroups.com.
> Visit this group at https://groups.google.com/group/seastar-dev.

> To view this discussion on the web visit https://groups.google.com/d/msgid/seastar-dev/50229f98a26d078a72ba0a612455c19239acef29.1549480614.git.jhaberku%40scylladb.com.

[Jesse Haber-Kucharsky]

On Thu, Feb 7, 2019 at 1:24 PM Rafael Avila de Espindola <espi...@scylladb.com> wrote:

Why not delete Seastar_DEVELOPMENT_LIBS completely?

I intended to. It must have been a rebase error. Thanks for spotting this!

[Jesse Haber-Kucharsky]

While some sanitizers do not have to be enabled for programs which link
against libraries with sanitizers enabled, we can be conservative and
ensure that sanitizers are enabled transitively all the time.

This was already the way that pkg-config worked, and this change ensures
that consumers of Seastar using CMake behave the same way.

We can eliminate the `sanitizers_cflags` pkg-config variable because the
sanitizers are part of the expansion of `INTERFACE_COMPILE_OPTIONS`.

We can also eliminate the `Seastar_DEVELOPMENT_LIBS` variable.

Signed-off-by: Jesse Haber-Kucharsky <jhab...@scylladb.com>
---

Changes since v3:

- Remove the definition of `Seastar_DEVELOPMENT_LIBS`

CMakeLists.txt | 22 ++++------------------
pkgconfig/seastar.pc.in | 3 +--
2 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 094dc5ae..0aae3c16 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -596,10 +596,6 @@ if (Seastar_COMPRESS_DEBUG)

list (APPEND Seastar_DEVELOPMENT_FLAGS -gz)
endif()

-set (Seastar_DEVELOPMENT_LIBS
- debug Sanitizers::address

- debug Sanitizers::undefined_behavior)
-

if (CMAKE_BUILD_TYPE STREQUAL "Debug")

if (NOT Sanitizers_FOUND)
message (FATAL_ERROR "Sanitizers are required for debug mode!")

@@ -608,6 +604,8 @@ endif ()

target_link_libraries (seastar
PUBLIC
+ debug Sanitizers::address
+ debug Sanitizers::undefined_behavior
Boost::boost
Boost::program_options
Boost::thread

@@ -760,9 +758,6 @@ target_compile_definitions (seastar

target_compile_options (seastar
PRIVATE ${Seastar_DEVELOPMENT_FLAGS})

-target_link_libraries (seastar
- PRIVATE ${Seastar_DEVELOPMENT_LIBS})
-
add_library (seastar_with_flags INTERFACE)

target_compile_definitions (seastar_with_flags

@@ -772,9 +767,7 @@ target_compile_options (seastar_with_flags

INTERFACE ${Seastar_DEVELOPMENT_FLAGS})

target_link_libraries (seastar_with_flags
- INTERFACE
- seastar
- ${Seastar_DEVELOPMENT_LIBS})
+ INTERFACE seastar)

#
# The testing library.

@@ -804,8 +797,7 @@ if (Seastar_INSTALL OR Seastar_TESTING)

target_link_libraries (seastar_testing
PUBLIC
Boost::unit_test_framework
- seastar
- PRIVATE ${Seastar_DEVELOPMENT_LIBS})
+ seastar)
endif ()

#

@@ -875,12 +867,6 @@ if (Seastar_INSTALL)

[Rafael Avila de Espindola]

LGTM

> --
> You received this message because you are subscribed to the Google Groups "seastar-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to seastar-dev...@googlegroups.com.
> To post to this group, send email to seast...@googlegroups.com.
> Visit this group at https://groups.google.com/group/seastar-dev.

> To view this discussion on the web visit https://groups.google.com/d/msgid/seastar-dev/fabd80a995ac280345a501614512c05d669bddd8.1549571398.git.jhaberku%40scylladb.com.

[Alexander Gallego]

Glad to see this, i just ran into this error myself (maybe for future changes, errors that don't change the behavior of seastar should be warnings and not fatal -  like sanitizers). 

[Commit Bot]

From: Jesse Haber-Kucharsky <jhab...@scylladb.com>
Committer: Avi Kivity <a...@scylladb.com>
Branch: master

build: Make the use of sanitizers transitive

While some sanitizers do not have to be enabled for programs which link
against libraries with sanitizers enabled, we can be conservative and
ensure that sanitizers are enabled transitively all the time.

This was already the way that pkg-config worked, and this change ensures
that consumers of Seastar using CMake behave the same way.

We can eliminate the `sanitizers_cflags` pkg-config variable because the
sanitizers are part of the expansion of `INTERFACE_COMPILE_OPTIONS`.

We can also eliminate the `Seastar_DEVELOPMENT_LIBS` variable.

Signed-off-by: Jesse Haber-Kucharsky <jhab...@scylladb.com>

Message-Id:
<fabd80a995ac280345a501614512c05d...@scylladb.com>

---
diff --git a/CMakeLists.txt b/CMakeLists.txt

[Alexander Gallego]

On Friday, February 8, 2019 at 11:47:59 AM UTC-5, Bot Droid wrote:

From: Jesse Haber-Kucharsky <jhab...@scylladb.com>
Committer: Avi Kivity <a...@scylladb.com>
Branch: master

build: Make the use of sanitizers transitive

While some sanitizers do not have to be enabled for programs which link
against libraries with sanitizers enabled, we can be conservative and
ensure that sanitizers are enabled transitively all the time.

This was already the way that pkg-config worked, and this change ensures
that consumers of Seastar using CMake behave the same way.

We can eliminate the `sanitizers_cflags` pkg-config variable because the
sanitizers are part of the expansion of `INTERFACE_COMPILE_OPTIONS`.

We can also eliminate the `Seastar_DEVELOPMENT_LIBS` variable.

Signed-off-by: Jesse Haber-Kucharsky <jhab...@scylladb.com>
Message-Id:  

<fabd80a995ac280345a501614512c05d669bddd8.1549571398.git.jhab...@scylladb.com>

---
diff --git a/CMakeLists.txt b/CMakeLists.txt
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -596,10 +596,6 @@ if (Seastar_COMPRESS_DEBUG)
    list (APPEND Seastar_DEVELOPMENT_FLAGS -gz)
  endif()

-set (Seastar_DEVELOPMENT_LIBS
-    debug Sanitizers::address
-    debug Sanitizers::undefined_behavior)
-
  if (CMAKE_BUILD_TYPE STREQUAL "Debug")
    if (NOT Sanitizers_FOUND)
      message (FATAL_ERROR "Sanitizers are required for debug mode!")

I just pulled the committed change and it is still breaking for cmake consumers: 

-- Performing Test Sanitizers_ADDRESS_FOUND

-- Performing Test Sanitizers_ADDRESS_FOUND - Failed

-- Performing Test Sanitizers_UNDEFINED_BEHAVIOR_FOUND      

-- Performing Test Sanitizers_UNDEFINED_BEHAVIOR_FOUND - Failed            

-- Performing Test Sanitizers_FIBER_SUPPORT

-- Performing Test Sanitizers_FIBER_SUPPORT - Failed

-- Could NOT find Sanitizers (missing: Sanitizers_ADDRESS_COMPILER_OPTIONS Sanitizers_UNDEFINED_BEHAVIOR_COMPILER_OPTIONS)

... snip

cooking_ingredient( ... 

GIT_REPOSITORY https://github.com/scylladb/seastar

GIT_TAG 26dff41

[Jesse Haber-Kucharsky]

On Fri, Feb 8, 2019 at 3:24 PM Alexander Gallego <galleg...@gmail.com> wrote:

On Friday, February 8, 2019 at 11:47:59 AM UTC-5, Bot Droid wrote:

From: Jesse Haber-Kucharsky <jhab...@scylladb.com>
Committer: Avi Kivity <a...@scylladb.com>
Branch: master

build: Make the use of sanitizers transitive

While some sanitizers do not have to be enabled for programs which link
against libraries with sanitizers enabled, we can be conservative and
ensure that sanitizers are enabled transitively all the time.

This was already the way that pkg-config worked, and this change ensures
that consumers of Seastar using CMake behave the same way.

We can eliminate the `sanitizers_cflags` pkg-config variable because the
sanitizers are part of the expansion of `INTERFACE_COMPILE_OPTIONS`.

We can also eliminate the `Seastar_DEVELOPMENT_LIBS` variable.

Signed-off-by: Jesse Haber-Kucharsky <jhab...@scylladb.com>
Message-Id:  

<fabd80a995ac280345a501614512c05d...@scylladb.com>

---
diff --git a/CMakeLists.txt b/CMakeLists.txt
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -596,10 +596,6 @@ if (Seastar_COMPRESS_DEBUG)
    list (APPEND Seastar_DEVELOPMENT_FLAGS -gz)
  endif()

-set (Seastar_DEVELOPMENT_LIBS
-    debug Sanitizers::address
-    debug Sanitizers::undefined_behavior)
-
  if (CMAKE_BUILD_TYPE STREQUAL "Debug")
    if (NOT Sanitizers_FOUND)
      message (FATAL_ERROR "Sanitizers are required for debug mode!")

I just pulled the committed change and it is still breaking for cmake consumers: 

That looks to be a separate issue than the one addressed by this patch.

In Seastar's `CMakeLists.txt`, we optionally include `Sanitizers`

 

-- Performing Test Sanitizers_ADDRESS_FOUND

-- Performing Test Sanitizers_ADDRESS_FOUND - Failed

-- Performing Test Sanitizers_UNDEFINED_BEHAVIOR_FOUND      

-- Performing Test Sanitizers_UNDEFINED_BEHAVIOR_FOUND - Failed            

-- Performing Test Sanitizers_FIBER_SUPPORT

-- Performing Test Sanitizers_FIBER_SUPPORT - Failed

-- Could NOT find Sanitizers (missing: Sanitizers_ADDRESS_COMPILER_OPTIONS Sanitizers_UNDEFINED_BEHAVIOR_COMPILER_OPTIONS)

This looks like a separate issue.

Since we have `find_package (Sanitizers)` and *not* `find_package (Sanitizers REQUIRED)`, the sanitizers themselves are optional. In debug builds, we *do* require that they're installed.

However, in "release" builds, the configuration should succeed even if sanitizers are not installed (and in spite of the warning). This is true for my local machine. Do you see something different?

 

--

You received this message because you are subscribed to the Google Groups "seastar-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to seastar-dev...@googlegroups.com.
To post to this group, send email to seast...@googlegroups.com.
Visit this group at https://groups.google.com/group/seastar-dev.

To view this discussion on the web visit https://groups.google.com/d/msgid/seastar-dev/21ec7d30-b246-4a8b-b183-fa822ddcb439%40googlegroups.com.