[PATCH] D55959: [TSan] Enable detection of lock-order-inversions for Objective-C @synchronized

[Julian Lettner via Phabricator via llvm-commits]

yln marked 3 inline comments as done.
yln added inline comments.


================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interceptors_mac.cc:328
+  uptr addr = SyncAddressForObjCObject(obj);
+  MutexPreLock(thr, pc, addr);
   int result = REAL(objc_sync_enter)(obj);
----------------
dvyukov wrote:
> dvyukov wrote:
> > 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.
> We don't check it today, but it can make sense to pass MutexFlagWriteReentrant because later we can start checking it.
Passing it on calls that could create the backing `SyncVar` is sufficient, correct?
That is, `MutexPreLock`, or `MutexPostLock` (when `detect_deadlocks=0`).



================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interceptors_mac.cc:328
+  uptr addr = SyncAddressForObjCObject(obj);
+  MutexPreLock(thr, pc, addr);
   int result = REAL(objc_sync_enter)(obj);
----------------
yln wrote:
> dvyukov wrote:
> > dvyukov wrote:
> > > 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.
> > We don't check it today, but it can make sense to pass MutexFlagWriteReentrant because later we can start checking it.
> Passing it on calls that could create the backing `SyncVar` is sufficient, correct?
> That is, `MutexPreLock`, or `MutexPostLock` (when `detect_deadlocks=0`).
> 
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?


Repository:
  rCRT Compiler Runtime

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55959/new/

https://reviews.llvm.org/D55959