ymandel created this revision.
ymandel added a reviewer: xazax.hun.
Herald added subscribers: martong, rnkovacs.
Herald added a reviewer: NoQ.
Herald added a project: All.
ymandel requested review of this revision.
Herald added a project: clang.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D140020
Files:
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===================================================================
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -1307,8 +1307,8 @@
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, 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 @@
)");
}
+// 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 @@
};
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;
}
)");
}
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -56,7 +56,7 @@
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 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 @@
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 @@
// 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 @@
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) {
Index: clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
===================================================================
--- clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
+++ clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
@@ -30,11 +30,12 @@
// 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 @@
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();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
