From dc361d5c2a2dd34839ff4cd48844cf1bf9a87c62 Mon Sep 17 00:00:00 2001
From: Nathan James <n.james93@hotmail.co.uk>
Date: Mon, 7 Dec 2020 20:20:08 +0000
Subject: [PATCH] [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
---
 clang/lib/ASTMatchers/ASTMatchersInternal.cpp | 17 +++++++----------
 llvm/include/llvm/ADT/IntrusiveRefCntPtr.h    | 23 +++++++++++++++++++++++
 2 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
index 257a893..7f96f4b 100644
--- a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -150,15 +150,9 @@ private:
 };
 
 /// 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 @@ private:
 
 } // 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 41e5e6b..7e65853 100644
--- a/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
+++ b/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
@@ -75,6 +75,18 @@ public:
   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 @@ protected:
   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); }
 
-- 
2.7.4