[alpha.webkit.UnretainedLambdaCapturesChecker] Add the support for protectedSelf #132363
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
…otectedSelf This PR adds the support for treating capturing of "self" as safe if the lambda simultaneously captures "protectedSelf", which is a RetainPtr of "self". This PR also fixes a bug that the checker wasn't generating a warning when "self" is implicitly captured. Note when "self" is implicitly captured, we use the lambda's getBeginLoc as a fallback source location. In addition, this PR also fixes a bug that the checker wasn't generating warnings for lambda caotures when ARC is enabled. While ARC protects variables in the stack, it does not automatically extend the lifetime of a lambda capture. So we now call RetainTypeChecker's isUnretained with ignoreARC set to true.
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Ryosuke Niwa (rniwa) ChangesThis PR adds the support for treating capturing of "self" as safe if the lambda simultaneously captures "protectedSelf", which is a RetainPtr of "self". This PR also fixes a bug that the checker wasn't generating a warning when "self" is implicitly captured. Note when "self" is implicitly captured, we use the lambda's getBeginLoc as a fallback source location. In addition, this PR also fixes a bug that the checker wasn't generating warnings for lambda caotures when ARC is enabled. While ARC protects variables in the stack, it does not automatically extend the lifetime of a lambda capture. So we now call RetainTypeChecker's isUnretained with ignoreARC set to true. Full diff: https://github.com/llvm/llvm-project/pull/132363.diff 4 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index b4d2353a03cd2..a161cf644b7d9 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -147,7 +147,7 @@ bool isCtorOfCheckedPtr(const clang::FunctionDecl *F) {
bool isCtorOfRetainPtr(const clang::FunctionDecl *F) {
const std::string &FunctionName = safeGetName(F);
return FunctionName == "RetainPtr" || FunctionName == "adoptNS" ||
- FunctionName == "adoptCF";
+ FunctionName == "adoptCF" || FunctionName == "retainPtr";
}
bool isCtorOfSafePtr(const clang::FunctionDecl *F) {
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp
index 8496a75c1b84f..e8dcf3aaf518e 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp
@@ -36,6 +36,7 @@ class RawPtrRefLambdaCapturesChecker
: Bug(this, description, "WebKit coding guidelines") {}
virtual std::optional<bool> isUnsafePtr(QualType) const = 0;
+ virtual bool isPtrType(const std::string &) const = 0;
virtual const char *ptrKind(QualType QT) const = 0;
void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
@@ -68,6 +69,15 @@ class RawPtrRefLambdaCapturesChecker
return DynamicRecursiveASTVisitor::TraverseCXXMethodDecl(CXXMD);
}
+ bool TraverseObjCMethodDecl(ObjCMethodDecl *OCMD) override {
+ llvm::SaveAndRestore SavedDecl(ClsType);
+ if (OCMD && OCMD->isInstanceMethod()) {
+ if (auto *ImplParamDecl = OCMD->getSelfDecl())
+ ClsType = ImplParamDecl->getType();
+ }
+ return DynamicRecursiveASTVisitor::TraverseObjCMethodDecl(OCMD);
+ }
+
bool VisitTypedefDecl(TypedefDecl *TD) override {
if (Checker->RTC)
Checker->RTC->visitTypedef(TD);
@@ -287,7 +297,7 @@ class RawPtrRefLambdaCapturesChecker
if (!Ctor)
return false;
auto clsName = safeGetName(Ctor->getParent());
- if (isRefType(clsName) && CE->getNumArgs()) {
+ if (Checker->isPtrType(clsName) && CE->getNumArgs()) {
Arg = CE->getArg(0)->IgnoreParenCasts();
continue;
}
@@ -307,6 +317,12 @@ class RawPtrRefLambdaCapturesChecker
Arg = CE->getArg(0)->IgnoreParenCasts();
continue;
}
+ if (auto *Callee = CE->getDirectCallee()) {
+ if (isCtorOfSafePtr(Callee) && CE->getNumArgs() == 1) {
+ Arg = CE->getArg(0)->IgnoreParenCasts();
+ continue;
+ }
+ }
}
if (auto *OpCE = dyn_cast<CXXOperatorCallExpr>(Arg)) {
auto OpCode = OpCE->getOperator();
@@ -315,7 +331,7 @@ class RawPtrRefLambdaCapturesChecker
if (!Callee)
return false;
auto clsName = safeGetName(Callee->getParent());
- if (!isRefType(clsName) || !OpCE->getNumArgs())
+ if (!Checker->isPtrType(clsName) || !OpCE->getNumArgs())
return false;
Arg = OpCE->getArg(0)->IgnoreParenCasts();
continue;
@@ -330,8 +346,15 @@ class RawPtrRefLambdaCapturesChecker
}
break;
} while (Arg);
- if (auto *DRE = dyn_cast<DeclRefExpr>(Arg))
- return ProtectedThisDecls.contains(DRE->getDecl());
+ if (auto *DRE = dyn_cast<DeclRefExpr>(Arg)) {
+ auto *Decl = DRE->getDecl();
+ if (auto *ImplicitParam = dyn_cast<ImplicitParamDecl>(Decl)) {
+ auto kind = ImplicitParam->getParameterKind();
+ return kind == ImplicitParamKind::ObjCSelf ||
+ kind == ImplicitParamKind::CXXThis;
+ }
+ return ProtectedThisDecls.contains(Decl);
+ }
return isa<CXXThisExpr>(Arg);
}
};
@@ -351,10 +374,16 @@ class RawPtrRefLambdaCapturesChecker
ValueDecl *CapturedVar = C.getCapturedVar();
if (ignoreParamVarDecl && isa<ParmVarDecl>(CapturedVar))
continue;
+ if (auto *ImplicitParam = dyn_cast<ImplicitParamDecl>(CapturedVar)) {
+ auto kind = ImplicitParam->getParameterKind();
+ if ((kind == ImplicitParamKind::ObjCSelf ||
+ kind == ImplicitParamKind::CXXThis) && !shouldCheckThis)
+ continue;
+ }
QualType CapturedVarQualType = CapturedVar->getType();
auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType());
if (IsUncountedPtr && *IsUncountedPtr)
- reportBug(C, CapturedVar, CapturedVarQualType);
+ reportBug(C, CapturedVar, CapturedVarQualType, L);
} else if (C.capturesThis() && shouldCheckThis) {
if (ignoreParamVarDecl) // this is always a parameter to this function.
continue;
@@ -364,11 +393,12 @@ class RawPtrRefLambdaCapturesChecker
}
void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar,
- const QualType T) const {
+ const QualType T, LambdaExpr *L) const {
assert(CapturedVar);
- if (isa<ImplicitParamDecl>(CapturedVar) && !Capture.getLocation().isValid())
- return; // Ignore implicit captruing of self.
+ auto Location = Capture.getLocation();
+ if (isa<ImplicitParamDecl>(CapturedVar) && !Location.isValid())
+ Location = L->getBeginLoc();
SmallString<100> Buf;
llvm::raw_svector_ostream Os(Buf);
@@ -387,7 +417,7 @@ class RawPtrRefLambdaCapturesChecker
printQuotedQualifiedName(Os, CapturedVar);
Os << " to " << ptrKind(T) << " type is unsafe.";
- PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
+ PathDiagnosticLocation BSLoc(Location, BR->getSourceManager());
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
BR->emitReport(std::move(Report));
}
@@ -429,6 +459,10 @@ class UncountedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker {
return result2;
}
+ virtual bool isPtrType(const std::string &Name) const final {
+ return isRefType(Name) || isCheckedPtr(Name);
+ }
+
const char *ptrKind(QualType QT) const final {
if (isUncounted(QT))
return "uncounted";
@@ -445,7 +479,11 @@ class UnretainedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker {
}
std::optional<bool> isUnsafePtr(QualType QT) const final {
- return RTC->isUnretained(QT);
+ return RTC->isUnretained(QT, /* ignoreARC */ true);
+ }
+
+ virtual bool isPtrType(const std::string &Name) const final {
+ return isRetainPtr(Name);
}
const char *ptrKind(QualType QT) const final { return "unretained"; }
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures-arc.mm b/clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures-arc.mm
index 4e3d9c2708d96..3b718e1e3d76f 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures-arc.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures-arc.mm
@@ -123,19 +123,23 @@ explicit CallableWrapper(CallableType& callable)
void raw_ptr() {
SomeObj* obj = make_obj();
auto foo1 = [obj](){
+ // expected-warning@-1{{Captured raw-pointer 'obj' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
[obj doWork];
};
call(foo1);
auto foo2 = [&obj](){
+ // expected-warning@-1{{Captured raw-pointer 'obj' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
[obj doWork];
};
auto foo3 = [&](){
[obj doWork];
+ // expected-warning@-1{{Implicitly captured raw-pointer 'obj' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
obj = nullptr;
};
auto foo4 = [=](){
[obj doWork];
+ // expected-warning@-1{{Implicitly captured raw-pointer 'obj' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
};
auto cf = make_cf();
@@ -218,6 +222,7 @@ void noescape_lambda() {
[otherObj doWork];
}, [&](SomeObj *obj) {
[otherObj doWork];
+ // expected-warning@-1{{Implicitly captured raw-pointer 'otherObj' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
});
([&] {
[someObj doWork];
@@ -242,11 +247,13 @@ void lambda_converted_to_function(SomeObj* obj, CFMutableArrayRef cf)
{
callFunction([&]() {
[obj doWork];
+ // expected-warning@-1{{Implicitly captured raw-pointer 'obj' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
CFArrayAppendValue(cf, nullptr);
// expected-warning@-1{{Implicitly captured reference 'cf' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
});
callFunctionOpaque([&]() {
[obj doWork];
+ // expected-warning@-1{{Implicitly captured raw-pointer 'obj' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
CFArrayAppendValue(cf, nullptr);
// expected-warning@-1{{Implicitly captured reference 'cf' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
});
@@ -262,6 +269,7 @@ -(void)run;
@implementation ObjWithSelf
-(void)doWork {
auto doWork = [&] {
+ // expected-warning@-1{{Implicitly captured raw-pointer 'self' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
someFunction();
[delegate doWork];
};
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures.mm b/clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures.mm
index 073eff9386baa..33be0eaaae914 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures.mm
@@ -279,17 +279,33 @@ @interface ObjWithSelf : NSObject {
RetainPtr<id> delegate;
}
-(void)doWork;
+-(void)doMoreWork;
-(void)run;
@end
@implementation ObjWithSelf
-(void)doWork {
auto doWork = [&] {
+ // expected-warning@-1{{Implicitly captured raw-pointer 'self' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
+ someFunction();
+ [delegate doWork];
+ };
+ auto doExtraWork = [&, protectedSelf = retainPtr(self)] {
someFunction();
[delegate doWork];
};
callFunctionOpaque(doWork);
+ callFunctionOpaque(doExtraWork);
}
+
+-(void)doMoreWork {
+ auto doWork = [self, protectedSelf = retainPtr(self)] {
+ someFunction();
+ [self doWork];
+ };
+ callFunctionOpaque(doWork);
+}
+
-(void)run {
someFunction();
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
This PR adds the support for treating capturing of "self" as safe if the lambda simultaneously captures "protectedSelf", which is a RetainPtr of "self".
This PR also fixes a bug that the checker wasn't generating a warning when "self" is implicitly captured. Note when "self" is implicitly captured, we use the lambda's getBeginLoc as a fallback source location.
In addition, this PR also fixes a bug that the checker wasn't generating warnings for lambda caotures when ARC is enabled. While ARC protects variables in the stack, it does not automatically extend the lifetime of a lambda capture. So we now call RetainTypeChecker's isUnretained with ignoreARC set to true.