llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis Author: Jan Voung (jvoung) <details> <summary>Changes</summary> Part 2 (and final part) following https://github.com/llvm/llvm-project/pull/120102 Allows users to do things like: ``` if (o->x.has_value()) { ((*o).x).value(); } ``` where the `->` and `*` are operator overload calls. A user could instead extract the nested optional into a local variable once instead of doing two accessor calls back to back, but currently they are unsure why the code is flagged. --- Full diff: https://github.com/llvm/llvm-project/pull/120249.diff 7 Files Affected: - (modified) clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h (+41) - (modified) clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h (+7-8) - (modified) clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h (+107) - (modified) clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp (+52-6) - (modified) clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp (+21) - (modified) clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp (+30) - (modified) clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp (+48) ``````````diff diff --git a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h index 48c5287367739a..6b5dacf9f66d2d 100644 --- a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h +++ b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h @@ -13,7 +13,9 @@ #ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H #define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H +#include "clang/AST/Decl.h" #include "clang/AST/Expr.h" +#include "clang/AST/Type.h" #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h" #include "clang/Analysis/FlowSensitive/DataflowLattice.h" #include "clang/Analysis/FlowSensitive/StorageLocation.h" @@ -71,10 +73,28 @@ template <typename Base> class CachedConstAccessorsLattice : public Base { /// Requirements: /// /// - `CE` should return a location (GLValue or a record type). + /// + /// DEPRECATED: switch users to the below overload which takes Callee and Type + /// directly. StorageLocation *getOrCreateConstMethodReturnStorageLocation( const RecordStorageLocation &RecordLoc, const CallExpr *CE, Environment &Env, llvm::function_ref<void(StorageLocation &)> Initialize); + /// Creates or returns a previously created `StorageLocation` associated with + /// a const method call `obj.getFoo()` where `RecordLoc` is the + /// `RecordStorageLocation` of `obj`, `Callee` is the decl for `getFoo`, + /// and `Type` is the return type of `getFoo`. + /// + /// The callback `Initialize` runs on the storage location if newly created. + /// + /// Requirements: + /// + /// - `Type` should return a location (GLValue or a record type). + StorageLocation &getOrCreateConstMethodReturnStorageLocation( + const RecordStorageLocation &RecordLoc, const FunctionDecl *Callee, + QualType Type, Environment &Env, + llvm::function_ref<void(StorageLocation &)> Initialize); + void clearConstMethodReturnValues(const RecordStorageLocation &RecordLoc) { ConstMethodReturnValues.erase(&RecordLoc); } @@ -212,6 +232,27 @@ CachedConstAccessorsLattice<Base>::getOrCreateConstMethodReturnStorageLocation( return &Loc; } +template <typename Base> +StorageLocation & +CachedConstAccessorsLattice<Base>::getOrCreateConstMethodReturnStorageLocation( + const RecordStorageLocation &RecordLoc, const FunctionDecl *Callee, + QualType Type, Environment &Env, + llvm::function_ref<void(StorageLocation &)> Initialize) { + assert(Callee != nullptr); + assert(!Type.isNull()); + assert(Type->isReferenceType() || Type->isRecordType()); + auto &ObjMap = ConstMethodReturnStorageLocations[&RecordLoc]; + auto it = ObjMap.find(Callee); + if (it != ObjMap.end()) + return *it->second; + + StorageLocation &Loc = Env.createStorageLocation(Type.getNonReferenceType()); + Initialize(Loc); + + ObjMap.insert({Callee, &Loc}); + return Loc; +} + } // namespace dataflow } // namespace clang diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h index 713494178b97bd..fb11c2e230e328 100644 --- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h +++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h @@ -37,14 +37,13 @@ struct UncheckedOptionalAccessModelOptions { /// 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[]`. - /// FIXME: we currently cache and equate the result of const accessors - /// returning pointers, so cover the case of operator-> followed by - /// operator->, which covers the common case of smart pointers. We also cover - /// some limited cases of returning references (if return type is an optional - /// type), so cover some cases of operator* followed by operator*. We don't - /// cover mixing operator-> and operator*. Once we are confident in this const - /// accessor caching, we shouldn't need the IgnoreSmartPointerDereference - /// option anymore. + /// + /// FIXME: we now cache and equate the result of const accessors + /// that look like unique_ptr, have both `->` (returning a pointer type) and + /// `*` (returning a reference type). This includes mixing `->` and + /// `*` in a sequence of calls as long as the object is not modified. Once we + /// are confident in this const accessor caching, we shouldn't need the + /// IgnoreSmartPointerDereference option anymore. bool IgnoreSmartPointerDereference = false; }; diff --git a/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h index 3e4016518eaac9..2abd4caac01f87 100644 --- a/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h +++ b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h @@ -27,8 +27,13 @@ #include <cassert> #include "clang/AST/Decl.h" +#include "clang/AST/Expr.h" #include "clang/AST/Stmt.h" #include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Analysis/FlowSensitive/MatchSwitch.h" +#include "clang/Analysis/FlowSensitive/StorageLocation.h" +#include "clang/Analysis/FlowSensitive/Value.h" +#include "llvm/ADT/STLFunctionalExtras.h" namespace clang::dataflow { @@ -58,6 +63,108 @@ ast_matchers::StatementMatcher isSmartPointerLikeOperatorArrow(); ast_matchers::StatementMatcher isSmartPointerLikeValueMethodCall(); ast_matchers::StatementMatcher isSmartPointerLikeGetMethodCall(); +// Common transfer functions. + +/// Returns the "canonical" callee for smart pointer operators (`*` and `->`) +/// as a key for caching. +/// +/// We choose `*` as the canonical one, since it needs a +/// StorageLocation anyway. +/// +/// Note: there may be multiple `operator*` (one const, one non-const). +/// We pick the const one, which the above provided matchers require to exist. +const FunctionDecl * +getCanonicalSmartPointerLikeOperatorCallee(const CallExpr *CE); + +/// A transfer function for `operator*` (and `value`) calls +/// that can be cached. +/// +/// Requirements: +/// +/// - LatticeT should use the `CachedConstAccessorsLattice` mixin. +template <typename LatticeT> +void transferSmartPointerLikeCachedDeref( + const CallExpr *DerefExpr, RecordStorageLocation *SmartPointerLoc, + TransferState<LatticeT> &State, + llvm::function_ref<void(StorageLocation &)> InitializeLoc); + +/// A transfer function for `operator->` (and `get`) calls +/// that can be cached. +/// +/// Requirements: +/// +/// - LatticeT should use the `CachedConstAccessorsLattice` mixin. +template <typename LatticeT> +void transferSmartPointerLikeCachedGet( + const CallExpr *GetExpr, RecordStorageLocation *SmartPointerLoc, + TransferState<LatticeT> &State, + llvm::function_ref<void(StorageLocation &)> InitializeLoc); + +template <typename LatticeT> +void transferSmartPointerLikeCachedDeref( + const CallExpr *DerefExpr, RecordStorageLocation *SmartPointerLoc, + TransferState<LatticeT> &State, + llvm::function_ref<void(StorageLocation &)> InitializeLoc) { + if (State.Env.getStorageLocation(*DerefExpr) != nullptr) + return; + if (SmartPointerLoc == nullptr) + return; + + const FunctionDecl *Callee = DerefExpr->getDirectCallee(); + if (Callee == nullptr) + return; + const FunctionDecl *CanonicalCallee = + getCanonicalSmartPointerLikeOperatorCallee(DerefExpr); + // This shouldn't happen, as we should at least find `Callee` itself. + assert(CanonicalCallee != nullptr); + if (CanonicalCallee != Callee) { + // When using the provided matchers, we should always get a reference to + // the same type. + assert(CanonicalCallee->getReturnType()->isReferenceType() && + Callee->getReturnType()->isReferenceType()); + assert(CanonicalCallee->getReturnType() + .getNonReferenceType() + ->getCanonicalTypeUnqualified() == + Callee->getReturnType() + .getNonReferenceType() + ->getCanonicalTypeUnqualified()); + } + + StorageLocation &LocForValue = + State.Lattice.getOrCreateConstMethodReturnStorageLocation( + *SmartPointerLoc, CanonicalCallee, CanonicalCallee->getReturnType(), + State.Env, InitializeLoc); + State.Env.setStorageLocation(*DerefExpr, LocForValue); +} + +template <typename LatticeT> +void transferSmartPointerLikeCachedGet( + const CallExpr *GetExpr, RecordStorageLocation *SmartPointerLoc, + TransferState<LatticeT> &State, + llvm::function_ref<void(StorageLocation &)> InitializeLoc) { + if (SmartPointerLoc == nullptr) + return; + + const FunctionDecl *CanonicalCallee = + getCanonicalSmartPointerLikeOperatorCallee(GetExpr); + + if (CanonicalCallee != nullptr) { + auto &LocForValue = + State.Lattice.getOrCreateConstMethodReturnStorageLocation( + *SmartPointerLoc, CanonicalCallee, CanonicalCallee->getReturnType(), + State.Env, InitializeLoc); + State.Env.setValue(*GetExpr, + State.Env.template create<PointerValue>(LocForValue)); + } else { + // Otherwise, just cache the pointer value as if it was a const accessor. + Value *Val = State.Lattice.getOrCreateConstMethodReturnValue( + *SmartPointerLoc, GetExpr, State.Env); + if (Val == nullptr) + return; + State.Env.setValue(*GetExpr, *Val); + } +} + } // namespace clang::dataflow #endif // LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_SMARTPOINTERACCESSORCACHING_H diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index da5dda063344f9..d287d25910c873 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -25,8 +25,10 @@ #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h" #include "clang/Analysis/FlowSensitive/Formula.h" #include "clang/Analysis/FlowSensitive/RecordOps.h" +#include "clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h" #include "clang/Analysis/FlowSensitive/StorageLocation.h" #include "clang/Analysis/FlowSensitive/Value.h" +#include "clang/Basic/OperatorKinds.h" #include "clang/Basic/SourceLocation.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Casting.h" @@ -555,24 +557,26 @@ void handleConstMemberCall(const CallExpr *CE, LatticeTransferState &State) { // If the const method returns an optional or reference to an optional. if (RecordLoc != nullptr && isSupportedOptionalType(CE->getType())) { - StorageLocation *Loc = + const FunctionDecl *DirectCallee = CE->getDirectCallee(); + if (DirectCallee == nullptr) + return; + StorageLocation &Loc = State.Lattice.getOrCreateConstMethodReturnStorageLocation( - *RecordLoc, CE, State.Env, [&](StorageLocation &Loc) { + *RecordLoc, DirectCallee, CE->getType(), State.Env, + [&](StorageLocation &Loc) { setHasValue(cast<RecordStorageLocation>(Loc), State.Env.makeAtomicBoolValue(), State.Env); }); - if (Loc == nullptr) - return; if (CE->isGLValue()) { // If the call to the const method returns a reference to an optional, // link the call expression to the cached StorageLocation. - State.Env.setStorageLocation(*CE, *Loc); + State.Env.setStorageLocation(*CE, Loc); } else { // If the call to the const method returns an optional by value, we // need to use CopyRecord to link the optional to the result object // of the call expression. auto &ResultLoc = State.Env.getResultObjectLocation(*CE); - copyRecord(*cast<RecordStorageLocation>(Loc), ResultLoc, State.Env); + copyRecord(cast<RecordStorageLocation>(Loc), ResultLoc, State.Env); } return; } @@ -1031,6 +1035,48 @@ auto buildTransferMatchSwitch() { transferOptionalAndValueCmp(Cmp, Cmp->getArg(1), State.Env); }) + // Smart-pointer-like operator* and operator-> calls that may look like + // const accessors (below) but need special handling to allow mixing + // the accessor calls. + .CaseOfCFGStmt<CXXOperatorCallExpr>( + isSmartPointerLikeOperatorStar(), + [](const CXXOperatorCallExpr *E, + const MatchFinder::MatchResult &Result, + LatticeTransferState &State) { + transferSmartPointerLikeCachedDeref( + E, + dyn_cast_or_null<RecordStorageLocation>( + getLocBehindPossiblePointer(*E->getArg(0), State.Env)), + State, [](StorageLocation &Loc) {}); + }) + .CaseOfCFGStmt<CXXOperatorCallExpr>( + isSmartPointerLikeOperatorArrow(), + [](const CXXOperatorCallExpr *E, + const MatchFinder::MatchResult &Result, + LatticeTransferState &State) { + transferSmartPointerLikeCachedGet( + E, + dyn_cast_or_null<RecordStorageLocation>( + getLocBehindPossiblePointer(*E->getArg(0), State.Env)), + State, [](StorageLocation &Loc) {}); + }) + .CaseOfCFGStmt<CXXMemberCallExpr>( + isSmartPointerLikeValueMethodCall(), + [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &Result, + LatticeTransferState &State) { + transferSmartPointerLikeCachedDeref( + E, getImplicitObjectLocation(*E, State.Env), State, + [](StorageLocation &Loc) {}); + }) + .CaseOfCFGStmt<CXXMemberCallExpr>( + isSmartPointerLikeGetMethodCall(), + [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &Result, + LatticeTransferState &State) { + transferSmartPointerLikeCachedGet( + E, getImplicitObjectLocation(*E, State.Env), State, + [](StorageLocation &Loc) {}); + }) + // const accessor calls .CaseOfCFGStmt<CXXMemberCallExpr>(isZeroParamConstMemberCall(), transferValue_ConstMemberCall) diff --git a/clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp b/clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp index a0c81aa933da8e..c58bd309545dbf 100644 --- a/clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp +++ b/clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp @@ -132,6 +132,7 @@ ast_matchers::StatementMatcher isSmartPointerLikeOperatorArrow() { callee(cxxMethodDecl(parameterCountIs(0), returns(pointerType()), ofClass(smartPointerClassWithGetOrValue())))); } + ast_matchers::StatementMatcher isSmartPointerLikeValueMethodCall() { return cxxMemberCallExpr(callee( cxxMethodDecl(parameterCountIs(0), returns(referenceType()), @@ -144,4 +145,24 @@ ast_matchers::StatementMatcher isSmartPointerLikeGetMethodCall() { ofClass(smartPointerClassWithGet())))); } +const FunctionDecl * +getCanonicalSmartPointerLikeOperatorCallee(const CallExpr *CE) { + const FunctionDecl *CanonicalCallee = nullptr; + const CXXMethodDecl *Callee = + cast_or_null<CXXMethodDecl>(CE->getDirectCallee()); + if (Callee == nullptr) + return nullptr; + const CXXRecordDecl *RD = Callee->getParent(); + if (RD == nullptr) + return nullptr; + for (const auto *MD : RD->methods()) { + if (MD->getOverloadedOperator() == OO_Star && MD->isConst() && + MD->getNumParams() == 0 && MD->getReturnType()->isReferenceType()) { + CanonicalCallee = MD; + break; + } + } + return CanonicalCallee; +} + } // namespace clang::dataflow diff --git a/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp b/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp index 6488833bd14cf2..9944d44832b831 100644 --- a/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp @@ -148,6 +148,36 @@ TEST_F(CachedConstAccessorsLatticeTest, SameLocBeforeClearOrDiffAfterClear) { EXPECT_NE(Loc3, Loc2); } +TEST_F(CachedConstAccessorsLatticeTest, + SameLocBeforeClearOrDiffAfterClearWithCalleeAndType) { + CommonTestInputs Inputs; + auto *CE = Inputs.CallRef; + RecordStorageLocation Loc(Inputs.SType, RecordStorageLocation::FieldToLoc(), + {}); + + LatticeT Lattice; + auto NopInit = [](StorageLocation &) {}; + const FunctionDecl *Callee = CE->getDirectCallee(); + ASSERT_NE(Callee, nullptr); + QualType Type = Callee->getReturnType(); + StorageLocation &Loc1 = Lattice.getOrCreateConstMethodReturnStorageLocation( + Loc, Callee, Type, Env, NopInit); + auto NotCalled = [](StorageLocation &) { + ASSERT_TRUE(false) << "Not reached"; + }; + StorageLocation &Loc2 = Lattice.getOrCreateConstMethodReturnStorageLocation( + Loc, Callee, Type, Env, NotCalled); + + EXPECT_EQ(&Loc1, &Loc2); + + Lattice.clearConstMethodReturnStorageLocations(Loc); + StorageLocation &Loc3 = Lattice.getOrCreateConstMethodReturnStorageLocation( + Loc, Callee, Type, Env, NopInit); + + EXPECT_NE(&Loc3, &Loc1); + EXPECT_NE(&Loc3, &Loc2); +} + TEST_F(CachedConstAccessorsLatticeTest, SameStructValBeforeClearOrDiffAfterClear) { TestAST AST(R"cpp( diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp index de16f6be8eedbc..19c3ff49eab27e 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -3771,6 +3771,54 @@ TEST_P(UncheckedOptionalAccessTest, ConstPointerAccessorWithModInBetween) { /*IgnoreSmartPointerDereference=*/false); } +TEST_P(UncheckedOptionalAccessTest, SmartPointerAccessorMixed) { + ExpectDiagnosticsFor(R"cc( + #include "unchecked_optional_access_test.h" + + struct A { + $ns::$optional<int> x; + }; + + namespace absl { + template<typename T> + class StatusOr { + public: + bool ok() const; + + const T& operator*() const&; + T& operator*() &; + + const T* operator->() const; + T* operator->(); + + const T& value() const; + T& value(); + }; + } + + void target(absl::StatusOr<A> &mut, const absl::StatusOr<A> &imm) { + if (!mut.ok() || !imm.ok()) + return; + + if (mut->x.has_value()) { + mut->x.value(); + ((*mut).x).value(); + (mut.value().x).value(); + + // check flagged after modifying + mut = imm; + mut->x.value(); // [[unsafe]] + } + if (imm->x.has_value()) { + imm->x.value(); + ((*imm).x).value(); + (imm.value().x).value(); + } + } + )cc", + /*IgnoreSmartPointerDereference=*/false); +} + TEST_P(UncheckedOptionalAccessTest, ConstBoolAccessor) { ExpectDiagnosticsFor(R"cc( #include "unchecked_optional_access_test.h" `````````` </details> https://github.com/llvm/llvm-project/pull/120249 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
