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

Cool, tried out using a CMake module which adds sanitisers for you #153

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 3 additions & 21 deletions .github/workflows/continuous-integration.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -38,25 +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@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
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 }}
Expand All @@ -76,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
Expand Down
3 changes: 3 additions & 0 deletions .gitmodules
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[submodule "cmake/Modules/externals/sanitizers-cmake"]
path = cmake/Modules/externals/sanitizers-cmake
url = [email protected]:arsenm/sanitizers-cmake.git
5 changes: 4 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -128,11 +128,14 @@ 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()

# 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)
Expand Down
2 changes: 2 additions & 0 deletions arby/include/arby/Nat.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,8 @@ namespace com::saxbophone::arby {
*/
constexpr Nat() : _digits{0} {
_validate_digits();
int i = std::numeric_limits<int>::max();
++i;
}
/**
* @brief Integer-constructor, initialises with the given integer value
Expand Down
1 change: 1 addition & 0 deletions cmake/Modules/externals/sanitizers-cmake
Submodule sanitizers-cmake added at c3dc84
10 changes: 10 additions & 0 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,13 @@ CPMFindPackage(
EXCLUDE_FROM_ALL YES
)

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

# common option-propagating target to use for test suite sub-targets
add_library(tests-config INTERFACE)
target_link_libraries(
Expand All @@ -14,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)
Expand Down
2 changes: 1 addition & 1 deletion tests/DivisionResult/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
add_library(DivisionResult OBJECT division_result.cpp)
target_link_libraries(DivisionResult PRIVATE tests-config)
target_precompile_headers(DivisionResult PRIVATE <arby/DivisionResult.hpp> <arby/Interval.hpp> <arby/Nat.hpp>)
# target_precompile_headers(DivisionResult PRIVATE <arby/DivisionResult.hpp> <arby/Interval.hpp> <arby/Nat.hpp>)
2 changes: 1 addition & 1 deletion tests/Interval/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
add_library(Interval OBJECT interval.cpp)
target_link_libraries(Interval PRIVATE tests-config)
target_precompile_headers(Interval PRIVATE <arby/Interval.hpp> <arby/Nat.hpp>)
# target_precompile_headers(Interval PRIVATE <arby/Interval.hpp> <arby/Nat.hpp>)
3 changes: 2 additions & 1 deletion tests/Nat/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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 <arby/DivisionResult.hpp> <arby/Interval.hpp> <arby/Nat.hpp>)
# target_precompile_headers(Nat PRIVATE <arby/DivisionResult.hpp> <arby/Interval.hpp> <arby/Nat.hpp>)