[alpha.webkit.NoUncheckedPtrMemberChecker] Introduce member variable checker for CheckedPtr/CheckedRef #108352
+257
−46
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…checker for CheckedPtr/CheckedRef This PR introduces new WebKit checker to warn a member variable that is a raw reference or a raw pointer to an object, which is capable of creating a CheckedRef/CheckedPtr.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Ryosuke Niwa (rniwa) ChangesThis PR introduces new WebKit checker to warn a member variable that is a raw reference or a raw pointer to an object, which is capable of creating a CheckedRef/CheckedPtr. Full diff: https://github.com/llvm/llvm-project/pull/108352.diff 8 Files Affected:
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 585246547b3dce..4759f680fb4ff7 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1771,6 +1771,10 @@ def UncountedLambdaCapturesChecker : Checker<"UncountedLambdaCapturesChecker">,
let ParentPackage = WebKitAlpha in {
+def NoUncheckedPtrMemberChecker : Checker<"NoUncheckedPtrMemberChecker">,
+ HelpText<"Check for no unchecked member variables.">,
+ Documentation<NotDocumented>;
+
def UncountedCallArgsChecker : Checker<"UncountedCallArgsChecker">,
HelpText<"Check uncounted call arguments.">,
Documentation<HasDocumentation>;
diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 414282d58f779f..6da3665ab9a4df 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -132,7 +132,7 @@ add_clang_library(clangStaticAnalyzerCheckers
VLASizeChecker.cpp
ValistChecker.cpp
VirtualCallChecker.cpp
- WebKit/NoUncountedMembersChecker.cpp
+ WebKit/RawPtrRefMemberChecker.cpp
WebKit/ASTUtils.cpp
WebKit/PtrTypesSemantics.cpp
WebKit/RefCntblBaseVirtualDtorChecker.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index f48b2fd9dca71b..09298102993f99 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -53,7 +53,9 @@ hasPublicMethodInBase(const CXXBaseSpecifier *Base, const char *NameToMatch) {
return hasPublicMethodInBaseClass(R, NameToMatch) ? R : nullptr;
}
-std::optional<bool> isRefCountable(const CXXRecordDecl* R)
+std::optional<bool> isSmartPtrCompatible(const CXXRecordDecl *R,
+ const char *IncMethodName,
+ const char *DecMethodName)
{
assert(R);
@@ -61,8 +63,8 @@ std::optional<bool> isRefCountable(const CXXRecordDecl* R)
if (!R)
return std::nullopt;
- bool hasRef = hasPublicMethodInBaseClass(R, "ref");
- bool hasDeref = hasPublicMethodInBaseClass(R, "deref");
+ bool hasRef = hasPublicMethodInBaseClass(R, IncMethodName);
+ bool hasDeref = hasPublicMethodInBaseClass(R, DecMethodName);
if (hasRef && hasDeref)
return true;
@@ -71,8 +73,8 @@ std::optional<bool> isRefCountable(const CXXRecordDecl* R)
bool AnyInconclusiveBase = false;
const auto hasPublicRefInBase =
- [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
- auto hasRefInBase = clang::hasPublicMethodInBase(Base, "ref");
+ [&](const CXXBaseSpecifier *Base, CXXBasePath &) {
+ auto hasRefInBase = clang::hasPublicMethodInBase(Base, IncMethodName);
if (!hasRefInBase) {
AnyInconclusiveBase = true;
return false;
@@ -87,8 +89,8 @@ std::optional<bool> isRefCountable(const CXXRecordDecl* R)
Paths.clear();
const auto hasPublicDerefInBase =
- [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
- auto hasDerefInBase = clang::hasPublicMethodInBase(Base, "deref");
+ [&](const CXXBaseSpecifier *Base, CXXBasePath &) {
+ auto hasDerefInBase = clang::hasPublicMethodInBase(Base, DecMethodName);
if (!hasDerefInBase) {
AnyInconclusiveBase = true;
return false;
@@ -103,11 +105,23 @@ std::optional<bool> isRefCountable(const CXXRecordDecl* R)
return hasRef && hasDeref;
}
+std::optional<bool> isRefCountable(const clang::CXXRecordDecl *R) {
+ return isSmartPtrCompatible(R, "ref", "deref");
+}
+
+std::optional<bool> isCheckedPtrCapable(const clang::CXXRecordDecl *R) {
+ return isSmartPtrCompatible(R, "incrementPtrCount", "decrementPtrCount");
+}
+
bool isRefType(const std::string &Name) {
return Name == "Ref" || Name == "RefAllowingPartiallyDestroyed" ||
Name == "RefPtr" || Name == "RefPtrAllowingPartiallyDestroyed";
}
+bool isCheckedPtr(const std::string &Name) {
+ return Name == "CheckedPtr" || Name == "CheckedRef";
+}
+
bool isCtorOfRefCounted(const clang::FunctionDecl *F) {
assert(F);
const std::string &FunctionName = safeGetName(F);
@@ -218,6 +232,15 @@ bool isRefCounted(const CXXRecordDecl *R) {
return false;
}
+bool isCheckedPtr(const CXXRecordDecl *R) {
+ assert(R);
+ if (auto *TmplR = R->getTemplateInstantiationPattern()) {
+ const auto &ClassName = safeGetName(TmplR);
+ return isCheckedPtr(ClassName);
+ }
+ return false;
+}
+
bool isPtrConversion(const FunctionDecl *F) {
assert(F);
if (isCtorOfRefCounted(F))
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
index 2932e62ad06e4b..08f9be49970394 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
@@ -40,9 +40,16 @@ hasPublicMethodInBase(const CXXBaseSpecifier *Base, const char *NameToMatch);
/// inconclusive.
std::optional<bool> isRefCountable(const clang::CXXRecordDecl* Class);
+/// \returns true if \p Class is checked-pointer compatible, false if not,
+/// std::nullopt if inconclusive.
+std::optional<bool> isCheckedPtrCapable(const clang::CXXRecordDecl* Class);
+
/// \returns true if \p Class is ref-counted, false if not.
bool isRefCounted(const clang::CXXRecordDecl *Class);
+/// \returns true if \p Class is a CheckedPtr / CheckedRef, false if not.
+bool isCheckedPtr(const clang::CXXRecordDecl *Class);
+
/// \returns true if \p Class is ref-countable AND not ref-counted, false if
/// not, std::nullopt if inconclusive.
std::optional<bool> isUncounted(const clang::QualType T);
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
similarity index 66%
rename from clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp
rename to clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
index 69a0eb3086ab72..455bb27e0207e9 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
@@ -1,4 +1,4 @@
-//=======- NoUncountedMembersChecker.cpp -------------------------*- C++ -*-==//
+//=======- RawPtrRefMemberChecker.cpp ----------------------------*- C++ -*-==//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
@@ -25,18 +25,20 @@ using namespace ento;
namespace {
-class NoUncountedMemberChecker
+class RawPtrRefMemberChecker
: public Checker<check::ASTDecl<TranslationUnitDecl>> {
private:
BugType Bug;
mutable BugReporter *BR;
public:
- NoUncountedMemberChecker()
- : Bug(this,
- "Member variable is a raw-pointer/reference to reference-countable "
- "type",
- "WebKit coding guidelines") {}
+ RawPtrRefMemberChecker(const char *description)
+ : Bug(this, description, "WebKit coding guidelines") {}
+
+ virtual std::optional<bool> isPtrCompatible(const clang::CXXRecordDecl *) const = 0;
+ virtual bool isPtrCls(const clang::CXXRecordDecl *) const = 0;
+ virtual const char* typeName() const = 0;
+ virtual const char* invariantName() const = 0;
void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
BugReporter &BRArg) const {
@@ -46,8 +48,8 @@ class NoUncountedMemberChecker
// visit template instantiations or lambda classes. We
// want to visit those, so we make our own RecursiveASTVisitor.
struct LocalVisitor : public RecursiveASTVisitor<LocalVisitor> {
- const NoUncountedMemberChecker *Checker;
- explicit LocalVisitor(const NoUncountedMemberChecker *Checker)
+ const RawPtrRefMemberChecker *Checker;
+ explicit LocalVisitor(const RawPtrRefMemberChecker *Checker)
: Checker(Checker) {
assert(Checker);
}
@@ -77,7 +79,7 @@ class NoUncountedMemberChecker
if (auto *MemberCXXRD = MemberType->getPointeeCXXRecordDecl()) {
// If we don't see the definition we just don't know.
if (MemberCXXRD->hasDefinition()) {
- std::optional<bool> isRCAble = isRefCountable(MemberCXXRD);
+ std::optional<bool> isRCAble = isPtrCompatible(MemberCXXRD);
if (isRCAble && *isRCAble)
reportBug(Member, MemberType, MemberCXXRD, RD);
}
@@ -114,7 +116,7 @@ class NoUncountedMemberChecker
// a member but we trust them to handle it correctly.
auto CXXRD = llvm::dyn_cast_or_null<CXXRecordDecl>(RD);
if (CXXRD)
- return isRefCounted(CXXRD);
+ return isPtrCls(CXXRD);
return false;
}
@@ -135,9 +137,13 @@ class NoUncountedMemberChecker
printQuotedQualifiedName(Os, ClassCXXRD);
Os << " is a "
<< (isa<PointerType>(MemberType) ? "raw pointer" : "reference")
- << " to ref-countable type ";
+ << " to "
+ << typeName()
+ << " ";
printQuotedQualifiedName(Os, MemberCXXRD);
- Os << "; member variables must be ref-counted.";
+ Os << "; "
+ << invariantName()
+ << ".";
PathDiagnosticLocation BSLoc(Member->getSourceRange().getBegin(),
BR->getSourceManager());
@@ -146,6 +152,53 @@ class NoUncountedMemberChecker
BR->emitReport(std::move(Report));
}
};
+
+class NoUncountedMemberChecker final : public RawPtrRefMemberChecker {
+public:
+ NoUncountedMemberChecker()
+ : RawPtrRefMemberChecker("Member variable is a raw-pointer/reference to "
+ "reference-countable type") {}
+
+ std::optional<bool> isPtrCompatible(const clang::CXXRecordDecl *R) const final {
+ return isRefCountable(R);
+ }
+
+ bool isPtrCls(const clang::CXXRecordDecl *R) const final {
+ return isRefCounted(R);
+ }
+
+ const char* typeName() const final {
+ return "ref-countable type";
+ }
+
+ const char* invariantName() const final {
+ return "member variables must be ref-counted";
+ }
+};
+
+class NoUncheckedPtrMemberChecker final : public RawPtrRefMemberChecker {
+public:
+ NoUncheckedPtrMemberChecker()
+ : RawPtrRefMemberChecker("Member variable is a raw-pointer/reference to "
+ "checked-pointer capable type") {}
+
+ std::optional<bool> isPtrCompatible(const clang::CXXRecordDecl *R) const final {
+ return isCheckedPtrCapable(R);
+ }
+
+ bool isPtrCls(const clang::CXXRecordDecl *R) const final {
+ return isCheckedPtr(R);
+ }
+
+ const char* typeName() const final {
+ return "CheckedPtr capable type";
+ }
+
+ const char* invariantName() const final {
+ return "member variables must be a CheckedPtr or CheckedRef";
+ }
+};
+
} // namespace
void ento::registerNoUncountedMemberChecker(CheckerManager &Mgr) {
@@ -156,3 +209,12 @@ bool ento::shouldRegisterNoUncountedMemberChecker(
const CheckerManager &Mgr) {
return true;
}
+
+void ento::registerNoUncheckedPtrMemberChecker(CheckerManager &Mgr) {
+ Mgr.registerChecker<NoUncheckedPtrMemberChecker>();
+}
+
+bool ento::shouldRegisterNoUncheckedPtrMemberChecker(
+ const CheckerManager &Mgr) {
+ return true;
+}
diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h
index c427b22fd683e5..933b4c5e62a79c 100644
--- a/clang/test/Analysis/Checkers/WebKit/mock-types.h
+++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h
@@ -108,4 +108,52 @@ struct RefCountable {
template <typename T> T *downcast(T *t) { return t; }
+template <typename T> struct CheckedRef {
+private:
+ T *t;
+
+public:
+ CheckedRef() : t{} {};
+ CheckedRef(T &t) : t(t) { t->incrementPtrCount(); }
+ CheckedRef(const CheckedRef& o) : t(o.t) { if (t) t->incrementPtrCount(); }
+ ~CheckedRef() { if (t) t->decrementPtrCount(); }
+ T &get() { return *t; }
+ T *ptr() { return t; }
+ T *operator->() { return t; }
+ operator const T &() const { return *t; }
+ operator T &() { return *t; }
+};
+
+template <typename T> struct CheckedPtr {
+private:
+ T *t;
+
+public:
+ CheckedPtr() : t(nullptr) {}
+ CheckedPtr(T *t)
+ : t(t) {
+ if (t)
+ t->incrementPtrCount();
+ }
+ CheckedPtr(Ref<T>&& o)
+ : t(o.leakRef())
+ { }
+ ~CheckedPtr() {
+ if (t)
+ t->decrementPtrCount();
+ }
+ T *get() { return t; }
+ T *operator->() { return t; }
+ const T *operator->() const { return t; }
+ T &operator*() { return *t; }
+ CheckedPtr &operator=(T *) { return *this; }
+ operator bool() const { return t; }
+};
+
+class CheckedObj {
+public:
+ void incrementPtrCount();
+ void decrementPtrCount();
+};
+
#endif
diff --git a/clang/test/Analysis/Checkers/WebKit/unchecked-members.cpp b/clang/test/Analysis/Checkers/WebKit/unchecked-members.cpp
new file mode 100644
index 00000000000000..4450d5cab60f0a
--- /dev/null
+++ b/clang/test/Analysis/Checkers/WebKit/unchecked-members.cpp
@@ -0,0 +1,53 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.NoUncheckedPtrMemberChecker -verify %s
+
+#include "mock-types.h"
+#include "mock-system-header.h"
+
+namespace members {
+
+ struct Foo {
+ private:
+ CheckedObj* a = nullptr;
+// expected-warning@-1{{Member variable 'a' in 'members::Foo' is a raw pointer to CheckedPtr capable type 'CheckedObj'}}
+ CheckedObj& b;
+// expected-warning@-1{{Member variable 'b' in 'members::Foo' is a reference to CheckedPtr capable type 'CheckedObj'}}
+
+ [[clang::suppress]]
+ CheckedObj* a_suppressed = nullptr;
+
+ [[clang::suppress]]
+ CheckedObj& b_suppressed;
+
+ CheckedPtr<CheckedObj> c;
+ CheckedRef<CheckedObj> d;
+
+ public:
+ Foo();
+ };
+
+ template <typename S>
+ struct FooTmpl {
+ S* e;
+// expected-warning@-1{{Member variable 'e' in 'members::FooTmpl<CheckedObj>' is a raw pointer to CheckedPtr capable type 'CheckedObj'}}
+ };
+
+ void forceTmplToInstantiate(FooTmpl<CheckedObj>) { }
+
+} // namespace members
+
+namespace ignore_unions {
+
+ union Foo {
+ CheckedObj* a;
+ CheckedPtr<CheckedObj> c;
+ CheckedRef<CheckedObj> d;
+ };
+
+ template<class T>
+ union FooTmpl {
+ T* a;
+ };
+
+ void forceTmplToInstantiate(FooTmpl<CheckedObj>) { }
+
+} // namespace ignore_unions
diff --git a/llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Checkers/BUILD.gn b/llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Checkers/BUILD.gn
index 3b640ae41b9f62..7a6c360e88c14e 100644
--- a/llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Checkers/BUILD.gn
+++ b/llvm/utils/gn/secondary/clang/lib/StaticAnalyzer/Checkers/BUILD.gn
@@ -141,7 +141,7 @@ static_library("Checkers") {
"VforkChecker.cpp",
"VirtualCallChecker.cpp",
"WebKit/ASTUtils.cpp",
- "WebKit/NoUncountedMembersChecker.cpp",
+ "WebKit/RawPtrRefMemberChecker.cpp",
"WebKit/PtrTypesSemantics.cpp",
"WebKit/RefCntblBaseVirtualDtorChecker.cpp",
"WebKit/UncountedCallArgsChecker.cpp",
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
haoNoQ
approved these changes
Sep 25, 2024
| @@ -146,13 +149,67 @@ class NoUncountedMemberChecker | |||
| BR->emitReport(std::move(Report)); | |||
| } | |||
| }; | |||
|
|
|||
| class NoUncountedMemberChecker final : public RawPtrRefMemberChecker { | |||
There was a problem hiding this comment.
Yes this is a perfectly valid way to reuse code here!
| @@ -146,13 +149,67 @@ class NoUncountedMemberChecker | |||
| BR->emitReport(std::move(Report)); | |||
There was a problem hiding this comment.
setDeclWithIssue() goes into a different PR right?
There was a problem hiding this comment.
I'm not calling setDeclWithIssue in this checker since the checker works on classes, and not functions. I guess there could be classes declared within a function?
haoNoQ
reviewed
Sep 25, 2024
…ringRef. Also added simple documentation for alpha.webkit.NoUncountedMemberChecker.
Sterling-Augustine
pushed a commit
to Sterling-Augustine/llvm-project
that referenced
this pull request
Sep 27, 2024
…checker for CheckedPtr/CheckedRef (llvm#108352) This PR introduces new WebKit checker to warn a member variable that is a raw reference or a raw pointer to an object, which is capable of creating a CheckedRef/CheckedPtr.
rniwa
added a commit
to rniwa/llvm-project
that referenced
this pull request
Feb 3, 2025
…checker for CheckedPtr/CheckedRef (llvm#108352) This PR introduces new WebKit checker to warn a member variable that is a raw reference or a raw pointer to an object, which is capable of creating a CheckedRef/CheckedPtr.
devincoughlin
pushed a commit
to swiftlang/llvm-project
that referenced
this pull request
Feb 25, 2025
…checker for CheckedPtr/CheckedRef (llvm#108352) This PR introduces new WebKit checker to warn a member variable that is a raw reference or a raw pointer to an object, which is capable of creating a CheckedRef/CheckedPtr.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces new WebKit checker to warn a member variable that is a raw reference or a raw pointer to an object, which is capable of creating a CheckedRef/CheckedPtr.