Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[coroutine.handle] clarify whether it's legal to create handles for promise base classes #6039

Closed
jacobsa opened this issue Dec 26, 2022 · 6 comments
Labels
not-editorial Issue is not deemed editorial; the editorial issue is kept open for tracking.

Comments

@jacobsa
Copy link
Contributor

jacobsa commented Dec 26, 2022

Here is a hypothetical coroutine promise type, which has a base class that is responsible for storing a handle to the coroutine that should be resumed when the promise's coroutine finishes:

struct PromiseBase {
  // The handle to resume when done, also used for walking the linked list as
  // described below.
  std::coroutine_handle<PromiseBase> waiter;
};

template <typename Result>
struct Promise : PromiseBase {
  // Obtain a handle for this promise's coroutine, in the form required when
  // initializing PromiseBase::waiter. For example, this can be called when
  // co_awaiting a call to another coroutine with promise type Promise<T> for
  // some type T, at which point we have to fill in our own handle as the
  // waiter for the callee.
  std::coroutine_handle<PromiseBase> get_base_handle() {
    std::coroutine_handle<PromiseBase>::from_promise(*this);
  }
  
  // ... other promise machinery ...
};

The reason I need to do this, splitting the base class out and storing a handle with the base class as the promise type instead of using std::coroutine_handle<>, is to be able to walk the chain of waiters as a linked list. If Foo0 calls and waits on Foo1 … calls and waits on FooN, I need to be able to walk the chain of coroutines from FooN back to Foo0 (after synchronizing on them being suspended). This is useful for example when cleaning up after cancellation while suspended, or for implementing user-friendly async stack traces. I can't do this using std::coroutine_handle<Promise<T>>, because T may vary between the different functions.


My general question here is whether walking such a list is intended to be legal. I see the lack of clarity on this as a shortcoming in the standard's wording (or maybe I'm just missing something obvious). The code I've got above is the closest I can come up with to a UB-free way to implement it:

  • According to [coroutine.handle.con], the precondition for calling from_promise is only that the parameter be a reference to the promise object of a coroutine. That is certainly the case for the expression *this in Promise::get_base_handle, and presumably also the case even when that reference is expressed with a different type.

  • I see no mention anywhere in [coroutine.handle] of a requirement that the T in std::coroutine_handle<T> must be the promise type of a coroutine, rather than a base class of a promise type.

But that said, I can't see how std::coroutine_handle<T>::from_promise would be efficiently implementable without requiring something additional about T. For example if the actual promise type inherits from multiple classes including T, then the address of the promise is not necessarily the address of the T object within the promise. Therefore std::coroutine_handle<T>::from_promise can't statically know the offset of the T object within the coroutine frame if its input is of type T& (rather than being templated on the promise type).


To be clear, I would like this pattern to be legal in some (potentially restricted) form. I can't see how to implement things like good async stack traces without it. But I'm looking for clarification on whether it's intended to be legal, and perhaps the restrictions under which it is legal. Two ideas come to mind:

  1. std::coroutine_handle<T>::from_promise could be templated on the input reference type, with the restriction that T must be a base class of the referred-to type. I believe this would allow the implementation to calculate the correct offset for the coroutine frame, even in the face of multiple inheritance.

  2. The signature of std::coroutine_handle<T>::from_promise could be left the same, but the precondition tightened to require that the parameter must reference the promise of a coroutine where the promise has the same address as addressof(parameter). I'm not 100% sure this is guaranteed by the standard, but I believe this means the example above is legal because there is no multiple inheritance to cause their addresses to differ.

Apologies if I've got any facts incorrect here; mostly I'm looking for clarification from the experts.

(See also here for some previous discussion on Stack Overflow.)

@jacobsa
Copy link
Contributor Author

jacobsa commented Dec 26, 2022

  1. std::coroutine_handle<T>::from_promise could be templated on the input reference type, with the restriction that T must be a base class of the referred-to type. I believe this would allow the implementation to calculate the correct offset for the coroutine frame, even in the face of multiple inheritance.

I realized this idea wouldn't work (at least without the overhead of extra indirection): although it would allow you to calculate the address of the coroutine frame from the input reference, it wouldn't let you later synthesize a T& reference again when later calling coroutine_handle<T>::promise.

So I guess I'm asking for something like clarifying that from_promise can accept a reference to the base class of the promise, as long as it shares an address with the promise. (Or some other solution to storing a partially type-erased reference to the coroutine frame that still allows access to member variables of the promise in some form.)

@timsong-cpp
Copy link
Contributor

from_promise is not a template, so the parameter refers to the PromiseBase subobject of *this, and not the promise object iself. Since the PromiseBase object is not the promise object of any coroutine, it follows that the precondition is not met and the behavior is undefined.

Changing this would be evolutionary (and certainly not editorial).

@jensmaurer jensmaurer added the not-editorial Issue is not deemed editorial; the editorial issue is kept open for tracking. label Jan 2, 2023
@jensmaurer
Copy link
Member

I'm not seeing a defect here, editorial or otherwise.

This is a feature request, which needs a paper to be reviewed by (L)EWG.

@jacobsa
Copy link
Contributor Author

jacobsa commented Jan 2, 2023

Thanks, and sorry for choosing the wrong forum. I am new to the standards process; please be gentle.

Having thought about it more I agree it's currently UB. I think all I'm asking for is to change this line:

Preconditions: p is a reference to a promise object of a coroutine.

to something like the following (and similarly with this line):

Preconditions: p is a reference to an object that is pointer-interconvertible with the promise object of a coroutine.

From my limited knowledge of how coroutine_handle is implemented in practice this should be acceptable in terms of implementations, and it makes possible important functionality that exists in other languages like Rust that I can't see how to implement without UB otherwise.

What would be the best way to propose this change? Is there a step-by-step guide somewhere?

@jensmaurer
Copy link
Member

jensmaurer commented Jan 2, 2023

What would be the best way to propose this change? Is there a step-by-step guide somewhere?

Please read this:

https://isocpp.org/std/submit-a-proposal

It might be more efficient to convince Gor Nishanov, the original C++ coroutines author, of the missing feature here.

@jacobsa
Copy link
Contributor Author

jacobsa commented Jan 3, 2023

Thanks, that's exactly the link I was looking for. I've posted this std-proposals thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-editorial Issue is not deemed editorial; the editorial issue is kept open for tracking.
Projects
None yet
Development

No branches or pull requests

3 participants