llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang 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. --- Full diff: https://github.com/llvm/llvm-project/pull/132518.diff 3 Files Affected: - (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp (+1-1) - (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp (+50-11) - (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..da403d591a2e2 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); @@ -275,10 +285,10 @@ class RawPtrRefLambdaCapturesChecker auto *VD = dyn_cast<VarDecl>(ValueDecl); if (!VD) return false; - auto *Init = VD->getInit()->IgnoreParenCasts(); + auto *Init = VD->getInit(); if (!Init) return false; - const Expr *Arg = Init; + const Expr *Arg = Init->IgnoreParenCasts(); do { if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Arg)) Arg = BTE->getSubExpr()->IgnoreParenCasts(); @@ -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,17 @@ 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 +394,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 +418,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 +460,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"; @@ -448,6 +483,10 @@ class UnretainedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker { return RTC->isUnretained(QT); } + 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.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/132518 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
