On 2/22/21 3:59 PM, Iain Sandoe wrote:
Hi,
PR96251 (and dups) is a rejects-valid bug against coroutines (but I’m
not sure that the problem is really there).
* coroutines cannot be constexpr.
* Part of the way that the coroutine implementation works is a
consequence of the "lazy discovery of coroutine-ness”; whenever
we first encounter a coroutine keyword...
.. we mark the function as a coroutine, and then we can deal with
diagnostics etc. that change under these circumstances. This
marking of the function is independent of whether keyword
expressions are type-dependent or not.
* when we encounter a coroutine keyword in a constexpr context
we error.
* the constexpr machinery also has been taught that coroutine
expressions should cause constexpr-ness to be rejected when
checking for "potentially constexpr".
So why is there a problem?
* lambdas are implicitly constexpr from C++17.
* the constexpr machinery tends to bail early *with a conservative
assumption that the expression is potentially constexpr* when it
finds a type-dependent expression - without evaluating sub-
expressions to see if they are valid (thus missing coroutine exprs.
in such positions).
The combination of these ^^ two things, means that for many generic
lambdas with non-trivial bodies - we then enter instantiation with a
constexpr type-dependent coroutine (which is a Thing that Should not
Be). As soon as we try to tsub any coroutine keyword expression
encountered, we error out.
* I was not able to see any way in which the instantiation process
could be made to bail in this case and re-try for non-constexpr.
Many of the other places that set cp_function_chain->invalid_constexpr
condition their errors on !is_instantiation_of_constexpr, which should
also fix this testcase.
* Nor able to see somewhere else where that decision could be made.
^^ these two points reflect that I'm not familiar with the constexpr
machinery.
* Proposed solution.
Since coroutines cannot be constexpr (not even sure what that would
mean in any practicable implementation). The fix proposed is to
reject constexpr-ness for coroutine functions as early as possible.
* a) We can prevent a generic lambda coroutine from being converted
to constexpr in finish_function (). Likewise, via the tests below,
we can
avoid it for regular lambdas.
b) We can also reject coroutine function decls early in both
is_valid_constexpr_fn () and potential_constant_expression_1 ().
So - is there some alternate solution that would be better?
====
(in some ways it seems pointless to delay rejection of a coroutine
function to some later point).
OTOH, I suppose that one could have some weird code where coroutine
coroutine keywords only appeared in a non-constexpr paths of the code
- but our current lowering is not prepared for such a circumstance.
AFAIU the standard, there's no dispensation from the "cannot be
constexpr" for such a code construction .. but ICBW.
In any event, the cases reported (and fixed by this patch) are not trying
anything so fiendishly clever.
Tested on x86_64-darwin.
Suggestions?
OK for master (after wider testing)?
thanks
Iain
= ----
coroutines cannot be constexpr, but generic lambdas (from C++17)
are implicitly assumed to be, and the processing of type-
dependent lambda bodies often terminates before any coroutine
expressions are encountered. For example, the PR notes cases
where the coro expressions are hidden by type-dependent for or
switch expressions.
The solution proposed is to deny coroutine generic lambdas. We also
tighten up the checks for is_valid_constexpr_fn() and do an early
test for coroutine functions in checking for potential constant
expressions.
gcc/cp/ChangeLog:
PR c++/96251
* constexpr.c (is_valid_constexpr_fn): Test for a
coroutine before the chekc for lambda.
(potential_constant_expression_1): Disallow coroutine
function decls.
* decl.c (finish_function): Do not mark generic
coroutine lambda templates as constexpr.
gcc/testsuite/ChangeLog:
PR c++/96251
* g++.dg/coroutines/pr96251.C: New test.
---
gcc/cp/constexpr.c | 7 ++++++-
gcc/cp/decl.c | 2 +-
gcc/testsuite/g++.dg/coroutines/pr96251.C | 22 ++++++++++++++++++++++
3 files changed, 29 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/coroutines/pr96251.C
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 377fe322ee8..036bf04efa9 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -210,7 +210,9 @@ is_valid_constexpr_fn (tree fun, bool complain)
}
}
- if (LAMBDA_TYPE_P (CP_DECL_CONTEXT (fun)) && cxx_dialect < cxx17)
+ if (DECL_COROUTINE_P (fun))
+ ret = false;
+ else if (LAMBDA_TYPE_P (CP_DECL_CONTEXT (fun)) && cxx_dialect < cxx17)
{
ret = false;
if (complain)
@@ -7827,6 +7829,9 @@ potential_constant_expression_1 (tree t, bool
want_rval, bool strict, bool now,
switch (TREE_CODE (t))
{
case FUNCTION_DECL:
+ if (DECL_COROUTINE_P (t))
+ return false;
+ /* FALLTHROUGH. */
case BASELINK:
case TEMPLATE_DECL:
case OVERLOAD:
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 7fa8f52d667..3a089b5bee5 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -17374,7 +17374,7 @@ finish_function (bool inline_p)
if (cxx_dialect >= cxx17
&& LAMBDA_TYPE_P (CP_DECL_CONTEXT (fndecl)))
DECL_DECLARED_CONSTEXPR_P (fndecl)
- = ((processing_template_decl
+ = (((processing_template_decl && !DECL_COROUTINE_P(fndecl))
|| is_valid_constexpr_fn (fndecl, /*complain*/false))
&& potential_constant_expression (DECL_SAVED_TREE (fndecl)));
diff --git a/gcc/testsuite/g++.dg/coroutines/pr96251.C
b/gcc/testsuite/g++.dg/coroutines/pr96251.C
new file mode 100644
index 00000000000..6824d783d5f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr96251.C
@@ -0,0 +1,22 @@
+#include <coroutine>
+
+struct coroutine {
+ struct promise_type {
+ auto get_return_object() { return coroutine(); }
+ auto initial_suspend() { return std::suspend_always(); }
+ auto yield_value(int) { return std::suspend_always(); }
+ void return_void() {}
+ auto final_suspend() noexcept { return std::suspend_always(); }
+ void unhandled_exception() {}
+ };
+};
+
+int main() {
+ auto f = [](auto max) -> coroutine {
+ for (int i = 0; i < max; ++i) {
+ co_yield i;
+ }
+ };
+
+ f(10);
+}
