This is an archive of the discontinued LLVM Phabricator instance.

[Coroutines] Catch exceptions in await_resume
ClosedPublic

Authored by modocache on Apr 19 2018, 8:24 PM.

Details

Summary

http://wg21.link/P0664r2 section "Evolution/Core Issues 24" describes a
proposed change to Coroutines TS that would have any exceptions thrown
after the initial suspend point of a coroutine be caught by the handler
specified by the promise type's 'unhandled_exception' member function.
This commit provides a sample implementation of the specified behavior.

Test Plan: check-clang

Diff Detail

Repository
rC Clang

Event Timeline

modocache created this revision.Apr 19 2018, 8:24 PM

Thank you for doing this. It looks very elegant, but, it is a little bit wrong. It creates two different initial_suspend objects.
Run this example:

https://wandbox.org/permlink/Q1Zd2NUlolmw9YmX

You will observe that in trunk we are getting the output:

0x216808c: constructed(initial)
0x216808c: await_ready
0x216808c: await_suspend
0x216808c: await_resume
0x216808c: destroyed
0x21680b8: constructed(final)
0x21680b8: await_ready
0x21680b8: await_suspend
consumed 10 values with sum 45
0x21680b8: destroyed
promise destroyed

With this change, the output becomes:

0x1965c4c: constructed(initial)
0x1965c4c: await_ready
0x1965c4c: await_suspend
0x1965c4c: destroyed
0x1965c60: constructed(initial)
0x1965c60: await_resume
0x1965c60: destroyed
0x1965c80: constructed(final)
0x1965c80: await_ready
0x1965c80: await_suspend
consumed 10 values with sum 45
0x1965c80: destroyed
0x1965c60: destroyed
promise destroyed

I suggest, first modify the unit test to check that we do not create two initial_suspend objects. Then to fix it :-)

lib/CodeGen/CGCoroutine.cpp
620
636

auto *AwaitExpr =

639

Consider:

std::array<Stmt *,2> Stmts = {AwaitExpr->getResumeExpr(), S.getBody()};

instead

GorNishanov requested changes to this revision.Apr 25 2018, 2:11 PM
This revision now requires changes to proceed.Apr 25 2018, 2:11 PM
modocache updated this revision to Diff 144908.May 2 2018, 11:24 AM

Thanks for the review, @GorNishanov. Here's a more correct solution: an i1 is used to keep track of whether await_resume threw. If it did, the coroutine body is skipped, and we go straight to the final suspend point. Otherwise, the coroutine body is executed as normal.

modocache marked 3 inline comments as done.May 2 2018, 11:25 AM

Looks good. Make sure to run the tests in release and debug mode.
On my box, these three test failed in release mode.

Clang :: CodeGenCoroutines/coro-await-resume-eh.cpp
Clang :: CodeGenCoroutines/coro-promise-dtor.cpp
Clang :: CodeGenCoroutines/coro-unhandled-exception.cpp

There is a small difference in friendly names emission in release mode

lib/CodeGen/CGCoroutine.cpp
220

I suggest to check whether await_resume expression is noexcept and omit emission of all of the goo. Most of the time the await_ready for initial_suspend will be noexcept and thus we would not need to emit extra stuff.

modocache updated this revision to Diff 144961.May 2 2018, 6:06 PM

Oops, thanks for testing on release mode, @GorNishanov. Turns out I had a dangling pointer. With this update the tests pass on both release and debug.

This revision is now accepted and ready to land.May 3 2018, 5:09 PM
This revision was automatically updated to reflect the committed changes.

Thanks again for all the reviews, @GorNishanov! Very much appreciated.

lib/CodeGen/CGCoroutine.cpp
220

Oh shoot, I forgot to do this! I'll take care of this in a follow-up commit within the next week.