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

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.

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

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;
+}

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

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.

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

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;
+}

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;
+}

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

>