[clang] 5d22d1f - [clang][dataflow] Improve optional model's support for ignoring smart pointers.
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Thu Dec 15 07:41:47 PST 2022
Author: Yitzhak Mandelbaum
Date: 2022-12-15T15:39:52Z
New Revision: 5d22d1f54836263ead1971ef8128f5128290dfa7
URL: https://github.com/llvm/llvm-project/commit/5d22d1f54836263ead1971ef8128f5128290dfa7
DIFF: https://github.com/llvm/llvm-project/commit/5d22d1f54836263ead1971ef8128f5128290dfa7.diff
LOG: [clang][dataflow] Improve optional model's support for ignoring smart pointers.
The optional model has an option to ignore optionals accessed through smart
pointer types (other than optional itself). This patch improves this feature in
two ways:
1. We extend support to optionals accessed directly through the smart pointer,
like `ptr->value()`. Previously, support was limited to cases that went through
an intermediate field.
2. We clean up the implementation by removing the option from the analysis,
leaving it only in the diagnostic phase (where it is relevant).
3. Adjusts a test which was misleading in what it was testing.
Differential Revision: https://reviews.llvm.org/D140020
Added:
Modified:
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
index b053a10327c3f..037858fe752e9 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
@@ -30,11 +30,12 @@ namespace dataflow {
// analysis are always enabled and additional constructs are enabled through the
// `Options`.
struct UncheckedOptionalAccessModelOptions {
- /// Ignore optionals reachable through overloaded `operator*` or `operator->`
- /// (other than those of the optional type itself). The analysis does not
- /// equate the results of such calls, so it 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[]`.
+ /// In generating diagnostics, ignore optionals reachable through overloaded
+ /// `operator*` or `operator->` (other than those of the optional type
+ /// itself). The analysis does not equate the results of such calls, so it
+ /// 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[]`.
bool IgnoreSmartPointerDereference = false;
};
@@ -44,8 +45,7 @@ struct UncheckedOptionalAccessModelOptions {
class UncheckedOptionalAccessModel
: public DataflowAnalysis<UncheckedOptionalAccessModel, NoopLattice> {
public:
- UncheckedOptionalAccessModel(
- ASTContext &Ctx, UncheckedOptionalAccessModelOptions Options = {});
+ UncheckedOptionalAccessModel(ASTContext &Ctx);
/// Returns a matcher for the optional classes covered by this model.
static ast_matchers::DeclarationMatcher optionalClassDecl();
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 44a6f46c0f447..da3ffce1d1d8e 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -56,7 +56,7 @@ auto hasOptionalType() { return hasType(optionalOrAliasType()); }
auto isOptionalMemberCallWithName(
llvm::StringRef MemberName,
- llvm::Optional<StatementMatcher> Ignorable = std::nullopt) {
+ const llvm::Optional<StatementMatcher> &Ignorable = std::nullopt) {
auto Exception = unless(Ignorable ? expr(anyOf(*Ignorable, cxxThisExpr()))
: cxxThisExpr());
return cxxMemberCallExpr(
@@ -66,7 +66,7 @@ auto isOptionalMemberCallWithName(
auto isOptionalOperatorCallWithName(
llvm::StringRef operator_name,
- llvm::Optional<StatementMatcher> Ignorable = std::nullopt) {
+ const llvm::Optional<StatementMatcher> &Ignorable = std::nullopt) {
return cxxOperatorCallExpr(
hasOverloadedOperatorName(operator_name),
callee(cxxMethodDecl(ofClass(optionalClass()))),
@@ -612,31 +612,31 @@ void transferOptionalAndValueCmp(const clang::CXXOperatorCallExpr *CmpExpr,
llvm::Optional<StatementMatcher>
ignorableOptional(const UncheckedOptionalAccessModelOptions &Options) {
- if (Options.IgnoreSmartPointerDereference)
- return memberExpr(hasObjectExpression(ignoringParenImpCasts(
- cxxOperatorCallExpr(anyOf(hasOverloadedOperatorName("->"),
- hasOverloadedOperatorName("*")),
- unless(hasArgument(0, expr(hasOptionalType())))))));
+ if (Options.IgnoreSmartPointerDereference) {
+ auto SmartPtrUse = expr(ignoringParenImpCasts(cxxOperatorCallExpr(
+ anyOf(hasOverloadedOperatorName("->"), hasOverloadedOperatorName("*")),
+ unless(hasArgument(0, expr(hasOptionalType()))))));
+ return expr(
+ anyOf(SmartPtrUse, memberExpr(hasObjectExpression(SmartPtrUse))));
+ }
return std::nullopt;
}
StatementMatcher
-valueCall(llvm::Optional<StatementMatcher> &IgnorableOptional) {
+valueCall(const llvm::Optional<StatementMatcher> &IgnorableOptional) {
return isOptionalMemberCallWithName("value", IgnorableOptional);
}
StatementMatcher
-valueOperatorCall(llvm::Optional<StatementMatcher> &IgnorableOptional) {
+valueOperatorCall(const llvm::Optional<StatementMatcher> &IgnorableOptional) {
return expr(anyOf(isOptionalOperatorCallWithName("*", IgnorableOptional),
isOptionalOperatorCallWithName("->", IgnorableOptional)));
}
-auto buildTransferMatchSwitch(
- const UncheckedOptionalAccessModelOptions &Options) {
+auto buildTransferMatchSwitch() {
// FIXME: Evaluate the efficiency of matchers. If using matchers results in a
// lot of duplicated work (e.g. string comparisons), consider providing APIs
// that avoid it through memoization.
- auto IgnorableOptional = ignorableOptional(Options);
return CFGMatchSwitchBuilder<LatticeTransferState>()
// Attach a symbolic "has_value" state to optional values that we see for
// the first time.
@@ -685,14 +685,14 @@ auto buildTransferMatchSwitch(
// optional::value
.CaseOfCFGStmt<CXXMemberCallExpr>(
- valueCall(IgnorableOptional),
+ valueCall(std::nullopt),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
transferUnwrapCall(E, E->getImplicitObjectArgument(), State);
})
// optional::operator*, optional::operator->
- .CaseOfCFGStmt<CallExpr>(valueOperatorCall(IgnorableOptional),
+ .CaseOfCFGStmt<CallExpr>(valueOperatorCall(std::nullopt),
[](const CallExpr *E,
const MatchFinder::MatchResult &,
LatticeTransferState &State) {
@@ -817,10 +817,9 @@ UncheckedOptionalAccessModel::optionalClassDecl() {
return optionalClass();
}
-UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(
- ASTContext &Ctx, UncheckedOptionalAccessModelOptions Options)
+UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx)
: DataflowAnalysis<UncheckedOptionalAccessModel, NoopLattice>(Ctx),
- TransferMatchSwitch(buildTransferMatchSwitch(Options)) {}
+ TransferMatchSwitch(buildTransferMatchSwitch()) {}
void UncheckedOptionalAccessModel::transfer(const CFGElement *Elt,
NoopLattice &L, Environment &Env) {
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 17ec08bffef25..7c95119ef68d6 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -1307,8 +1307,8 @@ class UncheckedOptionalAccessTest
llvm::Error Error = checkDataflow<UncheckedOptionalAccessModel>(
AnalysisInputs<UncheckedOptionalAccessModel>(
SourceCode, std::move(FuncMatcher),
- [Options](ASTContext &Ctx, Environment &) {
- return UncheckedOptionalAccessModel(Ctx, Options);
+ [](ASTContext &Ctx, Environment &) {
+ return UncheckedOptionalAccessModel(Ctx);
})
.withPostVisitCFG(
[&Diagnostics,
@@ -2107,9 +2107,30 @@ TEST_P(UncheckedOptionalAccessTest, StdSwap) {
)");
}
+TEST_P(UncheckedOptionalAccessTest, UniquePtrToOptional) {
+ // We suppress diagnostics for optionals in smart pointers (other than
+ // `optional` itself).
+ ExpectDiagnosticsFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ template <typename T>
+ struct smart_ptr {
+ T& operator*() &;
+ T* operator->();
+ };
+
+ void target() {
+ smart_ptr<$ns::$optional<bool>> foo;
+ foo->value();
+ (*foo).value();
+ }
+ )");
+}
+
TEST_P(UncheckedOptionalAccessTest, UniquePtrToStructWithOptionalField) {
- // We suppress diagnostics for values reachable from smart pointers (other
- // than `optional` itself).
+ // We suppress diagnostics for optional fields reachable from smart pointers
+ // (other than `optional` itself) through (exactly) one member access.
ExpectDiagnosticsFor(
R"(
#include "unchecked_optional_access_test.h"
@@ -2601,6 +2622,10 @@ TEST_P(UncheckedOptionalAccessTest, OptionalValueInitialization) {
)");
}
+// This test is aimed at the core model, not the diagnostic. It is a regression
+// test against a crash when using non-trivial smart pointers, like
+// `std::unique_ptr`. As such, it doesn't test the access itself, which would be
+// ignored regardless because of `IgnoreSmartPointerDereference = true`, above.
TEST_P(UncheckedOptionalAccessTest, AssignThroughLvalueReferencePtr) {
ExpectDiagnosticsFor(
R"(
@@ -2612,9 +2637,9 @@ TEST_P(UncheckedOptionalAccessTest, AssignThroughLvalueReferencePtr) {
};
void target() {
- smart_ptr<$ns::$optional<float>> x;
- *x = $ns::nullopt;
- (*x).value(); // [[unsafe]]
+ smart_ptr<$ns::$optional<int>> x;
+ // Verify that this assignment does not crash.
+ *x = 3;
}
)");
}
More information about the cfe-commits
mailing list