Page MenuHomePhabricator

[coroutines] Add std::experimental::task<T> type
Needs ReviewPublic

Authored by lewissbaker on Apr 26 2018, 11:30 AM.

Details

Summary

Adds the coroutine std::experimental::task<T> type described in proposal P1056R0.
See https://wg21.link/P1056R0.

This implementation allows customization of the allocator used to allocate the
coroutine frame by passing std::allocator_arg as the first argument, followed by
the allocator to use.

This supports co_awaiting the same task multiple times. The second and
subsequent times it returns a reference to the already-computed value.

This diff also adds some implementations of other utilities that have potential for
standardization as helpers within the test/... area:

  • sync_wait(awaitable) - See P1171R0
  • manual_reset_event

Move the definition of the aligned_allocation_size helper function
from <experimental/memory_resource> to <experimental/
memory>
so it can be more widely used without pulling in memory_resource.

Outstanding work:

  • Use C++14 keywords directly rather than macro versions eg. use noexcept instead of _NOEXCEPT).
  • Add support for overaligned coroutine frames. This may need wording in the Coroutines TS to support passing the extra std::align_val_t.
  • Eliminate use of if constexpr if we want it to compile under C++14.

Diff Detail

Repository
rCXX libc++

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Make task<T> constructor from coroutine_handle private.
  • Run clang-format over <experimental/task>.
GorNishanov accepted this revision.EditedAug 29 2018, 1:02 PM

LGTM

With a few suggestions.

  1. Remove swap. It is not part of the proposal at the moment.
  2. If you feel like, add a lazy<T> alias to task<T>, that way we can taste both names and see how it feels.
  3. Also, there is still an outstanding work to add handling of implicit 'this'. Probably would be good to add before committing,

My apologies for taking so long to review.

This revision is now accepted and ready to land.Aug 29 2018, 1:02 PM
lewissbaker added inline comments.Sep 20 2018, 10:32 AM
test/std/experimental/task/sync_wait.hpp
37–38

The call to notify_all() needs to be inside the lock.
Otherwise, it's possible the waiting thread may see the write to __isSet_ inside wait() below and return, destroying the condition_variable before __cv_.notify_all() call completes.

lewissbaker edited the summary of this revision. (Show Details)

Fix race condition in __oneshot_event::set() method used by sync_wait().

lewissbaker retitled this revision from [coroutines] std::task type (WIP) to [coroutines] Add std::experimental::task<T> type.Mar 26 2019, 10:16 AM
lewissbaker edited the summary of this revision. (Show Details)
modocache closed this revision.Mar 26 2019, 10:49 AM

Committed in rL357010. Apologies, I forgot to include the differential revision in the commit message so this diff wasn't closed automatically as a result. I'll comment on rL357010 with the missing information.

lewissbaker reopened this revision.Mar 26 2019, 4:02 PM

Reopening as the commit for this diff was reverted due to it breaking the buildbot module builds.

This revision is now accepted and ready to land.Mar 26 2019, 4:02 PM

Added missing 'require coroutines' for experimental/task entry in module.modulemap
Added missing #include in experimental/task header that was failing module builds

lewissbaker marked an inline comment as done.Mar 26 2019, 11:06 PM

@EricWF This implementation will currently not work with MSVC as MSVC does not yet support the symmetric-transfer capability added to the coroutines specification in P0913R0.

Is MSVC a target that libc++ needs to support coroutines with?
Or can we just say the task<T> class is unsupported for compilers that don't support P0913?

I have added the 'requires coroutines' line to the task modulemap entry which should hopefully fix the module build failures this diff was seeing.
Are there any other potential gotchas that I should make sure I test?

test/std/experimental/task/task.basic/task_of_value.pass.cpp
26

@EricWF This line may fail to compile on older versions of clang without the coroutines bugfix for https://bugs.llvm.org/show_bug.cgi?id=37265.

Should I just change this back to std::move(p) here, since the intention is to test the library, not the compiler?
Alternatively I can conditionally compile this line as either co_return p; or co_return std::move(p); depending on the compiler version.

CaseyCarter requested changes to this revision.Mar 27 2019, 11:01 AM
CaseyCarter added a subscriber: mclow.lists.
CaseyCarter added inline comments.
include/experimental/__memory
80

This is missing preconditions that __a is a power of 2, and that __s <= -__a.

include/experimental/task
29

"requires a compiler with support for coroutines" would be more informative.

142

The return value of allocate isn't necessarily convertible to void*, it could be a fancy pointer. We should either static_assert(is_same_v<typename allocator_traits<_CharAlloc>::void_pointer, void*>, "Piss off with your fancy pointers"); or use pointer_traits here and in __deallocFunc to unfancy and re-fancy the pointer.

211

Pure style comment: I recommend using the non-elaborated friend __task_promise_final_awaitable; whenever possible. friend struct foo declares or redeclares struct foo in the enclosing namespace, whereas friend foo uses name lookup to find foo and makes it a friend. The latter form makes it far easier to analyze compiler errors when you screw something up in maintenance. (And on 480)

221

This mem-initializer for __state_ is redundant with the default member initializer on 306.

228

I suggest moving the case label and break outside the #ifndef here so the compiler won't warn about this case being unhandled when _LIBCPP_NO_EXCEPTIONS.

245

Are you certain that unhandled_exception can't possibly be called after storing a value? If so, this would leak the value.

255

Style: you've been using the _v variable templates for traits elsewhere.

262

Unnecessary std:: qualifier. (Occurs repeatedly.)

264

Style: the use of trailing-return-type SFINAE here is inconsistent with the use of template parameter SFINAE on 254.

309

These __empty_ members seem extraneous.

310

Should we static_assert that _Tp is a destructible object type?

374

Do we care about task<cv-void>?

390

Should these __foovalue_result members be foo-qualified to make them harder to misuse?

408

Similar to above, should we static_assert that _Tp is void, an lvalue reference type, or a destructible non-array object type?

454

this-> is extraneous in these classes that derive from the concrete _AwaiterBase. (And on 470)

459

Is missing a subject. How about "co_await on an invalid task<T> has undefined behavior"? (And on 475)

test/std/experimental/task/awaitable_traits.hpp
18

It's super sketchy for test code to inject things into the production namespace and to use reserved names. It's also inappropriate to use libc++ internals in a test in the std tree. (Occurs repeatedly.)

test/std/experimental/task/counted.hpp
82

IIUC that the tests all require C++17, you could declare these inline and avoid the external definitions / DEFINE_COUNTED_VARIABLES macro.

91

Should nextId_ be initialized to 1, as it is in reset on 40?

test/std/experimental/task/manual_reset_event.hpp
19

"currently". Also, "was not currently" is weird.

24

Here's a great example of why elaborated friend can be weird. This doesn't make the _Awaiter class defined on 25 a friend - which needn't be a friend, it's nested so it has full access to manual_reset_event - it declares a class named _Awaiter in the enclosing namespace that is a friend.

26

Ditto "using reserved names in test code". (Occurs repeatedly.)

41

_LIBCPP_ASSERT is inappropriate in std test code; it's undefined when the library under test is not libc++. Use LIBCPP_ASSERT (with no leading underscore) from test/support/test_macros.h to mean "this assertion must hold only if the library under test is libc++" and assert if the assertion must hold regardless of the tested library. (Occurs repeatedly.)

(Ignore the misuses of _LIBCPP_ASSERT that @mclow.lists put in the span tests; I'll fix them when we implement span and enable the tests.)

EDIT: Actually, I'll fix them now to save myself the trouble of figuring out why the tests are broken later.

48

Why are we concerned about data races on __event_->__state_, but not __event_->__awaitingCoroutine_?

50

s/this means that//; s/and so we/so we/; s/suspend - this/suspend. This/ (Ok, I'm nitpicking, but I actually did have issues parsing this comment.)

87

thread

89

"awaits the" what? The suspense is killing me!

93

std::exchange

103

What's the antecedent of "it"?

test/std/experimental/task/sync_wait.hpp
15

There's no such header in the working draft or the Coroutines TS.

37–38

The write to __isSet_ must be under the lock, the call to notify_all need not be. (Not that I care either way, just clarifying.)

71

Ditto "doesn't do what you think it does and is unnecessary."

116

Ditto "not in test code". You should include "test_macros.h" and use TEST_NO_EXCEPTIONS. (Occurs repeatedly.)

136

#include<new> for the definition of True Placement New. (This may be missing above as well, I don't recall or have time to scroll back.)

140

Here's another _VSTD that should be std. Please audit all test code.

test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp
19

This is not a Standard or Coroutines TS header.

27

static is extraneous in an anonymous namespace.

38

These are the defaults for size_type, difference_type, and is_always_equal; you should omit them to verify that the tested code properly uses allocator_traits.

55

This is almost certainly going to cause problems, since it leaves the moved-from allocator invalid. (And on 68.)

76

I suggest assert(allocator_instance_count > 0) before decrementing. We're testing code that explicitly destroys things, we should make sure it doesn't destroy more things than it constructs.

79

allocate needs to return T*. (Also, std::size_t.) (And on 88)

91

Here again, I suggest an assertion that we're not freeing more bytes than have been allocated.

222

In libc++ tests main is always declared int main(int, char**) (and explicitly returns 0) to support the freestanding work. (Occurs repeatedly.)

test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp
47

Again, leaving me in suspense.

This revision now requires changes to proceed.Mar 27 2019, 11:01 AM
lewissbaker marked 40 inline comments as done.Mar 29 2019, 4:56 PM

Thanks very much for the detailed review @CaseyCarter! Very much appreciated :)

include/experimental/__memory
80

Is there a recommended way of documenting/implementing these preconditions on a constexpr function in libc++?

The previous version that lived in <experimental/memory_resource> was not marked constexpr and so was able to use _LIBCPP_ASSERT.

include/experimental/task
29

This messaging was copied from <experimental/coroutine>.

I'll update the message there as well.

142

I'm not quite sure how to go about using pointer_traits here when interacting with coroutines and allocators.

Under C++20 I guess I would do something like:

typename allocator_traits<_CharAlloc>::void_pointer __pointer = __charAllocator.allocate(...);
...
return _VSTD::to_address(__pointer);

I assume that operator new() still needs to return void* rather than the fancy pointer so we need the to_address call in there. Is there some fallback for to_address() for unfancying the pointer in earlier standard versions?

Maybe we should just go with the static_assert() for now?

221

I'll remove the default member initializer on 306 then.

We can't = default the constructor here due to the union member containing types with non-trivial constructors/destructors.

245

Yes, I think there is a case where this could happen.

If we execute a co_return someValue which calls promise.return_value() and constructs __value_.
Then while executing the goto final_suspend; if any of the destructors of in-scope variables throw an exception which then escapes the coroutine body we could end up calling unhandled_exception() with __value_ already having been constructed.

Similarly, the exception thrown from a destructor could be caught and then a new co_return someOtherValue; executed.
So we should probably be guarding against __value_ containing a value inside return_value() as well.

309

The __empty_ members were added at your request ;)

See https://reviews.llvm.org/D46140?id=144168#inline-403822

I can remove them if you think they're not necessary.

310

Sure, will do.

I wonder if we should make this a requirement in the wording of P1056?
What do you think @GorNishanov?

374

I don't feel strongly either way.

It's not something I've ever had a use for, but maybe it should be done for completeness?

390

I'm not sure if it makes it much safer.
We already have 'rvalue' in the method name which gives a hint to its move-ness.

It's currently only called in one place:

return this->__coro_.promise().__rvalue_result();

would become:

return std::move(this->__coro_.promise()).__rvalue_result();
408

Sure, can do.

Further, to support task<T>::operator co_await() && T needs to both be destructible and move-constructible.
For task<T>::operator co_await() & T only needs to be destructible.

454

I get compile-errors if I remove the this->.

libcxx/include/experimental/task:502:16: error: 'std::experimental::coroutines_v1::task<counted>::__coro_' is not a member of class '_Awaiter'
        return __coro_.promise().__rvalue_result();

Compiler bug?

test/std/experimental/task/awaitable_traits.hpp
18

These were intended to be eventually added to the <experimental/coroutine> header and under the std::experimental namespace which is why they were using these macros. See P1288R0.

Similarly for sync_wait() which is proposed in P1171R0.

Is there some common namespace that test utilities should be declared within?
If not I'll just move them to global scope and de-uglify them for now.

test/std/experimental/task/counted.hpp
91

The intent was that any test would always initialise these values by calling reset() before running.
I hadn't intended the static-initialization to "do the right thing" here but I can if you think that's preferable.

test/std/experimental/task/manual_reset_event.hpp
48

Only the awaiting thread writes to __awaitingCoroutine_ and publishes this value when it writes to __state_ by setting the state to __not_set_waiting_coroutine.

A thread that calls .set() will only read from __awaitingCoroutine_ if, when it updates the __state_ variable, sees that the previous state was __not_set_waiting_coroutine.

So the write the __awaitingCoroutine_ should strictly happens-before any read from __awaitingCoroutine_.

test/std/experimental/task/sync_wait.hpp
37–38

The problem here is that once we write true to __isSet_ and release the lock that the thread calling wait() can potentially return and the on to destroy the __oneshot_event object, leaving the current thread with a dangling reference to a destroyed std::condition_variable.

I ran into a similar problem in cppcoro.
See https://github.com/lewissbaker/cppcoro/issues/98#issuecomment-450695759

test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp
19

I can't #include <memory_resource> as libc++ doesn't currently provide that header.

How should I go about writing a test that makes sure that std::experimental::task works with std::experimental::pmr::polymorphic_allocator (or std::pmr::polymorphic_allocator if available)?

55

Isn't a moved-from allocator left in a valid but unspecified state?
ie. you can't assume it's ok to use. but should be ok to destruct or assign to.

lewissbaker marked 6 inline comments as done.

This updated diff should address most of @CaseyCarter's review comments.

For detailed changelog you can find individual changes here https://github.com/lewissbaker/libcxx/commits/coro_task

There are a few outstanding issues that still need some clarification on how to proceed:

  • Should the task tests be taking a dependency on <experimental/memory_resource>?
  • How should I implement a precondition check for __aligned_allocation_size when it is declared constexpr?
  • I've not yet implemented support for task<cv-void>. Is this something we should support?
  • I've not yet handling allocators with fancy pointer types. If we are to support them is there a standard way to convert a fancy pointer to a void* to return from promise_type::operator new()? Does it even make sense to support them if the compiler is potentially going to be storing raw pointers to things inside the coroutine frame?
lewissbaker marked 3 inline comments as done.Apr 3 2019, 12:45 PM
lewissbaker added inline comments.
include/experimental/task
160

This should use explicit move construction rather than implicit move construction.

lewissbaker marked an inline comment as done.Apr 8 2019, 8:45 PM

Gentle ping.

Is there anything else people would like to see changed?