[clang] [analyzer] WebKit checkers: support ref and deref defined on different classes. (PR #68170)
Artem Dergachev via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 19 15:32:09 PDT 2023
================
@@ -44,9 +46,25 @@ bool hasPublicRefAndDeref(const CXXRecordDecl *R) {
namespace clang {
-std::optional<const clang::CXXRecordDecl*>
-isRefCountable(const CXXBaseSpecifier* Base)
-{
+std::optional<const clang::CXXRecordDecl *>
+hasPublicRefInBase(const CXXBaseSpecifier *Base) {
+ assert(Base);
+
+ const Type *T = Base->getType().getTypePtrOrNull();
+ if (!T)
+ return std::nullopt;
----------------
haoNoQ wrote:
All these nullopt cases are so frustrating to spell out! It looks like the only reason we have them is that we wanted to analyze not-fully-instantiated templates. (If they're ever instantiated, we'd still reanalyze each instantiation. So it only fights false negatives on templates that literally nobody instantiates.) In particular, this alternative approach doesn't seem to cause any test failures:
```diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index a66fa38315f4..042682487710 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -46,47 +46,31 @@ bool hasPublicDerefMethod(const CXXRecordDecl *R) {
namespace clang {
-std::optional<const clang::CXXRecordDecl *>
+const CXXRecordDecl *
hasPublicRefInBase(const CXXBaseSpecifier *Base) {
assert(Base);
- const Type *T = Base->getType().getTypePtrOrNull();
- if (!T)
- return std::nullopt;
-
+ QualType T = Base->getType();
const CXXRecordDecl *R = T->getAsCXXRecordDecl();
- if (!R)
- return std::nullopt;
- if (!R->hasDefinition())
- return std::nullopt;
-
+ assert(R && R->hasDefinition());
return hasPublicRefMethod(R) ? R : nullptr;
}
-std::optional<const clang::CXXRecordDecl *>
+const CXXRecordDecl *
hasPublicDerefInBase(const CXXBaseSpecifier *Base) {
assert(Base);
- const Type *T = Base->getType().getTypePtrOrNull();
- if (!T)
- return std::nullopt;
-
+ QualType T = Base->getType();
const CXXRecordDecl *R = T->getAsCXXRecordDecl();
- if (!R)
- return std::nullopt;
- if (!R->hasDefinition())
- return std::nullopt;
-
+ assert(R && R->hasDefinition());
return hasPublicDerefMethod(R) ? R : nullptr;
}
std::optional<bool> isRefCountable(const CXXRecordDecl* R)
{
assert(R);
-
R = R->getDefinition();
- if (!R)
- return std::nullopt;
+ assert(R);
bool hasRef = hasPublicRefMethod(R);
bool hasDeref = hasPublicDerefMethod(R);
@@ -96,37 +80,19 @@ std::optional<bool> isRefCountable(const CXXRecordDecl* R)
CXXBasePaths Paths;
Paths.setOrigin(const_cast<CXXRecordDecl *>(R));
- bool AnyInconclusiveBase = false;
- const auto hasPublicRefInBase =
- [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
- auto hasRefInBase = clang::hasPublicRefInBase(Base);
- if (!hasRefInBase) {
- AnyInconclusiveBase = true;
- return false;
- }
- return (*hasRefInBase) != nullptr;
- };
-
- hasRef =
- R->lookupInBases(hasPublicRefInBase, Paths, /*LookupInDependent =*/true);
- if (AnyInconclusiveBase)
- return std::nullopt;
-
- const auto hasPublicDerefInBase =
- [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
- auto hasDerefInBase = clang::hasPublicDerefInBase(Base);
- if (!hasDerefInBase) {
- AnyInconclusiveBase = true;
- return false;
- }
- return (*hasDerefInBase) != nullptr;
- };
- hasDeref = R->lookupInBases(hasPublicDerefInBase, Paths,
- /*LookupInDependent =*/true);
- if (AnyInconclusiveBase)
- return std::nullopt;
-
- return hasRef && hasDeref;
+ bool hasDeepRef = R->lookupInBases(
+ [](const CXXBaseSpecifier *B, CXXBasePath &) {
+ return (bool)hasPublicRefInBase(B);
+ },
+ Paths,
+ /*LookupInDependent =*/true);
+ bool hasDeepDeref = R->lookupInBases(
+ [](const CXXBaseSpecifier *B, CXXBasePath &) {
+ return (bool)hasPublicDerefInBase(B);
+ },
+ Paths,
+ /*LookupInDependent =*/true);
+ return (hasRef || hasDeepRef) && (hasDeref || hasDeepDeref);
}
bool isCtorOfRefCounted(const clang::FunctionDecl *F) {
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
index 9c7a933b9ee9..ec80870d58e9 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
@@ -27,13 +27,13 @@ class Type;
// Ref<T>.
/// \returns CXXRecordDecl of the base if the type has ref as a public method,
-/// nullptr if not, std::nullopt if inconclusive.
-std::optional<const clang::CXXRecordDecl *>
+/// nullptr if not.
+const clang::CXXRecordDecl *
hasPublicRefInBase(const CXXBaseSpecifier *Base);
/// \returns CXXRecordDecl of the base if the type has deref as a public
-/// method, nullptr if not, std::nullopt if inconclusive.
-std::optional<const clang::CXXRecordDecl *>
+/// method, nullptr if not.
+const clang::CXXRecordDecl *
hasPublicDerefInBase(const CXXBaseSpecifier *Base);
/// \returns true if \p Class is ref-countable, false if not, std::nullopt if
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index 6b8b952c8274..9421208adc4a 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -77,53 +77,35 @@ public:
(AccSpec == AS_none && RD->isClass()))
return false;
- auto hasRefInBase = clang::hasPublicRefInBase(Base);
- auto hasDerefInBase = clang::hasPublicDerefInBase(Base);
+ bool hasRef = hasPublicRefInBase(Base);
+ bool hasDeref = hasPublicDerefInBase(Base);
- bool hasRef = hasRefInBase && *hasRefInBase != nullptr;
- bool hasDeref = hasDerefInBase && *hasDerefInBase != nullptr;
+ QualType T = Base->getType();
+ const CXXRecordDecl *R = T->getAsCXXRecordDecl();
- const Type *T = Base->getType().getTypePtrOrNull();
- if (!T)
- return false;
-
- const CXXRecordDecl *C = T->getAsCXXRecordDecl();
- if (!C)
- return false;
- bool AnyInconclusiveBase = false;
- const auto hasPublicRefInBase =
- [&AnyInconclusiveBase](const CXXBaseSpecifier *Base,
- CXXBasePath &) {
- auto hasRefInBase = clang::hasPublicRefInBase(Base);
- if (!hasRefInBase) {
- AnyInconclusiveBase = true;
- return false;
- }
- return (*hasRefInBase) != nullptr;
- };
- const auto hasPublicDerefInBase =
- [&AnyInconclusiveBase](const CXXBaseSpecifier *Base,
- CXXBasePath &) {
- auto hasDerefInBase = clang::hasPublicDerefInBase(Base);
- if (!hasDerefInBase) {
- AnyInconclusiveBase = true;
- return false;
- }
- return (*hasDerefInBase) != nullptr;
- };
CXXBasePaths Paths;
- Paths.setOrigin(C);
- hasRef = hasRef || C->lookupInBases(hasPublicRefInBase, Paths,
- /*LookupInDependent =*/true);
- hasDeref = hasDeref || C->lookupInBases(hasPublicDerefInBase, Paths,
- /*LookupInDependent =*/true);
- if (AnyInconclusiveBase || !hasRef || !hasDeref)
+ Paths.setOrigin(R);
+ bool hasDeepRef = R->lookupInBases(
+ [](const CXXBaseSpecifier *B, CXXBasePath &) {
+ return (bool)hasPublicRefInBase(B);
+ },
+ Paths,
+ /*LookupInDependent =*/true);
+ bool hasDeepDeref = R->lookupInBases(
+ [](const CXXBaseSpecifier *B, CXXBasePath &) {
+ return (bool)hasPublicDerefInBase(B);
+ },
+ Paths,
+ /*LookupInDependent =*/true);
+ hasRef = hasRef || hasDeepRef;
+ hasDeref = hasDeref || hasDeepDeref;
+ if (!hasRef || !hasDeref)
return false;
- const auto *Dtor = C->getDestructor();
+ const auto *Dtor = R->getDestructor();
if (!Dtor || !Dtor->isVirtual()) {
ProblematicBaseSpecifier = Base;
- ProblematicBaseClass = C;
+ ProblematicBaseClass = R;
return true;
}
@@ -137,6 +119,16 @@ public:
}
bool shouldSkipDecl(const CXXRecordDecl *RD) const {
+ // Do not visit templates. We'll visit instantiations anyway
+ // and they're much easier to analyze.
+ if (const ClassTemplateDecl *TD = RD->getDescribedClassTemplate()) {
+ if (TD->getTemplatedDecl() == RD)
+ return true;
+
+ if (RD->getTemplateSpecializationKind() == TSK_ExplicitSpecialization)
+ return true;
+ }
+
if (!RD->isThisDeclarationADefinition())
return true;
```
https://github.com/llvm/llvm-project/pull/68170
More information about the cfe-commits
mailing list