[PATCH] D14987: [tsan] Add interceptors for Darwin-specific locking APIs

[Dmitry Vyukov via llvm-commits]

dvyukov accepted this revision.
dvyukov added a comment.
This revision is now accepted and ready to land.

LGTM

But MutexLock/MutexUnlock annotations are actually super useful for several reasons:

- they allow to detect deadlocks
- the allow to detect unsafe publication of objects with mutexes (which are usually undetectable otherwise, because a first mutex lock synchronizes the rest of the accesses)
- they allow to detect mutex unlock racing with free (which is a very frequent bug: last touch of object is an unlock)
- they allow to print sets of locked mutexes in reports
- they allow to report misuses of mutexes

and there can be other reasons and future improvements.

So please do MutexLock/MutexUnlock in a subsequent patch.  The issue with double lock is not specific to only these mutexes. Go mutexes are also just zero chunks of memory without an explicit ctor. Java mutexes are the same. Tsan also allows to annotate user mutexes, which well can well be the same.

I see that in java interface (tsan_interface_java.cc) I do the following hack to set recursive flag on mutex:

  MutexCreate(thr, pc, addr, true, true, true);
  MutexLock(thr, pc, addr);

This is somewhat ugly and slow. I was thinking about a better solution to allow MutexLock to also update mutex flags. But the complication here is that for pthread mutexes/condvars we don't know whether the mutex is recursive or not. I think we need something along the following lines. MutexLocks accepts a tri-state recursive flag which can be Yes, No or DontKnow. If it DontKnow then we assume that MutexCreate has initialized it. If it is Yes or No then we update the mutex flag. Should not be too difficult to do, there are few existing callers but for them you can just pass DontKnow as it won't change existing behavior.

Does it sound good to you?


http://reviews.llvm.org/D14987