[llvm] Add asserts in (ThreadSafe)?RefCountedBase destructors

[njames93]

Summary

Added a trivial destructor in release mode and in debug mode a destructor that asserts RefCount is indeed zero.
This ensure people aren't manually (maybe accidentally) destroying these objects like in this contrived example.

{
  std::unique_ptr<SomethingRefCounted> Object;
  holdIntrusiveOwnership(Object.get());
  // Object Destructor called here will assert.
}

[njames93]

llvm/include/llvm/ADT/IntrusiveRefCntPtr.h

120–121

Hmmm... This assert is causing clang-tidy tests to fail??? Investigating.

[njames93]

Fix assertion due to a pointer stored in a ManagedStatic.

[benlangmuir]

Removing myself as reviewer since I no longer work in this area. Someone who is more involved with llvm Support/ADT should review this.

[njames93]

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

190–196 ↗

(On Diff #309093)

Is this a case that happens in other places, ManagedStatics holding RefCounted objects, if so this could be extracted and reused in other places.

[dblaikie]

llvm/include/llvm/ADT/IntrusiveRefCntPtr.h

75–76

I can't quite understand this comment - perhaps you could try rephrasing it?

79–80

What does it mean to copy a ref counted object with a builtin ref count?

I would /guess/ maybe copies (or moves) of ref counted objects like this should only be valid when the ref count is 1 or maybe only 0? (since all the other users who might have an active reference certainly aren't getting a reference to the newly copied object implicitly)

Do you know which users which relied on any of the copy behavior? Maybe leave it as = default for now?

I guess it makes sense that the copy would have a zero ref count - same as if it were newly default constructed. But if no one's been using this effectively/at all already - maybe avoid creating this new behavior/changing the semantics here? What if we only allowed copies of objects with a zero ref count - is that sufficient for existing uses?

[dexonsmith]

llvm/include/llvm/ADT/IntrusiveRefCntPtr.h

75–80

I'm not quite following why you needed to add these special members; it seems like just the destructor would be enough for the assertion; but if you do need them, can/should they be = default?

[njames93]

Added test cases demonstrating copy and move behaviour.

[njames93]

I've added test cases which demonstrate what these copies/moves are about.
The move constructor probably would never get used. I can't see of a reason to move the contents of a ref counted pointer. In which case there could be an argument to delete it.
The copy constructor OTOH is very likely to be used (which i think is the reason its currently defined in RefCountedBase).
Adding the CopyAssignment and MoveAssignment operators are there to make sure the compiler wont define them implicitly, copying the ref count.

llvm/include/llvm/ADT/IntrusiveRefCntPtr.h

75–76

I understand it in my head, just can't think of the best way to write it down.

75–80

They can't be defaulted as we don't want to copy the RefCount.

[dexonsmith]

llvm/include/llvm/ADT/IntrusiveRefCntPtr.h

75–76

Maybe something like this would work:

/// Do not copy or move RefCount when constructing or assigning from
/// a derived type. These operations don't imply anything about the
/// number of IntrusiveRefCntPtr instances.

WDYT?

75–80

Ah, I get it, thanks for explaining.

[dblaikie]

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

190–196 ↗

(On Diff #309120)

I think probably the right solution is to have TrueMatcherImpl's dtor Release the same way its ctor Retains. Symmetry is nice/helps avoid splitting this quirk into two different places.

llvm/include/llvm/ADT/IntrusiveRefCntPtr.h

75–80

Do we need them at all, though?

The code prior to this patch has a copy ctor that "does the right thing" by producing a ref count of 0 for the new copy (same as a newly constructed object). No move operations will be provided - nor probably should there be any, the copy semantics are cheap/correct & not sure there's a more optimized implementation for move.

The copy assignment operaotr currently is probably broken (the implicit copy assignment is deprecated in the presence of a user-declared copy ctor - so we could probably delete that rather than adding it if it's not used (& at least, currently if it is used it'd be pretty broken).

[dblaikie]

I've committed 2e83ccc2ee333389110659f3cb313968a0c970d4 which takes parts of this patch - specifically fixing the test that was already intended to test the RefCountedBase's copy constructor, but didn't (because the test called the default constructor instead) and extending it to test/cover (& also fix) the similar behavior in ThreadSafeRefCountedBase.

That should leave this proposed patch to add the dtors and assertions.

[njames93]

Made this just focus on the destruction asserts.

[njames93]

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

190–196 ↗

(On Diff #309120)

That method is a nasty can of worms. If TrueMatcherImpl dtor calls Release, that Release call would realise the RefCount is now zero. That in turn would then call delete on TrueMatcherImpl resulting in a double-free.
I think symmetry is possibly the right way to go though, just in using a custom creator that calls Retain itself, remove that responsibility from TrueMatcherImpl.

[njames93]

Made ManagedStatic code transparent to users for RefCounted objects.

[njames93]

Remove unnecessary comment added in ASTMatchersInternal.cpp

[dblaikie]

llvm/include/llvm/Support/ManagedStatic.h

25 ↗

(On Diff #309524)

Are there many uses that rely on this? I don't think it's really worth all this infrastructure - compared to having it supported on an as-needed basis, such as directly in TrueMatcherImpl's ctor and dtor.

[njames93]

llvm/include/llvm/Support/ManagedStatic.h

25 ↗

(On Diff #309524)

It doesn't work in TrueMatcherImpl ctor/dtor due to the whole double free issue(See previous comment). I can go back to just supporting it for TrueMatcherImpl by writing a single custom creator/deleter for that class.

This approach taken was a little overkill but likely a little more foolproof for someone using the library. I'll go ahead with whichever approach you would prefer.

[dblaikie]

llvm/include/llvm/Support/ManagedStatic.h

25 ↗

(On Diff #309524)

Ah, right - thanks for walking me through it again, now I better understand your previous comment - sorry for that erroneous suggestion/confusion.

Fair points all.

Given all that, I'm sort of leaning towards the idea that maybe the right solution here is for the TrueMatcherInstance bear the cost of the complexity here (if it's the only one) with something like:

struct TrueMatcherImplCreator {
  static void *call() {
    return new IntrusiveRefCntPtr<TrueMatcherImpl>(new TrueMatcherImpl());  
  }
};
static llvm::ManagedStatic<IntrusiveRefCntPtr<TrueMatcherImpl>, TrueMatcherImplCreator> TrueMatcherInstance;

I worry about creating a fairly generic extension point for customizing how elements in ManagedStatic can be constructed and destroyed via specialization rather than via explicit creator/destroyer parameters.

And while the custom destroyer is a bit simpler mechanically (doesn't involve dynamically allocating an IntrusiveCntPtr, which is unintuitive to say the least) - I think sticking to the "if you ever share ownership of a RefCountedBase object, you must've allocated it with 'new' and be really sharing ownership - no lies" is probably a healthier model for RefCountedBase/IntrusiveRefCntPtr.

[njames93]

llvm/include/llvm/Support/ManagedStatic.h

25 ↗

(On Diff #309524)

Decided to take a step back. This is trying to fix a problem that's only here because we are using a ManagedStatic. when its not needed. A function scope static has nearly the same semantics of ManagedStatic and as this is only used in one function it seems a much better fit.

[njames93]

Refactored the ManagedStatic instance into a function static.

[dblaikie]

clang/lib/ASTMatchers/ASTMatchersInternal.cpp

155 ↗

(On Diff #309730)

You can remove this ctor declaration, since it's the same as the implicit default.

llvm/include/llvm/ADT/IntrusiveRefCntPtr.h

73

You could probably commit this separately - no need for extra review, but it's separate/independent of the rest of this change, I think?

llvm/include/llvm/Support/ManagedStatic.h

25 ↗

(On Diff #309524)

Yeah, I'm good with that - I guess ManagedStatic has the extra "deallocate/destroy on llvm_shutdown" and this object doesn't seem big/significant enough that it'll matter to destroy them on shutdown. No doubt we've got lots of other function-local statics of similar weight/use that we don't worry about destroying on llvm_shutdown.

llvm/include/llvm/ADT/IntrusiveRefCntPtr.h

 ///
 /// The lifetime of an object which inherits from RefCountedBase is managed by
 /// calls to Release() and Retain(), which increment and decrement the object's
 /// refcount, respectively.  When a Release() call decrements the refcount to 0,
 /// the object deletes itself.
 template <class Derived> class RefCountedBase {
   mutable unsigned RefCount = 0;
 
-public:
+protected:
   RefCountedBase() = default;
+  // Copy and move constructors/assignments are no-ops as the RefCount isn't
+  // dictated by the class directly.
   RefCountedBase(const RefCountedBase &) {}
+  RefCountedBase(RefCountedBase &&) {}
+  RefCountedBase &operator=(const RefCountedBase &) { return *this; }
+  RefCountedBase &operator=(RefCountedBase &&) { return *this; }
+
+#ifndef NDEBUG
+  ~RefCountedBase() {
+    assert(RefCount == 0 &&
+           "Destruction occured when there are still references to this.");
+  }
+#else
+  // Default the destructor in release builds, A trivial destructor may enable
+  // better codegen.
+  ~RefCountedBase() = default;
+#endif
 
+public:
   void Retain() const { ++RefCount; }
 
   void Release() const {
     assert(RefCount > 0 && "Reference count is already zero.");
     if (--RefCount == 0)
       delete static_cast<const Derived *>(this);
   }
 };
 
 /// A thread-safe version of \c RefCountedBase.
 template <class Derived> class ThreadSafeRefCountedBase {
-  mutable std::atomic<int> RefCount;
+  mutable std::atomic<int> RefCount{0};
 
 protected:
-  ThreadSafeRefCountedBase() : RefCount(0) {}
+  ThreadSafeRefCountedBase() = default;
+  ThreadSafeRefCountedBase(const ThreadSafeRefCountedBase &) {}
+  ThreadSafeRefCountedBase(ThreadSafeRefCountedBase &&) {}
+  ThreadSafeRefCountedBase &operator=(const ThreadSafeRefCountedBase &) {
+    return *this;
+  }
+  ThreadSafeRefCountedBase &operator=(ThreadSafeRefCountedBase &&) {
+    return *this;
+  }
+
+#ifndef NDEBUG
+  ~ThreadSafeRefCountedBase() {
+    assert(RefCount == 0 &&
+           "Destruction occured when there are still references to this.");
+  }
+#else
+  // Default the destructor in release builds, A trivial destructor may enable
+  // better codegen.
+  ~ThreadSafeRefCountedBase() = default;
+#endif
 
 public:
   void Retain() const { RefCount.fetch_add(1, std::memory_order_relaxed); }
 
   void Release() const {
     int NewRefCount = RefCount.fetch_sub(1, std::memory_order_acq_rel) - 1;
     assert(NewRefCount >= 0 && "Reference count was already zero.");
     if (NewRefCount == 0)