Micgrier/updatepil#172
Merged
Merged
Conversation
added 20 commits
June 12, 2026 15:10
- tracing: store monitor messages in typed unique_ptr storage, destroy them in reverse order with a hard tripwire asserting all envelopes returned - tracing: add monitor teardown harness tests (leak baseline + death test) - threadpool: add dedicated state condition variable so work_item wait/wait_for/ wait_until block until the item state is genuinely terminal (done/canceled), closing a race where the packaged_task future went ready before the state set - assorted fixes in formatters, debugging, pil, sstring, envelope
…drain - Add public work_queue::close() / do_close() so owners can drain a queue deterministically; the destructor performs the same drain if close() was not called. close() is idempotent and guarantees no further callbacks run after it returns. - Add perform_platform_teardown() hook to work_queue_base; do_close() routes to it. Windows drains via tp_work.wait_for_callbacks(true) (cancel pending + wait in-flight); Linux is a no-op (nothing initialized). - Windows: replace the weak_ptr callback back-pointer with a raw pointer and drop enable_shared_from_this. Because callbacks hold no ownership, the last shared_ptr release can never occur on a pool thread, so the destructor's synchronous drain cannot self-deadlock. Removes the unchecked weak_ptr::lock() deref that could null-deref a destroyed queue. - Tests: add close()/teardown coverage (idle, after-drain, in-flight drain, idempotent, destroy-without-close). Reduce QueueNBig to 100k for a fast normal pass and move the 1.3M-item soak to opt-in DISABLED_QueueNBigStress.
…ed helper - Fix safe_math_helper<unsigned,unsigned,signed>::try_negate, which rejected the two most-negative representable results (e.g. subtract(0u,128u,int8) threw instead of yielding -128, and 127 -> -127). Delegate to the already-correct unsigned->signed unary negate helper, which admits the full negative range including the target's most-negative value. Also covers unsigned-signed->signed subtraction, which shares the path. Adds regression test MostNegativeBoundary. - Add the previously-missing (signed [op] signed) -> unsigned specialization (add/subtract/multiply/divide). Computes in Z then requires a non-negative, in-range result; negative results overflow. Magnitudes are taken via a helper that is correct for intmax_t::min. New test file signed_signed_to_unsigned.cpp.
…e-case + constexpr tests - Fix the (signed / unsigned) -> signed and (unsigned / signed) -> signed divide helpers, which rejected a quotient whose magnitude equals the result type's most-negative value (e.g. divide(INT64_MIN, 1u, int64) and divide(2^63, -1, int64) threw instead of yielding INT64_MIN). Both negative-result branches now delegate to unary_safe_math_helper<uintmax_t, ResultT>::negate, mirroring the earlier try_negate fix; the multiply helpers already handled this via explicit max-negative checks. - Add test_edge_cases.cpp (mixed-sign most-negative add/subtract, 64-bit INT_MIN multiply branches, multiply narrowing, 64-bit negate boundaries, and DivideMostNegativeResult regression tests for both fixed divide paths). - Add test_constexpr.cpp (static_assert compile-time evaluation across all sign combinations) and register both files in test/CMakeLists.txt.
The CAS failure path re-incremented a raw T* loaded from m_ptr with no held reference (load-then-addref use-after-free race), the fundamental hazard of a lock-free atomic shared pointer that a plain pointer CAS cannot fix. The method was used nowhere outside the library's own tests; the only real consumers (const_string, sstring) use arefc_ptr purely as an Arc<T>-style shared owner. Removing it makes arefc_ptr a faithful Arc<T>. Also drops the four AreFcPtr_CAS tests.
…assign #1: if the constructing callback throws, the object was never constructed, but the aggregate's unique_ptr deleter defaulted to do_destroy=true and ran std::destroy_at on the unconstructed storage (UB for non-trivial dtors; const_string uses this path). Wrap the callback in try/catch and deallocate with do_destroy=false on failure before rethrowing. #3: copy-assignment took a non-const arefc_ptr&, so 'p = const_src;' failed to compile. addref() is const, so widen the parameter to arefc_ptr const&. Add regression tests: AreFcPtr_MakeEx.ThrowingConstructorDoesNotDestroyUnconstructed and AreFcPtr_CopyAssign.FromConstSource.
… exception safety aggregate::allocate() returns a unique_ptr owning raw storage whose object is not yet constructed; construction happens later via the caller's callback. Move the not-destroy decision into the deleter (deallocate with do_destroy=false) so a throwing callback unwinds the unique_ptr and frees the storage without running a destructor on unconstructed memory. Removes the explicit try/catch from mmake_arefc_ex.
Remove the ill-formed self-assignment guard in operator=(arefc_ptr<U> const&); comparing unrelated arefc_ptr<T>* and arefc_ptr<U>* pointers is invalid and a distinct specialization can never alias *this. Make the raw-pointer reset(T*) private (footgun for callers) while keeping the public no-arg reset(). Add a converting-assignment regression test.
Add the missing direct <atomic> include (m_c_str/std::memory_order were only available transitively). Constrain the templated operator+ and operator== with a requires-expression so unrelated RHS types are SFINAE-rejected instead of producing hard errors from the body. Pass basic_sstring by const& in the compare() overloads to avoid a refcount bump per call. Drop the redundant to_basic_string_view_t round-trip in operator<=>(view_type const&).
Replace the fragile operator<=>(this->view(), r.view()) self-call (which only resolved via ADL to std::operator<=> after the member candidate failed on arity) with a direct view() <=> view(). Add static_asserts that each basic_const_string specialization is exactly sizeof(std::size_t), since get_buffer_ptr() derives the trailing buffer address from (&m_size + 1) and that is only valid when m_size is the sole member.
Replace raw size_t arithmetic in c_str(), substr(), right(), and make_c_str() with m::math::add/subtract (result type std::size_t), which throws std::overflow_error instead of silently wrapping on overflow.
…default) Per the safe-math-by-default policy, convert the remaining raw size_t arithmetic in split_at/split_at_first_of (split_point + 1, split_point + view_to_find.size()), last() (size - 1), and the M_DEBUG self_validate() subtractions to m::math::add/subtract. Pointer arithmetic and atomic RMW counters remain raw (out of scope for the integer-checked math library).
…ault) Fixes a real overflow-before-bounds-check defect in append_range and insert(pos, first, last): 'size() + n > capacity()' could wrap when n is near SIZE_MAX, passing the guard and overrunning the buffer. Now uses m::math::add (result type std::size_t), which throws std::overflow_error instead of wrapping. Also converts the size +/- 1 growth/shrink sites (unchecked_emplace_back, back(), pop_back, erase, resize) to m::math::add/subtract. Pointer arithmetic (begin()+size(), end()-1, iterator differences) and compile-time-N-bounded expressions remain raw. Adds <m/math/math.h>.
The previous operator<=> returned int, ordered by size() first, and aggregated all_equal/all_less flags instead of comparing at the first differing element. That produced wrong orderings for equal-size vectors that differ (and prefix relationships). Replace it with std::lexicographical_compare_three_way using an exposition-only synth_three_way helper, so element types with operator<=> use it directly and types with only operator< / operator== get a synthesized weak_ordering. The operator is constrained (auto return + requires) so it is not a candidate for non-comparable element types, preserving inplace_vector instantiation for such types. Adds ordering tests (equal, less/greater by element, prefix-is-less, empty, synth-from-less-only).
Adds AppendRangeIntegerOverflowThrows, which drives the new m::math::add integer-overflow guard via a sized range that reports SIZE_MAX (so size()+n wraps) and asserts std::overflow_error plus that the vector is unchanged. Adds AppendRangeOverCapacityThrows to cover the ordinary capacity guard (std::bad_alloc). The insert(pos,first,last) overflow path is not exercised because std::distance is bounded by PTRDIFF_MAX and cannot wrap SIZE_MAX when added to a capacity-bounded size().
Use m::math::add(m_index, 1) so an integer wrap on m_index+1 cannot shrink the string and turn the subsequent operator[] into an out-of-bounds wild write. operator* stays noexcept (overflow -> terminate is acceptable fail-stop).
…ally_ordered stringish: use std::decay_t instead of remove_cvref_t so string-literal array operands decay to their pointer form and satisfy the stringish/any_stringish/stringish_char_type predicates at every call site (cp_acp, utf, sstring). decay_t is equivalent to remove_cvref_t for all previously accepted types, so this only adds the array forms mapped to already-accepted pointers; no new false positives. sstring: constrain operator+ and operator== with the pure stringish type predicate instead of a requires-expression over to_basic_string_view_t(). The latter returns auto and forced its body to instantiate during constraint checking, turning a missing higher-layer view_converter specialization into a hard error and breaking the self-contained totally_ordered static_asserts.
encode: the validity guards used 0xdc00 as the surrogate lower bound, so lone high surrogates (U+D800..U+DBFF) were accepted and encoded. Use 0xd800 so the whole surrogate range U+D800..U+DFFF is rejected (18 sites across encode_utf8/16/32 and the compute_encoded_* size helpers). decode: the iterator-based decode_utf8 (3-byte form) and decode_utf32 did not reject surrogate code points, unlike the byte-based decoders. Add surrogate rejection to both throwing and error_code variants so all decoders agree that surrogates are invalid Unicode scalar values. The >U+10FFFF cap is intentionally left unchanged pending a separate decision.
basic_ostream_sink::close() sets m_done then calls message_queue::wake_waiters() (notify_all) before joining the sink thread. If the worker had checked the stop flags and not yet entered wait(), the notify was lost: the worker then parked in m_cv.wait() with no predicate and never woke, so close()'s join() blocked forever. This surfaced intermittently as a multi-thousand-second hang in DirectRegistryMonitoring.MonitorKey during sink teardown. Add a sticky m_wake flag set under m_mutex by wake_waiters() and consumed by wait(); wait() now uses a predicate (!queue.empty() || m_wake). A wake requested before the worker reaches wait() is observed instead of lost.
…ption hierarchy encode_* and the compute_encoded_* size helpers threw a bare std::runtime_error on out-of-range or surrogate code points, and decode_utf16/decode_utf32 threw bare std::runtime_error for truncation and invalid encodings, while decode_utf8 already used the dedicated hierarchy in m/utf/exceptions.h. Route these through utf_invalid_encoding_error (invalid scalar values) and utf_sequence_truncated_error (utf-16 truncation) so callers can catch a single utf_encoding_error base across every encoder and decoder. Both derive from std::runtime_error, so existing handlers keep working. The two logic_error guards in the encode_char if-constexpr dispatch are left unchanged: they are internal-invariant guards, not encoding-data errors.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR spans several core utility libraries (ref-counted pointer, math, UTF, tracing/threadpool, string utilities, and Windows formatters) with the goal of tightening correctness around lifetime/teardown, integer overflow handling, and Unicode validity, while adding regression tests for newly enforced edge cases.
Changes:
- Hardens ownership/lifetime behavior:
arefc_ptrconstruction failure cleanup, tracing monitor teardown invariants, and threadpool work-queue teardown/close semantics. - Improves correctness and safety in utilities: UTF surrogate rejection, overflow-safe sizing via
m::math::add/subtract, and broader “stringish” type recognition (incl. string literals). - Adds/updates tests to lock in edge-case behavior (math boundaries, tracing teardown, inplace_vector comparisons, Windows formatter output).
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Windows/libraries/formatters/test/test_FILETIME.cpp | Updates expected formatted FILETIME output after formatter fix. |
| src/Windows/libraries/formatters/test/test_FILE_NOTIFY_EXTENDED_INFORMATION.cpp | Updates expected formatted timestamps after FILETIME formatting fix. |
| src/Windows/libraries/formatters/include/m/formatters/FILETIME.h | Fixes FILETIME formatter argument ordering (removes duplicated hour/minute fields). |
| src/libraries/utf/src/decode_utf8.cpp | Rejects surrogate code points when decoding UTF-8. |
| src/libraries/utf/include/m/utf/encode.h | Tightens surrogate-range validation and throws UTF-specific exception type. |
| src/libraries/utf/include/m/utf/decode.h | Rejects surrogate code points in UTF-8/UTF-32 paths; refines exception types. |
| src/libraries/tracing/test/test_monitor_teardown.cpp | Adds teardown/lifecycle tests, including invariant death-test for retained messages. |
| src/libraries/tracing/test/CMakeLists.txt | Adds new tracing teardown test to the test target. |
| src/libraries/tracing/src/monitor_class_impl.h | Moves raw message storage definitions into the class (typed storage + count). |
| src/libraries/tracing/src/monitor_class_impl.cpp | Uses typed unique_ptr for raw storage; adds teardown invariant and explicit message destruction. |
| src/libraries/tracing/src/message_queue.cpp | Fixes lost-wakeup race by adding predicate wait + sticky wake flag behavior. |
| src/libraries/tracing/src/envelope.cpp | Fixes move-assignment leak by returning held message to sender before taking ownership. |
| src/libraries/tracing/include/m/tracing/message_queue.h | Adds sticky wake flag to support reliable teardown wakeups. |
| src/libraries/threadpool/test/test_work_queue.cpp | Adds queue size/perf-ish and close/drain teardown tests (plus a large non-disabled test). |
| src/libraries/threadpool/src/work_queue_base.h | Introduces platform teardown hook and do_close() override. |
| src/libraries/threadpool/src/work_queue_base.cpp | Implements do_close() delegating to platform teardown. |
| src/libraries/threadpool/src/work_item.h | Adds condition variable for terminal-state waiting. |
| src/libraries/threadpool/src/work_item.cpp | Changes wait APIs to wait on explicit terminal state rather than future readiness. |
| src/libraries/threadpool/src/Platforms/windows/work_queue.h | Removes shared_from_this usage; adds platform teardown override; callback context uses raw back-pointer. |
| src/libraries/threadpool/src/Platforms/windows/work_queue.cpp | Adds destructor drain; implements platform teardown via wait_for_callbacks(true); switches callback back-pointer to raw pointer. |
| src/libraries/threadpool/src/Platforms/linux/work_queue.h | Adds platform teardown override. |
| src/libraries/threadpool/src/Platforms/linux/work_queue.cpp | Implements no-op platform teardown (linux queue unimplemented). |
| src/libraries/threadpool/include/m/threadpool/work_queue.h | Adds public close() API and new virtual do_close(). |
| src/libraries/sstring/include/m/sstring/sstring.h | Makes compare APIs take const&; uses overflow-safe math helpers; improves stringish constraints. |
| src/libraries/pil/src/registry_key.cpp | Fixes key assignment semantics; implements REG_MULTI_SZ parsing; fixes value-type propagation in get_value loop. |
| src/libraries/pil/src/key_path.cpp | Fixes parent_path() to use last delimiter instead of first. |
| src/libraries/pil/src/buffered/registry_key_value_operations.cpp | Ensures buffered value node captures moved value bytes. |
| src/libraries/math/test/test_subtraction.cpp | Adds regression test for most-negative boundary behavior. |
| src/libraries/math/test/test_edge_cases.cpp | Adds broader math edge-case coverage (64-bit INT_MIN branches, narrowing, negate, etc.). |
| src/libraries/math/test/test_constexpr.cpp | Adds constexpr-usage validation for math APIs. |
| src/libraries/math/test/signed_signed_to_unsigned.cpp | Adds tests for new signed-signed-to-unsigned specialization behavior. |
| src/libraries/math/test/CMakeLists.txt | Adds new math test files to build. |
| src/libraries/math/include/m/math/math.h | Fixes most-negative negation handling; refactors divide negation; adds signed-signed-to-unsigned safe_math specialization. |
| src/libraries/inplace_vector/test/test_inplace_vector.cpp | Adds coverage for three-way comparison and overflow-guarded append_range behavior. |
| src/libraries/inplace_vector/include/m/inplace_vector/inplace_vector.h | Uses overflow-safe math ops; implements synth-three-way comparator and lexicographical three-way compare. |
| src/libraries/debugging/include/m/debugging/dbg_format.h | Fixes long-line suffix copying to include terminator safely without overwriting last character. |
| src/libraries/const_string/include/m/const_string/const_string.h | Fixes operator<=> implementation; adds layout invariants via static_assert on size. |
| src/libraries/arefc_ptr/test/test_arefc_ptr.cpp | Adds tests for const copy-assign, converting assign, and throwing-constructor behavior; removes CAS tests. |
| src/libraries/arefc_ptr/include/m/arefc_ptr/arefc_ptr.h | Fixes allocation cleanup semantics for unconstructed storage; refines copy assignment/reset API; removes CAS method. |
| src/include/m/utility/stringish.h | Uses std::decay_t to recognize string literals/arrays as stringish. |
| src/include/m/utility/string_inserter.h | Uses overflow-safe m::math::add when resizing during insertion. |
- work_queue.h: tighten close() docs to state guarantee is conditional (callers must not enqueue during/after close; no enforced closed state). - test_work_queue.cpp: reduce QueueNBig from 100k to 10k items so the default test pass stays fast/quiet; heavy soak remains in DISABLED_QueueNBigStress. - test_monitor_teardown.cpp: add <mutex> include and the missing m_mutex member to retaining_sink so the file compiles.
Copy the REG_MULTI_SZ bytes into an aligned std::u16string before interpreting them as char16_t instead of reinterpret_cast-ing the 1-byte-aligned std::vector<std::byte> buffer in place (potential UB on alignment-sensitive platforms). Reject odd byte counts rather than silently truncating the trailing byte. Add <cstring> for std::memcpy.
…tomic init - work_item: add cancel_if_queued() to transition a queued item to the canceled terminal state and notify waiters. - windows work_queue: perform_platform_teardown() now drains m_ready_queue under m_mutex and cancels never-started items so work_item::wait*() cannot deadlock on work that will never run; notifies queue waiters. - work_queue.h: document that close() moves not-yet-started items to canceled and unblocks their waiters. - test_work_queue.cpp: value-initialize the atomic flag arrays (new[]()) so reads are well-defined.
- dbg_format.h: copy the ellipsis/newline/terminator from the long_line_chars array (which legitimately includes the null) instead of reading one past the end of long_line_suffix (a view that excludes the terminator), which was UB. - envelope.cpp: move-assignment and move-ctor now null out other.m_message_source as well as m_imessage so a moved-from envelope is left in a consistent empty state and cannot return a dangling message_source().
…llback The placement-new loop that constructs the m_raw_messages array was not exception-safe: if a message constructor threw, previously-constructed messages would not be destroyed and the storage would be freed with live objects in it. Guard the loop with a unique_ptr-based RAII rollback (same deleter-as-cleanup idiom as mmake_arefc_ex) that destroys the constructed prefix in reverse order on unwinding; dismiss it with release() on success. The subsequent enqueue() loop is noexcept and needs no rollback.
wait()/wake_waiters() used a single shared sticky bool: with multiple concurrent waiters the first to wake cleared the flag before the others re-checked, so a notify_all() broadcast was consumed by only one waiter and the rest could block forever. Replace the bool with a monotonic wake-generation counter. wake_waiters() advances the generation under the lock and broadcasts; wait(last_wake_generation) blocks only while the generation is unchanged. Each waiter samples wake_generation() before it drains/checks and passes that baseline in, so every waiter observes the broadcast independently and a wake issued before a thread reaches wait() is still seen. ostream_sink's sink_thread now samples the generation at the top of each loop iteration.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request makes several improvements and bug fixes to the
arefc_ptrimplementation and related utilities, focusing on safer memory management, improved type handling, and more robust string utilities. The most important changes are grouped below.arefc_ptr Memory Management and Assignment Fixes
arefc_ptrto ensure that only deallocation (not destruction) occurs when cleaning up storage for objects that were never constructed, preventing undefined behavior if construction fails. [1] [2]reset()safer by splitting it into a public no-argument version (for general use) and a private raw-pointer version (for internal use only). [1] [2]String Utility Improvements
stringishandany_stringishconcepts and related type traits to usestd::decay_tinstead ofremove_cvref_t, allowing string literals (arrays) to be correctly recognized as string-like types.string_inserterto usem::math::addfor resizing, which fail-stops on overflow instead of wrapping, preventing potential buffer overflows and wild writes. [1] [2]Testing and Bug Fixes
arefc_ptrspecializations, ensuring correct ownership semantics and fixing previous compilation issues. [1] [2]mmake_arefc_exdoes not result in the destructor being called on unconstructed storage. [1] [2]FILETIME Formatting and Tests
FILETIMEformatter where hour and minute fields were duplicated, and updated related test expectations to match the corrected output. [1] [2] [3] [4] [5]