[PATCH] D21609: [tsan] Intercept libcxx __release_shared to avoid false positive with weak_ptrs and destructors in C++

Kuba Brecka via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 23 07:30:08 PDT 2016


kubabrecka added inline comments.

================
Comment at: lib/tsan/rtl/tsan_interceptors_mac.cc:289
@@ +288,3 @@
+  SCOPED_INTERCEPTOR_RAW(_ZNSt3__119__shared_weak_count16__release_sharedEv, o);
+  Acquire(thr, pc, (uptr)o);
+  Release(thr, pc, (uptr)o);
----------------
dvyukov wrote:
> kubabrecka wrote:
> > dvyukov wrote:
> > > kubabrecka wrote:
> > > > dvyukov wrote:
> > > > > I think Acquire should happen _after_ the actual release to synchronize with all preceding Release's.
> > > > > Normally,  you can't touch the object after release in all but the last thread, because the object can already be freed (unmapped). So ideally we need to determine the last thread and do Acquire only in it.
> > > > > But I wonder if tsan Release is OK -- it won't touch object memory.
> > > > > 
> > > > I’m not sure what you’re suggesting here -- are you saying that Acquire should be at the end of the interceptor?  Note that `o` here isn’t the user’s object, nor the shared_ptr, it’s the control block, which is heap-allocated.  I actually think that even in the last thread (last shared reference is destroyed), `o` is already destroyed when the REAL() call finishes (the object destroys itself).
> > > > 
> > > > Secondly, I remember accidentally calling Release() on just-freed memory, which resulted in subtle crashes and many wasted hours of debugging.  I was actually thinking of implementing a DCHECK that would trigger whenever Release or Acquire is called on a deallocated heap object.
> > > > 
> > > > What is the problem you’re seeing here with this patch?  Is it about two threads doing Acquires first and only then doing Releases, in which case we’d still report a false positive?  Then it sounds we need an atomic Acquire+Release, which we could solve with some atomic CAS loop or, worst case, a spinlock.
> > > > 
> > > > Suggestions?
> > > > Is it about two threads doing Acquires first and only then doing Releases, in which case we’d still report a false positive?
> > > 
> > > Yes.
> > > I see there are some other helper functions that return true/false and don't destroy the object. Is it possible to intercept any of them?
> > > 
> > I don’t think so, unfortunately.  The other functions get inlined into `__shared_weak_count::__release_shared` and they’re not called directly.
> The simplest and the most reliable thing would be:
> 
> u64 shared_ptr_sync[kSomePrime];
> 
>   uptr sync = (uptr)&shared_ptr_sync[hash(o) % kSomePrime];
>   Release(thr, pc, sync);
>   SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_START();
>   REAL(_ZNSt3__119__shared_weak_count16__release_sharedEv)(o);
>   SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_END();
>   Acquire(thr, pc, sync);
> 
> Note that kSomePrime should not be too large, because eventually every slot will contain maximally-sized vector clock which takes 8*num_threads bytes.
> 
> Also comment it all.
I’m probably missing some point.  This doesn’t work in the case when two threads release a shared ptr:  T1 calls Release(), T2 calls Release(), T1 calls REAL(), T2 calls REAL(), which is the last reference, it gets destroyed, the user’s object destructor is called and we report a false positive (because there hasn’t been any acquires at all).  If I put a `Sleep(1)` before Acquire(), the test case I provided reports the false positive reliably.


http://reviews.llvm.org/D21609





More information about the llvm-commits mailing list