From dadfd4a288d9fd87314c1542a052e5942960c40b Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Thu, 31 Oct 2024 00:30:18 -0700 Subject: [PATCH] Revert "[webkit.UncountedLambdaCapturesChecker] Ignore trivial functions and [[clang::noescape]]." (#114372) Reverts llvm/llvm-project#113845. Introduced a test failure. --- .../Checkers/WebKit/PtrTypesSemantics.h | 4 - .../WebKit/UncountedLambdaCapturesChecker.cpp | 108 +----------- .../Analysis/Checkers/WebKit/mock-types.h | 2 - .../WebKit/uncounted-lambda-captures.cpp | 159 +++--------------- 4 files changed, 33 insertions(+), 240 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h index 814015c311d61e..4b41ca96e1df1d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h @@ -63,10 +63,6 @@ std::optional isUncounted(const clang::CXXRecordDecl* Class); /// class, false if not, std::nullopt if inconclusive. std::optional isUncountedPtr(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 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 d3484d74a2e3eb..998bd4ccee07db 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp @@ -6,7 +6,6 @@ // //===----------------------------------------------------------------------===// -#include "ASTUtils.h" #include "DiagOutputUtils.h" #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" @@ -27,7 +26,6 @@ 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, @@ -39,8 +37,6 @@ class UncountedLambdaCapturesChecker // want to visit those, so we make our own RecursiveASTVisitor. struct LocalVisitor : public RecursiveASTVisitor { const UncountedLambdaCapturesChecker *Checker; - llvm::DenseSet DeclRefExprsToIgnore; - explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker) : Checker(Checker) { assert(Checker); @@ -49,100 +45,32 @@ class UncountedLambdaCapturesChecker bool shouldVisitTemplateInstantiations() const { return true; } bool shouldVisitImplicitCode() const { return false; } - bool VisitDeclRefExpr(DeclRefExpr *DRE) { - if (DeclRefExprsToIgnore.contains(DRE)) - return true; - auto *VD = dyn_cast_or_null(DRE->getDecl()); - if (!VD) - return true; - auto *Init = VD->getInit()->IgnoreParenCasts(); - auto *L = dyn_cast_or_null(Init); - if (!L) - return true; + bool VisitLambdaExpr(LambdaExpr *L) { Checker->visitLambdaExpr(L); 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(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() && !TreatAllArgsAsNoEscape) { - if (auto *L = dyn_cast_or_null(Arg)) - Checker->visitLambdaExpr(L); - } - ++ArgIndex; - } - } - return true; - } - - void checkCalleeLambda(CallExpr *CE) { - auto *Callee = CE->getCallee(); - if (!Callee) - return; - auto *DRE = dyn_cast(Callee->IgnoreParenCasts()); - if (!DRE) - return; - auto *MD = dyn_cast_or_null(DRE->getDecl()); - if (!MD || CE->getNumArgs() != 1) - return; - auto *Arg = CE->getArg(0)->IgnoreParenCasts(); - auto *ArgRef = dyn_cast(Arg); - if (!ArgRef) - return; - auto *VD = dyn_cast_or_null(ArgRef->getDecl()); - if (!VD) - return; - auto *Init = VD->getInit()->IgnoreParenCasts(); - auto *L = dyn_cast_or_null(Init); - if (!L) - return; - DeclRefExprsToIgnore.insert(ArgRef); - Checker->visitLambdaExpr(L, /* ignoreParamVarDecl */ true); - } }; LocalVisitor visitor(this); visitor.TraverseDecl(const_cast(TUD)); } - void visitLambdaExpr(LambdaExpr *L, bool ignoreParamVarDecl = false) const { - if (TFA.isTrivial(L->getBody())) - return; + void visitLambdaExpr(LambdaExpr *L) const { for (const LambdaCapture &C : L->captures()) { if (C.capturesVariable()) { ValueDecl *CapturedVar = C.getCapturedVar(); - if (ignoreParamVarDecl && isa(CapturedVar)) - continue; QualType CapturedVarQualType = CapturedVar->getType(); - auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType()); - if (IsUncountedPtr && *IsUncountedPtr) - reportBug(C, CapturedVar, CapturedVarQualType); - } else if (C.capturesThis()) { - if (ignoreParamVarDecl) // this is always a parameter to this function. - continue; - reportBugOnThisPtr(C); + if (auto *CapturedVarType = CapturedVarQualType.getTypePtrOrNull()) { + auto IsUncountedPtr = isUncountedPtr(CapturedVarQualType); + if (IsUncountedPtr && *IsUncountedPtr) + reportBug(C, CapturedVar, CapturedVarType); + } } } } void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar, - const QualType T) const { + const Type *T) const { assert(CapturedVar); SmallString<100> Buf; @@ -161,25 +89,7 @@ class UncountedLambdaCapturesChecker } printQuotedQualifiedName(Os, Capture.getCapturedVar()); - Os << " to ref-counted / CheckedPtr capable type is unsafe."; - - PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager()); - auto Report = std::make_unique(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."; + Os << " to uncounted type is unsafe."; PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager()); auto Report = std::make_unique(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 30ea7839a223d9..82c79c97a83de6 100644 --- a/clang/test/Analysis/Checkers/WebKit/mock-types.h +++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h @@ -135,9 +135,7 @@ struct RefCountable { void ref() {} void deref() {} void method(); - void constMethod() const; int trivial() { return 123; } - RefCountable* next(); }; template 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 f676367fe74d0b..27e0a74d583cd3 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp @@ -1,73 +1,39 @@ -// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.UncountedLambdaCapturesChecker -verify %s -//#include "mock-types.h" - -struct RefCountable { -// static Ref create(); - void ref() {} - void deref() {} - void method(); - void constMethod() const; - int trivial() { return 123; } - RefCountable* next(); -}; - -RefCountable* make_obj(); - -void someFunction(); -template void call(Callback callback) { - someFunction(); - callback(); -} +// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.UncountedLambdaCapturesChecker %s 2>&1 | FileCheck %s --strict-whitespace +#include "mock-types.h" void raw_ptr() { - 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); + 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] // Confirm that the checker respects [[clang::suppress]]. RefCountable* suppressed_ref_countable = nullptr; [[clang::suppress]] auto foo5 = [suppressed_ref_countable](){}; - // no warning. - call(foo5); + // CHECK-NOT: warning: Captured raw-pointer 'suppressed_ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker] } 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]}} - call(foo1); - call(foo2); - call(foo3); - call(foo4); + 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] } void quiet() { @@ -79,82 +45,5 @@ void quiet() { auto foo3 = [&]() {}; auto foo4 = [=]() {}; - - call(foo3); - call(foo4); - RefCountable *ref_countable = nullptr; } - -template -void map(RefCountable* start, [[clang::noescape]] Callback&& callback) -{ - while (start) { - callback(*start); - start = start->next(); - } -} - -template -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); - } -}; - -void trivial_lambda() { - RefCountable* ref_countable = make_obj(); - auto trivial_lambda = [&]() { - return ref_countable->trivial(); - }; - trivial_lambda(); -}