[clang] [webkit.UncountedLambdaCapturesChecker] Recognize nested protectedThis pattern (PR #126443)
via cfe-commits
cfe-commits at lists.llvm.org
Sun Feb 9 14:03:15 PST 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: Ryosuke Niwa (rniwa)
<details>
<summary>Changes</summary>
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.
---
Full diff: https://github.com/llvm/llvm-project/pull/126443.diff
2 Files Affected:
- (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp (+71-47)
- (modified) clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp (+24)
``````````diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
index a56f48c83c660a9..c0a9e38b6aab41b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
@@ -41,6 +41,7 @@ class UncountedLambdaCapturesChecker
const UncountedLambdaCapturesChecker *Checker;
llvm::DenseSet<const DeclRefExpr *> DeclRefExprsToIgnore;
llvm::DenseSet<const LambdaExpr *> LambdasToIgnore;
+ llvm::DenseSet<const ValueDecl *> ProtectedThisDecls;
QualType ClsType;
explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker)
@@ -65,7 +66,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;
}
@@ -93,7 +94,7 @@ class UncountedLambdaCapturesChecker
if (!L)
return true;
LambdasToIgnore.insert(L);
- Checker->visitLambdaExpr(L, shouldCheckThis());
+ Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L));
return true;
}
@@ -118,7 +119,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;
}
@@ -145,6 +147,11 @@ class UncountedLambdaCapturesChecker
return nullptr;
if (auto *Lambda = dyn_cast<LambdaExpr>(CtorArg))
return Lambda;
+ if (auto *TempExpr = dyn_cast<CXXBindTemporaryExpr>(CtorArg)) {
+ E = TempExpr->getSubExpr()->IgnoreParenCasts();
+ if (auto *Lambda = dyn_cast<LambdaExpr>(E))
+ return Lambda;
+ }
auto *DRE = dyn_cast<DeclRefExpr>(CtorArg);
if (!DRE)
return nullptr;
@@ -189,9 +196,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);
@@ -214,53 +280,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 4f4a96028225303..4895879c4a385db 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();
@@ -299,6 +300,29 @@ struct RefCountableWithLambdaCapturingThis {
return obj->next();
});
}
+
+ 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();
+ });
+ });
+ }
};
struct NonRefCountableWithLambdaCapturingThis {
``````````
</details>
https://github.com/llvm/llvm-project/pull/126443
More information about the cfe-commits
mailing list