[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