[clang] [analyzer] WebKit checkers: support ref and deref defined on different classes. (PR #68170)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 3 18:24:50 PDT 2023
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
<details>
<summary>Changes</summary>
Patch by Ryosuke Niwa!
---
Full diff: https://github.com/llvm/llvm-project/pull/68170.diff
5 Files Affected:
- (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp (+62-28)
- (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h (+9-4)
- (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp (+43-4)
- (added) clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-ref-deref-on-diff-classes.cpp (+22)
- (added) clang/test/Analysis/Checkers/WebKit/uncounted-members-ref-deref-on-diff-classes.cpp (+23)
``````````diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index 9b1d7ae3e6a320c..a66fa38315f4d11 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -18,24 +18,26 @@ using namespace clang;
namespace {
-bool hasPublicRefAndDeref(const CXXRecordDecl *R) {
+bool hasPublicRefMethod(const CXXRecordDecl *R) {
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)
+ return true;
+ }
+ return false;
+}
- 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;
- }
+bool hasPublicDerefMethod(const CXXRecordDecl *R) {
+ assert(R);
+ assert(R->hasDefinition());
+
+ for (const CXXMethodDecl *MD : R->methods()) {
+ const auto MethodName = safeGetName(MD);
+ if (MethodName == "deref" && MD->getAccess() == AS_public)
+ return true;
}
return false;
}
@@ -44,9 +46,25 @@ bool hasPublicRefAndDeref(const CXXRecordDecl *R) {
namespace clang {
-std::optional<const clang::CXXRecordDecl*>
-isRefCountable(const CXXBaseSpecifier* Base)
-{
+std::optional<const clang::CXXRecordDecl *>
+hasPublicRefInBase(const CXXBaseSpecifier *Base) {
+ assert(Base);
+
+ const Type *T = Base->getType().getTypePtrOrNull();
+ if (!T)
+ return std::nullopt;
+
+ const CXXRecordDecl *R = T->getAsCXXRecordDecl();
+ if (!R)
+ return std::nullopt;
+ if (!R->hasDefinition())
+ return std::nullopt;
+
+ return hasPublicRefMethod(R) ? R : nullptr;
+}
+
+std::optional<const clang::CXXRecordDecl *>
+hasPublicDerefInBase(const CXXBaseSpecifier *Base) {
assert(Base);
const Type *T = Base->getType().getTypePtrOrNull();
@@ -59,7 +77,7 @@ isRefCountable(const CXXBaseSpecifier* Base)
if (!R->hasDefinition())
return std::nullopt;
- return hasPublicRefAndDeref(R) ? R : nullptr;
+ return hasPublicDerefMethod(R) ? R : nullptr;
}
std::optional<bool> isRefCountable(const CXXRecordDecl* R)
@@ -70,29 +88,45 @@ std::optional<bool> isRefCountable(const CXXRecordDecl* R)
if (!R)
return std::nullopt;
- if (hasPublicRefAndDeref(R))
+ bool hasRef = hasPublicRefMethod(R);
+ bool hasDeref = hasPublicDerefMethod(R);
+ 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::hasPublicRefInBase(Base);
+ if (!hasRefInBase) {
+ AnyInconclusiveBase = true;
+ return false;
+ }
+ return (*hasRefInBase) != nullptr;
};
- bool BasesResult = R->lookupInBases(isRefCountableBase, Paths,
- /*LookupInDependent =*/true);
+ hasRef =
+ R->lookupInBases(hasPublicRefInBase, Paths, /*LookupInDependent =*/true);
+ if (AnyInconclusiveBase)
+ return std::nullopt;
+
+ const auto hasPublicDerefInBase =
+ [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
+ auto hasDerefInBase = clang::hasPublicDerefInBase(Base);
+ if (!hasDerefInBase) {
+ AnyInconclusiveBase = true;
+ return false;
+ }
+ return (*hasDerefInBase) != nullptr;
+ };
+ hasDeref = R->lookupInBases(hasPublicDerefInBase, Paths,
+ /*LookupInDependent =*/true);
if (AnyInconclusiveBase)
return std::nullopt;
- return BasesResult;
+ 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..9c7a933b9ee91d9 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
@@ -26,10 +26,15 @@ 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 *>
+hasPublicRefInBase(const CXXBaseSpecifier *Base);
+
+/// \returns CXXRecordDecl of the base if the type has deref as a public
+/// method, nullptr if not, std::nullopt if inconclusive.
+std::optional<const clang::CXXRecordDecl *>
+hasPublicDerefInBase(const CXXBaseSpecifier *Base);
/// \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..6b8b952c82748eb 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::hasPublicRefInBase(Base);
+ auto hasDerefInBase = clang::hasPublicDerefInBase(Base);
+
+ 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::hasPublicRefInBase(Base);
+ if (!hasRefInBase) {
+ AnyInconclusiveBase = true;
+ return false;
+ }
+ return (*hasRefInBase) != nullptr;
+ };
+ const auto hasPublicDerefInBase =
+ [&AnyInconclusiveBase](const CXXBaseSpecifier *Base,
+ CXXBasePath &) {
+ auto hasDerefInBase = clang::hasPublicDerefInBase(Base);
+ 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'}}
+};
``````````
</details>
https://github.com/llvm/llvm-project/pull/68170
More information about the cfe-commits
mailing list