SyntaxArena: thread safety part 2 #857
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
|
@swift-ci test |
|
CC @jpsim |
jpsim
reviewed
Sep 28, 2022
|
Swift-CI had a hiccup this morning @swift-ci test |
jpsim
added a commit
to realm/SwiftLint
that referenced
this pull request
Sep 29, 2022
|
@swift-ci test |
jpsim
reviewed
Sep 29, 2022
| @@ -37,7 +37,18 @@ struct PlatformMutex { | |||
| let storage = allocator.allocate(Primitive.self, count: 1).baseAddress! | |||
| storage.initialize(to: Primitive()) | |||
| #if canImport(Darwin) || canImport(Glibc) | |||
| pthread_mutex_init(storage, nil) | |||
| #if canImport(Darwin) | |||
| // HACK: On Darwin, the value of PTHREAD_MUTEX_INITIALIZER installs | |||
|
I'm still seeing a race with the latest commit (2556643): And with TSan enabled: |
|
Here's the fork I'm using btw which has Rintaro's original changes applied on top of If I may be so bold as to make a suggestion: if you're going to bump the macOS deployment target soon, and the end goal is to move macOS to use os_unfair_lock, just live with the races in older macOS versions in the interim. That's how |
Use mutex to guard concurrent mutation to the arena. swiftlang#785 rdar://99812330
Passing `os_unfair_lock` with inout expression is considered "mutation" by thread sanitizer. Instead, keep the lock object with `let UnsafeMutablePointer` form, and pass it.
CodaFi
force-pushed
the
a-critique-of-pure-reason
branch
3 times, most recently
from
609d26b to
5b7bdc3
Compare
|
@jpsim Could you try your local tests one last time with this PR? |
|
@swift-ci test |
Use pthread_mutex_t on Darwin platforms
Did you know that pthread_mutex_init can _fail_? On macOS, the value of PTHREAD_MUTEX_INITIALIZER is not just zeroes - there's optional signature bits in there. POSIX says that you're allowed to check for those signature bits in the API and issue EINVAL if they don't match. By default, pthread_mutex_t() in swift will zero-fill through those signature bits, which results in an invalid mutex variable - even WRT pthread_mutex_init. Once that fails, all subsequent calls to lock and unlock will fail too and leave all of your critical sections completely unguarded. Thank you POSIX On Linuxes this is (often) not the case, and they tend to just use zeroes here, don't check the signature, or both. This allows compilers to allocate lock variables in .bss, which is kinda neat. So, on macOS, we need to install those signature bits, BUT Swift cannot import PTHREAD_MUTEX_INITIALIZER since it's a non-trivial macro that uses brace initialization. Really what we care about is just the signature bits, so we'll install those by hand.
CodaFi
force-pushed
the
a-critique-of-pure-reason
branch
from
5c1aabe to
3dd7635
Compare
|
@swift-ci test |
jpsim
approved these changes
Sep 30, 2022
jpsim
added a commit
to realm/SwiftLint
that referenced
this pull request
Sep 30, 2022
jpsim
added a commit
to realm/SwiftLint
that referenced
this pull request
Sep 30, 2022
jpsim
added a commit
to realm/SwiftLint
that referenced
this pull request
Sep 30, 2022
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.
Restore Rintaro's original patch with a few adjustments: