[PATCH] D55959: [TSan] Enable detection of lock-order-inversions for Objective-C @synchronized
Julian Lettner via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Dec 21 11:25:10 PST 2018
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
More information about the llvm-commits
mailing list