[C++,Patch/RFC] PR 88987 ("[9 Regression] ICE: unexpected expression '(bool)sm' of kind implicit_conv_expr")

Message ID c26f5fbb-9358-1ab8-7086-1ab667e46f18@oracle.com
State New
Headers show
Series
  • [C++,Patch/RFC] PR 88987 ("[9 Regression] ICE: unexpected expression '(bool)sm' of kind implicit_conv_expr")
Related show

Commit Message

Paolo Carlini Feb. 26, 2019, 10:42 a.m.
Hi,

the issue is rather easy to explain: after Alexandre' change  in 
r266874, which simplified the condition at the beginning of 
build_noexcept_spec to evaluate early all the expressions which aren't 
deferred or value-dependent, we obviously ICE during error-recovery on 
the new testcase because an expression which isn't a potential constant 
filters through and cxx_constant_value can't handle it.  We can avoid 
this in various ways - for example by adding a gate && 
potential_rvalue_constant_expression_p (expr) in the condition at the 
beginning of build_noexcept_spec and adjust/remove the final gcc_assert 
(which is already a bit obsolete wrt Alexandre' change). Or we can 
handle this earlier, in cp_parser_noexcept_specification_opt - in 
complete analogy, with, say, cp_parser_initializer_list - thus don't let 
through those expressions at all (possible variant: set expr = 
error_mark_node), which has the advantage of avoiding a duplicate 
potential_rvalue_constant_expression call (note: in the parser 
build_no_except_spec is called only by cp_parser_noexcept_specification_opt)

All those variants pass the testsuite on x86_64-linux.

Thanks, Paolo.

////////////////////////////

Comments

Jason Merrill Feb. 26, 2019, 2:33 p.m. | #1
On 2/26/19 5:42 AM, Paolo Carlini wrote:
> Hi,

> 

> the issue is rather easy to explain: after Alexandre' change  in 

> r266874, which simplified the condition at the beginning of 

> build_noexcept_spec to evaluate early all the expressions which aren't 

> deferred or value-dependent, we obviously ICE during error-recovery on 

> the new testcase because an expression which isn't a potential constant 

> filters through and cxx_constant_value can't handle it.  We can avoid 

> this in various ways - for example by adding a gate && 

> potential_rvalue_constant_expression_p (expr) in the condition at the 

> beginning of build_noexcept_spec and adjust/remove the final gcc_assert 

> (which is already a bit obsolete wrt Alexandre' change). Or we can 

> handle this earlier, in cp_parser_noexcept_specification_opt - in 

> complete analogy, with, say, cp_parser_initializer_list - thus don't let 

> through those expressions at all (possible variant: set expr = 

> error_mark_node), which has the advantage of avoiding a duplicate 

> potential_rvalue_constant_expression call (note: in the parser 

> build_no_except_spec is called only by 

> cp_parser_noexcept_specification_opt)

> 

> All those variants pass the testsuite on x86_64-linux.


What happens if 'sm' is constexpr?

Jason
Paolo Carlini Feb. 26, 2019, 3:22 p.m. | #2
Hi,

On 26/02/19 15:33, Jason Merrill wrote:
> On 2/26/19 5:42 AM, Paolo Carlini wrote:

>> Hi,

>>

>> the issue is rather easy to explain: after Alexandre' change  in 

>> r266874, which simplified the condition at the beginning of 

>> build_noexcept_spec to evaluate early all the expressions which 

>> aren't deferred or value-dependent, we obviously ICE during 

>> error-recovery on the new testcase because an expression which isn't 

>> a potential constant filters through and cxx_constant_value can't 

>> handle it.  We can avoid this in various ways - for example by adding 

>> a gate && potential_rvalue_constant_expression_p (expr) in the 

>> condition at the beginning of build_noexcept_spec and adjust/remove 

>> the final gcc_assert (which is already a bit obsolete wrt Alexandre' 

>> change). Or we can handle this earlier, in 

>> cp_parser_noexcept_specification_opt - in complete analogy, with, 

>> say, cp_parser_initializer_list - thus don't let through those 

>> expressions at all (possible variant: set expr = error_mark_node), 

>> which has the advantage of avoiding a duplicate 

>> potential_rvalue_constant_expression call (note: in the parser 

>> build_no_except_spec is called only by 

>> cp_parser_noexcept_specification_opt)

>>

>> All those variants pass the testsuite on x86_64-linux.

> What happens if 'sm' is constexpr?


Well, if 'sm' is constexpr we accept the snippet, as we should, AFAICS. 
This is true for all my tentative patches, I think, certainly for what I 
posted.

Note, this is supposed to be only an error-recovery issue.

Paolo.
Jason Merrill Feb. 26, 2019, 9:18 p.m. | #3
On 2/26/19 10:22 AM, Paolo Carlini wrote:
> Hi,

> 

> On 26/02/19 15:33, Jason Merrill wrote:

>> On 2/26/19 5:42 AM, Paolo Carlini wrote:

>>> Hi,

>>>

>>> the issue is rather easy to explain: after Alexandre' change  in 

>>> r266874, which simplified the condition at the beginning of 

>>> build_noexcept_spec to evaluate early all the expressions which 

>>> aren't deferred or value-dependent, we obviously ICE during 

>>> error-recovery on the new testcase because an expression which isn't 

>>> a potential constant filters through and cxx_constant_value can't 

>>> handle it.  We can avoid this in various ways - for example by adding 

>>> a gate && potential_rvalue_constant_expression_p (expr) in the 

>>> condition at the beginning of build_noexcept_spec and adjust/remove 

>>> the final gcc_assert (which is already a bit obsolete wrt Alexandre' 

>>> change). Or we can handle this earlier, in 

>>> cp_parser_noexcept_specification_opt - in complete analogy, with, 

>>> say, cp_parser_initializer_list - thus don't let through those 

>>> expressions at all (possible variant: set expr = error_mark_node), 

>>> which has the advantage of avoiding a duplicate 

>>> potential_rvalue_constant_expression call (note: in the parser 

>>> build_no_except_spec is called only by 

>>> cp_parser_noexcept_specification_opt)

>>>

>>> All those variants pass the testsuite on x86_64-linux.

>> What happens if 'sm' is constexpr?

> 

> Well, if 'sm' is constexpr we accept the snippet, as we should, AFAICS. 

> This is true for all my tentative patches, I think, certainly for what I 

> posted.


Yes, we should.  Then the patch is OK.

Jason

Patch

Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 269187)
+++ cp/parser.c	(working copy)
@@ -25143,7 +25143,17 @@  cp_parser_noexcept_specification_opt (cp_parser* p
 	      parser->type_definition_forbidden_message
 	      = G_("types may not be defined in an exception-specification");
 
-	      expr = cp_parser_constant_expression (parser);
+	      bool non_constant_p;
+	      expr
+		= cp_parser_constant_expression (parser,
+						 /*allow_non_constant=*/true,
+						 &non_constant_p);
+	      if (non_constant_p
+		  && !require_potential_rvalue_constant_expression (expr))
+		{
+		  expr = NULL_TREE;
+		  return_cond = true;
+		}
 
 	      /* Restore the saved message.  */
 	      parser->type_definition_forbidden_message = saved_message;
Index: testsuite/g++.dg/cpp0x/pr88987.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr88987.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/pr88987.C	(working copy)
@@ -0,0 +1,9 @@ 
+// { dg-do compile { target c++11 } }
+
+int sm;
+
+template <typename T> T
+pk () noexcept (sm)  // { dg-error "constant expression" }
+{
+  return 0;
+}