[clang] [alpha.webkit.NoUnretainedMemberChecker] Add a new WebKit checker for unretained member variables and ivars. (PR #128641)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 24 23:06:46 PST 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-static-analyzer-1
Author: Ryosuke Niwa (rniwa)
<details>
<summary>Changes</summary>
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.
---
Full diff: https://github.com/llvm/llvm-project/pull/128641.diff
5 Files Affected:
- (modified) clang/docs/analyzer/checkers.rst (+13)
- (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+4)
- (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp (+97-20)
- (added) clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm (+39)
- (added) clang/test/Analysis/Checkers/WebKit/unretained-members.mm (+57)
``````````diff
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>) {}
+}
``````````
</details>
https://github.com/llvm/llvm-project/pull/128641
More information about the cfe-commits
mailing list