llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-static-analyzer-1 Author: Ryosuke Niwa (rniwa) <details> <summary>Changes</summary> In WebKit, it's pretty common to capture "this" and "protectedThis" where "protectedThis" is a guardian variable of type Ref or RefPtr for "this". Furthermore, it's common for this "protectedThis" variable from being passed to an inner lambda by std::move. Recognize this pattern so that we don't emit warnings for nested inner lambdas. To recognize this pattern, we introduce a new DenseSet, ProtectedThisDecls, which contains every "protectedThis" we've recognized to our subclass of DynamicRecursiveASTVisitor. This set is now populated in "hasProtectedThis" and "declProtectsThis" uses this DenseSet to determine a given value declaration constitutes a "protectedThis" pattern or not. Because hasProtectedThis and declProtectsThis had to be moved from the checker class to the visitor class, it's now a responsibility of each caller of visitLambdaExpr to check whether a given lambda captures "this" without a "protectedThis" or not. Finally, this PR improves the code to recognize "protectedThis" pattern by allowing more nested CXXBindTemporaryExpr, CXXOperatorCallExpr, and UnaryOperator expressions. --- Full diff: https://github.com/llvm/llvm-project/pull/126443.diff 2 Files Affected: - (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp (+71-47) - (modified) clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp (+24) ``````````diff diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp index a56f48c83c660a9..c0a9e38b6aab41b 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 ValueDecl *> ProtectedThisDecls; QualType ClsType; explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker) @@ -65,7 +66,7 @@ class UncountedLambdaCapturesChecker bool VisitLambdaExpr(LambdaExpr *L) override { if (LambdasToIgnore.contains(L)) return true; - Checker->visitLambdaExpr(L, shouldCheckThis()); + Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L)); return true; } @@ -93,7 +94,7 @@ class UncountedLambdaCapturesChecker if (!L) return true; LambdasToIgnore.insert(L); - Checker->visitLambdaExpr(L, shouldCheckThis()); + Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L)); return true; } @@ -118,7 +119,8 @@ class UncountedLambdaCapturesChecker if (auto *L = findLambdaInArg(Arg)) { LambdasToIgnore.insert(L); if (!Param->hasAttr<NoEscapeAttr>() && !TreatAllArgsAsNoEscape) - Checker->visitLambdaExpr(L, shouldCheckThis()); + Checker->visitLambdaExpr(L, shouldCheckThis() && + !hasProtectedThis(L)); } ++ArgIndex; } @@ -145,6 +147,11 @@ class UncountedLambdaCapturesChecker return nullptr; if (auto *Lambda = dyn_cast<LambdaExpr>(CtorArg)) return Lambda; + if (auto *TempExpr = dyn_cast<CXXBindTemporaryExpr>(CtorArg)) { + E = TempExpr->getSubExpr()->IgnoreParenCasts(); + if (auto *Lambda = dyn_cast<LambdaExpr>(E)) + return Lambda; + } auto *DRE = dyn_cast<DeclRefExpr>(CtorArg); if (!DRE) return nullptr; @@ -189,9 +196,68 @@ class UncountedLambdaCapturesChecker return; DeclRefExprsToIgnore.insert(ArgRef); LambdasToIgnore.insert(L); - Checker->visitLambdaExpr(L, shouldCheckThis(), + Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L), /* ignoreParamVarDecl */ true); } + + bool hasProtectedThis(LambdaExpr *L) { + for (const LambdaCapture &OtherCapture : L->captures()) { + if (!OtherCapture.capturesVariable()) + continue; + if (auto *ValueDecl = OtherCapture.getCapturedVar()) { + if (declProtectsThis(ValueDecl)) { + ProtectedThisDecls.insert(ValueDecl); + return true; + } + } + } + return false; + } + + bool declProtectsThis(const ValueDecl *ValueDecl) const { + auto *VD = dyn_cast<VarDecl>(ValueDecl); + if (!VD) + return false; + auto *Init = VD->getInit()->IgnoreParenCasts(); + if (!Init) + return false; + const Expr *Arg = Init; + do { + if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Arg)) + Arg = BTE->getSubExpr()->IgnoreParenCasts(); + if (auto *CE = dyn_cast_or_null<CXXConstructExpr>(Arg)) { + auto *Ctor = CE->getConstructor(); + if (!Ctor) + return false; + auto clsName = safeGetName(Ctor->getParent()); + if (!isRefType(clsName) || !CE->getNumArgs()) + return false; + Arg = CE->getArg(0)->IgnoreParenCasts(); + continue; + } + if (auto *OpCE = dyn_cast<CXXOperatorCallExpr>(Arg)) { + auto OpCode = OpCE->getOperator(); + if (OpCode == OO_Star || OpCode == OO_Amp) { + auto *Callee = OpCE->getDirectCallee(); + auto clsName = safeGetName(Callee->getParent()); + if (!isRefType(clsName) || !OpCE->getNumArgs()) + return false; + Arg = OpCE->getArg(0)->IgnoreParenCasts(); + continue; + } + } + if (auto *UO = dyn_cast<UnaryOperator>(Arg)) { + auto OpCode = UO->getOpcode(); + if (OpCode == UO_Deref || OpCode == UO_AddrOf) + Arg = UO->getSubExpr()->IgnoreParenCasts(); + continue; + } + break; + } while (Arg); + if (auto *DRE = dyn_cast<DeclRefExpr>(Arg)) + return ProtectedThisDecls.contains(DRE->getDecl()); + return isa<CXXThisExpr>(Arg); + } }; LocalVisitor visitor(this); @@ -214,53 +280,11 @@ class UncountedLambdaCapturesChecker } else if (C.capturesThis() && shouldCheckThis) { if (ignoreParamVarDecl) // this is always a parameter to this function. continue; - bool hasProtectThis = false; - for (const LambdaCapture &OtherCapture : L->captures()) { - if (!OtherCapture.capturesVariable()) - continue; - if (auto *ValueDecl = OtherCapture.getCapturedVar()) { - if (protectThis(ValueDecl)) { - hasProtectThis = true; - break; - } - } - } - if (!hasProtectThis) - reportBugOnThisPtr(C); + reportBugOnThisPtr(C); } } } - bool protectThis(const ValueDecl *ValueDecl) const { - auto *VD = dyn_cast<VarDecl>(ValueDecl); - if (!VD) - return false; - auto *Init = VD->getInit()->IgnoreParenCasts(); - if (!Init) - return false; - auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Init); - if (!BTE) - return false; - auto *CE = dyn_cast_or_null<CXXConstructExpr>(BTE->getSubExpr()); - if (!CE) - return false; - auto *Ctor = CE->getConstructor(); - if (!Ctor) - return false; - auto clsName = safeGetName(Ctor->getParent()); - if (!isRefType(clsName) || !CE->getNumArgs()) - return false; - auto *Arg = CE->getArg(0)->IgnoreParenCasts(); - while (auto *UO = dyn_cast<UnaryOperator>(Arg)) { - auto OpCode = UO->getOpcode(); - if (OpCode == UO_Deref || OpCode == UO_AddrOf) - Arg = UO->getSubExpr(); - else - break; - } - return isa<CXXThisExpr>(Arg); - } - void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar, const QualType T) const { assert(CapturedVar); diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp index 4f4a96028225303..4895879c4a385db 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp @@ -89,6 +89,7 @@ template <typename Callback> void call(Callback callback) { someFunction(); callback(); } +void callAsync(const WTF::Function<void()>&); void raw_ptr() { RefCountable* ref_countable = make_obj(); @@ -299,6 +300,29 @@ struct RefCountableWithLambdaCapturingThis { return obj->next(); }); } + + void callAsyncNoescape([[clang::noescape]] WTF::Function<bool(RefCountable&)>&&); + void method_temp_lambda(RefCountable* obj) { + callAsyncNoescape([this, otherObj = RefPtr { obj }](auto& obj) { + return otherObj == &obj; + }); + } + + void method_nested_lambda() { + callAsync([this, protectedThis = Ref { *this }] { + callAsync([this, protectedThis = static_cast<const Ref<RefCountableWithLambdaCapturingThis>&&>(protectedThis)] { + nonTrivial(); + }); + }); + } + + void method_nested_lambda2() { + callAsync([this, protectedThis = RefPtr { this }] { + callAsync([this, protectedThis = static_cast<const Ref<RefCountableWithLambdaCapturingThis>&&>(*protectedThis)] { + nonTrivial(); + }); + }); + } }; struct NonRefCountableWithLambdaCapturingThis { `````````` </details> https://github.com/llvm/llvm-project/pull/126443 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
