From 5e8a8f1a11097f78b5f94fc495b75fa0569d3933 Mon Sep 17 00:00:00 2001 From: Guo Chen Date: Sun, 16 Aug 2026 11:40:29 +0800 Subject: [PATCH 1/3] fix: Let an owning null handle be copied `Handle::incr_ref` incremented the reference count unconditionally, while `decr_ref` already tolerated a null pointer. An owning handle can hold a null pointer: `Iter_handle::set_next` stores the result of `PyIter_Next` with `allow_null`, so an exhausted iterator carries an owning null value. Copying such an iterator, which `std::vector(it.begin(), it.end())` does, dereferenced the null pointer and crashed. This is what brings down gristmill on a product with no indices at all (gristmill#45): `std::vector dims(dims_inp.begin(), dims_inp.end())` over an empty list. Use `Py_XINCREF`, matching `decr_ref`, and cover the exhausted-iterator copy, the container built from an empty iterable, and the plain null copy. Co-Authored-By: Claude Fable 5 --- include/cpypp.hpp | 8 ++++++-- test/iteratorprotocol.cpp | 31 +++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/include/cpypp.hpp b/include/cpypp.hpp index 05b028d..a53fe1f 100644 --- a/include/cpypp.hpp +++ b/include/cpypp.hpp @@ -636,13 +636,17 @@ class Handle { } } - /** Increments the reference count only for owning reference. + /** Increments the reference count only for owning reference to a non-null. + * + * An owning handle can hold a null pointer, for instance the value of an + * exhausted `Iter_handle`, so this has to tolerate null in the same way as + * `decr_ref` does. */ void incr_ref() noexcept { if (!if_borrow_) { - Py_INCREF(ref_); + Py_XINCREF(ref_); } } diff --git a/test/iteratorprotocol.cpp b/test/iteratorprotocol.cpp index 28f092c..d98ac7e 100644 --- a/test/iteratorprotocol.cpp +++ b/test/iteratorprotocol.cpp @@ -54,6 +54,37 @@ TEST_CASE("Handles has basic iterator protocol", "[Handle][Iter_handle]") } } +TEST_CASE("Iterators over empty iterables can be copied", "[Handle][Iter_handle]") +{ + // An exhausted iterator holds an owning null handle as its value. Copying + // it used to increment the reference count of the null pointer. + Handle empty("[]"); + + SECTION("an exhausted iterator can be copied") + { + auto it = empty.begin(); + check_exc(); + REQUIRE_FALSE(it.has_val()); + Iter_handle copy(it); + CHECK_FALSE(copy.has_val()); + CHECK(copy == empty.end()); + } + + SECTION("a container can be built from an empty iterable") + { + std::vector res(empty.begin(), empty.end()); + check_exc(); + CHECK(res.empty()); + } + + SECTION("an owning null handle can be copied") + { + Handle null(nullptr, STEAL, true); + Handle copy(null); + CHECK_FALSE(copy); + } +} + TEST_CASE("Iterator utilities report wrong types", "[Handle][Iter_handle]") { // A non-iterable and non-iterator integer. From cc1559c91a6959f60fbde2708a5b5ba647cf8988 Mon Sep 17 00:00:00 2001 From: Guo Chen Date: Sun, 16 Aug 2026 12:25:16 +0800 Subject: [PATCH 2/3] build: Move to C++20, Catch2 v3 and FindPython3, and make the tests hold on modern CPython The library header already compiles as C++20, since gristmill builds it that way, but the project's own build was pinned to C++14 with a Catch2 2.0.1 single header that no longer compiles under C++20 (std::uncaught_exception is gone) nor on arm64 (its breakpoint trap is x86 assembly). Set C++20, fetch Catch2 v3.11.0 the way fbitset and libparenth do, and find Python with FindPython3 instead of the deprecated FindPythonLibs. The reference-count tests assumed that the count of a small integer or of a static type moves when a handle is taken. From Python 3.12 on those objects are immortal and their count is frozen, so 48 assertions failed on any current interpreter. The handle tests now use large integers, which are fresh objects, and the two tests that need the cached small integer for identity check its count only when it can move. CI runs on ubuntu-latest with the system CMake and Python, Debug and Release. Co-Authored-By: Claude Fable 5 --- .github/workflows/ci.yml | 63 ++++++++++++++++--------------------- CMakeLists.txt | 11 ++++--- test/CMakeLists.txt | 18 ++++++----- test/exc.cpp | 2 +- test/fundamentalobjects.cpp | 8 +++-- test/handle.cpp | 17 +++++----- test/iteratorprotocol.cpp | 2 +- test/numberprotocol.cpp | 2 +- test/numericobjects.cpp | 13 ++++++-- test/objectprotocol.cpp | 2 +- test/otherobjects.cpp | 2 +- test/parsebuild.cpp | 13 ++++++-- test/sequenceobjects.cpp | 2 +- test/testmain.cpp | 3 +- 14 files changed, 87 insertions(+), 71 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 13d76ea..e5005f9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -11,44 +11,35 @@ on: workflow_dispatch: jobs: - test_debug: + test: runs-on: ubuntu-latest permissions: contents: read - container: - image: tschijnmo/drudge:base - - steps: - - name: Checkout repository with submodules - uses: actions/checkout@v3 - with: - submodules: recursive - - - name: Build and test (debug) - run: | - mkdir cmake-test-debug - cd cmake-test-debug - cmake -DCMAKE_BUILD_TYPE=debug .. - make - ./test/testmain + strategy: + matrix: + build_type: [Debug, Release] - test_release: - runs-on: ubuntu-latest - permissions: - contents: read - container: - image: tschijnmo/drudge:base - steps: - - name: Checkout repository with submodules - uses: actions/checkout@v3 - with: - submodules: recursive - - - name: Build and test (release) - run: | - mkdir cmake-test-release - cd cmake-test-release - cmake -DCMAKE_BUILD_TYPE=release .. - make - ./test/testmain + - name: Checkout code + uses: actions/checkout@v6 + + - name: Install dependencies + run: | + sudo apt-get update + sudo apt-get install -y cmake g++ python3-dev + + - name: Configure CMake + run: | + mkdir build + cd build + cmake -DCMAKE_BUILD_TYPE=${{ matrix.build_type }} .. + + - name: Build + run: | + cd build + make + + - name: Run tests + run: | + cd build + ./test/testmain diff --git a/CMakeLists.txt b/CMakeLists.txt index 255432b..989df01 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,20 +1,21 @@ -cmake_minimum_required(VERSION 3.6) +cmake_minimum_required(VERSION 3.18) project(cpypp VERSION 0.1.0 LANGUAGES CXX) # OPTIONS option(BUILD_TESTS "Build unit tests" ON) # Set the building options. -set(CMAKE_CXX_STANDARD 14) +set(CMAKE_CXX_STANDARD 20) +set(CMAKE_CXX_STANDARD_REQUIRED ON) +set(CMAKE_CXX_EXTENSIONS OFF) # Find the Python runtime. -include(FindPythonLibs) +find_package(Python3 REQUIRED COMPONENTS Development.Embed) message(STATUS - "Find Python header at ${PYTHON_INCLUDE_DIRS}\nand library at ${PYTHON_LIBRARIES}" + "Find Python header at ${Python3_INCLUDE_DIRS}\nand library at ${Python3_LIBRARIES}" ) if (BUILD_TESTS) enable_testing() add_subdirectory(test) endif () - diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index a228902..8114d5d 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -1,10 +1,14 @@ -# Download the Catch2 single header. +# Fetch Catch2 v3.11.0 +include(FetchContent) -file(DOWNLOAD - "https://github.com/catchorg/Catch2/releases/download/v2.0.1/catch.hpp" - "${CMAKE_CURRENT_BINARY_DIR}/catch.hpp" +FetchContent_Declare( + Catch2 + GIT_REPOSITORY https://github.com/catchorg/Catch2.git + GIT_TAG v3.11.0 ) +FetchContent_MakeAvailable(Catch2) + # The main test driver. add_executable(testmain testmain.cpp @@ -21,11 +25,9 @@ add_executable(testmain ) target_include_directories(testmain - PRIVATE ${CMAKE_CURRENT_BINARY_DIR} PRIVATE "${PROJECT_SOURCE_DIR}/include" - PRIVATE ${PYTHON_INCLUDE_DIRS} ) target_link_libraries(testmain - PRIVATE ${PYTHON_LIBRARIES} + PRIVATE Catch2::Catch2 + PRIVATE Python3::Python ) - diff --git a/test/exc.cpp b/test/exc.cpp index 337d6c1..7465bcb 100644 --- a/test/exc.cpp +++ b/test/exc.cpp @@ -1,7 +1,7 @@ /** Tests for the utilities related to Python exception handling. */ -#include +#include #include diff --git a/test/fundamentalobjects.cpp b/test/fundamentalobjects.cpp index 6fc14fb..18ccd08 100644 --- a/test/fundamentalobjects.cpp +++ b/test/fundamentalobjects.cpp @@ -1,7 +1,7 @@ /** Tests for the utility for fundamental objects. */ -#include +#include #include @@ -77,7 +77,11 @@ TEST_CASE("Trivial static type has basic features", "[Handle][Static_type]") auto handle = noddy_type.get_handle(true); CHECK(handle.get() == noddy_ptr); CHECK(!handle.if_borrow()); - CHECK(Py_REFCNT(noddy_ptr) == init_count + 1); + // Static types are immortal from Python 3.12 on, and then their + // reference count does not move. + if (init_count < (static_cast(1) << 30)) { + CHECK(Py_REFCNT(noddy_ptr) == init_count + 1); + } } CHECK(Py_REFCNT(noddy_ptr) == init_count); } diff --git a/test/handle.cpp b/test/handle.cpp index a3333ec..2f2a693 100644 --- a/test/handle.cpp +++ b/test/handle.cpp @@ -11,7 +11,7 @@ #include -#include +#include #include @@ -21,17 +21,18 @@ using namespace cpypp; TEST_CASE("Owning handles correctly manages reference counts", "[Handle]") { - PyObject* one = Py_BuildValue("i", 1); + // Large integers are used, since small integers are cached by CPython + // and immortal from Python 3.12 on, where their reference count never + // moves. These two are fresh objects whose count we fully control. + PyObject* one = Py_BuildValue("l", 1L << 40); // Py_BuildValue gives a new reference to be possibly stolen by // Handles. Here we increment the reference count again to make sure // that even after that reference is destroyed, we still have at least - // one reference. Practically this is unnecessary in that the integer - // unity normally has hundreds of references to prevent it from being - // collected. + // one reference. Py_INCREF(one); Py_ssize_t init_count = Py_REFCNT(one); - PyObject* two = Py_BuildValue("i", 2); + PyObject* two = Py_BuildValue("l", 1L << 41); Py_INCREF(two); // For the same rational as one. Py_ssize_t init_count2 = Py_REFCNT(two); @@ -351,8 +352,8 @@ TEST_CASE("Borrowing handles correctly treats reference counts", "[Handle]") { // Testing of borrowing handles is relatively easy, no matter what // happens, the reference count should never be touched. - PyObject* one = Py_BuildValue("i", 1); - PyObject* two = Py_BuildValue("i", 2); + PyObject* one = Py_BuildValue("l", 1L << 40); + PyObject* two = Py_BuildValue("l", 1L << 41); Py_ssize_t init_count = Py_REFCNT(one); Py_ssize_t init_count2 = Py_REFCNT(two); auto check_ref = [&]() { diff --git a/test/iteratorprotocol.cpp b/test/iteratorprotocol.cpp index d98ac7e..7f8ceb6 100644 --- a/test/iteratorprotocol.cpp +++ b/test/iteratorprotocol.cpp @@ -5,7 +5,7 @@ #include #include -#include +#include #include diff --git a/test/numberprotocol.cpp b/test/numberprotocol.cpp index 3b384a0..cb64d0b 100644 --- a/test/numberprotocol.cpp +++ b/test/numberprotocol.cpp @@ -3,7 +3,7 @@ #include -#include +#include #include diff --git a/test/numericobjects.cpp b/test/numericobjects.cpp index 1c2097a..47017e4 100644 --- a/test/numericobjects.cpp +++ b/test/numericobjects.cpp @@ -1,7 +1,7 @@ /** Tests for the utilities to related numeric objects. */ -#include +#include #include @@ -9,6 +9,13 @@ using namespace cpypp; +// Small integers are immortal from Python 3.12 on, and the reference count of +// an immortal object does not move. Their count is only checked when it can. +static bool count_moves(PyObject* obj) +{ + return Py_REFCNT(obj) < (static_cast(1) << 30); +} + TEST_CASE("Integer can be built and parsed", "[Handle]") { @@ -22,7 +29,9 @@ TEST_CASE("Integer can be built and parsed", "[Handle]") { Handle from_gen(v); CHECK(from_gen.get() == one); - CHECK(Py_REFCNT(one) == curr_count + 1); + if (count_moves(one)) { + CHECK(Py_REFCNT(one) == curr_count + 1); + } long val; from_gen.as(val); diff --git a/test/objectprotocol.cpp b/test/objectprotocol.cpp index ff25d76..3a97281 100644 --- a/test/objectprotocol.cpp +++ b/test/objectprotocol.cpp @@ -3,7 +3,7 @@ #include -#include +#include #include diff --git a/test/otherobjects.cpp b/test/otherobjects.cpp index 0088a99..16a6325 100644 --- a/test/otherobjects.cpp +++ b/test/otherobjects.cpp @@ -1,7 +1,7 @@ /** Tests for utility for uncategorized concrete object. */ -#include +#include #include diff --git a/test/parsebuild.cpp b/test/parsebuild.cpp index c51c0ae..df4c160 100644 --- a/test/parsebuild.cpp +++ b/test/parsebuild.cpp @@ -5,7 +5,7 @@ * objects. */ -#include +#include #include @@ -13,6 +13,13 @@ using namespace cpypp; +// Small integers are immortal from Python 3.12 on, and the reference count of +// an immortal object does not move. Their count is only checked when it can. +static bool count_moves(PyObject* obj) +{ + return Py_REFCNT(obj) < (static_cast(1) << 30); +} + TEST_CASE("General utility can make and parse simple integers", "[Handle]") { @@ -26,7 +33,9 @@ TEST_CASE("General utility can make and parse simple integers", "[Handle]") { Handle from_gen("i", 1); CHECK(from_gen.get() == one); - CHECK(Py_REFCNT(one) == curr_count + 1); + if (count_moves(one)) { + CHECK(Py_REFCNT(one) == curr_count + 1); + } long val; from_gen.as(val); diff --git a/test/sequenceobjects.cpp b/test/sequenceobjects.cpp index 2fdc5c5..40de1d4 100644 --- a/test/sequenceobjects.cpp +++ b/test/sequenceobjects.cpp @@ -1,7 +1,7 @@ /** Tests for the utility for sequence objects. */ -#include +#include #include diff --git a/test/testmain.cpp b/test/testmain.cpp index 0dd0df5..ad221bc 100644 --- a/test/testmain.cpp +++ b/test/testmain.cpp @@ -1,5 +1,4 @@ -#define CATCH_CONFIG_RUNNER -#include +#include #include From a92167431820245fd9c8ce9e381ad518f5e42218 Mon Sep 17 00:00:00 2001 From: Guo Chen Date: Sun, 16 Aug 2026 12:31:34 +0800 Subject: [PATCH 3/3] test: Build the large test integers portably Copilot pointed out that 1L << 40 is not valid where long is 32 bits. Use long long, and reword the incr_ref comment. Co-Authored-By: Claude Fable 5 --- include/cpypp.hpp | 6 +++--- test/handle.cpp | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/include/cpypp.hpp b/include/cpypp.hpp index a53fe1f..25f59f4 100644 --- a/include/cpypp.hpp +++ b/include/cpypp.hpp @@ -636,11 +636,11 @@ class Handle { } } - /** Increments the reference count only for owning reference to a non-null. + /** Increments the reference count for an owning reference, if not null. * * An owning handle can hold a null pointer, for instance the value of an - * exhausted `Iter_handle`, so this has to tolerate null in the same way as - * `decr_ref` does. + * exhausted `Iter_handle`, so null is tolerated here just as it is in + * `decr_ref`. */ void incr_ref() noexcept diff --git a/test/handle.cpp b/test/handle.cpp index 2f2a693..fc62d5a 100644 --- a/test/handle.cpp +++ b/test/handle.cpp @@ -24,7 +24,7 @@ TEST_CASE("Owning handles correctly manages reference counts", "[Handle]") // Large integers are used, since small integers are cached by CPython // and immortal from Python 3.12 on, where their reference count never // moves. These two are fresh objects whose count we fully control. - PyObject* one = Py_BuildValue("l", 1L << 40); + PyObject* one = Py_BuildValue("L", static_cast(1) << 40); // Py_BuildValue gives a new reference to be possibly stolen by // Handles. Here we increment the reference count again to make sure // that even after that reference is destroyed, we still have at least @@ -32,7 +32,7 @@ TEST_CASE("Owning handles correctly manages reference counts", "[Handle]") Py_INCREF(one); Py_ssize_t init_count = Py_REFCNT(one); - PyObject* two = Py_BuildValue("l", 1L << 41); + PyObject* two = Py_BuildValue("L", static_cast(1) << 41); Py_INCREF(two); // For the same rational as one. Py_ssize_t init_count2 = Py_REFCNT(two); @@ -352,8 +352,8 @@ TEST_CASE("Borrowing handles correctly treats reference counts", "[Handle]") { // Testing of borrowing handles is relatively easy, no matter what // happens, the reference count should never be touched. - PyObject* one = Py_BuildValue("l", 1L << 40); - PyObject* two = Py_BuildValue("l", 1L << 41); + PyObject* one = Py_BuildValue("L", static_cast(1) << 40); + PyObject* two = Py_BuildValue("L", static_cast(1) << 41); Py_ssize_t init_count = Py_REFCNT(one); Py_ssize_t init_count2 = Py_REFCNT(two); auto check_ref = [&]() {