[clang] 77041da - [webkit.UncountedLambdaCapturesChecker] Recognize nested protectedThis pattern (#126443)
via cfe-commits
cfe-commits at lists.llvm.org
Fri Feb 14 16:33:37 PST 2025
Author: Ryosuke Niwa
Date: 2025-02-14T16:33:34-08:00
New Revision: 77041da98932f77896d48366703d956ae7a82036
URL: https://github.com/llvm/llvm-project/commit/77041da98932f77896d48366703d956ae7a82036
DIFF: https://github.com/llvm/llvm-project/commit/77041da98932f77896d48366703d956ae7a82036.diff
LOG: [webkit.UncountedLambdaCapturesChecker] Recognize nested protectedThis pattern (#126443)
In WebKit, it's pretty common to capture "this" and "protectedThis"
where "protectedThis" is a guardian variable of type Ref or RefPtr for
"this". Furthermore, it's common for this "protectedThis" variable from
being passed to an inner lambda by std::move. Recognize this pattern so
that we don't emit warnings for nested inner lambdas.
To recognize this pattern, we introduce a new DenseSet,
ProtectedThisDecls, which contains every "protectedThis" we've
recognized to our subclass of DynamicRecursiveASTVisitor. This set is
now populated in "hasProtectedThis" and "declProtectsThis" uses this
DenseSet to determine a given value declaration constitutes a
"protectedThis" pattern or not.
Because hasProtectedThis and declProtectsThis had to be moved from the
checker class to the visitor class, it's now a responsibility of each
caller of visitLambdaExpr to check whether a given lambda captures
"this" without a "protectedThis" or not.
Finally, this PR improves the code to recognize "protectedThis" pattern
by allowing more nested CXXBindTemporaryExpr, CXXOperatorCallExpr, and
UnaryOperator expressions.
Added:
Modified:
clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
index 972364a855eb4..4ffdac5ca4873 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
@@ -41,7 +41,9 @@ class UncountedLambdaCapturesChecker
const UncountedLambdaCapturesChecker *Checker;
llvm::DenseSet<const DeclRefExpr *> DeclRefExprsToIgnore;
llvm::DenseSet<const LambdaExpr *> LambdasToIgnore;
+ llvm::DenseSet<const ValueDecl *> ProtectedThisDecls;
llvm::DenseSet<const CXXConstructExpr *> ConstructToIgnore;
+
QualType ClsType;
explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker)
@@ -66,7 +68,7 @@ class UncountedLambdaCapturesChecker
bool VisitLambdaExpr(LambdaExpr *L) override {
if (LambdasToIgnore.contains(L))
return true;
- Checker->visitLambdaExpr(L, shouldCheckThis());
+ Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L));
return true;
}
@@ -94,7 +96,7 @@ class UncountedLambdaCapturesChecker
if (!L)
return true;
LambdasToIgnore.insert(L);
- Checker->visitLambdaExpr(L, shouldCheckThis());
+ Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L));
return true;
}
@@ -119,7 +121,8 @@ class UncountedLambdaCapturesChecker
if (auto *L = findLambdaInArg(Arg)) {
LambdasToIgnore.insert(L);
if (!Param->hasAttr<NoEscapeAttr>())
- Checker->visitLambdaExpr(L, shouldCheckThis());
+ Checker->visitLambdaExpr(L, shouldCheckThis() &&
+ !hasProtectedThis(L));
}
++ArgIndex;
}
@@ -139,7 +142,8 @@ class UncountedLambdaCapturesChecker
if (auto *L = findLambdaInArg(Arg)) {
LambdasToIgnore.insert(L);
if (!Param->hasAttr<NoEscapeAttr>() && !TreatAllArgsAsNoEscape)
- Checker->visitLambdaExpr(L, shouldCheckThis());
+ Checker->visitLambdaExpr(L, shouldCheckThis() &&
+ !hasProtectedThis(L));
}
++ArgIndex;
}
@@ -168,6 +172,13 @@ class UncountedLambdaCapturesChecker
ConstructToIgnore.insert(CE);
return Lambda;
}
+ if (auto *TempExpr = dyn_cast<CXXBindTemporaryExpr>(CtorArg)) {
+ E = TempExpr->getSubExpr()->IgnoreParenCasts();
+ if (auto *Lambda = dyn_cast<LambdaExpr>(E)) {
+ ConstructToIgnore.insert(CE);
+ return Lambda;
+ }
+ }
auto *DRE = dyn_cast<DeclRefExpr>(CtorArg);
if (!DRE)
return nullptr;
@@ -213,9 +224,68 @@ class UncountedLambdaCapturesChecker
return;
DeclRefExprsToIgnore.insert(ArgRef);
LambdasToIgnore.insert(L);
- Checker->visitLambdaExpr(L, shouldCheckThis(),
+ Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L),
/* ignoreParamVarDecl */ true);
}
+
+ bool hasProtectedThis(LambdaExpr *L) {
+ for (const LambdaCapture &OtherCapture : L->captures()) {
+ if (!OtherCapture.capturesVariable())
+ continue;
+ if (auto *ValueDecl = OtherCapture.getCapturedVar()) {
+ if (declProtectsThis(ValueDecl)) {
+ ProtectedThisDecls.insert(ValueDecl);
+ return true;
+ }
+ }
+ }
+ return false;
+ }
+
+ bool declProtectsThis(const ValueDecl *ValueDecl) const {
+ auto *VD = dyn_cast<VarDecl>(ValueDecl);
+ if (!VD)
+ return false;
+ auto *Init = VD->getInit()->IgnoreParenCasts();
+ if (!Init)
+ return false;
+ const Expr *Arg = Init;
+ do {
+ if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Arg))
+ Arg = BTE->getSubExpr()->IgnoreParenCasts();
+ if (auto *CE = dyn_cast_or_null<CXXConstructExpr>(Arg)) {
+ auto *Ctor = CE->getConstructor();
+ if (!Ctor)
+ return false;
+ auto clsName = safeGetName(Ctor->getParent());
+ if (!isRefType(clsName) || !CE->getNumArgs())
+ return false;
+ Arg = CE->getArg(0)->IgnoreParenCasts();
+ continue;
+ }
+ if (auto *OpCE = dyn_cast<CXXOperatorCallExpr>(Arg)) {
+ auto OpCode = OpCE->getOperator();
+ if (OpCode == OO_Star || OpCode == OO_Amp) {
+ auto *Callee = OpCE->getDirectCallee();
+ auto clsName = safeGetName(Callee->getParent());
+ if (!isRefType(clsName) || !OpCE->getNumArgs())
+ return false;
+ Arg = OpCE->getArg(0)->IgnoreParenCasts();
+ continue;
+ }
+ }
+ if (auto *UO = dyn_cast<UnaryOperator>(Arg)) {
+ auto OpCode = UO->getOpcode();
+ if (OpCode == UO_Deref || OpCode == UO_AddrOf)
+ Arg = UO->getSubExpr()->IgnoreParenCasts();
+ continue;
+ }
+ break;
+ } while (Arg);
+ if (auto *DRE = dyn_cast<DeclRefExpr>(Arg))
+ return ProtectedThisDecls.contains(DRE->getDecl());
+ return isa<CXXThisExpr>(Arg);
+ }
};
LocalVisitor visitor(this);
@@ -238,53 +308,11 @@ class UncountedLambdaCapturesChecker
} else if (C.capturesThis() && shouldCheckThis) {
if (ignoreParamVarDecl) // this is always a parameter to this function.
continue;
- bool hasProtectThis = false;
- for (const LambdaCapture &OtherCapture : L->captures()) {
- if (!OtherCapture.capturesVariable())
- continue;
- if (auto *ValueDecl = OtherCapture.getCapturedVar()) {
- if (protectThis(ValueDecl)) {
- hasProtectThis = true;
- break;
- }
- }
- }
- if (!hasProtectThis)
- reportBugOnThisPtr(C);
+ reportBugOnThisPtr(C);
}
}
}
- bool protectThis(const ValueDecl *ValueDecl) const {
- auto *VD = dyn_cast<VarDecl>(ValueDecl);
- if (!VD)
- return false;
- auto *Init = VD->getInit()->IgnoreParenCasts();
- if (!Init)
- return false;
- auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Init);
- if (!BTE)
- return false;
- auto *CE = dyn_cast_or_null<CXXConstructExpr>(BTE->getSubExpr());
- if (!CE)
- return false;
- auto *Ctor = CE->getConstructor();
- if (!Ctor)
- return false;
- auto clsName = safeGetName(Ctor->getParent());
- if (!isRefType(clsName) || !CE->getNumArgs())
- return false;
- auto *Arg = CE->getArg(0)->IgnoreParenCasts();
- while (auto *UO = dyn_cast<UnaryOperator>(Arg)) {
- auto OpCode = UO->getOpcode();
- if (OpCode == UO_Deref || OpCode == UO_AddrOf)
- Arg = UO->getSubExpr();
- else
- break;
- }
- return isa<CXXThisExpr>(Arg);
- }
-
void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar,
const QualType T) const {
assert(CapturedVar);
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
index 62a8245db99b9..82058bf13d137 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
@@ -89,6 +89,7 @@ template <typename Callback> void call(Callback callback) {
someFunction();
callback();
}
+void callAsync(const WTF::Function<void()>&);
void raw_ptr() {
RefCountable* ref_countable = make_obj();
@@ -303,6 +304,37 @@ struct RefCountableWithLambdaCapturingThis {
});
}
+ void callAsyncNoescape([[clang::noescape]] WTF::Function<bool(RefCountable&)>&&);
+ void method_temp_lambda(RefCountable* obj) {
+ callAsyncNoescape([this, otherObj = RefPtr { obj }](auto& obj) {
+ return otherObj == &obj;
+ });
+ }
+
+ void method_nested_lambda() {
+ callAsync([this, protectedThis = Ref { *this }] {
+ callAsync([this, protectedThis = static_cast<const Ref<RefCountableWithLambdaCapturingThis>&&>(protectedThis)] {
+ nonTrivial();
+ });
+ });
+ }
+
+ void method_nested_lambda2() {
+ callAsync([this, protectedThis = RefPtr { this }] {
+ callAsync([this, protectedThis = static_cast<const Ref<RefCountableWithLambdaCapturingThis>&&>(*protectedThis)] {
+ nonTrivial();
+ });
+ });
+ }
+
+ void method_nested_lambda3() {
+ callAsync([this, protectedThis = RefPtr { this }] {
+ callAsync([this] {
+ // expected-warning at -1{{Captured raw-pointer 'this' to ref-counted type or CheckedPtr-capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+ nonTrivial();
+ });
+ });
+ }
};
struct NonRefCountableWithLambdaCapturingThis {
More information about the cfe-commits
mailing list