https://github.com/jvoung created https://github.com/llvm/llvm-project/pull/120249
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. >From c526263a7accc434dbf6e93c2995ceb2f95873b8 Mon Sep 17 00:00:00 2001 From: Jan Voung <jvo...@gmail.com> Date: Tue, 17 Dec 2024 15:38:19 +0000 Subject: [PATCH] [clang][dataflow] Use smart pointer caching in unchecked optional accessor 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. --- .../CachedConstAccessorsLattice.h | 41 ++++ .../SmartPointerAccessorCaching.h | 168 +++++++++++++++ .../lib/Analysis/FlowSensitive/CMakeLists.txt | 1 + .../Models/UncheckedOptionalAccessModel.cpp | 58 +++++- .../SmartPointerAccessorCaching.cpp | 153 ++++++++++++++ .../Analysis/FlowSensitive/CMakeLists.txt | 1 + .../CachedConstAccessorsLatticeTest.cpp | 32 +++ .../SmartPointerAccessorCachingTest.cpp | 194 ++++++++++++++++++ .../UncheckedOptionalAccessModelTest.cpp | 50 +++++ 9 files changed, 692 insertions(+), 6 deletions(-) create mode 100644 clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h create mode 100644 clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp create mode 100644 clang/unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp 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/SmartPointerAccessorCaching.h b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h new file mode 100644 index 00000000000000..e89e82c893d8cb --- /dev/null +++ b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h @@ -0,0 +1,168 @@ +//===-- SmartPointerAccessorCaching.h ---------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +// +// This file defines utilities to help cache accessors for smart pointer +// like objects. +// +// These should be combined with CachedConstAccessorsLattice. +// Beyond basic const accessors, smart pointers may have the following two +// additional issues: +// +// 1) There may be multiple accessors for the same underlying object, e.g. +// `operator->`, `operator*`, and `get`. Users may use a mixture of these +// accessors, so the cache should unify them. +// +// 2) There may be non-const overloads of accessors. They are still safe to +// cache, as they don't modify the container object. +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_SMARTPOINTERACCESSORCACHING_H +#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_SMARTPOINTERACCESSORCACHING_H + +#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 { + +/// Matchers: +/// For now, these match on any class with an `operator*` or `operator->` +/// where the return types have a similar shape as std::unique_ptr +/// and std::optional. +/// +/// - `*` returns a reference to a type `T` +/// - `->` returns a pointer to `T` +/// - `get` returns a pointer to `T` +/// - `value` returns a reference `T` +/// +/// (1) The `T` should all match across the accessors (ignoring qualifiers). +/// +/// (2) The specific accessor used in a call isn't required to be const, +/// but the class must have a const overload of each accessor. +/// +/// For now, we don't have customization to ignore certain classes. +/// For example, if writing a ClangTidy check for `std::optional`, these +/// would also match `std::optional`. In order to have special handling +/// for `std::optional`, we assume the (Matcher, TransferFunction) case +/// with custom handling is ordered early so that these generic cases +/// do not trigger. +ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeOperatorStar(); +ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeOperatorArrow(); +ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeValueMethodCall(); +ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeGetMethodCall(); + +// Common transfer functions. + +/// Returns the "canonical" callee for smart pointer operators (`*` and `->`) +/// as a key for caching. +/// +/// We choose `operator *` 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. +/// +/// Requirements: +/// +/// - LatticeT should use the `CachedConstAccessorsLattice` mixin. +template <typename LatticeT> +void transferSmartPointerLikeDeref( + const CallExpr *DerefExpr, RecordStorageLocation *SmartPointerLoc, + TransferState<LatticeT> &State, + llvm::function_ref<void(StorageLocation &)> InitializeLoc); + +/// A transfer function for `operator->` (and `get`) calls. +/// +/// Requirements: +/// +/// - LatticeT should use the `CachedConstAccessorsLattice` mixin. +template <typename LatticeT> +void transferSmartPointerLikeGet( + const CallExpr *GetExpr, RecordStorageLocation *SmartPointerLoc, + TransferState<LatticeT> &State, + llvm::function_ref<void(StorageLocation &)> InitializeLoc); + +template <typename LatticeT> +void transferSmartPointerLikeDeref( + 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 really 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 transferSmartPointerLikeGet( + 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/CMakeLists.txt b/clang/lib/Analysis/FlowSensitive/CMakeLists.txt index 05cdaa7e27823d..0c30df8b4b194f 100644 --- a/clang/lib/Analysis/FlowSensitive/CMakeLists.txt +++ b/clang/lib/Analysis/FlowSensitive/CMakeLists.txt @@ -10,6 +10,7 @@ add_clang_library(clangAnalysisFlowSensitive Logger.cpp RecordOps.cpp SimplifyConstraints.cpp + SmartPointerAccessorCaching.cpp Transfer.cpp TypeErasedDataflowAnalysis.cpp Value.cpp diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index da5dda063344f9..ce54b450a2121c 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) { + transferSmartPointerLikeDeref( + 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) { + transferSmartPointerLikeGet( + 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) { + transferSmartPointerLikeDeref( + E, getImplicitObjectLocation(*E, State.Env), State, + [](StorageLocation &Loc) {}); + }) + .CaseOfCFGStmt<CXXMemberCallExpr>( + isSmartPointerLikeGetMethodCall(), + [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &Result, + LatticeTransferState &State) { + transferSmartPointerLikeGet( + 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 new file mode 100644 index 00000000000000..12d0bdb82cb538 --- /dev/null +++ b/clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp @@ -0,0 +1,153 @@ +#include "clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h" + +#include "clang/AST/CanonicalType.h" +#include "clang/AST/DeclCXX.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Basic/OperatorKinds.h" + +namespace clang::dataflow { + +namespace { + +using ast_matchers::callee; +using ast_matchers::cxxMemberCallExpr; +using ast_matchers::cxxMethodDecl; +using ast_matchers::cxxOperatorCallExpr; +using ast_matchers::hasName; +using ast_matchers::hasOverloadedOperatorName; +using ast_matchers::ofClass; +using ast_matchers::parameterCountIs; +using ast_matchers::pointerType; +using ast_matchers::referenceType; +using ast_matchers::returns; + +bool hasSmartPointerClassShape(const CXXRecordDecl &RD, bool &HasGet, + bool &HasValue) { + // We may want to cache this search, but in current profiles it hasn't shown + // up as a hot spot (possibly because there aren't many hits, relatively). + bool HasArrow = false; + bool HasStar = false; + CanQualType StarReturnType, ArrowReturnType, GetReturnType, ValueReturnType; + for (const auto *MD : RD.methods()) { + // We only consider methods that are const and have zero parameters. + // It may be that there is a non-const overload for the method, but + // there should at least be a const overload as well. + if (!MD->isConst() || MD->getNumParams() != 0) + continue; + if (MD->getOverloadedOperator() == OO_Star && + MD->getReturnType()->isReferenceType()) { + HasStar = true; + StarReturnType = MD->getReturnType() + .getNonReferenceType() + ->getCanonicalTypeUnqualified(); + } else if (MD->getOverloadedOperator() == OO_Arrow && + MD->getReturnType()->isPointerType()) { + HasArrow = true; + ArrowReturnType = + MD->getReturnType()->getPointeeType()->getCanonicalTypeUnqualified(); + } else { + IdentifierInfo *II = MD->getIdentifier(); + if (II == nullptr) + continue; + if (II->isStr("get") && MD->getReturnType()->isPointerType()) { + HasGet = true; + GetReturnType = MD->getReturnType() + ->getPointeeType() + ->getCanonicalTypeUnqualified(); + } else if (II->isStr("value") && MD->getReturnType()->isReferenceType()) { + HasValue = true; + ValueReturnType = MD->getReturnType() + .getNonReferenceType() + ->getCanonicalTypeUnqualified(); + } + } + } + + if (!HasStar || !HasArrow || StarReturnType != ArrowReturnType) + return false; + HasGet = HasGet && (GetReturnType == StarReturnType); + HasValue = HasValue && (ValueReturnType == StarReturnType); + return true; +} + +} // namespace +} // namespace clang::dataflow + +// AST_MATCHER macros create an "internal" namespace, so we put it in +// its own anonymous namespace instead of in clang::dataflow. +namespace { + +AST_MATCHER(clang::CXXRecordDecl, smartPointerClassWithGet) { + bool HasGet = false; + bool HasValue = false; + bool HasStarAndArrow = + clang::dataflow::hasSmartPointerClassShape(Node, HasGet, HasValue); + return HasStarAndArrow && HasGet; +} + +AST_MATCHER(clang::CXXRecordDecl, smartPointerClassWithValue) { + bool HasGet = false; + bool HasValue = false; + bool HasStarAndArrow = + clang::dataflow::hasSmartPointerClassShape(Node, HasGet, HasValue); + return HasStarAndArrow && HasValue; +} + +AST_MATCHER(clang::CXXRecordDecl, smartPointerClassWithGetOrValue) { + bool HasGet = false; + bool HasValue = false; + bool HasStarAndArrow = + clang::dataflow::hasSmartPointerClassShape(Node, HasGet, HasValue); + return HasStarAndArrow && (HasGet || HasValue); +} + +} // namespace + +namespace clang::dataflow { + +ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeOperatorStar() { + return cxxOperatorCallExpr( + hasOverloadedOperatorName("*"), + callee(cxxMethodDecl(parameterCountIs(0), returns(referenceType()), + ofClass(smartPointerClassWithGetOrValue())))); +} + +ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeOperatorArrow() { + return cxxOperatorCallExpr( + hasOverloadedOperatorName("->"), + callee(cxxMethodDecl(parameterCountIs(0), returns(pointerType()), + ofClass(smartPointerClassWithGetOrValue())))); +} +ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeValueMethodCall() { + return cxxMemberCallExpr(callee( + cxxMethodDecl(parameterCountIs(0), returns(referenceType()), + hasName("value"), ofClass(smartPointerClassWithValue())))); +} + +ast_matchers::internal::Matcher<Stmt> isSmartPointerLikeGetMethodCall() { + return cxxMemberCallExpr(callee( + cxxMethodDecl(parameterCountIs(0), returns(pointerType()), hasName("get"), + 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/CMakeLists.txt b/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt index 4e1819bfa166a8..6c01ae8fc2e541 100644 --- a/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt +++ b/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt @@ -21,6 +21,7 @@ add_clang_unittest(ClangAnalysisFlowSensitiveTests SignAnalysisTest.cpp SimplifyConstraintsTest.cpp SingleVarConstantPropagationTest.cpp + SmartPointerAccessorCachingTest.cpp TestingSupport.cpp TestingSupportTest.cpp TransferBranchTest.cpp diff --git a/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp b/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp index 6488833bd14cf2..62704b0e94b39a 100644 --- a/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp @@ -148,6 +148,38 @@ 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(); + Type.dump(); + CE->dump(); + 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/SmartPointerAccessorCachingTest.cpp b/clang/unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp new file mode 100644 index 00000000000000..a69d606cdb85ea --- /dev/null +++ b/clang/unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp @@ -0,0 +1,194 @@ +//===- unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp ==// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h" + +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Testing/TestAST.h" +#include "llvm/ADT/StringRef.h" +#include "gtest/gtest.h" + +namespace clang::dataflow { +namespace { + +using clang::ast_matchers::match; + +template <typename MatcherT> +bool matches(llvm::StringRef Decls, llvm::StringRef TestInput, + MatcherT Matcher) { + TestAST InputAST(Decls.str() + TestInput.str()); + return !match(Matcher, InputAST.context()).empty(); +} + +TEST(SmartPointerAccessorCachingTest, MatchesClassWithStarArrowGet) { + llvm::StringRef Decls(R"cc( + namespace std { + template <class T> + struct unique_ptr { + T* operator->() const; + T& operator*() const; + T* get() const; + }; + } // namespace std + + template <class T> + using UniquePtrAlias = std::unique_ptr<T>; + + struct S { int i; }; + )cc"); + + EXPECT_TRUE(matches(Decls, + "int target(std::unique_ptr<S> P) { return (*P).i; }", + isSmartPointerLikeOperatorStar())); + EXPECT_TRUE(matches(Decls, + "int target(std::unique_ptr<S> P) { return P->i; }", + isSmartPointerLikeOperatorArrow())); + EXPECT_TRUE(matches(Decls, + "int target(std::unique_ptr<S> P) { return P.get()->i; }", + isSmartPointerLikeGetMethodCall())); + + EXPECT_TRUE(matches(Decls, "int target(UniquePtrAlias<S> P) { return P->i; }", + isSmartPointerLikeOperatorArrow())); +} + +TEST(SmartPointerAccessorCachingTest, NoMatchIfUnexpectedReturnTypes) { + llvm::StringRef Decls(R"cc( + namespace std { + // unique_ptr isn't really like this, but we aren't matching by name + template <class T, class U> + struct unique_ptr { + U* operator->() const; + T& operator*() const; + T* get() const; + }; + } // namespace std + + struct S { int i; }; + struct T { int j; }; + )cc"); + + EXPECT_FALSE(matches(Decls, + "int target(std::unique_ptr<S, T> P) { return (*P).i; }", + isSmartPointerLikeOperatorStar())); + EXPECT_FALSE(matches(Decls, + "int target(std::unique_ptr<S, T> P) { return P->j; }", + isSmartPointerLikeOperatorArrow())); + // The class matching arguably accidentally matches, just because the + // instantiation is with S, S. Hopefully doesn't happen too much in real code + // with such operator* and operator-> overloads. + EXPECT_TRUE(matches(Decls, + "int target(std::unique_ptr<S, S> P) { return P->i; }", + isSmartPointerLikeOperatorArrow())); +} + +TEST(SmartPointerAccessorCachingTest, NoMatchIfBinaryStar) { + llvm::StringRef Decls(R"cc( + namespace std { + template <class T> + struct unique_ptr { + T* operator->() const; + T& operator*(int x) const; + T* get() const; + }; + } // namespace std + + struct S { int i; }; + )cc"); + + EXPECT_FALSE( + matches(Decls, "int target(std::unique_ptr<S> P) { return (P * 10).i; }", + isSmartPointerLikeOperatorStar())); +} + +TEST(SmartPointerAccessorCachingTest, NoMatchIfNoConstOverloads) { + llvm::StringRef Decls(R"cc( + namespace std { + template <class T> + struct unique_ptr { + T* operator->(); + T& operator*(); + T* get(); + }; + } // namespace std + + struct S { int i; }; + )cc"); + + EXPECT_FALSE(matches(Decls, + "int target(std::unique_ptr<S> P) { return (*P).i; }", + isSmartPointerLikeOperatorStar())); + EXPECT_FALSE(matches(Decls, + "int target(std::unique_ptr<S> P) { return P->i; }", + isSmartPointerLikeOperatorArrow())); + EXPECT_FALSE( + matches(Decls, "int target(std::unique_ptr<S> P) { return P.get()->i; }", + isSmartPointerLikeGetMethodCall())); +} + +TEST(SmartPointerAccessorCachingTest, NoMatchIfNoStarMethod) { + llvm::StringRef Decls(R"cc( + namespace std { + template <class T> + struct unique_ptr { + T* operator->(); + T* get(); + }; + } // namespace std + + struct S { int i; }; + )cc"); + + EXPECT_FALSE(matches(Decls, + "int target(std::unique_ptr<S> P) { return P->i; }", + isSmartPointerLikeOperatorArrow())); + EXPECT_FALSE(matches(Decls, + "int target(std::unique_ptr<S> P) { return P->i; }", + isSmartPointerLikeGetMethodCall())); +} + +TEST(SmartPointerAccessorCachingTest, MatchesWithValueAndNonConstOverloads) { + llvm::StringRef Decls(R"cc( + namespace std { + template <class T> + struct optional { + const T* operator->() const; + T* operator->(); + const T& operator*() const; + T& operator*(); + const T& value() const; + T& value(); + }; + } // namespace std + + struct S { int i; }; + )cc"); + + EXPECT_TRUE(matches( + Decls, "int target(std::optional<S> &NonConst) { return (*NonConst).i; }", + isSmartPointerLikeOperatorStar())); + EXPECT_TRUE(matches( + Decls, "int target(const std::optional<S> &Const) { return (*Const).i; }", + isSmartPointerLikeOperatorStar())); + EXPECT_TRUE(matches( + Decls, "int target(std::optional<S> &NonConst) { return NonConst->i; }", + isSmartPointerLikeOperatorArrow())); + EXPECT_TRUE(matches( + Decls, "int target(const std::optional<S> &Const) { return Const->i; }", + isSmartPointerLikeOperatorArrow())); + EXPECT_TRUE(matches( + Decls, + "int target(std::optional<S> &NonConst) { return NonConst.value().i; }", + isSmartPointerLikeValueMethodCall())); + EXPECT_TRUE(matches( + Decls, + "int target(const std::optional<S> &Const) { return Const.value().i; }", + isSmartPointerLikeValueMethodCall())); +} + +} // namespace +} // namespace clang::dataflow \ No newline at end of file diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp index de16f6be8eedbc..6f590a54f6da9b 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -3771,6 +3771,56 @@ 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* 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" _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
