coroutines: Fix ICE on invalid (PR93458).

Message ID D4FBEF18-5C3C-47CC-B75D-A6370A2782AF@sandoe.co.uk
State New
Headers show
Series
  • coroutines: Fix ICE on invalid (PR93458).
Related show

Commit Message

Iain Sandoe Jan. 28, 2020, 4:14 p.m.
Hi,

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 (nor tolerating that level of fail for subsequent diagnostics).

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).

tested on x86_64-darwin16,
OK for trunk?
thanks
iain

gcc/cp/ChangeLog:

2020-01-28  Iain Sandoe  <iain@sandoe.co.uk>

	* coroutines.cc (get_coroutine_info): Tolerate fatal errors that might
	leave the info unset.
	(find_coro_traits_template_decl): Note we emitted the error and
	suppress duplicates.
	(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-28  Iain Sandoe  <iain@sandoe.co.uk>

	* g++.dg/coroutines/pr93458-1-missing-traits.C: New test.
	* g++.dg/coroutines/pr93458-2-bad-coro-type.C: New test.
---
 gcc/cp/coroutines.cc                          | 76 ++++++++++++++-----
 .../coroutines/pr93458-1-missing-traits.C     | 10 +++
 .../coroutines/pr93458-2-bad-coro-type.C      | 12 +++
 3 files changed, 78 insertions(+), 20 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-coro-type.C

-- 
2.24.1

Comments

Nathan Sidwell Jan. 29, 2020, 2:39 p.m. | #1
On 1/28/20 11:14 AM, Iain Sandoe wrote:
> Hi,

> 

> 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 (nor tolerating that level of fail for subsequent diagnostics).

> 

> 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).

> 

> tested on x86_64-darwin16,

> OK for trunk?

> thanks

> iain

> 

> gcc/cp/ChangeLog:

> 

> 2020-01-28  Iain Sandoe  <iain@sandoe.co.uk>

> 

> 	* coroutines.cc (get_coroutine_info): Tolerate fatal errors that might

> 	leave the info unset.

> 	(find_coro_traits_template_decl): Note we emitted the error and

> 	suppress duplicates.

> 	(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.



>     coroutine_info **slot = coroutine_info_table->find_slot_with_hash

>       (fn_decl, coroutine_info_hasher::hash (fn_decl), NO_INSERT);

> @@ -255,11 +256,16 @@ static GTY(()) tree void_coro_handle_type;

>   static tree

>   find_coro_traits_template_decl (location_t kw)

>   {

> +  static bool 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)

> +  /* If we are missing fundmental information, such as the traits, then don't

> +     emit this for every keyword in a TU.  */

> +  if (!error_emitted &&

> +      (traits_decl == NULL_TREE || traits_decl == error_mark_node))

>       {

>         error_at (kw, "cannot find %<coroutine traits%> template");

> +      error_emitted = true;

>         return NULL_TREE;

>       }


don't you just want to protect the error_at call with error_emitted? 
Then I think the logic in the else branch will be simpler.  You might 
want to pick a canonical 'error' value -- either NULL_TREE or 
error_mark_node, but not both?

nathan

-- 
Nathan Sidwell

Patch

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index e8a6a4033f6..ca86c7e6950 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -169,7 +169,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 +256,16 @@  static GTY(()) tree void_coro_handle_type;
 static tree
 find_coro_traits_template_decl (location_t kw)
 {
+  static bool 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)
+  /* If we are missing fundmental information, such as the traits, then don't
+     emit this for every keyword in a TU.  */
+  if (!error_emitted &&
+      (traits_decl == NULL_TREE || traits_decl == error_mark_node))
     {
       error_at (kw, "cannot find %<coroutine traits%> template");
+      error_emitted = true;
       return NULL_TREE;
     }
   else
@@ -370,34 +376,45 @@  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
+	  || coro_traits_templ == error_mark_node)
+	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
+	  || coro_handle_templ == error_mark_node)
+	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
+	  || void_coro_handle_type == error_mark_node)
+	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 +424,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, ...>  */
+      if (!CLASS_TYPE_P (TREE_TYPE (TREE_TYPE (fndecl))))
+	{
+	  static bool coro_type_error_emitted = false;
+	  /* 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_type_error_emitted)
+	    error_at (DECL_SOURCE_LOCATION (fndecl), "a coroutine must have a"
+		      " class or struct return type");
+	  coro_type_error_emitted = true;
+	  return false;
+	}
+
       tree templ_class = instantiate_coro_traits (fndecl, loc);
 
       /* Find the promise type for that.  */
@@ -422,7 +452,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)
 	return false;
 
       /* Complete this, we're going to use it.  */
@@ -597,11 +627,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..46adccded98
--- /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 (fail to include <coroutine>).
+
+int
+bad_coroutine (void)
+{
+  co_yield 5; // { dg-error {cannot find 'coroutine traits' template} }
+  co_return;
+}
diff --git a/gcc/testsuite/g++.dg/coroutines/pr93458-2-bad-coro-type.C b/gcc/testsuite/g++.dg/coroutines/pr93458-2-bad-coro-type.C
new file mode 100644
index 00000000000..aab9b34b69a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr93458-2-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 {a coroutine must have a class or struct return type} }
+{
+  co_yield 5;
+  co_return;
+}