[clang] 203a2ca - [webkit.RefCntblBaseVirtualDtor] Make ThreadSafeRefCounted not generate warnings (#107676)

[via cfe-commits]

Author: Ryosuke Niwa
Date: 2024-09-10T22:25:03-07:00
New Revision: 203a2ca8cd6af505e11a38aebceeaf864271042c

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

LOG: [webkit.RefCntblBaseVirtualDtor] Make ThreadSafeRefCounted not generate warnings (#107676)

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.

Added: 
    clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp

Modified: 
    clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index 9df108e28ecdbb..ecba5f9aa23ee3 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -67,6 +67,32 @@ class DerefFuncDeleteExprVisitor
     const Decl *D = CE->getCalleeDecl();
     if (D && D->hasBody())
       return VisitBody(D->getBody());
+    else {
+      auto name = safeGetName(D);
+      if (name == "ensureOnMainThread" || name == "ensureOnMainRunLoop") {
+        for (unsigned i = 0; i < CE->getNumArgs(); ++i) {
+          auto *Arg = CE->getArg(i);
+          if (VisitLabmdaArgument(Arg))
+            return true;
+        }
+      }
+    }
+    return false;
+  }
+
+  bool VisitLabmdaArgument(const Expr *E) {
+    E = E->IgnoreParenCasts();
+    if (auto *TempE = dyn_cast<CXXBindTemporaryExpr>(E))
+      E = TempE->getSubExpr();
+    if (auto *ConstructE = dyn_cast<CXXConstructExpr>(E)) {
+      for (unsigned i = 0; i < ConstructE->getNumArgs(); ++i) {
+        auto *Arg = ConstructE->getArg(i);
+        if (auto *Lambda = dyn_cast<LambdaExpr>(Arg)) {
+          if (VisitBody(Lambda->getBody()))
+            return true;
+        }
+      }
+    }
     return false;
   }
 

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..01527addb52992
--- /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;
+        ensureOnMainRunLoop([&] {
+          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();
+};