[C++] PR 85112 ("[8 Regression] ICE with invalid constexpr")

Message ID 800d19a8-a85a-d35c-f97e-e5999bf99467@oracle.com
State New
Headers show
Series
  • [C++] PR 85112 ("[8 Regression] ICE with invalid constexpr")
Related show

Commit Message

Paolo Carlini April 13, 2018, 9:05 a.m.
Hi,

in this error-recovery regression, after a sensible error message issued 
by cxx_constant_init, store_init_value stores an error_mark_node as 
DECL_INITIAL of the VAR_DECL for 'j'. This error_mark_node reappears 
much later, to cause a crash during gimplification. As far as I know, 
the choice of storing error_mark_nodes too is the outcome of a rather 
delicate error-recovery strategy and I don't think we want to revisit it 
at this time, thus the remaining option is catching later the 
error_mark_node, at a "good" time. I note, in passing, that the do loop 
in gimplify_expr which uses the crashing STRIP_USELESS_TYPE_CONVERSION 
seems a bit lacking from the error-recovery point of view, because at 
each iteration it *does* cover for error_operand_p (save_expr) but only 
immediately after the call, when it's too late.

All the above said, I believe that at least for the 8.1.0 needs we may 
want to catch the error_mark_node in cp_build_modify_expr, when we are 
handling the assignment 'a.n = j;': convert_for_assignment produces a 
NOP_EXPR from the VAR_DECL for 'j' which then cp_convert_and_check 
regenerates (deep in convert_to_integer_1 via maybe_fold_build1_loc) in 
the final bare-bone form, with the error_mark_node as the first operand. 
Passes testing on x86_64-linux.

Thanks, Paolo.

/////////////////////////
/cp
2018-04-13  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/85112
	* typeck.c (cp_build_modify_expr): Return error_mark_node upon
	an error_mark_node wrapped in a NOP_EXPR too.

/testsuite
2018-04-13  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/85112
	* g++.dg/cpp0x/pr85112.C: New.

Comments

Jason Merrill April 13, 2018, 2:06 p.m. | #1
On Fri, Apr 13, 2018 at 5:05 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,

>

> in this error-recovery regression, after a sensible error message issued by

> cxx_constant_init, store_init_value stores an error_mark_node as

> DECL_INITIAL of the VAR_DECL for 'j'. This error_mark_node reappears much

> later, to cause a crash during gimplification. As far as I know, the choice

> of storing error_mark_nodes too is the outcome of a rather delicate

> error-recovery strategy and I don't think we want to revisit it at this

> time, thus the remaining option is catching later the error_mark_node, at a

> "good" time. I note, in passing, that the do loop in gimplify_expr which

> uses the crashing STRIP_USELESS_TYPE_CONVERSION seems a bit lacking from the

> error-recovery point of view, because at each iteration it *does* cover for

> error_operand_p (save_expr) but only immediately after the call, when it's

> too late.

>

> All the above said, I believe that at least for the 8.1.0 needs we may want

> to catch the error_mark_node in cp_build_modify_expr, when we are handling

> the assignment 'a.n = j;': convert_for_assignment produces a NOP_EXPR from

> the VAR_DECL for 'j' which then cp_convert_and_check regenerates (deep in

> convert_to_integer_1 via maybe_fold_build1_loc) in the final bare-bone form,

> with the error_mark_node as the first operand. Passes testing on

> x86_64-linux.


We should avoid wrapping an error_mark_node in a NOP_EXPR in the first place.

Jason
Paolo Carlini April 13, 2018, 5:05 p.m. | #2
Hi,

On 13/04/2018 16:06, Jason Merrill wrote:
> On Fri, Apr 13, 2018 at 5:05 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:

>> Hi,

>>

>> in this error-recovery regression, after a sensible error message issued by

>> cxx_constant_init, store_init_value stores an error_mark_node as

>> DECL_INITIAL of the VAR_DECL for 'j'. This error_mark_node reappears much

>> later, to cause a crash during gimplification. As far as I know, the choice

>> of storing error_mark_nodes too is the outcome of a rather delicate

>> error-recovery strategy and I don't think we want to revisit it at this

>> time, thus the remaining option is catching later the error_mark_node, at a

>> "good" time. I note, in passing, that the do loop in gimplify_expr which

>> uses the crashing STRIP_USELESS_TYPE_CONVERSION seems a bit lacking from the

>> error-recovery point of view, because at each iteration it *does* cover for

>> error_operand_p (save_expr) but only immediately after the call, when it's

>> too late.

>>

>> All the above said, I believe that at least for the 8.1.0 needs we may want

>> to catch the error_mark_node in cp_build_modify_expr, when we are handling

>> the assignment 'a.n = j;': convert_for_assignment produces a NOP_EXPR from

>> the VAR_DECL for 'j' which then cp_convert_and_check regenerates (deep in

>> convert_to_integer_1 via maybe_fold_build1_loc) in the final bare-bone form,

>> with the error_mark_node as the first operand. Passes testing on

>> x86_64-linux.

> We should avoid wrapping an error_mark_node in a NOP_EXPR in the first place.

Basing on my analysis, that's easy to do, in convert_to_integer_1. I 
wasn't sure we wanted to touch such basic facilities ;) 
Implementation-wise, I wondered whether we wanted to handle NOP_EXPR and 
error_mark_node specially inside build1 itself, but, again, that seems a 
bit invasive, plus all the build* facilities always allocate a new node 
as the first step. Anyway, fully testing the below will require a bit of 
time, shall I go ahead with that, given that g++.dg passed?

Thanks!
Paolo.

////////////////////////
Index: convert.c
===================================================================
--- convert.c	(revision 259376)
+++ convert.c	(working copy)
@@ -743,6 +743,8 @@ convert_to_integer_1 (tree type, tree expr, bool d
 	{
 	  expr = convert (lang_hooks.types.type_for_mode
 			  (TYPE_MODE (type), TYPE_UNSIGNED (type)), expr);
+	  if (expr == error_mark_node)
+	    return error_mark_node;
 	  return maybe_fold_build1_loc (dofold, loc, NOP_EXPR, type, expr);
 	}
 
Index: testsuite/g++.dg/cpp0x/pr85112.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr85112.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/pr85112.C	(working copy)
@@ -0,0 +1,17 @@
+// PR c++/85112
+// { dg-do compile { target c++11 } }
+
+struct A
+{
+  int m;
+  int n : 4;
+};
+
+int i;  // { dg-message "not const" }
+
+void foo()
+{
+  constexpr int j = i;  // { dg-error "not usable" }
+  A a;
+  a.n = j;
+}
Jason Merrill April 13, 2018, 5:14 p.m. | #3
On Fri, Apr 13, 2018 at 1:05 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,

>

> On 13/04/2018 16:06, Jason Merrill wrote:

>>

>> On Fri, Apr 13, 2018 at 5:05 AM, Paolo Carlini <paolo.carlini@oracle.com>

>> wrote:

>>>

>>> Hi,

>>>

>>> in this error-recovery regression, after a sensible error message issued

>>> by

>>> cxx_constant_init, store_init_value stores an error_mark_node as

>>> DECL_INITIAL of the VAR_DECL for 'j'. This error_mark_node reappears much

>>> later, to cause a crash during gimplification. As far as I know, the

>>> choice

>>> of storing error_mark_nodes too is the outcome of a rather delicate

>>> error-recovery strategy and I don't think we want to revisit it at this

>>> time, thus the remaining option is catching later the error_mark_node, at

>>> a

>>> "good" time. I note, in passing, that the do loop in gimplify_expr which

>>> uses the crashing STRIP_USELESS_TYPE_CONVERSION seems a bit lacking from

>>> the

>>> error-recovery point of view, because at each iteration it *does* cover

>>> for

>>> error_operand_p (save_expr) but only immediately after the call, when

>>> it's

>>> too late.

>>>

>>> All the above said, I believe that at least for the 8.1.0 needs we may

>>> want

>>> to catch the error_mark_node in cp_build_modify_expr, when we are

>>> handling

>>> the assignment 'a.n = j;': convert_for_assignment produces a NOP_EXPR

>>> from

>>> the VAR_DECL for 'j' which then cp_convert_and_check regenerates (deep in

>>> convert_to_integer_1 via maybe_fold_build1_loc) in the final bare-bone

>>> form,

>>> with the error_mark_node as the first operand. Passes testing on

>>> x86_64-linux.

>>

>> We should avoid wrapping an error_mark_node in a NOP_EXPR in the first

>> place.

>

> Basing on my analysis, that's easy to do, in convert_to_integer_1.


Are we passing an error_mark_node down into convert_to_integer_1?
Where are we folding away the VAR_DECL?

Jason
Paolo Carlini April 13, 2018, 5:18 p.m. | #4
Hi,

On 13/04/2018 19:14, Jason Merrill wrote:
> On Fri, Apr 13, 2018 at 1:05 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:

>> Hi,

>>

>> On 13/04/2018 16:06, Jason Merrill wrote:

>>> On Fri, Apr 13, 2018 at 5:05 AM, Paolo Carlini <paolo.carlini@oracle.com>

>>> wrote:

>>>> Hi,

>>>>

>>>> in this error-recovery regression, after a sensible error message issued

>>>> by

>>>> cxx_constant_init, store_init_value stores an error_mark_node as

>>>> DECL_INITIAL of the VAR_DECL for 'j'. This error_mark_node reappears much

>>>> later, to cause a crash during gimplification. As far as I know, the

>>>> choice

>>>> of storing error_mark_nodes too is the outcome of a rather delicate

>>>> error-recovery strategy and I don't think we want to revisit it at this

>>>> time, thus the remaining option is catching later the error_mark_node, at

>>>> a

>>>> "good" time. I note, in passing, that the do loop in gimplify_expr which

>>>> uses the crashing STRIP_USELESS_TYPE_CONVERSION seems a bit lacking from

>>>> the

>>>> error-recovery point of view, because at each iteration it *does* cover

>>>> for

>>>> error_operand_p (save_expr) but only immediately after the call, when

>>>> it's

>>>> too late.

>>>>

>>>> All the above said, I believe that at least for the 8.1.0 needs we may

>>>> want

>>>> to catch the error_mark_node in cp_build_modify_expr, when we are

>>>> handling

>>>> the assignment 'a.n = j;': convert_for_assignment produces a NOP_EXPR

>>>> from

>>>> the VAR_DECL for 'j' which then cp_convert_and_check regenerates (deep in

>>>> convert_to_integer_1 via maybe_fold_build1_loc) in the final bare-bone

>>>> form,

>>>> with the error_mark_node as the first operand. Passes testing on

>>>> x86_64-linux.

>>> We should avoid wrapping an error_mark_node in a NOP_EXPR in the first

>>> place.

>> Basing on my analysis, that's easy to do, in convert_to_integer_1.

> Are we passing an error_mark_node down into convert_to_integer_1?

> Where are we folding away the VAR_DECL?

That convert gets a NOP_EXPR with the VAR_DECL for 'j' as argument and 
returns the error_mark_node.

Paolo.
Jason Merrill April 13, 2018, 5:45 p.m. | #5
On Fri, Apr 13, 2018 at 1:18 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,

>

>

> On 13/04/2018 19:14, Jason Merrill wrote:

>>

>> On Fri, Apr 13, 2018 at 1:05 PM, Paolo Carlini <paolo.carlini@oracle.com>

>> wrote:

>>>

>>> Hi,

>>>

>>> On 13/04/2018 16:06, Jason Merrill wrote:

>>>>

>>>> On Fri, Apr 13, 2018 at 5:05 AM, Paolo Carlini

>>>> <paolo.carlini@oracle.com>

>>>> wrote:

>>>>>

>>>>> Hi,

>>>>>

>>>>> in this error-recovery regression, after a sensible error message

>>>>> issued

>>>>> by

>>>>> cxx_constant_init, store_init_value stores an error_mark_node as

>>>>> DECL_INITIAL of the VAR_DECL for 'j'. This error_mark_node reappears

>>>>> much

>>>>> later, to cause a crash during gimplification. As far as I know, the

>>>>> choice

>>>>> of storing error_mark_nodes too is the outcome of a rather delicate

>>>>> error-recovery strategy and I don't think we want to revisit it at this

>>>>> time, thus the remaining option is catching later the error_mark_node,

>>>>> at

>>>>> a

>>>>> "good" time. I note, in passing, that the do loop in gimplify_expr

>>>>> which

>>>>> uses the crashing STRIP_USELESS_TYPE_CONVERSION seems a bit lacking

>>>>> from

>>>>> the

>>>>> error-recovery point of view, because at each iteration it *does* cover

>>>>> for

>>>>> error_operand_p (save_expr) but only immediately after the call, when

>>>>> it's

>>>>> too late.

>>>>>

>>>>> All the above said, I believe that at least for the 8.1.0 needs we may

>>>>> want

>>>>> to catch the error_mark_node in cp_build_modify_expr, when we are

>>>>> handling

>>>>> the assignment 'a.n = j;': convert_for_assignment produces a NOP_EXPR

>>>>> from

>>>>> the VAR_DECL for 'j' which then cp_convert_and_check regenerates (deep

>>>>> in

>>>>> convert_to_integer_1 via maybe_fold_build1_loc) in the final bare-bone

>>>>> form,

>>>>> with the error_mark_node as the first operand. Passes testing on

>>>>> x86_64-linux.

>>>>

>>>> We should avoid wrapping an error_mark_node in a NOP_EXPR in the first

>>>> place.

>>>

>>> Basing on my analysis, that's easy to do, in convert_to_integer_1.

>>

>> Are we passing an error_mark_node down into convert_to_integer_1?

>> Where are we folding away the VAR_DECL?

>

> That convert gets a NOP_EXPR with the VAR_DECL for 'j' as argument and

> returns the error_mark_node.


Ah, I see.  The problem is that even though convert_to_integer_1 was
called with dofold false, we lose that when it calls convert().  Why
not recurse directly to convert_to_integer_1 with the underlying type?
 That would avoid the undesired folding.

Jason
Paolo Carlini April 13, 2018, 10:12 p.m. | #6
Hi,

On 13/04/2018 19:45, Jason Merrill wrote:
> Ah, I see.  The problem is that even though convert_to_integer_1 was

> called with dofold false, we lose that when it calls convert().  Why

> not recurse directly to convert_to_integer_1 with the underlying type?

>   That would avoid the undesired folding.

Interesting. I had no idea that this seemingly simple error-recovery 
issue was ultimately due to a substantive if latent issue in convert.c. 
Anyway, in the meanwhile I tried a few variants of direct recursion 
without much success, my boostraps all failed pretty quickly and pretty 
badly. However the below - which isn't far from the spirit of your 
analysis, I think - is holding up well, at least bootstrap + C++ 
testsuite are definitely OK. Should I continue testing it over the weekend?

Thanks!
Paolo.

///////////////////
Index: convert.c
===================================================================
--- convert.c	(revision 259376)
+++ convert.c	(working copy)
@@ -741,8 +741,12 @@ convert_to_integer_1 (tree type, tree expr, bool d
       else if (TREE_CODE (type) == ENUMERAL_TYPE
 	       || maybe_ne (outprec, GET_MODE_PRECISION (TYPE_MODE (type))))
 	{
-	  expr = convert (lang_hooks.types.type_for_mode
-			  (TYPE_MODE (type), TYPE_UNSIGNED (type)), expr);
+	  tree utype = (lang_hooks.types.type_for_mode
+			(TYPE_MODE (type), TYPE_UNSIGNED (type)));
+	  if (dofold)
+	    expr = convert (utype, expr);
+	  else
+	    expr = build1 (CONVERT_EXPR, utype, expr);
 	  return maybe_fold_build1_loc (dofold, loc, NOP_EXPR, type, expr);
 	}
 
Index: testsuite/g++.dg/cpp0x/pr85112.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr85112.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/pr85112.C	(working copy)
@@ -0,0 +1,17 @@
+// PR c++/85112
+// { dg-do compile { target c++11 } }
+
+struct A
+{
+  int m;
+  int n : 4;
+};
+
+int i;  // { dg-message "not const" }
+
+void foo()
+{
+  constexpr int j = i;  // { dg-error "not usable" }
+  A a;
+  a.n = j;
+}
Paolo Carlini April 13, 2018, 11:53 p.m. | #7
Hi again,

On 14/04/2018 00:12, Paolo Carlini wrote:
> Hi,

>

> On 13/04/2018 19:45, Jason Merrill wrote:

>> Ah, I see.  The problem is that even though convert_to_integer_1 was

>> called with dofold false, we lose that when it calls convert(). Why

>> not recurse directly to convert_to_integer_1 with the underlying type?

>>   That would avoid the undesired folding.

> Interesting. I had no idea that this seemingly simple error-recovery 

> issue was ultimately due to a substantive if latent issue in 

> convert.c. Anyway, in the meanwhile I tried a few variants of direct 

> recursion without much success, my boostraps all failed pretty quickly 

> and pretty badly. However the below - which isn't far from the spirit 

> of your analysis, I think - is holding up well, at least bootstrap + 

> C++ testsuite are definitely OK. Should I continue testing it over the 

> weekend?

The below seems much better, also bootstraps fine and I'm now fully 
testing it.

Thanks,
Paolo.

PS: no idea why earlier today I wasted a lot of time trying to 
completely avoid maybe_fold_build1_loc :(

/////////////////////////
Index: convert.c
===================================================================
--- convert.c	(revision 259376)
+++ convert.c	(working copy)
@@ -741,8 +741,10 @@ convert_to_integer_1 (tree type, tree expr, bool d
       else if (TREE_CODE (type) == ENUMERAL_TYPE
 	       || maybe_ne (outprec, GET_MODE_PRECISION (TYPE_MODE (type))))
 	{
-	  expr = convert (lang_hooks.types.type_for_mode
-			  (TYPE_MODE (type), TYPE_UNSIGNED (type)), expr);
+	  expr
+	    = convert_to_integer_1 (lang_hooks.types.type_for_mode
+				    (TYPE_MODE (type), TYPE_UNSIGNED (type)),
+				    expr, dofold);
 	  return maybe_fold_build1_loc (dofold, loc, NOP_EXPR, type, expr);
 	}
 
Index: testsuite/g++.dg/cpp0x/pr85112.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr85112.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/pr85112.C	(working copy)
@@ -0,0 +1,17 @@
+// PR c++/85112
+// { dg-do compile { target c++11 } }
+
+struct A
+{
+  int m;
+  int n : 4;
+};
+
+int i;  // { dg-message "not const" }
+
+void foo()
+{
+  constexpr int j = i;  // { dg-error "not usable" }
+  A a;
+  a.n = j;
+}
Jason Merrill April 14, 2018, 12:10 a.m. | #8
On Fri, Apr 13, 2018, 7:53 PM Paolo Carlini <paolo.carlini@oracle.com>
wrote:

> Hi again,

>

> On 14/04/2018 00:12, Paolo Carlini wrote:

> > Hi,

> >

> > On 13/04/2018 19:45, Jason Merrill wrote:

> >> Ah, I see.  The problem is that even though convert_to_integer_1 was

> >> called with dofold false, we lose that when it calls convert(). Why

> >> not recurse directly to convert_to_integer_1 with the underlying type?

> >>   That would avoid the undesired folding.

> > Interesting. I had no idea that this seemingly simple error-recovery

> > issue was ultimately due to a substantive if latent issue in

> > convert.c. Anyway, in the meanwhile I tried a few variants of direct

> > recursion without much success, my boostraps all failed pretty quickly

> > and pretty badly. However the below - which isn't far from the spirit

> > of your analysis, I think - is holding up well, at least bootstrap +

> > C++ testsuite are definitely OK. Should I continue testing it over the

> > weekend?

> The below seems much better, also bootstraps fine and I'm now fully

> testing it.

>

> Thanks,

> Paolo.

>

> PS: no idea why earlier today I wasted a lot of time trying to

> completely avoid maybe_fold_build1_loc :(

>


Ok if testing passes.

Jason

>

Patch

Index: cp/typeck.c
===================================================================
--- cp/typeck.c	(revision 259359)
+++ cp/typeck.c	(working copy)
@@ -8234,7 +8234,9 @@  cp_build_modify_expr (location_t loc, tree lhs, en
 			 TREE_OPERAND (newrhs, 0));
     }
 
-  if (newrhs == error_mark_node)
+  if (newrhs == error_mark_node
+      || (TREE_CODE (newrhs) == NOP_EXPR
+	  && TREE_OPERAND (newrhs, 0) == error_mark_node))
     return error_mark_node;
 
   if (c_dialect_objc () && flag_objc_gc)
Index: testsuite/g++.dg/cpp0x/pr85112.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr85112.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/pr85112.C	(working copy)
@@ -0,0 +1,17 @@ 
+// PR c++/85112
+// { dg-do compile { target c++11 } }
+
+struct A
+{
+  int m;
+  int n : 4;
+};
+
+int i;  // { dg-message "not const" }
+
+void foo()
+{
+  constexpr int j = i;  // { dg-error "not usable" }
+  A a;
+  a.n = j;
+}