From caa3be3243375b00e2d70d15d7ca71e8fd2a34a7 Mon Sep 17 00:00:00 2001 From: Edward Boggis-Rolfe Date: Fri, 20 Feb 2026 16:18:22 +0000 Subject: [PATCH 1/2] Improved reference count checks --- CMakePresets.json | 6 +- cmake/Canopy.cmake | 8 +++ rpc/include/rpc/internal/logger.h | 1 + rpc/include/rpc/internal/remote_pointer.h | 84 ++++++++--------------- 4 files changed, 40 insertions(+), 59 deletions(-) diff --git a/CMakePresets.json b/CMakePresets.json index 2ad7428..151bc2a 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -40,7 +40,8 @@ "CANOPY_DEBUG_ALL": "OFF", "CANOPY_ENABLE_COVERAGE": "OFF", "CANOPY_ENABLE_CLANG_TIDY": "OFF", - "CANOPY_ENABLE_CLANG_TIDY_FIX": "OFF" + "CANOPY_ENABLE_CLANG_TIDY_FIX": "OFF", + "CANOPY_ADD_REF_COUNT_CHECKS": "OFF" } }, { @@ -55,7 +56,8 @@ "CANOPY_USE_TELEMETRY": "ON", "CANOPY_USE_CONSOLE_TELEMETRY": "ON", "CANOPY_USE_TELEMETRY_RAII_LOGGING": "ON", - "CANOPY_DEBUG_DEFAULT_DESTRUCTOR": "ON" + "CANOPY_DEBUG_DEFAULT_DESTRUCTOR": "ON", + "CANOPY_ADD_REF_COUNT_CHECKS": "ON" } }, { diff --git a/cmake/Canopy.cmake b/cmake/Canopy.cmake index ce06619..f7c4c7a 100644 --- a/cmake/Canopy.cmake +++ b/cmake/Canopy.cmake @@ -60,6 +60,7 @@ if(NOT DEPENDENCIES_LOADED) # Logging and Telemetry Options # #################################################################################################################### option(CANOPY_USE_LOGGING "Turn on Canopy logging" OFF) + option(CANOPY_ADD_REF_COUNT_CHECKS "Turn on Canopy refcount checks" OFF) option(CANOPY_USE_THREAD_LOCAL_LOGGING "Turn on thread-local circular buffer logging" OFF) option(CANOPY_HANG_ON_FAILED_ASSERT "Hang on failed assert" OFF) option(CANOPY_USE_TELEMETRY "Turn on Canopy telemetry" OFF) @@ -210,6 +211,12 @@ if(NOT DEPENDENCIES_LOADED) set(CANOPY_USE_LOGGING_FLAG) endif() + if(CANOPY_ADD_REF_COUNT_CHECKS) + set(CANOPY_ADD_REF_COUNT_CHECKS_FLAG CANOPY_ADD_REF_COUNT_CHECKS) + else() + set(CANOPY_ADD_REF_COUNT_CHECKS_FLAG) + endif() + if(CANOPY_USE_THREAD_LOCAL_LOGGING) set(CANOPY_USE_THREAD_LOCAL_LOGGING_FLAG CANOPY_USE_THREAD_LOCAL_LOGGING) else() @@ -262,6 +269,7 @@ if(NOT DEPENDENCIES_LOADED) NOMINMAX _SILENCE_ALL_MS_EXT_DEPRECATION_WARNINGS ${CANOPY_USE_LOGGING_FLAG} + ${CANOPY_ADD_REF_COUNT_CHECKS} ${CANOPY_USE_THREAD_LOCAL_LOGGING_FLAG} ${CANOPY_BUILD_COROUTINE_FLAG} ${CANOPY_HANG_ON_FAILED_ASSERT_FLAG} diff --git a/rpc/include/rpc/internal/logger.h b/rpc/include/rpc/internal/logger.h index ce861cc..5d96f8a 100644 --- a/rpc/include/rpc/internal/logger.h +++ b/rpc/include/rpc/internal/logger.h @@ -89,6 +89,7 @@ extern "C" #define RPC_ERROR(format_str, ...) \ do \ { \ + RPC_ASSERT(false); \ auto formatted = fmt::format(format_str, ##__VA_ARGS__); \ RPC_LOG_BACKEND(4, formatted); \ } while (0) diff --git a/rpc/include/rpc/internal/remote_pointer.h b/rpc/include/rpc/internal/remote_pointer.h index 4449b89..2528535 100644 --- a/rpc/include/rpc/internal/remote_pointer.h +++ b/rpc/include/rpc/internal/remote_pointer.h @@ -2339,49 +2339,35 @@ namespace rpc else { // Remote object: establish optimistic reference (0→1 transition, async!) -#ifdef CANOPY_USE_LOGGING - // TELEMETRY: Check reference counts BEFORE establishing optimistic reference - long cb_shared_before = cb->shared_count_.load(std::memory_order_acquire); +#ifdef CANOPY_ADD_REF_COUNT_CHECKS + // TELEMETRY: shared_count is not touched by make_optimistic, so only check optimistic. + // optimistic_count fires a remote add_ref only on the 0→1 transition, so cb_optimistic + // can exceed inherited_optimistic when multiple local copies exist. The invariant is + // that they agree on whether a remote reference exists (both zero or both non-zero). long cb_optimistic_before = cb->optimistic_count_.load(std::memory_order_acquire); - // Get object_proxy to check inherited counts (service-level stub counts) auto casting_iface = reinterpret_cast(in.internal_get_ptr()); auto obj_proxy = casting_iface->__rpc_get_object_proxy(); - int inherited_shared_before = 0; int inherited_optimistic_before = 0; if (obj_proxy) { + int unused_shared = 0; __rpc_internal::__shared_ptr_control_block::get_object_proxy_reference_counts( - obj_proxy, inherited_shared_before, inherited_optimistic_before); + obj_proxy, unused_shared, inherited_optimistic_before); } - RPC_DEBUG("make_optimistic(shared_ptr→optimistic_ptr): BEFORE - control_block(shared={}, optimistic={}), " - "object_proxy(inherited_shared={}, inherited_optimistic={})", - cb_shared_before, + RPC_DEBUG("make_optimistic(shared_ptr→optimistic_ptr): BEFORE - control_block(optimistic={}), " + "object_proxy(inherited_optimistic={})", cb_optimistic_before, - inherited_shared_before, inherited_optimistic_before); - // NOTE: It's EXPECTED that before make_optimistic, we have shared=1, optimistic=0 - // This is the normal state when converting a shared_ptr to optimistic_ptr - // The validation below checks this expected state - - // Verify control block shared count matches object proxy shared count - if (obj_proxy && cb_shared_before != inherited_shared_before) + if (obj_proxy && (cb_optimistic_before == 0) != (inherited_optimistic_before == 0)) { - RPC_ERROR( - "make_optimistic: Control block shared count ({}) doesn't match object_proxy shared count ({})", - cb_shared_before, - inherited_shared_before); - } - - // Verify control block optimistic count matches object proxy optimistic count - if (obj_proxy && cb_optimistic_before != inherited_optimistic_before) - { - RPC_ERROR("make_optimistic: Control block optimistic count ({}) doesn't match object_proxy optimistic " - "count ({})", + RPC_ERROR("make_optimistic: Control block optimistic count ({}) and object_proxy optimistic " + "count ({}) disagree on whether a remote reference exists", cb_optimistic_before, inherited_optimistic_before); + RPC_ASSERT(false); } #endif @@ -2391,50 +2377,34 @@ namespace rpc CO_RETURN err; // Failed to establish remote reference } -#ifdef CANOPY_USE_LOGGING - // TELEMETRY: Check reference counts AFTER establishing optimistic reference - long cb_shared_after = cb->shared_count_.load(std::memory_order_acquire); +#ifdef CANOPY_ADD_REF_COUNT_CHECKS long cb_optimistic_after = cb->optimistic_count_.load(std::memory_order_acquire); - int inherited_shared_after = 0; int inherited_optimistic_after = 0; if (obj_proxy) { + int unused_shared = 0; __rpc_internal::__shared_ptr_control_block::get_object_proxy_reference_counts( - obj_proxy, inherited_shared_after, inherited_optimistic_after); + obj_proxy, unused_shared, inherited_optimistic_after); } - RPC_DEBUG("make_optimistic(shared_ptr→optimistic_ptr): AFTER - control_block(shared={}, optimistic={}), " - "object_proxy(inherited_shared={}, inherited_optimistic={})", - cb_shared_after, + RPC_DEBUG("make_optimistic(shared_ptr→optimistic_ptr): AFTER - control_block(optimistic={}), " + "object_proxy(inherited_optimistic={})", cb_optimistic_after, - inherited_shared_after, inherited_optimistic_after); - // After make_optimistic, we expect shared=1, optimistic=1 (both counts should be positive) - if (cb_shared_after == 0 || cb_optimistic_after == 0) + if (cb_optimistic_after == 0) { - RPC_ERROR("make_optimistic: Control block count zero AFTER increment - shared={} optimistic={} (both " - "should be > 0)", - cb_shared_after, - cb_optimistic_after); - } - - // Verify control block shared count matches object proxy shared count - if (obj_proxy && cb_shared_after != inherited_shared_after) - { - RPC_ERROR("make_optimistic: Control block shared count ({}) doesn't match object_proxy shared count " - "({}) AFTER", - cb_shared_after, - inherited_shared_after); + RPC_ERROR("make_optimistic: Control block optimistic count is zero AFTER increment"); + RPC_ASSERT(false); } - // Verify control block optimistic count matches object proxy optimistic count - if (obj_proxy && cb_optimistic_after != inherited_optimistic_after) + if (obj_proxy && (cb_optimistic_after == 0) != (inherited_optimistic_after == 0)) { - RPC_ERROR("make_optimistic: Control block optimistic count ({}) doesn't match object_proxy optimistic " - "count ({}) AFTER", + RPC_ERROR("make_optimistic: Control block optimistic count ({}) and object_proxy optimistic " + "count ({}) disagree on whether a remote reference exists AFTER increment", cb_optimistic_after, inherited_optimistic_after); + RPC_ASSERT(false); } #endif @@ -2467,7 +2437,7 @@ namespace rpc else { // Remote object: establish optimistic reference (0→1 transition, async!) -#ifdef CANOPY_USE_LOGGING +#ifdef CANOPY_ADD_REF_COUNT_CHECKS // TELEMETRY: Check reference counts BEFORE establishing optimistic reference long cb_shared_before = cb->shared_count_.load(std::memory_order_acquire); long cb_optimistic_before = cb->optimistic_count_.load(std::memory_order_acquire); @@ -2515,7 +2485,7 @@ namespace rpc CO_RETURN err; // Failed (likely OBJECT_GONE) } -#ifdef CANOPY_USE_LOGGING +#ifdef CANOPY_ADD_REF_COUNT_CHECKS // TELEMETRY: Check reference counts AFTER establishing optimistic reference long cb_shared_after = cb->shared_count_.load(std::memory_order_acquire); long cb_optimistic_after = cb->optimistic_count_.load(std::memory_order_acquire); From cfbd67045dc8b7a0420c1c7b41ead9800c93ec86 Mon Sep 17 00:00:00 2001 From: Edward Boggis-Rolfe Date: Fri, 20 Feb 2026 16:30:22 +0000 Subject: [PATCH 2/2] changed overeager assert on error check into a config option --- CMakePresets.json | 6 ++++-- cmake/Canopy.cmake | 10 +++++++++- rpc/include/rpc/internal/logger.h | 20 +++++++++++++++++++- 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/CMakePresets.json b/CMakePresets.json index 151bc2a..2897e89 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -41,7 +41,8 @@ "CANOPY_ENABLE_COVERAGE": "OFF", "CANOPY_ENABLE_CLANG_TIDY": "OFF", "CANOPY_ENABLE_CLANG_TIDY_FIX": "OFF", - "CANOPY_ADD_REF_COUNT_CHECKS": "OFF" + "CANOPY_ADD_REF_COUNT_CHECKS": "OFF", + "CANOPY_ASSERT_ON_LOGGER_ERROR": "OFF" } }, { @@ -57,7 +58,8 @@ "CANOPY_USE_CONSOLE_TELEMETRY": "ON", "CANOPY_USE_TELEMETRY_RAII_LOGGING": "ON", "CANOPY_DEBUG_DEFAULT_DESTRUCTOR": "ON", - "CANOPY_ADD_REF_COUNT_CHECKS": "ON" + "CANOPY_ADD_REF_COUNT_CHECKS": "ON", + "CANOPY_ASSERT_ON_LOGGER_ERROR": "ON" } }, { diff --git a/cmake/Canopy.cmake b/cmake/Canopy.cmake index f7c4c7a..e882088 100644 --- a/cmake/Canopy.cmake +++ b/cmake/Canopy.cmake @@ -61,6 +61,7 @@ if(NOT DEPENDENCIES_LOADED) # #################################################################################################################### option(CANOPY_USE_LOGGING "Turn on Canopy logging" OFF) option(CANOPY_ADD_REF_COUNT_CHECKS "Turn on Canopy refcount checks" OFF) + option(CANOPY_ASSERT_ON_LOGGER_ERROR "Turn on asserts when there is a logger error") option(CANOPY_USE_THREAD_LOCAL_LOGGING "Turn on thread-local circular buffer logging" OFF) option(CANOPY_HANG_ON_FAILED_ASSERT "Hang on failed assert" OFF) option(CANOPY_USE_TELEMETRY "Turn on Canopy telemetry" OFF) @@ -217,6 +218,12 @@ if(NOT DEPENDENCIES_LOADED) set(CANOPY_ADD_REF_COUNT_CHECKS_FLAG) endif() + if(CANOPY_ASSERT_ON_LOGGER_ERROR) + set(CANOPY_ASSERT_ON_LOGGER_ERROR_FLAG CANOPY_ASSERT_ON_LOGGER_ERROR) + else() + set(CANOPY_ASSERT_ON_LOGGER_ERROR_FLAG) + endif() + if(CANOPY_USE_THREAD_LOCAL_LOGGING) set(CANOPY_USE_THREAD_LOCAL_LOGGING_FLAG CANOPY_USE_THREAD_LOCAL_LOGGING) else() @@ -269,7 +276,8 @@ if(NOT DEPENDENCIES_LOADED) NOMINMAX _SILENCE_ALL_MS_EXT_DEPRECATION_WARNINGS ${CANOPY_USE_LOGGING_FLAG} - ${CANOPY_ADD_REF_COUNT_CHECKS} + ${CANOPY_ADD_REF_COUNT_CHECKS_FLAG} + ${CANOPY_ASSERT_ON_LOGGER_ERROR_FLAG} ${CANOPY_USE_THREAD_LOCAL_LOGGING_FLAG} ${CANOPY_BUILD_COROUTINE_FLAG} ${CANOPY_HANG_ON_FAILED_ASSERT_FLAG} diff --git a/rpc/include/rpc/internal/logger.h b/rpc/include/rpc/internal/logger.h index 5d96f8a..e9350c9 100644 --- a/rpc/include/rpc/internal/logger.h +++ b/rpc/include/rpc/internal/logger.h @@ -86,6 +86,7 @@ extern "C" RPC_LOG_BACKEND(3, formatted); \ } while (0) +#ifdef CANOPY_ASSERT_ON_LOGGER_ERROR #define RPC_ERROR(format_str, ...) \ do \ { \ @@ -93,14 +94,31 @@ extern "C" auto formatted = fmt::format(format_str, ##__VA_ARGS__); \ RPC_LOG_BACKEND(4, formatted); \ } while (0) +#else +#define RPC_ERROR(format_str, ...) \ + do \ + { \ + auto formatted = fmt::format(format_str, ##__VA_ARGS__); \ + RPC_LOG_BACKEND(4, formatted); \ + } while (0) +#endif +#ifdef CANOPY_ASSERT_ON_LOGGER_ERROR +#define RPC_CRITICAL(format_str, ...) \ + do \ + { \ + RPC_ASSERT(false); \ + auto formatted = fmt::format(format_str, ##__VA_ARGS__); \ + RPC_LOG_BACKEND(5, formatted); \ + } while (0) +#else #define RPC_CRITICAL(format_str, ...) \ do \ { \ auto formatted = fmt::format(format_str, ##__VA_ARGS__); \ RPC_LOG_BACKEND(5, formatted); \ } while (0) - +#endif #else // Disabled logging - all macros are no-ops #define RPC_DEBUG(format_str, ...)