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

[Dmitry Vyukov via Phabricator via llvm-commits]

dvyukov 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);
----------------
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.


================
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:
> 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.


================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interceptors_mac.cc:330
   int result = REAL(objc_sync_enter)(obj);
-  if (obj) Acquire(thr, pc, SyncAddressForObjCObject(obj));
+  assert(result == OBJC_SYNC_SUCCESS);
+  MutexPostLock(thr, pc, addr);
----------------
We do not use assert in this code base. Use DCHECK_EQ, or better CHECK_EQ since this is not super performance-critical part.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D55959