llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-static-analyzer-1 Author: Ryosuke Niwa (rniwa) <details> <summary>Changes</summary> --- Full diff: https://github.com/llvm/llvm-project/pull/137565.diff 5 Files Affected: - (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp (+67-61) - (modified) clang/test/Analysis/Checkers/WebKit/unchecked-members.cpp (+24-2) - (modified) clang/test/Analysis/Checkers/WebKit/uncounted-members.cpp (+26-4) - (modified) clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm (+27) - (modified) clang/test/Analysis/Checkers/WebKit/unretained-members.mm (+31-3) ``````````diff diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp index 10b9749319a57..b5395ca7e34ff 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp @@ -38,9 +38,7 @@ class RawPtrRefMemberChecker RawPtrRefMemberChecker(const char *description) : Bug(this, description, "WebKit coding guidelines") {} - virtual std::optional<bool> - isPtrCompatible(const clang::QualType, - const clang::CXXRecordDecl *R) const = 0; + virtual std::optional<bool> isUnsafePtr(QualType) const = 0; virtual const char *typeName() const = 0; virtual const char *invariant() const = 0; @@ -87,26 +85,44 @@ class RawPtrRefMemberChecker if (shouldSkipDecl(RD)) return; - for (auto *Member : RD->fields()) { - auto QT = Member->getType(); - const Type *MemberType = QT.getTypePtrOrNull(); - if (!MemberType) - continue; - - if (auto *MemberCXXRD = MemberType->getPointeeCXXRecordDecl()) { - 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); - } - } + for (auto *Member : RD->fields()) + visitMember(Member, RD); + } + + void visitMember(const FieldDecl* Member, const RecordDecl *RD) const { + auto QT = Member->getType(); + const Type *MemberType = QT.getTypePtrOrNull(); + + while (MemberType) { + auto IsUnsafePtr = isUnsafePtr(QT); + if (IsUnsafePtr && *IsUnsafePtr) + break; + if (!MemberType->isPointerType()) + return; + QT = MemberType->getPointeeType(); + MemberType = QT.getTypePtrOrNull(); } + + if (!MemberType) + return; + + if (auto *MemberCXXRD = MemberType->getPointeeCXXRecordDecl()) + reportBug(Member, MemberType, MemberCXXRD, RD); + else if (auto* ObjCDecl = getObjCDecl(MemberType)) + reportBug(Member, MemberType, ObjCDecl, RD); + } + + ObjCInterfaceDecl* getObjCDecl(const Type *TypePtr) const { + auto *PointeeType = TypePtr->getPointeeType().getTypePtrOrNull(); + if (!PointeeType) + return nullptr; + auto *Desugared = PointeeType->getUnqualifiedDesugaredType(); + if (!Desugared) + return nullptr; + auto *ObjCType = dyn_cast<ObjCInterfaceType>(Desugared); + if (!ObjCType) + return nullptr; + return ObjCType->getDecl(); } void visitObjCDecl(const ObjCContainerDecl *CD) const { @@ -138,19 +154,15 @@ class RawPtrRefMemberChecker const Type *IvarType = QT.getTypePtrOrNull(); if (!IvarType) return; - if (auto *IvarCXXRD = IvarType->getPointeeCXXRecordDecl()) { - std::optional<bool> IsCompatible = isPtrCompatible(QT, IvarCXXRD); - if (IsCompatible && *IsCompatible) - reportBug(Ivar, IvarType, IvarCXXRD, CD); - } else { - std::optional<bool> IsCompatible = isPtrCompatible(QT, nullptr); - auto *PointeeType = IvarType->getPointeeType().getTypePtrOrNull(); - if (IsCompatible && *IsCompatible) { - auto *Desugared = PointeeType->getUnqualifiedDesugaredType(); - if (auto *ObjCType = dyn_cast_or_null<ObjCInterfaceType>(Desugared)) - reportBug(Ivar, IvarType, ObjCType->getDecl(), CD); - } - } + + auto IsUnsafePtr = isUnsafePtr(QT); + if (!IsUnsafePtr || !*IsUnsafePtr) + return; + + if (auto *MemberCXXRD = IvarType->getPointeeCXXRecordDecl()) + reportBug(Ivar, IvarType, MemberCXXRD, CD); + else if (auto* ObjCDecl = getObjCDecl(IvarType)) + reportBug(Ivar, IvarType, ObjCDecl, CD); } void visitObjCPropertyDecl(const ObjCContainerDecl *CD, @@ -161,19 +173,15 @@ class RawPtrRefMemberChecker const Type *PropType = QT.getTypePtrOrNull(); if (!PropType) return; - if (auto *PropCXXRD = PropType->getPointeeCXXRecordDecl()) { - std::optional<bool> IsCompatible = isPtrCompatible(QT, PropCXXRD); - if (IsCompatible && *IsCompatible) - reportBug(PD, PropType, PropCXXRD, CD); - } else { - std::optional<bool> IsCompatible = isPtrCompatible(QT, nullptr); - auto *PointeeType = PropType->getPointeeType().getTypePtrOrNull(); - if (IsCompatible && *IsCompatible) { - auto *Desugared = PointeeType->getUnqualifiedDesugaredType(); - if (auto *ObjCType = dyn_cast_or_null<ObjCInterfaceType>(Desugared)) - reportBug(PD, PropType, ObjCType->getDecl(), CD); - } - } + + auto IsUnsafePtr = isUnsafePtr(QT); + if (!IsUnsafePtr || !*IsUnsafePtr) + return; + + if (auto *MemberCXXRD = PropType->getPointeeCXXRecordDecl()) + reportBug(PD, PropType, MemberCXXRD, CD); + else if (auto* ObjCDecl = getObjCDecl(PropType)) + reportBug(PD, PropType, ObjCDecl, CD); } bool shouldSkipDecl(const RecordDecl *RD) const { @@ -194,7 +202,8 @@ class RawPtrRefMemberChecker const auto Kind = RD->getTagKind(); // FIMXE: Should we check union members too? - if (Kind != TagTypeKind::Struct && Kind != TagTypeKind::Class) + if (Kind != TagTypeKind::Struct && Kind != TagTypeKind::Class && + Kind != TagTypeKind::Union) return true; // Ignore CXXRecords that come from system headers. @@ -231,7 +240,10 @@ class RawPtrRefMemberChecker printQuotedName(Os, Member); Os << " in "; printQuotedQualifiedName(Os, ClassCXXRD); - Os << " is a "; + if (Member->getType().getTypePtrOrNull() == MemberType) + Os << " is a "; + else + Os << " contains a "; if (printPointer(Os, MemberType) == PrintDeclKind::Pointer) { auto Typedef = MemberType->getAs<TypedefType>(); assert(Typedef); @@ -263,10 +275,8 @@ class NoUncountedMemberChecker final : public RawPtrRefMemberChecker { : RawPtrRefMemberChecker("Member variable is a raw-pointer/reference to " "reference-countable type") {} - std::optional<bool> - isPtrCompatible(const clang::QualType, - const clang::CXXRecordDecl *R) const final { - return R ? isRefCountable(R) : std::nullopt; + std::optional<bool> isUnsafePtr(QualType QT) const final { + return isUncountedPtr(QT.getCanonicalType()); } const char *typeName() const final { return "ref-countable type"; } @@ -282,10 +292,8 @@ class NoUncheckedPtrMemberChecker final : public RawPtrRefMemberChecker { : RawPtrRefMemberChecker("Member variable is a raw-pointer/reference to " "checked-pointer capable type") {} - std::optional<bool> - isPtrCompatible(const clang::QualType, - const clang::CXXRecordDecl *R) const final { - return R ? isCheckedPtrCapable(R) : std::nullopt; + std::optional<bool> isUnsafePtr(QualType QT) const final { + return isUncheckedPtr(QT.getCanonicalType()); } const char *typeName() const final { return "CheckedPtr capable type"; } @@ -304,9 +312,7 @@ class NoUnretainedMemberChecker final : public RawPtrRefMemberChecker { RTC = RetainTypeChecker(); } - std::optional<bool> - isPtrCompatible(const clang::QualType QT, - const clang::CXXRecordDecl *) const final { + std::optional<bool> isUnsafePtr(QualType QT) const final { return RTC->isUnretained(QT); } diff --git a/clang/test/Analysis/Checkers/WebKit/unchecked-members.cpp b/clang/test/Analysis/Checkers/WebKit/unchecked-members.cpp index 048ffbffcdefb..3fe15d88ff312 100644 --- a/clang/test/Analysis/Checkers/WebKit/unchecked-members.cpp +++ b/clang/test/Analysis/Checkers/WebKit/unchecked-members.cpp @@ -34,10 +34,11 @@ namespace members { } // namespace members -namespace ignore_unions { +namespace unions { union Foo { CheckedObj* a; + // expected-warning@-1{{Member variable 'a' in 'unions::Foo' is a raw pointer to CheckedPtr capable type 'CheckedObj'}} CheckedPtr<CheckedObj> c; CheckedRef<CheckedObj> d; }; @@ -45,11 +46,12 @@ namespace ignore_unions { template<class T> union FooTmpl { T* a; + // expected-warning@-1{{Member variable 'a' in 'unions::FooTmpl<CheckedObj>' is a raw pointer to CheckedPtr capable type 'CheckedObj'}} }; void forceTmplToInstantiate(FooTmpl<CheckedObj>) { } -} // namespace ignore_unions +} // namespace unions namespace checked_ptr_ref_ptr_capable { @@ -59,3 +61,23 @@ namespace checked_ptr_ref_ptr_capable { } } // checked_ptr_ref_ptr_capable + +namespace ptr_to_ptr_to_checked_ptr_capable { + + struct List { + CheckedObj** elements; + // expected-warning@-1{{Member variable 'elements' in 'ptr_to_ptr_to_checked_ptr_capable::List' contains a raw pointer to CheckedPtr capable type 'CheckedObj'}} + }; + + template <typename T> + struct TemplateList { + T** elements; + // expected-warning@-1{{Member variable 'elements' in 'ptr_to_ptr_to_checked_ptr_capable::TemplateList<CheckedObj>' contains a raw pointer to CheckedPtr capable type 'CheckedObj'}} + }; + TemplateList<CheckedObj> list; + + struct SafeList { + CheckedPtr<CheckedObj>* elements; + }; + +} // namespace ptr_to_ptr_to_checked_ptr_capable diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-members.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-members.cpp index 130777a9a5fee..b8c443cda4f8e 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-members.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-members.cpp @@ -36,20 +36,22 @@ namespace members { }; } // members -namespace ignore_unions { +namespace unions { union Foo { RefCountable* a; + // expected-warning@-1{{Member variable 'a' in 'unions::Foo' is a raw pointer to ref-countable type 'RefCountable'}} RefPtr<RefCountable> b; Ref<RefCountable> c; }; template<class T> - union RefPtr { + union FooTmpl { T* a; + // expected-warning@-1{{Member variable 'a' in 'unions::FooTmpl<RefCountable>' is a raw pointer to ref-countable type 'RefCountable'}} }; - void forceTmplToInstantiate(RefPtr<RefCountable>) {} -} // ignore_unions + void forceTmplToInstantiate(FooTmpl<RefCountable>) {} +} // unions namespace ignore_system_header { @@ -77,3 +79,23 @@ namespace checked_ptr_ref_ptr_capable { } } // checked_ptr_ref_ptr_capable + +namespace ptr_to_ptr_to_ref_counted { + + struct List { + RefCountable** elements; + // expected-warning@-1{{Member variable 'elements' in 'ptr_to_ptr_to_ref_counted::List' contains a raw pointer to ref-countable type 'RefCountable'}} + }; + + template <typename T> + struct TemplateList { + T** elements; + // expected-warning@-1{{Member variable 'elements' in 'ptr_to_ptr_to_ref_counted::TemplateList<RefCountable>' contains a raw pointer to ref-countable type 'RefCountable'}} + }; + TemplateList<RefCountable> list; + + struct SafeList { + RefPtr<RefCountable>* elements; + }; + +} // namespace ptr_to_ptr_to_ref_counted diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm b/clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm index 9820c875b87c0..3491bc93ed98a 100644 --- a/clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm +++ b/clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm @@ -21,6 +21,12 @@ CFMutableArrayRef e = nullptr; // expected-warning@-1{{Member variable 'e' in 'members::Foo' is a retainable type 'CFMutableArrayRef'}} }; + + union FooUnion { + SomeObj* a; + CFMutableArrayRef b; + // expected-warning@-1{{Member variable 'b' in 'members::FooUnion' is a retainable type 'CFMutableArrayRef'}} + }; template<class T, class S> struct FooTmpl { @@ -37,3 +43,24 @@ void forceTmplToInstantiate(FooTmpl<SomeObj, CFMutableArrayRef>) {} }; } + +namespace ptr_to_ptr_to_retained { + + struct List { + CFMutableArrayRef* elements2; + // expected-warning@-1{{Member variable 'elements2' in 'ptr_to_ptr_to_retained::List' contains a retainable type 'CFMutableArrayRef'}} + }; + + template <typename T, typename S> + struct TemplateList { + S* elements2; + // expected-warning@-1{{Member variable 'elements2' in 'ptr_to_ptr_to_retained::TemplateList<SomeObj, __CFArray *>' contains a raw pointer to retainable type '__CFArray'}} + }; + TemplateList<SomeObj, CFMutableArrayRef> list; + + struct SafeList { + RetainPtr<SomeObj>* elements1; + RetainPtr<CFMutableArrayRef>* elements2; + }; + +} // namespace ptr_to_ptr_to_retained diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-members.mm b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm index fff1f8ede091b..0cb4c4ac0f6a0 100644 --- a/clang/test/Analysis/Checkers/WebKit/unretained-members.mm +++ b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm @@ -47,21 +47,49 @@ void forceTmplToInstantiate(FooTmpl<SomeObj, CFMutableArrayRef>) {} } -namespace ignore_unions { +namespace unions { union Foo { SomeObj* a; + // expected-warning@-1{{Member variable 'a' in 'unions::Foo' is a raw pointer to retainable type 'SomeObj'}} RetainPtr<SomeObj> b; CFMutableArrayRef c; + // expected-warning@-1{{Member variable 'c' in 'unions::Foo' is a retainable type 'CFMutableArrayRef'}} }; template<class T> - union RefPtr { + union FooTempl { T* a; + // expected-warning@-1{{Member variable 'a' in 'unions::FooTempl<SomeObj>' is a raw pointer to retainable type 'SomeObj'}} }; - void forceTmplToInstantiate(RefPtr<SomeObj>) {} + void forceTmplToInstantiate(FooTempl<SomeObj>) {} } +namespace ptr_to_ptr_to_retained { + + struct List { + SomeObj** elements1; + // expected-warning@-1{{Member variable 'elements1' in 'ptr_to_ptr_to_retained::List' contains a raw pointer to retainable type 'SomeObj'}} + CFMutableArrayRef* elements2; + // expected-warning@-1{{Member variable 'elements2' in 'ptr_to_ptr_to_retained::List' contains a retainable type 'CFMutableArrayRef'}} + }; + + template <typename T, typename S> + struct TemplateList { + T** elements1; + // expected-warning@-1{{Member variable 'elements1' in 'ptr_to_ptr_to_retained::TemplateList<SomeObj, __CFArray *>' contains a raw pointer to retainable type 'SomeObj'}} + S* elements2; + // expected-warning@-1{{Member variable 'elements2' in 'ptr_to_ptr_to_retained::TemplateList<SomeObj, __CFArray *>' contains a raw pointer to retainable type '__CFArray'}} + }; + TemplateList<SomeObj, CFMutableArrayRef> list; + + struct SafeList { + RetainPtr<SomeObj>* elements1; + RetainPtr<CFMutableArrayRef>* elements2; + }; + +} // namespace ptr_to_ptr_to_retained + @interface AnotherObject : NSObject { NSString *ns_string; // expected-warning@-1{{Instance variable 'ns_string' in 'AnotherObject' is a raw pointer to retainable type 'NSString'; member variables must be a RetainPtr}} `````````` </details> https://github.com/llvm/llvm-project/pull/137565 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
