This is an archive of the discontinued LLVM Phabricator instance.

[Coroutines] Ensure co_await promise.final_suspend() does not throw
ClosedPublic

Authored by lxfind on Jun 17 2020, 10:33 AM.

Details

Summary

This patch addresses https://bugs.llvm.org/show_bug.cgi?id=46256
The spec of coroutine requires that the expression co_­await promise.final_­suspend() shall not be potentially-throwing.
To check this, we recursively look at every call (including Call, MemberCall, OperatorCall and Constructor) in all code
generated by the final suspend, and ensure that the callees are declared with noexcept. We also look at any returned data
type that requires explicit destruction, and check their destructors for noexcept.

This patch does not check declarations with dependent types yet, which will be done in future patches.

Updated all tests to add noexcept to the required functions, and added a dedicated test for this patch.

This patch might start to cause existing codebase fail to compile because most people may not have been strict in tagging
all the related functions noexcept.

Diff Detail

Event Timeline

lxfind created this revision.Jun 17 2020, 10:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2020, 10:33 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
modocache added a subscriber: junparser.

Excellent, thank you! The test failures on the diff appear to be legitimate, they reproduce for me when I apply this patch to my local checkout and run ninja check-clang. Could you take a look?

This LGTM otherwise! Also adding @junparser in case they have any thoughts, if not I'll accept after the test failures are addressed, Thanks!

clang/lib/Sema/SemaExceptionSpec.cpp
1051

Small nit-pick: Omit the { and } here, as that's the convention in the surrounding lines of this file and in the LLVM project in general.

lxfind updated this revision to Diff 271801.Jun 18 2020, 11:45 AM

Address feedback and update failed tests

lewissbaker added inline comments.Jun 18 2020, 2:16 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
10527

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

clang/lib/Sema/SemaCoroutine.cpp
662–664

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

clang/lib/Sema/SemaExceptionSpec.cpp
1049–1050

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?

clang/test/AST/Inputs/std-coroutine.h
13

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.

clang/test/SemaCXX/coroutine-final-suspend-noexcept.cpp
15

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.

lxfind marked an inline comment as done.Jun 18 2020, 2:28 PM
lxfind added inline comments.
clang/test/SemaCXX/coroutine-final-suspend-noexcept.cpp
15

from_address is in fact called as part of co_await __promise.final_suspend().
Specifically, when this co_await is translated into a call to finalSuspendObjAwaiter.await_suspend(handle), the handle parameter is obtained by calling from_address.
Since the specification requires that co_await __promise.final_suspend() should not be potentially throwing, it must requires from_address to be declared as noexcept.

lxfind updated this revision to Diff 272109.Jun 19 2020, 9:39 AM

Addressed comments: Updated error message, and sorted notes. The tests are kept unchanged.

lxfind marked 6 inline comments as done.Jun 19 2020, 9:44 AM

Excellent, thank you! The test failures on the diff appear to be legitimate, they reproduce for me when I apply this patch to my local checkout and run ninja check-clang. Could you take a look?

This LGTM otherwise! Also adding @junparser in case they have any thoughts, if not I'll accept after the test failures are addressed, Thanks!

@modocache LGTM, @lxfind Thank you!

All tests are passing now. Thanks for reviewing!

modocache accepted this revision.Jun 22 2020, 11:55 AM

Sweet! Thanks for the reviews/responses, LGTM :)

This revision is now accepted and ready to land.Jun 22 2020, 11:55 AM
This revision was automatically updated to reflect the committed changes.

Hi!

I see a bunch of failures when I run libcxx testcases with this patch. Should there be "noexcept" on a number of more places?

Failed Tests (8):

libc++ :: libcxx/experimental/language.support/support.coroutines/dialect_support.pass.cpp
libc++ :: std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.prom/promise.pass.cpp
libc++ :: std/experimental/language.support/support.coroutines/end.to.end/await_result.pass.cpp
libc++ :: std/experimental/language.support/support.coroutines/end.to.end/bool_await_suspend.pass.cpp
libc++ :: std/experimental/language.support/support.coroutines/end.to.end/expected.pass.cpp
libc++ :: std/experimental/language.support/support.coroutines/end.to.end/fullexpr-dtor.pass.cpp
libc++ :: std/experimental/language.support/support.coroutines/end.to.end/generator.pass.cpp
libc++ :: std/experimental/language.support/support.coroutines/end.to.end/go.pass.cpp
bjope added a subscriber: bjope.Jun 23 2020, 1:56 AM

Test failures are being fixed in https://reviews.llvm.org/D82338/new/

Thanks!