diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f9ddcba..bb0cbce 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -38,3 +38,35 @@ jobs: - name: Run tests run: ./build/test/testmain + + sanitize: + runs-on: ubuntu-latest + permissions: + contents: read + + steps: + - name: Checkout repository + uses: actions/checkout@v7 + with: + submodules: recursive + + - name: Install dependencies + run: | + sudo apt-get update + sudo apt-get install -y cmake g++ make + + # Out-of-bounds accesses in the bit manipulation are invisible to the + # plain test run, since the values read are normally discarded. + - name: Configure CMake with the address and undefined behaviour sanitizers + run: > + cmake -B build-sanitize -DCMAKE_BUILD_TYPE=Debug + -DCMAKE_CXX_FLAGS="-fsanitize=address,undefined -fno-omit-frame-pointer" + -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=address,undefined" + + - name: Build + run: cmake --build build-sanitize + + - name: Run tests + run: ./build-sanitize/test/testmain + env: + UBSAN_OPTIONS: halt_on_error=1:print_stacktrace=1 diff --git a/include/fbitset.hpp b/include/fbitset.hpp index 82aaa63..4eba4b5 100644 --- a/include/fbitset.hpp +++ b/include/fbitset.hpp @@ -32,9 +32,14 @@ using Size = size_t; namespace internal { /** Utility for checking if the container is `No_ext`. + * + * The explicit specialization needs to be `inline`. Without it the + * specialization has external linkage and gets emitted in every + * translation unit including this header, so a program including the + * header twice fails to link with a duplicate symbol. */ - template constexpr bool is_no_ext = false; - template <> constexpr bool is_no_ext = true; + template inline constexpr bool is_no_ext = false; + template <> inline constexpr bool is_no_ext = true; // // Bit operations using C++20 header @@ -523,13 +528,21 @@ class Fbitset : public Fbitset_base { * * The copy of the current limb and base will also be updated to keep * the invariant. + * + * Note that the current limb is only read after the incremented + * pointer has been checked against the sentinel. Reading it + * unconditionally would dereference one past the last limb whenever + * the final limb gets exhausted, which is undefined behaviour even + * though the value read is discarded right away. */ void get_next() noexcept { while (curr_ < last_ && limb_ == 0) { ++curr_; - limb_ = *curr_; base_ += LIMB_BITS; + if (curr_ < last_) { + limb_ = *curr_; + } } } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index dfdf8c3..e2a80f9 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -16,6 +16,7 @@ FetchContent_MakeAvailable(Catch2) add_executable(testmain testmain.cpp basic.cpp + multi_tu.cpp ) target_link_libraries(testmain PRIVATE Catch2::Catch2) diff --git a/test/multi_tu.cpp b/test/multi_tu.cpp new file mode 100644 index 0000000..3947ff8 --- /dev/null +++ b/test/multi_tu.cpp @@ -0,0 +1,54 @@ +/** Tests that the header can be used from more than one translation unit. + * + * This file exists mostly so that the header is included by a second + * translation unit of the test driver. A header-only library has to link + * cleanly in that case, which requires every namespace-scope definition in it + * to have either internal or inline linkage. + * + * The iteration test here also walks a bit set all the way to exhaustion for + * every supported configuration, which is where the iterator used to read one + * limb past the end of the storage. That read is only observable under a + * sanitizer, so the CI has a job building the tests with the address + * sanitizer. + */ + +#include +#include + +#include + +#include + +using namespace fbitset; + +TEST_CASE("Fbitset can be iterated to exhaustion from a second translation unit") +{ + constexpr Size N_BITS = 64; + + auto walk = [N_BITS](auto bits, const std::vector& idxes) { + for (auto i : idxes) { + bits.set(i); + } + + std::vector res{}; + for (auto i = bits.begin(); i; ++i) { + res.push_back(*i); + } + CHECK(res == idxes); + + // Walking an empty bit set has to stop straight away. + decltype(bits) empty(N_BITS); + CHECK(!empty.begin()); + }; + + // A bit in the very last limb of each configuration, so that the iterator + // has to run off the end of the storage to terminate. + std::vector idxes = { 0, 31, 63 }; + + walk(Fbitset<1, uint64_t, No_ext>(N_BITS), idxes); + walk(Fbitset<2, uint32_t, No_ext>(N_BITS), idxes); + walk(Fbitset<2, uint64_t, std::vector>(N_BITS), idxes); + walk(Fbitset<2, uint32_t, std::vector>(N_BITS), idxes); + walk(Fbitset<1, uint32_t, std::vector>(N_BITS), idxes); + walk(Fbitset<>(N_BITS), idxes); +}