Author: Yitzhak Mandelbaum Date: 2022-03-25T16:44:34Z New Revision: a184a0d8aae6efc1d7f19a900155b8694178d617
URL: https://github.com/llvm/llvm-project/commit/a184a0d8aae6efc1d7f19a900155b8694178d617 DIFF: https://github.com/llvm/llvm-project/commit/a184a0d8aae6efc1d7f19a900155b8694178d617.diff LOG: [clang][dataflow] Add support for disabling warnings on smart pointers. This patch provides the user with the ability to disable all checked of accesses to optionals that are the pointees of smart pointers. Since smart pointers are not modeled (yet), the system cannot distinguish safe from unsafe accesses to optionals through smart pointers. This results in false positives whenever optionals are used through smart pointers. The patch gives the user the choice of ignoring all positivess in these cases. Differential Revision: https://reviews.llvm.org/D122143 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 05a9165e59dde..235121b2e5759 100644 --- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h +++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h @@ -24,6 +24,18 @@ namespace clang { namespace dataflow { +// FIXME: Explore using an allowlist-approach, where constructs supported by the +// 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[]`. + bool IgnoreSmartPointerDereference = false; +}; + /// Dataflow analysis that discovers unsafe accesses of optional values and /// adds the respective source locations to the lattice. /// @@ -34,7 +46,8 @@ class UncheckedOptionalAccessModel : public DataflowAnalysis<UncheckedOptionalAccessModel, SourceLocationsLattice> { public: - explicit UncheckedOptionalAccessModel(ASTContext &AstContext); + UncheckedOptionalAccessModel( + ASTContext &AstContext, UncheckedOptionalAccessModelOptions Options = {}); static SourceLocationsLattice initialElement() { return SourceLocationsLattice(); diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index bf325b04967c2..b775698dafb5d 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -45,15 +45,23 @@ auto optionalClass() { auto hasOptionalType() { return hasType(optionalClass()); } -auto isOptionalMemberCallWithName(llvm::StringRef MemberName) { +auto isOptionalMemberCallWithName( + llvm::StringRef MemberName, + llvm::Optional<StatementMatcher> Ignorable = llvm::None) { + auto Exception = unless(Ignorable ? expr(anyOf(*Ignorable, cxxThisExpr())) + : cxxThisExpr()); return cxxMemberCallExpr( - on(expr(unless(cxxThisExpr()))), + on(expr(Exception)), callee(cxxMethodDecl(hasName(MemberName), ofClass(optionalClass())))); } -auto isOptionalOperatorCallWithName(llvm::StringRef OperatorName) { - return cxxOperatorCallExpr(hasOverloadedOperatorName(OperatorName), - callee(cxxMethodDecl(ofClass(optionalClass())))); +auto isOptionalOperatorCallWithName( + llvm::StringRef operator_name, + llvm::Optional<StatementMatcher> Ignorable = llvm::None) { + return cxxOperatorCallExpr( + hasOverloadedOperatorName(operator_name), + callee(cxxMethodDecl(ofClass(optionalClass()))), + Ignorable ? callExpr(unless(hasArgument(0, *Ignorable))) : callExpr()); } auto isMakeOptionalCall() { @@ -333,10 +341,22 @@ void transferStdSwapCall(const CallExpr *E, const MatchFinder::MatchResult &, transferSwap(*OptionalLoc1, *OptionalLoc2, State); } -auto buildTransferMatchSwitch() { +llvm::Optional<StatementMatcher> +ignorableOptional(const UncheckedOptionalAccessModelOptions &Options) { + if (Options.IgnoreSmartPointerDereference) + return memberExpr(hasObjectExpression(ignoringParenImpCasts( + cxxOperatorCallExpr(anyOf(hasOverloadedOperatorName("->"), + hasOverloadedOperatorName("*")), + unless(hasArgument(0, expr(hasOptionalType()))))))); + return llvm::None; +} + +auto buildTransferMatchSwitch( + const UncheckedOptionalAccessModelOptions &Options) { // 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 MatchSwitchBuilder<LatticeTransferState>() // Attach a symbolic "has_value" state to optional values that we see for // the first time. @@ -371,19 +391,20 @@ auto buildTransferMatchSwitch() { // optional::value .CaseOf<CXXMemberCallExpr>( - isOptionalMemberCallWithName("value"), + isOptionalMemberCallWithName("value", IgnorableOptional), [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { transferUnwrapCall(E, E->getImplicitObjectArgument(), State); }) // optional::operator*, optional::operator-> - .CaseOf<CallExpr>(expr(anyOf(isOptionalOperatorCallWithName("*"), - isOptionalOperatorCallWithName("->"))), - [](const CallExpr *E, const MatchFinder::MatchResult &, - LatticeTransferState &State) { - transferUnwrapCall(E, E->getArg(0), State); - }) + .CaseOf<CallExpr>( + expr(anyOf(isOptionalOperatorCallWithName("*", IgnorableOptional), + isOptionalOperatorCallWithName("->", IgnorableOptional))), + [](const CallExpr *E, const MatchFinder::MatchResult &, + LatticeTransferState &State) { + transferUnwrapCall(E, E->getArg(0), State); + }) // optional::has_value .CaseOf<CXXMemberCallExpr>(isOptionalMemberCallWithName("has_value"), @@ -423,10 +444,11 @@ auto buildTransferMatchSwitch() { } // namespace -UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx) +UncheckedOptionalAccessModel::UncheckedOptionalAccessModel( + ASTContext &Ctx, UncheckedOptionalAccessModelOptions Options) : DataflowAnalysis<UncheckedOptionalAccessModel, SourceLocationsLattice>( Ctx), - TransferMatchSwitch(buildTransferMatchSwitch()) {} + TransferMatchSwitch(buildTransferMatchSwitch(Options)) {} void UncheckedOptionalAccessModel::transfer(const Stmt *S, SourceLocationsLattice &L, diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp index 76340aad5fd4d..6ef4b2a9c3336 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -1219,7 +1219,9 @@ class UncheckedOptionalAccessTest llvm::Error Error = checkDataflow<UncheckedOptionalAccessModel>( SourceCode, FuncMatcher, [](ASTContext &Ctx, Environment &) { - return UncheckedOptionalAccessModel(Ctx); + return UncheckedOptionalAccessModel( + Ctx, UncheckedOptionalAccessModelOptions{ + /*IgnoreSmartPointerDereference=*/true}); }, [&MatchesLatticeChecks]( llvm::ArrayRef<std::pair< @@ -2004,6 +2006,34 @@ TEST_P(UncheckedOptionalAccessTest, StdSwap) { Pair("check-4", "unsafe: input.cc:13:7"))); } +TEST_P(UncheckedOptionalAccessTest, UniquePtrToStructWithOptionalField) { + // We suppress diagnostics for values reachable from smart pointers (other + // than `optional` itself). + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + template <typename T> + struct smart_ptr { + T& operator*() &; + T* operator->(); + }; + + struct Foo { + $ns::$optional<int> opt; + }; + + void target() { + smart_ptr<Foo> foo; + *foo->opt; + /*[[check-1]]*/ + *(*foo).opt; + /*[[check-2]]*/ + } + )", + UnorderedElementsAre(Pair("check-1", "safe"), Pair("check-2", "safe"))); +} + // FIXME: Add support for: // - constructors (copy, move) // - assignment operators (default, copy, move) _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
