https://github.com/higher-performance updated https://github.com/llvm/llvm-project/pull/172566
>From b16f83e4441e9f6aa71299c397ca47ee3744aac8 Mon Sep 17 00:00:00 2001 From: higher-performance <[email protected]> Date: Tue, 16 Dec 2025 17:22:13 -0500 Subject: [PATCH] [clang-tidy] Add support for use-after-suspend to bugprone-use-after-move --- .../clang-tidy/bugprone/UseAfterMoveCheck.cpp | 144 +++++++++++++---- .../clang-tidy/bugprone/UseAfterMoveCheck.h | 2 + .../checks/bugprone/use-after-move.rst | 28 +++- .../bugprone/use-after-move-cxx20.cpp | 152 ++++++++++++++++++ 4 files changed, 290 insertions(+), 36 deletions(-) create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move-cxx20.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp index 64387024dafd6..467d5f4f8fb49 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp @@ -81,6 +81,29 @@ class UseAfterMoveFinder { llvm::SmallPtrSet<const CFGBlock *, 8> Visited; }; +// Matches the expression awaited by the `co_await`. +// TODO: Merge with the `awaitable` matcher in CoroutineHostileRAIICheck. +AST_MATCHER_P(CoroutineSuspendExpr, suspendExpr, + ast_matchers::internal::Matcher<Expr>, InnerMatcher) { + if (const Expr *E = Node.getOperand()) { + return InnerMatcher.matches(*E, Finder, Builder); + } + return false; +} + +// TODO: Merge with the corresponding function in CoroutineHostileRAIICheck. +auto typeWithNameIn(const std::vector<StringRef> &Names) { + return hasType(hasCanonicalType( + hasDeclaration(namedDecl(matchers::matchesAnyListedName(Names))))); +} + +// TODO: Merge with the corresponding function in CoroutineHostileRAIICheck. +auto functionWithNameIn(const std::vector<StringRef> &Names) { + auto Call = + callExpr(callee(functionDecl(matchers::matchesAnyListedName(Names)))); + return anyOf(expr(cxxBindTemporaryExpr(has(Call))), expr(Call)); +} + } // namespace static auto getNameMatcher(llvm::ArrayRef<StringRef> InvalidationFunctions) { @@ -418,6 +441,7 @@ enum MoveType { Forward = 0, // std::forward Move = 1, // std::move Invalidation = 2, // other + Suspend, // co_yield, co_await }; } // namespace @@ -440,22 +464,25 @@ static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg, const SourceLocation MoveLoc = MovingCall->getExprLoc(); Check->diag(UseLoc, - "'%0' used after it was %select{forwarded|moved|invalidated}1") + "'%0' used after %select{it was forwarded|it was moved|it was " + "invalidated|a suspension point}1") << MoveArg->getDecl()->getName() << Type; - Check->diag(MoveLoc, "%select{forward|move|invalidation}0 occurred here", + Check->diag(MoveLoc, + "%select{forward|move|invalidation|suspension}0 occurred here", DiagnosticIDs::Note) << Type; if (Use.EvaluationOrderUndefined) { Check->diag( UseLoc, - "the use and %select{forward|move|invalidation}0 are unsequenced, i.e. " + "the use and %select{forward|move|invalidation|suspension}0 are " + "unsequenced, i.e. " "there is no guarantee about the order in which they are evaluated", DiagnosticIDs::Note) << Type; } else if (Use.UseHappensInLaterLoopIteration) { Check->diag(UseLoc, "the use happens in a later loop iteration than the " - "%select{forward|move|invalidation}0", + "%select{forward|move|invalidation|suspension}0", DiagnosticIDs::Note) << Type; } @@ -466,13 +493,21 @@ UseAfterMoveCheck::UseAfterMoveCheck(StringRef Name, ClangTidyContext *Context) InvalidationFunctions(utils::options::parseStringList( Options.get("InvalidationFunctions", ""))), ReinitializationFunctions(utils::options::parseStringList( - Options.get("ReinitializationFunctions", ""))) {} + Options.get("ReinitializationFunctions", ""))), + Awaitables( + utils::options::parseStringList(Options.get("Awaitables", ""))), + NonlocalAccessors(utils::options::parseStringList( + Options.get("NonlocalAccessors", "::std::suspend_always"))) {} void UseAfterMoveCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "InvalidationFunctions", utils::options::serializeStringList(InvalidationFunctions)); Options.store(Opts, "ReinitializationFunctions", utils::options::serializeStringList(ReinitializationFunctions)); + Options.store(Opts, "Awaitables", + utils::options::serializeStringList(Awaitables)); + Options.store(Opts, "NonlocalAccessors", + utils::options::serializeStringList(NonlocalAccessors)); } void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) { @@ -484,25 +519,29 @@ void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) { cxxMemberCallExpr(callee(cxxMethodDecl(hasName("try_emplace")))); auto Arg = declRefExpr().bind("arg"); auto IsMemberCallee = callee(functionDecl(unless(isStaticStorageClass()))); - auto CallMoveMatcher = - callExpr(callee(functionDecl(getNameMatcher(InvalidationFunctions)) - .bind("move-decl")), - anyOf(cxxMemberCallExpr(IsMemberCallee, on(Arg)), - callExpr(unless(cxxMemberCallExpr(IsMemberCallee)), - hasArgument(0, Arg))), - unless(inDecltypeOrTemplateArg()), - unless(hasParent(TryEmplaceMatcher)), expr().bind("call-move"), - anyOf(hasAncestor(compoundStmt( - hasParent(lambdaExpr().bind("containing-lambda")))), - hasAncestor(functionDecl(anyOf( - cxxConstructorDecl( - hasAnyConstructorInitializer(withInitializer( - expr(anyOf(equalsBoundNode("call-move"), - hasDescendant(expr( - equalsBoundNode("call-move"))))) - .bind("containing-ctor-init")))) - .bind("containing-ctor"), - functionDecl().bind("containing-func")))))); + auto Awaitee = suspendExpr( + anyOf(typeWithNameIn(Awaitables), functionWithNameIn(Awaitables))); + auto CallMoveMatcher = expr( + anyOf(callExpr(callee(functionDecl(getNameMatcher(InvalidationFunctions)) + .bind("move-decl")), + anyOf(cxxMemberCallExpr(IsMemberCallee, on(Arg)), + callExpr(unless(cxxMemberCallExpr(IsMemberCallee)), + hasArgument(0, Arg)))), + coyieldExpr(Awaitee), + coawaitExpr(allOf(unless(coawaitExpr(isImplicit())), Awaitee))), + unless(inDecltypeOrTemplateArg()), unless(hasParent(TryEmplaceMatcher)), + expr().bind("call-move"), + anyOf(hasAncestor(compoundStmt( + hasParent(lambdaExpr().bind("containing-lambda")))), + hasAncestor(functionDecl( + anyOf(cxxConstructorDecl( + hasAnyConstructorInitializer(withInitializer( + expr(anyOf(equalsBoundNode("call-move"), + hasDescendant(expr( + equalsBoundNode("call-move"))))) + .bind("containing-ctor-init")))) + .bind("containing-ctor"), + functionDecl().bind("containing-func")))))); Finder->addMatcher( traverse( @@ -533,7 +572,7 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) { Result.Nodes.getNodeAs<LambdaExpr>("containing-lambda"); const auto *ContainingFunc = Result.Nodes.getNodeAs<FunctionDecl>("containing-func"); - const auto *CallMove = Result.Nodes.getNodeAs<CallExpr>("call-move"); + const auto *CallMove = Result.Nodes.getNodeAs<Expr>("call-move"); const auto *MovingCall = Result.Nodes.getNodeAs<Expr>("moving-call"); const auto *Arg = Result.Nodes.getNodeAs<DeclRefExpr>("arg"); const auto *MoveDecl = Result.Nodes.getNodeAs<FunctionDecl>("move-decl"); @@ -543,13 +582,13 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) { // Ignore the std::move if the variable that was passed to it isn't a local // variable. - if (!Arg->getDecl()->getDeclContext()->isFunctionOrMethod()) + if (Arg && !Arg->getDecl()->getDeclContext()->isFunctionOrMethod()) return; // Collect all code blocks that could use the arg after move. - llvm::SmallVector<Stmt *> CodeBlocks{}; + llvm::SmallVector<std::pair<const Decl *, Stmt *>> CodeBlocks{}; if (ContainingCtor) { - CodeBlocks.push_back(ContainingCtor->getBody()); + CodeBlocks.push_back({ContainingCtor, ContainingCtor->getBody()}); if (ContainingCtorInit) { // Collect the constructor initializer expressions. bool BeforeMove{true}; @@ -558,21 +597,56 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) { ContainingCtorInit->IgnoreImplicit()) BeforeMove = false; if (!BeforeMove) - CodeBlocks.push_back(Init->getInit()); + CodeBlocks.push_back({ContainingCtor, Init->getInit()}); } } } else if (ContainingLambda) { - CodeBlocks.push_back(ContainingLambda->getBody()); + CodeBlocks.push_back( + {ContainingLambda->getCallOperator(), ContainingLambda->getBody()}); } else if (ContainingFunc) { - CodeBlocks.push_back(ContainingFunc->getBody()); + CodeBlocks.push_back({ContainingFunc, ContainingFunc->getBody()}); } - for (Stmt *CodeBlock : CodeBlocks) { + for (auto [ContainingDecl, CodeBlock] : CodeBlocks) { UseAfterMoveFinder Finder(Result.Context, InvalidationFunctions, ReinitializationFunctions); - if (auto Use = Finder.find(CodeBlock, MovingCall, Arg)) - emitDiagnostic(MovingCall, Arg, *Use, this, Result.Context, - determineMoveType(MoveDecl)); + if (Arg) { + // Non-coroutine cases + if (auto Use = Finder.find(CodeBlock, MovingCall, Arg)) + emitDiagnostic(MovingCall, Arg, *Use, this, Result.Context, + determineMoveType(MoveDecl)); + } else { + // Coroutine cases (use-after-suspend, to catch pointers to thread-locals) + llvm::SmallVector<const DeclRefExpr *> DeclRefs; + // Find all local variables declared inside this code block + auto InterestingCallMatcher = callExpr( + callee( + functionDecl(matchers::matchesAnyListedName(NonlocalAccessors))), + unless(hasParent(memberExpr( + hasDeclaration(functionDecl(unless(returns(hasCanonicalType( + anyOf(referenceType(), pointerType())))))))))); + auto DeclsMatcher = + declRefExpr(to(varDecl(unless(isImplicit()), + hasDeclContext(equalsNode(ContainingDecl)), + hasType(hasUnqualifiedDesugaredType( + anyOf(pointerType(), referenceType()))), + hasInitializer(anyOf( + InterestingCallMatcher, + hasDescendant(InterestingCallMatcher)))))) + .bind("declref"); + for (const auto &bound : + match(findAll(DeclsMatcher), *CodeBlock, *Result.Context)) { + DeclRefs.push_back(bound.getNodeAs<DeclRefExpr>("declref")); + } + for (const DeclRefExpr *DeclRef : DeclRefs) { + if (auto Use = Finder.find(CodeBlock, MovingCall, DeclRef)) { + emitDiagnostic(MovingCall, DeclRef, *Use, this, Result.Context, + isa<CoroutineSuspendExpr>(MovingCall) + ? MoveType::Suspend + : determineMoveType(MoveDecl)); + } + } + } } } diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.h b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.h index fff1c2621867d..c6b1fc6286b21 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.h @@ -31,6 +31,8 @@ class UseAfterMoveCheck : public ClangTidyCheck { private: std::vector<StringRef> InvalidationFunctions; std::vector<StringRef> ReinitializationFunctions; + std::vector<StringRef> Awaitables; + std::vector<StringRef> NonlocalAccessors; }; } // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst index 95a752e9399a9..5dab2141888bd 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst @@ -3,7 +3,8 @@ bugprone-use-after-move ======================= -Warns if an object is used after it has been moved, for example: +Warns if an object is used after it has been moved, forwarded, or otherwise +potentially invalidated, for example: .. code-block:: c++ @@ -254,6 +255,17 @@ forget to add the reinitialization for this additional member. Instead, it is safer to assign to the entire struct in one go, and this will also avoid the use-after-move warning. +Coroutines +---------- + +This check also searches for occurrences of "use-after-suspend" in C++ +coroutines. This can be used, for example, to detect when a coroutine accesses +thread-local data after a suspension point (i.e. `co_yield` or `co_await`), +where the reference or pointer used to access the data was acquired prior to the +suspension point. Such situations can be dangerous as the referenced memory may +belong to a different thread after suspension, or have been deallocated +entirely by the time the coroutine resumes. + Options ------- @@ -271,3 +283,17 @@ Options considered to be reinitialized. For non-member or static member functions, the first argument is considered to be reinitialized. Default value is an empty string. + +.. option:: Awaitables + + A semicolon-separated list of types or functions (the operands to `co_await`) + that may potentially suspend the current execution, causing the calling + coroutine to resume before the callee has finished. Default value is + `::std::suspend_always`. + +.. option:: NonlocalAccessors + + A semicolon-separated list of functions that return a pointer or reference + to data that may become invalidated after a coroutine suspension, such as + a function that returns a pointer to thread-local data. Default value is an + empty string. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move-cxx20.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move-cxx20.cpp new file mode 100644 index 0000000000000..811b0a50274a9 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move-cxx20.cpp @@ -0,0 +1,152 @@ +// RUN: %check_clang_tidy -std=c++20-or-later %s bugprone-use-after-move %t -- \ +// RUN: -config='{CheckOptions: { \ +// RUN: bugprone-use-after-move.Awaitables: "::std::suspend_always;::CustomAwaitable", \ +// RUN: bugprone-use-after-move.NonlocalAccessors: "::GetNonlocalState" \ +// RUN: }}' -- \ +// RUN: -fno-delayed-template-parsing + +namespace std { +template <typename R, typename...> +struct coroutine_traits { + using promise_type = typename R::promise_type; +}; + +template <typename Promise = void> +struct coroutine_handle; + +template <> +struct coroutine_handle<void> { + static coroutine_handle from_address(void *addr) noexcept; + void operator()(); + void *address() const noexcept; + void resume() const; + void destroy() const; + bool done() const; + coroutine_handle &operator=(decltype(nullptr)); + coroutine_handle(decltype(nullptr)); + coroutine_handle(); + explicit operator bool() const; +}; + +template <typename Promise> +struct coroutine_handle : coroutine_handle<> { + using coroutine_handle<>::operator=; + static coroutine_handle from_address(void *addr) noexcept; + Promise &promise() const; + static coroutine_handle from_promise(Promise &promise); +}; + +struct suspend_always { + bool await_ready() noexcept; + void await_suspend(coroutine_handle<>) noexcept; + void await_resume() noexcept; +}; + +} // namespace std + +struct CustomAwaitable { + bool await_ready() noexcept; + void await_suspend(std::coroutine_handle<>) noexcept; + void await_resume() noexcept; +}; + +class A { +public: + A(); + A(const A &); + A(A &&); + + A &operator=(const A &); + A &operator=(A &&); + + void foo() const; + void bar(int i) const; + int getInt() const; + + operator bool() const; + + int i; +}; + +template <class Elem, class Final> +class Coroutine final { + public: + struct promise_type; + explicit Coroutine(std::coroutine_handle<promise_type> h); + ~Coroutine(); + struct promise_type { + std::suspend_always final_suspend() noexcept; + Coroutine get_return_object() noexcept; + std::suspend_always initial_suspend() noexcept; + void return_void(); + void unhandled_exception(); + std::suspend_always yield_value(const Elem &); + }; +}; + +struct GlobalState { + int val() const; + const int &ref() const; + const void *ptr() const; +}; +const GlobalState &GetNonlocalState(); + +template <class T> +void use(const T &); + +namespace coroutines { + +Coroutine<int, void> simpleSuspension() { + { + A a; + a.foo(); + auto &&ctx = GetNonlocalState(); + use(ctx); +#if __cplusplus >= 202002L + co_yield 0; + a.foo(); + use(ctx); + // CHECK-NOTES: [[@LINE-1]]:9: warning: 'ctx' used after a suspension point [bugprone-use-after-move] + // CHECK-NOTES: [[@LINE-4]]:5: note: suspension occurred here +#endif + } + { + A a; + a.foo(); + auto &ctx = GetNonlocalState().ref(); + use(ctx); +#if __cplusplus >= 202002L + co_await CustomAwaitable(); + a.foo(); + use(ctx); + // CHECK-NOTES: [[@LINE-1]]:9: warning: 'ctx' used after a suspension point [bugprone-use-after-move] + // CHECK-NOTES: [[@LINE-4]]:5: note: suspension occurred here +#endif + } + { + A a; + a.foo(); + auto *ctx = GetNonlocalState().ptr(); + use(ctx); +#if __cplusplus >= 202002L + co_yield 0; + a.foo(); + use(ctx); + // CHECK-NOTES: [[@LINE-1]]:9: warning: 'ctx' used after a suspension point [bugprone-use-after-move] + // CHECK-NOTES: [[@LINE-4]]:5: note: suspension occurred here +#endif + } + { + A a; + a.foo(); + auto &&ctx = GetNonlocalState().val(); + use(ctx); +#if __cplusplus >= 202002L + co_yield 0; + a.foo(); + use(ctx); // No error +#endif + } +} + +} // namespace coroutines _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
