https://github.com/jvoung created
https://github.com/llvm/llvm-project/pull/113922
Previously, we covered returning refs, or copies of optional, and bools.
Now cover returning pointers (to any type).
This is useful for cases like operator-> of smart pointers.
Addresses more of issue llvm#58510
>From 21f15146b8a7941781b6d728cdbb0d0be50b02fc Mon Sep 17 00:00:00 2001
From: Jan Voung <jvo...@gmail.com>
Date: Mon, 28 Oct 2024 14:45:39 +0000
Subject: [PATCH] [clang][dataflow] Cache accessors returning pointers in
bugprone-unchecked-optional-access
Previously, we covered returning refs, or copies of optional, and bools.
Now cover returning pointers (to any type).
This is useful for cases like operator-> of smart pointers.
Addresses more of issue llvm#58510
---
.../Models/UncheckedOptionalAccessModel.h | 8 +++
.../Models/UncheckedOptionalAccessModel.cpp | 20 +++++-
.../UncheckedOptionalAccessModelTest.cpp | 72 ++++++++++++++++---
3 files changed, 87 insertions(+), 13 deletions(-)
diff --git
a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
index 9d81cacb507351..713494178b97bd 100644
---
a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
+++
b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
@@ -37,6 +37,14 @@ struct UncheckedOptionalAccessModelOptions {
/// can't identify when their results are used safely (across calls),
/// resulting in false positives in all such cases. Note: this option does
not
/// cover access through `operator[]`.
+ /// FIXME: we currently cache and equate the result of const accessors
+ /// returning pointers, so cover the case of operator-> followed by
+ /// operator->, which covers the common case of smart pointers. We also cover
+ /// some limited cases of returning references (if return type is an optional
+ /// type), so cover some cases of operator* followed by operator*. We don't
+ /// cover mixing operator-> and operator*. Once we are confident in this
const
+ /// accessor caching, we shouldn't need the IgnoreSmartPointerDereference
+ /// option anymore.
bool IgnoreSmartPointerDereference = false;
};
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 31ae2b94f5b617..da5dda063344f9 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -338,6 +338,11 @@ auto isZeroParamConstMemberCall() {
callee(cxxMethodDecl(parameterCountIs(0), isConst())));
}
+auto isZeroParamConstMemberOperatorCall() {
+ return cxxOperatorCallExpr(
+ callee(cxxMethodDecl(parameterCountIs(0), isConst())));
+}
+
auto isNonConstMemberCall() {
return cxxMemberCallExpr(callee(cxxMethodDecl(unless(isConst()))));
}
@@ -572,9 +577,10 @@ void handleConstMemberCall(const CallExpr *CE,
return;
}
- // Cache if the const method returns a boolean type.
+ // Cache if the const method returns a boolean or pointer type.
// We may decide to cache other return types in the future.
- if (RecordLoc != nullptr && CE->getType()->isBooleanType()) {
+ if (RecordLoc != nullptr &&
+ (CE->getType()->isBooleanType() || CE->getType()->isPointerType())) {
Value *Val = State.Lattice.getOrCreateConstMethodReturnValue(*RecordLoc,
CE,
State.Env);
if (Val == nullptr)
@@ -597,6 +603,14 @@ void transferValue_ConstMemberCall(const CXXMemberCallExpr
*MCE,
MCE, dataflow::getImplicitObjectLocation(*MCE, State.Env), Result,
State);
}
+void transferValue_ConstMemberOperatorCall(
+ const CXXOperatorCallExpr *OCE, const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ auto *RecordLoc = cast_or_null<dataflow::RecordStorageLocation>(
+ State.Env.getStorageLocation(*OCE->getArg(0)));
+ handleConstMemberCall(OCE, RecordLoc, Result, State);
+}
+
void handleNonConstMemberCall(const CallExpr *CE,
dataflow::RecordStorageLocation *RecordLoc,
const MatchFinder::MatchResult &Result,
@@ -1020,6 +1034,8 @@ auto buildTransferMatchSwitch() {
// const accessor calls
.CaseOfCFGStmt<CXXMemberCallExpr>(isZeroParamConstMemberCall(),
transferValue_ConstMemberCall)
+ .CaseOfCFGStmt<CXXOperatorCallExpr>(isZeroParamConstMemberOperatorCall(),
+
transferValue_ConstMemberOperatorCall)
// non-const member calls that may modify the state of an object.
.CaseOfCFGStmt<CXXMemberCallExpr>(isNonConstMemberCall(),
transferValue_NonConstMemberCall)
diff --git
a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 5b64eaca0e10d3..5890466488c3c3 100644
---
a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++
b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -1282,28 +1282,35 @@ static raw_ostream &operator<<(raw_ostream &OS,
class UncheckedOptionalAccessTest
: public ::testing::TestWithParam<OptionalTypeIdentifier> {
protected:
- void ExpectDiagnosticsFor(std::string SourceCode) {
- ExpectDiagnosticsFor(SourceCode, ast_matchers::hasName("target"));
+ void ExpectDiagnosticsFor(std::string SourceCode,
+ bool IgnoreSmartPointerDereference = true) {
+ ExpectDiagnosticsFor(SourceCode, ast_matchers::hasName("target"),
+ IgnoreSmartPointerDereference);
}
- void ExpectDiagnosticsForLambda(std::string SourceCode) {
+ void ExpectDiagnosticsForLambda(std::string SourceCode,
+ bool IgnoreSmartPointerDereference = true) {
ExpectDiagnosticsFor(
- SourceCode, ast_matchers::hasDeclContext(
-
ast_matchers::cxxRecordDecl(ast_matchers::isLambda())));
+ SourceCode,
+ ast_matchers::hasDeclContext(
+ ast_matchers::cxxRecordDecl(ast_matchers::isLambda())),
+ IgnoreSmartPointerDereference);
}
template <typename FuncDeclMatcher>
- void ExpectDiagnosticsFor(std::string SourceCode,
- FuncDeclMatcher FuncMatcher) {
+ void ExpectDiagnosticsFor(std::string SourceCode, FuncDeclMatcher
FuncMatcher,
+ bool IgnoreSmartPointerDereference = true) {
// Run in C++17 and C++20 mode to cover differences in the AST between
modes
// (e.g. C++20 can contain `CXXRewrittenBinaryOperator`).
for (const char *CxxMode : {"-std=c++17", "-std=c++20"})
- ExpectDiagnosticsFor(SourceCode, FuncMatcher, CxxMode);
+ ExpectDiagnosticsFor(SourceCode, FuncMatcher, CxxMode,
+ IgnoreSmartPointerDereference);
}
template <typename FuncDeclMatcher>
void ExpectDiagnosticsFor(std::string SourceCode, FuncDeclMatcher
FuncMatcher,
- const char *CxxMode) {
+ const char *CxxMode,
+ bool IgnoreSmartPointerDereference) {
ReplaceAllOccurrences(SourceCode, "$ns", GetParam().NamespaceName);
ReplaceAllOccurrences(SourceCode, "$optional", GetParam().TypeName);
@@ -1328,8 +1335,7 @@ class UncheckedOptionalAccessTest
template <typename T>
T Make();
)");
- UncheckedOptionalAccessModelOptions Options{
- /*IgnoreSmartPointerDereference=*/true};
+ UncheckedOptionalAccessModelOptions Options{IgnoreSmartPointerDereference};
std::vector<SourceLocation> Diagnostics;
llvm::Error Error = checkDataflow<UncheckedOptionalAccessModel>(
AnalysisInputs<UncheckedOptionalAccessModel>(
@@ -3721,6 +3727,50 @@ TEST_P(UncheckedOptionalAccessTest,
ConstByValueAccessorWithModInBetween) {
)cc");
}
+TEST_P(UncheckedOptionalAccessTest, ConstPointerAccessor) {
+ ExpectDiagnosticsFor(R"cc(
+ #include "unchecked_optional_access_test.h"
+
+ struct B {
+ $ns::$optional<int> x;
+ };
+
+ struct MyUniquePtr {
+ B* operator->() const;
+ };
+
+ void target(MyUniquePtr a) {
+ if (a->x) {
+ *a->x;
+ }
+ }
+ )cc",
+ /*IgnoreSmartPointerDereference=*/false);
+}
+
+TEST_P(UncheckedOptionalAccessTest, ConstPointerAccessorWithModInBetween) {
+ ExpectDiagnosticsFor(R"cc(
+ #include "unchecked_optional_access_test.h"
+
+ struct B {
+ $ns::$optional<int> x;
+ };
+
+ struct MyUniquePtr {
+ B* operator->() const;
+ void reset(B*);
+ };
+
+ void target(MyUniquePtr a) {
+ if (a->x) {
+ a.reset(nullptr);
+ *a->x; // [[unsafe]]
+ }
+ }
+ )cc",
+ /*IgnoreSmartPointerDereference=*/false);
+}
+
TEST_P(UncheckedOptionalAccessTest, ConstBoolAccessor) {
ExpectDiagnosticsFor(R"cc(
#include "unchecked_optional_access_test.h"
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
