Skip to content

[Analyzer][WebKit] Use tri-state types for relevant predicates #2037

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 26, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) {
}
if (auto *call = dyn_cast<CallExpr>(E)) {
if (auto *memberCall = dyn_cast<CXXMemberCallExpr>(call)) {
if (isGetterOfRefCounted(memberCall->getMethodDecl())) {
Optional<bool> IsGetterOfRefCt =
isGetterOfRefCounted(memberCall->getMethodDecl());
if (IsGetterOfRefCt && *IsGetterOfRefCt) {
E = memberCall->getImplicitObjectArgument();
if (StopAtFirstRefCountedObj) {
return {E, true};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,11 @@ class NoUncountedMemberChecker

if (auto *MemberCXXRD = MemberType->getPointeeCXXRecordDecl()) {
// If we don't see the definition we just don't know.
if (MemberCXXRD->hasDefinition() && isRefCountable(MemberCXXRD))
reportBug(Member, MemberType, MemberCXXRD, RD);
if (MemberCXXRD->hasDefinition()) {
llvm::Optional<bool> isRCAble = isRefCountable(MemberCXXRD);
if (isRCAble && *isRCAble)
reportBug(Member, MemberType, MemberCXXRD, RD);
}
}
}
}
Expand Down
58 changes: 40 additions & 18 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "clang/AST/Decl.h"
#include "clang/AST/DeclCXX.h"
#include "clang/AST/ExprCXX.h"
#include "llvm/ADT/Optional.h"

using llvm::Optional;
using namespace clang;
Expand All @@ -20,6 +21,7 @@ namespace {

bool hasPublicRefAndDeref(const CXXRecordDecl *R) {
assert(R);
assert(R->hasDefinition());

bool hasRef = false;
bool hasDeref = false;
Expand All @@ -43,39 +45,54 @@ bool hasPublicRefAndDeref(const CXXRecordDecl *R) {

namespace clang {

const CXXRecordDecl *isRefCountable(const CXXBaseSpecifier *Base) {
llvm::Optional<const clang::CXXRecordDecl *>
isRefCountable(const CXXBaseSpecifier *Base) {
assert(Base);

const Type *T = Base->getType().getTypePtrOrNull();
if (!T)
return nullptr;
return llvm::None;

const CXXRecordDecl *R = T->getAsCXXRecordDecl();
if (!R)
return nullptr;
return llvm::None;
if (!R->hasDefinition())
return llvm::None;

return hasPublicRefAndDeref(R) ? R : nullptr;
}

bool isRefCountable(const CXXRecordDecl *R) {
llvm::Optional<bool> isRefCountable(const CXXRecordDecl *R) {
assert(R);

R = R->getDefinition();
assert(R);
if (!R)
return llvm::None;

if (hasPublicRefAndDeref(R))
return true;

CXXBasePaths Paths;
Paths.setOrigin(const_cast<CXXRecordDecl *>(R));

const auto isRefCountableBase = [](const CXXBaseSpecifier *Base,
CXXBasePath &) {
return clang::isRefCountable(Base);
};
bool AnyInconclusiveBase = false;
const auto isRefCountableBase =
[&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
Optional<const clang::CXXRecordDecl *> IsRefCountable =
clang::isRefCountable(Base);
if (!IsRefCountable) {
AnyInconclusiveBase = true;
return false;
}
return (*IsRefCountable) != nullptr;
};

bool BasesResult = R->lookupInBases(isRefCountableBase, Paths,
/*LookupInDependent =*/true);
if (AnyInconclusiveBase)
return llvm::None;

return R->lookupInBases(isRefCountableBase, Paths,
/*LookupInDependent =*/true);
return BasesResult;
}

bool isCtorOfRefCounted(const clang::FunctionDecl *F) {
Expand All @@ -95,12 +112,19 @@ bool isCtorOfRefCounted(const clang::FunctionDecl *F) {
|| FunctionName == "Identifier";
}

bool isUncounted(const CXXRecordDecl *Class) {
llvm::Optional<bool> isUncounted(const CXXRecordDecl *Class) {
// Keep isRefCounted first as it's cheaper.
return !isRefCounted(Class) && isRefCountable(Class);
if (isRefCounted(Class))
return false;

llvm::Optional<bool> IsRefCountable = isRefCountable(Class);
if (!IsRefCountable)
return llvm::None;

return (*IsRefCountable);
}

bool isUncountedPtr(const Type *T) {
llvm::Optional<bool> isUncountedPtr(const Type *T) {
assert(T);

if (T->isPointerType() || T->isReferenceType()) {
Expand All @@ -111,7 +135,7 @@ bool isUncountedPtr(const Type *T) {
return false;
}

bool isGetterOfRefCounted(const CXXMethodDecl *M) {
Optional<bool> isGetterOfRefCounted(const CXXMethodDecl *M) {
assert(M);

if (isa<CXXMethodDecl>(M)) {
Expand All @@ -133,9 +157,7 @@ bool isGetterOfRefCounted(const CXXMethodDecl *M) {
if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M)) {
if (auto *targetConversionType =
maybeRefToRawOperator->getConversionType().getTypePtrOrNull()) {
if (isUncountedPtr(targetConversionType)) {
return true;
}
return isUncountedPtr(targetConversionType);
}
}
}
Expand Down
23 changes: 13 additions & 10 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
#ifndef LLVM_CLANG_ANALYZER_WEBKIT_PTRTYPESEMANTICS_H
#define LLVM_CLANG_ANALYZER_WEBKIT_PTRTYPESEMANTICS_H

#include "llvm/ADT/APInt.h"

namespace clang {
class CXXBaseSpecifier;
class CXXMethodDecl;
Expand All @@ -25,30 +27,31 @@ class Type;
// Ref<T>.

/// \returns CXXRecordDecl of the base if the type is ref-countable, nullptr if
/// not.
const clang::CXXRecordDecl *isRefCountable(const clang::CXXBaseSpecifier *Base);
/// not, None if inconclusive.
llvm::Optional<const clang::CXXRecordDecl *>
isRefCountable(const clang::CXXBaseSpecifier *Base);

/// \returns true if \p Class is ref-countable, false if not.
/// Asserts that \p Class IS a definition.
bool isRefCountable(const clang::CXXRecordDecl *Class);
/// \returns true if \p Class is ref-countable, false if not, None if
/// inconclusive.
llvm::Optional<bool> isRefCountable(const clang::CXXRecordDecl *Class);

/// \returns true if \p Class is ref-counted, false if not.
bool isRefCounted(const clang::CXXRecordDecl *Class);

/// \returns true if \p Class is ref-countable AND not ref-counted, false if
/// not. Asserts that \p Class IS a definition.
bool isUncounted(const clang::CXXRecordDecl *Class);
/// not, None if inconclusive.
llvm::Optional<bool> isUncounted(const clang::CXXRecordDecl *Class);

/// \returns true if \p T is either a raw pointer or reference to an uncounted
/// class, false if not.
bool isUncountedPtr(const clang::Type *T);
/// class, false if not, None if inconclusive.
llvm::Optional<bool> isUncountedPtr(const clang::Type *T);

/// \returns true if \p F creates ref-countable object from uncounted parameter,
/// false if not.
bool isCtorOfRefCounted(const clang::FunctionDecl *F);

/// \returns true if \p M is getter of a ref-counted class, false if not.
bool isGetterOfRefCounted(const clang::CXXMethodDecl *Method);
llvm::Optional<bool> isGetterOfRefCounted(const clang::CXXMethodDecl *Method);

/// \returns true if \p F is a conversion between ref-countable or ref-counted
/// pointer types.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,19 +76,15 @@ class RefCntblBaseVirtualDtorChecker
(AccSpec == AS_none && RD->isClass()))
return false;

llvm::Optional<const clang::CXXRecordDecl *> MaybeRefCntblBaseRD =
llvm::Optional<const CXXRecordDecl *> RefCntblBaseRD =
isRefCountable(Base);
if (!MaybeRefCntblBaseRD.hasValue())
if (!RefCntblBaseRD || !(*RefCntblBaseRD))
return false;

const CXXRecordDecl *RefCntblBaseRD = MaybeRefCntblBaseRD.getValue();
if (!RefCntblBaseRD)
return false;

const auto *Dtor = RefCntblBaseRD->getDestructor();
const auto *Dtor = (*RefCntblBaseRD)->getDestructor();
if (!Dtor || !Dtor->isVirtual()) {
ProblematicBaseSpecifier = Base;
ProblematicBaseClass = RefCntblBaseRD;
ProblematicBaseClass = *RefCntblBaseRD;
return true;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,8 @@ class UncountedCallArgsChecker
continue; // FIXME? Should we bail?

// FIXME: more complex types (arrays, references to raw pointers, etc)
if (!isUncountedPtr(ArgType))
Optional<bool> IsUncounted = isUncountedPtr(ArgType);
if (!IsUncounted || !(*IsUncounted))
continue;

const auto *Arg = CE->getArg(ArgIdx);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ class UncountedLambdaCapturesChecker
if (C.capturesVariable()) {
VarDecl *CapturedVar = C.getCapturedVar();
if (auto *CapturedVarType = CapturedVar->getType().getTypePtrOrNull()) {
if (isUncountedPtr(CapturedVarType)) {
Optional<bool> IsUncountedPtr = isUncountedPtr(CapturedVarType);
if (IsUncountedPtr && *IsUncountedPtr) {
reportBug(C, CapturedVar, CapturedVarType);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,8 @@ class UncountedLocalVarsChecker
if (!ArgType)
return;

if (isUncountedPtr(ArgType)) {
Optional<bool> IsUncountedPtr = isUncountedPtr(ArgType);
if (IsUncountedPtr && *IsUncountedPtr) {
const Expr *const InitExpr = V->getInit();
if (!InitExpr)
return; // FIXME: later on we might warn on uninitialized vars too
Expand Down