[WebKit checkers] Recognize adoptRef as a safe function #119846
Merged
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
adoptRef in WebKit constructs Ref/RefPtr so treat it as such in isCtorOfRefCounted. Also removed the support for makeRef and makeRefPtr as they don't exist any more.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Ryosuke Niwa (rniwa) ChangesadoptRef in WebKit constructs Ref/RefPtr so treat it as such in isCtorOfRefCounted. Also removed the support for makeRef and makeRefPtr as they don't exist any more. Full diff: https://github.com/llvm/llvm-project/pull/119846.diff 3 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index 797f3e1f3fba5a..5487fea1b956c8 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -125,9 +125,8 @@ bool isCtorOfRefCounted(const clang::FunctionDecl *F) {
assert(F);
const std::string &FunctionName = safeGetName(F);
- return isRefType(FunctionName) || FunctionName == "makeRef" ||
- FunctionName == "makeRefPtr" || FunctionName == "UniqueRef" ||
- FunctionName == "makeUniqueRef" ||
+ return isRefType(FunctionName) || FunctionName == "adoptRef" ||
+ FunctionName == "UniqueRef" || FunctionName == "makeUniqueRef" ||
FunctionName == "makeUniqueRefWithoutFastMallocCheck"
|| FunctionName == "String" || FunctionName == "AtomString" ||
diff --git a/clang/test/Analysis/Checkers/WebKit/call-args.cpp b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
index 94efddeaf66cd8..574e3aa6ef476a 100644
--- a/clang/test/Analysis/Checkers/WebKit/call-args.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
@@ -365,3 +365,20 @@ namespace call_with_explicit_temporary_obj {
RefPtr { provide() }->method();
}
}
+
+namespace call_with_adopt_ref {
+ class Obj {
+ public:
+ void ref() const;
+ void deref() const;
+ void method();
+ };
+
+ struct dummy {
+ RefPtr<Obj> any;
+ };
+
+ void foo() {
+ adoptRef(new Obj)->method();
+ }
+}
diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h
index fb1ee51c7ec1de..17c449b6c2ec26 100644
--- a/clang/test/Analysis/Checkers/WebKit/mock-types.h
+++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h
@@ -46,7 +46,10 @@ template<typename T> struct DefaultRefDerefTraits {
template <typename T, typename PtrTraits = RawPtrTraits<T>, typename RefDerefTraits = DefaultRefDerefTraits<T>> struct Ref {
typename PtrTraits::StorageType t;
+ enum AdoptTag { Adopt };
+
Ref() : t{} {};
+ Ref(T &t, AdoptTag) : t(&t) { }
Ref(T &t) : t(&RefDerefTraits::ref(t)) { }
Ref(const Ref& o) : t(RefDerefTraits::refIfNotNull(PtrTraits::unwrap(o.t))) { }
Ref(Ref&& o) : t(o.leakRef()) { }
@@ -73,10 +76,19 @@ template <typename T, typename PtrTraits = RawPtrTraits<T>, typename RefDerefTra
T* leakRef() { return PtrTraits::exchange(t, nullptr); }
};
+template <typename T> Ref<T> adoptRef(T& t) {
+ using Ref = Ref<T>;
+ return Ref(t, Ref::Adopt);
+}
+
+template<typename T> class RefPtr;
+template<typename T> RefPtr<T> adoptRef(T*);
+
template <typename T> struct RefPtr {
T *t;
- RefPtr() : t(new T) {}
+ RefPtr() : t(nullptr) { }
+
RefPtr(T *t)
: t(t) {
if (t)
@@ -85,6 +97,9 @@ template <typename T> struct RefPtr {
RefPtr(Ref<T>&& o)
: t(o.leakRef())
{ }
+ RefPtr(RefPtr&& o)
+ : t(o.leakRef())
+ { }
~RefPtr() {
if (t)
t->deref();
@@ -110,8 +125,19 @@ template <typename T> struct RefPtr {
return *this;
}
operator bool() const { return t; }
+
+private:
+ friend RefPtr adoptRef<T>(T*);
+
+ // call_with_adopt_ref in call-args.cpp requires this method to be private.
+ enum AdoptTag { Adopt };
+ RefPtr(T *t, AdoptTag) : t(t) { }
};
+template <typename T> RefPtr<T> adoptRef(T* t) {
+ return RefPtr<T>(t, RefPtr<T>::Adopt);
+}
+
template <typename T> bool operator==(const RefPtr<T> &, const RefPtr<T> &) {
return false;
}
|
|
@llvm/pr-subscribers-clang Author: Ryosuke Niwa (rniwa) ChangesadoptRef in WebKit constructs Ref/RefPtr so treat it as such in isCtorOfRefCounted. Also removed the support for makeRef and makeRefPtr as they don't exist any more. Full diff: https://github.com/llvm/llvm-project/pull/119846.diff 3 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index 797f3e1f3fba5a..5487fea1b956c8 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -125,9 +125,8 @@ bool isCtorOfRefCounted(const clang::FunctionDecl *F) {
assert(F);
const std::string &FunctionName = safeGetName(F);
- return isRefType(FunctionName) || FunctionName == "makeRef" ||
- FunctionName == "makeRefPtr" || FunctionName == "UniqueRef" ||
- FunctionName == "makeUniqueRef" ||
+ return isRefType(FunctionName) || FunctionName == "adoptRef" ||
+ FunctionName == "UniqueRef" || FunctionName == "makeUniqueRef" ||
FunctionName == "makeUniqueRefWithoutFastMallocCheck"
|| FunctionName == "String" || FunctionName == "AtomString" ||
diff --git a/clang/test/Analysis/Checkers/WebKit/call-args.cpp b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
index 94efddeaf66cd8..574e3aa6ef476a 100644
--- a/clang/test/Analysis/Checkers/WebKit/call-args.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
@@ -365,3 +365,20 @@ namespace call_with_explicit_temporary_obj {
RefPtr { provide() }->method();
}
}
+
+namespace call_with_adopt_ref {
+ class Obj {
+ public:
+ void ref() const;
+ void deref() const;
+ void method();
+ };
+
+ struct dummy {
+ RefPtr<Obj> any;
+ };
+
+ void foo() {
+ adoptRef(new Obj)->method();
+ }
+}
diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h
index fb1ee51c7ec1de..17c449b6c2ec26 100644
--- a/clang/test/Analysis/Checkers/WebKit/mock-types.h
+++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h
@@ -46,7 +46,10 @@ template<typename T> struct DefaultRefDerefTraits {
template <typename T, typename PtrTraits = RawPtrTraits<T>, typename RefDerefTraits = DefaultRefDerefTraits<T>> struct Ref {
typename PtrTraits::StorageType t;
+ enum AdoptTag { Adopt };
+
Ref() : t{} {};
+ Ref(T &t, AdoptTag) : t(&t) { }
Ref(T &t) : t(&RefDerefTraits::ref(t)) { }
Ref(const Ref& o) : t(RefDerefTraits::refIfNotNull(PtrTraits::unwrap(o.t))) { }
Ref(Ref&& o) : t(o.leakRef()) { }
@@ -73,10 +76,19 @@ template <typename T, typename PtrTraits = RawPtrTraits<T>, typename RefDerefTra
T* leakRef() { return PtrTraits::exchange(t, nullptr); }
};
+template <typename T> Ref<T> adoptRef(T& t) {
+ using Ref = Ref<T>;
+ return Ref(t, Ref::Adopt);
+}
+
+template<typename T> class RefPtr;
+template<typename T> RefPtr<T> adoptRef(T*);
+
template <typename T> struct RefPtr {
T *t;
- RefPtr() : t(new T) {}
+ RefPtr() : t(nullptr) { }
+
RefPtr(T *t)
: t(t) {
if (t)
@@ -85,6 +97,9 @@ template <typename T> struct RefPtr {
RefPtr(Ref<T>&& o)
: t(o.leakRef())
{ }
+ RefPtr(RefPtr&& o)
+ : t(o.leakRef())
+ { }
~RefPtr() {
if (t)
t->deref();
@@ -110,8 +125,19 @@ template <typename T> struct RefPtr {
return *this;
}
operator bool() const { return t; }
+
+private:
+ friend RefPtr adoptRef<T>(T*);
+
+ // call_with_adopt_ref in call-args.cpp requires this method to be private.
+ enum AdoptTag { Adopt };
+ RefPtr(T *t, AdoptTag) : t(t) { }
};
+template <typename T> RefPtr<T> adoptRef(T* t) {
+ return RefPtr<T>(t, RefPtr<T>::Adopt);
+}
+
template <typename T> bool operator==(const RefPtr<T> &, const RefPtr<T> &) {
return false;
}
|
rniwa
commented
Dec 13, 2024
| }; | ||
|
|
||
| struct dummy { | ||
| RefPtr<Obj> any; |
There was a problem hiding this comment.
For whatever reason, this must exist for the bug to reproduce.
There was a problem hiding this comment.
@rniwa I ran this test case under the debugger and here's what I found: The call to IsPtrOriginSafe returns true in the absence of struct dummy when visiting the CallExpr causing the checker to not issue a warning:
CXXMemberCallExpr 0x147945af8 'void'
`-MemberExpr 0x147945ac8 '<bound member function type>' ->method 0x14793ccb0
`-CXXOperatorCallExpr 0x147945988 'class call_with_adopt_ref::Obj *' '->'
|-ImplicitCastExpr 0x147945970 'class call_with_adopt_ref::Obj *(*)(void) const' <FunctionToPointerDecay>
| `-DeclRefExpr 0x1479458e8 'class call_with_adopt_ref::Obj *(void) const' lvalue CXXMethod 0x147944388 'operator->' 'class call_with_adopt_ref::Obj *(void) const'
`-ImplicitCastExpr 0x1479458d0 'const struct RefPtr<class call_with_adopt_ref::Obj>' lvalue <NoOp>
`-MaterializeTemporaryExpr 0x1479458b8 'RefPtr<Obj>':'struct RefPtr<class call_with_adopt_ref::Obj>' lvalue
`-CXXBindTemporaryExpr 0x147945898 'RefPtr<Obj>':'struct RefPtr<class call_with_adopt_ref::Obj>' (CXXTemporary 0x147945898)
`-CallExpr 0x1479407e8 'RefPtr<Obj>':'struct RefPtr<class call_with_adopt_ref::Obj>'
|-ImplicitCastExpr 0x1479407d0 'RefPtr<Obj> (*)(class call_with_adopt_ref::Obj *)' <FunctionToPointerDecay>
| `-DeclRefExpr 0x147940740 'RefPtr<Obj> (class call_with_adopt_ref::Obj *)' lvalue Function 0x14793fe00 'adoptRef' 'RefPtr<Obj> (class call_with_adopt_ref::Obj *)' (FunctionTemplate 0x14792ff48 'adoptRef')
`-CXXNewExpr 0x14793f408 'Obj *' Function 0x14793d0c8 'operator new' 'void *(unsigned long)'
`-CXXConstructExpr 0x14793f3e0 'Obj':'class call_with_adopt_ref::Obj' 'void (void) noexcept'
Does this give any clue for the unexpected behavior?
There was a problem hiding this comment.
It's still unclear to me why the presence of struct dummy affects the semantics of seemingly unrelated code.
There was a problem hiding this comment.
Looks like there is some bug in AST generation. The return type of callee is different for both the versions at
With dummy struct:
(lldb) p callee->getReturnType()->dump()
ElaboratedType 0x1309723b0 'RefPtr<Obj>' sugar
`-RecordType 0x130971240 'struct RefPtr<class call_with_adopt_ref::Obj>'
`-ClassTemplateSpecialization 0x130971160 'RefPtr'
Without the dummy struct:
(lldb) p callee->getReturnType()->dump()
ElaboratedType 0x12c973230 'RefPtr<Obj>' sugar
`-TemplateSpecializationType 0x12c9731e0 'RefPtr<class call_with_adopt_ref::Obj>' sugar
|-name: 'RefPtr' qualified
| `-ClassTemplateDecl 0x12c95ee30 prev 0x12c95e690 RefPtr
|-TemplateArgument type 'class call_with_adopt_ref::Obj'
| `-SubstTemplateTypeParmType 0x12c9730a0 'class call_with_adopt_ref::Obj' sugar typename depth 0 index 0 T
| |-FunctionTemplate 0x12c95ec18 'adoptRef'
| `-RecordType 0x12c970560 'class call_with_adopt_ref::Obj'
| `-CXXRecord 0x12c9704d0 'Obj'
`-RecordType 0x12c9731c0 'struct RefPtr<class call_with_adopt_ref::Obj>'
`-ClassTemplateSpecialization 0x12c9730e0 'RefPtr'
Maybe create a separate radar for this issue and add the radar ID to the comment in the test case?
Otherwise, LGTM!
There was a problem hiding this comment.
Filed rdar://141692212. Will add a comment to that end. Thanks for the review!
t-rasmud
approved these changes
Dec 18, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/14386 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/3/builds/9429 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/13452 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/3730 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/9652 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/133/builds/8703 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/174/builds/10391 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/9323 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/11720 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/11/builds/10006 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/56/builds/14950 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/18106 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/60/builds/15666 Here is the relevant piece of the build log for the reference |
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.
adoptRef in WebKit constructs Ref/RefPtr so treat it as such in isCtorOfRefCounted. Also removed the support for makeRef and makeRefPtr as they don't exist any more.