https://github.com/rniwa updated
https://github.com/llvm/llvm-project/pull/126443
>From a40e782b866ec4756ddf3f04cc09caff31845ebf Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rn...@webkit.org>
Date: Sun, 9 Feb 2025 13:50:26 -0800
Subject: [PATCH 1/4] [webkit.UncountedLambdaCapturesChecker] Recognize nested
protectedThis pattern
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.
---
.../WebKit/UncountedLambdaCapturesChecker.cpp | 118 +++++++++++-------
.../WebKit/uncounted-lambda-captures.cpp | 24 ++++
2 files changed, 95 insertions(+), 47 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
index a56f48c83c660..c0a9e38b6aab4 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 4f4a960282253..4895879c4a385 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 {
>From 822c46945d085487e813d2bd60f4feabdb20f818 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rn...@webkit.org>
Date: Sun, 9 Feb 2025 20:47:32 -0800
Subject: [PATCH 2/4] Fix formatting
---
.../Checkers/WebKit/UncountedLambdaCapturesChecker.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
index c0a9e38b6aab4..e3eeac8b98a63 100644
---
a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
+++
b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
@@ -120,7 +120,7 @@ class UncountedLambdaCapturesChecker
LambdasToIgnore.insert(L);
if (!Param->hasAttr<NoEscapeAttr>() && !TreatAllArgsAsNoEscape)
Checker->visitLambdaExpr(L, shouldCheckThis() &&
- !hasProtectedThis(L));
+ !hasProtectedThis(L));
}
++ArgIndex;
}
>From 13b739ba1e49771ba3b991055ab09701847711e5 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rn...@webkit.org>
Date: Fri, 14 Feb 2025 14:36:47 -0800
Subject: [PATCH 3/4] Add a nested lambda test case where warning will be
emitted
---
.../Checkers/WebKit/uncounted-lambda-captures.cpp | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
index 4895879c4a385..4314cd64bb402 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
@@ -323,6 +323,15 @@ struct RefCountableWithLambdaCapturingThis {
});
});
}
+
+ void method_nested_lambda3() {
+ callAsync([this, protectedThis = RefPtr { this }] {
+ callAsync([this] {
+ // expected-warning@-1{{Captured raw-pointer 'this' to ref-counted
type or CheckedPtr-capable type is unsafe
[webkit.UncountedLambdaCapturesChecker]}}
+ nonTrivial();
+ });
+ });
+ }
};
struct NonRefCountableWithLambdaCapturingThis {
>From eb59ac9aab27d1c3ec1ce687b09836274fc49ea4 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rn...@webkit.org>
Date: Fri, 14 Feb 2025 15:53:22 -0800
Subject: [PATCH 4/4] Fix merge error
---
.../Checkers/WebKit/UncountedLambdaCapturesChecker.cpp | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
index b282f515f0bc8..4ffdac5ca4873 100644
---
a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
+++
b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
@@ -121,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;
}
@@ -173,8 +174,10 @@ class UncountedLambdaCapturesChecker
}
if (auto *TempExpr = dyn_cast<CXXBindTemporaryExpr>(CtorArg)) {
E = TempExpr->getSubExpr()->IgnoreParenCasts();
- if (auto *Lambda = dyn_cast<LambdaExpr>(E))
+ if (auto *Lambda = dyn_cast<LambdaExpr>(E)) {
+ ConstructToIgnore.insert(CE);
return Lambda;
+ }
}
auto *DRE = dyn_cast<DeclRefExpr>(CtorArg);
if (!DRE)
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
