Page MenuHomePhabricator

[TSan] Enable detection of lock-order-inversions for Objective-C @synchronized
ClosedPublic

Authored by yln on Dec 20 2018, 3:11 PM.

Details

Summary

@synchronized semantics can be synthesized by using existing mutex_[un]lock operations.

@synchronized(obj) {
  // ...
}

=> 
{
  mutex_lock(obj);
  // ...
  mutex_unlock(obj);
}

Let me know whether you think this a good idea.

Diff Detail

Event Timeline

yln created this revision.Dec 20 2018, 3:11 PM
Herald added subscribers: Restricted Project, llvm-commits. · View Herald TranscriptDec 20 2018, 3:11 PM

LGTM with a nit. @dvyukov are we using the Mutex* calls correctly here?

compiler-rt/lib/tsan/rtl/tsan_interceptors_mac.cc
322 ↗(On Diff #179167)

Can we just #include <objc-sync.h> instead?

yln updated this revision to Diff 179192.Dec 20 2018, 4:00 PM

Added test for recursive @synchronized blocks, that is, nested @synchronized on the same object. This tests proves lock recursiveness.

Directly #include <objc/objc-sync.h>. Had to change parameter type from void *obj to id obj in the interceptors to avoid error: conflicting types for 'objc_sync_enter'.

yln marked an inline comment as done.Dec 20 2018, 4:16 PM
dvyukov added inline comments.Dec 20 2018, 10:14 PM
compiler-rt/lib/tsan/rtl/tsan_interceptors_mac.cc
328 ↗(On Diff #179192)

If we use these annotations, I think we need to fix SyncAddressForTaggedPointer in some way. Acquire/Release annotations can be arbitrary mixed on the same address; but Mutex annotations for different mutexes can't be mixed, e.g. 2 locks from different threads without unlock.

328 ↗(On Diff #179192)

We don't check it today, but it can make sense to pass MutexFlagWriteReentrant because later we can start checking it.

330 ↗(On Diff #179192)

We do not use assert in this code base. Use DCHECK_EQ, or better CHECK_EQ since this is not super performance-critical part.

yln marked 3 inline comments as done.Dec 21 2018, 11:23 AM
yln added inline comments.
compiler-rt/lib/tsan/rtl/tsan_interceptors_mac.cc
328 ↗(On Diff #179192)

Passing it on calls that could create the backing SyncVar is sufficient, correct?
That is, MutexPreLock, or MutexPostLock (when detect_deadlocks=0).

328 ↗(On Diff #179192)

Very good point. I will add a test for synchronizing on a tagged pointer. Also let me investigate what this should do. When synchronizing on tagged pointers we are already getting into murky waters.

To make this work with the current scheme, we would need to map tagged pointers to dummy, but valid addresses, right?

yln marked an inline comment as done.Dec 21 2018, 11:24 AM
yln updated this revision to Diff 179362.Dec 21 2018, 2:43 PM

Addressed most comments and added failing test for @synchronize with tagged pointers.

yln marked an inline comment as done.Dec 21 2018, 2:50 PM

I would like to land this and fix @synchronized with tagged pointers in a follow-up since more discussion is required what we actually want it to do.

yln marked an inline comment as done.Dec 21 2018, 2:50 PM
yln updated this revision to Diff 179380.Dec 21 2018, 4:05 PM

Fix bug in test.

dvyukov accepted this revision.Dec 22 2018, 12:00 AM

I would like to land this and fix @synchronized with tagged pointers in a follow-up since more discussion is required what we actually want it to do.

It's fine with me code-wise, but if we release this without follow up changes it will break real programs. If Kuba is OK with it, go ahead and commit.

This revision is now accepted and ready to land.Dec 22 2018, 12:00 AM
yln updated this revision to Diff 179905.Jan 2 2019, 11:37 AM

Create diff via arcanist

  • Add failing test for deadlock detection for Objective-C @synchronize blocks
  • Implement Obj-C @synchronized like full-fledged mutexes
  • Activate test
  • Refactor @synchronized interceptors
  • Add test for @synchronized nesting with recursive locks
  • #include <objc/objc-sync.h> directly
  • Use CHECK_EQ instead of assert
  • Pass MutexFlagWriteReentrant flag
  • Add failing test for @synchronized with tagged pointer
  • Fix broken test
yln updated this revision to Diff 179915.Jan 2 2019, 12:12 PM

Trying to arc diff on git monorepo and arc patch && arc commit on svn working copy is painful, because path is longer on monorepo.

This revision was automatically updated to reflect the committed changes.