Skip to content

Commit 203a2ca

Browse files
rniwarniwa
authored
[webkit.RefCntblBaseVirtualDtor] Make ThreadSafeRefCounted not generate warnings (llvm#107676)
This PR makes WebKit's RefCntblBaseVirtualDtor checker not generate a warning for ThreadSafeRefCounted when the destruction thread is a specific thread. Prior to this PR, we only allowed CRTP classes without a virtual destructor if its deref function had an explicit cast to the derived type, skipping any lambda declarations which aren't invoked. This ends up generating a warning for ThreadSafeRefCounted when a specific thread is used to destruct the object because there is no inline body / definition for ensureOnMainThread and ensureOnMainRunLoop and DerefFuncDeleteExprVisitor concludes that there is no explicit delete of the derived type. This PR relaxes the condition DerefFuncDeleteExprVisitor checks by allowing a delete expression to appear within a lambda declaration if it's an argument to an "opaque" function; i.e. a function without definition / body.
1 parent 323911d commit 203a2ca

File tree

2 files changed

+258
-0
lines changed

2 files changed

+258
-0
lines changed

‎clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp

+26
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,32 @@ class DerefFuncDeleteExprVisitor
6767
const Decl *D = CE->getCalleeDecl();
6868
if (D && D->hasBody())
6969
return VisitBody(D->getBody());
70+
else {
71+
auto name = safeGetName(D);
72+
if (name == "ensureOnMainThread" || name == "ensureOnMainRunLoop") {
73+
for (unsigned i = 0; i < CE->getNumArgs(); ++i) {
74+
auto *Arg = CE->getArg(i);
75+
if (VisitLabmdaArgument(Arg))
76+
return true;
77+
}
78+
}
79+
}
80+
return false;
81+
}
82+
83+
bool VisitLabmdaArgument(const Expr *E) {
84+
E = E->IgnoreParenCasts();
85+
if (auto *TempE = dyn_cast<CXXBindTemporaryExpr>(E))
86+
E = TempE->getSubExpr();
87+
if (auto *ConstructE = dyn_cast<CXXConstructExpr>(E)) {
88+
for (unsigned i = 0; i < ConstructE->getNumArgs(); ++i) {
89+
auto *Arg = ConstructE->getArg(i);
90+
if (auto *Lambda = dyn_cast<LambdaExpr>(Arg)) {
91+
if (VisitBody(Lambda->getBody()))
92+
return true;
93+
}
94+
}
95+
}
7096
return false;
7197
}
7298

‎clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp

+232
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,232 @@
1+
// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.RefCntblBaseVirtualDtor -verify %s
2+
3+
#include "mock-types.h"
4+
5+
namespace Detail {
6+
7+
template<typename Out, typename... In>
8+
class CallableWrapperBase {
9+
public:
10+
virtual ~CallableWrapperBase() { }
11+
virtual Out call(In...) = 0;
12+
};
13+
14+
template<typename, typename, typename...> class CallableWrapper;
15+
16+
template<typename CallableType, typename Out, typename... In>
17+
class CallableWrapper : public CallableWrapperBase<Out, In...> {
18+
public:
19+
explicit CallableWrapper(CallableType&& callable)
20+
: m_callable(WTFMove(callable)) { }
21+
CallableWrapper(const CallableWrapper&) = delete;
22+
CallableWrapper& operator=(const CallableWrapper&) = delete;
23+
Out call(In... in) final;
24+
private:
25+
CallableType m_callable;
26+
};
27+
28+
} // namespace Detail
29+
30+
template<typename> class Function;
31+
32+
template<typename Out, typename... In> Function<Out(In...)> adopt(Detail::CallableWrapperBase<Out, In...>*);
33+
34+
template <typename Out, typename... In>
35+
class Function<Out(In...)> {
36+
public:
37+
using Impl = Detail::CallableWrapperBase<Out, In...>;
38+
39+
Function() = default;
40+
41+
template<typename FunctionType>
42+
Function(FunctionType f);
43+
44+
Out operator()(In... in) const;
45+
explicit operator bool() const { return !!m_callableWrapper; }
46+
47+
private:
48+
enum AdoptTag { Adopt };
49+
Function(Impl* impl, AdoptTag)
50+
: m_callableWrapper(impl)
51+
{
52+
}
53+
54+
friend Function adopt<Out, In...>(Impl*);
55+
56+
Impl* m_callableWrapper;
57+
};
58+
59+
template<typename Out, typename... In> Function<Out(In...)> adopt(Detail::CallableWrapperBase<Out, In...>* impl)
60+
{
61+
return Function<Out(In...)>(impl, Function<Out(In...)>::Adopt);
62+
}
63+
64+
template<typename T, typename PtrTraits = RawPtrTraits<T>, typename RefDerefTraits = DefaultRefDerefTraits<T>> Ref<T, PtrTraits, RefDerefTraits> adoptRef(T&);
65+
66+
template<typename T, typename _PtrTraits, typename RefDerefTraits>
67+
inline Ref<T, _PtrTraits, RefDerefTraits> adoptRef(T& reference)
68+
{
69+
return Ref<T, _PtrTraits, RefDerefTraits>(reference);
70+
}
71+
72+
enum class DestructionThread : unsigned char { Any, Main, MainRunLoop };
73+
void ensureOnMainThread(Function<void()>&&); // Sync if called on main thread, async otherwise.
74+
void ensureOnMainRunLoop(Function<void()>&&); // Sync if called on main run loop, async otherwise.
75+
76+
class ThreadSafeRefCountedBase {
77+
public:
78+
ThreadSafeRefCountedBase() = default;
79+
80+
void ref() const
81+
{
82+
++m_refCount;
83+
}
84+
85+
bool hasOneRef() const
86+
{
87+
return refCount() == 1;
88+
}
89+
90+
unsigned refCount() const
91+
{
92+
return m_refCount;
93+
}
94+
95+
protected:
96+
bool derefBase() const
97+
{
98+
if (!--m_refCount) {
99+
m_refCount = 1;
100+
return true;
101+
}
102+
return false;
103+
}
104+
105+
private:
106+
mutable unsigned m_refCount { 1 };
107+
};
108+
109+
template<class T, DestructionThread destructionThread = DestructionThread::Any> class ThreadSafeRefCounted : public ThreadSafeRefCountedBase {
110+
public:
111+
void deref() const
112+
{
113+
if (!derefBase())
114+
return;
115+
116+
if constexpr (destructionThread == DestructionThread::Any) {
117+
delete static_cast<const T*>(this);
118+
} else if constexpr (destructionThread == DestructionThread::Main) {
119+
ensureOnMainThread([this] {
120+
delete static_cast<const T*>(this);
121+
});
122+
}
123+
}
124+
125+
protected:
126+
ThreadSafeRefCounted() = default;
127+
};
128+
129+
class FancyRefCountedClass final : public ThreadSafeRefCounted<FancyRefCountedClass, DestructionThread::Main> {
130+
public:
131+
static Ref<FancyRefCountedClass> create()
132+
{
133+
return adoptRef(*new FancyRefCountedClass());
134+
}
135+
136+
virtual ~FancyRefCountedClass();
137+
138+
private:
139+
FancyRefCountedClass();
140+
};
141+
142+
template<class T, DestructionThread destructionThread = DestructionThread::Any> class BadThreadSafeRefCounted : public ThreadSafeRefCountedBase {
143+
public:
144+
void deref() const
145+
{
146+
if (!derefBase())
147+
return;
148+
149+
[this] {
150+
delete static_cast<const T*>(this);
151+
};
152+
}
153+
154+
protected:
155+
BadThreadSafeRefCounted() = default;
156+
};
157+
158+
class FancyRefCountedClass2 final : public ThreadSafeRefCounted<FancyRefCountedClass, DestructionThread::Main> {
159+
// expected-warning@-1{{Class 'ThreadSafeRefCounted<FancyRefCountedClass, DestructionThread::Main>' is used as a base of class 'FancyRefCountedClass2' but doesn't have virtual destructor}}
160+
public:
161+
static Ref<FancyRefCountedClass2> create()
162+
{
163+
return adoptRef(*new FancyRefCountedClass2());
164+
}
165+
166+
virtual ~FancyRefCountedClass2();
167+
168+
private:
169+
FancyRefCountedClass2();
170+
};
171+
172+
template<class T, DestructionThread destructionThread = DestructionThread::Any> class NestedThreadSafeRefCounted : public ThreadSafeRefCountedBase {
173+
public:
174+
void deref() const
175+
{
176+
if (!derefBase())
177+
return;
178+
ensureOnMainRunLoop([&] {
179+
auto destroyThis = [&] {
180+
delete static_cast<const T*>(this);
181+
};
182+
destroyThis();
183+
});
184+
}
185+
186+
protected:
187+
NestedThreadSafeRefCounted() = default;
188+
};
189+
190+
class FancyRefCountedClass3 final : public NestedThreadSafeRefCounted<FancyRefCountedClass3, DestructionThread::Main> {
191+
public:
192+
static Ref<FancyRefCountedClass3> create()
193+
{
194+
return adoptRef(*new FancyRefCountedClass3());
195+
}
196+
197+
virtual ~FancyRefCountedClass3();
198+
199+
private:
200+
FancyRefCountedClass3();
201+
};
202+
203+
template<class T, DestructionThread destructionThread = DestructionThread::Any> class BadNestedThreadSafeRefCounted : public ThreadSafeRefCountedBase {
204+
public:
205+
void deref() const
206+
{
207+
if (!derefBase())
208+
return;
209+
ensureOnMainThread([&] {
210+
auto destroyThis = [&] {
211+
delete static_cast<const T*>(this);
212+
};
213+
});
214+
}
215+
216+
protected:
217+
BadNestedThreadSafeRefCounted() = default;
218+
};
219+
220+
class FancyRefCountedClass4 final : public BadNestedThreadSafeRefCounted<FancyRefCountedClass4, DestructionThread::Main> {
221+
// expected-warning@-1{{Class 'BadNestedThreadSafeRefCounted<FancyRefCountedClass4, DestructionThread::Main>' is used as a base of class 'FancyRefCountedClass4' but doesn't have virtual destructor}}
222+
public:
223+
static Ref<FancyRefCountedClass4> create()
224+
{
225+
return adoptRef(*new FancyRefCountedClass4());
226+
}
227+
228+
virtual ~FancyRefCountedClass4();
229+
230+
private:
231+
FancyRefCountedClass4();
232+
};

0 commit comments

Comments
 (0)