From 40e9f61a11737ab3392c4530475ee131f160b9d8 Mon Sep 17 00:00:00 2001 From: Joshua Saxby Date: Mon, 6 Mar 2023 21:08:40 +0000 Subject: [PATCH 1/7] Cool, tried out using a CMake module which adds sanitisers for you I'll see if this works on Github-actions before making it CI-only. I think we do want it only on CI because it's noticeably slower than normal (but still *MUCH* faster than Valgrind!) --- .gitmodules | 3 +++ CMakeLists.txt | 1 + cmake/Modules/externals/sanitizers-cmake | 1 + tests/CMakeLists.txt | 6 ++++++ 4 files changed, 11 insertions(+) create mode 100644 .gitmodules create mode 160000 cmake/Modules/externals/sanitizers-cmake diff --git a/.gitmodules b/.gitmodules new file mode 100644 index 0000000..1a8e97d --- /dev/null +++ b/.gitmodules @@ -0,0 +1,3 @@ +[submodule "cmake/Modules/externals/sanitizers-cmake"] + path = cmake/Modules/externals/sanitizers-cmake + url = git@github.com:arsenm/sanitizers-cmake.git diff --git a/CMakeLists.txt b/CMakeLists.txt index 11ff47c..2731d41 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -133,6 +133,7 @@ endif() # add custom dependencies directory set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${CMAKE_CURRENT_SOURCE_DIR}/cmake/Modules/") +set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${CMAKE_CURRENT_SOURCE_DIR}/cmake/Modules/externals/sanitizers-cmake/cmake") # a better way to load dependencies include(CPM) diff --git a/cmake/Modules/externals/sanitizers-cmake b/cmake/Modules/externals/sanitizers-cmake new file mode 160000 index 0000000..c3dc841 --- /dev/null +++ b/cmake/Modules/externals/sanitizers-cmake @@ -0,0 +1 @@ +Subproject commit c3dc841af4dbf44669e65b82cb68a575864326bd diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b228398..8d4c9f7 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -5,6 +5,11 @@ CPMFindPackage( EXCLUDE_FROM_ALL YES ) +set(SANITIZE_ADDRESS ON) +set(SANITIZE_UNDEFINED ON) + +find_package(Sanitizers) + # common option-propagating target to use for test suite sub-targets add_library(tests-config INTERFACE) target_link_libraries( @@ -35,6 +40,7 @@ target_link_libraries( arby Catch2::Catch2 # unit testing framework ) +add_sanitizers(tests) enable_testing() From 8b53969fc7fe4b1f9047c5632f9a42a166233b8c Mon Sep 17 00:00:00 2001 From: Joshua Saxby Date: Mon, 6 Mar 2023 21:11:46 +0000 Subject: [PATCH 2/7] Checkout git submodules with Github-Actions --- .github/workflows/continuous-integration.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index 49a69a3..938ca81 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.github/workflows/continuous-integration.yml @@ -43,7 +43,10 @@ jobs: memcheck: true # memory-testing on Linux only steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 + with: + submodules: true + # when building on master branch and not a pull request, build and test in release mode (optimised build) - name: Set Build Mode to Release From 6750aec24efd3a2130e6b46e33a7143f6579543e Mon Sep 17 00:00:00 2001 From: Joshua Saxby Date: Mon, 6 Mar 2023 21:22:21 +0000 Subject: [PATCH 3/7] Get rid of Valgrind build and disable sanitizer on MSVC Lol, suprise surprise! Valgrind and the Sanitizer don't run well together! :) MSVC isn't playing ball for some reason, it links but every test fails and I'm not seeing ASAN errors (unless those failures are caused by ASAN --need to troubleshoot this by forcing UB to see how ctest reports it...) --- .github/workflows/continuous-integration.yml | 21 -------------------- tests/CMakeLists.txt | 12 +++++++---- 2 files changed, 8 insertions(+), 25 deletions(-) diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index 938ca81..2ebb490 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.github/workflows/continuous-integration.yml @@ -25,7 +25,6 @@ jobs: matrix: os: [ubuntu-20.04] cxx: [g++-10, clang++] - memcheck: [false] include: # macos has two separate versions for the different compilers because # the new macos image needed for clang using a new version of XCode @@ -38,28 +37,18 @@ jobs: cxx: clang++ - os: windows-2019 cxx: msvc - - os: ubuntu-20.04 - cxx: g++-10 - memcheck: true # memory-testing on Linux only steps: - uses: actions/checkout@v3 with: submodules: true - # when building on master branch and not a pull request, build and test in release mode (optimised build) - name: Set Build Mode to Release shell: bash if: ${{ github.event_name != 'pull_request' && github.ref == 'refs/heads/master' }} run: echo "BUILD_TYPE=Release" >> $GITHUB_ENV - - name: Install Valgrind - if: ${{ matrix.memcheck }} - run: | - sudo apt-get update - sudo apt-get install valgrind - - name: Configure CMake env: CXX: ${{ matrix.cxx }} @@ -79,22 +68,12 @@ jobs: run: cmake --build . --config $BUILD_TYPE - name: Test - if: ${{ !matrix.memcheck }} working-directory: ${{github.workspace}}/build shell: bash # Execute tests defined by the CMake configuration. # See https://cmake.org/cmake/help/latest/manual/ctest.1.html for more detail run: ctest -C $BUILD_TYPE --no-tests=error - - name: Test with Valgrind - if: ${{ matrix.memcheck }} - working-directory: ${{github.workspace}}/build - shell: bash - # run ctest with memcheck mode to check for memory errors with Valgrind - # exclude the one test case for arby::Nat::from_float with NaN/Inf because it fails on Valgrind - # due to lack of long double support - run: ctest -C $BUILD_TYPE -T memcheck -j3 -E "with non-finite value throws" --no-tests=error - - name: Test Install working-directory: ${{github.workspace}}/build shell: bash diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 8d4c9f7..9b35b4a 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -5,10 +5,12 @@ CPMFindPackage( EXCLUDE_FROM_ALL YES ) -set(SANITIZE_ADDRESS ON) -set(SANITIZE_UNDEFINED ON) +if (NOT MSVC) # Sanitizers package doesn't seem to work well with MSVC on CI... + set(SANITIZE_ADDRESS ON) + set(SANITIZE_UNDEFINED ON) -find_package(Sanitizers) + find_package(Sanitizers) +endif() # common option-propagating target to use for test suite sub-targets add_library(tests-config INTERFACE) @@ -40,7 +42,9 @@ target_link_libraries( arby Catch2::Catch2 # unit testing framework ) -add_sanitizers(tests) +if (NOT MSVC) # Sanitizers package doesn't seem to work well with MSVC on CI... + add_sanitizers(tests) +endif() enable_testing() From 746164d5d586ce5854dd3d3ce7365d06c34a4436 Mon Sep 17 00:00:00 2001 From: Joshua Saxby Date: Mon, 6 Mar 2023 21:48:09 +0000 Subject: [PATCH 4/7] Hmph, well a sanitizer that doesn't actually fail the build is no use [skip ci] --- CMakeLists.txt | 4 +++- arby/include/arby/Nat.hpp | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 2731d41..953c041 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -115,7 +115,7 @@ else() # GCC/Clang warning option enable_cxx_compiler_flag_if_supported("-Wno-unknown-pragmas") # if tests are enabled, enable converting all warnings to errors too if (ENABLE_TESTS) - enable_cxx_compiler_flag_if_supported("-Werror") + # enable_cxx_compiler_flag_if_supported("-Werror") # exclude the following kinds of warnings from being converted into errors # unknown-pragma is useful to have as a warning but not as an error, if you have # pragmas which are for the consumption of one compiler only @@ -128,6 +128,8 @@ else() # GCC/Clang warning option enable_cxx_compiler_flag_if_supported("-Wno-error=unused-but-set-variable") # Clang's now introduced a "helpful" analysis of for-loops that increment a variable twice enable_cxx_compiler_flag_if_supported("-Wno-error=for-loop-analysis") + # XXX: make sanitizer force quit + # enable_cxx_compiler_flag_if_supported("-fno-sanitize-recover=all") endif() endif() diff --git a/arby/include/arby/Nat.hpp b/arby/include/arby/Nat.hpp index b1f42b1..50e22fc 100644 --- a/arby/include/arby/Nat.hpp +++ b/arby/include/arby/Nat.hpp @@ -213,6 +213,8 @@ namespace com::saxbophone::arby { */ constexpr Nat() : _digits{0} { _validate_digits(); + int k = 0x7fffffff; + k += 1; // cause integer overflow } /** * @brief Integer-constructor, initialises with the given integer value From d8e5e74eb1360b7e986f5d27fd66c210fb9da915 Mon Sep 17 00:00:00 2001 From: Joshua Saxby Date: Mon, 13 Mar 2023 15:35:38 +0000 Subject: [PATCH 5/7] Attempt enabling san on tests by adding it to tests config Maybe the object libraries are compiled only with `tests-config` options rather than options set on the `tests` target itself... --- tests/CMakeLists.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 9b35b4a..19882d2 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -21,6 +21,9 @@ target_link_libraries( arby Catch2::Catch2 # unit testing framework ) +if (NOT MSVC) # Sanitizers package doesn't seem to work well with MSVC on CI... + add_sanitizers(tests-config) +endif() # every sub-part of the test suite add_subdirectory(DivisionResult) @@ -42,9 +45,6 @@ target_link_libraries( arby Catch2::Catch2 # unit testing framework ) -if (NOT MSVC) # Sanitizers package doesn't seem to work well with MSVC on CI... - add_sanitizers(tests) -endif() enable_testing() From d70071039f0425506401b7f524911d0d635c8ee7 Mon Sep 17 00:00:00 2001 From: Joshua Saxby Date: Mon, 13 Mar 2023 15:50:32 +0000 Subject: [PATCH 6/7] Disabling both PCH and applying san to tests-config doesn't fix the issue I think I might have to rewrite cmake-sanitizers to expose a usage-requirements interface library for sanitizers: https://cmake.org/cmake/help/latest/manual/cmake-buildsystem.7.html#interface-libraries:~:text=Another%20use%2Dcase%20is%20to%20employ%20an%20entirely%20target%2Dfocussed%20design%20for%20usage%20requirements%3A --- tests/DivisionResult/CMakeLists.txt | 2 +- tests/Interval/CMakeLists.txt | 2 +- tests/Nat/CMakeLists.txt | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/DivisionResult/CMakeLists.txt b/tests/DivisionResult/CMakeLists.txt index b85f3a2..79955ae 100644 --- a/tests/DivisionResult/CMakeLists.txt +++ b/tests/DivisionResult/CMakeLists.txt @@ -1,3 +1,3 @@ add_library(DivisionResult OBJECT division_result.cpp) target_link_libraries(DivisionResult PRIVATE tests-config) -target_precompile_headers(DivisionResult PRIVATE ) +# target_precompile_headers(DivisionResult PRIVATE ) diff --git a/tests/Interval/CMakeLists.txt b/tests/Interval/CMakeLists.txt index e12d4da..e63d517 100644 --- a/tests/Interval/CMakeLists.txt +++ b/tests/Interval/CMakeLists.txt @@ -1,3 +1,3 @@ add_library(Interval OBJECT interval.cpp) target_link_libraries(Interval PRIVATE tests-config) -target_precompile_headers(Interval PRIVATE ) +# target_precompile_headers(Interval PRIVATE ) diff --git a/tests/Nat/CMakeLists.txt b/tests/Nat/CMakeLists.txt index 17271cc..b3e95b0 100644 --- a/tests/Nat/CMakeLists.txt +++ b/tests/Nat/CMakeLists.txt @@ -18,4 +18,4 @@ add_library( user_defined_literals.cpp ) target_link_libraries(Nat PRIVATE tests-config) -target_precompile_headers(Nat PRIVATE ) +# target_precompile_headers(Nat PRIVATE ) From c6c2700ed05fec8600e7e4646dcc6b4156d18e11 Mon Sep 17 00:00:00 2001 From: Joshua Saxby Date: Mon, 13 Mar 2023 16:00:31 +0000 Subject: [PATCH 7/7] Hmmm, cool this appears to now link asan/ubsan correctly... --- arby/include/arby/Nat.hpp | 4 ++-- tests/Nat/CMakeLists.txt | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/arby/include/arby/Nat.hpp b/arby/include/arby/Nat.hpp index 50e22fc..e304dbf 100644 --- a/arby/include/arby/Nat.hpp +++ b/arby/include/arby/Nat.hpp @@ -213,8 +213,8 @@ namespace com::saxbophone::arby { */ constexpr Nat() : _digits{0} { _validate_digits(); - int k = 0x7fffffff; - k += 1; // cause integer overflow + int i = std::numeric_limits::max(); + ++i; } /** * @brief Integer-constructor, initialises with the given integer value diff --git a/tests/Nat/CMakeLists.txt b/tests/Nat/CMakeLists.txt index b3e95b0..e3a349f 100644 --- a/tests/Nat/CMakeLists.txt +++ b/tests/Nat/CMakeLists.txt @@ -17,5 +17,6 @@ add_library( stringification.cpp user_defined_literals.cpp ) +add_sanitizers(Nat) target_link_libraries(Nat PRIVATE tests-config) # target_precompile_headers(Nat PRIVATE )