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; } )"); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
