Make sample recycling synchronization visible to ThreadSanitizer - #307
Merged
Merged
Conversation
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.
ThreadSanitizer reports a timestamp race when a sample is recycled after another thread releases its reference. The existing release decrement followed by an acquire fence is valid C++ synchronization, but TSan does not model the fence. Replace that pair with an acquire-release decrement so the final owner acquires earlier owners’ releases before publishing the sample for reuse.
Fixes #305. This addresses a sanitizer false positive; the investigation did not demonstrate premature sample reuse or corrupted timestamps. LLVM tracks the fence limitation in llvm/llvm-project#52942.
Changes
memory_order_acq_relinintrusive_ptr_release()instead of a release decrement plus conditional acquire fence. No suppressions or sanitizer-specific annotations are added. This acquires on every decrement, rather than only on final release; performance has not been benchmarked.factory::new_sample()concurrently despite its single-allocator contract. Serialize only allocation; buffer pushes remain concurrent. This is a separate test-only commit.Validation
Native macOS arm64, Xcode 27 / Apple Clang 21, Debug, with TSan instrumentation and no suppressions:
All functional assertions passed in both comparisons. Before-fix warning runs exited unsuccessfully under TSan; after-fix runs passed without warnings.
[outlet],[open],[reopen],[sync]suite: 1,741 assertions across 20 cases, passed normally and under TSan.[send_buffer],[sample]tests: passed normally and in 10/10 additional TSan runs.Address already in usefailure. A retry exposed the existing concurrent-allocation misuse corrected here; the final full internal run passed.git diff --checkpassed.Windows/Linux validation remains for CI.