[PATCH] D56238: [TSan] Support Objective-C @synchronized with tagged pointers

[Dmitry Vyukov via Phabricator via llvm-commits]

dvyukov accepted this revision.
dvyukov added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lib/tsan/rtl/tsan_interceptors_mac.cc:319
+  if (h.created()) {
+    *h = (uptr) InternalAlloc(/*size=*/1);
+  }
----------------
yln wrote:
> dvyukov wrote:
> > yln wrote:
> > > dvyukov wrote:
> > > > It's safer/better to use a user allocation. Internal allocations may not be covered by shadow in future (not sure it has not in all configurations). And mutexes have limited functionality in non-app mem.
> > > With `user_alloc` TSan reports a race in the `objc-synchronize-tagged.mm` test. I do not completely understand what the cause is: the address/allocation is supposed to be racy by nature since it represents a mutex, which itself is used to establish synchronization. Do you have any insights here?
> > > Plain old `malloc` seems to work. 
> > malloc and user_alloc should generally be the same. But I wonder if malloc interceptor detects that it's called a non-intrumented library and enables ignores so that it does not model access to the range.
> > This can be checked by running with TSAN_OPTIONS=ignore_noninstrumented_modules=0
> > 
> > Yes, I think a false report is possible if we use user memory without ignores.
> > Consider a thread creates a mutex, then passes it to 2 threads and these 2 threads lock the mutex concurrently. In this case all proper synchronization is in place and the code is correct.
> > However, what tsan sees is that the mutex is created by one of the 2 threads that calls lock first. So it looks like one thread creates a mutex and another thread immidiately tries to lock it without any synchronization.
> > 
> > Let's try to surround user_alloc with ThreadIgnoreBegin/End. Does it help?
> Your analysis is correct. With `TSAN_OPTIONS=ignore_noninstrumented_modules=0` also `malloc` reports a warning and `ThreadIgnoreBegin/End` fixes the failing test. Good that we had that test. :)
> 
> One last question: looking at the code I don't think so, but does the size of the dummy allocation matter, i.e., should it be something like `sizeof(pthread_mutex_t)`?
No, it doesn't have to be larger than 1 byte because a user mutex implementation can be just 1 byte (think of a simple spinlock). Tsan does not store any info inline (that would interfere with the mutex code itself), all info is stored on the side, so 1 byte enough.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D56238