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

>From 9c2ae2b2b14d27270589f3775df95a7547e74c83 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rn...@apple.com>
Date: Mon, 20 May 2024 16:12:44 -0700
Subject: [PATCH 1/2] [webkit.RefCntblBaseVirtualDtor] Allow CRTP classes
 without a virtual destructor.

Exempt CRTP (Curiously Recurring Template Pattern) classes with a delete 
operation
acting on "this" pointer with an appropriate cast from the requirement that
a ref-countable superclass must have a virtual destructor.

To do this, this PR introduces new DerefAnalysisVisitor, which looks for a 
delete
operation with an explicit cast to the derived class in a base class.

This PR also changes the checker so that we only check a given class's immediate
base class instead of all ancestor base classes in the class hierarchy. This is
sufficient because the checker will eventually see the definition for every 
class
in the class hierarchy and transitively proves every ref-counted base class has
a virtual destructor or deref function which casts this pointer back to the 
derived
class before deleting. Without this change, we would keep traversing the same 
list
of base classes whenever we encounter a new subclass, which is wholly 
unnecessary.

It's possible for DerefAnalysisVisitor to come to a conclusoin that there isn't
enough information to determine whether a given templated superclass invokes 
delete
operation on a subclass when the template isn't fully specialized for the 
subclass.
In this case, we return std::nullopt in HasSpecializedDelete, and 
visitCXXRecordDecl
will skip this declaration. This is okay because the checker will eventually see
a concreate fully specialized class definition if it ever gets instantiated.
---
 .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 311 +++++++++++++----
 ...virtual-dtor-ref-deref-on-diff-classes.cpp |   1 +
 .../ref-cntbl-base-virtual-dtor-templates.cpp | 324 +++++++++++++++++-
 3 files changed, 567 insertions(+), 69 deletions(-)

diff --git 
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index 7f4c3a7b787e8..efb7b4456f2ca 100644
--- 
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++ 
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -11,16 +11,134 @@
 #include "PtrTypesSemantics.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/StmtVisitor.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
+#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/SetVector.h"
 #include <optional>
 
 using namespace clang;
 using namespace ento;
 
 namespace {
+
+class DerefAnalysisVisitor
+    : public ConstStmtVisitor<DerefAnalysisVisitor, bool> {
+  // Returns true if any of child statements return true.
+  bool VisitChildren(const Stmt *S) {
+    for (const Stmt *Child : S->children()) {
+      if (Child && Visit(Child))
+        return true;
+    }
+    return false;
+  }
+
+  bool VisitBody(const Stmt *Body) {
+    if (!Body)
+      return false;
+
+    auto [It, IsNew] = VisitedBody.insert(Body);
+    if (!IsNew) // This body is recursive
+      return false;
+
+    return Visit(Body);
+  }
+
+public:
+  DerefAnalysisVisitor(const TemplateArgumentList &ArgList,
+                       const CXXRecordDecl *ClassDecl)
+      : ArgList(&ArgList), ClassDecl(ClassDecl) {}
+
+  DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) : ClassDecl(ClassDecl) 
{}
+
+  std::optional<bool> HasSpecializedDelete(CXXMethodDecl *Decl) {
+    if (auto *Body = Decl->getBody())
+      return VisitBody(Body);
+    if (auto *Tmpl = Decl->getTemplateInstantiationPattern())
+      return std::nullopt; // Indeterminate. There was no concrete instance.
+    return false;
+  }
+
+  bool VisitCallExpr(const CallExpr *CE) {
+    auto *Callee = CE->getCallee();
+    while (auto *Expr = dyn_cast<CastExpr>(Callee))
+      Callee = Expr->getSubExpr();
+    if (auto *DeclRef = dyn_cast<DeclRefExpr>(Callee)) {
+      auto *Decl = DeclRef->getDecl();
+      if (auto *VD = dyn_cast<VarDecl>(Decl)) {
+        if (auto *Init = VD->getInit()) {
+          if (auto *Lambda = dyn_cast<LambdaExpr>(Init))
+            return VisitBody(Lambda->getBody());
+        }
+      } else if (auto *FD = dyn_cast<FunctionDecl>(Decl))
+        return VisitBody(FD->getBody());
+    }
+    return false;
+  }
+
+  bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) {
+    auto *Callee = MCE->getMethodDecl();
+    if (!Callee)
+      return false;
+    return VisitBody(Callee->getBody());
+  }
+
+  bool VisitCXXDeleteExpr(const CXXDeleteExpr *E) {
+    auto *Arg = E->getArgument();
+    while (Arg) {
+      if (auto *Paren = dyn_cast<ParenExpr>(Arg))
+        Arg = Paren->getSubExpr();
+      else if (auto *Cast = dyn_cast<CastExpr>(Arg)) {
+        Arg = Cast->getSubExpr();
+        auto CastType = Cast->getType();
+        if (auto *PtrType = dyn_cast<PointerType>(CastType)) {
+          auto PointeeType = PtrType->getPointeeType();
+          while (auto *ET = dyn_cast<ElaboratedType>(PointeeType)) {
+            if (ET->isSugared())
+              PointeeType = ET->desugar();
+          }
+          if (auto *ParmType = dyn_cast<TemplateTypeParmType>(PointeeType)) {
+            if (ArgList) {
+              auto ParmIndex = ParmType->getIndex();
+              auto Type = ArgList->get(ParmIndex).getAsType();
+              if (auto *RD = dyn_cast<RecordType>(Type)) {
+                if (RD->getDecl() == ClassDecl)
+                  return true;
+              }
+            }
+          } else if (auto *RD = dyn_cast<RecordType>(PointeeType)) {
+            if (RD->getDecl() == ClassDecl)
+              return true;
+          } else if (auto *ST =
+                         dyn_cast<SubstTemplateTypeParmType>(PointeeType)) {
+            auto Type = ST->getReplacementType();
+            if (auto *RD = dyn_cast<RecordType>(Type)) {
+              if (RD->getDecl() == ClassDecl)
+                return true;
+            }
+          }
+        }
+      } else
+        break;
+    }
+    return false;
+  }
+
+  bool VisitStmt(const Stmt *S) { return VisitChildren(S); }
+
+  // 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; }
+
+private:
+  const TemplateArgumentList *ArgList{nullptr};
+  const CXXRecordDecl *ClassDecl;
+  llvm::DenseSet<const Stmt *> VisitedBody;
+};
+
 class RefCntblBaseVirtualDtorChecker
     : public Checker<check::ASTDecl<TranslationUnitDecl>> {
 private:
@@ -51,63 +169,94 @@ class RefCntblBaseVirtualDtorChecker
       bool shouldVisitImplicitCode() const { return false; }
 
       bool VisitCXXRecordDecl(const CXXRecordDecl *RD) {
-        Checker->visitCXXRecordDecl(RD);
+        if (!RD->hasDefinition())
+          return true;
+
+        Decls.insert(RD);
+
+        for (auto &Base : RD->bases()) {
+          const auto AccSpec = Base.getAccessSpecifier();
+          if (AccSpec == AS_protected || AccSpec == AS_private ||
+              (AccSpec == AS_none && RD->isClass()))
+            continue;
+
+          QualType T = Base.getType();
+          if (T.isNull())
+            continue;
+
+          const CXXRecordDecl *C = T->getAsCXXRecordDecl();
+          if (!C)
+            continue;
+
+          if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(C)) {
+            auto &Args = CTSD->getTemplateArgs();
+            for (unsigned i = 0; i < Args.size(); ++i) {
+              if (Args[i].getKind() != TemplateArgument::Type)
+                continue;
+              auto TemplT = Args[i].getAsType();
+              if (TemplT.isNull())
+                continue;
+
+              bool IsCRTP = TemplT->getAsCXXRecordDecl() == RD;
+              if (!IsCRTP)
+                continue;
+              CRTPs.insert(C);
+            }
+          }
+        }
+
         return true;
       }
+
+      llvm::SetVector<const CXXRecordDecl *> Decls;
+      llvm::DenseSet<const CXXRecordDecl *> CRTPs;
     };
 
     LocalVisitor visitor(this);
     visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
+    for (auto *RD : visitor.Decls) {
+      if (visitor.CRTPs.contains(RD))
+        continue;
+      visitCXXRecordDecl(RD);
+    }
   }
 
   void visitCXXRecordDecl(const CXXRecordDecl *RD) const {
     if (shouldSkipDecl(RD))
       return;
 
-    CXXBasePaths Paths;
-    Paths.setOrigin(RD);
-
-    const CXXBaseSpecifier *ProblematicBaseSpecifier = nullptr;
-    const CXXRecordDecl *ProblematicBaseClass = nullptr;
+    for (auto &Base : RD->bases()) {
+      const auto AccSpec = Base.getAccessSpecifier();
+      if (AccSpec == AS_protected || AccSpec == AS_private ||
+          (AccSpec == AS_none && RD->isClass()))
+        continue;
 
-    const auto IsPublicBaseRefCntblWOVirtualDtor =
-        [RD, &ProblematicBaseSpecifier,
-         &ProblematicBaseClass](const CXXBaseSpecifier *Base, CXXBasePath &) {
-          const auto AccSpec = Base->getAccessSpecifier();
-          if (AccSpec == AS_protected || AccSpec == AS_private ||
-              (AccSpec == AS_none && RD->isClass()))
-            return false;
+      auto hasRefInBase = clang::hasPublicMethodInBase(&Base, "ref");
+      auto hasDerefInBase = clang::hasPublicMethodInBase(&Base, "deref");
 
-          auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref");
-          auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref");
+      bool hasRef = hasRefInBase && *hasRefInBase != nullptr;
+      bool hasDeref = hasDerefInBase && *hasDerefInBase != nullptr;
 
-          bool hasRef = hasRefInBase && *hasRefInBase != nullptr;
-          bool hasDeref = hasDerefInBase && *hasDerefInBase != nullptr;
+      QualType T = Base.getType();
+      if (T.isNull())
+        continue;
 
-          QualType T = Base->getType();
-          if (T.isNull())
-            return false;
+      const CXXRecordDecl *C = T->getAsCXXRecordDecl();
+      if (!C)
+        continue;
 
-          const CXXRecordDecl *C = T->getAsCXXRecordDecl();
-          if (!C)
-            return false;
-          if (isRefCountedClass(C))
-            return false;
-
-          bool AnyInconclusiveBase = false;
-          const auto hasPublicRefInBase =
-              [&AnyInconclusiveBase](const CXXBaseSpecifier *Base,
-                                     CXXBasePath &) {
-                auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref");
-                if (!hasRefInBase) {
-                  AnyInconclusiveBase = true;
-                  return false;
-                }
-                return (*hasRefInBase) != nullptr;
-              };
-          const auto hasPublicDerefInBase = [&AnyInconclusiveBase](
-                                                const CXXBaseSpecifier *Base,
-                                                CXXBasePath &) {
+      bool AnyInconclusiveBase = false;
+      const auto hasPublicRefInBase =
+          [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
+            auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref");
+            if (!hasRefInBase) {
+              AnyInconclusiveBase = true;
+              return false;
+            }
+            return (*hasRefInBase) != nullptr;
+          };
+      const auto hasPublicDerefInBase =
+          [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
             auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref");
             if (!hasDerefInBase) {
               AnyInconclusiveBase = true;
@@ -115,28 +264,42 @@ class RefCntblBaseVirtualDtorChecker
             }
             return (*hasDerefInBase) != nullptr;
           };
-          CXXBasePaths Paths;
-          Paths.setOrigin(C);
-          hasRef = hasRef || C->lookupInBases(hasPublicRefInBase, Paths,
+      CXXBasePaths Paths;
+      Paths.setOrigin(C);
+      hasRef = hasRef || C->lookupInBases(hasPublicRefInBase, Paths,
+                                          /*LookupInDependent =*/true);
+      hasDeref = hasDeref || C->lookupInBases(hasPublicDerefInBase, Paths,
                                               /*LookupInDependent =*/true);
-          hasDeref = hasDeref || C->lookupInBases(hasPublicDerefInBase, Paths,
-                                                  /*LookupInDependent =*/true);
-          if (AnyInconclusiveBase || !hasRef || !hasDeref)
-            return false;
-
-          const auto *Dtor = C->getDestructor();
-          if (!Dtor || !Dtor->isVirtual()) {
-            ProblematicBaseSpecifier = Base;
-            ProblematicBaseClass = C;
-            return true;
-          }
-
-          return false;
-        };
-
-    if (RD->lookupInBases(IsPublicBaseRefCntblWOVirtualDtor, Paths,
-                          /*LookupInDependent =*/true)) {
-      reportBug(RD, ProblematicBaseSpecifier, ProblematicBaseClass);
+      if (AnyInconclusiveBase || !hasRef || !hasDeref)
+        continue;
+
+      auto HasSpecializedDelete = isClassWithSpecializedDelete(C, RD);
+      if (!HasSpecializedDelete || *HasSpecializedDelete)
+        continue;
+      if (C->lookupInBases(
+              [&](const CXXBaseSpecifier *Base, CXXBasePath &) {
+                auto *T = Base->getType().getTypePtrOrNull();
+                if (!T)
+                  return false;
+                auto *R = T->getAsCXXRecordDecl();
+                if (!R)
+                  return false;
+                auto Result = isClassWithSpecializedDelete(R, RD);
+                if (!Result)
+                  AnyInconclusiveBase = true;
+                return Result && *Result;
+              },
+              Paths, /*LookupInDependent =*/true))
+        continue;
+      if (AnyInconclusiveBase)
+        continue;
+
+      const auto *Dtor = C->getDestructor();
+      if (!Dtor || !Dtor->isVirtual()) {
+        auto *ProblematicBaseSpecifier = &Base;
+        auto *ProblematicBaseClass = C;
+        reportBug(RD, ProblematicBaseSpecifier, ProblematicBaseClass);
+      }
     }
   }
 
@@ -182,6 +345,32 @@ class RefCntblBaseVirtualDtorChecker
             ClsName == "ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr");
   }
 
+  static std::optional<bool>
+  isClassWithSpecializedDelete(const CXXRecordDecl *C,
+                               const CXXRecordDecl *DerivedClass) {
+    if (auto *ClsTmplSpDecl = dyn_cast<ClassTemplateSpecializationDecl>(C)) {
+      for (auto *MethodDecl : C->methods()) {
+        if (safeGetName(MethodDecl) == "deref") {
+          DerefAnalysisVisitor DerefAnalysis(ClsTmplSpDecl->getTemplateArgs(),
+                                             DerivedClass);
+          auto Result = DerefAnalysis.HasSpecializedDelete(MethodDecl);
+          if (!Result || *Result)
+            return Result;
+        }
+      }
+      return false;
+    }
+    for (auto *MethodDecl : C->methods()) {
+      if (safeGetName(MethodDecl) == "deref") {
+        DerefAnalysisVisitor DerefAnalysis(DerivedClass);
+        auto Result = DerefAnalysis.HasSpecializedDelete(MethodDecl);
+        if (!Result || *Result)
+          return Result;
+      }
+    }
+    return false;
+  }
+
   void reportBug(const CXXRecordDecl *DerivedClass,
                  const CXXBaseSpecifier *BaseSpec,
                  const CXXRecordDecl *ProblematicBaseClass) const {
diff --git 
a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-ref-deref-on-diff-classes.cpp
 
b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-ref-deref-on-diff-classes.cpp
index aac58c0c1dda6..85108bccfee71 100644
--- 
a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-ref-deref-on-diff-classes.cpp
+++ 
b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-ref-deref-on-diff-classes.cpp
@@ -19,4 +19,5 @@ struct Derived : Base { };
 
 void foo () {
   Derived d;
+  d.deref();
 }
diff --git 
a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp 
b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
index eeb62d5d89ec4..4fc1624d7a154 100644
--- 
a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
+++ 
b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
@@ -10,8 +10,7 @@ struct DerivedClassTmpl1 : T { };
 // expected-warning@-1{{Struct 'RefCntblBase' is used as a base of struct 
'DerivedClassTmpl1<RefCntblBase>' but doesn't have virtual destructor}}
 
 DerivedClassTmpl1<RefCntblBase> a;
-
-
+void foo(DerivedClassTmpl1<RefCntblBase>& obj) { obj.deref(); }
 
 template<class T>
 struct DerivedClassTmpl2 : T { };
@@ -21,7 +20,6 @@ template<class T> int foo(T) { DerivedClassTmpl2<T> f; return 
42; }
 int b = foo(RefCntblBase{});
 
 
-
 template<class T>
 struct DerivedClassTmpl3 : T { };
 // expected-warning@-1{{Struct 'RefCntblBase' is used as a base of struct 
'DerivedClassTmpl3<RefCntblBase>' but doesn't have virtual destructor}}
@@ -29,7 +27,6 @@ struct DerivedClassTmpl3 : T { };
 typedef DerivedClassTmpl3<RefCntblBase> Foo;
 Foo c;
 
-
 namespace WTF {
 
 class RefCountedBase {
@@ -58,33 +55,344 @@ class RefCounted : public RefCountedBase {
   RefCounted() { }
 };
 
+template <typename X, typename T>
+class ExoticRefCounted : public RefCountedBase {
+public:
+  void deref() const {
+    if (derefBase())
+      delete (const_cast<T*>(static_cast<const T*>(this)));
+  }
+};
+
+template <typename X, typename T>
+class BadBase : RefCountedBase {
+public:
+  void deref() const {
+    if (derefBase())
+      delete (const_cast<X*>(static_cast<const X*>(this)));
+  }
+};
+
+template <typename T>
+class FancyDeref {
+public:
+  void ref() const
+  {
+    ++refCount;
+  }
+
+  void deref() const
+  {
+    --refCount;
+    if (refCount)
+      return;
+    auto deleteThis = [this] {
+      delete static_cast<const T*>(this);
+    };
+    deleteThis();
+  }
+private:
+  mutable unsigned refCount { 0 };
+};
+
+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 { return m_callable(in...); }
+  private:
+    CallableType m_callable;
+  };
+
+} // namespace Detail
+
+template<typename> class Function;
+
+template <typename Out, typename... In>
+class Function<Out(In...)> {
+public:
+  using Impl = Detail::CallableWrapperBase<Out, In...>;
+
+  Function() = default;
+
+  template<typename CallableType>
+  Function(CallableType&& callable)
+      : m_callableWrapper(new Detail::CallableWrapper<CallableType, Out, 
In...>>(callable)) { }
+
+  template<typename FunctionType>
+  Function(FunctionType f)
+      : m_callableWrapper(new Detail::CallableWrapper<FunctionType, Out, 
In...>>(f)) { }
+
+  ~Function() {
+  }
+
+  Out operator()(In... in) const {
+      ASSERT(m_callableWrapper);
+      return m_callableWrapper->call(in...);
+  }
+
+  explicit operator bool() const { return !!m_callableWrapper; }
+
+private:
+  Impl* m_callableWrapper;
+};
+
+void ensureOnMainThread(const Function<void()>&& function);
+
+enum class DestructionThread { Any, MainThread };
+
+template <typename T, DestructionThread destructionThread = 
DestructionThread::Any>
+class FancyDeref2 {
+public:
+  void ref() const
+  {
+    ++refCount;
+  }
+
+  void deref() const
+  {
+    --refCount;
+    if (refCount)
+      return;
+    const_cast<FancyDeref2<T, destructionThread>*>(this)->destroy();
+  }
+
+private:
+  void destroy() {
+    delete static_cast<T*>(this);
+  }
+  mutable unsigned refCount { 0 };
+};
+
+template <typename S>
+class DerivedFancyDeref2 : public FancyDeref2<S> {
+};
+
+template <typename T>
+class BadFancyDeref {
+public:
+  void ref() const
+  {
+    ++refCount;
+  }
+
+  void deref() const
+  {
+    --refCount;
+    if (refCount)
+      return;
+    auto deleteThis = [this] {
+      delete static_cast<const T*>(this);
+    };
+    delete this;
+  }
+private:
+  mutable unsigned refCount { 0 };
+};
+
 template <typename T>
 class ThreadSafeRefCounted {
 public:
-  void ref() const;
-  bool deref() const;
+  void ref() const { ++refCount; }
+  void deref() const {
+    if (!--refCount)
+      delete const_cast<T*>(static_cast<const T*>(this));
+  }
+private:
+  mutable unsigned refCount { 0 };
 };
 
 template <typename T>
 class ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr {
 public:
-  void ref() const;
-  bool deref() const;
+  void ref() const { ++refCount; }
+  void deref() const {
+    if (!--refCount)
+      delete const_cast<T*>(static_cast<const T*>(this));
+  }
+private:
+  mutable unsigned refCount { 0 };
 };
 
 } // namespace WTF
 
 class DerivedClass4 : public WTF::RefCounted<DerivedClass4> { };
 
+class DerivedClass4b : public WTF::ExoticRefCounted<int, DerivedClass4b> { };
+
+class DerivedClass4cSub;
+class DerivedClass4c : public WTF::BadBase<DerivedClass4cSub, DerivedClass4c> 
{ };
+// expected-warning@-1{{Class 'WTF::BadBase<DerivedClass4cSub, 
DerivedClass4c>' is used as a base of class 'DerivedClass4c' but doesn't have 
virtual destructor}}
+class DerivedClass4cSub : public DerivedClass4c { };
+void UseDerivedClass4c(DerivedClass4c &obj) { obj.deref(); }
+
+class DerivedClass4d : public WTF::RefCounted<DerivedClass4d> {
+public:
+  virtual ~DerivedClass4d() { }
+};
+class DerivedClass4dSub : public DerivedClass4d { };
+
 class DerivedClass5 : public DerivedClass4 { };
 // expected-warning@-1{{Class 'DerivedClass4' is used as a base of class 
'DerivedClass5' but doesn't have virtual destructor}}
+void UseDerivedClass5(DerivedClass5 &obj) { obj.deref(); }
 
 class DerivedClass6 : public WTF::ThreadSafeRefCounted<DerivedClass6> { };
+void UseDerivedClass6(DerivedClass6 &obj) { obj.deref(); }
 
 class DerivedClass7 : public DerivedClass6 { };
 // expected-warning@-1{{Class 'DerivedClass6' is used as a base of class 
'DerivedClass7' but doesn't have virtual destructor}}
+void UseDerivedClass7(DerivedClass7 &obj) { obj.deref(); }
 
 class DerivedClass8 : public 
WTF::ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<DerivedClass8> { };
+void UseDerivedClass8(DerivedClass8 &obj) { obj.deref(); }
 
 class DerivedClass9 : public DerivedClass8 { };
 // expected-warning@-1{{Class 'DerivedClass8' is used as a base of class 
'DerivedClass9' but doesn't have virtual destructor}}
+void UseDerivedClass9(DerivedClass9 &obj) { obj.deref(); }
+
+class DerivedClass10 : public WTF::FancyDeref<DerivedClass10> { };
+void UseDerivedClass10(DerivedClass10 &obj) { obj.deref(); }
+
+class DerivedClass10b : public WTF::DerivedFancyDeref2<DerivedClass10b> { };
+void UseDerivedClass10b(DerivedClass10b &obj) { obj.deref(); }
+
+class DerivedClass10c : public WTF::BadFancyDeref<DerivedClass10c> { };
+// expected-warning@-1{{Class 'WTF::BadFancyDeref<DerivedClass10c>' is used as 
a base of class 'DerivedClass10c' but doesn't have virtual destructor}}
+void UseDerivedClass10c(DerivedClass10c &obj) { obj.deref(); }
+
+class BaseClass1 {
+public:
+  void ref() const { ++refCount; }
+  void deref() const;
+private:
+  enum class Type { Base, Derived } type { Type::Base };
+  mutable unsigned refCount { 0 };
+};
+
+class DerivedClass11 : public BaseClass1 { };
+
+void BaseClass1::deref() const
+{
+  --refCount;
+  if (refCount)
+    return;
+  switch (type) {
+  case Type::Base:
+    delete const_cast<BaseClass1*>(this);
+    break;
+  case Type::Derived:
+    delete const_cast<DerivedClass11*>(static_cast<const 
DerivedClass11*>(this));
+    break;
+  }
+}
+
+void UseDerivedClass11(DerivedClass11& obj) { obj.deref(); }
+
+class BaseClass2;
+static void deleteBase2(BaseClass2*);
+
+class BaseClass2 {
+public:
+  void ref() const { ++refCount; }
+  void deref() const
+  {
+    if (!--refCount)
+      deleteBase2(const_cast<BaseClass2*>(this));
+  }
+  virtual bool isDerived() { return false; }
+private:
+  mutable unsigned refCount { 0 };
+};
+
+class DerivedClass12 : public BaseClass2 {
+  bool isDerived() final { return true; }
+};
+
+void UseDerivedClass11(DerivedClass12& obj) { obj.deref(); }
+
+void deleteBase2(BaseClass2* obj) {
+  if (obj->isDerived())
+    delete static_cast<DerivedClass12*>(obj);
+  else
+    delete obj;
+}
+
+class BaseClass3 {
+public:
+  void ref() const { ++refCount; }
+  void deref() const
+  {
+    if (!--refCount)
+      const_cast<BaseClass3*>(this)->destory();
+  }
+  virtual bool isDerived() { return false; }
+
+private:
+  void destory();
+
+  mutable unsigned refCount { 0 };
+};
+
+class DerivedClass13 : public BaseClass3 {
+  bool isDerived() final { return true; }
+};
+
+void UseDerivedClass11(DerivedClass13& obj) { obj.deref(); }
+
+void BaseClass3::destory() {
+  if (isDerived())
+    delete static_cast<DerivedClass13*>(this);
+  else
+    delete this;
+}
+
+class RecursiveBaseClass {
+public:
+  void ref() const {
+    if (otherObject)
+      otherObject->ref();
+    else
+      ++refCount;
+  }
+  void deref() const {
+    if (otherObject)
+      otherObject->deref();
+    else {
+      --refCount;
+      if (refCount)
+        return;
+      delete this;
+    }
+  }
+private:
+  RecursiveBaseClass* otherObject { nullptr };
+  mutable unsigned refCount { 0 };
+};
+
+class RecursiveDerivedClass : public RecursiveBaseClass { };
+// expected-warning@-1{{Class 'RecursiveBaseClass' is used as a base of class 
'RecursiveDerivedClass' but doesn't have virtual destructor}}
+
+class DerivedClass14 : public WTF::RefCounted<DerivedClass14> {
+public:
+  virtual ~DerivedClass14() { }
+};
+
+void UseDerivedClass14(DerivedClass14& obj) { obj.deref(); }
+
+class DerivedClass15 : public DerivedClass14 { };
+
+void UseDerivedClass15(DerivedClass15& obj) { obj.deref(); }

>From 69103682e29b58b09da7773e205875b9c75f830e Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rn...@apple.com>
Date: Thu, 23 May 2024 22:55:37 -0700
Subject: [PATCH 2/2] Address review comments.

---
 .../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 49 ++++++-------------
 1 file changed, 16 insertions(+), 33 deletions(-)

diff --git 
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index efb7b4456f2ca..4547ffd800b6d 100644
--- 
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++ 
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -25,8 +25,8 @@ using namespace ento;
 
 namespace {
 
-class DerefAnalysisVisitor
-    : public ConstStmtVisitor<DerefAnalysisVisitor, bool> {
+class DerefFuncDeleteExprVisitor
+    : public ConstStmtVisitor<DerefFuncDeleteExprVisitor, bool> {
   // Returns true if any of child statements return true.
   bool VisitChildren(const Stmt *S) {
     for (const Stmt *Child : S->children()) {
@@ -48,11 +48,12 @@ class DerefAnalysisVisitor
   }
 
 public:
-  DerefAnalysisVisitor(const TemplateArgumentList &ArgList,
-                       const CXXRecordDecl *ClassDecl)
+  DerefFuncDeleteExprVisitor(const TemplateArgumentList &ArgList,
+                             const CXXRecordDecl *ClassDecl)
       : ArgList(&ArgList), ClassDecl(ClassDecl) {}
 
-  DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl) : ClassDecl(ClassDecl) 
{}
+  DerefFuncDeleteExprVisitor(const CXXRecordDecl *ClassDecl)
+      : ClassDecl(ClassDecl) {}
 
   std::optional<bool> HasSpecializedDelete(CXXMethodDecl *Decl) {
     if (auto *Body = Decl->getBody())
@@ -63,29 +64,12 @@ class DerefAnalysisVisitor
   }
 
   bool VisitCallExpr(const CallExpr *CE) {
-    auto *Callee = CE->getCallee();
-    while (auto *Expr = dyn_cast<CastExpr>(Callee))
-      Callee = Expr->getSubExpr();
-    if (auto *DeclRef = dyn_cast<DeclRefExpr>(Callee)) {
-      auto *Decl = DeclRef->getDecl();
-      if (auto *VD = dyn_cast<VarDecl>(Decl)) {
-        if (auto *Init = VD->getInit()) {
-          if (auto *Lambda = dyn_cast<LambdaExpr>(Init))
-            return VisitBody(Lambda->getBody());
-        }
-      } else if (auto *FD = dyn_cast<FunctionDecl>(Decl))
-        return VisitBody(FD->getBody());
-    }
+    const Decl *D = CE->getCalleeDecl();
+    if (D && D->hasBody())
+      return VisitBody(D->getBody());
     return false;
   }
 
-  bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) {
-    auto *Callee = MCE->getMethodDecl();
-    if (!Callee)
-      return false;
-    return VisitBody(Callee->getBody());
-  }
-
   bool VisitCXXDeleteExpr(const CXXDeleteExpr *E) {
     auto *Arg = E->getArgument();
     while (Arg) {
@@ -189,11 +173,10 @@ class RefCntblBaseVirtualDtorChecker
             continue;
 
           if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(C)) {
-            auto &Args = CTSD->getTemplateArgs();
-            for (unsigned i = 0; i < Args.size(); ++i) {
-              if (Args[i].getKind() != TemplateArgument::Type)
+            for (auto& Arg : CTSD->getTemplateArgs().asArray()) {
+              if (Arg.getKind() != TemplateArgument::Type)
                 continue;
-              auto TemplT = Args[i].getAsType();
+              auto TemplT = Arg.getAsType();
               if (TemplT.isNull())
                 continue;
 
@@ -351,9 +334,9 @@ class RefCntblBaseVirtualDtorChecker
     if (auto *ClsTmplSpDecl = dyn_cast<ClassTemplateSpecializationDecl>(C)) {
       for (auto *MethodDecl : C->methods()) {
         if (safeGetName(MethodDecl) == "deref") {
-          DerefAnalysisVisitor DerefAnalysis(ClsTmplSpDecl->getTemplateArgs(),
+          DerefFuncDeleteExprVisitor Visitor(ClsTmplSpDecl->getTemplateArgs(),
                                              DerivedClass);
-          auto Result = DerefAnalysis.HasSpecializedDelete(MethodDecl);
+          auto Result = Visitor.HasSpecializedDelete(MethodDecl);
           if (!Result || *Result)
             return Result;
         }
@@ -362,8 +345,8 @@ class RefCntblBaseVirtualDtorChecker
     }
     for (auto *MethodDecl : C->methods()) {
       if (safeGetName(MethodDecl) == "deref") {
-        DerefAnalysisVisitor DerefAnalysis(DerivedClass);
-        auto Result = DerefAnalysis.HasSpecializedDelete(MethodDecl);
+        DerefFuncDeleteExprVisitor Visitor(DerivedClass);
+        auto Result = Visitor.HasSpecializedDelete(MethodDecl);
         if (!Result || *Result)
           return Result;
       }

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to