| Message ID | 20191203084208.GB10088@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | [C++] Fix up constexpr TARGET_EXPR evaluation if there are cleanups (PR c++/91369) | expand |
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 >
--- 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);