[C++] PR 81055 ("[6/7/8 Regression] ICE with invalid initializer for array new")

Message ID 483ea99f-0d5c-2726-b5ed-1525a7919f35@oracle.com
State New
Headers show
Series
  • [C++] PR 81055 ("[6/7/8 Regression] ICE with invalid initializer for array new")
Related show

Commit Message

Paolo Carlini Dec. 20, 2017, 3:37 p.m.
Hi,

in this error recovery regression, after a sensible error produced by 
unqualified_name_lookup_error we ICE much later when 
gimplify_modify_expr encounters a corresponding error_mark_node as 
second argument of a MODIFY_EXPR. I believe we have a very general error 
recovery weakness with errors like unqualified_name_lookup_error and 
functions like cp_parser_initializer_list returning a vec: certainly we 
don't want to give up the parsing too early but then we have to cope 
with error_mark_nodes filtering down and reappearing much later in the 
compilation. The present bug is a rather clear example, but I have seen 
many others in the past: a couple of times I even tried doing something 
about it, but I have yet to figure out something worth sending to the 
mailing list. Anyway, here I'm wondering if at this stage it would make 
sense to handle the error_mark_node in gimplify_modify_expr - I believe 
we do have a couple other cases of such late handling in the gimplifier. 
Tested x86_64-linux.

Thanks, Paolo.

//////////////////////////////
2017-12-20  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/81055
	* gimplify.c (gimplify_modify_expr): When the second operand of
	the expression is error_mark_node immediately return GS_ERROR.

/testsuite
2017-12-20  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/81055
	* g++.dg/cpp0x/new2.C: New.

Comments

Jason Merrill Dec. 21, 2017, 4:04 p.m. | #1
On Wed, Dec 20, 2017 at 10:37 AM, Paolo Carlini
<paolo.carlini@oracle.com> wrote:
> in this error recovery regression, after a sensible error produced by

> unqualified_name_lookup_error we ICE much later when gimplify_modify_expr

> encounters a corresponding error_mark_node as second argument of a

> MODIFY_EXPR. I believe we have a very general error recovery weakness with

> errors like unqualified_name_lookup_error and functions like

> cp_parser_initializer_list returning a vec: certainly we don't want to give

> up the parsing too early but then we have to cope with error_mark_nodes

> filtering down and reappearing much later in the compilation. The present

> bug is a rather clear example, but I have seen many others in the past: a

> couple of times I even tried doing something about it, but I have yet to

> figure out something worth sending to the mailing list. Anyway, here I'm

> wondering if at this stage it would make sense to handle the error_mark_node

> in gimplify_modify_expr - I believe we do have a couple other cases of such

> late handling in the gimplifier. Tested x86_64-linux.


This seems fine, but the front end shouldn't have created such a
MODIFY_EXPR in the first place.  How does this happen?

Jason
Paolo Carlini Dec. 21, 2017, 7:21 p.m. | #2
Hi,

On 21/12/2017 17:04, Jason Merrill wrote:
> On Wed, Dec 20, 2017 at 10:37 AM, Paolo Carlini

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

>> in this error recovery regression, after a sensible error produced by

>> unqualified_name_lookup_error we ICE much later when gimplify_modify_expr

>> encounters a corresponding error_mark_node as second argument of a

>> MODIFY_EXPR. I believe we have a very general error recovery weakness with

>> errors like unqualified_name_lookup_error and functions like

>> cp_parser_initializer_list returning a vec: certainly we don't want to give

>> up the parsing too early but then we have to cope with error_mark_nodes

>> filtering down and reappearing much later in the compilation. The present

>> bug is a rather clear example, but I have seen many others in the past: a

>> couple of times I even tried doing something about it, but I have yet to

>> figure out something worth sending to the mailing list. Anyway, here I'm

>> wondering if at this stage it would make sense to handle the error_mark_node

>> in gimplify_modify_expr - I believe we do have a couple other cases of such

>> late handling in the gimplifier. Tested x86_64-linux.

> This seems fine, but the front end shouldn't have created such a

> MODIFY_EXPR in the first place.  How does this happen?

Thanks for asking: a good cure for my laziness ;)

In fact, it's an INIT_EXPR, created by build_vec_init around line #4402. 
The below works for the testcase and I'm finishing regtesting it. 
Alternately, only setting errors = true when init == error_mark_node, 
which eventually leads anyway to build_vec_init returning 
error_mark_node, also works for the testcase: it's a little lighter, so 
to speak, but less explicit (setting elt_init = error_mark_node leads to 
errors = true too a few lines below).

Thanks!
Paolo.

////////////////////////
/cp
2017-12-21  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/81055
	* init.c (build_vec_init): Avoid building an INIT_EXPR with 
	error_mark_node as second argument.

/testsuite
2017-12-21  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/81055
	* g++.dg/cpp0x/new2.C: New.
Index: cp/init.c
===================================================================
--- cp/init.c	(revision 255946)
+++ cp/init.c	(working copy)
@@ -4399,7 +4399,9 @@ build_vec_init (tree base, tree maxindex, tree ini
 	      if (TREE_CODE (init) == TREE_LIST)
 		init = build_x_compound_expr_from_list (init, ELK_INIT,
 							complain);
-	      elt_init = build2 (INIT_EXPR, type, to, init);
+	      elt_init = (init == error_mark_node
+			  ? error_mark_node
+			  : build2 (INIT_EXPR, type, to, init));
 	    }
 	}
 
Index: testsuite/g++.dg/cpp0x/new2.C
===================================================================
--- testsuite/g++.dg/cpp0x/new2.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/new2.C	(working copy)
@@ -0,0 +1,4 @@
+// PR c++/81055
+// { dg-do compile { target c++11 } }
+
+int** p = new int*[1]{q};  // { dg-error "not declared" }
Jason Merrill Jan. 10, 2018, 3:06 p.m. | #3
OK.

On Thu, Dec 21, 2017 at 2:21 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,

>

> On 21/12/2017 17:04, Jason Merrill wrote:

>>

>> On Wed, Dec 20, 2017 at 10:37 AM, Paolo Carlini

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

>>>

>>> in this error recovery regression, after a sensible error produced by

>>> unqualified_name_lookup_error we ICE much later when gimplify_modify_expr

>>> encounters a corresponding error_mark_node as second argument of a

>>> MODIFY_EXPR. I believe we have a very general error recovery weakness

>>> with

>>> errors like unqualified_name_lookup_error and functions like

>>> cp_parser_initializer_list returning a vec: certainly we don't want to

>>> give

>>> up the parsing too early but then we have to cope with error_mark_nodes

>>> filtering down and reappearing much later in the compilation. The present

>>> bug is a rather clear example, but I have seen many others in the past: a

>>> couple of times I even tried doing something about it, but I have yet to

>>> figure out something worth sending to the mailing list. Anyway, here I'm

>>> wondering if at this stage it would make sense to handle the

>>> error_mark_node

>>> in gimplify_modify_expr - I believe we do have a couple other cases of

>>> such

>>> late handling in the gimplifier. Tested x86_64-linux.

>>

>> This seems fine, but the front end shouldn't have created such a

>> MODIFY_EXPR in the first place.  How does this happen?

>

> Thanks for asking: a good cure for my laziness ;)

>

> In fact, it's an INIT_EXPR, created by build_vec_init around line #4402. The

> below works for the testcase and I'm finishing regtesting it. Alternately,

> only setting errors = true when init == error_mark_node, which eventually

> leads anyway to build_vec_init returning error_mark_node, also works for the

> testcase: it's a little lighter, so to speak, but less explicit (setting

> elt_init = error_mark_node leads to errors = true too a few lines below).

>

> Thanks!

> Paolo.

>

> ////////////////////////

Patch

Index: gimplify.c
===================================================================
--- gimplify.c	(revision 255894)
+++ gimplify.c	(working copy)
@@ -5554,6 +5553,9 @@  gimplify_modify_expr (tree *expr_p, gimple_seq *pr
   gcc_assert (TREE_CODE (*expr_p) == MODIFY_EXPR
 	      || TREE_CODE (*expr_p) == INIT_EXPR);
 
+  if (*from_p == error_mark_node)
+    return GS_ERROR;
+
   /* Trying to simplify a clobber using normal logic doesn't work,
      so handle it here.  */
   if (TREE_CLOBBER_P (*from_p))
Index: testsuite/g++.dg/cpp0x/new2.C
===================================================================
--- testsuite/g++.dg/cpp0x/new2.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/new2.C	(working copy)
@@ -0,0 +1,4 @@ 
+// PR c++/81055
+// { dg-do compile { target c++11 } }
+
+int** p = new int*[1]{q};  // { dg-error "not declared" }