Hi Nathan,
Nathan Sidwell <nat...@acm.org> wrote:
> On 1/29/20 10:39 AM, Iain Sandoe wrote:
>> Hi Nathan,
>> + /* If we are missing fundmental information, such as the traits, then
>> don't
>> + emit this for every keyword in a TU. This particular error is per TU
>> + so we don't need to keep the indicator per function. */
>> + static bool traits_error_emitted = false;
>
> You can of course move this into the if's block scope.
done.
>> + if (traits_decl == error_mark_node)
>> + {
>> + if (!traits_error_emitted)
>> + error_at (kw, "cannot find %<coroutine traits%> template");
>
> Give the name you were looking for:
> "%<%E::%E%> ...", std_node, coro_traits_identifier
I had the “complain” flag on to the lookup so that it was emitting an error with
that information.
however. ….
> also, what if you find something, but it's not a type template?
… I’ve switched the complain off on lookup_qualified_name and now check for
a type template.
I took the liberty of repeating this treatment in the coroutine handle lookup
code
(new testcases attached).
so the errors now look like:
"cannot find a valid coroutine traits template using 'std::coroutine_traits’”
and
"cannot find a valid coroutine handle class template using
'std::coroutine_handle’”
.. but open to tweaking them if there could be bette wording.
>> /* Coroutine traits template. */
>> coro_traits_templ = find_coro_traits_template_decl (loc);
>> - gcc_checking_assert (coro_traits_templ != NULL);
>> + if (coro_traits_templ == NULL_TREE
>> + || coro_traits_templ == error_mark_node)
>> + return false;
>
> ISTM that find_coro_traits_template_decl should be returning exactly one of
> NULL_TREE of error_mark_node on failure. Its users don't particularly care
> why it failed (not found vs found ambiguous/not template).
fair enough, settled for NULL_TREE.
> + /* Save the coroutine data on the side to avoid the overhead on every
>> + if (!coro_info->coro_ret_type_error_emitted)
>> + error_at (DECL_SOURCE_LOCATION (fndecl), "a coroutine must have a"
>> + " class or struct return type");
>
> Perhaps something like "coroutine return type %qT is not a class"? I.e. show
> them the type.
> (structs are classes, there's no need to say 'class or struct’)
yes, that’s nicer, done.
>> + coro_info->coro_ret_type_error_emitted = true;
>> + return false;
>> + }
>> +
>> tree templ_class = instantiate_coro_traits (fndecl, loc);
>> /* Find the promise type for that. */
>> @@ -422,7 +454,7 @@ coro_promise_type_found_p (tree fndecl, location_t loc)
>> /* Try to find the handle type for the promise. */
>> tree handle_type =
>> instantiate_coro_handle_for_promise_type (loc, coro_info->promise_type);
>> - if (handle_type == NULL_TREE)
>> + if (handle_type == NULL_TREE || handle_type == error_mark_node)
>
> similar to coro_traits_template_decl.
gave this a similar treatment.
As below with those changes and additional test cases,
OK / tweak error messages?
thanks
Iain.
Since coroutine-ness is discovered lazily, we encounter the diagnostics
during each keyword parse. We were not handling the case where a user code
failed to include fundamental information (e.g. the traits) in a graceful
manner.
Once we've emitted an error for this level of fail, then we suppress
additional copies (otherwise the same thing will be reported for every
coroutine keyword seen).
gcc/cp/ChangeLog:
2020-01-30 Iain Sandoe <i...@sandoe.co.uk>
* coroutines.cc (struct coroutine_info): Add a bool flag to note
that we emitted an error for a bad function return type.
(get_coroutine_info): Tolerate an unset info table in case of
missing traits.
(find_coro_traits_template_decl): In case of error or if we didn't
find a type template, note we emitted the error and suppress
duplicates.
(find_coro_handle_template_decl): Likewise.
(instantiate_coro_traits): Only check for error_mark_node in the
return from lookup_qualified_name.
(coro_promise_type_found_p): Reorder initialization so that we check
for the traits and their usability before allocation of the info
table. Check for a suitable return type and emit a diagnostic for
here instead of relying on the lookup machinery. This allows the
error to have a better location, and means we can suppress multiple
copies.
(coro_function_valid_p): Re-check for a valid promise (and thus the
traits) before proceeding. Tolerate missing info as a fatal error.
gcc/testsuite/ChangeLog:
2020-01-30 Iain Sandoe <i...@sandoe.co.uk>
* g++.dg/coroutines/pr93458-1-missing-traits.C: New test.
* g++.dg/coroutines/pr93458-2-bad-traits.C: New test.
* g++.dg/coroutines/pr93458-3-missing-handle.C: New test.
* g++.dg/coroutines/pr93458-4-bad-coro-handle.C: New test.
* g++.dg/coroutines/pr93458-5-bad-coro-type.C: New test.
---
gcc/cp/coroutines.cc | 99 ++++++++++++++-----
.../coroutines/pr93458-1-missing-traits.C | 10 ++
.../g++.dg/coroutines/pr93458-2-bad-traits.C | 16 +++
.../coroutines/pr93458-3-missing-handle.C | 17 ++++
.../coroutines/pr93458-4-bad-coro-handle.C | 21 ++++
.../coroutines/pr93458-5-bad-coro-type.C | 12 +++
6 files changed, 148 insertions(+), 27 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/coroutines/pr93458-1-missing-traits.C
create mode 100644 gcc/testsuite/g++.dg/coroutines/pr93458-2-bad-traits.C
create mode 100644 gcc/testsuite/g++.dg/coroutines/pr93458-3-missing-handle.C
create mode 100644 gcc/testsuite/g++.dg/coroutines/pr93458-4-bad-coro-handle.C
create mode 100644 gcc/testsuite/g++.dg/coroutines/pr93458-5-bad-coro-type.C
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index f7f85cb7643..99750d7df29 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -91,6 +91,8 @@ struct GTY((for_user)) coroutine_info
tree promise_proxy; /* Likewise, a proxy promise instance. */
location_t first_coro_keyword; /* The location of the keyword that made this
function into a coroutine. */
+ /* Flags to avoid repeated errors for per-function issues. */
+ bool coro_ret_type_error_emitted;
};
struct coroutine_info_hasher : ggc_ptr_hash<coroutine_info>
@@ -169,7 +171,8 @@ get_or_insert_coroutine_info (tree fn_decl)
coroutine_info *
get_coroutine_info (tree fn_decl)
{
- gcc_checking_assert (coroutine_info_table != NULL);
+ if (coroutine_info_table == NULL)
+ return NULL;
coroutine_info **slot = coroutine_info_table->find_slot_with_hash
(fn_decl, coroutine_info_hasher::hash (fn_decl), NO_INSERT);
@@ -256,10 +259,18 @@ static tree
find_coro_traits_template_decl (location_t kw)
{
tree traits_decl = lookup_qualified_name (std_node, coro_traits_identifier,
- 0, true);
- if (traits_decl == NULL_TREE || traits_decl == error_mark_node)
- {
- error_at (kw, "cannot find %<coroutine traits%> template");
+ 0, /*complain=*/false);
+ if (traits_decl == error_mark_node
+ || !DECL_TYPE_TEMPLATE_P (traits_decl))
+ {
+ /* If we are missing fundmental information, such as the traits, (or the
+ declaration found is not a type template), then don't emit an error
+ for every keyword in a TU, just do it once. */
+ static bool traits_error_emitted = false;
+ if (!traits_error_emitted)
+ error_at (kw, "cannot find a valid coroutine traits template"
+ " using %<%E::%E%>", std_node, coro_traits_identifier);
+ traits_error_emitted = true;
return NULL_TREE;
}
else
@@ -299,7 +310,7 @@ instantiate_coro_traits (tree fndecl, location_t kw)
/*in_decl=*/NULL_TREE, /*context=*/NULL_TREE,
/*entering scope=*/false, tf_warning_or_error);
- if (traits_class == error_mark_node || traits_class == NULL_TREE)
+ if (traits_class == error_mark_node)
{
error_at (kw, "cannot instantiate %<coroutine traits%>");
return NULL_TREE;
@@ -314,10 +325,17 @@ static tree
find_coro_handle_template_decl (location_t kw)
{
tree handle_decl = lookup_qualified_name (std_node, coro_handle_identifier,
- 0, true);
- if (handle_decl == NULL_TREE || handle_decl == error_mark_node)
- {
- error_at (kw, "cannot find %<coroutine handle%> template");
+ 0, /*complain=*/false);
+ if (handle_decl == error_mark_node
+ || !DECL_CLASS_TEMPLATE_P (handle_decl))
+ {
+ /* As for the coroutine traits, this error is per TU, so only emit it
+ once. */
+ static bool coro_handle_error_emitted = false;
+ if (!coro_handle_error_emitted)
+ error_at (kw, "cannot find a valid coroutine handle class template"
+ " using %<%E::%E%>", std_node, coro_handle_identifier);
+ coro_handle_error_emitted = true;
return NULL_TREE;
}
else
@@ -370,34 +388,42 @@ coro_promise_type_found_p (tree fndecl, location_t loc)
{
gcc_assert (fndecl != NULL_TREE);
- /* Save the coroutine data on the side to avoid the overhead on every
- function decl. */
-
- /* We only need one entry per coroutine in a TU, the assumption here is that
- there are typically not 1000s. */
if (!coro_initialized)
{
- gcc_checking_assert (coroutine_info_table == NULL);
- /* A table to hold the state, per coroutine decl. */
- coroutine_info_table =
- hash_table<coroutine_info_hasher>::create_ggc (11);
- /* Set up the identifiers we will use. */
- gcc_checking_assert (coro_traits_identifier == NULL);
+ /* Trees we only need to create once.
+ Set up the identifiers we will use. */
coro_init_identifiers ();
- /* Trees we only need to create once. */
+
/* Coroutine traits template. */
coro_traits_templ = find_coro_traits_template_decl (loc);
- gcc_checking_assert (coro_traits_templ != NULL);
+ if (coro_traits_templ == NULL_TREE)
+ return false;
+
/* coroutine_handle<> template. */
coro_handle_templ = find_coro_handle_template_decl (loc);
- gcc_checking_assert (coro_handle_templ != NULL);
+ if (coro_handle_templ == NULL_TREE)
+ return false;
+
/* We can also instantiate the void coroutine_handle<> */
void_coro_handle_type =
instantiate_coro_handle_for_promise_type (loc, NULL_TREE);
- gcc_checking_assert (void_coro_handle_type != NULL);
+ if (void_coro_handle_type == NULL_TREE)
+ return false;
+
+ /* A table to hold the state, per coroutine decl. */
+ gcc_checking_assert (coroutine_info_table == NULL);
+ coroutine_info_table =
+ hash_table<coroutine_info_hasher>::create_ggc (11);
+
+ if (coroutine_info_table == NULL)
+ return false;
+
coro_initialized = true;
}
+ /* Save the coroutine data on the side to avoid the overhead on every
+ function decl tree. */
+
coroutine_info *coro_info = get_or_insert_coroutine_info (fndecl);
/* Without this, we cannot really proceed. */
gcc_checking_assert (coro_info);
@@ -407,6 +433,19 @@ coro_promise_type_found_p (tree fndecl, location_t loc)
{
/* Get the coroutine traits template class instance for the function
signature we have - coroutine_traits <R, ...> */
+ tree return_type = TREE_TYPE (TREE_TYPE (fndecl));
+ if (!CLASS_TYPE_P (return_type))
+ {
+ /* It makes more sense to show the function header for this, even
+ though we will have encountered it when processing a keyword.
+ Only emit the error once, not for every keyword we encounter. */
+ if (!coro_info->coro_ret_type_error_emitted)
+ error_at (DECL_SOURCE_LOCATION (fndecl), "coroutine return type"
+ " %qT is not a class", return_type);
+ coro_info->coro_ret_type_error_emitted = true;
+ return false;
+ }
+
tree templ_class = instantiate_coro_traits (fndecl, loc);
/* Find the promise type for that. */
@@ -597,11 +636,17 @@ coro_function_valid_p (tree fndecl)
{
location_t f_loc = DECL_SOURCE_LOCATION (fndecl);
+ /* For cases where fundamental information cannot be found, e.g. the
+ coroutine traits are missing, we need to punt early. */
+ if (!coro_promise_type_found_p (fndecl, f_loc))
+ return false;
+
/* Since we think the function is a coroutine, that implies we parsed
a keyword that triggered this. Keywords check promise validity for
their context and thus the promise type should be known at this point. */
- gcc_checking_assert (get_coroutine_handle_type (fndecl) != NULL_TREE
- && get_coroutine_promise_type (fndecl) != NULL_TREE);
+ if (get_coroutine_handle_type (fndecl) == NULL_TREE
+ || get_coroutine_promise_type (fndecl) == NULL_TREE)
+ return false;
if (current_function_returns_value || current_function_returns_null)
{
diff --git a/gcc/testsuite/g++.dg/coroutines/pr93458-1-missing-traits.C
b/gcc/testsuite/g++.dg/coroutines/pr93458-1-missing-traits.C
new file mode 100644
index 00000000000..1677a134dfd
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr93458-1-missing-traits.C
@@ -0,0 +1,10 @@
+// { dg-additional-options "-fsyntax-only -fexceptions -w" }
+
+// Diagose missing traits (e.g. fail to include <coroutine>).
+
+int
+bad_coroutine (void)
+{
+ co_yield 5; // { dg-error {cannot find a valid coroutine traits template
using 'std::coroutine_traits'} }
+ co_return;
+}
diff --git a/gcc/testsuite/g++.dg/coroutines/pr93458-2-bad-traits.C
b/gcc/testsuite/g++.dg/coroutines/pr93458-2-bad-traits.C
new file mode 100644
index 00000000000..7801656faa1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr93458-2-bad-traits.C
@@ -0,0 +1,16 @@
+// { dg-additional-options "-fsyntax-only -fexceptions -w" }
+
+// Diagose bad traits traits : fake something faulty.
+
+namespace std {
+ // name is present, but not a template.
+ struct coroutine_traits {
+ };
+}
+
+int
+bad_coroutine (void)
+{
+ co_yield 5; // { dg-error {cannot find a valid coroutine traits template
using 'std::coroutine_traits'} }
+ co_return;
+}
diff --git a/gcc/testsuite/g++.dg/coroutines/pr93458-3-missing-handle.C
b/gcc/testsuite/g++.dg/coroutines/pr93458-3-missing-handle.C
new file mode 100644
index 00000000000..caff2a45594
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr93458-3-missing-handle.C
@@ -0,0 +1,17 @@
+// { dg-additional-options "-fsyntax-only -fexceptions -w" }
+
+// Diagose missing coroutine handle class template.
+
+namespace std {
+ // coroutine traits
+ template<typename _R, typename...> struct coroutine_traits {
+ using promise_type = typename _R::promise_type;
+ };
+}
+
+int
+bad_coroutine (void)
+{
+ co_yield 5; // { dg-error {cannot find a valid coroutine handle class
template using 'std::coroutine_handle'} }
+ co_return;
+}
diff --git a/gcc/testsuite/g++.dg/coroutines/pr93458-4-bad-coro-handle.C
b/gcc/testsuite/g++.dg/coroutines/pr93458-4-bad-coro-handle.C
new file mode 100644
index 00000000000..7bb4f2de9e2
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr93458-4-bad-coro-handle.C
@@ -0,0 +1,21 @@
+// { dg-additional-options "-fsyntax-only -fexceptions -w" }
+
+// Diagose missing coroutine handle class template.
+
+namespace std {
+ // coroutine traits
+ template<typename _R, typename...> struct coroutine_traits {
+ using promise_type = typename _R::promise_type;
+ };
+
+ // name is present, but not a template.
+ struct coroutine_handle {
+ };
+}
+
+int
+bad_coroutine (void)
+{
+ co_yield 5; // { dg-error {cannot find a valid coroutine handle class
template using 'std::coroutine_handle'} }
+ co_return;
+}
diff --git a/gcc/testsuite/g++.dg/coroutines/pr93458-5-bad-coro-type.C
b/gcc/testsuite/g++.dg/coroutines/pr93458-5-bad-coro-type.C
new file mode 100644
index 00000000000..8bb58cc0a78
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr93458-5-bad-coro-type.C
@@ -0,0 +1,12 @@
+// { dg-additional-options "-fsyntax-only -fexceptions -w" }
+
+// Diagose bad coroutine function type.
+
+#include "coro.h"
+
+int
+bad_coroutine (void) // { dg-error {coroutine return type 'int' is not a
class} }
+{
+ co_yield 5;
+ co_return;
+}
--
2.24.1
