[webkit.UncountedLambdaCapturesChecker] Support [[clang::noescape]] on a constructor #126869
Conversation
This file contains hidden or 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
…n a constructor Added the support for annotating a constructor's argument with [[clang::noescape]]. We explicitly ignore CXXConstructExpr which is visited as a part of CallExpr so that construction of closures like Function, CompletionHandler, etc... don't result in a warning.
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang Author: Ryosuke Niwa (rniwa) ChangesAdded the support for annotating a constructor's argument with [[clang::noescape]]. We explicitly ignore CXXConstructExpr which is visited as a part of CallExpr so that construction of closures like Function, CompletionHandler, etc... don't result in a warning. Full diff: https://github.com/llvm/llvm-project/pull/126869.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
index a56f48c83c660..972364a855eb4 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 CXXConstructExpr *> ConstructToIgnore;
QualType ClsType;
explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker)
@@ -106,6 +107,26 @@ class UncountedLambdaCapturesChecker
return safeGetName(NsDecl) == "WTF" && safeGetName(Decl) == "switchOn";
}
+ bool VisitCXXConstructExpr(CXXConstructExpr *CE) override {
+ if (ConstructToIgnore.contains(CE))
+ return true;
+ if (auto *Callee = CE->getConstructor()) {
+ unsigned ArgIndex = 0;
+ for (auto *Param : Callee->parameters()) {
+ if (ArgIndex >= CE->getNumArgs())
+ return true;
+ auto *Arg = CE->getArg(ArgIndex)->IgnoreParenCasts();
+ if (auto *L = findLambdaInArg(Arg)) {
+ LambdasToIgnore.insert(L);
+ if (!Param->hasAttr<NoEscapeAttr>())
+ Checker->visitLambdaExpr(L, shouldCheckThis());
+ }
+ ++ArgIndex;
+ }
+ }
+ return true;
+ }
+
bool VisitCallExpr(CallExpr *CE) override {
checkCalleeLambda(CE);
if (auto *Callee = CE->getDirectCallee()) {
@@ -143,8 +164,10 @@ class UncountedLambdaCapturesChecker
auto *CtorArg = CE->getArg(0)->IgnoreParenCasts();
if (!CtorArg)
return nullptr;
- if (auto *Lambda = dyn_cast<LambdaExpr>(CtorArg))
+ if (auto *Lambda = dyn_cast<LambdaExpr>(CtorArg)) {
+ ConstructToIgnore.insert(CE);
return Lambda;
+ }
auto *DRE = dyn_cast<DeclRefExpr>(CtorArg);
if (!DRE)
return nullptr;
@@ -157,6 +180,7 @@ class UncountedLambdaCapturesChecker
TempExpr = dyn_cast<CXXBindTemporaryExpr>(Init->IgnoreParenCasts());
if (!TempExpr)
return nullptr;
+ ConstructToIgnore.insert(CE);
return dyn_cast_or_null<LambdaExpr>(TempExpr->getSubExpr());
}
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
index 4f4a960282253..62a8245db99b9 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
@@ -298,7 +298,11 @@ struct RefCountableWithLambdaCapturingThis {
callLambda([&]() -> RefPtr<RefCountable> {
return obj->next();
});
+ WTF::HashMap<int, RefPtr<RefCountable>> anotherMap([&] {
+ return obj->next();
+ });
}
+
};
struct NonRefCountableWithLambdaCapturingThis {
|
Member
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Ryosuke Niwa (rniwa) ChangesAdded the support for annotating a constructor's argument with [[clang::noescape]]. We explicitly ignore CXXConstructExpr which is visited as a part of CallExpr so that construction of closures like Function, CompletionHandler, etc... don't result in a warning. Full diff: https://github.com/llvm/llvm-project/pull/126869.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
index a56f48c83c660..972364a855eb4 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 CXXConstructExpr *> ConstructToIgnore;
QualType ClsType;
explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker)
@@ -106,6 +107,26 @@ class UncountedLambdaCapturesChecker
return safeGetName(NsDecl) == "WTF" && safeGetName(Decl) == "switchOn";
}
+ bool VisitCXXConstructExpr(CXXConstructExpr *CE) override {
+ if (ConstructToIgnore.contains(CE))
+ return true;
+ if (auto *Callee = CE->getConstructor()) {
+ unsigned ArgIndex = 0;
+ for (auto *Param : Callee->parameters()) {
+ if (ArgIndex >= CE->getNumArgs())
+ return true;
+ auto *Arg = CE->getArg(ArgIndex)->IgnoreParenCasts();
+ if (auto *L = findLambdaInArg(Arg)) {
+ LambdasToIgnore.insert(L);
+ if (!Param->hasAttr<NoEscapeAttr>())
+ Checker->visitLambdaExpr(L, shouldCheckThis());
+ }
+ ++ArgIndex;
+ }
+ }
+ return true;
+ }
+
bool VisitCallExpr(CallExpr *CE) override {
checkCalleeLambda(CE);
if (auto *Callee = CE->getDirectCallee()) {
@@ -143,8 +164,10 @@ class UncountedLambdaCapturesChecker
auto *CtorArg = CE->getArg(0)->IgnoreParenCasts();
if (!CtorArg)
return nullptr;
- if (auto *Lambda = dyn_cast<LambdaExpr>(CtorArg))
+ if (auto *Lambda = dyn_cast<LambdaExpr>(CtorArg)) {
+ ConstructToIgnore.insert(CE);
return Lambda;
+ }
auto *DRE = dyn_cast<DeclRefExpr>(CtorArg);
if (!DRE)
return nullptr;
@@ -157,6 +180,7 @@ class UncountedLambdaCapturesChecker
TempExpr = dyn_cast<CXXBindTemporaryExpr>(Init->IgnoreParenCasts());
if (!TempExpr)
return nullptr;
+ ConstructToIgnore.insert(CE);
return dyn_cast_or_null<LambdaExpr>(TempExpr->getSubExpr());
}
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
index 4f4a960282253..62a8245db99b9 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
@@ -298,7 +298,11 @@ struct RefCountableWithLambdaCapturingThis {
callLambda([&]() -> RefPtr<RefCountable> {
return obj->next();
});
+ WTF::HashMap<int, RefPtr<RefCountable>> anotherMap([&] {
+ return obj->next();
+ });
}
+
};
struct NonRefCountableWithLambdaCapturingThis {
|
t-rasmud
reviewed
Feb 14, 2025
| if (ArgIndex >= CE->getNumArgs()) | ||
| return true; | ||
| auto *Arg = CE->getArg(ArgIndex)->IgnoreParenCasts(); | ||
| if (auto *L = findLambdaInArg(Arg)) { |
Contributor
There was a problem hiding this comment.
For my understanding: Is the LambdaExpr not covered in VisitLambdaExpr? What's special about the expression being an argument to a constructor?
Contributor
Author
There was a problem hiding this comment.
Oh, so we wanna ignore lambda expressions that are passed to [[clang::noescape]] argument. The code in VisitCallExpr and now VisitCXXConstructExpr is there to detect this specific case, and then ignore the argument.
Contributor
Author
|
Thanks for the review! |
rniwa
added a commit
to rniwa/llvm-project
that referenced
this pull request
Feb 15, 2025
…n a constructor (llvm#126869) Added the support for annotating a constructor's argument with [[clang::noescape]]. We explicitly ignore CXXConstructExpr which is visited as a part of CallExpr so that construction of closures like Function, CompletionHandler, etc... don't result in a warning.
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Feb 24, 2025
…n a constructor (llvm#126869) Added the support for annotating a constructor's argument with [[clang::noescape]]. We explicitly ignore CXXConstructExpr which is visited as a part of CallExpr so that construction of closures like Function, CompletionHandler, etc... don't result in a warning.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Added the support for annotating a constructor's argument with [[clang::noescape]].
We explicitly ignore CXXConstructExpr which is visited as a part of CallExpr so that construction of closures like Function, CompletionHandler, etc... don't result in a warning.