[C++] PR 89571 ("[9 Regression] ICE in nothrow_spec_p, at cp/except.c:1238")

Hi,

this is just an error recovery ICE but maybe we can fix it in time for 
9.1.0, like c++/89488, which is somehow related. Indeed, the problem is 
again a !DEFERRED_NOEXCEPT_SPEC_P gcc_assert triggering where, earlier, 
process_subob_fn had maybe_instantiate_noexcept returning false (in 
c++/89448 the assertion triggered almost immediately, when 
merge_exception_specifiers was called).

Anyway, what we committed for c++/89488 doesn't help here because the 
error_mark_node assigned to *spec_p is propagated back only up to 
build_over_call, where mark_used returns false but isn't acted upon 
because of the additional && !(complain & tf_error). Thus I had the idea 
of simply removing the latter (by itself a bit invasive at this stage) 
but that leads to duplicated diagnostics because then, in 
cp_parser_decltype_expr, when cp_parser_postfix_expression returns 
error_mark_node we go on and try a full expression too, and emit the 
same diagnostics again.

Given the above we can imagine reworking the parser, which seems to me a 
bit tricky at this time, or we can go back to my initial proposal for 
c++/89488, attached below, which doesn't set *spec_p to error_mark_node 
when maybe_instantiate_noexcept returns false. My rationale being that, 
elsewhere, we very often discard completely the return value of 
maybe_instantiate_noexcept, thus it doesn't seem a big deal only using 
it to avoid immediately calling marge_exception_specifiers and ICE. If 
we do this, for c++/89448 we emit the additional "cannot convert" error, 
which may not be a bad thing, given that we used to emit it forever, and 
the same do both clang and edg, thus it looks like considering that 'zl 
()' not completely broken may make sense for error-recovery purposes...

Tested x86_64-linux.

Thanks, Paolo.

///////////////////////////////
/cp
2019-03-12  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/89571
	* method.c (process_subob_fn): Do not set *spec_p to error_mark_node
	when maybe_instantiate_noexcept returns false.

/testsuite
2019-03-12  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/89571
	* g++.dg/cpp0x/noexcept36.C: New.
	* g++.dg/cpp0x/nsdmi15.C: Adjust.

On 3/12/19 7:21 AM, Paolo Carlini wrote:
> Hi,
> 
> this is just an error recovery ICE but maybe we can fix it in time for 
> 9.1.0, like c++/89488, which is somehow related. Indeed, the problem is 
> again a !DEFERRED_NOEXCEPT_SPEC_P gcc_assert triggering where, earlier, 
> process_subob_fn had maybe_instantiate_noexcept returning false (in 
> c++/89448 the assertion triggered almost immediately, when 
> merge_exception_specifiers was called).
> 
> Anyway, what we committed for c++/89488 doesn't help here because the 
> error_mark_node assigned to *spec_p is propagated back only up to 
> build_over_call, where mark_used returns false but isn't acted upon 
> because of the additional && !(complain & tf_error). Thus I had the idea 
> of simply removing the latter (by itself a bit invasive at this stage) 
> but that leads to duplicated diagnostics because then, in 
> cp_parser_decltype_expr, when cp_parser_postfix_expression returns 
> error_mark_node we go on and try a full expression too, and emit the 
> same diagnostics again.
> 
> Given the above we can imagine reworking the parser, which seems to me a 
> bit tricky at this time, or we can go back to my initial proposal for 
> c++/89488, attached below, which doesn't set *spec_p to error_mark_node 
> when maybe_instantiate_noexcept returns false. My rationale being that, 
> elsewhere, we very often discard completely the return value of 
> maybe_instantiate_noexcept, thus it doesn't seem a big deal only using 
> it to avoid immediately calling marge_exception_specifiers and ICE. If 
> we do this, for c++/89448 we emit the additional "cannot convert" error, 
> which may not be a bad thing, given that we used to emit it forever, and 
> the same do both clang and edg, thus it looks like considering that 'zl 
> ()' not completely broken may make sense for error-recovery purposes...

The thing is, we need to defer instantiation of a noexcept-specifier 
that depends on a DMI we haven't parsed yet, since it can succeed later 
and we don't want to have given up and said false.

But other cases of failed instantiation we can and should diagnose once 
and then recover from.  What do you think about this approach?
commit 9b633e74683e5b22102b4bc6811cdb05756602b0
Author: Jason Merrill <jason@redhat.com>
Date:   Thu Mar 14 14:10:37 2019 -0400

            * pt.c (maybe_instantiate_noexcept): Only return false for defaulted.
            (regenerate_decl_from_template): Use it for noexcept-specs.

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 2ab6e273c3a..9367d49629c 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -23991,12 +23991,17 @@ regenerate_decl_from_template (tree decl, tree tmpl, tree args)
       if (args_depth > parms_depth)
 	args = get_innermost_template_args (args, parms_depth);
 
-      specs = tsubst_exception_specification (TREE_TYPE (code_pattern),
-					      args, tf_error, NULL_TREE,
-					      /*defer_ok*/false);
-      if (specs && specs != error_mark_node)
-	TREE_TYPE (decl) = build_exception_variant (TREE_TYPE (decl),
-						    specs);
+      /* Instantiate a dynamic exception-specification.  */
+      if (tree raises = TYPE_RAISES_EXCEPTIONS (TREE_TYPE (code_pattern)))
+	if (TREE_VALUE (raises))
+	  {
+	    specs = tsubst_exception_specification (TREE_TYPE (code_pattern),
+						    args, tf_error, NULL_TREE,
+						    /*defer_ok*/false);
+	    if (specs && specs != error_mark_node)
+	      TREE_TYPE (decl) = build_exception_variant (TREE_TYPE (decl),
+							  specs);
+	  }
 
       /* Merge parameter declarations.  */
       decl_parm = skip_artificial_parms_for (decl,
@@ -24062,6 +24067,8 @@ regenerate_decl_from_template (tree decl, tree tmpl, tree args)
       if (DECL_DECLARED_INLINE_P (code_pattern)
 	  && !DECL_DECLARED_INLINE_P (decl))
 	DECL_DECLARED_INLINE_P (decl) = 1;
+
+      maybe_instantiate_noexcept (decl, tf_error);
     }
   else if (VAR_P (decl))
     {
@@ -24187,7 +24194,11 @@ maybe_instantiate_noexcept (tree fn, tsubst_flags_t complain)
       static hash_set<tree>* fns = new hash_set<tree>;
       bool added = false;
       if (DEFERRED_NOEXCEPT_PATTERN (noex) == NULL_TREE)
-	spec = get_defaulted_eh_spec (fn, complain);
+	{
+	  spec = get_defaulted_eh_spec (fn, complain);
+	  if (spec == error_mark_node)
+	    return false;
+	}
       else if (!(added = !fns->add (fn)))
 	{
 	  /* If hash_set::add returns true, the element was already there.  */
@@ -24247,7 +24258,10 @@ maybe_instantiate_noexcept (tree fn, tsubst_flags_t complain)
 	fns->remove (fn);
 
       if (spec == error_mark_node)
-	return false;
+	{
+	  gcc_assert (seen_error ());
+	  spec = noexcept_false_spec;
+	}
 
       TREE_TYPE (fn) = build_exception_variant (fntype, spec);
     }

Hi,

On 14/03/19 21:03, Jason Merrill wrote:
> On 3/12/19 7:21 AM, Paolo Carlini wrote:
>> Hi,
>>
>> this is just an error recovery ICE but maybe we can fix it in time 
>> for 9.1.0, like c++/89488, which is somehow related. Indeed, the 
>> problem is again a !DEFERRED_NOEXCEPT_SPEC_P gcc_assert triggering 
>> where, earlier, process_subob_fn had maybe_instantiate_noexcept 
>> returning false (in c++/89448 the assertion triggered almost 
>> immediately, when merge_exception_specifiers was called).
>>
>> Anyway, what we committed for c++/89488 doesn't help here because the 
>> error_mark_node assigned to *spec_p is propagated back only up to 
>> build_over_call, where mark_used returns false but isn't acted upon 
>> because of the additional && !(complain & tf_error). Thus I had the 
>> idea of simply removing the latter (by itself a bit invasive at this 
>> stage) but that leads to duplicated diagnostics because then, in 
>> cp_parser_decltype_expr, when cp_parser_postfix_expression returns 
>> error_mark_node we go on and try a full expression too, and emit the 
>> same diagnostics again.
>>
>> Given the above we can imagine reworking the parser, which seems to 
>> me a bit tricky at this time, or we can go back to my initial 
>> proposal for c++/89488, attached below, which doesn't set *spec_p to 
>> error_mark_node when maybe_instantiate_noexcept returns false. My 
>> rationale being that, elsewhere, we very often discard completely the 
>> return value of maybe_instantiate_noexcept, thus it doesn't seem a 
>> big deal only using it to avoid immediately calling 
>> marge_exception_specifiers and ICE. If we do this, for c++/89448 we 
>> emit the additional "cannot convert" error, which may not be a bad 
>> thing, given that we used to emit it forever, and the same do both 
>> clang and edg, thus it looks like considering that 'zl ()' not 
>> completely broken may make sense for error-recovery purposes...
>
> The thing is, we need to defer instantiation of a noexcept-specifier 
> that depends on a DMI we haven't parsed yet, since it can succeed 
> later and we don't want to have given up and said false.
>
> But other cases of failed instantiation we can and should diagnose 
> once and then recover from.  What do you think about this approach?

Oh, nice, I didn't go that far... However, I definitely wanted to spend 
more time on that if (spec ==error_mark_node) return false; which seemed 
at the center of the issues we were having.

Anyway, I'm reassigning the bug to you.

Thanks! Paolo.