llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-static-analyzer-1 Author: Ryosuke Niwa (rniwa) <details> <summary>Changes</summary> 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. --- Full diff: https://github.com/llvm/llvm-project/pull/132363.diff 4 Files Affected: - (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp (+1-1) - (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp (+48-10) - (modified) clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures-arc.mm (+8) - (modified) clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures.mm (+16) ``````````diff 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(); } `````````` </details> https://github.com/llvm/llvm-project/pull/132363 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
