[clang] [webkit.RefCntblBaseVirtualDtor] Make ThreadSafeRefCounted not generate warnings (PR #107676)

[Ryosuke Niwa via cfe-commits]

https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/107676

>From 9a8c60355f88d7d4cee6d9f75312bb87d13b74a9 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Sat, 7 Sep 2024 01:45:05 -0700
Subject: [PATCH] [webkit.RefCntblBaseVirtualDtor] Make ThreadSafeRefCounted
 not generate warnings

This PR makes WebKit's RefCntblBaseVirtualDtor checker not generate a warning for
ThreadSafeRefCounted when the destruction thread is a specific thread.

Prior to this PR, we only allowed CRTP classes without a virtual destructor
if its deref function had an explicit cast to the derived type, skipping any
lambda declarations which aren't invoked. This ends up generating a warning for
ThreadSafeRefCounted when a specific thread is used to destruct the object
because there is no inline body / definition for ensureOnMainThread and
ensureOnMainRunLoop and DerefFuncDeleteExprVisitor concludes that there is no
explicit delete of the derived type.

This PR relaxes the condition DerefFuncDeleteExprVisitor checks by allowing
a delete expression to appear within a lambda declaration if it's an argument
to an "opaque" function; i.e. a function without definition / body.
---
 .../WebKit/RefCntblBaseVirtualDtorChecker.cpp |  22 +-
 .../ref-cntbl-crtp-base-no-virtual-dtor.cpp   | 232 ++++++++++++++++++
 2 files changed, 253 insertions(+), 1 deletion(-)
 create mode 100644 clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp

diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index 26879f2f87c8be..8e97efc1ab8e8e 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -17,6 +17,7 @@
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SetVector.h"
 #include <optional>
 
@@ -67,6 +68,15 @@ class DerefFuncDeleteExprVisitor
     const Decl *D = CE->getCalleeDecl();
     if (D && D->hasBody())
       return VisitBody(D->getBody());
+    else if (!VisitLambdaBody) {
+      for (unsigned i = 0; i < CE->getNumArgs(); ++i) {
+        auto *Arg = CE->getArg(i);
+        VisitLambdaBody = true;
+        auto Restore = llvm::make_scope_exit([&] { VisitLambdaBody = false; });
+        if (VisitChildren(Arg))
+          return true;
+      }
+    }
     return false;
   }
 
@@ -113,12 +123,22 @@ class DerefFuncDeleteExprVisitor
 
   // Return false since the contents of lambda isn't necessarily executed.
   // If it is executed, VisitCallExpr above will visit its body.
-  bool VisitLambdaExpr(const LambdaExpr *) { return false; }
+  // Allow returning true for a lambda if it's a function argument to another
+  // function without body / definition.
+  bool VisitLambdaExpr(const LambdaExpr *E) {
+    if (VisitLambdaBody) {
+      VisitLambdaBody = false;
+      auto Restore = llvm::make_scope_exit([&] { VisitLambdaBody = true; });
+      return VisitChildren(E);
+    }
+    return false;
+  }
 
 private:
   const TemplateArgumentList *ArgList{nullptr};
   const CXXRecordDecl *ClassDecl;
   llvm::DenseSet<const Stmt *> VisitedBody;
+  bool VisitLambdaBody{false};
 };
 
 class RefCntblBaseVirtualDtorChecker
diff --git a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp
new file mode 100644
index 00000000000000..e149c1d3d3575f
--- /dev/null
+++ b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp
@@ -0,0 +1,232 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.RefCntblBaseVirtualDtor -verify %s
+
+#include "mock-types.h"
+
+namespace Detail {
+
+template<typename Out, typename... In>
+class CallableWrapperBase {
+public:
+    virtual ~CallableWrapperBase() { }
+    virtual Out call(In...) = 0;
+};
+
+template<typename, typename, typename...> class CallableWrapper;
+
+template<typename CallableType, typename Out, typename... In>
+class CallableWrapper : public CallableWrapperBase<Out, In...> {
+public:
+    explicit CallableWrapper(CallableType&& callable)
+        : m_callable(WTFMove(callable)) { }
+    CallableWrapper(const CallableWrapper&) = delete;
+    CallableWrapper& operator=(const CallableWrapper&) = delete;
+    Out call(In... in) final;
+private:
+    CallableType m_callable;
+};
+
+} // namespace Detail
+
+template<typename> class Function;
+
+template<typename Out, typename... In> Function<Out(In...)> adopt(Detail::CallableWrapperBase<Out, In...>*);
+
+template <typename Out, typename... In>
+class Function<Out(In...)> {
+public:
+    using Impl = Detail::CallableWrapperBase<Out, In...>;
+
+    Function() = default;
+
+    template<typename FunctionType>
+    Function(FunctionType f);
+
+    Out operator()(In... in) const;
+    explicit operator bool() const { return !!m_callableWrapper; }
+
+private:
+    enum AdoptTag { Adopt };
+    Function(Impl* impl, AdoptTag)
+        : m_callableWrapper(impl)
+    {
+    }
+
+    friend Function adopt<Out, In...>(Impl*);
+
+    Impl* m_callableWrapper;
+};
+
+template<typename Out, typename... In> Function<Out(In...)> adopt(Detail::CallableWrapperBase<Out, In...>* impl)
+{
+    return Function<Out(In...)>(impl, Function<Out(In...)>::Adopt);
+}
+
+template<typename T, typename PtrTraits = RawPtrTraits<T>, typename RefDerefTraits = DefaultRefDerefTraits<T>> Ref<T, PtrTraits, RefDerefTraits> adoptRef(T&);
+
+template<typename T, typename _PtrTraits, typename RefDerefTraits>
+inline Ref<T, _PtrTraits, RefDerefTraits> adoptRef(T& reference)
+{
+    return Ref<T, _PtrTraits, RefDerefTraits>(reference);
+}
+
+enum class DestructionThread : unsigned char { Any, Main, MainRunLoop };
+void ensureOnMainThread(Function<void()>&&); // Sync if called on main thread, async otherwise.
+void ensureOnMainRunLoop(Function<void()>&&); // Sync if called on main run loop, async otherwise.
+
+class ThreadSafeRefCountedBase {
+public:
+    ThreadSafeRefCountedBase() = default;
+
+    void ref() const
+    {
+        ++m_refCount;
+    }
+
+    bool hasOneRef() const
+    {
+        return refCount() == 1;
+    }
+
+    unsigned refCount() const
+    {
+        return m_refCount;
+    }
+
+protected:
+    bool derefBase() const
+    {
+      if (!--m_refCount) {
+          m_refCount = 1;
+          return true;
+      }
+      return false;
+    }
+
+private:
+    mutable unsigned m_refCount { 1 };
+};
+
+template<class T, DestructionThread destructionThread = DestructionThread::Any> class ThreadSafeRefCounted : public ThreadSafeRefCountedBase {
+public:
+    void deref() const
+    {
+        if (!derefBase())
+            return;
+
+        if constexpr (destructionThread == DestructionThread::Any) {
+            delete static_cast<const T*>(this);
+        } else if constexpr (destructionThread == DestructionThread::Main) {
+            ensureOnMainThread([this] {
+                delete static_cast<const T*>(this);
+            });
+        }
+    }
+
+protected:
+    ThreadSafeRefCounted() = default;
+};
+
+class FancyRefCountedClass final : public ThreadSafeRefCounted<FancyRefCountedClass, DestructionThread::Main> {
+public:
+    static Ref<FancyRefCountedClass> create()
+    {
+        return adoptRef(*new FancyRefCountedClass());
+    }
+
+    virtual ~FancyRefCountedClass();
+
+private:
+    FancyRefCountedClass();
+};
+
+template<class T, DestructionThread destructionThread = DestructionThread::Any> class BadThreadSafeRefCounted : public ThreadSafeRefCountedBase {
+public:
+    void deref() const
+    {
+        if (!derefBase())
+            return;
+
+        [this] {
+          delete static_cast<const T*>(this);
+        };
+    }
+
+protected:
+    BadThreadSafeRefCounted() = default;
+};
+
+class FancyRefCountedClass2 final : public ThreadSafeRefCounted<FancyRefCountedClass, DestructionThread::Main> {
+// expected-warning at -1{{Class 'ThreadSafeRefCounted<FancyRefCountedClass, DestructionThread::Main>' is used as a base of class 'FancyRefCountedClass2' but doesn't have virtual destructor}}
+public:
+    static Ref<FancyRefCountedClass2> create()
+    {
+        return adoptRef(*new FancyRefCountedClass2());
+    }
+
+    virtual ~FancyRefCountedClass2();
+
+private:
+    FancyRefCountedClass2();
+};
+
+template<class T, DestructionThread destructionThread = DestructionThread::Any> class NestedThreadSafeRefCounted : public ThreadSafeRefCountedBase {
+public:
+    void deref() const
+    {
+        if (!derefBase())
+            return;
+        ensureOnMainThread([&] {
+          auto destroyThis = [&] {
+            delete static_cast<const T*>(this);
+          };
+          destroyThis();
+        });
+    }
+
+protected:
+    NestedThreadSafeRefCounted() = default;
+};
+
+class FancyRefCountedClass3 final : public NestedThreadSafeRefCounted<FancyRefCountedClass3, DestructionThread::Main> {
+public:
+    static Ref<FancyRefCountedClass3> create()
+    {
+        return adoptRef(*new FancyRefCountedClass3());
+    }
+
+    virtual ~FancyRefCountedClass3();
+
+private:
+    FancyRefCountedClass3();
+};
+
+template<class T, DestructionThread destructionThread = DestructionThread::Any> class BadNestedThreadSafeRefCounted : public ThreadSafeRefCountedBase {
+public:
+    void deref() const
+    {
+        if (!derefBase())
+            return;
+        ensureOnMainThread([&] {
+          auto destroyThis = [&] {
+            delete static_cast<const T*>(this);
+          };
+        });
+    }
+
+protected:
+    BadNestedThreadSafeRefCounted() = default;
+};
+
+class FancyRefCountedClass4 final : public BadNestedThreadSafeRefCounted<FancyRefCountedClass4, DestructionThread::Main> {
+// expected-warning at -1{{Class 'BadNestedThreadSafeRefCounted<FancyRefCountedClass4, DestructionThread::Main>' is used as a base of class 'FancyRefCountedClass4' but doesn't have virtual destructor}}
+public:
+    static Ref<FancyRefCountedClass4> create()
+    {
+        return adoptRef(*new FancyRefCountedClass4());
+    }
+
+    virtual ~FancyRefCountedClass4();
+
+private:
+    FancyRefCountedClass4();
+};