Re: [C++ PATCH] Fix up constexpr TARGET_EXPR evaluation if there are cleanups (PR c++/91369)
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
PR c++/91369
* constexpr.c (struct constexpr_global_ctx): Add cleanups member,
initialize it in the ctor.
(cxx_eval_constant_expression) : 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.
: Save outer cleanups, set cleanups to
local auto_vec, after evaluating the body evaluate cleanups and
restore previous cleanups.
: 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 heap_vars;
+ /* Cleanups that need to be evaluated at the end of CLEANUP_POINT_EXPR. */
+ vec *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
[C++ PATCH] Fix up constexpr TARGET_EXPR evaluation if there are cleanups (PR c++/91369)
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
PR c++/91369
* constexpr.c (struct constexpr_global_ctx): Add cleanups member,
initialize it in the ctor.
(cxx_eval_constant_expression) : 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.
: Save outer cleanups, set cleanups to
local auto_vec, after evaluating the body evaluate cleanups and
restore previous cleanups.
: 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 heap_vars;
+ /* Cleanups that need to be evaluated at the end of CLEANUP_POINT_EXPR. */
+ vec *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);
+
