llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: Harald-R <details> <summary>Changes</summary> As proposed in [this thread](https://discourse.llvm.org/t/clang-tidy-checks-for-common-mlir-anti-patterns/91179), a clang-tidy check could be introduced which detects cases where an operation is erased, but it still has uses after. The check targets the following calls for erasing an operation: - `mlir::Operation::erase` - `mlir::Operation::destroy` - `mlir::RewriterBase::eraseOp` - `mlir::RewriterBase::replaceOp` - through derived operations of `mlir::OpState` that are wrappers around `mlir::Operation` (e.g. `mlir::func::FuncOp` and all others that inherit from `mlir::Op`) Cases where an operation's declaration variable is reinitialized are also taken into account. Additionally, cases where an operation is erased in a loop's iteration are covered as well. The implementation of this check is based on the existing [bugprone-use-after-move](https://clang.llvm.org/extra/clang-tidy/checks/bugprone/use-after-move.html). The findings and fixes for this check over the LLVM project can be found in https://github.com/llvm/llvm-project/pull/210733. --- Patch is 37.13 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/210727.diff 8 Files Affected: - (modified) clang-tools-extra/clang-tidy/llvm/CMakeLists.txt (+1) - (modified) clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp (+3) - (added) clang-tools-extra/clang-tidy/llvm/MlirUseAfterEraseCheck.cpp (+365) - (added) clang-tools-extra/clang-tidy/llvm/MlirUseAfterEraseCheck.h (+37) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+9) - (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1) - (added) clang-tools-extra/docs/clang-tidy/checks/llvm/mlir-use-after-erase.rst (+39) - (added) clang-tools-extra/test/clang-tidy/checkers/llvm/mlir-use-after-erase.cpp (+476) ``````````diff diff --git a/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt b/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt index bec3ba50c81c5..847695c853cf5 100644 --- a/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt @@ -8,6 +8,7 @@ add_clang_library(clangTidyLLVMModule STATIC HeaderGuardCheck.cpp IncludeOrderCheck.cpp LLVMTidyModule.cpp + MlirUseAfterEraseCheck.cpp PreferIsaOrDynCastInConditionalsCheck.cpp PreferRegisterOverUnsignedCheck.cpp PreferStaticOverAnonymousNamespaceCheck.cpp diff --git a/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp b/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp index 918af88c979e0..1f42cce26a0b8 100644 --- a/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp @@ -14,6 +14,7 @@ #include "FormatvStringCheck.h" #include "HeaderGuardCheck.h" #include "IncludeOrderCheck.h" +#include "MlirUseAfterEraseCheck.h" #include "PreferIsaOrDynCastInConditionalsCheck.h" #include "PreferRegisterOverUnsignedCheck.h" #include "PreferStaticOverAnonymousNamespaceCheck.h" @@ -36,6 +37,8 @@ class LLVMModule : public ClangTidyModule { CheckFactories.registerCheck<FormatvStringCheck>("llvm-formatv-string"); CheckFactories.registerCheck<LLVMHeaderGuardCheck>("llvm-header-guard"); CheckFactories.registerCheck<IncludeOrderCheck>("llvm-include-order"); + CheckFactories.registerCheck<MlirUseAfterEraseCheck>( + "llvm-mlir-use-after-erase"); CheckFactories.registerCheck<readability::NamespaceCommentCheck>( "llvm-namespace-comment"); CheckFactories.registerCheck<PreferIsaOrDynCastInConditionalsCheck>( diff --git a/clang-tools-extra/clang-tidy/llvm/MlirUseAfterEraseCheck.cpp b/clang-tools-extra/clang-tidy/llvm/MlirUseAfterEraseCheck.cpp new file mode 100644 index 0000000000000..5b805cef34b40 --- /dev/null +++ b/clang-tools-extra/clang-tidy/llvm/MlirUseAfterEraseCheck.cpp @@ -0,0 +1,365 @@ +//===----------------------------------------------------------------------===// +// +// 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 "MlirUseAfterEraseCheck.h" +#include "../utils/ExprSequence.h" +#include "clang/AST/Expr.h" +#include "clang/AST/ExprCXX.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Analysis/Analyses/CFGReachabilityAnalysis.h" +#include "clang/Analysis/CFG.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/SmallPtrSet.h" +#include <optional> + +using namespace clang::ast_matchers; +using namespace clang::tidy::utils; + +namespace clang::tidy::llvm_check { + +namespace { + +/// Contains information about a use-after-erase. +struct UseAfterErase { + // The DeclRefExpr that constituted the use of the operation. + const DeclRefExpr *DeclRef; + + // Is the order in which the erase and the use are evaluated undefined? + bool EvaluationOrderUndefined = false; + + // Does the use happen in a later loop iteration than the erase? + // + // We default to false and change it to true if required in find(). + bool UseHappensInLaterLoopIteration = false; +}; + +/// Finds uses of an `mlir::Operation` variable that are reachable after the +/// operation was erased (and maintains state required by the various internal +/// helper functions). +class UseAfterEraseFinder { +public: + UseAfterEraseFinder(ASTContext *TheContext); + + // Within the given code block, finds the first use of 'ErasedVariable' that + // occurs after 'EraseCall' (the expression that erases the operation). If a + // use-after-erase is found, returns information about it. + std::optional<UseAfterErase> find(Stmt *CodeBlock, const Expr *EraseCall, + const DeclRefExpr *ErasedVariable); + +private: + std::optional<UseAfterErase> findInternal(const CFGBlock *Block, + const Expr *EraseCall, + const ValueDecl *ErasedVariable); + void getUsesAndReinits(const CFGBlock *Block, const ValueDecl *ErasedVariable, + SmallVectorImpl<const DeclRefExpr *> *Uses, + llvm::SmallPtrSetImpl<const Stmt *> *Reinits); + void getDeclRefs(const CFGBlock *Block, const Decl *ErasedVariable, + llvm::SmallPtrSetImpl<const DeclRefExpr *> *DeclRefs); + void getReinits(const CFGBlock *Block, const Decl *ErasedVariable, + llvm::SmallPtrSetImpl<const Stmt *> *Stmts, + llvm::SmallPtrSetImpl<const DeclRefExpr *> *DeclRefs); + + ASTContext *Context; + std::unique_ptr<ExprSequence> Sequence; + std::unique_ptr<StmtToBlockMap> BlockMap; + llvm::SmallPtrSet<const CFGBlock *, 8> Visited; +}; + +} // namespace + +UseAfterEraseFinder::UseAfterEraseFinder(ASTContext *TheContext) + : Context(TheContext) {} + +std::optional<UseAfterErase> +UseAfterEraseFinder::find(Stmt *CodeBlock, const Expr *EraseCall, + const DeclRefExpr *ErasedVariable) { + // We include implicit and temporary destructors in the CFG so that + // destructors marked [[noreturn]] are handled correctly in the control flow + // analysis. (These are used in some styles of assertion macros.) + CFG::BuildOptions Options; + Options.AddImplicitDtors = true; + Options.AddTemporaryDtors = true; + const auto TheCFG = CFG::buildCFG(nullptr, CodeBlock, Context, Options); + if (!TheCFG) + return std::nullopt; + + Sequence = std::make_unique<ExprSequence>(TheCFG.get(), CodeBlock, Context); + BlockMap = std::make_unique<StmtToBlockMap>(TheCFG.get(), Context); + Visited.clear(); + + const CFGBlock *EraseBlock = BlockMap->blockContainingStmt(EraseCall); + if (!EraseBlock) + EraseBlock = &TheCFG->getEntry(); + + auto TheUseAfterErase = + findInternal(EraseBlock, EraseCall, ErasedVariable->getDecl()); + + if (TheUseAfterErase) { + if (const CFGBlock *UseBlock = + BlockMap->blockContainingStmt(TheUseAfterErase->DeclRef)) { + // Does the use happen in a later loop iteration than the erase? + // - If they are in the same CFG block, we know the use happened in a + // later iteration if we visited that block a second time. + // - Otherwise, we know the use happened in a later iteration if the + // erase is reachable from the use. + CFGReverseBlockReachabilityAnalysis CFA(*TheCFG); + TheUseAfterErase->UseHappensInLaterLoopIteration = + UseBlock == EraseBlock ? Visited.contains(UseBlock) + : CFA.isReachable(UseBlock, EraseBlock); + } + } + return TheUseAfterErase; +} + +std::optional<UseAfterErase> +UseAfterEraseFinder::findInternal(const CFGBlock *Block, const Expr *EraseCall, + const ValueDecl *ErasedVariable) { + if (Visited.contains(Block)) + return std::nullopt; + + // Mark the block as visited (except if this is the block containing the erase + // call and it's being visited the first time). + if (!EraseCall) + Visited.insert(Block); + + // Get all uses and reinits in the block. + SmallVector<const DeclRefExpr *, 1> Uses; + llvm::SmallPtrSet<const Stmt *, 1> Reinits; + getUsesAndReinits(Block, ErasedVariable, &Uses, &Reinits); + + // Ignore all reinitializations where the erase potentially comes after the + // reinit. + SmallVector<const Stmt *, 1> ReinitsToDelete; + for (const auto *Reinit : Reinits) + if (EraseCall && Sequence->potentiallyAfter(EraseCall, Reinit)) + ReinitsToDelete.push_back(Reinit); + for (const auto *Reinit : ReinitsToDelete) + Reinits.erase(Reinit); + + // Find all uses that potentially come after the erase. + for (const auto *Use : Uses) { + if (!EraseCall || Sequence->potentiallyAfter(Use, EraseCall)) { + // Does the use have a saving reinit? A reinit is saving if it definitely + // comes before the use, i.e. if there's no potential that the reinit is + // after the use. + bool HaveSavingReinit = false; + for (const auto *Reinit : Reinits) + if (!Sequence->potentiallyAfter(Reinit, Use)) + HaveSavingReinit = true; + + if (!HaveSavingReinit) { + UseAfterErase TheUseAfterErase; + TheUseAfterErase.DeclRef = Use; + + // Is this a use-after-erase that depends on order of evaluation? + // This is the case if the erase potentially comes after the use (and we + // already know that the use potentially comes after the erase, which + // taken together tells us that the ordering is unclear). + TheUseAfterErase.EvaluationOrderUndefined = + EraseCall != nullptr && Sequence->potentiallyAfter(EraseCall, Use); + + return TheUseAfterErase; + } + } + } + + // If the operation wasn't reinitialized, call ourselves recursively on all + // successors. + if (Reinits.empty()) { + for (const auto &Succ : Block->succs()) + if (Succ) + if (auto Found = findInternal(Succ, nullptr, ErasedVariable)) + return Found; + } + + return std::nullopt; +} + +void UseAfterEraseFinder::getUsesAndReinits( + const CFGBlock *Block, const ValueDecl *ErasedVariable, + SmallVectorImpl<const DeclRefExpr *> *Uses, + llvm::SmallPtrSetImpl<const Stmt *> *Reinits) { + llvm::SmallPtrSet<const DeclRefExpr *, 1> DeclRefs; + llvm::SmallPtrSet<const DeclRefExpr *, 1> ReinitDeclRefs; + + getDeclRefs(Block, ErasedVariable, &DeclRefs); + getReinits(Block, ErasedVariable, Reinits, &ReinitDeclRefs); + + // All references to the variable that aren't reinitializations are uses. + Uses->clear(); + for (const DeclRefExpr *DeclRef : DeclRefs) + if (!ReinitDeclRefs.contains(DeclRef)) + Uses->push_back(DeclRef); + + // Sort the uses by their occurrence in the source code. + llvm::sort(*Uses, [](const DeclRefExpr *D1, const DeclRefExpr *D2) { + return D1->getExprLoc() < D2->getExprLoc(); + }); +} + +void UseAfterEraseFinder::getDeclRefs( + const CFGBlock *Block, const Decl *ErasedVariable, + llvm::SmallPtrSetImpl<const DeclRefExpr *> *DeclRefs) { + DeclRefs->clear(); + const auto DeclRefMatcher = + declRefExpr(to(equalsNode(ErasedVariable))).bind("declref"); + + for (const auto &Elem : *Block) { + const auto S = Elem.getAs<CFGStmt>(); + if (!S) + continue; + + for (const auto &Match : + match(findAll(DeclRefMatcher), *S->getStmt(), *Context)) { + const auto *DeclRef = Match.getNodeAs<DeclRefExpr>("declref"); + if (DeclRef && BlockMap->blockContainingStmt(DeclRef) == Block) + DeclRefs->insert(DeclRef); + } + } +} + +void UseAfterEraseFinder::getReinits( + const CFGBlock *Block, const Decl *ErasedVariable, + llvm::SmallPtrSetImpl<const Stmt *> *Stmts, + llvm::SmallPtrSetImpl<const DeclRefExpr *> *DeclRefs) { + const auto DeclRefMatcher = + declRefExpr(to(equalsNode(ErasedVariable))).bind("declref"); + + // A reinitialization gives the variable a new, valid operation, which makes + // subsequent uses safe again. We treat plain assignments and + // (re-)declarations as reinitializations. For "derived" ops (e.g. + // `mlir::func::FuncOp`), the assignment is an overloaded `operator=`, which + // is represented as a `CXXOperatorCallExpr` rather than a `BinaryOperator`. + const auto ReinitMatcher = + stmt(anyOf(binaryOperator(hasOperatorName("="), + hasLHS(ignoringParenImpCasts(DeclRefMatcher))), + cxxOperatorCallExpr( + hasOverloadedOperatorName("="), + hasArgument(0, ignoringParenImpCasts(DeclRefMatcher))), + declStmt(hasDescendant(equalsNode(ErasedVariable))))) + .bind("reinit"); + + Stmts->clear(); + DeclRefs->clear(); + for (const CFGElement &Elem : *Block) { + const auto S = Elem.getAs<CFGStmt>(); + if (!S) + continue; + + for (const auto &Match : + match(findAll(ReinitMatcher), *S->getStmt(), *Context)) { + const auto *TheStmt = Match.getNodeAs<Stmt>("reinit"); + const auto *DeclRef = Match.getNodeAs<DeclRefExpr>("declref"); + if (TheStmt && BlockMap->blockContainingStmt(TheStmt) == Block) { + Stmts->insert(TheStmt); + + // We count DeclStmts as reinitializations, but they don't have a + // DeclRefExpr associated with them -- so we need to check 'DeclRef' + // before adding it to the set. + if (DeclRef) + DeclRefs->insert(DeclRef); + } + } + } +} + +static void emitDiagnostic(const Expr *EraseCall, const DeclRefExpr *EraseArg, + const UseAfterErase &Use, ClangTidyCheck *Check) { + const auto UseLoc = Use.DeclRef->getExprLoc(); + const auto EraseLoc = EraseCall->getExprLoc(); + + Check->diag(UseLoc, "operation %0 is used after it was erased") + << EraseArg->getDecl(); + Check->diag(EraseLoc, "operation erased here", DiagnosticIDs::Note); + if (Use.EvaluationOrderUndefined) { + Check->diag(UseLoc, + "the use and erase are unsequenced, i.e. there is no guarantee " + "about the order in which they are evaluated", + DiagnosticIDs::Note); + } else if (Use.UseHappensInLaterLoopIteration) { + Check->diag(UseLoc, + "the use happens in a later loop iteration than the erase", + DiagnosticIDs::Note); + } +} + +void MlirUseAfterEraseCheck::registerMatchers(MatchFinder *Finder) { + // The reference to the operation that is being erased. + const auto Arg = declRefExpr(to(varDecl())).bind("arg"); + + // A "derived" op is a value of a subclass of `mlir::OpState` (e.g. + // `mlir::func::FuncOp`) that wraps an `mlir::Operation *`. The underlying + // operation can be obtained via the overloaded `operator->` or via + // `getOperation()`, both of which are declared in `mlir::OpState`. + const auto OfOpState = ofClass(isSameOrDerivedFrom("::mlir::OpState")); + const auto UnwrapDerivedOp = + expr(anyOf(cxxMemberCallExpr( + on(ignoringParenImpCasts(Arg)), + callee(cxxMethodDecl(hasName("getOperation"), OfOpState))), + cxxOperatorCallExpr(hasOverloadedOperatorName("->"), + hasArgument(0, ignoringParenImpCasts(Arg)), + callee(cxxMethodDecl(OfOpState))))); + + // A reference to the operation, either directly (a variable of type + // `mlir::Operation *`) or through a derived op that is unwrapped to the + // underlying `mlir::Operation *`. + const auto OpRef = expr(ignoringParenImpCasts(anyOf(Arg, UnwrapDerivedOp))); + + // Erasure through `mlir::Operation::erase` and `mlir::Operation::destroy` + // member functions. + const auto MemberErase = cxxMemberCallExpr( + on(OpRef), callee(cxxMethodDecl(hasAnyName("erase", "destroy"), + ofClass(hasName("::mlir::Operation"))))); + + // Erasure through `mlir::RewriterBase`. The operation that gets invalidated + // is always the first argument of these methods. + const auto RewriterErase = cxxMemberCallExpr( + callee( + cxxMethodDecl(hasAnyName("eraseOp", "eraseOpResults", "replaceOp", + "replaceOpWithNewOp"), + ofClass(isSameOrDerivedFrom("::mlir::RewriterBase")))), + hasArgument(0, OpRef)); + + Finder->addMatcher( + traverse(TK_AsIs, + expr(anyOf(MemberErase, RewriterErase), + anyOf(hasAncestor(compoundStmt(hasParent( + lambdaExpr().bind("containing-lambda")))), + hasAncestor(functionDecl().bind("containing-func")))) + .bind("erase-call")), + this); +} + +void MlirUseAfterEraseCheck::check(const MatchFinder::MatchResult &Result) { + const auto *EraseCall = Result.Nodes.getNodeAs<Expr>("erase-call"); + const auto *Arg = Result.Nodes.getNodeAs<DeclRefExpr>("arg"); + const auto *ContainingLambda = + Result.Nodes.getNodeAs<LambdaExpr>("containing-lambda"); + const auto *ContainingFunc = + Result.Nodes.getNodeAs<FunctionDecl>("containing-func"); + + // Only track operations that are stored in a local variable. + if (!Arg->getDecl()->getDeclContext()->isFunctionOrMethod()) + return; + + // Collect the code block that could use the operation after it was erased. + SmallVector<Stmt *> CodeBlocks; + if (ContainingLambda) + CodeBlocks.push_back(ContainingLambda->getBody()); + else if (ContainingFunc) + CodeBlocks.push_back(ContainingFunc->getBody()); + + for (Stmt *CodeBlock : CodeBlocks) { + UseAfterEraseFinder Finder(Result.Context); + if (auto Use = Finder.find(CodeBlock, EraseCall, Arg)) + emitDiagnostic(EraseCall, Arg, *Use, this); + } +} + +} // namespace clang::tidy::llvm_check diff --git a/clang-tools-extra/clang-tidy/llvm/MlirUseAfterEraseCheck.h b/clang-tools-extra/clang-tidy/llvm/MlirUseAfterEraseCheck.h new file mode 100644 index 0000000000000..52574ac76fdf9 --- /dev/null +++ b/clang-tools-extra/clang-tidy/llvm/MlirUseAfterEraseCheck.h @@ -0,0 +1,37 @@ +//===----------------------------------------------------------------------===// +// +// 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 +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_MLIRUSEAFTERERASECHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_MLIRUSEAFTERERASECHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::llvm_check { + +/// Detects uses of an ``mlir::Operation`` after it has been erased, either +/// directly through ``Operation::erase``/``Operation::destroy``, through +/// ``mlir::RewriterBase`` helpers such as ``eraseOp`` and ``replaceOp``, +/// or through derived ops of ``mlir::OpState`` that provide are wrappers +/// around an ``mlir::Operation``. +/// +/// For the user-facing documentation see: +/// https://clang.llvm.org/extra/clang-tidy/checks/llvm/mlir-use-after-erase.html +class MlirUseAfterEraseCheck : public ClangTidyCheck { +public: + MlirUseAfterEraseCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus; + } +}; + +} // namespace clang::tidy::llvm_check + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_MLIRUSEAFTERERASECHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index dd397b574b6f4..f7bd617eb1919 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -97,6 +97,15 @@ Improvements to clang-tidy New checks ^^^^^^^^^^ +- New :doc:`llvm-mlir-use-after-erase + <clang-tidy/checks/llvm/mlir-use-after-erase>` check. + + Detects uses of an ``mlir::Operation`` after it has been erased, either + directly through ``Operation::erase``/``Operation::destroy``, through + ``mlir::RewriterBase`` helpers such as ``eraseOp`` and ``replaceOp``, + or through derived ops of ``mlir::OpState`` that are wrappers around + an ``mlir::Operation``. + - New :doc:`performance-expensive-value-or <clang-tidy/checks/performance/expensive-value-or>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 8758efe14080c..d20cf004afcda 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -246,6 +246,7 @@ Clang-Tidy Checks :doc:`llvm-formatv-string <llvm/formatv-string>`, :doc:`llvm-header-guard <llvm/header-guard>`, :doc:`llvm-include-order <llvm/include-order>`, "Yes" + :doc:`llvm-mlir-use-after-erase <llvm/mlir-use-after-erase>`, :doc:`llvm-namesp... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/210727 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
