Skip to content

[Threading][TSan] Fix TSan errors from lazy init on Linux. #66877

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 29, 2023

Conversation

al45tair
Copy link
Contributor

Move the TSan functionality from Concurrency into Threading. Use it in the Linux ulock implementation so that TSan knows about ulock and will tolerate the newer swift_once implementation that uses it.

This should also slightly improve the performance of Concurrency, since the tsan tests will be inlined rather than it always doing a subroutine call.

rdar://110665213

Move the TSan functionality from Concurrency into Threading. Use it
in the Linux ulock implementation so that TSan knows about ulock
and will tolerate the newer swift_once implementation that uses it.

This should also slightly improve the performance of Concurrency,
since the tsan tests will be inlined rather than it always doing a
subroutine call.

rdar://110665213
@al45tair al45tair added 🍒 release cherry pick Flag: Release branch cherry picks swift 5.9 labels Jun 23, 2023
@al45tair al45tair requested a review from a team as a code owner June 23, 2023 10:30
@al45tair
Copy link
Contributor Author

al45tair commented Jun 23, 2023

Explanation: The Linux implementation of swift::once uses a custom lock implementation so that it fits into a swift_once_t (which is necessary because the compiler reserves space for those when generating code). TSan doesn't see the lock in question as it has no interceptor for it, so we need to tell TSan about the lock otherwise it'll generate errors for accesses made during a lazy Swift initializer. The original PR (#66721) added a use of dlsym(), which then broke the freestanding build, necessitating #66883. Since the changes in #66883 require #66721, and since we would never want one without the other, I've put both into this one cherry-pick.
Risk: Low. First part affects TSan on Linux only; second part fixes a resulting issue with freestanding builds.
Original PR: #66721 + #66883
Reviewed by: @yln @weissi (#66721), @mikeash (#66883)
Resolves: rdar://110665213 (#66721), rdar://111214571, rdar://106555012 (#66883)
Tests: This PR adds a new test, test/Sanitizers/tsan/once.swift, that explicitly tests this case. Before the code changes, the test fails. After the code changes, it passes. Without #66721, the freestanding build will fail. With it, that passes too.

@al45tair
Copy link
Contributor Author

@swift-ci Please test

We shouldn't be using stat() or dlsym() in the freestanding
runtime.

rdar://111214571
rdar://106555012
@al45tair
Copy link
Contributor Author

@swift-ci Please test

@al45tair
Copy link
Contributor Author

preset=stdlib_S_standalone_minimal_macho_x86_64,build,test
@swift-ci please test with toolchain and preset

@al45tair al45tair merged commit 97866bf into swiftlang:release/5.9 Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍒 release cherry pick Flag: Release branch cherry picks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants