[v3] coroutines: Fix ICE on invalid (PR93458).

Message ID A5C8D76E-E6DA-4E19-BC37-F11F561B1247@sandoe.co.uk
State New
Headers show
Series
  • [v3] coroutines: Fix ICE on invalid (PR93458).
Related show

Commit Message

Iain Sandoe Jan. 30, 2020, 2:43 p.m.
Hi Nathan,

Nathan Sidwell <nathan@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  <iain@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  <iain@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

-- 
2.24.1

Comments

Nathan Sidwell Jan. 31, 2020, 1:22 p.m. | #1
On 1/30/20 9:43 AM, Iain Sandoe wrote:
> Hi Nathan,


> 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'm not sure that's helpful.  I think you should still complain on the 
lookup.  If that returns error_mark_node, you're done.  If it returns 
NULL, does it emit an error?  If not, you should emit a not found error. 
  Finally, if it does return a thing, check if it's a class template 
decl (or alias template I guess --  DECL_TEMPLATE_RESULT being a 
TYPE_DECL is close enough) and if not, error on that (X is not a 
template class).  Then the user's fully clued in.

> 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’”


hm, 'valid'.  If you find a template_decl, but cannot instantiate it, 
that sounds not valid.  But I suspect you do not diagnose that here, 
because in general you cannot :)

sorry to be a pain.

nathan
-- 
Nathan Sidwell
Iain Sandoe Feb. 1, 2020, 11:55 a.m. | #2
Nathan Sidwell <nathan@acm.org> wrote:

> On 1/30/20 9:43 AM, Iain Sandoe wrote:

>> Hi Nathan,

> 

>> 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'm not sure that's helpful.  I think you should still complain on the lookup.  If that returns error_mark_node, you're done.  If it returns NULL, does it emit an error?  If not, you should emit a not found error.  Finally, if it does return a thing, check if it's a class template decl (or alias template I guess --  DECL_TEMPLATE_RESULT being a TYPE_DECL is close enough) and if not, error on that (X is not a template class).  Then the user's fully clued in.

> 

>> 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’”

> 

> hm, 'valid'.  If you find a template_decl, but cannot instantiate it, that sounds not valid.  But I suspect you do not diagnose that here, because in general you cannot :)


OK. So in my mind there are two cases here:

1. The user doesn’t know to, or forgot to, add <coroutine> (or can’t spell it).

- this would trigger the top level error and mean that nothing much is going to work.

2.

2a The user has a corrupted installation and the <coroutine> header is broken.
2b The user has been more creative and tried to invent their own header.

- this might result in many more unexpected pathways leading to error.

———

I have revised the code to:

a) re-enable “complain” on the lookup_qualified_name (providing no error for missing traits has been emitted).
b) I added an inform “perhaps '#include <coroutine>' is missing (I couldn’t get the rich location missing include logic to fire here).
c) the diagnostic now will consist of:
   - any emitted by “lookup_qualified_name()"  (usually, there will be none, since we are supplying std namespace as the scope).
   - "coroutines require a traits template; cannot find ‘std::coroutine_traits’”
   - "note: perhaps ‘#include <coroutine>’ is missing”

I think that deals reasonably with the common case (1) without being too annoying in terms of diagnostic verbosity.

it leaves  “lookup_qualified_name()" reported errors to deal with the less common and more exotic possibilities in (2).

The handle is treated the same way (without the <coroutine> note, since we must have already found or made a usable traits).  In any case, I think bad handle fails only really occur under (2) above.

still open to rewording of error messages, of course,

OK / reword?
thanks
Iain

coroutines: Fix ICE on invalid (PR93458).

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-02-02  Iain Sandoe  <iain@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-02-02  Iain Sandoe  <iain@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                          | 103 +++++++++++++-----
 .../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, 153 insertions(+), 26 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..43cedea6071 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);
@@ -255,11 +258,25 @@ static GTY(()) tree void_coro_handle_type;
 static tree
 find_coro_traits_template_decl (location_t kw)
 {
+  /* 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;
+
   tree traits_decl = lookup_qualified_name (std_node, coro_traits_identifier,
-					    0, true);
-  if (traits_decl == NULL_TREE || traits_decl == error_mark_node)
+					    0,
+					    /*complain=*/!traits_error_emitted);
+  if (traits_decl == error_mark_node
+      || !DECL_TYPE_TEMPLATE_P (traits_decl))
     {
-      error_at (kw, "cannot find %<coroutine traits%> template");
+      if (!traits_error_emitted)
+	{
+	  gcc_rich_location richloc (kw);
+	  error_at (&richloc, "coroutines require a traits template; cannot"
+		    " find %<%E::%E%>", std_node, coro_traits_identifier);
+	  inform (&richloc, "perhaps %<#include <coroutine>%> is missing");
+	  traits_error_emitted = true;
+	}
       return NULL_TREE;
     }
   else
@@ -299,7 +316,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;
@@ -313,11 +330,18 @@ instantiate_coro_traits (tree fndecl, location_t kw)
 static tree
 find_coro_handle_template_decl (location_t kw)
 {
+  /* As for the coroutine traits, this error is per TU, so only emit
+    it once.  */
+  static bool coro_handle_error_emitted = false;
   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, !coro_handle_error_emitted);
+  if (handle_decl == error_mark_node
+      || !DECL_CLASS_TEMPLATE_P (handle_decl))
+    {
+      if (!coro_handle_error_emitted)
+	error_at (kw, "coroutines require a handle class template;"
+		  " cannot find %<%E::%E%>", std_node, coro_handle_identifier);
+      coro_handle_error_emitted = true;
       return NULL_TREE;
     }
   else
@@ -370,34 +394,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 +439,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 +642,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..638a10606f3
--- /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 {coroutines require a traits template; cannot find '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..0466af8e15a
--- /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 {coroutines require a traits template; cannot find '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..766f740fcbd
--- /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 {coroutines require a handle class template; cannot find '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..5d212ace8d4
--- /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 {coroutines require a handle class template; cannot find '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
Nathan Sidwell Feb. 3, 2020, 12:45 p.m. | #3
On 2/1/20 6:55 AM, Iain Sandoe wrote:
> Nathan Sidwell <nathan@acm.org> wrote:

> 

>> On 1/30/20 9:43 AM, Iain Sandoe wrote:

>>> Hi Nathan,

>>

>>> 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'm not sure that's helpful.  I think you should still complain on the lookup.  If that returns error_mark_node, you're done.  If it returns NULL, does it emit an error?  If not, you should emit a not found error.  Finally, if it does return a thing, check if it's a class template decl (or alias template I guess --  DECL_TEMPLATE_RESULT being a TYPE_DECL is close enough) and if not, error on that (X is not a template class).  Then the user's fully clued in.

>>

>>> 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’”

>>

>> hm, 'valid'.  If you find a template_decl, but cannot instantiate it, that sounds not valid.  But I suspect you do not diagnose that here, because in general you cannot :)

> 

> OK. So in my mind there are two cases here:

> 

> 1. The user doesn’t know to, or forgot to, add <coroutine> (or can’t spell it).

> 

> - this would trigger the top level error and mean that nothing much is going to work.

> 

> 2.

> 

> 2a The user has a corrupted installation and the <coroutine> header is broken.

> 2b The user has been more creative and tried to invent their own header.

> 

> - this might result in many more unexpected pathways leading to error.

> 

> ———

> 

> I have revised the code to:

> 

> a) re-enable “complain” on the lookup_qualified_name (providing no error for missing traits has been emitted).

> b) I added an inform “perhaps '#include <coroutine>' is missing (I couldn’t get the rich location missing include logic to fire here).

> c) the diagnostic now will consist of:

>     - any emitted by “lookup_qualified_name()"  (usually, there will be none, since we are supplying std namespace as the scope).

>     - "coroutines require a traits template; cannot find ‘std::coroutine_traits’”

>     - "note: perhaps ‘#include <coroutine>’ is missing”

> 

> I think that deals reasonably with the common case (1) without being too annoying in terms of diagnostic verbosity.

> 

> it leaves  “lookup_qualified_name()" reported errors to deal with the less common and more exotic possibilities in (2).

> 

> The handle is treated the same way (without the <coroutine> note, since we must have already found or made a usable traits).  In any case, I think bad handle fails only really occur under (2) above.

> 

> still open to rewording of error messages, of course,

> 

> OK / reword?


thanks, looks good now.


nathan


-- 
Nathan Sidwell

Patch

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;
+}