WIP smartpointers #164016
Draft
WIP smartpointers #164016
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Created using spr 1.3.7
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: Florian Mayer (fmayer) ChangesFull diff: https://github.com/llvm/llvm-project/pull/164016.diff 6 Files Affected:
diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h
index cb619fb0cb5bb..3b407cc4f20b2 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h
@@ -13,6 +13,7 @@
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Analysis/CFG.h"
#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
+#include "clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h"
#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/Analysis/FlowSensitive/MatchSwitch.h"
@@ -70,7 +71,8 @@ struct UncheckedStatusOrAccessModelOptions {};
// Dataflow analysis that discovers unsafe uses of StatusOr values.
class UncheckedStatusOrAccessModel
- : public DataflowAnalysis<UncheckedStatusOrAccessModel, NoopLattice> {
+ : public DataflowAnalysis<UncheckedStatusOrAccessModel,
+ CachedConstAccessorsLattice<NoopLattice>> {
public:
explicit UncheckedStatusOrAccessModel(ASTContext &Ctx, Environment &Env);
diff --git a/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
index e55b83aa845d4..1f188e3eaa194 100644
--- a/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
+++ b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
@@ -58,6 +58,7 @@ namespace clang::dataflow {
/// for `std::optional`, we assume the (Matcher, TransferFunction) case
/// with custom handling is ordered early so that these generic cases
/// do not trigger.
+ast_matchers::StatementMatcher isSmartPointerLikeContructor();
ast_matchers::StatementMatcher isPointerLikeOperatorStar();
ast_matchers::StatementMatcher isSmartPointerLikeOperatorStar();
ast_matchers::StatementMatcher isPointerLikeOperatorArrow();
@@ -80,6 +81,8 @@ isSmartPointerLikeGetMethodCall(clang::StringRef MethodName = "get");
const FunctionDecl *
getCanonicalSmartPointerLikeOperatorCallee(const CallExpr *CE);
+const FunctionDecl *
+getCanonicalSmartPointerLikeOperatorCallee2(const CXXRecordDecl *RD);
/// A transfer function for `operator*` (and `value`) calls that can be
/// cached. Runs the `InitializeLoc` callback to initialize any new
/// StorageLocations.
@@ -141,6 +144,19 @@ void transferSmartPointerLikeCachedDeref(
State.Env.setStorageLocation(*DerefExpr, LocForValue);
}
+template <typename LatticeT>
+void transferSmartPointerLikeConstructor(
+ const CXXConstructExpr *ConstructOperator,
+ RecordStorageLocation *SmartPointerLoc, TransferState<LatticeT> &State,
+ llvm::function_ref<void(StorageLocation &)> InitializeLoc) {
+ abort();
+ const FunctionDecl *CanonicalCallee =
+ getCanonicalSmartPointerLikeOperatorCallee2(
+ ConstructOperator->getType()->getAsCXXRecordDecl());
+ State.Lattice.getOrCreateConstMethodReturnStorageLocation(
+ *SmartPointerLoc, CanonicalCallee, State.Env, InitializeLoc);
+}
+
template <typename LatticeT>
void transferSmartPointerLikeCachedGet(
const CallExpr *GetExpr, RecordStorageLocation *SmartPointerLoc,
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
index 7a698f276d6c1..8a3670ae6ba5b 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
@@ -25,6 +25,7 @@
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/Analysis/FlowSensitive/MatchSwitch.h"
#include "clang/Analysis/FlowSensitive/RecordOps.h"
+#include "clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h"
#include "clang/Analysis/FlowSensitive/StorageLocation.h"
#include "clang/Analysis/FlowSensitive/Value.h"
#include "clang/Basic/LLVM.h"
@@ -608,6 +609,29 @@ static void transferStatusConstructor(const CXXConstructExpr *Expr,
initializeStatus(StatusLoc, State.Env);
}
+static RecordStorageLocation *
+getSmartPtrLikeStorageLocation(const Expr &E, const Environment &Env) {
+ if (!E.isPRValue())
+ return dyn_cast_or_null<RecordStorageLocation>(Env.getStorageLocation(E));
+ if (auto *PointerVal = dyn_cast_or_null<PointerValue>(Env.getValue(E)))
+ return dyn_cast_or_null<RecordStorageLocation>(
+ &PointerVal->getPointeeLoc());
+ return nullptr;
+}
+
+static void maybeInitLocForExpr(const Expr *E, StorageLocation &Loc,
+ Environment &Env) {
+ auto T = E->getType();
+ if (!T->isPointerOrReferenceType())
+ return;
+ T = T->getPointeeType();
+ if (isStatusOrType(T)) {
+ initializeStatusOr(cast<RecordStorageLocation>(Loc), Env);
+ } else if (isStatusType(T)) {
+ initializeStatus(cast<RecordStorageLocation>(Loc), Env);
+ }
+}
+
CFGMatchSwitch<LatticeTransferState>
buildTransferMatchSwitch(ASTContext &Ctx,
CFGMatchSwitchBuilder<LatticeTransferState> Builder) {
@@ -667,6 +691,47 @@ buildTransferMatchSwitch(ASTContext &Ctx,
transferStatusOrConstructor)
.CaseOfCFGStmt<CXXConstructExpr>(isStatusConstructor(),
transferStatusConstructor)
+ .CaseOfCFGStmt<CXXOperatorCallExpr>(
+ isPointerLikeOperatorArrow(),
+ [](const CXXOperatorCallExpr *E,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ transferSmartPointerLikeCachedGet(
+ E, getSmartPtrLikeStorageLocation(*E->getArg(0), State.Env),
+ State, [&](StorageLocation &Loc) {
+ maybeInitLocForExpr(E, Loc, State.Env);
+ });
+ })
+ .CaseOfCFGStmt<CXXConstructExpr>(
+ isSmartPointerLikeContructor(),
+ [](const CXXConstructExpr *E, const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ transferSmartPointerLikeConstructor(
+ E, &State.Env.getResultObjectLocation(*E), State,
+ [&](StorageLocation &Loc) {
+ maybeInitLocForExpr(E, Loc, State.Env);
+ });
+ })
+ .CaseOfCFGStmt<CXXMemberCallExpr>(
+ isSmartPointerLikeValueMethodCall(),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ transferSmartPointerLikeCachedDeref(
+ E, getImplicitObjectLocation(*E, State.Env), State,
+ [&](StorageLocation &Loc) {
+ maybeInitLocForExpr(E, Loc, State.Env);
+ });
+ })
+ .CaseOfCFGStmt<CXXMemberCallExpr>(
+ isSmartPointerLikeGetMethodCall(),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ transferSmartPointerLikeCachedGet(
+ E, getImplicitObjectLocation(*E, State.Env), State,
+ [&](StorageLocation &Loc) {
+ maybeInitLocForExpr(E, Loc, State.Env);
+ });
+ })
.Build();
}
diff --git a/clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp b/clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp
index d87b2e6f03857..45bfbb07932dd 100644
--- a/clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp
+++ b/clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp
@@ -129,6 +129,12 @@ AST_MATCHER(clang::CXXRecordDecl, pointerClass) {
namespace clang::dataflow {
+ast_matchers::StatementMatcher isSmartPointerLikeContructor() {
+ using namespace ast_matchers;
+ return cxxConstructExpr(hasType(hasCanonicalType(qualType(
+ hasDeclaration(cxxRecordDecl(smartPointerClassWithGetOrValue()))))));
+}
+
ast_matchers::StatementMatcher isSmartPointerLikeOperatorStar() {
return cxxOperatorCallExpr(
hasOverloadedOperatorName("*"),
@@ -177,15 +183,8 @@ isSmartPointerLikeGetMethodCall(clang::StringRef MethodName) {
}
const FunctionDecl *
-getCanonicalSmartPointerLikeOperatorCallee(const CallExpr *CE) {
+getCanonicalSmartPointerLikeOperatorCallee2(const CXXRecordDecl *RD) {
const FunctionDecl *CanonicalCallee = nullptr;
- const CXXMethodDecl *Callee =
- cast_or_null<CXXMethodDecl>(CE->getDirectCallee());
- if (Callee == nullptr)
- return nullptr;
- const CXXRecordDecl *RD = Callee->getParent();
- if (RD == nullptr)
- return nullptr;
for (const auto *MD : RD->methods()) {
if (MD->getOverloadedOperator() == OO_Star && MD->isConst() &&
MD->getNumParams() == 0 && MD->getReturnType()->isReferenceType()) {
@@ -196,4 +195,16 @@ getCanonicalSmartPointerLikeOperatorCallee(const CallExpr *CE) {
return CanonicalCallee;
}
+const FunctionDecl *
+getCanonicalSmartPointerLikeOperatorCallee(const CallExpr *CE) {
+ const CXXMethodDecl *Callee =
+ cast_or_null<CXXMethodDecl>(CE->getDirectCallee());
+ if (Callee == nullptr)
+ return nullptr;
+ const CXXRecordDecl *RD = Callee->getParent();
+ if (RD == nullptr)
+ return nullptr;
+ return getCanonicalSmartPointerLikeOperatorCallee2(RD);
+}
+
} // namespace clang::dataflow
diff --git a/clang/unittests/Analysis/FlowSensitive/MockHeaders.cpp b/clang/unittests/Analysis/FlowSensitive/MockHeaders.cpp
index 2e528edd7c1f9..779d277ccaa6d 100644
--- a/clang/unittests/Analysis/FlowSensitive/MockHeaders.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/MockHeaders.cpp
@@ -1809,6 +1809,53 @@ template <class T1, class T2> pair<T1, T2> make_pair(T1 &&t1, T2 &&t2);
#endif // STD_PAIR_H
)cc";
+static constexpr const char StdUniquePtrHeader[] = R"cc(
+namespace std {
+ template <typename T>
+ struct default_delete {};
+
+ template <typename T, typename D = default_delete<T>>
+ class unique_ptr {
+ public:
+ using element_type = T;
+ using deleter_type = D;
+
+ constexpr unique_ptr();
+ constexpr unique_ptr(nullptr_t) noexcept;
+ unique_ptr(unique_ptr&&);
+ explicit unique_ptr(T*);
+ template <typename U, typename E>
+ unique_ptr(unique_ptr<U, E>&&);
+
+ ~unique_ptr();
+
+ unique_ptr& operator=(unique_ptr&&);
+ template <typename U, typename E>
+ unique_ptr& operator=(unique_ptr<U, E>&&);
+ unique_ptr& operator=(nullptr_t);
+
+ void reset(T* = nullptr) noexcept;
+ T* release();
+ T* get() const;
+
+ T& operator*() const;
+ T* operator->() const;
+ explicit operator bool() const noexcept;
+ };
+
+ template <typename T, typename D>
+ class unique_ptr<T[], D> {
+ public:
+ T* get() const;
+ T& operator[](size_t i);
+ const T& operator[](size_t i) const;
+ };
+
+ template <typename T, typename... Args>
+ unique_ptr<T> make_unique(Args&&...);
+}
+)cc";
+
constexpr const char AbslLogHeader[] = R"cc(
#ifndef ABSL_LOG_H
#define ABSL_LOG_H
@@ -2245,6 +2292,7 @@ std::vector<std::pair<std::string, std::string>> getMockHeaders() {
Headers.emplace_back("base_optional.h", BaseOptionalHeader);
Headers.emplace_back("std_vector.h", StdVectorHeader);
Headers.emplace_back("std_pair.h", StdPairHeader);
+ Headers.emplace_back("std_unique_ptr.h", StdUniquePtrHeader);
Headers.emplace_back("status_defs.h", StatusDefsHeader);
Headers.emplace_back("statusor_defs.h", StatusOrDefsHeader);
Headers.emplace_back("absl_log.h", AbslLogHeader);
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
index 67c37e1e0f77f..70cd6751e4024 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
@@ -3174,6 +3174,23 @@ TEST_P(UncheckedStatusOrAccessModelTest, ConstructStatusFromReference) {
)cc");
}
+TEST_P(UncheckedStatusOrAccessModelTest, SmartPtr) {
+ ExpectDiagnosticsFor(R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+ void target() {
+ const auto sor1 = Make<std::unique_ptr<STATUSOR_INT>>();
+ const auto sor2 = Make<std::unique_ptr<STATUSOR_INT>>();
+ if (!sor1->ok() && !sor2->ok()) return;
+ if (sor1->ok() && !sor2->ok()) {
+ } else if (!sor1->ok() && sor2->ok()) {
+ } else {
+ sor1->value();
+ sor2->value();
+ }
+ }
+ )cc");
+}
+
} // namespace
std::string
@@ -3221,6 +3238,7 @@ GetHeaders(UncheckedStatusOrAccessModelTestAliasKind AliasKind) {
#include "std_optional.h"
#include "std_vector.h"
#include "std_pair.h"
+#include "std_unique_ptr.h"
#include "absl_log.h"
#include "testing_defs.h"
|
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions cpp,h -- clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp clang/unittests/Analysis/FlowSensitive/MockHeaders.cpp clang/unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.h --diff_from_common_commit
View the diff from clang-format here.diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
index 3fb73a421..092639da7 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
@@ -213,12 +213,12 @@ static auto isStatusConstructor() {
}
static auto isNonConstMemberCall() {
- using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
return cxxMemberCallExpr(callee(cxxMethodDecl(unless(isConst()))));
}
static auto isNonConstMemberOperatorCall() {
- using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
return cxxOperatorCallExpr(callee(cxxMethodDecl(unless(isConst()))));
}
@@ -619,9 +619,9 @@ static void transferStatusConstructor(const CXXConstructExpr *Expr,
initializeStatus(StatusLoc, State.Env);
}
-static void transferStatusOrReturningCall(const CallExpr* Expr,
- LatticeTransferState& State) {
- RecordStorageLocation* StatusOrLoc =
+static void transferStatusOrReturningCall(const CallExpr *Expr,
+ LatticeTransferState &State) {
+ RecordStorageLocation *StatusOrLoc =
Expr->isPRValue() ? &State.Env.getResultObjectLocation(*Expr)
: State.Env.get<RecordStorageLocation>(*Expr);
if (StatusOrLoc != nullptr &&
@@ -629,28 +629,30 @@ static void transferStatusOrReturningCall(const CallExpr* Expr,
initializeStatusOr(*StatusOrLoc, State.Env);
}
-static void handleNonConstMemberCall(const CallExpr* Expr,
- RecordStorageLocation* RecordLoc,
- const MatchFinder::MatchResult& Result,
- LatticeTransferState& State) {
- if (RecordLoc == nullptr) return;
+static void handleNonConstMemberCall(const CallExpr *Expr,
+ RecordStorageLocation *RecordLoc,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ if (RecordLoc == nullptr)
+ return;
State.Lattice.clearConstMethodReturnValues(*RecordLoc);
State.Lattice.clearConstMethodReturnStorageLocations(*RecordLoc);
if (isStatusOrType(Expr->getType()))
transferStatusOrReturningCall(Expr, State);
}
-static void transferNonConstMemberCall(const CXXMemberCallExpr* Expr,
- const MatchFinder::MatchResult& Result,
- LatticeTransferState& State) {
+static void transferNonConstMemberCall(const CXXMemberCallExpr *Expr,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
handleNonConstMemberCall(Expr, getImplicitObjectLocation(*Expr, State.Env),
Result, State);
}
-static void transferNonConstMemberOperatorCall(
- const CXXOperatorCallExpr* Expr, const MatchFinder::MatchResult& Result,
- LatticeTransferState& State) {
- auto* RecordLoc = cast_or_null<RecordStorageLocation>(
+static void
+transferNonConstMemberOperatorCall(const CXXOperatorCallExpr *Expr,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ auto *RecordLoc = cast_or_null<RecordStorageLocation>(
State.Env.getStorageLocation(*Expr->getArg(0)));
handleNonConstMemberCall(Expr, RecordLoc, Result, State);
}
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.