Skip to content

Commit

Permalink
[alpha.webkit.UncountedLocalVarsChecker] Allow uncounted object refer…
Browse files Browse the repository at this point in the history 
…ences within trivial statements (#82229)

This PR makes alpha.webkit.UncountedLocalVarsChecker ignore raw
references and pointers to a ref counted type which appears within
"trival" statements. To do this, this PR extends TrivialFunctionAnalysis
so that it can also analyze "triviality" of statements as well as that
of functions Each Visit* function is now augmented with
withCachedResult, which is responsible for looking up and updating the
cache for each Visit* functions.

As this PR dramatically improves the false positive rate of the checker,
it also deletes the code to ignore raw pointers and references within if
and for statements.
  • Loading branch information
@rniwa
rniwa committed Mar 7, 2024
1 parent c59129a commit 7ce1cfe
Show file tree
Hide file tree
Showing 5 changed files with 203 additions and 65 deletions.
76 changes: 55 additions & 21 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,19 @@ class TrivialFunctionAnalysisVisitor
return true;
}

template <typename CheckFunction>
bool WithCachedResult(const Stmt *S, CheckFunction Function) {
// If the statement isn't in the cache, conservatively assume that
// it's not trivial until analysis completes. Insert false to the cache
// first to avoid infinite recursion.
auto [It, IsNew] = Cache.insert(std::make_pair(S, false));
if (!IsNew)
return It->second;
bool Result = Function();
Cache[S] = Result;
return Result;
}

public:
using CacheTy = TrivialFunctionAnalysis::CacheTy;

Expand All @@ -267,7 +280,7 @@ class TrivialFunctionAnalysisVisitor
bool VisitCompoundStmt(const CompoundStmt *CS) {
// A compound statement is allowed as long each individual sub-statement
// is trivial.
return VisitChildren(CS);
return WithCachedResult(CS, [&]() { return VisitChildren(CS); });
}

bool VisitReturnStmt(const ReturnStmt *RS) {
Expand All @@ -279,17 +292,36 @@ class TrivialFunctionAnalysisVisitor

bool VisitDeclStmt(const DeclStmt *DS) { return VisitChildren(DS); }
bool VisitDoStmt(const DoStmt *DS) { return VisitChildren(DS); }
bool VisitIfStmt(const IfStmt *IS) { return VisitChildren(IS); }
bool VisitIfStmt(const IfStmt *IS) {
return WithCachedResult(IS, [&]() { return VisitChildren(IS); });
}
bool VisitForStmt(const ForStmt *FS) {
return WithCachedResult(FS, [&]() { return VisitChildren(FS); });
}
bool VisitCXXForRangeStmt(const CXXForRangeStmt *FS) {
return WithCachedResult(FS, [&]() { return VisitChildren(FS); });
}
bool VisitWhileStmt(const WhileStmt *WS) {
return WithCachedResult(WS, [&]() { return VisitChildren(WS); });
}
bool VisitSwitchStmt(const SwitchStmt *SS) { return VisitChildren(SS); }
bool VisitCaseStmt(const CaseStmt *CS) { return VisitChildren(CS); }
bool VisitDefaultStmt(const DefaultStmt *DS) { return VisitChildren(DS); }

bool VisitUnaryOperator(const UnaryOperator *UO) {
// Operator '*' and '!' are allowed as long as the operand is trivial.
if (UO->getOpcode() == UO_Deref || UO->getOpcode() == UO_AddrOf ||
UO->getOpcode() == UO_LNot)
auto op = UO->getOpcode();
if (op == UO_Deref || op == UO_AddrOf || op == UO_LNot)
return Visit(UO->getSubExpr());

if (UO->isIncrementOp() || UO->isDecrementOp()) {
// Allow increment or decrement of a POD type.
if (auto *RefExpr = dyn_cast<DeclRefExpr>(UO->getSubExpr())) {
if (auto *Decl = dyn_cast<VarDecl>(RefExpr->getDecl()))
return Decl->isLocalVarDeclOrParm() &&
Decl->getType().isPODType(Decl->getASTContext());
}
}
// Other operators are non-trivial.
return false;
}
Expand All @@ -304,22 +336,6 @@ class TrivialFunctionAnalysisVisitor
return VisitChildren(CO);
}

bool VisitDeclRefExpr(const DeclRefExpr *DRE) {
if (auto *decl = DRE->getDecl()) {
if (isa<ParmVarDecl>(decl))
return true;
if (isa<EnumConstantDecl>(decl))
return true;
if (auto *VD = dyn_cast<VarDecl>(decl)) {
if (VD->hasConstantInitialization() && VD->getEvaluatedValue())
return true;
auto *Init = VD->getInit();
return !Init || Visit(Init);
}
}
return false;
}

bool VisitAtomicExpr(const AtomicExpr *E) { return VisitChildren(E); }

bool VisitStaticAssertDecl(const StaticAssertDecl *SAD) {
Expand Down Expand Up @@ -436,6 +452,11 @@ class TrivialFunctionAnalysisVisitor
return true;
}

bool VisitDeclRefExpr(const DeclRefExpr *DRE) {
// The use of a variable is trivial.
return true;
}

// Constant literal expressions are always trivial
bool VisitIntegerLiteral(const IntegerLiteral *E) { return true; }
bool VisitFloatingLiteral(const FloatingLiteral *E) { return true; }
Expand All @@ -449,7 +470,7 @@ class TrivialFunctionAnalysisVisitor
}

private:
CacheTy Cache;
CacheTy &Cache;
};

bool TrivialFunctionAnalysis::isTrivialImpl(
Expand All @@ -474,4 +495,17 @@ bool TrivialFunctionAnalysis::isTrivialImpl(
return Result;
}

bool TrivialFunctionAnalysis::isTrivialImpl(
const Stmt *S, TrivialFunctionAnalysis::CacheTy &Cache) {
// If the statement isn't in the cache, conservatively assume that
// it's not trivial until analysis completes. Unlike a function case,
// we don't insert an entry into the cache until Visit returns
// since Visit* functions themselves make use of the cache.

TrivialFunctionAnalysisVisitor V(Cache);
bool Result = V.Visit(S);
assert(Cache.contains(S) && "Top-level statement not properly cached!");
return Result;
}

} // namespace clang
7 changes: 6 additions & 1 deletion clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

#include "llvm/ADT/APInt.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/PointerUnion.h"
#include <optional>

namespace clang {
Expand All @@ -19,6 +20,7 @@ class CXXMethodDecl;
class CXXRecordDecl;
class Decl;
class FunctionDecl;
class Stmt;
class Type;

// Ref-countability of a type is implicitly defined by Ref<T> and RefPtr<T>
Expand Down Expand Up @@ -71,14 +73,17 @@ class TrivialFunctionAnalysis {
public:
/// \returns true if \p D is a "trivial" function.
bool isTrivial(const Decl *D) const { return isTrivialImpl(D, TheCache); }
bool isTrivial(const Stmt *S) const { return isTrivialImpl(S, TheCache); }

private:
friend class TrivialFunctionAnalysisVisitor;

using CacheTy = llvm::DenseMap<const Decl *, bool>;
using CacheTy =
llvm::DenseMap<llvm::PointerUnion<const Decl *, const Stmt *>, bool>;
mutable CacheTy TheCache{};

static bool isTrivialImpl(const Decl *D, CacheTy &Cache);
static bool isTrivialImpl(const Stmt *S, CacheTy &Cache);
};

} // namespace clang
Expand Down
82 changes: 45 additions & 37 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,28 +26,6 @@ using namespace ento;

namespace {

// for ( int a = ...) ... true
// for ( int a : ...) ... true
// if ( int* a = ) ... true
// anything else ... false
bool isDeclaredInForOrIf(const VarDecl *Var) {
assert(Var);
auto &ASTCtx = Var->getASTContext();
auto parent = ASTCtx.getParents(*Var);

if (parent.size() == 1) {
if (auto *DS = parent.begin()->get<DeclStmt>()) {
DynTypedNodeList grandParent = ASTCtx.getParents(*DS);
if (grandParent.size() == 1) {
return grandParent.begin()->get<ForStmt>() ||
grandParent.begin()->get<IfStmt>() ||
grandParent.begin()->get<CXXForRangeStmt>();
}
}
}
return false;
}

// FIXME: should be defined by anotations in the future
bool isRefcountedStringsHack(const VarDecl *V) {
assert(V);
Expand Down Expand Up @@ -143,6 +121,11 @@ class UncountedLocalVarsChecker
// want to visit those, so we make our own RecursiveASTVisitor.
struct LocalVisitor : public RecursiveASTVisitor<LocalVisitor> {
const UncountedLocalVarsChecker *Checker;

TrivialFunctionAnalysis TFA;

using Base = RecursiveASTVisitor<LocalVisitor>;

explicit LocalVisitor(const UncountedLocalVarsChecker *Checker)
: Checker(Checker) {
assert(Checker);
Expand All @@ -155,6 +138,36 @@ class UncountedLocalVarsChecker
Checker->visitVarDecl(V);
return true;
}

bool TraverseIfStmt(IfStmt *IS) {
if (!TFA.isTrivial(IS))
return Base::TraverseIfStmt(IS);
return true;
}

bool TraverseForStmt(ForStmt *FS) {
if (!TFA.isTrivial(FS))
return Base::TraverseForStmt(FS);
return true;
}

bool TraverseCXXForRangeStmt(CXXForRangeStmt *FRS) {
if (!TFA.isTrivial(FRS))
return Base::TraverseCXXForRangeStmt(FRS);
return true;
}

bool TraverseWhileStmt(WhileStmt *WS) {
if (!TFA.isTrivial(WS))
return Base::TraverseWhileStmt(WS);
return true;
}

bool TraverseCompoundStmt(CompoundStmt *CS) {
if (!TFA.isTrivial(CS))
return Base::TraverseCompoundStmt(CS);
return true;
}
};

LocalVisitor visitor(this);
Expand Down Expand Up @@ -189,18 +202,16 @@ class UncountedLocalVarsChecker
dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) {
const auto *MaybeGuardianArgType =
MaybeGuardian->getType().getTypePtr();
if (!MaybeGuardianArgType)
return;
const CXXRecordDecl *const MaybeGuardianArgCXXRecord =
MaybeGuardianArgType->getAsCXXRecordDecl();
if (!MaybeGuardianArgCXXRecord)
return;

if (MaybeGuardian->isLocalVarDecl() &&
(isRefCounted(MaybeGuardianArgCXXRecord) ||
isRefcountedStringsHack(MaybeGuardian)) &&
isGuardedScopeEmbeddedInGuardianScope(V, MaybeGuardian)) {
return;
if (MaybeGuardianArgType) {
const CXXRecordDecl *const MaybeGuardianArgCXXRecord =
MaybeGuardianArgType->getAsCXXRecordDecl();
if (MaybeGuardianArgCXXRecord) {
if (MaybeGuardian->isLocalVarDecl() &&
(isRefCounted(MaybeGuardianArgCXXRecord) ||
isRefcountedStringsHack(MaybeGuardian)) &&
isGuardedScopeEmbeddedInGuardianScope(V, MaybeGuardian))
return;
}
}

// Parameters are guaranteed to be safe for the duration of the call
Expand All @@ -219,9 +230,6 @@ class UncountedLocalVarsChecker
if (!V->isLocalVarDecl())
return true;

if (isDeclaredInForOrIf(V))
return true;

return false;
}

Expand Down
2 changes: 2 additions & 0 deletions clang/test/Analysis/Checkers/WebKit/mock-types.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ struct RefCountable {
static Ref<RefCountable> create();
void ref() {}
void deref() {}
void method();
int trivial() { return 123; }
};

template <typename T> T *downcast(T *t) { return t; }
Expand Down
Loading

0 comments on commit 7ce1cfe

Please sign in to comment.