From 6b26b42cf53da275da402c8b9b9b0f27c8c49b35 Mon Sep 17 00:00:00 2001 From: shijiashuai Date: Fri, 22 May 2026 10:28:29 +0800 Subject: [PATCH] hardening: close header-only safety gaps Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- examples/03-modern-cpp/include/buffer.hpp | 18 ++++-- .../03-modern-cpp/include/compile_time.hpp | 2 +- .../03-modern-cpp/include/ranges_utils.hpp | 22 +++---- .../include/lock_free_queue.hpp | 63 ++++++++++++------- .../changes/header-only-hardening/design.md | 27 ++++++++ .../changes/header-only-hardening/proposal.md | 26 ++++++++ .../specs/concurrency-multithreading/spec.md | 17 +++++ .../specs/modern-cpp-features/spec.md | 17 +++++ .../changes/header-only-hardening/tasks.md | 22 +++++++ .../unit/concurrency/lock_free_queue_test.cpp | 26 ++++++++ tests/unit/modern_cpp/CMakeLists.txt | 14 +++++ .../modern_cpp/modern_cpp_examples_test.cpp | 12 ++++ .../modern_cpp_header_only_smoke_extra.cpp | 11 ++++ .../modern_cpp_header_only_smoke_test.cpp | 22 +++++++ 14 files changed, 257 insertions(+), 42 deletions(-) create mode 100644 openspec/changes/header-only-hardening/design.md create mode 100644 openspec/changes/header-only-hardening/proposal.md create mode 100644 openspec/changes/header-only-hardening/specs/concurrency-multithreading/spec.md create mode 100644 openspec/changes/header-only-hardening/specs/modern-cpp-features/spec.md create mode 100644 openspec/changes/header-only-hardening/tasks.md create mode 100644 tests/unit/modern_cpp/modern_cpp_header_only_smoke_extra.cpp create mode 100644 tests/unit/modern_cpp/modern_cpp_header_only_smoke_test.cpp diff --git a/examples/03-modern-cpp/include/buffer.hpp b/examples/03-modern-cpp/include/buffer.hpp index a014c74..680b4d9 100644 --- a/examples/03-modern-cpp/include/buffer.hpp +++ b/examples/03-modern-cpp/include/buffer.hpp @@ -86,6 +86,7 @@ class Buffer { } size_t size() const { return size_; } + bool empty() const { return size_ == 0 || data_ == nullptr; } char* data() { return data_; } const char* data() const { return data_; } @@ -104,6 +105,15 @@ class Buffer { } }; +inline void observe_buffer(const Buffer& buf) { + if (buf.empty()) { + return; + } + + volatile char c = buf.data()[0]; + (void)c; +} + //------------------------------------------------------------------------------ // Functions demonstrating copy vs move //------------------------------------------------------------------------------ @@ -112,18 +122,14 @@ class Buffer { * @brief Process buffer by copy (expensive) */ inline void process_by_copy(Buffer buf) { - // Do something with buf - volatile char c = buf.data()[0]; - (void)c; + observe_buffer(buf); } /** * @brief Process buffer by const reference (no copy) */ inline void process_by_ref(const Buffer& buf) { - // Do something with buf - volatile char c = buf.data()[0]; - (void)c; + observe_buffer(buf); } } // namespace hpc::move_semantics diff --git a/examples/03-modern-cpp/include/compile_time.hpp b/examples/03-modern-cpp/include/compile_time.hpp index e61b2ad..699c65d 100644 --- a/examples/03-modern-cpp/include/compile_time.hpp +++ b/examples/03-modern-cpp/include/compile_time.hpp @@ -26,7 +26,7 @@ namespace hpc::compile_time { /** * @brief Runtime factorial (for comparison) */ -int64_t factorial_runtime(int n) { +inline int64_t factorial_runtime(int n) { int64_t result = 1; for (int i = 2; i <= n; ++i) { result *= i; diff --git a/examples/03-modern-cpp/include/ranges_utils.hpp b/examples/03-modern-cpp/include/ranges_utils.hpp index 50f7788..30a59ac 100644 --- a/examples/03-modern-cpp/include/ranges_utils.hpp +++ b/examples/03-modern-cpp/include/ranges_utils.hpp @@ -29,7 +29,7 @@ namespace hpc::ranges { /** * @brief Transform using raw loop */ -void transform_raw_loop(const std::vector& input, std::vector& output) { +inline void transform_raw_loop(const std::vector& input, std::vector& output) { output.resize(input.size()); for (size_t i = 0; i < input.size(); ++i) { output[i] = input[i] * 2 + 1; @@ -39,7 +39,7 @@ void transform_raw_loop(const std::vector& input, std::vector& output) /** * @brief Transform using std::transform */ -void transform_algorithm(const std::vector& input, std::vector& output) { +inline void transform_algorithm(const std::vector& input, std::vector& output) { output.resize(input.size()); std::transform(input.begin(), input.end(), output.begin(), [](int x) { return x * 2 + 1; }); } @@ -47,7 +47,7 @@ void transform_algorithm(const std::vector& input, std::vector& output /** * @brief Transform using ranges */ -void transform_ranges(const std::vector& input, std::vector& output) { +inline void transform_ranges(const std::vector& input, std::vector& output) { output.resize(input.size()); std::ranges::transform(input, output.begin(), [](int x) { return x * 2 + 1; }); } @@ -59,7 +59,7 @@ void transform_ranges(const std::vector& input, std::vector& output) { /** * @brief Filter using raw loop */ -std::vector filter_raw_loop(const std::vector& input) { +inline std::vector filter_raw_loop(const std::vector& input) { std::vector output; output.reserve(input.size() / 2); // Estimate for (int x : input) { @@ -73,7 +73,7 @@ std::vector filter_raw_loop(const std::vector& input) { /** * @brief Filter using std::copy_if */ -std::vector filter_algorithm(const std::vector& input) { +inline std::vector filter_algorithm(const std::vector& input) { std::vector output; output.reserve(input.size() / 2); std::copy_if(input.begin(), input.end(), std::back_inserter(output), @@ -84,7 +84,7 @@ std::vector filter_algorithm(const std::vector& input) { /** * @brief Filter using ranges view (lazy) */ -auto filter_ranges_view(const std::vector& input) { +inline auto filter_ranges_view(const std::vector& input) { return input | std::views::filter([](int x) { return x % 2 == 0; }); } @@ -95,7 +95,7 @@ auto filter_ranges_view(const std::vector& input) { /** * @brief Filter then transform using raw loops */ -std::vector chain_raw_loop(const std::vector& input) { +inline std::vector chain_raw_loop(const std::vector& input) { std::vector output; output.reserve(input.size() / 2); for (int x : input) { @@ -109,7 +109,7 @@ std::vector chain_raw_loop(const std::vector& input) { /** * @brief Filter then transform using ranges (lazy, single pass) */ -auto chain_ranges_view(const std::vector& input) { +inline auto chain_ranges_view(const std::vector& input) { return input | std::views::filter([](int x) { return x % 2 == 0; }) | std::views::transform([](int x) { return x * 2 + 1; }); } @@ -133,7 +133,7 @@ std::vector> to_vector(R&& range) { /** * @brief Sum using raw loop */ -int64_t sum_raw_loop(const std::vector& input) { +inline int64_t sum_raw_loop(const std::vector& input) { int64_t sum = 0; for (int x : input) { sum += x; @@ -144,7 +144,7 @@ int64_t sum_raw_loop(const std::vector& input) { /** * @brief Sum using std::accumulate */ -int64_t sum_algorithm(const std::vector& input) { +inline int64_t sum_algorithm(const std::vector& input) { return std::accumulate(input.begin(), input.end(), int64_t{0}); } @@ -154,7 +154,7 @@ int64_t sum_algorithm(const std::vector& input) { * C++23 introduces std::ranges::fold_left for this purpose. * In C++20 we iterate over a ranges::subrange to stay within the ranges API. */ -int64_t sum_ranges(const std::vector& input) { +inline int64_t sum_ranges(const std::vector& input) { int64_t sum = 0; for (int x : std::ranges::subrange(input)) { sum += x; diff --git a/examples/05-concurrency/include/lock_free_queue.hpp b/examples/05-concurrency/include/lock_free_queue.hpp index cd5c7df..4fb7e24 100644 --- a/examples/05-concurrency/include/lock_free_queue.hpp +++ b/examples/05-concurrency/include/lock_free_queue.hpp @@ -19,6 +19,7 @@ #include #include #include +#include #include "concurrency_utils.hpp" @@ -150,7 +151,7 @@ class MPMCQueue { struct Cell { std::atomic sequence; - T data; + std::optional data; }; public: @@ -166,70 +167,84 @@ class MPMCQueue { * @return true if successful, false if queue is full */ bool push(const T& value) { + return push_impl(value); + } + + /** + * @brief Push an element with move semantics (thread-safe) + * @param value Element to push + * @return true if successful, false if queue is full + */ + bool push(T&& value) { + return push_impl(std::move(value)); + } + + /** + * @brief Pop an element from the queue (thread-safe) + * @return optional containing the value, or empty if queue is empty + */ + std::optional pop() { Cell* cell; - size_t pos = enqueue_pos_.load(std::memory_order_relaxed); + size_t pos = dequeue_pos_.load(std::memory_order_relaxed); int backoff = 1; for (;;) { cell = &cells_[pos & MASK]; size_t seq = cell->sequence.load(std::memory_order_acquire); - intptr_t diff = static_cast(seq) - static_cast(pos); + intptr_t diff = static_cast(seq) - static_cast(pos + 1); if (diff == 0) { - if (enqueue_pos_.compare_exchange_weak(pos, pos + 1, std::memory_order_relaxed)) { + if (dequeue_pos_.compare_exchange_weak(pos, pos + 1, std::memory_order_relaxed)) { break; } } else if (diff < 0) { - return false; + return std::nullopt; } else { for (int i = 0; i < backoff; ++i) { std::this_thread::yield(); } backoff = std::min(backoff * 2, 64); - pos = enqueue_pos_.load(std::memory_order_relaxed); + pos = dequeue_pos_.load(std::memory_order_relaxed); } } - cell->data = value; - cell->sequence.store(pos + 1, std::memory_order_release); - return true; + T value = std::move(*cell->data); + cell->data.reset(); + cell->sequence.store(pos + Capacity, std::memory_order_release); + return value; } - /** - * @brief Pop an element from the queue (thread-safe) - * @return optional containing the value, or empty if queue is empty - */ - std::optional pop() { +private: + template + bool push_impl(U&& value) { Cell* cell; - size_t pos = dequeue_pos_.load(std::memory_order_relaxed); + size_t pos = enqueue_pos_.load(std::memory_order_relaxed); int backoff = 1; for (;;) { cell = &cells_[pos & MASK]; size_t seq = cell->sequence.load(std::memory_order_acquire); - intptr_t diff = static_cast(seq) - static_cast(pos + 1); + intptr_t diff = static_cast(seq) - static_cast(pos); if (diff == 0) { - if (dequeue_pos_.compare_exchange_weak(pos, pos + 1, std::memory_order_relaxed)) { + if (enqueue_pos_.compare_exchange_weak(pos, pos + 1, std::memory_order_relaxed)) { break; } } else if (diff < 0) { - return std::nullopt; + return false; } else { for (int i = 0; i < backoff; ++i) { std::this_thread::yield(); } backoff = std::min(backoff * 2, 64); - pos = dequeue_pos_.load(std::memory_order_relaxed); + pos = enqueue_pos_.load(std::memory_order_relaxed); } } - T value = std::move(cell->data); - cell->sequence.store(pos + Capacity, std::memory_order_release); - return value; + cell->data.emplace(std::forward(value)); + cell->sequence.store(pos + 1, std::memory_order_release); + return true; } - -private: static constexpr size_t MASK = Capacity - 1; alignas(hpc::core::CACHE_LINE_SIZE) Cell cells_[Capacity]; diff --git a/openspec/changes/header-only-hardening/design.md b/openspec/changes/header-only-hardening/design.md new file mode 100644 index 0000000..ba0de90 --- /dev/null +++ b/openspec/changes/header-only-hardening/design.md @@ -0,0 +1,27 @@ +# Design: Header-Only Hardening + +## Overview + +This change deepens three fragile modules by moving hidden invariants behind their own interfaces instead of forcing callers and future maintainers to remember them. + +## Design Decisions + +### 1. `MPMCQueue` owns payload lifetime explicitly + +The queue cell will store `std::optional` instead of a permanently live `T`. This removes the accidental default-constructible requirement and gives the module a real interface for generic payloads. The queue will also expose an rvalue enqueue path so move-only and expensive-to-copy types can cross the seam safely. + +### 2. `Buffer` processing absorbs the empty-buffer invariant + +The move-semantics example currently requires callers to know that `buf.data()[0]` is only valid when `size() > 0`. That is a shallow interface. The hardening change moves the empty-buffer check into the processing helpers so the module remains safe for default-constructed and zero-sized buffers. + +### 3. Modern C++ example headers become explicitly link-safe + +The repository treats these example headers as header-only modules. Non-template free functions defined in headers will be marked `inline`, and a dedicated multi-translation-unit smoke target will prove the contract during normal builds. + +## Testing Strategy + +1. Add a failing queue test that uses non-default-constructible and move-only payloads. +2. Add a failing move-semantics test that exercises empty buffers. +3. Add a failing multi-translation-unit smoke target for the modern C++ headers. +4. Apply the minimal production changes to make each loop pass. +5. Re-run the full preset-driven debug validation path. diff --git a/openspec/changes/header-only-hardening/proposal.md b/openspec/changes/header-only-hardening/proposal.md new file mode 100644 index 0000000..963eb31 --- /dev/null +++ b/openspec/changes/header-only-hardening/proposal.md @@ -0,0 +1,26 @@ +# Proposal: Header-Only Hardening + +## Summary + +Harden the header-only teaching modules where the current interface promises more than the implementation can safely deliver, focusing on queue payload support, empty-buffer safety, and multi-translation-unit link safety. + +## Motivation + +The repository is in closure and hardening mode. Several teaching modules already pass today's tests, but still carry design drift that weakens long-term maintainability: + +1. `MPMCQueue` documents generic payload support, yet its implementation requires default-constructible element types and does not expose a move-oriented enqueue path. +2. The move-semantics example leaks a hidden caller invariant: `process_by_copy` and `process_by_ref` dereference element zero even when the buffer is empty. +3. Header-defined non-template functions in the modern C++ examples are not explicitly `inline`, which makes the header-only contract fragile across multiple translation units. + +## Scope + +- Harden the concurrency queue interface so the implementation matches its documented payload story. +- Harden the move-semantics example against empty-buffer use. +- Enforce link-safe header-only behavior in modern C++ example headers. +- Add targeted regression coverage for each fix. + +## Out of Scope + +- New performance claims or benchmark rewrites. +- Broad API redesign across unrelated modules. +- Reworking the docs site or unrelated repository governance surfaces. diff --git a/openspec/changes/header-only-hardening/specs/concurrency-multithreading/spec.md b/openspec/changes/header-only-hardening/specs/concurrency-multithreading/spec.md new file mode 100644 index 0000000..ef7f6c5 --- /dev/null +++ b/openspec/changes/header-only-hardening/specs/concurrency-multithreading/spec.md @@ -0,0 +1,17 @@ +# Concurrency and Multithreading + +## ADDED Requirements + +### Requirement: Queue Payload Lifetime Safety + +THE Example_Module SHALL allow the lock-free queue interface to store payload types without requiring a default constructor. + +#### Scenario: MPMC queue accepts non-default-constructible payloads + +- **WHEN** a maintainer instantiates `MPMCQueue` with a payload type that is move-constructible but has no default constructor +- **THEN** the queue builds successfully and preserves payload values through push/pop operations + +#### Scenario: MPMC queue preserves move-only payloads + +- **WHEN** a maintainer enqueues move-only payloads into `MPMCQueue` +- **THEN** the queue transfers ownership correctly and pops each payload exactly once diff --git a/openspec/changes/header-only-hardening/specs/modern-cpp-features/spec.md b/openspec/changes/header-only-hardening/specs/modern-cpp-features/spec.md new file mode 100644 index 0000000..f8bbc36 --- /dev/null +++ b/openspec/changes/header-only-hardening/specs/modern-cpp-features/spec.md @@ -0,0 +1,17 @@ +# Modern C++ Features + +## ADDED Requirements + +### Requirement: Header-Only Example Safety + +THE Example_Module SHALL keep header-defined utilities safe to include from multiple translation units. + +#### Scenario: Header-only examples link from multiple translation units + +- **WHEN** the modern C++ example headers are included from more than one translation unit in the same target +- **THEN** the target links successfully without duplicate symbol errors + +#### Scenario: Empty buffers are safe to observe + +- **WHEN** the move-semantics helper functions process a default-constructed or zero-sized `Buffer` +- **THEN** they do not dereference invalid memory and complete without crashing diff --git a/openspec/changes/header-only-hardening/tasks.md b/openspec/changes/header-only-hardening/tasks.md new file mode 100644 index 0000000..937fb2b --- /dev/null +++ b/openspec/changes/header-only-hardening/tasks.md @@ -0,0 +1,22 @@ +# Tasks: Header-Only Hardening + +## 1. Queue payload hardening + +- [x] Add regression coverage showing `MPMCQueue` handles non-default-constructible and move-only payloads. +- [x] Refactor queue cell storage so element lifetime matches the queue seam. +- [x] Add move enqueue support without weakening existing behavior. + +## 2. Empty-buffer safety + +- [x] Add regression coverage for processing default-constructed and zero-sized buffers. +- [x] Move the empty-buffer invariant behind the move-semantics helper interface. + +## 3. Header-only link safety + +- [x] Add a multi-translation-unit smoke target that includes the modern C++ headers from more than one source file. +- [x] Mark header-defined non-template free functions `inline` where needed. + +## 4. Validation + +- [x] Run the targeted red/green loops for the concurrency and modern C++ test surfaces. +- [x] Run `cmake --preset=debug && cmake --build build/debug && ctest --preset=debug`. diff --git a/tests/unit/concurrency/lock_free_queue_test.cpp b/tests/unit/concurrency/lock_free_queue_test.cpp index 53546a8..7a63782 100644 --- a/tests/unit/concurrency/lock_free_queue_test.cpp +++ b/tests/unit/concurrency/lock_free_queue_test.cpp @@ -18,6 +18,22 @@ namespace hpc::concurrency::test { +namespace { + +struct MoveOnlyPayload { + int value; + + explicit MoveOnlyPayload(int v) : value(v) {} + + MoveOnlyPayload() = delete; + MoveOnlyPayload(const MoveOnlyPayload&) = delete; + MoveOnlyPayload& operator=(const MoveOnlyPayload&) = delete; + MoveOnlyPayload(MoveOnlyPayload&&) noexcept = default; + MoveOnlyPayload& operator=(MoveOnlyPayload&&) noexcept = default; +}; + +} // namespace + //------------------------------------------------------------------------------ // SPSCQueue Tests //------------------------------------------------------------------------------ @@ -212,6 +228,16 @@ TEST(MPMCQueueTest, ConcurrentMultipleProducersConsumers) { EXPECT_EQ(items_consumed.load(), expected); } +TEST(MPMCQueueTest, SupportsNonDefaultConstructiblePayloads) { + MPMCQueue queue; + + EXPECT_TRUE(queue.push(MoveOnlyPayload{7})); + + auto value = queue.pop(); + ASSERT_TRUE(value.has_value()); + EXPECT_EQ(value->value, 7); +} + //------------------------------------------------------------------------------ // Stress Tests //------------------------------------------------------------------------------ diff --git a/tests/unit/modern_cpp/CMakeLists.txt b/tests/unit/modern_cpp/CMakeLists.txt index 5171452..3573b6e 100644 --- a/tests/unit/modern_cpp/CMakeLists.txt +++ b/tests/unit/modern_cpp/CMakeLists.txt @@ -15,3 +15,17 @@ hpc_set_compiler_options(modern_cpp_examples_test) hpc_enable_sanitizers(modern_cpp_examples_test) gtest_add_tests(TARGET modern_cpp_examples_test) + +add_executable(modern_cpp_header_only_smoke_test + modern_cpp_header_only_smoke_test.cpp + modern_cpp_header_only_smoke_extra.cpp +) + +target_include_directories(modern_cpp_header_only_smoke_test PRIVATE + ${CMAKE_SOURCE_DIR}/examples/03-modern-cpp/include +) + +hpc_set_compiler_options(modern_cpp_header_only_smoke_test) +hpc_enable_sanitizers(modern_cpp_header_only_smoke_test) + +add_test(NAME modern_cpp_header_only_smoke_test COMMAND modern_cpp_header_only_smoke_test) diff --git a/tests/unit/modern_cpp/modern_cpp_examples_test.cpp b/tests/unit/modern_cpp/modern_cpp_examples_test.cpp index a7a70e2..19d06c1 100644 --- a/tests/unit/modern_cpp/modern_cpp_examples_test.cpp +++ b/tests/unit/modern_cpp/modern_cpp_examples_test.cpp @@ -58,6 +58,18 @@ TEST(MoveSemanticsExamplesTest, ProcessByCopyAndRefCountDifferently) { EXPECT_EQ(metrics.copy_count, 0u); } +TEST(MoveSemanticsExamplesTest, EmptyBuffersAreSafeToProcess) { + using hpc::move_semantics::Buffer; + + Buffer empty_default; + Buffer empty_sized(0); + + hpc::move_semantics::process_by_ref(empty_default); + hpc::move_semantics::process_by_copy(empty_default); + hpc::move_semantics::process_by_ref(empty_sized); + hpc::move_semantics::process_by_copy(empty_sized); +} + TEST(VectorReserveExamplesTest, ReserveReducesAllocationCount) { using Alloc = hpc::vector_reserve::CountingAllocator; diff --git a/tests/unit/modern_cpp/modern_cpp_header_only_smoke_extra.cpp b/tests/unit/modern_cpp/modern_cpp_header_only_smoke_extra.cpp new file mode 100644 index 0000000..4b5d6b8 --- /dev/null +++ b/tests/unit/modern_cpp/modern_cpp_header_only_smoke_extra.cpp @@ -0,0 +1,11 @@ +#include + +#include "compile_time.hpp" +#include "ranges_utils.hpp" + +int modern_cpp_header_only_helper() { + std::vector input{1, 2, 3}; + std::vector output; + hpc::ranges::transform_algorithm(input, output); + return static_cast(hpc::compile_time::factorial_runtime(3) + hpc::ranges::sum_raw_loop(output)); +} diff --git a/tests/unit/modern_cpp/modern_cpp_header_only_smoke_test.cpp b/tests/unit/modern_cpp/modern_cpp_header_only_smoke_test.cpp new file mode 100644 index 0000000..60fefce --- /dev/null +++ b/tests/unit/modern_cpp/modern_cpp_header_only_smoke_test.cpp @@ -0,0 +1,22 @@ +#include + +#include "compile_time.hpp" +#include "ranges_utils.hpp" + +int modern_cpp_header_only_helper(); + +int main() { + std::vector input{0, 1, 2, 3, 4}; + std::vector output; + hpc::ranges::transform_raw_loop(input, output); + + if (hpc::compile_time::factorial_runtime(5) != 120) { + return 1; + } + + if (output != std::vector({1, 3, 5, 7, 9})) { + return 2; + } + + return modern_cpp_header_only_helper() == 21 ? 0 : 3; +}