[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 06:08:17 PDT 2016


kubabrecka added inline comments.

================
Comment at: lib/tsan/rtl/tsan_interceptors_mac.cc:286
@@ +285,3 @@
+// atomic decrement that is used to check when the object's refcount drops to 0.
+STDCXX_INTERCEPTOR(void, _ZNSt3__119__shared_weak_count16__release_sharedEv,
+                   void *o) {
----------------
dvyukov wrote:
> Why we don't need to intercept release of weak references?
> Is it that last shared references releases weak, or vice versa?
As far as I know, the destructor of the object is only called when the last shared reference is destroyed.  Weak references don’t matter, as they don’t run the destructor.  The only issue I’m trying to solve here is a false positive when the destructor of a user’s object is called when the last shared reference is freed.  I’m not aware of other false positives with C++ shared/weak pointers.

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


http://reviews.llvm.org/D21609





More information about the llvm-commits mailing list