Skip to content

Commit

Permalink
Revert "[webkit.UncountedLambdaCapturesChecker] Ignore trivial functi…
Browse files Browse the repository at this point in the history 
…ons and [[clang::noescape]]." (#114372)

Reverts #113845. Introduced a test failure.
  • Loading branch information
@rniwa
rniwa authored Oct 31, 2024
1 parent 287781c commit dadfd4a
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 240 deletions.
4 changes: 0 additions & 4 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,6 @@ std::optional<bool> isUncounted(const clang::CXXRecordDecl* Class);
/// class, false if not, std::nullopt if inconclusive.
std::optional<bool> 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<bool> 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);
Expand Down
108 changes: 9 additions & 99 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
//
//===----------------------------------------------------------------------===//

#include "ASTUtils.h"
#include "DiagOutputUtils.h"
#include "PtrTypesSemantics.h"
#include "clang/AST/CXXInheritance.h"
Expand All @@ -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,
Expand All @@ -39,8 +37,6 @@ class UncountedLambdaCapturesChecker
// want to visit those, so we make our own RecursiveASTVisitor.
struct LocalVisitor : public RecursiveASTVisitor<LocalVisitor> {
const UncountedLambdaCapturesChecker *Checker;
llvm::DenseSet<const DeclRefExpr *> DeclRefExprsToIgnore;

explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker)
: Checker(Checker) {
assert(Checker);
Expand All @@ -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<VarDecl>(DRE->getDecl());
if (!VD)
return true;
auto *Init = VD->getInit()->IgnoreParenCasts();
auto *L = dyn_cast_or_null<LambdaExpr>(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<NamespaceDecl>(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<NoEscapeAttr>() && !TreatAllArgsAsNoEscape) {
if (auto *L = dyn_cast_or_null<LambdaExpr>(Arg))
Checker->visitLambdaExpr(L);
}
++ArgIndex;
}
}
return true;
}

void checkCalleeLambda(CallExpr *CE) {
auto *Callee = CE->getCallee();
if (!Callee)
return;
auto *DRE = dyn_cast<DeclRefExpr>(Callee->IgnoreParenCasts());
if (!DRE)
return;
auto *MD = dyn_cast_or_null<CXXMethodDecl>(DRE->getDecl());
if (!MD || CE->getNumArgs() != 1)
return;
auto *Arg = CE->getArg(0)->IgnoreParenCasts();
auto *ArgRef = dyn_cast<DeclRefExpr>(Arg);
if (!ArgRef)
return;
auto *VD = dyn_cast_or_null<VarDecl>(ArgRef->getDecl());
if (!VD)
return;
auto *Init = VD->getInit()->IgnoreParenCasts();
auto *L = dyn_cast_or_null<LambdaExpr>(Init);
if (!L)
return;
DeclRefExprsToIgnore.insert(ArgRef);
Checker->visitLambdaExpr(L, /* ignoreParamVarDecl */ true);
}
};

LocalVisitor visitor(this);
visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(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<ParmVarDecl>(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;
Expand All @@ -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<BasicBugReport>(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<BasicBugReport>(Bug, Os.str(), BSLoc);
Expand Down
2 changes: 0 additions & 2 deletions clang/test/Analysis/Checkers/WebKit/mock-types.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,7 @@ struct RefCountable {
void ref() {}
void deref() {}
void method();
void constMethod() const;
int trivial() { return 123; }
RefCountable* next();
};

template <typename T> T *downcast(T *t) { return t; }
Expand Down
159 changes: 24 additions & 135 deletions clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,73 +1,39 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.UncountedLambdaCapturesChecker -verify %s
//#include "mock-types.h"

struct RefCountable {
// static Ref<RefCountable> create();
void ref() {}
void deref() {}
void method();
void constMethod() const;
int trivial() { return 123; }
RefCountable* next();
};

RefCountable* make_obj();

void someFunction();
template <typename Callback> 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() {
Expand All @@ -79,82 +45,5 @@ void quiet() {

auto foo3 = [&]() {};
auto foo4 = [=]() {};

call(foo3);
call(foo4);

RefCountable *ref_countable = nullptr;
}

template <typename Callback>
void map(RefCountable* start, [[clang::noescape]] Callback&& callback)
{
while (start) {
callback(*start);
start = start->next();
}
}

template <typename Callback1, typename Callback2>
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();
}

0 comments on commit dadfd4a

Please sign in to comment.