https://github.com/higher-performance updated https://github.com/llvm/llvm-project/pull/172566
>From 173c81f3cf74ef5c39d09a21db17d090085b06ba Mon Sep 17 00:00:00 2001 From: higher-performance <[email protected]> Date: Tue, 16 Dec 2025 17:21:53 -0500 Subject: [PATCH 1/2] Make isImplicit() AST matcher work with CoawaitExpr --- clang/include/clang/ASTMatchers/ASTMatchers.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/clang/include/clang/ASTMatchers/ASTMatchers.h b/clang/include/clang/ASTMatchers/ASTMatchers.h index e3ec26207d2bc..2c21ffa846d91 100644 --- a/clang/include/clang/ASTMatchers/ASTMatchers.h +++ b/clang/include/clang/ASTMatchers/ASTMatchers.h @@ -775,7 +775,8 @@ AST_MATCHER_P(ClassTemplateSpecializationDecl, hasSpecializedTemplate, /// implicit default/copy constructors). AST_POLYMORPHIC_MATCHER(isImplicit, AST_POLYMORPHIC_SUPPORTED_TYPES(Decl, Attr, - LambdaCapture)) { + LambdaCapture, + CoawaitExpr)) { return Node.isImplicit(); } >From b76eca19fd6bfdc71cb49c8a35fb79e8cf1269ab Mon Sep 17 00:00:00 2001 From: higher-performance <[email protected]> Date: Tue, 16 Dec 2025 17:22:13 -0500 Subject: [PATCH 2/2] [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 b2e08fe688a1b..f7278b849191a 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp @@ -79,8 +79,31 @@ 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; +} + } // namespace +// TODO: Merge with the corresponding function in CoroutineHostileRAIICheck. +static auto typeWithNameIn(const std::vector<StringRef> &Names) { + return hasType(hasCanonicalType( + hasDeclaration(namedDecl(matchers::matchesAnyListedName(Names))))); +} + +// TODO: Merge with the corresponding function in CoroutineHostileRAIICheck. +static auto functionWithNameIn(const std::vector<StringRef> &Names) { + auto Call = + callExpr(callee(functionDecl(matchers::matchesAnyListedName(Names)))); + return anyOf(expr(cxxBindTemporaryExpr(has(Call))), expr(Call)); +} + static auto getNameMatcher(llvm::ArrayRef<StringRef> InvalidationFunctions) { return anyOf(hasAnyName("::std::move", "::std::forward"), matchers::matchesAnyListedName(InvalidationFunctions)); @@ -401,6 +424,7 @@ enum MoveType { Forward = 0, // std::forward Move = 1, // std::move Invalidation = 2, // other + Suspend = 3, // co_yield, co_await }; static MoveType determineMoveType(const FunctionDecl *FuncDecl) { @@ -421,22 +445,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; } @@ -445,11 +472,19 @@ static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg, UseAfterMoveCheck::UseAfterMoveCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), InvalidationFunctions(utils::options::parseStringList( - Options.get("InvalidationFunctions", ""))) {} + Options.get("InvalidationFunctions", ""))), + Awaitables(utils::options::parseStringList( + Options.get("Awaitables", "::std::suspend_always"))), + NonlocalAccessors(utils::options::parseStringList( + Options.get("NonlocalAccessors", ""))) {} void UseAfterMoveCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "InvalidationFunctions", utils::options::serializeStringList(InvalidationFunctions)); + Options.store(Opts, "Awaitables", + utils::options::serializeStringList(Awaitables)); + Options.store(Opts, "NonlocalAccessors", + utils::options::serializeStringList(NonlocalAccessors)); } void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) { @@ -461,25 +496,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( @@ -510,7 +549,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"); @@ -520,13 +559,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}; @@ -535,20 +574,55 @@ 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); - 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 1bbf5c00785ff..b51d0c9d7f9a0 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.h @@ -30,6 +30,8 @@ class UseAfterMoveCheck : public ClangTidyCheck { private: std::vector<StringRef> InvalidationFunctions; + 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 77424d3d620bb..0648624c07197 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 ------- @@ -263,3 +275,17 @@ Options arguments to be invalidated (e.g., closing a handle). For member functions, the first argument is considered to be the implicit object argument (``this``). 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
