[llvm-branch-commits] [clang] dc361d5 - [llvm] Add asserts in (ThreadSafe)?RefCountedBase destructors

[Nathan James via llvm-branch-commits]

Author: Nathan James
Date: 2020-12-07T20:20:08Z
New Revision: dc361d5c2a2dd34839ff4cd48844cf1bf9a87c62

URL: https://github.com/llvm/llvm-project/commit/dc361d5c2a2dd34839ff4cd48844cf1bf9a87c62
DIFF: https://github.com/llvm/llvm-project/commit/dc361d5c2a2dd34839ff4cd48844cf1bf9a87c62.diff

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

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.
```lang=c++
{
  std::unique_ptr<SomethingRefCounted> Object;
  holdIntrusiveOwnership(Object.get());
  // Object Destructor called here will assert.
}
```

Reviewed By: dblaikie

Differential Revision: https://reviews.llvm.org/D92480

Added: 
    

Modified: 
    clang/lib/ASTMatchers/ASTMatchersInternal.cpp
    llvm/include/llvm/ADT/IntrusiveRefCntPtr.h

Removed: 
    


################################################################################
diff  --git a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
index 257a893ccaa6..7f96f4bc03af 100644
--- a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -150,15 +150,9 @@ class IdDynMatcher : public DynMatcherInterface {
 };
 
 /// A matcher that always returns true.
-///
-/// We only ever need one instance of this matcher, so we create a global one
-/// and reuse it to reduce the overhead of the matcher and increase the chance
-/// of cache hits.
 class TrueMatcherImpl : public DynMatcherInterface {
 public:
-  TrueMatcherImpl() {
-    Retain(); // Reference count will never become zero.
-  }
+  TrueMatcherImpl() = default;
 
   bool dynMatches(const DynTypedNode &, ASTMatchFinder *,
                   BoundNodesTreeBuilder *) const override {
@@ -193,8 +187,6 @@ class DynTraversalMatcherImpl : public DynMatcherInterface {
 
 } // namespace
 
-static llvm::ManagedStatic<TrueMatcherImpl> TrueMatcherInstance;
-
 bool ASTMatchFinder::isTraversalIgnoringImplicitNodes() const {
   return getASTContext().getParentMapContext().getTraversalKind() ==
          TK_IgnoreUnlessSpelledInSource;
@@ -273,7 +265,12 @@ DynTypedMatcher::withTraversalKind(ast_type_traits::TraversalKind TK) {
 }
 
 DynTypedMatcher DynTypedMatcher::trueMatcher(ASTNodeKind NodeKind) {
-  return DynTypedMatcher(NodeKind, NodeKind, &*TrueMatcherInstance);
+  // We only ever need one instance of TrueMatcherImpl, so we create a static
+  // instance and reuse it to reduce the overhead of the matcher and increase
+  // the chance of cache hits.
+  static const llvm::IntrusiveRefCntPtr<TrueMatcherImpl> Instance =
+      new TrueMatcherImpl();
+  return DynTypedMatcher(NodeKind, NodeKind, Instance);
 }
 
 bool DynTypedMatcher::canMatchNodesOfKind(ASTNodeKind Kind) const {

diff  --git a/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h b/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
index 41e5e6b9f8ed..7e6585378ec4 100644
--- a/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
+++ b/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
@@ -75,6 +75,18 @@ template <class Derived> class RefCountedBase {
   RefCountedBase(const RefCountedBase &) {}
   RefCountedBase &operator=(const RefCountedBase &) = delete;
 
+#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 {
@@ -94,6 +106,17 @@ template <class Derived> class ThreadSafeRefCountedBase {
   ThreadSafeRefCountedBase &
   operator=(const ThreadSafeRefCountedBase &) = delete;
 
+#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); }