[webkit.UncountedLambdaCapturesChecker] Ignore trivial functions and [[clang::noescape]]. #114897
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
…[[clang::noescape]]. This PR makes webkit.UncountedLambdaCapturesChecker ignore trivial functions as well as the one being passed to an argument with [[clang::noescape]] attribute. This dramatically reduces the false positive rate for this checker. To do this, this PR replaces VisitLambdaExpr in favor of checking lambdas via VisitDeclRefExpr and VisitCallExpr. The idea is that if a lambda is defined but never called or stored somewhere, then capturing whatever variable in such a lambda is harmless. VisitCallExpr explicitly looks for direct invocation of lambdas and registers its DeclRefExpr to be ignored in VisitDeclRefExpr. If a lambda is being passed to a function, it checks whether its argument is annotated with [[clang::noescape]]. If it's not annotated such, it checks captures for their safety. Because WTF::switchOn could not be annotated with [[clang::noescape]] as function type parameters are variadic template function so we hard-code this function into the checker. In order to check whether "this" pointer is ref-counted type or not, we override TraverseDecl and record the most recent method's declaration. In addition, this PR fixes a bug in isUnsafePtr that it was erroneously checking whether std::nullopt was returned by isUncounted and isUnchecked as opposed to the actual boolean value. Finally, this PR also converts the accompanying test to use -verify and adds a bunch of tests.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Ryosuke Niwa (rniwa) ChangesThis PR makes webkit.UncountedLambdaCapturesChecker ignore trivial functions as well as the one being passed to an argument with [[clang::noescape]] attribute. This dramatically reduces the false positive rate for this checker. To do this, this PR replaces VisitLambdaExpr in favor of checking lambdas via VisitDeclRefExpr and VisitCallExpr. The idea is that if a lambda is defined but never called or stored somewhere, then capturing whatever variable in such a lambda is harmless. VisitCallExpr explicitly looks for direct invocation of lambdas and registers its DeclRefExpr to be ignored in VisitDeclRefExpr. If a lambda is being passed to a function, it checks whether its argument is annotated with [[clang::noescape]]. If it's not annotated such, it checks captures for their safety. Because WTF::switchOn could not be annotated with [[clang::noescape]] as function type parameters are variadic template function so we hard-code this function into the checker. In order to check whether "this" pointer is ref-counted type or not, we override TraverseDecl and record the most recent method's declaration. In addition, this PR fixes a bug in isUnsafePtr that it was erroneously checking whether std::nullopt was returned by isUncounted and isUnchecked as opposed to the actual boolean value. Finally, this PR also converts the accompanying test to use -verify and adds a bunch of tests. Full diff: https://github.com/llvm/llvm-project/pull/114897.diff 5 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index 46819d5ca12058..b32483369b5db1 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -211,7 +211,13 @@ std::optional<bool> isUncheckedPtr(const QualType T) {
std::optional<bool> isUnsafePtr(const QualType T) {
if (T->isPointerType() || T->isReferenceType()) {
if (auto *CXXRD = T->getPointeeCXXRecordDecl()) {
- return isUncounted(CXXRD) || isUnchecked(CXXRD);
+ auto isUncountedPtr = isUncounted(CXXRD);
+ auto isUncheckedPtr = isUnchecked(CXXRD);
+ if (isUncountedPtr && isUncheckedPtr)
+ return *isUncountedPtr || *isUncheckedPtr;
+ if (isUncountedPtr)
+ return *isUncountedPtr;
+ return isUncheckedPtr;
}
}
return false;
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
index 30bdaed706bb53..97baeabea8c7f0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
@@ -67,6 +67,10 @@ std::optional<bool> isUncountedPtr(const clang::QualType T);
/// class, false if not, std::nullopt if inconclusive.
std::optional<bool> isUncheckedPtr(const clang::QualType T);
+/// \returns true if \p T is either a raw pointer or reference to an uncounted
+/// or unchecked class, false if not, std::nullopt if inconclusive.
+std::optional<bool> isUnsafePtr(const QualType T);
+
/// \returns true if \p T is a RefPtr, Ref, CheckedPtr, CheckedRef, or its
/// variant, false if not.
bool isSafePtrType(const clang::QualType T);
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
index 998bd4ccee07db..ef61099416376d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
@@ -6,6 +6,7 @@
//
//===----------------------------------------------------------------------===//
+#include "ASTUtils.h"
#include "DiagOutputUtils.h"
#include "PtrTypesSemantics.h"
#include "clang/AST/CXXInheritance.h"
@@ -26,6 +27,7 @@ class UncountedLambdaCapturesChecker
BugType Bug{this, "Lambda capture of uncounted variable",
"WebKit coding guidelines"};
mutable BugReporter *BR = nullptr;
+ TrivialFunctionAnalysis TFA;
public:
void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
@@ -37,6 +39,11 @@ class UncountedLambdaCapturesChecker
// want to visit those, so we make our own RecursiveASTVisitor.
struct LocalVisitor : public RecursiveASTVisitor<LocalVisitor> {
const UncountedLambdaCapturesChecker *Checker;
+ llvm::DenseSet<const DeclRefExpr *> DeclRefExprsToIgnore;
+ QualType ClsType;
+
+ using Base = RecursiveASTVisitor<LocalVisitor>;
+
explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker)
: Checker(Checker) {
assert(Checker);
@@ -45,32 +52,119 @@ class UncountedLambdaCapturesChecker
bool shouldVisitTemplateInstantiations() const { return true; }
bool shouldVisitImplicitCode() const { return false; }
- bool VisitLambdaExpr(LambdaExpr *L) {
- Checker->visitLambdaExpr(L);
+ bool TraverseDecl(Decl *D) {
+ if (auto *CXXMD = dyn_cast<CXXMethodDecl>(D)) {
+ llvm::SaveAndRestore SavedDecl(ClsType);
+ ClsType = CXXMD->getThisType();
+ return Base::TraverseDecl(D);
+ }
+ return Base::TraverseDecl(D);
+ }
+
+ bool shouldCheckThis() {
+ auto result = !ClsType.isNull() ? isUnsafePtr(ClsType) : std::nullopt;
+ return result && *result;
+ }
+
+ bool VisitDeclRefExpr(DeclRefExpr *DRE) {
+ if (DeclRefExprsToIgnore.contains(DRE))
+ return true;
+ auto *VD = dyn_cast_or_null<VarDecl>(DRE->getDecl());
+ if (!VD)
+ return true;
+ auto *Init = VD->getInit();
+ if (!Init)
+ return true;
+ auto *L = dyn_cast_or_null<LambdaExpr>(Init->IgnoreParenCasts());
+ if (!L)
+ return true;
+ Checker->visitLambdaExpr(L, shouldCheckThis());
+ return true;
+ }
+
+ // WTF::switchOn(T, F... f) is a variadic template function and couldn't
+ // be annotated with NOESCAPE. We hard code it here to workaround that.
+ bool shouldTreatAllArgAsNoEscape(FunctionDecl *Decl) {
+ auto *NsDecl = Decl->getParent();
+ if (!NsDecl || !isa<NamespaceDecl>(NsDecl))
+ return false;
+ return safeGetName(NsDecl) == "WTF" && safeGetName(Decl) == "switchOn";
+ }
+
+ bool VisitCallExpr(CallExpr *CE) {
+ checkCalleeLambda(CE);
+ if (auto *Callee = CE->getDirectCallee()) {
+ bool TreatAllArgsAsNoEscape = shouldTreatAllArgAsNoEscape(Callee);
+ unsigned ArgIndex = 0;
+ for (auto *Param : Callee->parameters()) {
+ if (ArgIndex >= CE->getNumArgs())
+ break;
+ auto *Arg = CE->getArg(ArgIndex)->IgnoreParenCasts();
+ if (!Param->hasAttr<NoEscapeAttr>() && !TreatAllArgsAsNoEscape) {
+ if (auto *L = dyn_cast_or_null<LambdaExpr>(Arg)) {
+ Checker->visitLambdaExpr(L, shouldCheckThis());
+ }
+ }
+ ++ArgIndex;
+ }
+ }
return true;
}
+
+ void checkCalleeLambda(CallExpr *CE) {
+ auto *Callee = CE->getCallee();
+ if (!Callee)
+ return;
+ auto *DRE = dyn_cast<DeclRefExpr>(Callee->IgnoreParenCasts());
+ if (!DRE)
+ return;
+ auto *MD = dyn_cast_or_null<CXXMethodDecl>(DRE->getDecl());
+ if (!MD || CE->getNumArgs() != 1)
+ return;
+ auto *Arg = CE->getArg(0)->IgnoreParenCasts();
+ auto *ArgRef = dyn_cast<DeclRefExpr>(Arg);
+ if (!ArgRef)
+ return;
+ auto *VD = dyn_cast_or_null<VarDecl>(ArgRef->getDecl());
+ if (!VD)
+ return;
+ auto *Init = VD->getInit()->IgnoreParenCasts();
+ auto *L = dyn_cast_or_null<LambdaExpr>(Init);
+ if (!L)
+ return;
+ DeclRefExprsToIgnore.insert(ArgRef);
+ Checker->visitLambdaExpr(L, shouldCheckThis(),
+ /* ignoreParamVarDecl */ true);
+ }
};
LocalVisitor visitor(this);
visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
}
- void visitLambdaExpr(LambdaExpr *L) const {
+ void visitLambdaExpr(LambdaExpr *L, bool shouldCheckThis,
+ bool ignoreParamVarDecl = false) const {
+ if (TFA.isTrivial(L->getBody()))
+ return;
for (const LambdaCapture &C : L->captures()) {
if (C.capturesVariable()) {
ValueDecl *CapturedVar = C.getCapturedVar();
+ if (ignoreParamVarDecl && isa<ParmVarDecl>(CapturedVar))
+ continue;
QualType CapturedVarQualType = CapturedVar->getType();
- if (auto *CapturedVarType = CapturedVarQualType.getTypePtrOrNull()) {
- auto IsUncountedPtr = isUncountedPtr(CapturedVarQualType);
- if (IsUncountedPtr && *IsUncountedPtr)
- reportBug(C, CapturedVar, CapturedVarType);
- }
+ auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType());
+ if (IsUncountedPtr && *IsUncountedPtr)
+ reportBug(C, CapturedVar, CapturedVarQualType);
+ } else if (C.capturesThis() && shouldCheckThis) {
+ if (ignoreParamVarDecl) // this is always a parameter to this function.
+ continue;
+ reportBugOnThisPtr(C);
}
}
}
void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar,
- const Type *T) const {
+ const QualType T) const {
assert(CapturedVar);
SmallString<100> Buf;
@@ -89,7 +183,25 @@ class UncountedLambdaCapturesChecker
}
printQuotedQualifiedName(Os, Capture.getCapturedVar());
- Os << " to uncounted type is unsafe.";
+ Os << " to ref-counted / CheckedPtr capable type is unsafe.";
+
+ PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
+ auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
+ BR->emitReport(std::move(Report));
+ }
+
+ void reportBugOnThisPtr(const LambdaCapture &Capture) const {
+ SmallString<100> Buf;
+ llvm::raw_svector_ostream Os(Buf);
+
+ if (Capture.isExplicit()) {
+ Os << "Captured ";
+ } else {
+ Os << "Implicitly captured ";
+ }
+
+ Os << "raw-pointer 'this' to ref-counted / CheckedPtr capable type is "
+ "unsafe.";
PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h
index 8d95926e419beb..0d7c5a415af662 100644
--- a/clang/test/Analysis/Checkers/WebKit/mock-types.h
+++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h
@@ -135,7 +135,9 @@ struct RefCountable {
void ref() {}
void deref() {}
void method();
+ void constMethod() const;
int trivial() { return 123; }
+ RefCountable* next();
};
template <typename T> T *downcast(T *t) { return t; }
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
index 27e0a74d583cd3..5d6941aee71aec 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
@@ -1,39 +1,73 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.UncountedLambdaCapturesChecker %s 2>&1 | FileCheck %s --strict-whitespace
-#include "mock-types.h"
+// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.UncountedLambdaCapturesChecker -verify %s
+//#include "mock-types.h"
+
+struct RefCountable {
+// static Ref<RefCountable> create();
+ void ref() {}
+ void deref() {}
+ void method();
+ void constMethod() const;
+ int trivial() { return 123; }
+ RefCountable* next();
+};
+
+RefCountable* make_obj();
+
+void someFunction();
+template <typename Callback> void call(Callback callback) {
+ someFunction();
+ callback();
+}
void raw_ptr() {
- RefCountable* ref_countable = nullptr;
- auto foo1 = [ref_countable](){};
- // CHECK: warning: Captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
- // CHECK-NEXT:{{^ 6 | }} auto foo1 = [ref_countable](){};
- // CHECK-NEXT:{{^ | }} ^
- auto foo2 = [&ref_countable](){};
- // CHECK: warning: Captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
- auto foo3 = [&](){ ref_countable = nullptr; };
- // CHECK: warning: Implicitly captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
- // CHECK-NEXT:{{^ 12 | }} auto foo3 = [&](){ ref_countable = nullptr; };
- // CHECK-NEXT:{{^ | }} ^
- auto foo4 = [=](){ (void) ref_countable; };
- // CHECK: warning: Implicitly captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
+ RefCountable* ref_countable = make_obj();
+ auto foo1 = [ref_countable](){
+ // expected-warning@-1{{Captured raw-pointer 'ref_countable' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+ ref_countable->method();
+ };
+ auto foo2 = [&ref_countable](){
+ // expected-warning@-1{{Captured raw-pointer 'ref_countable' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+ ref_countable->method();
+ };
+ auto foo3 = [&](){
+ ref_countable->method();
+ // expected-warning@-1{{Implicitly captured raw-pointer 'ref_countable' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+ ref_countable = nullptr;
+ };
+
+ auto foo4 = [=](){
+ ref_countable->method();
+ // expected-warning@-1{{Implicitly captured raw-pointer 'ref_countable' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+ };
+
+ call(foo1);
+ call(foo2);
+ call(foo3);
+ call(foo4);
// Confirm that the checker respects [[clang::suppress]].
RefCountable* suppressed_ref_countable = nullptr;
[[clang::suppress]] auto foo5 = [suppressed_ref_countable](){};
- // CHECK-NOT: warning: Captured raw-pointer 'suppressed_ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
+ // no warning.
+ call(foo5);
}
void references() {
RefCountable automatic;
RefCountable& ref_countable_ref = automatic;
+ auto foo1 = [ref_countable_ref](){ ref_countable_ref.constMethod(); };
+ // expected-warning@-1{{Captured reference 'ref_countable_ref' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+ auto foo2 = [&ref_countable_ref](){ ref_countable_ref.method(); };
+ // expected-warning@-1{{Captured reference 'ref_countable_ref' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+ auto foo3 = [&](){ ref_countable_ref.method(); };
+ // expected-warning@-1{{Implicitly captured reference 'ref_countable_ref' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+ auto foo4 = [=](){ ref_countable_ref.constMethod(); };
+ // expected-warning@-1{{Implicitly captured reference 'ref_countable_ref' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
- auto foo1 = [ref_countable_ref](){};
- // CHECK: warning: Captured reference 'ref_countable_ref' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
- auto foo2 = [&ref_countable_ref](){};
- // CHECK: warning: Captured reference 'ref_countable_ref' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
- auto foo3 = [&](){ (void) ref_countable_ref; };
- // CHECK: warning: Implicitly captured reference 'ref_countable_ref' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
- auto foo4 = [=](){ (void) ref_countable_ref; };
- // CHECK: warning: Implicitly captured reference 'ref_countable_ref' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
+ call(foo1);
+ call(foo2);
+ call(foo3);
+ call(foo4);
}
void quiet() {
@@ -45,5 +79,100 @@ void quiet() {
auto foo3 = [&]() {};
auto foo4 = [=]() {};
+
+ call(foo3);
+ call(foo4);
+
RefCountable *ref_countable = nullptr;
}
+
+template <typename Callback>
+void map(RefCountable* start, [[clang::noescape]] Callback&& callback)
+{
+ while (start) {
+ callback(*start);
+ start = start->next();
+ }
+}
+
+template <typename Callback1, typename Callback2>
+void doubleMap(RefCountable* start, [[clang::noescape]] Callback1&& callback1, Callback2&& callback2)
+{
+ while (start) {
+ callback1(*start);
+ callback2(*start);
+ start = start->next();
+ }
+}
+
+void noescape_lambda() {
+ RefCountable* someObj = make_obj();
+ RefCountable* otherObj = make_obj();
+ map(make_obj(), [&](RefCountable& obj) {
+ otherObj->method();
+ });
+ doubleMap(make_obj(), [&](RefCountable& obj) {
+ otherObj->method();
+ }, [&](RefCountable& obj) {
+ otherObj->method();
+ // expected-warning@-1{{Implicitly captured raw-pointer 'otherObj' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+ });
+ ([&] {
+ someObj->method();
+ })();
+}
+
+void lambda_capture_param(RefCountable* obj) {
+ auto someLambda = [&] {
+ obj->method();
+ };
+ someLambda();
+ someLambda();
+}
+
+struct RefCountableWithLambdaCapturingThis {
+ void ref() const;
+ void deref() const;
+ void nonTrivial();
+
+ void method_captures_this_safe() {
+ auto lambda = [&]() {
+ nonTrivial();
+ };
+ lambda();
+ }
+
+ void method_captures_this_unsafe() {
+ auto lambda = [&]() {
+ nonTrivial();
+ // expected-warning@-1{{Implicitly captured raw-pointer 'this' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+ };
+ call(lambda);
+ }
+};
+
+struct NonRefCountableWithLambdaCapturingThis {
+ void nonTrivial();
+
+ void method_captures_this_safe() {
+ auto lambda = [&]() {
+ nonTrivial();
+ };
+ lambda();
+ }
+
+ void method_captures_this_unsafe() {
+ auto lambda = [&]() {
+ nonTrivial();
+ };
+ call(lambda);
+ }
+};
+
+void trivial_lambda() {
+ RefCountable* ref_countable = make_obj();
+ auto trivial_lambda = [&]() {
+ return ref_countable->trivial();
+ };
+ trivial_lambda();
+}
|
haoNoQ
reviewed
Nov 5, 2024
There was a problem hiding this comment.
The attribute gets some action! Nice!!
I noticed that currently the attribute's documentation doesn't say it can be placed on lambdas. But it doesn't look like it's actively rejected either. So it might be a good idea to update the documentation as part of the patch. Possibly even have a bit of a broader discussion about this - though I don't really see any downsides to simply allowing this.
clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
Outdated
Show resolved
Hide resolved
Comment on lines
+85
to
+86
| // WTF::switchOn(T, F... f) is a variadic template function and couldn't | ||
| // be annotated with NOESCAPE. We hard code it here to workaround that. |
There was a problem hiding this comment.
Ooo interesting.
Technically there's standardized syntax for this sort of stuff: https://en.cppreference.com/w/cpp/language/parameter_pack
Pack expansions are allowed in the lists of attributes if permitted by the attribute's specification. For example:
template<int... args> [[vendor::attr(args)...]] void* f();
But, IIUC, this requires the attribute to be, like, a function attribute that receives the parameter identifier as an attribute argument(?)
So it'll probably not work out of the box but it's probably doable. If you run into many more functions of that nature, this could be a way out.
There was a problem hiding this comment.
I see. It's probably an overkill for this one function but I'd keep that in my mind.
I'm more than happy to bring the discussion up but which venue (forum, mailing list, etc...) should I be using to solicit a broader discussion? |
t-rasmud
approved these changes
Nov 11, 2024
| unsigned ArgIndex = 0; | ||
| for (auto *Param : Callee->parameters()) { | ||
| if (ArgIndex >= CE->getNumArgs()) | ||
| break; |
There was a problem hiding this comment.
nit: Could we just return true instead of a break here?
There was a problem hiding this comment.
Sure, we can do that.
| Os << "Implicitly captured "; | ||
| } | ||
|
|
||
| Os << "raw-pointer 'this' to ref-counted / CheckedPtr capable type is " |
There was a problem hiding this comment.
nit: Can this warning be better worded? .. ref-counted (or CheckedPtr capable) type ...? Or anything else that conveys that it's unsafe to capture raw-pointer to a ref-counted type or a "CheckedPtr capable" type.
There was a problem hiding this comment.
Rephrased to say "ref-counted type or CheckedPtr-capable type"
Also fix a bug that TraverseCXXMethodDecl was erroneously calling TraverseDecl, and cleanup the test.
rniwa
added a commit
to rniwa/llvm-project
that referenced
this pull request
Feb 3, 2025
…[[clang::noescape]]. (llvm#114897) This PR makes webkit.UncountedLambdaCapturesChecker ignore trivial functions as well as the one being passed to an argument with [[clang::noescape]] attribute. This dramatically reduces the false positive rate for this checker. To do this, this PR replaces VisitLambdaExpr in favor of checking lambdas via VisitDeclRefExpr and VisitCallExpr. The idea is that if a lambda is defined but never called or stored somewhere, then capturing whatever variable in such a lambda is harmless. VisitCallExpr explicitly looks for direct invocation of lambdas and registers its DeclRefExpr to be ignored in VisitDeclRefExpr. If a lambda is being passed to a function, it checks whether its argument is annotated with [[clang::noescape]]. If it's not annotated such, it checks captures for their safety. Because WTF::switchOn could not be annotated with [[clang::noescape]] as function type parameters are variadic template function so we hard-code this function into the checker. In order to check whether "this" pointer is ref-counted type or not, we override TraverseDecl and record the most recent method's declaration. In addition, this PR fixes a bug in isUnsafePtr that it was erroneously checking whether std::nullopt was returned by isUncounted and isUnchecked as opposed to the actual boolean value. Finally, this PR also converts the accompanying test to use -verify and adds a bunch of tests.
devincoughlin
pushed a commit
to swiftlang/llvm-project
that referenced
this pull request
Feb 25, 2025
…[[clang::noescape]]. (llvm#114897) This PR makes webkit.UncountedLambdaCapturesChecker ignore trivial functions as well as the one being passed to an argument with [[clang::noescape]] attribute. This dramatically reduces the false positive rate for this checker. To do this, this PR replaces VisitLambdaExpr in favor of checking lambdas via VisitDeclRefExpr and VisitCallExpr. The idea is that if a lambda is defined but never called or stored somewhere, then capturing whatever variable in such a lambda is harmless. VisitCallExpr explicitly looks for direct invocation of lambdas and registers its DeclRefExpr to be ignored in VisitDeclRefExpr. If a lambda is being passed to a function, it checks whether its argument is annotated with [[clang::noescape]]. If it's not annotated such, it checks captures for their safety. Because WTF::switchOn could not be annotated with [[clang::noescape]] as function type parameters are variadic template function so we hard-code this function into the checker. In order to check whether "this" pointer is ref-counted type or not, we override TraverseDecl and record the most recent method's declaration. In addition, this PR fixes a bug in isUnsafePtr that it was erroneously checking whether std::nullopt was returned by isUncounted and isUnchecked as opposed to the actual boolean value. Finally, this PR also converts the accompanying test to use -verify and adds a bunch of tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR makes webkit.UncountedLambdaCapturesChecker ignore trivial functions as well as the one being passed to an argument with [[clang::noescape]] attribute. This dramatically reduces the false positive rate for this checker.
To do this, this PR replaces VisitLambdaExpr in favor of checking lambdas via VisitDeclRefExpr and VisitCallExpr. The idea is that if a lambda is defined but never called or stored somewhere, then capturing whatever variable in such a lambda is harmless.
VisitCallExpr explicitly looks for direct invocation of lambdas and registers its DeclRefExpr to be ignored in VisitDeclRefExpr. If a lambda is being passed to a function, it checks whether its argument is annotated with [[clang::noescape]]. If it's not annotated such, it checks captures for their safety.
Because WTF::switchOn could not be annotated with [[clang::noescape]] as function type parameters are variadic template function so we hard-code this function into the checker.
In order to check whether "this" pointer is ref-counted type or not, we override TraverseDecl and record the most recent method's declaration.
In addition, this PR fixes a bug in isUnsafePtr that it was erroneously checking whether std::nullopt was returned by isUncounted and isUnchecked as opposed to the actual boolean value.
Finally, this PR also converts the accompanying test to use -verify and adds a bunch of tests.