From 6b854ea78d359b60928d7dd090f26c7c3fbbf00a Mon Sep 17 00:00:00 2001 From: Gary Wolfman Date: Sat, 4 Jul 2026 23:45:28 -0400 Subject: [PATCH] Fix audit findings L3, L5: LIBSTATS_PORTABLE option; cache doc accuracy; remove CachedProperty L3: LIBSTATS_PORTABLE=OFF option wraps the global /arch: block on Windows. L5: Fix lock-free doc claim; add torn-pair note; remove CachedProperty. 46/46 tests pass. Co-Authored-By: Oz --- CMakeLists.txt | 57 ++++++++++++++++++----------- include/core/distribution_cache.h | 59 +++++++++---------------------- 2 files changed, 53 insertions(+), 63 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index df7dae44..f91f50b2 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -7,6 +7,16 @@ endif() # Verbose messaging control option(LIBSTATS_VERBOSE_BUILD "Enable verbose build messages for debugging" OFF) +# LIBSTATS_PORTABLE: skip the global /arch:AVX512 / /arch:AVX2 flag on Windows MSVC / Clang-cl +# builds so that the binary is safe to distribute across machines with different ISA capabilities. +# Per-file ISA flags on the SIMD kernel TUs (applied by +# cmake/SIMDDetection.cmake:apply_simd_source_flags) remain active and unaffected. Default OFF +# (native performance build). Recommended ON for pylibstats wheel builds. See libstats issue +# tracker for the long-term plan to extend per-file flags to all TUs on Windows. +option( + LIBSTATS_PORTABLE + "Skip global /arch: SIMD flag on Windows builds (safe for distributable wheels; OFF by default)" + OFF) # Threading system preference control option(LIBSTATS_FORCE_TBB @@ -682,28 +692,33 @@ endif() # source-file-specific flags (cmake/SIMDDetection.cmake) - All platforms: Definitions are set by # SIMDDetection.cmake based on detection -# Windows compilers: Use highest detected SIMD level as global flag. SIMDDetection.cmake has already -# run by this point and set LIBSTATS_HAS_AVX512 etc. -if(CMAKE_CXX_COMPILER_ID STREQUAL "MSVC" AND CMAKE_SIZEOF_VOID_P EQUAL 8) - if(LIBSTATS_HAS_AVX512) - add_compile_options(/arch:AVX512) - message(STATUS "Applied MSVC x64 SIMD flags: /arch:AVX512") - else() - add_compile_options(/arch:AVX2) - message(STATUS "Applied MSVC x64 SIMD flags: /arch:AVX2") - endif() - -elseif( - CMAKE_CXX_COMPILER_ID MATCHES "Clang" - AND WIN32 - AND CMAKE_SIZEOF_VOID_P EQUAL 8) - if(LIBSTATS_HAS_AVX512) - add_compile_options(-mavx512f) - message(STATUS "Applied Clang-cl x64 SIMD flags: -mavx512f") - else() - add_compile_options(-mavx2) - message(STATUS "Applied Clang-cl x64 SIMD flags: -mavx2") +# Windows compilers: apply the build-machine SIMD level as a global flag. LIBSTATS_PORTABLE=ON skips +# this block so the binary is distributable to machines with lower ISA than the build host. Per-file +# flags from cmake/SIMDDetection.cmake:apply_simd_source_flags() remain active. +if(NOT LIBSTATS_PORTABLE) + if(CMAKE_CXX_COMPILER_ID STREQUAL "MSVC" AND CMAKE_SIZEOF_VOID_P EQUAL 8) + if(LIBSTATS_HAS_AVX512) + add_compile_options(/arch:AVX512) + message(STATUS "Applied MSVC x64 SIMD flags: /arch:AVX512") + else() + add_compile_options(/arch:AVX2) + message(STATUS "Applied MSVC x64 SIMD flags: /arch:AVX2") + endif() + elseif( + CMAKE_CXX_COMPILER_ID MATCHES "Clang" + AND WIN32 + AND CMAKE_SIZEOF_VOID_P EQUAL 8) + if(LIBSTATS_HAS_AVX512) + add_compile_options(-mavx512f) + message(STATUS "Applied Clang-cl x64 SIMD flags: -mavx512f") + else() + add_compile_options(-mavx2) + message(STATUS "Applied Clang-cl x64 SIMD flags: -mavx2") + endif() endif() +else() + message(STATUS "LIBSTATS_PORTABLE=ON: skipping global Windows /arch: flag " + "(SIMD kernel TUs still use per-file flags from SIMDDetection.cmake)") endif() # IMPORTANT: SIMD compile definitions are handled by cmake/SIMDDetection.cmake That system detects diff --git a/include/core/distribution_cache.h b/include/core/distribution_cache.h index f9228616..db14d847 100644 --- a/include/core/distribution_cache.h +++ b/include/core/distribution_cache.h @@ -21,11 +21,23 @@ namespace stats { * * ### Dual-flag pattern (cache_valid_ + cacheValidAtomic_) * Two flags represent the same state deliberately: - * - cacheValidAtomic_ enables a lock-free fast path in getCachedValue(). An - * acquire load synchronizes-with the release store in updateCacheUnsafe(), - * ensuring cache_valid_ and the cached values are visible before the atomic - * is seen as true. - * - cache_valid_ is the plain bool read under shared_lock in the slow path. + * - cacheValidAtomic_ enables a contention-reduced fast path in + * getCachedValue(): the atomic load (acquire) avoids a lock entirely when + * the cache is stale (proceeding directly to the unique-lock slow path); + * when valid it still acquires shared_lock before reading values. This is + * NOT a fully lock-free read — it is a contention-reduced path that avoids + * the exclusive (write) lock on the hot path. + * - cache_valid_ is the plain bool read under shared_lock; it serves as the + * ground-truth flag for code paths that already hold cache_mutex_. + * + * ### Per-parameter atomic fast paths (derived classes) + * Some derived classes (e.g. GaussianDistribution) expose individual + * parameter atomics (atomicMean_, atomicStdDev_) for lock-free + * getMean()/getStdDev() calls. Each value is individually consistent, but + * reading two atomics across separate calls does not form an atomic pair: + * a concurrent setParameters() between the two reads yields a torn snapshot. + * Callers that need a consistent pair of parameters should use the snapshot + * pattern (shared_lock, read both under the same lock). * * v2.0.0 removed the old derived-class cacheValidAtomic_ shadows; this base * member is now the single atomic cache-validity flag for all distributions. @@ -77,41 +89,4 @@ class ThreadSafeCacheManager { auto getCachedValue(Func&& accessor) const -> decltype(accessor()); }; -// ============================================================================= -// CACHED PROPERTY TEMPLATE -// ============================================================================= - -/** - * @brief Template helper for cached statistical properties - * @tparam PropertyType Type of cached property - * - * @warning **Not thread-safe.** This class has no synchronisation of its own. - * Use it only within a scope that already holds an appropriate lock from - * ThreadSafeCacheManager (e.g. under cache_mutex_ unique_lock). Concurrent - * access from multiple threads without external locking is a data race. - * Full per-property atomic protection can be considered in a future v2.x - * cache redesign. v2.0.0 guarantees noexcept moves by not moving mutex/cache - * state and by invalidating/rebuilding caches after moves. - */ -template -class CachedProperty { - private: - mutable PropertyType value_; - mutable bool valid_{false}; - - public: - template - PropertyType get(ComputeFunc&& compute_func) const { - if (!valid_) { - value_ = compute_func(); - valid_ = true; - } - return value_; - } - - void invalidate() noexcept { valid_ = false; } - - bool isValid() const noexcept { return valid_; } -}; - } // namespace stats