[PATCH] D108696: [Coroutines] [Frontend] Lookup in std namespace first

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker added a comment.

So am I correct in understanding that the main issue with the chicken/egg 
problem for updating both the compiler to use the new stdlib facilities and 
updating the stdlib facilities is that we don't want to issue warnings about 
using `` and telling users to use `` instead 
if there is no `` header.

I wonder if a suitable transition approach here might be to have the compiler 
do:

- try lookup of std::coroutine_handle/coroutine_traits; if found try to 
instantiate the expression (e.g. await-suspend) using the std:: type
- if not found then try lookup of 
std::experimental::coroutine_handle/coroutine_traits; and if found try to 
instantiate the expression using the std::experimental:: type; and if 
successful, then;
  - if std:: type was found but instantiating the expression failed, then issue 
a warning about using deprecated std::experimental:: types
  - otherwise if std:: type was not found, try to find  header and 
if present then issue a warning suggesting using  instead of 


Then you should be able to land the compiler change and it should continue to 
have the same behaviour for existing code using std::experimental:: types with 
the existing stdlib.
It is only once the stdlib changes that land which add the  header 
that users would start seeing the warnings about using  instead of 


Note that there are still some potential breakages for code that could go on 
here, however.

Imagine that some header I included has been updated to use  while 
my current header is still using  so that both are in 
scope.

I might have defined an awaitable type that looks like the following:

  c++
  #include  // which transitively includes 
  #include 
  
  struct my_awaitable {
bool await_ready();
void await_suspend(std::experimental::coroutine_handle<> coro);
void await_resume();
  };

In this case, the compiler would find the `std::coroutine_handle<>` and try to 
instantiate the await-suspend expression with that type, which would fail 
because `await_suspend()` is not callable with a `std::coroutine_handle`.
The compiler would need to fall back to instantiating the expression with 
`std::experimental::coroutine_handle`.

There is also the variation where await_suspend() is a template function that 
deduces the promise-type argument.
e.g.

  c++
  template
  void await_suspend(std::experimental::coroutine_handle coro);

There are also cases where the template parameter to await_suspend() is 
unconstrained.
e.g.

  c++
  struct promise_type {
...
std::experimental::coroutine_handle<> continuation;
  };
  
  struct my_awaitable {
bool await_ready();
template
auto await_suspend(CoroHandle handle) {
  coro.promise().continuation = handle;
  return coro;
}
void await_resume();
  
std::experimental::coroutine_handle coro;
  };

Such a type would successfully deduce the template parameter using 
std::coroutine_handle and so the await-suspend expression would be valid, but 
the instantiation of the await_suspend() body would fail as a 
std::coroutine_handle cannot be assigned to an lvalue of type 
`std::experimentl::coroutine_handle<>`. This would not produce a 
SFINAE-friendly error that the compiler could retry with std::experimental 
types.

I'm not sure if there's a great way of keeping such code working in a mixed 
`` and `` world.
Maybe the cost of doing so is not worth the benefit?

The only way I can think of making this work is to just make 
`std::experimental::*` an alias for `std::*`.
But that only works for `std::experimental::coroutine_handle`. It doesn't work 
for `std::experimental::coroutine_traits` as you can't add specialisations 
through an alias.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108696/new/

https://reviews.llvm.org/D108696

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:4450
+Value *Extra = Builder.CreateSelect(Cmp, Diff, Zero);
+return RValue::get(Builder.CreateAdd(SizeCall, Extra));
   }

rjmccall wrote:
> lewissbaker wrote:
> > ychen wrote:
> > > rjmccall wrote:
> > > > ychen wrote:
> > > > > ychen wrote:
> > > > > > rjmccall wrote:
> > > > > > > Okay, so you're implicitly increasing the coroutine size to allow 
> > > > > > > you to round up to get an aligned frame.  But how do you round 
> > > > > > > back down to get the actual pointer that you need to delete?  
> > > > > > > This just doesn't work.
> > > > > > > 
> > > > > > > You really ought to just be using the aligned `operator new` 
> > > > > > > instead when the required alignment is too high.  If that means 
> > > > > > > checking the alignment "dynamically" before calling `operator 
> > > > > > > new` / `operator delete`, so be it.  In practice, it will not be 
> > > > > > > dynamic because lowering will replace the `coro.align` call with 
> > > > > > > a constant, at which point the branch will be foldable.
> > > > > > > 
> > > > > > > I don't know what to suggest if the aligned `operator new` isn't 
> > > > > > > reliably available on the target OS.  You could outline a 
> > > > > > > function to pick the best allocator/deallocator, I suppose.
> > > > > > Thanks for the review!
> > > > > > 
> > > > > > > Okay, so you're implicitly increasing the coroutine size to allow 
> > > > > > > you to round up to get an aligned frame. But how do you round 
> > > > > > > back down to get the actual pointer that you need to delete? This 
> > > > > > > just doesn't work.
> > > > > > 
> > > > > > Hmm, you're right that I missed the `delete` part, thanks. The 
> > > > > > adjusted amount is constant, I could just dial it down in 
> > > > > > `coro.free`, right?
> > > > > > 
> > > > > > >  You really ought to just be using the aligned operator new 
> > > > > > > instead when the required alignment is too high. If that means 
> > > > > > > checking the alignment "dynamically" before calling operator new 
> > > > > > > / operator delete, so be it. In practice, it will not be dynamic 
> > > > > > > because lowering will replace the coro.align call with a 
> > > > > > > constant, at which point the branch will be foldable.
> > > > > >  
> > > > > > That's my intuition at first. But spec is written in a way 
> > > > > > suggesting (IMHO) that the aligned version should not be used? What 
> > > > > > if the user specify their own allocator, then which one they should 
> > > > > > use?
> > > > > Sorry, I meant the adjusted amount is coro.align - std::max_align_t,  
> > > > > I could subtract it in `coro.free` . I think it should work?
> > > > No, because the adjustment you have to do in `coro.alloc` isn't just an 
> > > > addition, it's an addition plus a mask, which isn't reversible.  
> > > > Suppose the frame needs to be 32-byte-aligned, but the allocator only 
> > > > promises 8-byte alignment.  The problem is that when you go to free a 
> > > > frame pointer, and you see that it's 32-byte-aligned (which, again, it 
> > > > always will be), the pointer you got from the allocator might be the 
> > > > frame pointer minus any of 8, 16, or 24 — or it might be exactly the 
> > > > same.  The only way to reverse that is to store some sort of cookie, 
> > > > either the amount to subtract or even just the original pointer.
> > > > 
> > > > Now, if you could change the entire coroutine ABI, you could make the 
> > > > frame handle that you pass around be the unadjusted pointer and then 
> > > > just repeat the adjustment every time you enter the coroutine.  But 
> > > > that doesn't work because the ABI relies on things like the promise 
> > > > being at a reliable offset from the frame handle.
> > > > 
> > > > I think the best solution would be to figure out a way to use an 
> > > > aligned allocator, which at worst does this in a more systematic way 
> > > > and at best can actually just satisfy your requirement directly without 
> > > > any overhead.  If you can't do that, adding an offset to the frame 
> > > > would be best; if you can't do that, doing it as a cookie is okay.
> > > > 
> > > > > That's my intuition at first. But spec is written in a way suggesting 
> > > > > (IMHO) that the aligned version should not be used? What if the user 
> > > > > specify their own allocator, then which one they should use?
> > > > 
> > > > It seems like a spec bug that this doesn't use aligned allocators even 
> > > > when they're available.  If there's an aligned allocator available, I 
> > > > think this should essentially do dynamically what it would normally do 
> > > > statically, i.e.:
> > > > 
> > > > ```
> > > > void *allocation = alignment > __STDCPP_DEFAULT_NEW_ALIGNMENT__ ? 
> > > > operator new(size, align_val_t(alignment)) : operator new(size);
> > > > ```
> > > > 
> > > > This would ODR-use both 

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:4450
+Value *Extra = Builder.CreateSelect(Cmp, Diff, Zero);
+return RValue::get(Builder.CreateAdd(SizeCall, Extra));
   }

ychen wrote:
> rjmccall wrote:
> > ychen wrote:
> > > ychen wrote:
> > > > rjmccall wrote:
> > > > > Okay, so you're implicitly increasing the coroutine size to allow you 
> > > > > to round up to get an aligned frame.  But how do you round back down 
> > > > > to get the actual pointer that you need to delete?  This just doesn't 
> > > > > work.
> > > > > 
> > > > > You really ought to just be using the aligned `operator new` instead 
> > > > > when the required alignment is too high.  If that means checking the 
> > > > > alignment "dynamically" before calling `operator new` / `operator 
> > > > > delete`, so be it.  In practice, it will not be dynamic because 
> > > > > lowering will replace the `coro.align` call with a constant, at which 
> > > > > point the branch will be foldable.
> > > > > 
> > > > > I don't know what to suggest if the aligned `operator new` isn't 
> > > > > reliably available on the target OS.  You could outline a function to 
> > > > > pick the best allocator/deallocator, I suppose.
> > > > Thanks for the review!
> > > > 
> > > > > Okay, so you're implicitly increasing the coroutine size to allow you 
> > > > > to round up to get an aligned frame. But how do you round back down 
> > > > > to get the actual pointer that you need to delete? This just doesn't 
> > > > > work.
> > > > 
> > > > Hmm, you're right that I missed the `delete` part, thanks. The adjusted 
> > > > amount is constant, I could just dial it down in `coro.free`, right?
> > > > 
> > > > >  You really ought to just be using the aligned operator new instead 
> > > > > when the required alignment is too high. If that means checking the 
> > > > > alignment "dynamically" before calling operator new / operator 
> > > > > delete, so be it. In practice, it will not be dynamic because 
> > > > > lowering will replace the coro.align call with a constant, at which 
> > > > > point the branch will be foldable.
> > > >  
> > > > That's my intuition at first. But spec is written in a way suggesting 
> > > > (IMHO) that the aligned version should not be used? What if the user 
> > > > specify their own allocator, then which one they should use?
> > > Sorry, I meant the adjusted amount is coro.align - std::max_align_t,  I 
> > > could subtract it in `coro.free` . I think it should work?
> > No, because the adjustment you have to do in `coro.alloc` isn't just an 
> > addition, it's an addition plus a mask, which isn't reversible.  Suppose 
> > the frame needs to be 32-byte-aligned, but the allocator only promises 
> > 8-byte alignment.  The problem is that when you go to free a frame pointer, 
> > and you see that it's 32-byte-aligned (which, again, it always will be), 
> > the pointer you got from the allocator might be the frame pointer minus any 
> > of 8, 16, or 24 — or it might be exactly the same.  The only way to reverse 
> > that is to store some sort of cookie, either the amount to subtract or even 
> > just the original pointer.
> > 
> > Now, if you could change the entire coroutine ABI, you could make the frame 
> > handle that you pass around be the unadjusted pointer and then just repeat 
> > the adjustment every time you enter the coroutine.  But that doesn't work 
> > because the ABI relies on things like the promise being at a reliable 
> > offset from the frame handle.
> > 
> > I think the best solution would be to figure out a way to use an aligned 
> > allocator, which at worst does this in a more systematic way and at best 
> > can actually just satisfy your requirement directly without any overhead.  
> > If you can't do that, adding an offset to the frame would be best; if you 
> > can't do that, doing it as a cookie is okay.
> > 
> > > That's my intuition at first. But spec is written in a way suggesting 
> > > (IMHO) that the aligned version should not be used? What if the user 
> > > specify their own allocator, then which one they should use?
> > 
> > It seems like a spec bug that this doesn't use aligned allocators even when 
> > they're available.  If there's an aligned allocator available, I think this 
> > should essentially do dynamically what it would normally do statically, 
> > i.e.:
> > 
> > ```
> > void *allocation = alignment > __STDCPP_DEFAULT_NEW_ALIGNMENT__ ? operator 
> > new(size, align_val_t(alignment)) : operator new(size);
> > ```
> > 
> > This would ODR-use both allocation functions, of course.
> > 
> > Maybe it's right to do this cookie thing if we can't rely on an aligned 
> > allocator, like if the promise class provides only an `operator 
> > new(size_t)`.
> > No, because the adjustment you have to do in `coro.alloc` isn't just an 
> > addition, it's an addition plus a mask, which isn't reversible.  Suppose 
> > the frame needs to be 

[PATCH] D82314: [RFC][Coroutines] Optimize the lifespan of temporary co_await object

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker added a comment.

>   There are still room for improvements, in particular, GCC has a 4K frame 
> for this function.

I think GCC having a smaller coroutine frame is probably because it does not 
yet implement the allocation-elision optimisation which inlines the nested 
coroutine frame (which is 4K) into the parent coroutine frame.
I have not looked yet, but I suspect that you'll see within the 
get_big_object2() code it will perform another heap allocation for the 
get_big_object() frame.

Until we have more general support for async RVO, I think 8K is probably as 
good as we're going to be able to get (4K for this coroutine's promise and 4K 
for child coroutine-frame and its promise).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82314/new/

https://reviews.llvm.org/D82314



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82415: [Coroutines] Special handle __builtin_coro_resume for final_suspend nothrow check

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker accepted this revision.
lewissbaker added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: clang/lib/Sema/SemaCoroutine.cpp:621
+// returns a handle. In that case, even __builtin_coro_resume is not
+// declared as noexcept, we claim that logically it does not throw.
+if (FD->getBuiltinID() == Builtin::BI__builtin_coro_resume)

... it does not throw _into_ the coroutine that just suspended, but rather 
throws back out from whoever called coroutine_handle::resume().


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82415/new/

https://reviews.llvm.org/D82415



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82029: [Coroutines] Ensure co_await promise.final_suspend() does not throw

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10521
+def err_coroutine_promise_final_suspend_requires_nothrow : Error<
+  "all code generated by co_await __promise.final_suspend() must not throw"
+>;

I'd perhaps word this to be more direct in what the compiler is requiring.
It's possible to have a function that does not throw that is not declared 
noexcept, but what the compiler requires here is that the expression is 
noexcept - or in standardese "not potentially throwing".

So maybe something like:
> the expression 'co_await __promise.final_suspend()' is required to be 
> non-throwing




Comment at: clang/lib/Sema/SemaCoroutine.cpp:662-664
+  for (const auto *D : ThrowingDecls) {
+S.Diag(D->getEndLoc(), diag::note_coroutine_function_declare_noexcept);
+  }

Should we be sorting the ThrowingDecls by something other than the pointer so 
that the error output has a deterministic order here?



Comment at: clang/lib/Sema/SemaExceptionSpec.cpp:1049
 
-  FT = S.ResolveExceptionSpec(Loc.isInvalid() ? E->getBeginLoc() : Loc, FT);
+  if (Loc.isValid() || (Loc.isInvalid() && E)) {
+FT = S.ResolveExceptionSpec(Loc.isInvalid() ? E->getBeginLoc() : Loc, FT);

The function documentation says at least one of E and Loc must be true.
Should this be an assertion, then, rather than a conditional check?



Comment at: clang/test/AST/Inputs/std-coroutine.h:13
 struct coroutine_handle {
-  static coroutine_handle from_address(void *);
+  static coroutine_handle from_address(void *) noexcept;
 };

Note that the actual `std::coroutine_handle::from_address()` method is not 
specified to have a noexcept declaration, even though it is defined as such in 
both libstdc++/libc++ implementation.

See http://eel.is/c++draft/coroutine.handle#export.import-itemdecl:2

Note, however, that the specification doesn't define a co_await expression in 
terms of coroutine_handle::from_address() or coroutine_handle::from_promise(), 
but is rather just defined in terms of some handle 'h' that refers to the 
current coroutine.

See http://eel.is/c++draft/expr.await#3.5

So while technically we shouldn't be requiring the from_address() method to be 
noexcept, I think that it's probably ok since the compiler, at least in theory, 
has control over how it constructs a handle and can choose to call the 
from_address() method assuming that it is defined noexcept, even though it is 
not required to be.




Comment at: clang/test/SemaCXX/coroutine-final-suspend-noexcept.cpp:14
+struct coroutine_handle {
+  static coroutine_handle from_address(void *); // expected-note {{must be 
declared with 'noexcept'}}
+};

I'm not sure that we should be _requiring_ the compiler to emit an error for 
this line.

The language specification does not require implementations to declare the 
from_address() method as noexcept, even though Clang now requires standard 
library implementations to declare this method as noexcept - this is an 
additional implementation requirement that Clang is placing on standard library 
implementations for them to be compatible with Clang's coroutines 
implementation.

I guess this is probably ok to raise as an error, though, as most users will 
just be using the compiler-provided implementation and both libc++/libstdc++ 
are (currently) compatible.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82029/new/

https://reviews.llvm.org/D82029



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46140: [coroutines] Add std::experimental::task type

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker marked an inline comment as done.
lewissbaker added a comment.

Gentle ping.

Is there anything else people would like to see changed?


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D46140/new/

https://reviews.llvm.org/D46140



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46140: [coroutines] Add std::experimental::task type

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker marked 3 inline comments as done.
lewissbaker added inline comments.



Comment at: include/experimental/task:160
+__get_allocator<_CharAlloc>(__pointer, __size);
+_CharAlloc __allocatorOnStack = _VSTD::move(__allocatorInFrame);
+__allocatorInFrame.~_CharAlloc();

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


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D46140/new/

https://reviews.llvm.org/D46140



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46140: [coroutines] Add std::experimental::task type

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker updated this revision to Diff 192944.
lewissbaker marked 6 inline comments as done.
lewissbaker added a comment.

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 
?
- How should I implement a precondition check for `__aligned_allocation_size` 
when it is declared constexpr?
- I've not yet implemented support for `task`. 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?


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D46140/new/

https://reviews.llvm.org/D46140

Files:
  include/CMakeLists.txt
  include/experimental/__memory
  include/experimental/coroutine
  include/experimental/memory_resource
  include/experimental/task
  include/module.modulemap
  test/std/experimental/task/awaitable_traits.hpp
  test/std/experimental/task/counted.hpp
  test/std/experimental/task/lit.local.cfg
  test/std/experimental/task/manual_reset_event.hpp
  test/std/experimental/task/sync_wait.hpp
  test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp
  test/std/experimental/task/task.basic/task_of_value.pass.cpp
  test/std/experimental/task/task.basic/task_of_void.pass.cpp
  test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp
  test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp

Index: test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp
===
--- /dev/null
+++ test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp
@@ -0,0 +1,157 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+
+#include 
+#include 
+#include 
+
+#include "../counted.hpp"
+#include "../sync_wait.hpp"
+
+void test_return_value_lifetime()
+{
+  counted::reset();
+
+  auto f = [](bool x) -> std::experimental::task
+  {
+if (x) {
+  counted c;
+  co_return std::move(c);
+}
+co_return {};
+  };
+
+  {
+auto t = f(true);
+
+assert(counted::active_instance_count() == 0);
+assert(counted::copy_constructor_count() == 0);
+assert(counted::move_constructor_count() == 0);
+
+{
+  auto c = ::sync_wait(std::move(t));
+  assert(c.id() == 1);
+
+  assert(counted::active_instance_count() == 2);
+  assert(counted::copy_constructor_count() == 0);
+  assert(counted::move_constructor_count() > 0);
+  assert(counted::default_constructor_count() == 1);
+}
+
+// The result value in 't' is still alive until 't' destructs.
+assert(counted::active_instance_count() == 1);
+  }
+
+  assert(counted::active_instance_count() == 0);
+
+  counted::reset();
+
+  {
+auto t = f(false);
+
+assert(counted::active_instance_count() == 0);
+assert(counted::copy_constructor_count() == 0);
+assert(counted::move_constructor_count() == 0);
+
+{
+  auto c = ::sync_wait(std::move(t));
+  assert(c.id() == 1);
+
+  assert(counted::active_instance_count() == 2);
+  assert(counted::copy_constructor_count() == 0);
+  assert(counted::move_constructor_count() > 0);
+  assert(counted::default_constructor_count() == 1);
+}
+
+// The result value in 't' is still alive until 't' destructs.
+assert(counted::active_instance_count() == 1);
+  }
+}
+
+struct my_error {};
+
+struct throws_on_destruction
+{
+  ~throws_on_destruction() noexcept(false)
+  {
+throw my_error{};
+  }
+};
+
+void test_uncaught_exception_thrown_after_co_return()
+{
+  counted::reset();
+
+  assert(counted::active_instance_count() == 0);
+  assert(counted::copy_constructor_count() == 0);
+  assert(counted::move_constructor_count() == 0);
+
+  {
+auto t = []() -> std::experimental::task
+{
+  throws_on_destruction d;
+  co_return counted{};
+}();
+
+try {
+  (void)::sync_wait(std::move(t));
+  assert(false);
+} catch (const my_error&) {
+}
+
+assert(counted::active_instance_count() == 0);
+assert(counted::copy_constructor_count() == 0);
+

[PATCH] D46140: [coroutines] Add std::experimental::task type

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker marked 40 inline comments as done.
lewissbaker added a comment.

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




Comment at: include/experimental/__memory:80
+{
+return (__s + __a - 1) & ~(__a - 1);
+}

CaseyCarter wrote:
> This is missing preconditions that `__a` is a power of 2, and that `__s <=  
> -__a`.
Is there a recommended way of documenting/implementing these preconditions on a 
constexpr function in libc++?

The previous version that lived in `` was not 
marked constexpr and so was able to use `_LIBCPP_ASSERT`.



Comment at: include/experimental/task:28
+#if defined(_LIBCPP_WARNING)
+_LIBCPP_WARNING(" cannot be used with this compiler")
+#else

CaseyCarter wrote:
> "requires a compiler with support for coroutines" would be more informative.
This messaging was copied from .

I'll update the message there as well.



Comment at: include/experimental/task:141
+
+void* __pointer = __charAllocator.allocate(
+__get_padded_frame_size_with_allocator<_CharAlloc>(__size));

CaseyCarter wrote:
> 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 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.
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?




Comment at: include/experimental/task:220
+public:
+  __task_promise() _NOEXCEPT : __state_(_State::__no_value) {}
+

CaseyCarter wrote:
> This mem-initializer for `__state_` is redundant with the default member 
> initializer on 306.
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.



Comment at: include/experimental/task:244
+exception_ptr(current_exception());
+__state_ = _State::__exception;
+#else

CaseyCarter wrote:
> Are you certain that `unhandled_exception` can't possibly be called after 
> storing a value? If so, this would leak the value.
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.



Comment at: include/experimental/task:308
+  union {
+char __empty_;
+_Tp __value_;

CaseyCarter wrote:
> These `__empty_` members seem extraneous. 
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.



Comment at: include/experimental/task:309
+char __empty_;
+_Tp __value_;
+exception_ptr __exception_;

CaseyCarter wrote:
> Should we `static_assert` that `_Tp` is a destructible object type?
Sure, will do.

I wonder if we should make this a requirement in the wording of 
[P1056](https://wg21.link/P1056)?
What do you think @GorNishanov?



Comment at: include/experimental/task:373
+template <>
+class __task_promise final : public __task_promise_base {
+  using _Handle = coroutine_handle<__task_promise>;

CaseyCarter wrote:
> Do we care about `task`?
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?



Comment at: include/experimental/task:389
+
+  void __rvalue_result() { __throw_if_exception(); }
+

CaseyCarter wrote:
> Should these `__foovalue_result` members be `foo`-qualified to make them 
> harder to misuse?
I'm not sure if it makes it much safer.
We already have 'rvalue' in the method name which gives a hint to its 

[PATCH] D46140: [coroutines] Add std::experimental::task type

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker marked an inline comment as done.
lewissbaker added a comment.

@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` 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?




Comment at: test/std/experimental/task/task.basic/task_of_value.pass.cpp:25
+   auto p = std::make_unique(123);
+   co_return p; // Should be implicit std::move(p) here.
+ }

@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.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D46140/new/

https://reviews.llvm.org/D46140



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46140: [coroutines] Add std::experimental::task type

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker updated this revision to Diff 192384.
lewissbaker added a comment.

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


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D46140/new/

https://reviews.llvm.org/D46140

Files:
  include/CMakeLists.txt
  include/experimental/__memory
  include/experimental/memory_resource
  include/experimental/task
  include/module.modulemap
  test/std/experimental/task/awaitable_traits.hpp
  test/std/experimental/task/counted.hpp
  test/std/experimental/task/lit.local.cfg
  test/std/experimental/task/manual_reset_event.hpp
  test/std/experimental/task/sync_wait.hpp
  test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp
  test/std/experimental/task/task.basic/task_of_value.pass.cpp
  test/std/experimental/task/task.basic/task_of_void.pass.cpp
  test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp
  test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp

Index: test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp
===
--- /dev/null
+++ test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp
@@ -0,0 +1,86 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+
+#include 
+#include 
+#include 
+
+#include "../counted.hpp"
+#include "../sync_wait.hpp"
+
+DEFINE_COUNTED_VARIABLES();
+
+void test_return_value_lifetime()
+{
+  counted::reset();
+
+  auto f = [](bool x) -> std::experimental::task
+  {
+if (x) {
+  counted c;
+  co_return std::move(c);
+}
+co_return {};
+  };
+
+  {
+auto t = f(true);
+
+assert(counted::active_instance_count() == 0);
+assert(counted::copy_constructor_count() == 0);
+assert(counted::move_constructor_count() == 0);
+
+{
+  auto c = sync_wait(std::move(t));
+  assert(c.id() == 1);
+
+  assert(counted::active_instance_count() == 2);
+  assert(counted::copy_constructor_count() == 0);
+  assert(counted::move_constructor_count() > 0);
+  assert(counted::default_constructor_count() == 1);
+}
+
+// The result value in 't' is still alive until 't' destructs.
+assert(counted::active_instance_count() == 1);
+  }
+
+  assert(counted::active_instance_count() == 0);
+
+  counted::reset();
+
+  {
+auto t = f(false);
+
+assert(counted::active_instance_count() == 0);
+assert(counted::copy_constructor_count() == 0);
+assert(counted::move_constructor_count() == 0);
+
+{
+  auto c = sync_wait(std::move(t));
+  assert(c.id() == 1);
+
+  assert(counted::active_instance_count() == 2);
+  assert(counted::copy_constructor_count() == 0);
+  assert(counted::move_constructor_count() > 0);
+  assert(counted::default_constructor_count() == 1);
+}
+
+// The result value in 't' is still alive until 't' destructs.
+assert(counted::active_instance_count() == 1);
+  }
+}
+
+int main()
+{
+  test_return_value_lifetime();
+  return 0;
+}
Index: test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp
===
--- /dev/null
+++ test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp
@@ -0,0 +1,57 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+
+#include 
+#include 
+
+#include "../counted.hpp"
+#include "../sync_wait.hpp"
+
+DEFINE_COUNTED_VARIABLES();
+
+void test_parameter_lifetime()
+{
+  counted::reset();
+
+  auto f = [](counted c) -> std::experimental::task
+  {
+co_return c.id();
+  };
+
+  {
+auto t = f({});
+
+assert(counted::active_instance_count() == 1);
+assert(counted::copy_constructor_count() == 0);
+assert(counted::move_constructor_count() <= 2); // Ideally <= 1
+
+auto id = sync_wait(t);
+assert(id == 1);
+
+assert(counted::active_instance_count() == 1);
+assert(counted::copy_constructor_count() == 0);
+
+// We are relying on C++17 copy-elision when passing the temporary counter
+// into f(). Then f() must move the parameter into the coroutine 

[PATCH] D46140: [coroutines] Add std::experimental::task type

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker reopened this revision.
lewissbaker added a comment.
This revision is now accepted and ready to land.

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


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D46140/new/

https://reviews.llvm.org/D46140



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46140: [coroutines] std::task type (WIP)

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker updated this revision to Diff 192208.
lewissbaker added a comment.

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


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D46140/new/

https://reviews.llvm.org/D46140

Files:
  include/CMakeLists.txt
  include/experimental/__memory
  include/experimental/memory_resource
  include/experimental/task
  include/module.modulemap
  test/std/experimental/task/awaitable_traits.hpp
  test/std/experimental/task/counted.hpp
  test/std/experimental/task/lit.local.cfg
  test/std/experimental/task/manual_reset_event.hpp
  test/std/experimental/task/sync_wait.hpp
  test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp
  test/std/experimental/task/task.basic/task_of_value.pass.cpp
  test/std/experimental/task/task.basic/task_of_void.pass.cpp
  test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp
  test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp

Index: test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp
===
--- /dev/null
+++ test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp
@@ -0,0 +1,86 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+
+#include 
+#include 
+#include 
+
+#include "../counted.hpp"
+#include "../sync_wait.hpp"
+
+DEFINE_COUNTED_VARIABLES();
+
+void test_return_value_lifetime()
+{
+  counted::reset();
+
+  auto f = [](bool x) -> std::experimental::task
+  {
+if (x) {
+  counted c;
+  co_return std::move(c);
+}
+co_return {};
+  };
+
+  {
+auto t = f(true);
+
+assert(counted::active_instance_count() == 0);
+assert(counted::copy_constructor_count() == 0);
+assert(counted::move_constructor_count() == 0);
+
+{
+  auto c = sync_wait(std::move(t));
+  assert(c.id() == 1);
+
+  assert(counted::active_instance_count() == 2);
+  assert(counted::copy_constructor_count() == 0);
+  assert(counted::move_constructor_count() > 0);
+  assert(counted::default_constructor_count() == 1);
+}
+
+// The result value in 't' is still alive until 't' destructs.
+assert(counted::active_instance_count() == 1);
+  }
+
+  assert(counted::active_instance_count() == 0);
+
+  counted::reset();
+
+  {
+auto t = f(false);
+
+assert(counted::active_instance_count() == 0);
+assert(counted::copy_constructor_count() == 0);
+assert(counted::move_constructor_count() == 0);
+
+{
+  auto c = sync_wait(std::move(t));
+  assert(c.id() == 1);
+
+  assert(counted::active_instance_count() == 2);
+  assert(counted::copy_constructor_count() == 0);
+  assert(counted::move_constructor_count() > 0);
+  assert(counted::default_constructor_count() == 1);
+}
+
+// The result value in 't' is still alive until 't' destructs.
+assert(counted::active_instance_count() == 1);
+  }
+}
+
+int main()
+{
+  test_return_value_lifetime();
+  return 0;
+}
Index: test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp
===
--- /dev/null
+++ test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp
@@ -0,0 +1,57 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+
+#include 
+#include 
+
+#include "../counted.hpp"
+#include "../sync_wait.hpp"
+
+DEFINE_COUNTED_VARIABLES();
+
+void test_parameter_lifetime()
+{
+  counted::reset();
+
+  auto f = [](counted c) -> std::experimental::task
+  {
+co_return c.id();
+  };
+
+  {
+auto t = f({});
+
+assert(counted::active_instance_count() == 1);
+assert(counted::copy_constructor_count() == 0);
+assert(counted::move_constructor_count() <= 2); // Ideally <= 1
+
+auto id = sync_wait(t);
+assert(id == 1);
+
+assert(counted::active_instance_count() == 1);
+assert(counted::copy_constructor_count() == 0);
+
+// We are relying on C++17 copy-elision when passing the temporary counter
+// into f(). Then f() must move the parameter into the coroutine frame by
+// calling the move-constructor. This move could also potentially be
+// 

[PATCH] D46140: [coroutines] std::task type (WIP)

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker updated this revision to Diff 192203.
lewissbaker edited the summary of this revision.
lewissbaker added a comment.
Herald added subscribers: libcxx-commits, jdoerfert.

- Don't use __allocator as variable name (it is #defined to NASTY_MACRO).
- Remove workaround in test for https://bugs.llvm.org/show_bug.cgi?id=37265
- Add support for custom allocators on member functions.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D46140/new/

https://reviews.llvm.org/D46140

Files:
  include/CMakeLists.txt
  include/experimental/__memory
  include/experimental/memory_resource
  include/experimental/task
  include/module.modulemap
  test/std/experimental/task/awaitable_traits.hpp
  test/std/experimental/task/counted.hpp
  test/std/experimental/task/lit.local.cfg
  test/std/experimental/task/manual_reset_event.hpp
  test/std/experimental/task/sync_wait.hpp
  test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp
  test/std/experimental/task/task.basic/task_of_value.pass.cpp
  test/std/experimental/task/task.basic/task_of_void.pass.cpp
  test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp
  test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp

Index: test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp
===
--- /dev/null
+++ test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp
@@ -0,0 +1,86 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+
+#include 
+#include 
+#include 
+
+#include "../counted.hpp"
+#include "../sync_wait.hpp"
+
+DEFINE_COUNTED_VARIABLES();
+
+void test_return_value_lifetime()
+{
+  counted::reset();
+
+  auto f = [](bool x) -> std::experimental::task
+  {
+if (x) {
+  counted c;
+  co_return std::move(c);
+}
+co_return {};
+  };
+
+  {
+auto t = f(true);
+
+assert(counted::active_instance_count() == 0);
+assert(counted::copy_constructor_count() == 0);
+assert(counted::move_constructor_count() == 0);
+
+{
+  auto c = sync_wait(std::move(t));
+  assert(c.id() == 1);
+
+  assert(counted::active_instance_count() == 2);
+  assert(counted::copy_constructor_count() == 0);
+  assert(counted::move_constructor_count() > 0);
+  assert(counted::default_constructor_count() == 1);
+}
+
+// The result value in 't' is still alive until 't' destructs.
+assert(counted::active_instance_count() == 1);
+  }
+
+  assert(counted::active_instance_count() == 0);
+
+  counted::reset();
+
+  {
+auto t = f(false);
+
+assert(counted::active_instance_count() == 0);
+assert(counted::copy_constructor_count() == 0);
+assert(counted::move_constructor_count() == 0);
+
+{
+  auto c = sync_wait(std::move(t));
+  assert(c.id() == 1);
+
+  assert(counted::active_instance_count() == 2);
+  assert(counted::copy_constructor_count() == 0);
+  assert(counted::move_constructor_count() > 0);
+  assert(counted::default_constructor_count() == 1);
+}
+
+// The result value in 't' is still alive until 't' destructs.
+assert(counted::active_instance_count() == 1);
+  }
+}
+
+int main()
+{
+  test_return_value_lifetime();
+  return 0;
+}
Index: test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp
===
--- /dev/null
+++ test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp
@@ -0,0 +1,57 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+
+#include 
+#include 
+
+#include "../counted.hpp"
+#include "../sync_wait.hpp"
+
+DEFINE_COUNTED_VARIABLES();
+
+void test_parameter_lifetime()
+{
+  counted::reset();
+
+  auto f = [](counted c) -> std::experimental::task
+  {
+co_return c.id();
+  };
+
+  {
+auto t = f({});
+
+assert(counted::active_instance_count() == 1);
+assert(counted::copy_constructor_count() == 0);
+assert(counted::move_constructor_count() <= 2); // Ideally <= 1
+
+auto id = sync_wait(t);
+assert(id == 1);
+
+assert(counted::active_instance_count() == 1);
+assert(counted::copy_constructor_count() == 0);
+
+// We 

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker added inline comments.



Comment at: lib/Sema/SemaCoroutine.cpp:675
+  // Second emphasis of [expr.await]p2: must be outside of an exception 
handler.
+  if (S.getCurScope()->getFlags() & Scope::CatchScope) {
+S.Diag(Loc, diag::err_coroutine_within_handler) << Keyword;

modocache wrote:
> modocache wrote:
> > lewissbaker wrote:
> > > modocache wrote:
> > > > EricWF wrote:
> > > > > We can still build a valid AST after encountering this error, no?
> > > > > 
> > > > > 
> > > > I believe so. Just to be clear: you'd like me to continue building the 
> > > > AST even after emitting this error diagnostic? My understanding is that 
> > > > most of this file bails soon after any error is encountered (correct me 
> > > > if that's wrong). I'm happy to change that, but I wonder if it'd be 
> > > > better to do that in a separate diff...?
> > > Do the scope flags reset when a lambda occurs within a catch-block?
> > > 
> > > eg. The following should still be valid.
> > > ```
> > > try { ... }
> > > catch (...) {
> > >   []() -> task { co_await foo(); }();
> > > }
> > > ```
> > I believe they're reset, the example you posted compiles fine with this 
> > patch. I'll add a test case specifically for this and confirm exactly where 
> > the scope flags are reset or ignored in the lambda case.
> When the parser encounters a lambda, it takes the path 
> `Parser::ParseLambdaExpression -> 
> Parser::ParseLambdaExpressionAfterIntroducer -> 
> Parser::ParseCompoundStatementBody`, which creates an inner scope for the 
> body of the lambda. This inner scope does not have the `Scope::CatchScope` 
> flag, so it doesn't result in the error.
> 
> Although, your question did make me realize that this compiles just fine, 
> even with this patch:
> 
> ```
> void example() {
>   try {
> throw;
>   } catch (...) {
> try {
>   co_await a;
> }
>   }
> }
> ```
> 
> But I believe this above case should also be treated as an error, right?
Yes, I believe that co_await within a try-block within a catch-block should not 
be allowed.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59076/new/

https://reviews.llvm.org/D59076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker added inline comments.



Comment at: test/SemaCXX/coroutines.cpp:299-311
   // FIXME: The spec says this is ill-formed.
   void operator=(CtorDtor&) {
 co_yield 0; // expected-error {{'co_yield' cannot be used in a copy 
assignment operator}}
   }
   void operator=(CtorDtor const &) {
 co_yield 0; // expected-error {{'co_yield' cannot be used in a copy 
assignment operator}}
   }

GorNishanov wrote:
> modocache wrote:
> > lewissbaker wrote:
> > > Not related to this diff, but...
> > > 
> > > I don't think that these should be ill-formed.
> > > 
> > > According to N4775 there are only exclusions added for [class.ctor] and 
> > > [class.dtor].
> > > I can't see anything in the spec that says that assignment special member 
> > > functions cannot be coroutines.
> > That's a great point. Could you create a Bugzilla for this work? And maybe 
> > we can get @GorNishanov's opinion?
> In 2015, such as https://wg21.link/N4499 there was a blank prohibition: 
> "A special member function shall not be a coroutine."
> 
> Later, at @rsmith suggestion, we relaxed it a little and banned only 
> constructors and destructors.
> 
> I am not yet aware of any use cases where such coroutines would be useful.
See https://bugs.llvm.org/show_bug.cgi?id=40997


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59076/new/

https://reviews.llvm.org/D59076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker added inline comments.



Comment at: lib/Sema/SemaCoroutine.cpp:197-200
-  if (S.isUnevaluatedContext()) {
-S.Diag(Loc, diag::err_coroutine_unevaluated_context) << Keyword;
-return false;
-  }

Does removing this check now mean that we're not checking that `co_return` 
statements don't appear in unevaluated contexts?

Or is that already handled elsewhere by the fact that `co_return` statements 
are not expressions and are therefore detected earlier as a grammar violation 
when parsing `sizeof()` expression?



Comment at: lib/Sema/SemaCoroutine.cpp:675
+  // Second emphasis of [expr.await]p2: must be outside of an exception 
handler.
+  if (S.getCurScope()->getFlags() & Scope::CatchScope) {
+S.Diag(Loc, diag::err_coroutine_within_handler) << Keyword;

modocache wrote:
> EricWF wrote:
> > We can still build a valid AST after encountering this error, no?
> > 
> > 
> I believe so. Just to be clear: you'd like me to continue building the AST 
> even after emitting this error diagnostic? My understanding is that most of 
> this file bails soon after any error is encountered (correct me if that's 
> wrong). I'm happy to change that, but I wonder if it'd be better to do that 
> in a separate diff...?
Do the scope flags reset when a lambda occurs within a catch-block?

eg. The following should still be valid.
```
try { ... }
catch (...) {
  []() -> task { co_await foo(); }();
}
```



Comment at: test/SemaCXX/coroutines.cpp:299-311
   // FIXME: The spec says this is ill-formed.
   void operator=(CtorDtor&) {
 co_yield 0; // expected-error {{'co_yield' cannot be used in a copy 
assignment operator}}
   }
   void operator=(CtorDtor const &) {
 co_yield 0; // expected-error {{'co_yield' cannot be used in a copy 
assignment operator}}
   }

Not related to this diff, but...

I don't think that these should be ill-formed.

According to N4775 there are only exclusions added for [class.ctor] and 
[class.dtor].
I can't see anything in the spec that says that assignment special member 
functions cannot be coroutines.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59076/new/

https://reviews.llvm.org/D59076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46140: [coroutines] std::task type (WIP)

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker added inline comments.



Comment at: test/std/experimental/task/sync_wait.hpp:36-37
+  __isSet_ = true;
+}
+__cv_.notify_all();
+  }

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.


https://reviews.llvm.org/D46140



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46140: [coroutines] std::task type (WIP)

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker updated this revision to Diff 156641.
lewissbaker edited the summary of this revision.
lewissbaker added a comment.

- Make `task` constructor from `coroutine_handle` private.
- Run clang-format over .


https://reviews.llvm.org/D46140

Files:
  include/CMakeLists.txt
  include/experimental/__memory
  include/experimental/memory_resource
  include/experimental/task
  include/module.modulemap
  test/std/experimental/task/awaitable_traits.hpp
  test/std/experimental/task/counted.hpp
  test/std/experimental/task/lit.local.cfg
  test/std/experimental/task/manual_reset_event.hpp
  test/std/experimental/task/sync_wait.hpp
  test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp
  test/std/experimental/task/task.basic/task_of_value.pass.cpp
  test/std/experimental/task/task.basic/task_of_void.pass.cpp
  test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp
  test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp

Index: test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp
===
--- /dev/null
+++ test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp
@@ -0,0 +1,86 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+
+#include 
+#include 
+#include 
+
+#include "../counted.hpp"
+#include "../sync_wait.hpp"
+
+DEFINE_COUNTED_VARIABLES();
+
+void test_return_value_lifetime()
+{
+  counted::reset();
+
+  auto f = [](bool x) -> std::experimental::task
+  {
+if (x) {
+  counted c;
+  co_return std::move(c);
+}
+co_return {};
+  };
+
+  {
+auto t = f(true);
+
+assert(counted::active_instance_count() == 0);
+assert(counted::copy_constructor_count() == 0);
+assert(counted::move_constructor_count() == 0);
+
+{
+  auto c = sync_wait(std::move(t));
+  assert(c.id() == 1);
+
+  assert(counted::active_instance_count() == 2);
+  assert(counted::copy_constructor_count() == 0);
+  assert(counted::move_constructor_count() > 0);
+  assert(counted::default_constructor_count() == 1);
+}
+
+// The result value in 't' is still alive until 't' destructs.
+assert(counted::active_instance_count() == 1);
+  }
+
+  assert(counted::active_instance_count() == 0);
+
+  counted::reset();
+
+  {
+auto t = f(false);
+
+assert(counted::active_instance_count() == 0);
+assert(counted::copy_constructor_count() == 0);
+assert(counted::move_constructor_count() == 0);
+
+{
+  auto c = sync_wait(std::move(t));
+  assert(c.id() == 1);
+
+  assert(counted::active_instance_count() == 2);
+  assert(counted::copy_constructor_count() == 0);
+  assert(counted::move_constructor_count() > 0);
+  assert(counted::default_constructor_count() == 1);
+}
+
+// The result value in 't' is still alive until 't' destructs.
+assert(counted::active_instance_count() == 1);
+  }
+}
+
+int main()
+{
+  test_return_value_lifetime();
+  return 0;
+}
Index: test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp
===
--- /dev/null
+++ test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp
@@ -0,0 +1,57 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+
+#include 
+#include 
+
+#include "../counted.hpp"
+#include "../sync_wait.hpp"
+
+DEFINE_COUNTED_VARIABLES();
+
+void test_parameter_lifetime()
+{
+  counted::reset();
+
+  auto f = [](counted c) -> std::experimental::task
+  {
+co_return c.id();
+  };
+
+  {
+auto t = f({});
+
+assert(counted::active_instance_count() == 1);
+assert(counted::copy_constructor_count() == 0);
+assert(counted::move_constructor_count() <= 2); // Ideally <= 1
+
+auto id = sync_wait(t);
+assert(id == 1);
+
+assert(counted::active_instance_count() == 1);
+assert(counted::copy_constructor_count() == 0);
+
+// We are relying on C++17 copy-elision when passing the temporary counter
+// into f(). Then f() must move the parameter into the coroutine frame by
+// calling the move-constructor. This move could also potentially be
+// elided by the
+

[PATCH] D46140: [coroutines] std::task type (WIP)

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker updated this revision to Diff 156565.
lewissbaker added a comment.
Herald added subscribers: cfe-commits, ldionne, mgorny.

Many improvements:

- Remove move-assignment operator
- `task::operator co_await()` now returns by value when awaiting `task` 
rvalue.
- Explicitly scope calls to std-library functions that live in `_VSTD`.
- Make `__task_promise` ABI stable under exception/no-exception compilation 
modes.
- Moved `__aligned_allocation_size` to 
- Make `task` have a default template parameter of void, allowing `task<>` 
to mean `task`
- Allow `co_return { ... }` by adding overload of `return_value()` taking `T&&` 
and another taking `initializer_list`.
- Implement optimisation that avoids storing the allocator in the coroutine 
frame if `allocator_traits::is_always_equal` is true and the allocator is 
default constructible.
- Added unit-tests (currently failing some custom allocator tests under debug 
builds)


Repository:
  rCXX libc++

https://reviews.llvm.org/D46140

Files:
  include/CMakeLists.txt
  include/experimental/__memory
  include/experimental/memory_resource
  include/experimental/task
  include/module.modulemap
  test/std/experimental/task/awaitable_traits.hpp
  test/std/experimental/task/counted.hpp
  test/std/experimental/task/lit.local.cfg
  test/std/experimental/task/manual_reset_event.hpp
  test/std/experimental/task/sync_wait.hpp
  test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp
  test/std/experimental/task/task.basic/task_of_value.pass.cpp
  test/std/experimental/task/task.basic/task_of_void.pass.cpp
  test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp
  test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp

Index: test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp
===
--- /dev/null
+++ test/std/experimental/task/task.lifetime/task_return_value_lifetime.pass.cpp
@@ -0,0 +1,86 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+
+#include 
+#include 
+#include 
+
+#include "../counted.hpp"
+#include "../sync_wait.hpp"
+
+DEFINE_COUNTED_VARIABLES();
+
+void test_return_value_lifetime()
+{
+  counted::reset();
+
+  auto f = [](bool x) -> std::experimental::task
+  {
+if (x) {
+  counted c;
+  co_return std::move(c);
+}
+co_return {};
+  };
+
+  {
+auto t = f(true);
+
+assert(counted::active_instance_count() == 0);
+assert(counted::copy_constructor_count() == 0);
+assert(counted::move_constructor_count() == 0);
+
+{
+  auto c = sync_wait(std::move(t));
+  assert(c.id() == 1);
+
+  assert(counted::active_instance_count() == 2);
+  assert(counted::copy_constructor_count() == 0);
+  assert(counted::move_constructor_count() > 0);
+  assert(counted::default_constructor_count() == 1);
+}
+
+// The result value in 't' is still alive until 't' destructs.
+assert(counted::active_instance_count() == 1);
+  }
+
+  assert(counted::active_instance_count() == 0);
+
+  counted::reset();
+
+  {
+auto t = f(false);
+
+assert(counted::active_instance_count() == 0);
+assert(counted::copy_constructor_count() == 0);
+assert(counted::move_constructor_count() == 0);
+
+{
+  auto c = sync_wait(std::move(t));
+  assert(c.id() == 1);
+
+  assert(counted::active_instance_count() == 2);
+  assert(counted::copy_constructor_count() == 0);
+  assert(counted::move_constructor_count() > 0);
+  assert(counted::default_constructor_count() == 1);
+}
+
+// The result value in 't' is still alive until 't' destructs.
+assert(counted::active_instance_count() == 1);
+  }
+}
+
+int main()
+{
+  test_return_value_lifetime();
+  return 0;
+}
Index: test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp
===
--- /dev/null
+++ test/std/experimental/task/task.lifetime/task_parameter_lifetime.pass.cpp
@@ -0,0 +1,57 @@
+// -*- C++ -*-
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+
+#include 
+#include 
+
+#include "../counted.hpp"
+#include "../sync_wait.hpp"
+
+DEFINE_COUNTED_VARIABLES();
+
+void test_parameter_lifetime()
+{
+ 

[PATCH] D45121: [coroutines] Add noop_coroutine to

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker added inline comments.



Comment at: include/experimental/coroutine:294
+
+inline _LIBCPP_ALWAYS_INLINE
+noop_coroutine_handle noop_coroutine() _NOEXCEPT {

EricWF wrote:
> This should just be `_LIBCPP_INLINE_VISIBILITY`. We try not to use 
> `_LIBCPP_ALWAYS_INLINE` in new code.
Should the same change be applied to the other usages of 
`_LIBCPP_ALWAYS_INLINE` in this file?
Should some of them be marked `constexpr` to be consistent with 
`noop_coroutine_handle` member functions above?


https://reviews.llvm.org/D45121



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45121: [coroutines] Add noop_coroutine to

[Lewis Baker via Phabricator via cfe-commits]

lewissbaker accepted this revision.
lewissbaker added a comment.
This revision is now accepted and ready to land.

The `coroutine_handle` type does not have a 
`from_address()` or a `from_promise()` static functions in the same way that 
the `coroutine_handle` implementation does.
Is this intentional or an oversight in the TS wording?

They don't seem hugely useful, so I'm not that worried.
If you know that you have a `coroutine_handle` then you 
can just use `noop_coroutine()` to get the handle instead.




Comment at: 
test/std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.noop/noop_coroutine.pass.cpp:62
+  assert(h.address() != nullptr);
+}
+

Maybe also add a test for `from_address()`?
eg. `assert(coroutine_handle<>::from_address(h.address()) == base);`


https://reviews.llvm.org/D45121



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits