[Threading][TSan] Fix TSan errors from lazy init on Linux. #66721
Conversation
|
@swift-ci Please test |
|
Note that your new tsan test is not run on the linux CI, most likely because of #53973: |
I'm aware. I've been manually running locally it for that reason; it took me a while to work out that I needed |
|
@swift-ci Please smoke test |
| if (ulock_fastpath(__atomic_compare_exchange_n(lock, &zero, tid, | ||
| true, __ATOMIC_ACQUIRE, | ||
| __ATOMIC_RELAXED))) | ||
| break; |
|
Looks like I've somehow broken things elsewhere. Looking at that now. |
|
OK, so this is broken on macOS because of the behaviour of |
|
@swift-ci Please smoke test |
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. rdar://110665213
* Use the longer name ThreadSanitizer rather than TSan for the new files. * Don't implement `tsan::consume` at all for now. * Do the `tsan::release` for `ulock_unlock()` at the head of the function, not at the tail. * Add a comment to test/Sanitizers/tsan/once.swift to explain the test a little more clearly. rdar://110665213
On Darwin, `RTLD_NEXT` doesn't do what we need it to here, with the result that if `libswiftCore`'s TSan initializer gets found first, then `libswift_Concurrency` won't have its initializer called at all, in spite of us using `RTLD_NEXT` to find the next definition. Fix this by centralising the initializer in `libswiftCore` instead. rdar://110665213
al45tair
force-pushed
the
eng/PR-110665213
branch
from
3f18c7f to
8ed8a28
Compare
|
(Rebased to fix conflict) |
|
@swift-ci Please smoke test |
|
I think this version will likely fail the unit tests on Linux, because various things besides the runtime libraries use the Threading library, and not all of them link against the runtime. It looks like I'm going to need to rearrange things a little more. |
We need ThreadSanitizer.cpp in libswiftCore for the runtime case, but we also need it in libswiftThreading for non-runtime cases. rdar://1106655213
We need to pick up the `_swift_tsan_xxx` symbols from libswiftCore in most cases, but sometimes we're statically linked and in that case we want to use a local copy. rdar://1106655213
|
@swift-ci Please smoke test |
On builds where TSan isn't supported, don't include any code in ThreadSanitizer.cpp. rdar://110665213
|
@swift-ci Please smoke test |
There was a problem hiding this comment.
Thanks so much Alastair!
LGTM, with one nit.
Please document the annotation APIs in terms of TSan "model synchronization operation" instead of C memory model.
Something like we had before:
/// release() establishes a happens-before relation with a preceding acquire()
/// on the same address.
Something that describes that we are modeling / "teaching" TSan about asynchronization operation (not necessarily lock-based):
T1 | T2
access from y
tsan_release(x)
lock given away
--> sync point --> <<< this is what we try to model
lock taken
tsan_aquire(x)
access to y
Updated the comments for tsan::acquire and tsan::release to better reflect what TSan is actually doing. rdar://110665213
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke test Windows platform |
Move the TSan functionality from Concurrency into Threading. Use it in the Linux
ulockimplementation so that TSan knows aboutulockand will tolerate the newerswift_onceimplementation 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