[C++] Fix up constexpr TARGET_EXPR evaluation if there are cleanups (PR c++/91369)

Message ID 20191203084208.GB10088@tucnak
State New
Headers show
Series
  • [C++] Fix up constexpr TARGET_EXPR evaluation if there are cleanups (PR c++/91369)
Related show

Commit Message

Jakub Jelinek Dec. 3, 2019, 8:42 a.m.
Hi!

The following testcase shows that during constexpr evaluation we didn't
handle TARGET_EXPR_CLEANUP at all (which was probably fine when there
weren't constexpr dtors).  My understanding is that TARGET_EXPR cleanups
should be queued and evaluated only at the end of CLEANUP_POINT_EXPR, so
that is what the patch implements.

Regtesting of the first iteration revealed quite some FAILs in libstdc++,
my assumption that a TARGET_EXPR with cleanups will always appear inside of
some CLEANUP_POINT_EXPR is violated e.g. when cp_fold attempts to evaluate
some call expressions with TARGET_EXPR in arguments, so I had to add
evaluating of cleanups in cxx_eval_outermost_constant_expr too as if the
outermost expression were wrapped into another CLEANUP_POINT_EXPR.

There was another issue only seen in libstdc++, in particular,
TRY_CATCH_EXPR created by wrap_cleanups_r can sometimes have NULL_TREE
first argument.

Furthermore (though just theoretical, I haven't tried to create a testcase),
I believe a TARGET_EXPR shuld behave similarly to SAVE_EXPR that if the same
TARGET_EXPR tree is encountered multiple times, the initializer expression
is evaluated just once, not multiple times.  Maybe it didn't matter before
if things were evaluated multiple times, but e.g. with constexpr new/delete,
evaluating new multiple times without corresponding deletes is incorrect,
so in the patch I've tried to handle caching of the result and reuse of it
once it has been evaluated already (with pushing into save_exprs so that
e.g. when evaluating a loop second time or a function we undo the caching).

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

Note, the description of CLEANUP_POINT_EXPR in tree.def makes me worry a
little bit, with the:
"   Note that if the expression is a reference to storage, it is forced out
   of memory before the cleanups are run.  This is necessary to handle
   cases where the cleanups modify the storage referenced; in the
   expression 't.i', if 't' is a struct with an integer member 'i' and a
   cleanup which modifies 'i', the value of the expression depends on
   whether the cleanup is run before or after 't.i' is evaluated.  When
   expand_expr is run on 't.i', it returns a MEM.  This is not good enough;
   the value of 't.i' must be forced out of memory."
note.  Does that mean the CLEANUP_POINT_EXPR handling should do
unshare_constructor on the result if there are any cleanups (and similarly
outermost expr handling)?  Don't want on the other side to waste too much
memory if it is not necessary...

2019-12-03  Jakub Jelinek  <jakub@redhat.com>

	PR c++/91369
	* constexpr.c (struct constexpr_global_ctx): Add cleanups member,
	initialize it in the ctor.
	(cxx_eval_constant_expression) <case TARGET_EXPR>: If TARGET_EXPR_SLOT
	is already in the values hash_map, don't evaluate it again.  Put
	TARGET_EXPR_SLOT into hash_map even if not lval, and push it into
	save_exprs too.  If there is TARGET_EXPR_CLEANUP and not
	CLEANUP_EH_ONLY, push the cleanup to cleanups vector.
	<case CLEANUP_POINT_EXPR>: Save outer cleanups, set cleanups to
	local auto_vec, after evaluating the body evaluate cleanups and
	restore previous cleanups.
	<case TRY_CATCH_EXPR>: Don't crash if the first operand is NULL_TREE.
	(cxx_eval_outermost_constant_expr): Set cleanups to local auto_vec,
	after evaluating the expression evaluate cleanups.

	* g++.dg/cpp2a/constexpr-new8.C: New test.


	Jakub

Comments

Jason Merrill Dec. 3, 2019, 7:16 p.m. | #1
On 12/3/19 3:42 AM, Jakub Jelinek wrote:
> Hi!

> 

> The following testcase shows that during constexpr evaluation we didn't

> handle TARGET_EXPR_CLEANUP at all (which was probably fine when there

> weren't constexpr dtors).  My understanding is that TARGET_EXPR cleanups

> should be queued and evaluated only at the end of CLEANUP_POINT_EXPR, so

> that is what the patch implements.

> 

> Regtesting of the first iteration revealed quite some FAILs in libstdc++,

> my assumption that a TARGET_EXPR with cleanups will always appear inside of

> some CLEANUP_POINT_EXPR is violated e.g. when cp_fold attempts to evaluate

> some call expressions with TARGET_EXPR in arguments, so I had to add

> evaluating of cleanups in cxx_eval_outermost_constant_expr too as if the

> outermost expression were wrapped into another CLEANUP_POINT_EXPR.

> 

> There was another issue only seen in libstdc++, in particular,

> TRY_CATCH_EXPR created by wrap_cleanups_r can sometimes have NULL_TREE

> first argument.

> 

> Furthermore (though just theoretical, I haven't tried to create a testcase),

> I believe a TARGET_EXPR shuld behave similarly to SAVE_EXPR that if the same

> TARGET_EXPR tree is encountered multiple times, the initializer expression

> is evaluated just once, not multiple times.  Maybe it didn't matter before

> if things were evaluated multiple times, but e.g. with constexpr new/delete,

> evaluating new multiple times without corresponding deletes is incorrect,

> so in the patch I've tried to handle caching of the result and reuse of it

> once it has been evaluated already (with pushing into save_exprs so that

> e.g. when evaluating a loop second time or a function we undo the caching).

> 

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

> 

> Note, the description of CLEANUP_POINT_EXPR in tree.def makes me worry a

> little bit, with the:

> "   Note that if the expression is a reference to storage, it is forced out

>     of memory before the cleanups are run.  This is necessary to handle

>     cases where the cleanups modify the storage referenced; in the

>     expression 't.i', if 't' is a struct with an integer member 'i' and a

>     cleanup which modifies 'i', the value of the expression depends on

>     whether the cleanup is run before or after 't.i' is evaluated.  When

>     expand_expr is run on 't.i', it returns a MEM.  This is not good enough;

>     the value of 't.i' must be forced out of memory."

> note.  Does that mean the CLEANUP_POINT_EXPR handling should do

> unshare_constructor on the result if there are any cleanups (and similarly

> outermost expr handling)?  Don't want on the other side to waste too much

> memory if it is not necessary...

> 

> 2019-12-03  Jakub Jelinek  <jakub@redhat.com>

> 

> 	PR c++/91369

> 	* constexpr.c (struct constexpr_global_ctx): Add cleanups member,

> 	initialize it in the ctor.

> 	(cxx_eval_constant_expression) <case TARGET_EXPR>: If TARGET_EXPR_SLOT

> 	is already in the values hash_map, don't evaluate it again.  Put

> 	TARGET_EXPR_SLOT into hash_map even if not lval, and push it into

> 	save_exprs too.  If there is TARGET_EXPR_CLEANUP and not

> 	CLEANUP_EH_ONLY, push the cleanup to cleanups vector.

> 	<case CLEANUP_POINT_EXPR>: Save outer cleanups, set cleanups to

> 	local auto_vec, after evaluating the body evaluate cleanups and

> 	restore previous cleanups.

> 	<case TRY_CATCH_EXPR>: Don't crash if the first operand is NULL_TREE.

> 	(cxx_eval_outermost_constant_expr): Set cleanups to local auto_vec,

> 	after evaluating the expression evaluate cleanups.

> 

> 	* g++.dg/cpp2a/constexpr-new8.C: New test.

> 

> --- gcc/cp/constexpr.c.jj	2019-11-28 09:02:21.896897228 +0100

> +++ gcc/cp/constexpr.c	2019-12-03 01:14:06.674952015 +0100

> @@ -1025,8 +1025,10 @@ struct constexpr_global_ctx {

>     /* Heap VAR_DECLs created during the evaluation of the outermost constant

>        expression.  */

>     auto_vec<tree, 16> heap_vars;

> +  /* Cleanups that need to be evaluated at the end of CLEANUP_POINT_EXPR.  */

> +  vec<tree> *cleanups;

>     /* Constructor.  */

> -  constexpr_global_ctx () : constexpr_ops_count (0) {}

> +  constexpr_global_ctx () : constexpr_ops_count (0), cleanups (NULL) {}

>   };

>   

>   /* The constexpr expansion context.  CALL is the current function

> @@ -4984,6 +4986,14 @@ cxx_eval_constant_expression (const cons

>   	  *non_constant_p = true;

>   	  break;

>   	}

> +      /* Avoid evaluating a TARGET_EXPR more than once.  */

> +      if (tree *p = ctx->global->values.get (TARGET_EXPR_SLOT (t)))

> +	{

> +	  if (lval)

> +	    return TARGET_EXPR_SLOT (t);

> +	  r = *p;

> +	  break;

> +	}

>         if ((AGGREGATE_TYPE_P (TREE_TYPE (t)) || VECTOR_TYPE_P (TREE_TYPE (t))))

>   	{

>   	  /* We're being expanded without an explicit target, so start

> @@ -5004,13 +5014,14 @@ cxx_eval_constant_expression (const cons

>         if (!*non_constant_p)

>   	/* Adjust the type of the result to the type of the temporary.  */

>   	r = adjust_temp_type (TREE_TYPE (t), r);

> +      if (TARGET_EXPR_CLEANUP (t) && !CLEANUP_EH_ONLY (t))

> +	ctx->global->cleanups->safe_push (TARGET_EXPR_CLEANUP (t));

> +      r = unshare_constructor (r);

> +      ctx->global->values.put (TARGET_EXPR_SLOT (t), r);

> +      if (ctx->save_exprs)

> +	ctx->save_exprs->safe_push (TARGET_EXPR_SLOT (t));


Please at least adjust the comment for the save_exprs field to note that 
it can also have TARGET_EXPR slots in it.  OK with that change.

>         if (lval)

> -	{

> -	  tree slot = TARGET_EXPR_SLOT (t);

> -	  r = unshare_constructor (r);

> -	  ctx->global->values.put (slot, r);

> -	  return slot;

> -	}

> +	return TARGET_EXPR_SLOT (t);

>         break;

>   

>       case INIT_EXPR:

> @@ -5060,10 +5071,15 @@ cxx_eval_constant_expression (const cons

>   	}

>         break;

>   

> -    case NON_LVALUE_EXPR:

>       case TRY_CATCH_EXPR:

> +      if (TREE_OPERAND (t, 0) == NULL_TREE)

> +	{

> +	  r = void_node;

> +	  break;

> +	}

> +      /* FALLTHRU */

> +    case NON_LVALUE_EXPR:

>       case TRY_BLOCK:

> -    case CLEANUP_POINT_EXPR:

>       case MUST_NOT_THROW_EXPR:

>       case EXPR_STMT:

>       case EH_SPEC_BLOCK:

> @@ -5073,6 +5089,26 @@ cxx_eval_constant_expression (const cons

>   					jump_target);

>         break;

>   

> +    case CLEANUP_POINT_EXPR:

> +      {

> +	auto_vec<tree, 2> cleanups;

> +	vec<tree> *prev_cleanups = ctx->global->cleanups;

> +	ctx->global->cleanups = &cleanups;

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

> +					  lval,

> +					  non_constant_p, overflow_p,

> +					  jump_target);

> +	ctx->global->cleanups = prev_cleanups;

> +	unsigned int i;

> +	tree cleanup;

> +	/* Evaluate the cleanups.  */

> +	FOR_EACH_VEC_ELT_REVERSE (cleanups, i, cleanup)

> +	  cxx_eval_constant_expression (ctx, cleanup, false,

> +					non_constant_p, overflow_p,

> +					jump_target);

> +      }

> +      break;

> +

>       case TRY_FINALLY_EXPR:

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

>   					non_constant_p, overflow_p,

> @@ -5883,6 +5919,9 @@ cxx_eval_outermost_constant_expr (tree t

>   	r = TARGET_EXPR_INITIAL (r);

>       }

>   

> +  auto_vec<tree, 16> cleanups;

> +  global_ctx.cleanups = &cleanups;

> +

>     instantiate_constexpr_fns (r);

>     r = cxx_eval_constant_expression (&ctx, r,

>   				    false, &non_constant_p, &overflow_p);

> @@ -5892,6 +5931,13 @@ cxx_eval_outermost_constant_expr (tree t

>     else

>       DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (object) = true;

>   

> +  unsigned int i;

> +  tree cleanup;

> +  /* Evaluate the cleanups.  */

> +  FOR_EACH_VEC_ELT_REVERSE (cleanups, i, cleanup)

> +    cxx_eval_constant_expression (&ctx, cleanup, false,

> +				  &non_constant_p, &overflow_p);

> +

>     /* Mutable logic is a bit tricky: we want to allow initialization of

>        constexpr variables with mutable members, but we can't copy those

>        members to another constexpr variable.  */

> --- gcc/testsuite/g++.dg/cpp2a/constexpr-new8.C.jj	2019-12-02 11:48:58.595976239 +0100

> +++ gcc/testsuite/g++.dg/cpp2a/constexpr-new8.C	2019-12-02 11:51:34.137550815 +0100

> @@ -0,0 +1,18 @@

> +// PR c++/91369

> +// { dg-do compile { target c++2a } }

> +

> +struct A {

> +  constexpr A () : p{new int} {}

> +  constexpr ~A () { delete p; }

> +  int *p;

> +};

> +

> +constexpr bool

> +test ()

> +{

> +  A{};

> +  return true;

> +}

> +

> +constexpr auto res = test ();

> +static_assert (res);

> 

> 	Jakub

>

Patch

--- gcc/cp/constexpr.c.jj	2019-11-28 09:02:21.896897228 +0100
+++ gcc/cp/constexpr.c	2019-12-03 01:14:06.674952015 +0100
@@ -1025,8 +1025,10 @@  struct constexpr_global_ctx {
   /* Heap VAR_DECLs created during the evaluation of the outermost constant
      expression.  */
   auto_vec<tree, 16> heap_vars;
+  /* Cleanups that need to be evaluated at the end of CLEANUP_POINT_EXPR.  */
+  vec<tree> *cleanups;
   /* Constructor.  */
-  constexpr_global_ctx () : constexpr_ops_count (0) {}
+  constexpr_global_ctx () : constexpr_ops_count (0), cleanups (NULL) {}
 };
 
 /* The constexpr expansion context.  CALL is the current function
@@ -4984,6 +4986,14 @@  cxx_eval_constant_expression (const cons
 	  *non_constant_p = true;
 	  break;
 	}
+      /* Avoid evaluating a TARGET_EXPR more than once.  */
+      if (tree *p = ctx->global->values.get (TARGET_EXPR_SLOT (t)))
+	{
+	  if (lval)
+	    return TARGET_EXPR_SLOT (t);
+	  r = *p;
+	  break;
+	}
       if ((AGGREGATE_TYPE_P (TREE_TYPE (t)) || VECTOR_TYPE_P (TREE_TYPE (t))))
 	{
 	  /* We're being expanded without an explicit target, so start
@@ -5004,13 +5014,14 @@  cxx_eval_constant_expression (const cons
       if (!*non_constant_p)
 	/* Adjust the type of the result to the type of the temporary.  */
 	r = adjust_temp_type (TREE_TYPE (t), r);
+      if (TARGET_EXPR_CLEANUP (t) && !CLEANUP_EH_ONLY (t))
+	ctx->global->cleanups->safe_push (TARGET_EXPR_CLEANUP (t));
+      r = unshare_constructor (r);
+      ctx->global->values.put (TARGET_EXPR_SLOT (t), r);
+      if (ctx->save_exprs)
+	ctx->save_exprs->safe_push (TARGET_EXPR_SLOT (t));
       if (lval)
-	{
-	  tree slot = TARGET_EXPR_SLOT (t);
-	  r = unshare_constructor (r);
-	  ctx->global->values.put (slot, r);
-	  return slot;
-	}
+	return TARGET_EXPR_SLOT (t);
       break;
 
     case INIT_EXPR:
@@ -5060,10 +5071,15 @@  cxx_eval_constant_expression (const cons
 	}
       break;
 
-    case NON_LVALUE_EXPR:
     case TRY_CATCH_EXPR:
+      if (TREE_OPERAND (t, 0) == NULL_TREE)
+	{
+	  r = void_node;
+	  break;
+	}
+      /* FALLTHRU */
+    case NON_LVALUE_EXPR:
     case TRY_BLOCK:
-    case CLEANUP_POINT_EXPR:
     case MUST_NOT_THROW_EXPR:
     case EXPR_STMT:
     case EH_SPEC_BLOCK:
@@ -5073,6 +5089,26 @@  cxx_eval_constant_expression (const cons
 					jump_target);
       break;
 
+    case CLEANUP_POINT_EXPR:
+      {
+	auto_vec<tree, 2> cleanups;
+	vec<tree> *prev_cleanups = ctx->global->cleanups;
+	ctx->global->cleanups = &cleanups;
+	r = cxx_eval_constant_expression (ctx, TREE_OPERAND (t, 0),
+					  lval,
+					  non_constant_p, overflow_p,
+					  jump_target);
+	ctx->global->cleanups = prev_cleanups;
+	unsigned int i;
+	tree cleanup;
+	/* Evaluate the cleanups.  */
+	FOR_EACH_VEC_ELT_REVERSE (cleanups, i, cleanup)
+	  cxx_eval_constant_expression (ctx, cleanup, false,
+					non_constant_p, overflow_p,
+					jump_target);
+      }
+      break;
+
     case TRY_FINALLY_EXPR:
       r = cxx_eval_constant_expression (ctx, TREE_OPERAND (t, 0), lval,
 					non_constant_p, overflow_p,
@@ -5883,6 +5919,9 @@  cxx_eval_outermost_constant_expr (tree t
 	r = TARGET_EXPR_INITIAL (r);
     }
 
+  auto_vec<tree, 16> cleanups;
+  global_ctx.cleanups = &cleanups;
+
   instantiate_constexpr_fns (r);
   r = cxx_eval_constant_expression (&ctx, r,
 				    false, &non_constant_p, &overflow_p);
@@ -5892,6 +5931,13 @@  cxx_eval_outermost_constant_expr (tree t
   else
     DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (object) = true;
 
+  unsigned int i;
+  tree cleanup;
+  /* Evaluate the cleanups.  */
+  FOR_EACH_VEC_ELT_REVERSE (cleanups, i, cleanup)
+    cxx_eval_constant_expression (&ctx, cleanup, false,
+				  &non_constant_p, &overflow_p);
+
   /* Mutable logic is a bit tricky: we want to allow initialization of
      constexpr variables with mutable members, but we can't copy those
      members to another constexpr variable.  */
--- gcc/testsuite/g++.dg/cpp2a/constexpr-new8.C.jj	2019-12-02 11:48:58.595976239 +0100
+++ gcc/testsuite/g++.dg/cpp2a/constexpr-new8.C	2019-12-02 11:51:34.137550815 +0100
@@ -0,0 +1,18 @@ 
+// PR c++/91369
+// { dg-do compile { target c++2a } }
+
+struct A {
+  constexpr A () : p{new int} {}
+  constexpr ~A () { delete p; }
+  int *p;
+};
+
+constexpr bool
+test ()
+{
+  A{};
+  return true;
+}
+
+constexpr auto res = test ();
+static_assert (res);