[Coroutines] Allow [[clang::coro_wrapper]] for class

[ChuanqiXu9]

Previously we allow [[clang::coro_wrapper]] to be marked with function to allow it to not be checked by [[clang::coro_return_type]].

But in our internal practice, there are classes can be return type of coroutines and non-coroutines and there are too many uses. It is slightly hard to mark every use with [[clang::coro_wrapper]]. So it is a sugar to allow we mark the class as [[clang::coro_wrapper]].

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-coroutines

Author: Chuanqi Xu (ChuanqiXu9)

Changes

Previously we allow [[clang::coro_wrapper]] to be marked with function to allow it to not be checked by [[clang::coro_return_type]].

But in our internal practice, there are classes can be return type of coroutines and non-coroutines and there are too many uses. It is slightly hard to mark every use with [[clang::coro_wrapper]]. So it is a sugar to allow we mark the class as [[clang::coro_wrapper]].

---

Full diff: https://github.com/llvm/llvm-project/pull/93268.diff

4 Files Affected:

• (modified) clang/include/clang/Basic/Attr.td (+1-1)
• (modified) clang/lib/Sema/SemaDecl.cpp (+2-1)
• (modified) clang/test/Misc/pragma-attribute-supported-attributes-list.test (+1-1)
• (modified) clang/test/SemaCXX/coro-return-type-and-wrapper.cpp (+19)

diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td index e59cccccdd369..7933fd5b38d87 100644 --- a/clang/include/clang/Basic/Attr.td +++ b/clang/include/clang/Basic/Attr.td @@ -1192,7 +1192,7 @@ def CoroReturnType : InheritableAttr { def CoroWrapper : InheritableAttr { let Spellings = [Clang<"coro_wrapper">]; - let Subjects = SubjectList<[Function]>; + let Subjects = SubjectList<[Function, CXXRecord]>; let LangOpts = [CPlusPlus]; let Documentation = [CoroReturnTypeAndWrapperDoc]; let SimpleHandler = 1; diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 2a87b26f17a2b..9cae3a0022dc2 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -15998,7 +15998,8 @@ void Sema::CheckCoroutineWrapper(FunctionDecl *FD) { // Allow some_promise_type::get_return_object(). if (CanBeGetReturnObject(FD) || CanBeGetReturnTypeOnAllocFailure(FD)) return; - if (!FD->hasAttr<CoroWrapperAttr>()) + if (!FD->hasAttr<CoroWrapperAttr>() && + !RD->getUnderlyingDecl()->hasAttr<CoroWrapperAttr>()) Diag(FD->getLocation(), diag::err_coroutine_return_type) << RD; } diff --git a/clang/test/Misc/pragma-attribute-supported-attributes-list.test b/clang/test/Misc/pragma-attribute-supported-attributes-list.test index 99732694f72a5..04dc2962ac04e 100644 --- a/clang/test/Misc/pragma-attribute-supported-attributes-list.test +++ b/clang/test/Misc/pragma-attribute-supported-attributes-list.test @@ -62,7 +62,7 @@ // CHECK-NEXT: CoroLifetimeBound (SubjectMatchRule_record) // CHECK-NEXT: CoroOnlyDestroyWhenComplete (SubjectMatchRule_record) // CHECK-NEXT: CoroReturnType (SubjectMatchRule_record) -// CHECK-NEXT: CoroWrapper (SubjectMatchRule_function) +// CHECK-NEXT: CoroWrapper (SubjectMatchRule_function, SubjectMatchRule_record) // CHECK-NEXT: DLLExport (SubjectMatchRule_function, SubjectMatchRule_variable, SubjectMatchRule_record, SubjectMatchRule_objc_interface) // CHECK-NEXT: DLLImport (SubjectMatchRule_function, SubjectMatchRule_variable, SubjectMatchRule_record, SubjectMatchRule_objc_interface) // CHECK-NEXT: Destructor (SubjectMatchRule_function) diff --git a/clang/test/SemaCXX/coro-return-type-and-wrapper.cpp b/clang/test/SemaCXX/coro-return-type-and-wrapper.cpp index b08e1c9c065a0..3c174f3ffaecf 100644 --- a/clang/test/SemaCXX/coro-return-type-and-wrapper.cpp +++ b/clang/test/SemaCXX/coro-return-type-and-wrapper.cpp @@ -139,3 +139,22 @@ template<> class coroutine_traits<Task, int> { } // namespace std // expected-error@+1 {{neither a coroutine nor a coroutine wrapper}} Task foo(int) { return Task{}; } + +// Testing that we can add a `[[clang::coro_wrapper]]` attribute to the class. +class [[clang::coro_return_type]] [[clang::coro_wrapper]] WrappedTask{ +public: + struct promise_type { + Task get_return_object() { + return {}; + } + suspend_always initial_suspend(); + suspend_always final_suspend() noexcept; + void unhandled_exception(); + }; +}; +namespace std { +template<> class coroutine_traits<WrappedTask, int> { + using promise_type = WrappedTask::promise_type; +}; +} // namespace std +WrappedTask wrapped_class(int) { return WrappedTask{}; } 

[snarkmaster]

@ChuanqiXu9, I just ran across this PR -- and I immediately wondered if it could be used to improve stack-use-after-free checks for folly::result (the short-circuiting coro you saw on my recent PR).

Like the classes you mention in the PR description here, folly::result has many reasonable "coro" and "coro wrapper" uses, and it would be onerous to annotate every wrapper usage as coro_wrapper.

What did you envision the effect of adding coro_wrapper to a class would be on lifetime checks?

Can you walk me through your reasoning / expected usage?

[ChuanqiXu9]

@ChuanqiXu9, I just ran across this PR -- and I immediately wondered if it could be used to improve stack-use-after-free checks for folly::result (the short-circuiting coro you saw on my recent PR).

Like the classes you mention in the PR description here, folly::result has many reasonable "coro" and "coro wrapper" uses, and it would be onerous to annotate every wrapper usage as coro_wrapper.

What did you envision the effect of adding coro_wrapper to a class would be on lifetime checks?

Can you walk me through your reasoning / expected usage?

I think it makes sense to add coro_wrapper to classes. The reasoning is, the behavior of coroutine is controlled by its promise_type. And generally in practice, we have the consistent promise_type for the same return type, which is the class I mentioned here. So given the coroutines have the same behavior, it should be helpful to add these attributes to the class. And of course, as I said, it is best to add this to the promise_type, but it is not conflicting.

[usx95]

Sorry for not responding earlier. This makes sense to me.

[usx95]

Please also add release notes.

[usx95]

How does this work atm ? I expect some changes to Sema::CheckCoroutineWrapper without which this should still be an error.

[usx95]

Please mention this change in the docs as well.

[ChuanqiXu9]

Sorry for not responding earlier. This makes sense to me.

Never mind.

And also @snarkmaster , if you're interested, given now I don't work on the safety checks for coroutines, feel free to take this if you want.

[usx95]

@snarkmaster

What did you envision the effect of adding coro_wrapper to a class would be on lifetime checks?

All coro wrappers continue to participate in lifetime checks (i.e., all params implicitly treated as lifetimebound) unless annotated with coro_disable_lifetimebound!
In Sema/CheckExprLifetime.cpp:

 if (const auto *RD = Callee->getReturnType()->getAsRecordDecl()) { CheckCoroCall = RD->hasAttr<CoroLifetimeBoundAttr>() && RD->hasAttr<CoroReturnTypeAttr>() && !Callee->hasAttr<CoroDisableLifetimeBoundAttr>(); }