https://github.com/jvoung created https://github.com/llvm/llvm-project/pull/120102
This is part 1 of caching for smart pointer accessors, building on top of the CachedConstAccessorsLattice, which caches "normal" accessors. Smart pointer accessors are a bit different in that they may: - have aliases to access the same underlying data (but potentially returning slightly different types like `&` vs `*`). Within a "checked" sequence users may mix uses of the different aliases and the check should apply to any of the spellings. - may have non-const overloads in addition to the const version, where the non-const doesn't actually modify the container Part 2 will follow and add transfer functions utilities. It will also add a user UncheckedOptionalAccessModel. We'd seen false positives when nesting StatusOr<optional<T>> and optional<StatusOr<T>>, etc. which this can help address. >From f9b8cbaed4c01c2051cdcde105d6a9bca1684388 Mon Sep 17 00:00:00 2001 From: Jan Voung <jvo...@gmail.com> Date: Mon, 16 Dec 2024 16:06:43 +0000 Subject: [PATCH] [clang][dataflow] Add matchers for smart pointer accessors to be cached This is part 1 of caching for smart pointer accessors, building on top of the CachedConstAccessorsLattice, which caches "normal" accessors. Smart pointer accessors are a bit different in that they may: - have aliases to access the same underlying data (but potentially returning slightly different types like `&` vs `*`). Within a "checked" sequence users may mix uses of the different aliases and the check should apply to any of the spellings. - may have non-const overloads in addition to the const version, where the non-const doesn't actually modify the container Part 2 will follow and add transfer functions utilities. It will also add a user UncheckedOptionalAccessModel. We'd seen false positives when nesting StatusOr<optional<T>> and optional<StatusOr<T>>, etc. which this can help address. --- .../SmartPointerAccessorCaching.h | 63 ++++++ .../lib/Analysis/FlowSensitive/CMakeLists.txt | 1 + .../SmartPointerAccessorCaching.cpp | 134 ++++++++++++ .../Analysis/FlowSensitive/CMakeLists.txt | 1 + .../SmartPointerAccessorCachingTest.cpp | 194 ++++++++++++++++++ 5 files changed, 393 insertions(+) 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/SmartPointerAccessorCaching.h b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h new file mode 100644 index 00000000000000..1adb63af4e724b --- /dev/null +++ b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h @@ -0,0 +1,63 @@ +//===-- 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/Stmt.h" +#include "clang/ASTMatchers/ASTMatchers.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(); + +} // 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/SmartPointerAccessorCaching.cpp b/clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp new file mode 100644 index 00000000000000..967455322b57fe --- /dev/null +++ b/clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp @@ -0,0 +1,134 @@ +#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())))); +} + +} // 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/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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
