[clang] [alpha.webkit.NoUnretainedMemberChecker] Add a new WebKit checker for unretained member variables and ivars. (PR #128641)
Ryosuke Niwa via cfe-commits
cfe-commits at lists.llvm.org
Sun Mar 9 10:05:07 PDT 2025
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/128641
>From a4cd301d6176a8ee3441d0c74f6d2c4e32a50cdd Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Mon, 24 Feb 2025 23:01:31 -0800
Subject: [PATCH 1/3] [alpha.webkit.NoUnretainedMemberChecker] Add a new WebKit
checker for unretained member variables and ivars.
Add a new WebKit checker for member variables and instance variables of NS and CF types.
A member variable or instance variable to a CF type should be RetainPtr regardless of whether
ARC is enabled or disabled, and that of a NS type should be RetainPtr when ARC is disabled.
---
clang/docs/analyzer/checkers.rst | 13 ++
.../clang/StaticAnalyzer/Checkers/Checkers.td | 4 +
.../WebKit/RawPtrRefMemberChecker.cpp | 117 +++++++++++++++---
.../Checkers/WebKit/unretained-members-arc.mm | 39 ++++++
.../Checkers/WebKit/unretained-members.mm | 57 +++++++++
5 files changed, 210 insertions(+), 20 deletions(-)
create mode 100644 clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm
create mode 100644 clang/test/Analysis/Checkers/WebKit/unretained-members.mm
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index c1eedb33e74d2..78372c3f1ca66 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -3487,6 +3487,19 @@ Raw pointers and references to an object which supports CheckedPtr or CheckedRef
See `WebKit Guidelines for Safer C++ Programming <https://github.com/WebKit/WebKit/wiki/Safer-CPP-Guidelines>`_ for details.
+alpha.webkit.NoUnretainedMemberChecker
+""""""""""""""""""""""""""""""""""""""""
+Raw pointers and references to a NS or CF object can't be used as class members or ivars. Only RetainPtr is allowed.
+
+.. code-block:: cpp
+
+ struct Foo {
+ NSObject *ptr; // warn
+ // ...
+ };
+
+See `WebKit Guidelines for Safer C++ Programming <https://github.com/WebKit/WebKit/wiki/Safer-CPP-Guidelines>`_ for details.
+
.. _alpha-webkit-UncountedCallArgsChecker:
alpha.webkit.UncountedCallArgsChecker
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 410f841630660..578b377cb0849 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1766,6 +1766,10 @@ def NoUncheckedPtrMemberChecker : Checker<"NoUncheckedPtrMemberChecker">,
HelpText<"Check for no unchecked member variables.">,
Documentation<HasDocumentation>;
+def NoUnretainedMemberChecker : Checker<"NoUnretainedMemberChecker">,
+ HelpText<"Check for no unretained member variables.">,
+ Documentation<HasDocumentation>;
+
def UncountedCallArgsChecker : Checker<"UncountedCallArgsChecker">,
HelpText<"Check uncounted call arguments.">,
Documentation<HasDocumentation>;
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
index 963f59831c8ed..0e0fd95e1d79d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
@@ -31,12 +31,16 @@ class RawPtrRefMemberChecker
BugType Bug;
mutable BugReporter *BR;
+protected:
+ mutable std::optional<RetainTypeChecker> RTC;
+
public:
RawPtrRefMemberChecker(const char *description)
: Bug(this, description, "WebKit coding guidelines") {}
virtual std::optional<bool>
- isPtrCompatible(const clang::CXXRecordDecl *) const = 0;
+ isPtrCompatible(const clang::QualType,
+ const clang::CXXRecordDecl *R) const = 0;
virtual bool isPtrCls(const clang::CXXRecordDecl *) const = 0;
virtual const char *typeName() const = 0;
virtual const char *invariant() const = 0;
@@ -57,6 +61,12 @@ class RawPtrRefMemberChecker
ShouldVisitImplicitCode = false;
}
+ bool VisitTypedefDecl(const TypedefDecl *TD) override {
+ if (Checker->RTC)
+ Checker->RTC->visitTypedef(TD);
+ return true;
+ }
+
bool VisitRecordDecl(const RecordDecl *RD) override {
Checker->visitRecordDecl(RD);
return true;
@@ -69,6 +79,8 @@ class RawPtrRefMemberChecker
};
LocalVisitor visitor(this);
+ if (RTC)
+ RTC->visitTranslationUnitDecl(TUD);
visitor.TraverseDecl(TUD);
}
@@ -77,16 +89,22 @@ class RawPtrRefMemberChecker
return;
for (auto *Member : RD->fields()) {
- const Type *MemberType = Member->getType().getTypePtrOrNull();
+ auto QT = Member->getType();
+ const Type *MemberType = QT.getTypePtrOrNull();
if (!MemberType)
continue;
if (auto *MemberCXXRD = MemberType->getPointeeCXXRecordDecl()) {
- // If we don't see the definition we just don't know.
- if (MemberCXXRD->hasDefinition()) {
- std::optional<bool> isRCAble = isPtrCompatible(MemberCXXRD);
- if (isRCAble && *isRCAble)
- reportBug(Member, MemberType, MemberCXXRD, RD);
+ std::optional<bool> IsCompatible = isPtrCompatible(QT, MemberCXXRD);
+ if (IsCompatible && *IsCompatible)
+ reportBug(Member, MemberType, MemberCXXRD, RD);
+ } else {
+ std::optional<bool> IsCompatible = isPtrCompatible(QT, nullptr);
+ auto* PointeeType = MemberType->getPointeeType().getTypePtrOrNull();
+ if (IsCompatible && *IsCompatible) {
+ auto *Desugared = PointeeType->getUnqualifiedDesugaredType();
+ if (auto *ObjCType = dyn_cast_or_null<ObjCInterfaceType>(Desugared))
+ reportBug(Member, MemberType, ObjCType->getDecl(), RD);
}
}
}
@@ -107,11 +125,12 @@ class RawPtrRefMemberChecker
void visitIvarDecl(const ObjCContainerDecl *CD,
const ObjCIvarDecl *Ivar) const {
- const Type *IvarType = Ivar->getType().getTypePtrOrNull();
+ auto QT = Ivar->getType();
+ const Type *IvarType = QT.getTypePtrOrNull();
if (!IvarType)
return;
if (auto *IvarCXXRD = IvarType->getPointeeCXXRecordDecl()) {
- std::optional<bool> IsCompatible = isPtrCompatible(IvarCXXRD);
+ std::optional<bool> IsCompatible = isPtrCompatible(QT, IvarCXXRD);
if (IsCompatible && *IsCompatible)
reportBug(Ivar, IvarType, IvarCXXRD, CD);
}
@@ -151,13 +170,13 @@ class RawPtrRefMemberChecker
return false;
}
- template <typename DeclType, typename ParentDeclType>
+ template <typename DeclType, typename PointeeType, typename ParentDeclType>
void reportBug(const DeclType *Member, const Type *MemberType,
- const CXXRecordDecl *MemberCXXRD,
+ const PointeeType *Pointee,
const ParentDeclType *ClassCXXRD) const {
assert(Member);
assert(MemberType);
- assert(MemberCXXRD);
+ assert(Pointee);
SmallString<100> Buf;
llvm::raw_svector_ostream Os(Buf);
@@ -169,10 +188,13 @@ class RawPtrRefMemberChecker
printQuotedName(Os, Member);
Os << " in ";
printQuotedQualifiedName(Os, ClassCXXRD);
- Os << " is a "
- << (isa<PointerType>(MemberType) ? "raw pointer" : "reference") << " to "
- << typeName() << " ";
- printQuotedQualifiedName(Os, MemberCXXRD);
+ Os << " is a ";
+ if (printPointer(Os, MemberType) == PrintDeclKind::Pointer) {
+ auto Typedef = MemberType->getAs<TypedefType>();
+ assert(Typedef);
+ printQuotedQualifiedName(Os, Typedef->getDecl());
+ } else
+ printQuotedQualifiedName(Os, Pointee);
Os << "; " << invariant() << ".";
PathDiagnosticLocation BSLoc(Member->getSourceRange().getBegin(),
@@ -181,6 +203,16 @@ class RawPtrRefMemberChecker
Report->addRange(Member->getSourceRange());
BR->emitReport(std::move(Report));
}
+
+ enum class PrintDeclKind { Pointee, Pointer };
+ virtual PrintDeclKind printPointer(llvm::raw_svector_ostream& Os,
+ const Type *T) const {
+ T = T->getUnqualifiedDesugaredType();
+ bool IsPtr = isa<PointerType>(T) || isa<ObjCObjectPointerType>(T);
+ Os << (IsPtr ? "raw pointer" : "reference") << " to "
+ << typeName() << " ";
+ return PrintDeclKind::Pointee;
+ }
};
class NoUncountedMemberChecker final : public RawPtrRefMemberChecker {
@@ -190,8 +222,9 @@ class NoUncountedMemberChecker final : public RawPtrRefMemberChecker {
"reference-countable type") {}
std::optional<bool>
- isPtrCompatible(const clang::CXXRecordDecl *R) const final {
- return isRefCountable(R);
+ isPtrCompatible(const clang::QualType,
+ const clang::CXXRecordDecl *R) const final {
+ return R && isRefCountable(R);
}
bool isPtrCls(const clang::CXXRecordDecl *R) const final {
@@ -212,8 +245,9 @@ class NoUncheckedPtrMemberChecker final : public RawPtrRefMemberChecker {
"checked-pointer capable type") {}
std::optional<bool>
- isPtrCompatible(const clang::CXXRecordDecl *R) const final {
- return isCheckedPtrCapable(R);
+ isPtrCompatible(const clang::QualType,
+ const clang::CXXRecordDecl *R) const final {
+ return R && isCheckedPtrCapable(R);
}
bool isPtrCls(const clang::CXXRecordDecl *R) const final {
@@ -228,6 +262,40 @@ class NoUncheckedPtrMemberChecker final : public RawPtrRefMemberChecker {
}
};
+class NoUnretainedMemberChecker final : public RawPtrRefMemberChecker {
+public:
+ NoUnretainedMemberChecker()
+ : RawPtrRefMemberChecker("Member variable is a raw-pointer/reference to "
+ "retainable type") {
+ RTC = RetainTypeChecker();
+ }
+
+ std::optional<bool>
+ isPtrCompatible(const clang::QualType QT,
+ const clang::CXXRecordDecl *) const final {
+ return RTC->isUnretained(QT);
+ }
+
+ bool isPtrCls(const clang::CXXRecordDecl *R) const final {
+ return isRetainPtr(R);
+ }
+
+ const char *typeName() const final { return "retainable type"; }
+
+ const char *invariant() const final {
+ return "member variables must be a RetainPtr";
+ }
+
+ PrintDeclKind printPointer(llvm::raw_svector_ostream& Os,
+ const Type *T) const final {
+ if (!isa<ObjCObjectPointerType>(T) && T->getAs<TypedefType>()) {
+ Os << typeName() << " ";
+ return PrintDeclKind::Pointer;
+ }
+ return RawPtrRefMemberChecker::printPointer(Os, T);
+ }
+};
+
} // namespace
void ento::registerNoUncountedMemberChecker(CheckerManager &Mgr) {
@@ -246,3 +314,12 @@ bool ento::shouldRegisterNoUncheckedPtrMemberChecker(
const CheckerManager &Mgr) {
return true;
}
+
+void ento::registerNoUnretainedMemberChecker(CheckerManager &Mgr) {
+ Mgr.registerChecker<NoUnretainedMemberChecker>();
+}
+
+bool ento::shouldRegisterNoUnretainedMemberChecker(
+ const CheckerManager &Mgr) {
+ return true;
+}
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm b/clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm
new file mode 100644
index 0000000000000..9820c875b87c0
--- /dev/null
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm
@@ -0,0 +1,39 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.NoUnretainedMemberChecker -fobjc-arc -verify %s
+
+#include "objc-mock-types.h"
+
+namespace members {
+
+ struct Foo {
+ private:
+ SomeObj* a = nullptr;
+
+ [[clang::suppress]]
+ SomeObj* a_suppressed = nullptr;
+
+ protected:
+ RetainPtr<SomeObj> b;
+
+ public:
+ SomeObj* c = nullptr;
+ RetainPtr<SomeObj> d;
+
+ CFMutableArrayRef e = nullptr;
+// expected-warning at -1{{Member variable 'e' in 'members::Foo' is a retainable type 'CFMutableArrayRef'}}
+ };
+
+ template<class T, class S>
+ struct FooTmpl {
+ T* x;
+ S y;
+// expected-warning at -1{{Member variable 'y' in 'members::FooTmpl<SomeObj, __CFArray *>' is a raw pointer to retainable type}}
+ };
+
+ void forceTmplToInstantiate(FooTmpl<SomeObj, CFMutableArrayRef>) {}
+
+ struct [[clang::suppress]] FooSuppressed {
+ private:
+ SomeObj* a = nullptr;
+ };
+
+}
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-members.mm b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
new file mode 100644
index 0000000000000..fc607bf52b176
--- /dev/null
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
@@ -0,0 +1,57 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.NoUnretainedMemberChecker -verify %s
+
+#include "objc-mock-types.h"
+
+namespace members {
+
+ struct Foo {
+ private:
+ SomeObj* a = nullptr;
+// expected-warning at -1{{Member variable 'a' in 'members::Foo' is a raw pointer to retainable type}}
+
+ [[clang::suppress]]
+ SomeObj* a_suppressed = nullptr;
+
+ protected:
+ RetainPtr<SomeObj> b;
+
+ public:
+ SomeObj* c = nullptr;
+// expected-warning at -1{{Member variable 'c' in 'members::Foo' is a raw pointer to retainable type}}
+ RetainPtr<SomeObj> d;
+
+ CFMutableArrayRef e = nullptr;
+// expected-warning at -1{{Member variable 'e' in 'members::Foo' is a retainable type 'CFMutableArrayRef'}}
+ };
+
+ template<class T, class S>
+ struct FooTmpl {
+ T* a;
+// expected-warning at -1{{Member variable 'a' in 'members::FooTmpl<SomeObj, __CFArray *>' is a raw pointer to retainable type}}
+ S b;
+// expected-warning at -1{{Member variable 'b' in 'members::FooTmpl<SomeObj, __CFArray *>' is a raw pointer to retainable type}}
+ };
+
+ void forceTmplToInstantiate(FooTmpl<SomeObj, CFMutableArrayRef>) {}
+
+ struct [[clang::suppress]] FooSuppressed {
+ private:
+ SomeObj* a = nullptr;
+ };
+
+}
+
+namespace ignore_unions {
+ union Foo {
+ SomeObj* a;
+ RetainPtr<SomeObj> b;
+ CFMutableArrayRef c;
+ };
+
+ template<class T>
+ union RefPtr {
+ T* a;
+ };
+
+ void forceTmplToInstantiate(RefPtr<SomeObj>) {}
+}
>From 572a81afa23807b236cecf8e0974f0f5c644937f Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Mon, 24 Feb 2025 23:11:49 -0800
Subject: [PATCH 2/3] Fix formatting.
---
.../Checkers/WebKit/RawPtrRefMemberChecker.cpp | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
index 0e0fd95e1d79d..593e6e3663639 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
@@ -100,7 +100,7 @@ class RawPtrRefMemberChecker
reportBug(Member, MemberType, MemberCXXRD, RD);
} else {
std::optional<bool> IsCompatible = isPtrCompatible(QT, nullptr);
- auto* PointeeType = MemberType->getPointeeType().getTypePtrOrNull();
+ auto *PointeeType = MemberType->getPointeeType().getTypePtrOrNull();
if (IsCompatible && *IsCompatible) {
auto *Desugared = PointeeType->getUnqualifiedDesugaredType();
if (auto *ObjCType = dyn_cast_or_null<ObjCInterfaceType>(Desugared))
@@ -205,12 +205,11 @@ class RawPtrRefMemberChecker
}
enum class PrintDeclKind { Pointee, Pointer };
- virtual PrintDeclKind printPointer(llvm::raw_svector_ostream& Os,
- const Type *T) const {
+ virtual PrintDeclKind printPointer(llvm::raw_svector_ostream &Os,
+ const Type *T) const {
T = T->getUnqualifiedDesugaredType();
bool IsPtr = isa<PointerType>(T) || isa<ObjCObjectPointerType>(T);
- Os << (IsPtr ? "raw pointer" : "reference") << " to "
- << typeName() << " ";
+ Os << (IsPtr ? "raw pointer" : "reference") << " to " << typeName() << " ";
return PrintDeclKind::Pointee;
}
};
@@ -286,7 +285,7 @@ class NoUnretainedMemberChecker final : public RawPtrRefMemberChecker {
return "member variables must be a RetainPtr";
}
- PrintDeclKind printPointer(llvm::raw_svector_ostream& Os,
+ PrintDeclKind printPointer(llvm::raw_svector_ostream &Os,
const Type *T) const final {
if (!isa<ObjCObjectPointerType>(T) && T->getAs<TypedefType>()) {
Os << typeName() << " ";
@@ -319,7 +318,6 @@ void ento::registerNoUnretainedMemberChecker(CheckerManager &Mgr) {
Mgr.registerChecker<NoUnretainedMemberChecker>();
}
-bool ento::shouldRegisterNoUnretainedMemberChecker(
- const CheckerManager &Mgr) {
+bool ento::shouldRegisterNoUnretainedMemberChecker(const CheckerManager &Mgr) {
return true;
}
>From 9c814864236ef74203e9417e1ad8612ac4c6249e Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Sun, 9 Mar 2025 10:04:48 -0700
Subject: [PATCH 3/3] Address review comments
---
clang/docs/analyzer/checkers.rst | 2 +-
clang/test/Analysis/Checkers/WebKit/unretained-members.mm | 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 78372c3f1ca66..9a9d3bcb54b03 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -3489,7 +3489,7 @@ See `WebKit Guidelines for Safer C++ Programming <https://github.com/WebKit/WebK
alpha.webkit.NoUnretainedMemberChecker
""""""""""""""""""""""""""""""""""""""""
-Raw pointers and references to a NS or CF object can't be used as class members or ivars. Only RetainPtr is allowed.
+Raw pointers and references to a NS or CF object can't be used as class members or ivars. Only RetainPtr is allowed for CF types regardless of whether ARC is enabled or disabled. Only RetainPtr is allowed for NS types when ARC is disabled.
.. code-block:: cpp
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-members.mm b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
index fc607bf52b176..e068a583c18c5 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
@@ -11,9 +11,11 @@
[[clang::suppress]]
SomeObj* a_suppressed = nullptr;
+// No warning.
protected:
RetainPtr<SomeObj> b;
+// No warning.
public:
SomeObj* c = nullptr;
@@ -37,6 +39,7 @@ void forceTmplToInstantiate(FooTmpl<SomeObj, CFMutableArrayRef>) {}
struct [[clang::suppress]] FooSuppressed {
private:
SomeObj* a = nullptr;
+// No warning.
};
}
More information about the cfe-commits
mailing list