Thread Sanitizer is too insensitive

Question

Consider this:

Having a struct:

struct Foo {
   let value: Int = 0
   mutating func bar1() {}
   mutating func bar2() {}
}

As we can see, there is no way any instance of Foo can be mutated, since these mutating functions don't actually mutate it.

Now, lets create a UnitTest and rund it with Thread Sanitizer enabled:

    func testDataRace() {
        let expect1 = self.expectation(description: "complete1")
        let expect2 = self.expectation(description: "complete2")
        var foo = Foo()
        DispatchQueue.global().async {
            foo.bar1()
            expect1.fulfill()
        }
        DispatchQueue.global().async {
            foo.bar2()
            expect2.fulfill()
        }
        self.waitForExpectations(timeout: 0.1, handler: nil)
    }

This will actually rise the following "Data Race" detection in Xcode 9.3 (see below). It reports that the starting address where the value `foo` is located has a data race. It does not say specifically which instance variable of the struct `foo`. It seems, it just makes guesses that the struct instance as a whole has been modified.

Furthermore, it seems, Thread Sanitizer is deducing a race condition solely on the `mutating` modifier of the function. If those will be called on different threads without synchronization (before they are called) it "detects" a race.

In all honesty, this is by far too insensitive: consider, we have a "real" struct where mutating functions actually do mutate the state of the instance, but the struct itself has defined a synchronization primitive (for example a pthread_mutex) which actually prevents a data race when any member function will be called. Then, Thread Sanitzer would issue false poitives, as above. This is not useful.

IMHO, Thread Sanitzer should perform a more sophisticated analysis - which perhaps selects candidates of functions based in their mutating modifier, but goes a step further where it actually checks which variable will be modified without synchronization.

So, in order to alleviate the poblem, are there any measurements to configure Thread Sanitizer so that it does not issue these kind of false positives?

WARNING: ThreadSanitizer: Swift access race (pid=28676)

Modifying access of Swift variable at 0x7b080000e070 by thread T3:

#0 closure #2 in FutureBaseTests.testDataRace() FutureBaseTests.swift:38 (RXFutureTests:x86_64+0x36d70)

#1 partial apply for closure #2 in FutureBaseTests.testDataRace() FutureBaseTests.swift (RXFutureTests:x86_64+0x36e5b)

#2 _T0Ieg_IeyB_TR FutureBasicContinuationsTests.swift (RXFutureTests:x86_64+0x1fd0)

#3 __tsan::invoke_and_release_block(void*) <null>:2136560 (libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x650ab)

#4 _dispatch_client_callout <null>:2136560 (libdispatch.dylib:x86_64+0x1e07)

Previous modifying access of Swift variable at 0x7b080000e070 by thread T5:

#0 closure #1 in FutureBaseTests.testDataRace() FutureBaseTests.swift:34 (RXFutureTests:x86_64+0x36b50)

#1 partial apply for closure #1 in FutureBaseTests.testDataRace() FutureBaseTests.swift (RXFutureTests:x86_64+0x36c3b)

#2 _T0Ieg_IeyB_TR FutureBasicContinuationsTests.swift (RXFutureTests:x86_64+0x1fd0)

#3 __tsan::invoke_and_release_block(void*) <null>:2136560 (libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x650ab)

#4 _dispatch_client_callout <null>:2136560 (libdispatch.dylib:x86_64+0x1e07)

Location is heap block of size 24 at 0x7b080000e060 allocated by main thread:

#0 malloc <null>:2136592 (libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x4998a)

#1 swift_slowAlloc <null>:2136592 (libswiftCore.dylib:x86_64+0x2d9928)

#2 FutureBaseTests.testDataRace() FutureBaseTests.swift (RXFutureTests:x86_64+0x3638d)

#3 @objc FutureBaseTests.testDataRace() FutureBaseTests.swift (RXFutureTests:x86_64+0x370b0)

#4 __invoking___ <null>:2136592 (CoreFoundation:x86_64h+0x82c3b)

Thread T3 (tid=1828145, running) is a GCD worker thread

Thread T5 (tid=1828158, running) is a GCD worker thread

SUMMARY: ThreadSanitizer: Swift access race FutureBaseTests.swift:38 in closure #2 in FutureBaseTests.testDataRace()

==================

Concurrency

2.7k

Posted by

couchdeveloper

Reply

Add a Comment

Answer 1

but the struct itself has defined a synchronization primitive (for example a pthread_mutex) which actually prevents a data race when any member function will be called.

Is it really a pthread mutex? If so, that’s problematic because you can’t safely put a pthread mutex into a struct (because structs can be copied at will). And if you’re using some sort of reference type for your mutex (think

NSLock

) then that’s problematic too because, on copy, each of instance will end up sharing the same mutex.

Personally I find this design suspect and I’m not surprised you’re having problems with the Thread Sanitizer. Remember that the sanitiser’s goal is to enforce the thread rules, not to check a specific implementation. Thus looking inside the implementation of a

mutating

method to find whether it actually mutates is outside of its scope.

Fortunately (for you :-) I don’t get to make this call. If you want to make your case to the folks who actually work on the sanitiser, you should file a bug describing your requirements. Please post your bug number, just for the record.

Share and Enjoy
—
Quinn “The Eskimo!”
Apple Developer Relations, Developer Technical Support, Core OS/Hardware

let myEmail = "eskimo" + "1" + "@apple.com"

Posted by

eskimo

Add a Comment

Answer 2

Personally I find this design suspect …

On further consideration I think it’d be useful for me to explain some of my conceptual objections. First up, consider that this:

struct MyStruct {
    func myFunc() { … }
}

is conceptually a shortcut for:

struct MyStruct {
    static func myFunc(self: MyStruct) { … }
}

Similarly, this:

mutating func myMutatingFunc() { … }

is a shortcut for this:

static func myMutatingFunc(self: inout MyStruct) { … }

Now keep in mind that, again talking conceptually,

inout

is not ‘pass by reference’, as it might be in a C-based language, but rather ‘copy in, copy out’. So a call site like this:

var s: MyStruct = …
s.myMutatingFunc()

is actually this:

var s: MyStruct = …
s = MyStruct.myMutatingFunc(self: s)

And that’s why I’m suspicious of your design. If you expand your code as I’ve shown above, putting a mutex within

MyStruct

is really worrying because you have multiple instances of

MyStruct

floating around.

Honestly, I think you might be better off using a class here.

Share and Enjoy
—
Quinn “The Eskimo!”
Apple Developer Relations, Developer Technical Support, Core OS/Hardware

let myEmail = "eskimo" + "1" + "@apple.com"

Posted by

eskimo

Add a Comment

Answer 3

Thank you for the reply 🙂

Your consideration with the pthread_mutex is correct. It shall not be copied. In fact, it seems difficult to use a pthread_mutex in Swift at all (no matter how), since we don't know the underlying generated code. A pthread_mutex value mihgt be copied even when we just try to initialize it (e.g. creates a temp copy, initialises this, and then "moves assigns" it to the actual target value. Here, the mention of the pthread_mutex was only an example. I did not provide the code.

This is not the point however. My question was about the Thread Sanitizer. Actually, it seems, TS does not consider any synchronization promitives which are (correctly) used, when the functions have the "mutating" modifier. This should be improved, IMHO. You can run this example with a DisptachQueue for synchronization which is a reference type, and Thread Sanitizer will as well issue the same error.

Regarding using pthread_mutex

In fact I use a struct where the pthread_mutex opaque value is layout. However, this struct will be exclusively used within a class type. So, it will be only shared where it is meant to be shared.

So, the idea is, that the mutex value may only be moved (during init) when it is _not_ yet initialised. There is a separate mutating function which actually initialises the mutex value when the address of the class value is known. However, this is still not safe, as you pointed out, due to the "copy-in copy-out call" Swift is implementing for inout parameters.

So after carefully looking at my code, it seems there is exactly this issue, and I will think about a viable solution. Note: the optimzed code will actualy use a _reference_, and there should be no issue. Of course, that's not a reliable design, though.

It turns out, making thread-safe code in Swift is unnecessary difficult. 😟

Nonetheless, I appreciate your hints and comment (as always) 🙂

Posted by

couchdeveloper

Add a Comment

Answer 4

It turns out, making thread-safe code in Swift is unnecessary difficult.

It’s hard to argue with that [1]. There are plans to give Swift a proper concurrency model, but those are some way out. Until that happens I try to err on the side of caution:

I only ever share immutable data between threads
I avoid atomics.
I use high-level primitives for serialising access to mutable data (typically a dispatch queue)

Then again, I rarely work on performance sensitive code [2], so it’s easy for me to live by these rules.

If you’re interested in the future of Swift concurrency I can recommend:

The first two are obviously relevant. The last one seems like a weird choice, but getting ownership right is the first step is being able to come up with a memory model (à la C11), which includes atomics.

Share and Enjoy
—
Quinn “The Eskimo!”
Apple Developer Relations, Developer Technical Support, Core OS/Hardware

let myEmail = "eskimo" + "1" + "@apple.com"

[1] Although you could argue that the same applies for most popular programming languages.

[2] Unless you count network performance, but in that case the CPU is so much faster than the network that the architecture is much more important than, say, whether you use a lock free data structure.

Posted by

eskimo

Add a Comment

Answer 5

>> Then, Thread Sanitzer would issue false poitives, as above. This is not useful.

It seems to me that your code falls foul of SE-0176 (github.com/apple/swift-evolution/blob/master/proposals/0176-enforce-exclusive-access-to-memory.md), because your calls to foo1 and foo2 are "non-instantaneous" accesses. The overlapping access rules in SE-0176 have nothing to do whether there are any race conditions over actual memory reads and writes, it's a higher-level formal constraint on the way you can write your code.

IOW, the compiler no longer allows you to take over responsibility for thread safety around mutable accesses, even if you've effectively done so in this particular case.

That is, this was not a false positive in Thread Sanitizer, but a violation of the language's ownership policy that's now enforced.

>> It turns out, making thread-safe code in Swift is unnecessary difficult.

Well, that's still true.

Posted by

QuinceyMorris

Add a Comment

Thread Sanitizer is too insensitive

Replies