[C++] Fix ICE with return in statement expression in constexpr.c (PR c++/84192)

Message ID 20180216082814.GM5867@tucnak
State New
Headers show
Series
  • [C++] Fix ICE with return in statement expression in constexpr.c (PR c++/84192)
Related show

Commit Message

Jakub Jelinek Feb. 16, 2018, 8:28 a.m.
Hi!

pop_stmt_list, if there is just a single stmt inside statement expression
moves the stmt out of the STATEMENT_LIST (and I think cp_fold does too).
We only initialize jump_target to non-NULL in cxx_eval_statement_list
or for calls, so before we have a chance to diagnose the error of using
an expression with void type, we ICE trying to dereference NULL jump_target.

This can't happen with BREAK_STMT nor CONTINUE_STMT, because they are not
potential constant expressions, and I think can only happen when ctx->quiet
is true, otherwise it should have been diagnosed already before.
If a RETURN_EXPR (or in theory break/continue) appears in a (potential) constant
expression we want to evaluate, not doing anything with jump_target if we
aren't inside a statement list makes sense to me, there is no following
statement to bypass.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2018-02-16  Marek Polacek  <polacek@redhat.com>
	    Jakub Jelinek  <jakub@redhat.com>

	PR c++/84192
	* constexpr.c (cxx_eval_constant_expression) <case RETURN_EXPR>: Don't
	set *jump_target to anything if jump_target is NULL.

	* g++.dg/cpp1y/constexpr-84192.C: New test.


	Jakub

Comments

Jason Merrill Feb. 16, 2018, 1:52 p.m. | #1
On Fri, Feb 16, 2018 at 3:28 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> pop_stmt_list, if there is just a single stmt inside statement expression

> moves the stmt out of the STATEMENT_LIST (and I think cp_fold does too).

> We only initialize jump_target to non-NULL in cxx_eval_statement_list

> or for calls, so before we have a chance to diagnose the error of using

> an expression with void type, we ICE trying to dereference NULL jump_target.

>

> This can't happen with BREAK_STMT nor CONTINUE_STMT, because they are not

> potential constant expressions, and I think can only happen when ctx->quiet

> is true, otherwise it should have been diagnosed already before.

> If a RETURN_EXPR (or in theory break/continue) appears in a (potential) constant

> expression we want to evaluate, not doing anything with jump_target if we

> aren't inside a statement list makes sense to me, there is no following

> statement to bypass.


I think we should also set *non_constant_p.

Jason
Jakub Jelinek Feb. 16, 2018, 3:19 p.m. | #2
On Fri, Feb 16, 2018 at 08:52:10AM -0500, Jason Merrill wrote:
> On Fri, Feb 16, 2018 at 3:28 AM, Jakub Jelinek <jakub@redhat.com> wrote:

> > pop_stmt_list, if there is just a single stmt inside statement expression

> > moves the stmt out of the STATEMENT_LIST (and I think cp_fold does too).

> > We only initialize jump_target to non-NULL in cxx_eval_statement_list

> > or for calls, so before we have a chance to diagnose the error of using

> > an expression with void type, we ICE trying to dereference NULL jump_target.

> >

> > This can't happen with BREAK_STMT nor CONTINUE_STMT, because they are not

> > potential constant expressions, and I think can only happen when ctx->quiet

> > is true, otherwise it should have been diagnosed already before.

> > If a RETURN_EXPR (or in theory break/continue) appears in a (potential) constant

> > expression we want to evaluate, not doing anything with jump_target if we

> > aren't inside a statement list makes sense to me, there is no following

> > statement to bypass.

> 

> I think we should also set *non_constant_p.


Just like this?  Tested so far just on the testcase, but given that we'd ICE
on the *jump_target before, it can't really regress anything else (though of
course I'll bootstrap/regtest it normally).

2018-02-16  Marek Polacek  <polacek@redhat.com>
	    Jakub Jelinek  <jakub@redhat.com>

	PR c++/84192
	* constexpr.c (cxx_eval_constant_expression) <case RETURN_EXPR>: Don't
	set *jump_target to anything if jump_target is NULL.

	* g++.dg/cpp1y/constexpr-84192.C: New test.

--- gcc/cp/constexpr.c.jj	2018-02-12 19:17:37.937216029 +0100
+++ gcc/cp/constexpr.c	2018-02-15 16:10:56.630572360 +0100
@@ -4254,7 +4254,16 @@ cxx_eval_constant_expression (const cons
 	r = cxx_eval_constant_expression (ctx, TREE_OPERAND (t, 0),
 					  lval,
 					  non_constant_p, overflow_p);
-      *jump_target = t;
+      if (jump_target)
+	*jump_target = t;
+      else
+	{
+	  /* Can happen with ({ return true; }) && false; passed to
+	     maybe_constant_value.  There is nothing to jump over in this
+	     case, and the bug will be diagnosed later.  */
+	  gcc_assert (ctx->quiet);
+	  *non_constant_p = true;
+	}
       break;
 
     case SAVE_EXPR:
--- gcc/testsuite/g++.dg/cpp1y/constexpr-84192.C.jj	2018-02-15 16:00:58.242588914 +0100
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-84192.C	2018-02-15 16:01:30.219585291 +0100
@@ -0,0 +1,41 @@
+// PR c++/84192
+// { dg-do compile { target c++14 } }
+// { dg-options "" }
+
+bool
+f1 ()
+{ 
+  return ({ return true; }) && false;	// { dg-error "could not convert" }
+}
+
+void
+f2 ()
+{ 
+  for (;;)
+    constexpr bool b = ({ break; false; }) && false;	// { dg-error "statement is not a constant expression" }
+}
+
+constexpr bool
+f3 (int n)
+{
+  bool b = false;
+  for (int i = 0; i < n; i++)
+    b = ({ break; });	// { dg-error "void value not ignored as it ought to be" }
+  return b;
+}
+
+constexpr bool b = f3 (4);
+
+bool
+f4 ()
+{
+  constexpr bool b = ({ return true; }) && false;	// { dg-error "could not convert" }
+  return false;
+}
+
+constexpr bool
+f5 (int x)
+{
+  constexpr bool b = ({ switch (x) case 0: true; }) && false;	// { dg-error "could not convert" }
+  return false;
+}


	Jakub
Jason Merrill Feb. 16, 2018, 3:20 p.m. | #3
OK.

On Fri, Feb 16, 2018 at 10:19 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Feb 16, 2018 at 08:52:10AM -0500, Jason Merrill wrote:

>> On Fri, Feb 16, 2018 at 3:28 AM, Jakub Jelinek <jakub@redhat.com> wrote:

>> > pop_stmt_list, if there is just a single stmt inside statement expression

>> > moves the stmt out of the STATEMENT_LIST (and I think cp_fold does too).

>> > We only initialize jump_target to non-NULL in cxx_eval_statement_list

>> > or for calls, so before we have a chance to diagnose the error of using

>> > an expression with void type, we ICE trying to dereference NULL jump_target.

>> >

>> > This can't happen with BREAK_STMT nor CONTINUE_STMT, because they are not

>> > potential constant expressions, and I think can only happen when ctx->quiet

>> > is true, otherwise it should have been diagnosed already before.

>> > If a RETURN_EXPR (or in theory break/continue) appears in a (potential) constant

>> > expression we want to evaluate, not doing anything with jump_target if we

>> > aren't inside a statement list makes sense to me, there is no following

>> > statement to bypass.

>>

>> I think we should also set *non_constant_p.

>

> Just like this?  Tested so far just on the testcase, but given that we'd ICE

> on the *jump_target before, it can't really regress anything else (though of

> course I'll bootstrap/regtest it normally).

>

> 2018-02-16  Marek Polacek  <polacek@redhat.com>

>             Jakub Jelinek  <jakub@redhat.com>

>

>         PR c++/84192

>         * constexpr.c (cxx_eval_constant_expression) <case RETURN_EXPR>: Don't

>         set *jump_target to anything if jump_target is NULL.

>

>         * g++.dg/cpp1y/constexpr-84192.C: New test.

>

> --- gcc/cp/constexpr.c.jj       2018-02-12 19:17:37.937216029 +0100

> +++ gcc/cp/constexpr.c  2018-02-15 16:10:56.630572360 +0100

> @@ -4254,7 +4254,16 @@ cxx_eval_constant_expression (const cons

>         r = cxx_eval_constant_expression (ctx, TREE_OPERAND (t, 0),

>                                           lval,

>                                           non_constant_p, overflow_p);

> -      *jump_target = t;

> +      if (jump_target)

> +       *jump_target = t;

> +      else

> +       {

> +         /* Can happen with ({ return true; }) && false; passed to

> +            maybe_constant_value.  There is nothing to jump over in this

> +            case, and the bug will be diagnosed later.  */

> +         gcc_assert (ctx->quiet);

> +         *non_constant_p = true;

> +       }

>        break;

>

>      case SAVE_EXPR:

> --- gcc/testsuite/g++.dg/cpp1y/constexpr-84192.C.jj     2018-02-15 16:00:58.242588914 +0100

> +++ gcc/testsuite/g++.dg/cpp1y/constexpr-84192.C        2018-02-15 16:01:30.219585291 +0100

> @@ -0,0 +1,41 @@

> +// PR c++/84192

> +// { dg-do compile { target c++14 } }

> +// { dg-options "" }

> +

> +bool

> +f1 ()

> +{

> +  return ({ return true; }) && false;  // { dg-error "could not convert" }

> +}

> +

> +void

> +f2 ()

> +{

> +  for (;;)

> +    constexpr bool b = ({ break; false; }) && false;   // { dg-error "statement is not a constant expression" }

> +}

> +

> +constexpr bool

> +f3 (int n)

> +{

> +  bool b = false;

> +  for (int i = 0; i < n; i++)

> +    b = ({ break; });  // { dg-error "void value not ignored as it ought to be" }

> +  return b;

> +}

> +

> +constexpr bool b = f3 (4);

> +

> +bool

> +f4 ()

> +{

> +  constexpr bool b = ({ return true; }) && false;      // { dg-error "could not convert" }

> +  return false;

> +}

> +

> +constexpr bool

> +f5 (int x)

> +{

> +  constexpr bool b = ({ switch (x) case 0: true; }) && false;  // { dg-error "could not convert" }

> +  return false;

> +}

>

>

>         Jakub

Patch

--- gcc/cp/constexpr.c.jj	2018-02-12 19:17:37.937216029 +0100
+++ gcc/cp/constexpr.c	2018-02-15 16:10:56.630572360 +0100
@@ -4254,7 +4254,13 @@  cxx_eval_constant_expression (const cons
 	r = cxx_eval_constant_expression (ctx, TREE_OPERAND (t, 0),
 					  lval,
 					  non_constant_p, overflow_p);
-      *jump_target = t;
+      if (jump_target)
+	*jump_target = t;
+      else
+	/* Can happen with ({ return true; }) && false; passed to
+	   maybe_constant_value.  There is nothing to jump over in this
+	   case, and the bug will be diagnosed later.  */
+	gcc_assert (ctx->quiet);
       break;
 
     case SAVE_EXPR:
--- gcc/testsuite/g++.dg/cpp1y/constexpr-84192.C.jj	2018-02-15 16:00:58.242588914 +0100
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-84192.C	2018-02-15 16:01:30.219585291 +0100
@@ -0,0 +1,41 @@ 
+// PR c++/84192
+// { dg-do compile { target c++14 } }
+// { dg-options "" }
+
+bool
+f1 ()
+{ 
+  return ({ return true; }) && false;	// { dg-error "could not convert" }
+}
+
+void
+f2 ()
+{ 
+  for (;;)
+    constexpr bool b = ({ break; false; }) && false;	// { dg-error "statement is not a constant expression" }
+}
+
+constexpr bool
+f3 (int n)
+{
+  bool b = false;
+  for (int i = 0; i < n; i++)
+    b = ({ break; });	// { dg-error "void value not ignored as it ought to be" }
+  return b;
+}
+
+constexpr bool b = f3 (4);
+
+bool
+f4 ()
+{
+  constexpr bool b = ({ return true; }) && false;	// { dg-error "could not convert" }
+  return false;
+}
+
+constexpr bool
+f5 (int x)
+{
+  constexpr bool b = ({ switch (x) case 0: true; }) && false;	// { dg-error "could not convert" }
+  return false;
+}