This revision was automatically updated to reflect the committed changes.
Closed by commit rG5d22d1f54836: [clang][dataflow] Improve optional
model's support for ignoring smart pointers. (authored by ymandel).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140020/new/
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
