Ok. After some thought, I think it is possible to have an enforcement policy that is more relaxed than the core guidelines. The root cause of this problem is that the lambda object is a lifetime-bound argument of the lambda call operator. So I think this can be solved easily.
Fix: [coroutines] Detect lifetime issues with coroutine lambda captures by usx95 · Pull Request #77066 · llvm/llvm-project · GitHub
@niekbouman
Update: Now coroutines lambda objects are also part of the analysis.
If it is valuable, I can try to surface this guideline as a clang warning instead of a tidy check.
Well, for us that would in fact not be valuable, because we do not (and neither the Seastar/ScyllaDB developers) adhere to the core-guideline advice on this point.
Namely, we mostly use coroutine lambdas (with captures) in cases where the coroutine is immediately awaited in the same full expression. Then, the temporary lambda object lives long enough.
To interact with older code (i.e. providing a coroutine lambda object) to a legacy function that takes a function object with future return type, we use the lambda wrapper described in: seastar/include/seastar/core/coroutine.hh at master · scylladb/seastar · GitHub
For other use cases, we use the following wrapper (which has allocation overhead) that passes the lambda by value to the wrapper type (which is iself a coroutine), so that the lambda is stored in the wrapper’s coroutine frame, and looks like:
namespace internal {
template <typename T, typename... Args>
inline T lifetime_wrapper(auto coroutine_lambda, Args&&... args)
{
co_return co_await coroutine_lambda(std::forward<Args>(args)...);
}
} // namespace internal
//! Helper for managing lifetime of a coroutine object
/*!
* This helper keeps the coroutine-lambda object (and with that, the captures) alive throughout the lifetime of its
* coroutine member function
*
* Without this wrapper, the lambda captures would be destructed after the first coroutine suspend, which easily leads
* to use-after-free bugs.
*/
template <typename Functor>
class keep_captures_alive {
Functor lambda_;
public:
explicit keep_captures_alive(Functor lambda) : lambda_(std::move(lambda))
{
}
template<typename... Args>
auto operator()(Args&&... args) -> std::invoke_result_t<Functor, Args...>
{
return internal::lifetime_wrapper<std::invoke_result_t<Functor, Args...>>(std::move(lambda_), std::forward<Args>(args)...);
}
template<typename... Args>
auto operator()(Args&&... args) const -> std::invoke_result_t<Functor, Args...>
{
return internal::lifetime_wrapper<std::invoke_result_t<Functor, Args...>>( Functor {lambda_}, std::forward<Args>(args)...);
}
};
That is very cool!
Unfortunately, I came across another issue when using your checker in practice, namely the virality of the coro_wrapper requirement. For example, in the following link to a code snippet in Compiler Explorer,
https://godbolt.org/z/eWjTofdPh
I use std::invoke in my code to invoke a (coroutine) lambda. Then, your checker complains that std::invoke is not marked as a coro_wrapper. But I think that level of virality is undesirable and greatly limits the usability of the checker. Even if you would add such annotation to std::invoke in libc++ (and all other generic stdlib functions that could possibly return some coroutine type, which would most probably not be a good idea ;-), I’d run into issues if I use GCC’s standard library…
If I understand correctly, the virality is an intrinsic part of the current design, or are there ways to get around this issue?
Your understanding is correct. This is part of the design. The intention is not to annotate such instances in the standard library but, on the contrary, to ban and discourage such uses.
For example, the use of coroutine with std::function or std::bind is core to the design since it is problematic because the coro frame can capture references to local variables of these forwarders. See example.
The solution to using such constructs is to develop our own coroutine-aware wrappers and invokers.