SyntaxArena: thread safety #807
Merged
Conversation
This file contains 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 Please test |
Use mutex to guard concurrent mutation to the arena. swiftlang#785 rdar://99812330
|
@swift-ci Please test |
|
Thank you, this certainly helps, although TSan now reports an access race in the guard code: Full report: https://buildkite.com/swiftlint/swiftlint/builds/3411#018354b5-95eb-4c14-ab21-f7d59113f241 |
ahoppen
reviewed
Sep 19, 2022
| /// Copies a source buffer in to the memory this arena manages, and returns | ||
| /// the interned buffer. | ||
| /// | ||
| /// The interned buffer is guaranteed to be null-terminated. | ||
| /// `contains(address _:)` is faster if the address is inside the memory | ||
| /// range this function returned. | ||
| public func internSourceBuffer(_ buffer: UnsafeBufferPointer<UInt8>) -> UnsafeBufferPointer<UInt8> { | ||
| let allocated = lock.withGuard { |
| @@ -114,7 +189,9 @@ public class SyntaxArena { | |||
| /// Copies a `RawSyntaxData` to the memory this arena manages, and retuns the | |||
| /// pointer to the destination. | |||
| func intern(_ value: RawSyntaxData) -> UnsafePointer<RawSyntaxData> { | |||
| let allocated = allocator.allocate(RawSyntaxData.self, count: 1).baseAddress! | |||
| let allocated = lock.withGuard { | |||
There was a problem hiding this comment.
Same here, should this be self.withGuard?
CodaFi
reviewed
Sep 19, 2022
CodaFi
requested changes
Sep 19, 2022
There was a problem hiding this comment.
These ScopeGuards are not correct. We cannot use the materialization operator here despite the fact that we're indexing through a heap allocation for the member access - it's still a logical data race even if it's not always a physical data race. These locks need to be allocated out-of-line and the resulting address passed.
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
approved these changes
Sep 20, 2022
|
@jpsim I confirmed the failure locally. And the modified implementation resolves it. Could you check the new implementation? |
|
@swift-ci Please test |
jpsim
approved these changes
Sep 21, 2022
jpsim
added a commit
to realm/SwiftLint
that referenced
this pull request
Sep 21, 2022
jpsim
added a commit
to realm/SwiftLint
that referenced
this pull request
Sep 21, 2022
jpsim
added a commit
to realm/SwiftLint
that referenced
this pull request
Sep 21, 2022
jpsim
added a commit
to realm/SwiftLint
that referenced
this pull request
Sep 25, 2022
jpsim
added a commit
to realm/SwiftLint
that referenced
this pull request
Sep 27, 2022
jpsim
added a commit
to realm/SwiftLint
that referenced
this pull request
Sep 28, 2022
jpsim
added a commit
to realm/SwiftLint
that referenced
this pull request
Sep 28, 2022
jpsim
added a commit
to realm/SwiftLint
that referenced
this pull request
Sep 29, 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.
Use mutex to guard concurrent mutation to the arena.
But make a "single thread" mode so that we don't regress performance too much when we know the arena is not used by other threads.
(non
Darwin/Glibcenvironment e.g. Windows implementation is currently a dummy)#785
rdar://99812330