[alpha.webkit.RawPtrRefMemberChecker] The checker doesn't warn Objective-C types in ivars. #132833
+65
−3
Conversation
This file contains 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
…ive-C types in ivars. This PR fixes the bug that we weren't generating warnings when a raw poiner is used to point to a NS type in Objective-C ivars. Also fix the bug that we weren't suppressing this warning in system headers.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Ryosuke Niwa (rniwa) ChangesThis PR fixes the bug that we weren't generating warnings when a raw poiner is used to point to a NS type in Objective-C ivars. Also fix the bug that we weren't suppressing this warning in system headers. Full diff: https://github.com/llvm/llvm-project/pull/132833.diff 3 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
index dc4e2c7d168fb..6d20869043358 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
@@ -111,6 +111,8 @@ class RawPtrRefMemberChecker
}
void visitObjCDecl(const ObjCContainerDecl *CD) const {
+ if (BR->getSourceManager().isInSystemHeader(CD->getLocation()))
+ return;
if (auto *ID = dyn_cast<ObjCInterfaceDecl>(CD)) {
for (auto *Ivar : ID->ivars())
visitIvarDecl(CD, Ivar);
@@ -133,6 +135,14 @@ class RawPtrRefMemberChecker
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);
+ }
}
}
diff --git a/clang/test/Analysis/Checkers/WebKit/mock-system-header.h b/clang/test/Analysis/Checkers/WebKit/mock-system-header.h
index e993fd697ffab..b377f5098c002 100644
--- a/clang/test/Analysis/Checkers/WebKit/mock-system-header.h
+++ b/clang/test/Analysis/Checkers/WebKit/mock-system-header.h
@@ -29,3 +29,14 @@ enum os_log_type_t : uint8_t {
typedef struct os_log_s *os_log_t;
os_log_t os_log_create(const char *subsystem, const char *category);
void os_log_msg(os_log_t oslog, os_log_type_t type, const char *msg, ...);
+
+typedef const struct __attribute__((objc_bridge(NSString))) __CFString * CFStringRef;
+
+#ifdef __OBJC__
+@class NSString;
+@interface SystemObject : NSObject {
+ NSString *ns_string;
+ CFStringRef cf_string;
+}
+@end
+#endif
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-members.mm b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
index e068a583c18c5..79f7a05caa1be 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
@@ -1,7 +1,8 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.NoUnretainedMemberChecker -verify %s
#include "objc-mock-types.h"
-
+#include "mock-system-header.h"
+#if 0
namespace members {
struct Foo {
@@ -58,3 +59,12 @@ void forceTmplToInstantiate(FooTmpl<SomeObj, CFMutableArrayRef>) {}
void forceTmplToInstantiate(RefPtr<SomeObj>) {}
}
+#endif
+
+@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}}
+ CFStringRef cf_string;
+ // expected-warning@-1{{Instance variable 'cf_string' in 'AnotherObject' is a retainable type 'CFStringRef'; member variables must be a RetainPtr}}
+}
+@end
|
|
@llvm/pr-subscribers-clang Author: Ryosuke Niwa (rniwa) ChangesThis PR fixes the bug that we weren't generating warnings when a raw poiner is used to point to a NS type in Objective-C ivars. Also fix the bug that we weren't suppressing this warning in system headers. Full diff: https://github.com/llvm/llvm-project/pull/132833.diff 3 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
index dc4e2c7d168fb..6d20869043358 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
@@ -111,6 +111,8 @@ class RawPtrRefMemberChecker
}
void visitObjCDecl(const ObjCContainerDecl *CD) const {
+ if (BR->getSourceManager().isInSystemHeader(CD->getLocation()))
+ return;
if (auto *ID = dyn_cast<ObjCInterfaceDecl>(CD)) {
for (auto *Ivar : ID->ivars())
visitIvarDecl(CD, Ivar);
@@ -133,6 +135,14 @@ class RawPtrRefMemberChecker
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);
+ }
}
}
diff --git a/clang/test/Analysis/Checkers/WebKit/mock-system-header.h b/clang/test/Analysis/Checkers/WebKit/mock-system-header.h
index e993fd697ffab..b377f5098c002 100644
--- a/clang/test/Analysis/Checkers/WebKit/mock-system-header.h
+++ b/clang/test/Analysis/Checkers/WebKit/mock-system-header.h
@@ -29,3 +29,14 @@ enum os_log_type_t : uint8_t {
typedef struct os_log_s *os_log_t;
os_log_t os_log_create(const char *subsystem, const char *category);
void os_log_msg(os_log_t oslog, os_log_type_t type, const char *msg, ...);
+
+typedef const struct __attribute__((objc_bridge(NSString))) __CFString * CFStringRef;
+
+#ifdef __OBJC__
+@class NSString;
+@interface SystemObject : NSObject {
+ NSString *ns_string;
+ CFStringRef cf_string;
+}
+@end
+#endif
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-members.mm b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
index e068a583c18c5..79f7a05caa1be 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
@@ -1,7 +1,8 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.NoUnretainedMemberChecker -verify %s
#include "objc-mock-types.h"
-
+#include "mock-system-header.h"
+#if 0
namespace members {
struct Foo {
@@ -58,3 +59,12 @@ void forceTmplToInstantiate(FooTmpl<SomeObj, CFMutableArrayRef>) {}
void forceTmplToInstantiate(RefPtr<SomeObj>) {}
}
+#endif
+
+@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}}
+ CFStringRef cf_string;
+ // expected-warning@-1{{Instance variable 'cf_string' in 'AnotherObject' is a retainable type 'CFStringRef'; member variables must be a RetainPtr}}
+}
+@end
|
t-rasmud
reviewed
Mar 26, 2025
| CFStringRef cf_string; | ||
| // expected-warning@-1{{Instance variable 'cf_string' in 'AnotherObject' is a retainable type 'CFStringRef'; member variables must be a RetainPtr}} | ||
| } | ||
| @end |
There was a problem hiding this comment.
This is more of a question for my understanding: Do we expect the same behavior for raw pointers that are properties?
@interface AnotherObject : NSObject
@property(nonatomic,strong) NSString *prop_string;
@end
There was a problem hiding this comment.
Can we add a test case for properties?
There was a problem hiding this comment.
Turns out I have to enumerate properties separately from ivars. Added a test with code fix.
|
Thanks for the review! |
rniwa
commented
Mar 28, 2025
| return; | ||
| if (auto *PropCXXRD = PropType->getPointeeCXXRecordDecl()) { | ||
| std::optional<bool> IsCompatible = isPtrCompatible(QT, PropCXXRD); | ||
| fprintf(stderr, "IsCompatible=%d\n", IsCompatible ? *IsCompatible : -1); |
There was a problem hiding this comment.
Oh oops, I forgot to remove this.
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 fixes the bug that we weren't generating warnings when a raw poiner is used to point to a NS type in Objective-C ivars. Also fix the bug that we weren't suppressing this warning in system headers.