Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[webkit.RefCntblBaseVirtualDtor] Ignore WTF::RefCounted<T> and its variants missing virtual destructor #91009

Merged

Conversation

rniwa
Copy link
Contributor

@rniwa rniwa commented May 3, 2024

No description provided.

@rniwa rniwa requested a review from haoNoQ May 3, 2024 20:37
Copy link

github-actions bot commented May 4, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link

github-actions bot commented May 4, 2024

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 37f6ba4fb2db2c78cda7d0a69cd0a2eff2b924e3 a9eb73de2ee7d2eadb742498bc0efb651e0b4d9a -- clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
View the diff from clang-format here.
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index e4d311851d..7f4c3a7b78 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -6,8 +6,8 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "DiagOutputUtils.h"
 #include "ASTUtils.h"
+#include "DiagOutputUtils.h"
 #include "PtrTypesSemantics.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/RecursiveASTVisitor.h"

@rniwa rniwa force-pushed the ignore-lack-of-virtual-destructor-on-ref-counted branch from 78fd7d7 to 3cec263 Compare May 5, 2024 00:44
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels May 5, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented May 5, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Ryosuke Niwa (rniwa)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/91009.diff

2 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp (+18)
  • (modified) clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp (+60)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index d879c110b75d33..7f4c3a7b787e88 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -6,6 +6,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "ASTUtils.h"
 #include "DiagOutputUtils.h"
 #include "PtrTypesSemantics.h"
 #include "clang/AST/CXXInheritance.h"
@@ -90,6 +91,9 @@ class RefCntblBaseVirtualDtorChecker
           const CXXRecordDecl *C = T->getAsCXXRecordDecl();
           if (!C)
             return false;
+          if (isRefCountedClass(C))
+            return false;
+
           bool AnyInconclusiveBase = false;
           const auto hasPublicRefInBase =
               [&AnyInconclusiveBase](const CXXBaseSpecifier *Base,
@@ -164,6 +168,20 @@ class RefCntblBaseVirtualDtorChecker
     return false;
   }
 
+  static bool isRefCountedClass(const CXXRecordDecl *D) {
+    if (!D->getTemplateInstantiationPattern())
+      return false;
+    auto *NsDecl = D->getParent();
+    if (!NsDecl || !isa<NamespaceDecl>(NsDecl))
+      return false;
+    auto NamespaceName = safeGetName(NsDecl);
+    auto ClsNameStr = safeGetName(D);
+    StringRef ClsName = ClsNameStr; // FIXME: Make safeGetName return StringRef.
+    return NamespaceName == "WTF" &&
+           (ClsName.ends_with("RefCounted") ||
+            ClsName == "ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr");
+  }
+
   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-templates.cpp b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
index 3338fa9368e4b5..eeb62d5d89ec41 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
@@ -28,3 +28,63 @@ struct DerivedClassTmpl3 : T { };
 
 typedef DerivedClassTmpl3<RefCntblBase> Foo;
 Foo c;
+
+
+namespace WTF {
+
+class RefCountedBase {
+public:
+  void ref() const { ++count; }
+
+protected:
+  bool derefBase() const
+  {
+    return !--count;
+  }
+
+private:
+  mutable unsigned count;
+};
+
+template <typename T>
+class RefCounted : public RefCountedBase {
+public:
+  void deref() const {
+    if (derefBase())
+      delete const_cast<T*>(static_cast<const T*>(this));
+  }
+
+protected:
+  RefCounted() { }
+};
+
+template <typename T>
+class ThreadSafeRefCounted {
+public:
+  void ref() const;
+  bool deref() const;
+};
+
+template <typename T>
+class ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr {
+public:
+  void ref() const;
+  bool deref() const;
+};
+
+} // namespace WTF
+
+class DerivedClass4 : public WTF::RefCounted<DerivedClass4> { };
+
+class DerivedClass5 : public DerivedClass4 { };
+// expected-warning@-1{{Class 'DerivedClass4' is used as a base of class 'DerivedClass5' but doesn't have virtual destructor}}
+
+class DerivedClass6 : public WTF::ThreadSafeRefCounted<DerivedClass6> { };
+
+class DerivedClass7 : public DerivedClass6 { };
+// expected-warning@-1{{Class 'DerivedClass6' is used as a base of class 'DerivedClass7' but doesn't have virtual destructor}}
+
+class DerivedClass8 : public WTF::ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<DerivedClass8> { };
+
+class DerivedClass9 : public DerivedClass8 { };
+// expected-warning@-1{{Class 'DerivedClass8' is used as a base of class 'DerivedClass9' but doesn't have virtual destructor}}

Copy link
Collaborator

@haoNoQ haoNoQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rniwa rniwa merged commit 6d6693e into llvm:main May 6, 2024
5 of 6 checks passed
@rniwa rniwa deleted the ignore-lack-of-virtual-destructor-on-ref-counted branch May 6, 2024 23:01
@rniwa
Copy link
Contributor Author

rniwa commented May 6, 2024

Thanks for the reviews!

rniwa added a commit to rniwa/llvm-project that referenced this pull request May 25, 2024
rniwa added a commit to rniwa/llvm-project that referenced this pull request Sep 6, 2024
rniwa added a commit to rniwa/llvm-project that referenced this pull request Sep 11, 2024
rniwa added a commit to rniwa/llvm-project that referenced this pull request Sep 18, 2024
rniwa added a commit to rniwa/llvm-project that referenced this pull request Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:static analyzer clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants