[clang] Fix WebKit static analyzers to support ref and deref methods to be defined on different classes. (PR #69985)
Ryosuke Niwa via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 23 16:38:50 PDT 2023
https://github.com/rniwa created https://github.com/llvm/llvm-project/pull/69985
None
>From d8236358e137967dbbd63606b7d43983709597e8 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Mon, 23 Oct 2023 16:38:10 -0700
Subject: [PATCH] Fix WebKit static analyzers to support ref and deref methods
to be defined on different classes.
---
.../Checkers/WebKit/PtrTypesSemantics.cpp | 61 ++++++++++---------
.../Checkers/WebKit/PtrTypesSemantics.h | 8 +--
.../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 47 ++++++++++++--
...virtual-dtor-ref-deref-on-diff-classes.cpp | 22 +++++++
...nted-members-ref-deref-on-diff-classes.cpp | 23 +++++++
5 files changed, 125 insertions(+), 36 deletions(-)
create mode 100644 clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-ref-deref-on-diff-classes.cpp
create mode 100644 clang/test/Analysis/Checkers/WebKit/uncounted-members-ref-deref-on-diff-classes.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index c1f180f31338cb3..8c7eaa3b9a5369d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -18,24 +18,14 @@ using namespace clang;
namespace {
-bool hasPublicRefAndDeref(const CXXRecordDecl *R) {
+bool hasPublicMethodInBaseClass(const CXXRecordDecl *R, const char* NameToMatch) {
assert(R);
assert(R->hasDefinition());
- bool hasRef = false;
- bool hasDeref = false;
for (const CXXMethodDecl *MD : R->methods()) {
const auto MethodName = safeGetName(MD);
-
- if (MethodName == "ref" && MD->getAccess() == AS_public) {
- if (hasDeref)
- return true;
- hasRef = true;
- } else if (MethodName == "deref" && MD->getAccess() == AS_public) {
- if (hasRef)
- return true;
- hasDeref = true;
- }
+ if (MethodName == NameToMatch && MD->getAccess() == AS_public)
+ return true;
}
return false;
}
@@ -44,9 +34,8 @@ bool hasPublicRefAndDeref(const CXXRecordDecl *R) {
namespace clang {
-std::optional<const clang::CXXRecordDecl*>
-isRefCountable(const CXXBaseSpecifier* Base)
-{
+std::optional<const clang::CXXRecordDecl *>
+hasPublicMethodInBase(const CXXBaseSpecifier *Base, const char *NameToMatch) {
assert(Base);
const Type *T = Base->getType().getTypePtrOrNull();
@@ -59,7 +48,7 @@ isRefCountable(const CXXBaseSpecifier* Base)
if (!R->hasDefinition())
return std::nullopt;
- return hasPublicRefAndDeref(R) ? R : nullptr;
+ return hasPublicMethodInBaseClass(R, NameToMatch) ? R : nullptr;
}
std::optional<bool> isRefCountable(const CXXRecordDecl* R)
@@ -70,29 +59,45 @@ std::optional<bool> isRefCountable(const CXXRecordDecl* R)
if (!R)
return std::nullopt;
- if (hasPublicRefAndDeref(R))
+ bool hasRef = hasPublicMethodInBaseClass(R, "ref");
+ bool hasDeref = hasPublicMethodInBaseClass(R, "deref");
+ if (hasRef && hasDeref)
return true;
CXXBasePaths Paths;
Paths.setOrigin(const_cast<CXXRecordDecl *>(R));
bool AnyInconclusiveBase = false;
- const auto isRefCountableBase =
- [&AnyInconclusiveBase](const CXXBaseSpecifier* Base, CXXBasePath&) {
- std::optional<const clang::CXXRecordDecl*> IsRefCountable = clang::isRefCountable(Base);
- if (!IsRefCountable) {
- AnyInconclusiveBase = true;
- return false;
- }
- return (*IsRefCountable) != nullptr;
+ const auto hasPublicRefInBase =
+ [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
+ auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref");
+ if (!hasRefInBase) {
+ AnyInconclusiveBase = true;
+ return false;
+ }
+ return (*hasRefInBase) != nullptr;
};
- bool BasesResult = R->lookupInBases(isRefCountableBase, Paths,
+ hasRef = hasRef || R->lookupInBases(hasPublicRefInBase, Paths,
/*LookupInDependent =*/true);
if (AnyInconclusiveBase)
return std::nullopt;
- return BasesResult;
+ const auto hasPublicDerefInBase =
+ [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
+ auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref");
+ if (!hasDerefInBase) {
+ AnyInconclusiveBase = true;
+ return false;
+ }
+ return (*hasDerefInBase) != nullptr;
+ };
+ hasDeref = hasDeref || R->lookupInBases(hasPublicDerefInBase, Paths,
+ /*LookupInDependent =*/true);
+ if (AnyInconclusiveBase)
+ return std::nullopt;
+
+ return hasRef && hasDeref;
}
bool isCtorOfRefCounted(const clang::FunctionDecl *F) {
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
index 91e3ccf2ec3047e..45b21cc0918443b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
@@ -26,10 +26,10 @@ class Type;
// In WebKit there are two ref-counted templated smart pointers: RefPtr<T> and
// Ref<T>.
-/// \returns CXXRecordDecl of the base if the type is ref-countable, nullptr if
-/// not, std::nullopt if inconclusive.
-std::optional<const clang::CXXRecordDecl*>
-isRefCountable(const clang::CXXBaseSpecifier* Base);
+/// \returns CXXRecordDecl of the base if the type has ref as a public method,
+/// nullptr if not, std::nullopt if inconclusive.
+std::optional<const clang::CXXRecordDecl *>
+hasPublicMethodInBase(const CXXBaseSpecifier *Base, const char *NameToMatch);
/// \returns true if \p Class is ref-countable, false if not, std::nullopt if
/// inconclusive.
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index 48dcfc4a3c4645d..bf54bb40c23c01c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -77,14 +77,53 @@ class RefCntblBaseVirtualDtorChecker
(AccSpec == AS_none && RD->isClass()))
return false;
- std::optional<const CXXRecordDecl*> RefCntblBaseRD = isRefCountable(Base);
- if (!RefCntblBaseRD || !(*RefCntblBaseRD))
+ auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref");
+ auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref");
+
+ bool hasRef = hasRefInBase && *hasRefInBase != nullptr;
+ bool hasDeref = hasDerefInBase && *hasDerefInBase != nullptr;
+
+ const Type *T = Base->getType().getTypePtrOrNull();
+ if (!T)
+ return false;
+
+ const CXXRecordDecl *C = T->getAsCXXRecordDecl();
+ if (!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 &) {
+ auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref");
+ if (!hasDerefInBase) {
+ AnyInconclusiveBase = true;
+ return false;
+ }
+ return (*hasDerefInBase) != nullptr;
+ };
+ CXXBasePaths Paths;
+ Paths.setOrigin(C);
+ hasRef = hasRef || C->lookupInBases(hasPublicRefInBase, Paths,
+ /*LookupInDependent =*/true);
+ hasDeref = hasDeref || C->lookupInBases(hasPublicDerefInBase, Paths,
+ /*LookupInDependent =*/true);
+ if (AnyInconclusiveBase || !hasRef || !hasDeref)
return false;
- const auto *Dtor = (*RefCntblBaseRD)->getDestructor();
+ const auto *Dtor = C->getDestructor();
if (!Dtor || !Dtor->isVirtual()) {
ProblematicBaseSpecifier = Base;
- ProblematicBaseClass = *RefCntblBaseRD;
+ ProblematicBaseClass = C;
return true;
}
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
new file mode 100644
index 000000000000000..aac58c0c1dda68e
--- /dev/null
+++ b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-ref-deref-on-diff-classes.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.RefCntblBaseVirtualDtor -verify %s
+
+struct RefCountedBase {
+ void ref() {}
+};
+
+template<typename T> struct RefCounted : RefCountedBase {
+public:
+ void deref() const { }
+};
+
+struct Base : RefCounted<Base> {
+// expected-warning at -1{{Struct 'RefCounted<Base>' is used as a base of struct 'Base' but doesn't have virtual destructor}}
+ virtual void foo() { }
+};
+
+struct Derived : Base { };
+// expected-warning at -1{{Struct 'Base' is used as a base of struct 'Derived' but doesn't have virtual destructor}}
+
+void foo () {
+ Derived d;
+}
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-members-ref-deref-on-diff-classes.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-members-ref-deref-on-diff-classes.cpp
new file mode 100644
index 000000000000000..4198b2388fed8f5
--- /dev/null
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-members-ref-deref-on-diff-classes.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.NoUncountedMemberChecker -verify %s
+
+#include "mock-types.h"
+
+class RefCountedBase {
+public:
+ void ref() const { }
+};
+
+template<typename T> class RefCounted : public RefCountedBase {
+public:
+ virtual ~RefCounted() { }
+ void deref() const { }
+};
+
+class TreeNode : public RefCounted<TreeNode> {
+public:
+ void setParent(TreeNode& parent) { m_parent = &parent; }
+
+private:
+ TreeNode* m_parent;
+// expected-warning at -1{{Member variable 'm_parent' in 'TreeNode' is a raw pointer to ref-countable type 'TreeNode'}}
+};
More information about the cfe-commits
mailing list