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

2020-01-29 Thread Nathan Sidwell

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  

* 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 % 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] coroutines: Fix ICE on invalid (PR93458).

2020-01-28 Thread Iain Sandoe
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  

* 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  

* 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

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 % 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::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