This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [C++ PATCH] Fix PLACEHOLDER_EXPR handling (PR c++/79937, PR c++/82410)
- From: Jason Merrill <jason at redhat dot com>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: gcc-patches List <gcc-patches at gcc dot gnu dot org>
- Date: Thu, 15 Mar 2018 22:17:27 -0400
- Subject: Re: [C++ PATCH] Fix PLACEHOLDER_EXPR handling (PR c++/79937, PR c++/82410)
- References: <20180314223346.GN8577@tucnak> <CADzB+2=4vdr9OocJSCNejyBLaRubK1L4ZhbA=LdYgUy-EhN2+Q@mail.gmail.com> <20180315083243.GP8577@tucnak> <CADzB+2mhu9aNApCRyPZ4ksMTMfeivqJcneUCdd3VuORcVh=qMg@mail.gmail.com> <20180315175900.GU8577@tucnak> <CADzB+2kOgLepQv6-K0GVSdyObNZ6Fkp+QkxvadTkqPVj2=G4TA@mail.gmail.com> <20180315202808.GY8577@tucnak> <CADzB+2k+dWj=g=-wEgxi1tM=eJ-CPchqwvX8aa_+DEY-3inWQw@mail.gmail.com> <20180315233311.GA8577@tucnak>
On Thu, Mar 15, 2018 at 7:33 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Mar 15, 2018 at 04:50:57PM -0400, Jason Merrill wrote:
>> > +/* Don't elide the initialization of TARGET_EXPR_SLOT for this TARGET_EXPR. */
>> > +#define TARGET_EXPR_NO_ELIDE(NODE) (TARGET_EXPR_CHECK (NODE)->base.private_flag)
>>
>> This should be specifically on the rhs of a MODIFY_EXPR; it's OK to
>> elide on the rhs of an INIT_EXPR.
>>
>> > @@ -3155,7 +3155,8 @@ gimplify_arg (tree *arg_p, gimple_seq *p
>> > {
>> > test = is_gimple_lvalue, fb = fb_either;
>> > /* Also strip a TARGET_EXPR that would force an extra copy. */
>> > - if (TREE_CODE (*arg_p) == TARGET_EXPR)
>> > + if (TREE_CODE (*arg_p) == TARGET_EXPR
>> > + && !TARGET_EXPR_NO_ELIDE (*arg_p))
>>
>> This is also an initialization context, so we don't need to check it here.
>
> Ah, ok, changed in the patch.
>
>> > @@ -5211,6 +5212,7 @@ gimplify_modify_expr_rhs (tree *expr_p,
>> > tree init = TARGET_EXPR_INITIAL (*from_p);
>> >
>> > if (init
>> > + && !TARGET_EXPR_NO_ELIDE (*from_p)
>>
>> Here should check TREE_CODE (*expr_p).
>
> The following (except the *t = unshare_expr (x) change) has successfully
> bootstrapped/regtested on x86_64-linux and i686-linux (without the
> unshare_expr it regressed g++.dg/cpp0x/pr83556.C, but with that change it
> succeeds; I'm afraid right now there is no easy way to determine if we
> need to unshare or not, if replace_placeholders is called before
> unshare_body at the start of gimplification, we don't need to unshare, but
> during gimplification we have to). Ok for trunk?
> 2018-03-15 Jakub Jelinek <jakub@redhat.com>
>
> PR c++/79937
> PR c++/82410
> * tree.h (TARGET_EXPR_NO_ELIDE): Define.
> * gimplify.c (gimplify_modify_expr_rhs): Don't elide TARGET_EXPRs with
> TARGET_EXPR_NO_ELIDE flag set unless *expr_p is INIT_EXPR.
>
> * cp-tree.h (CONSTRUCTOR_PLACEHOLDER_BOUNDARY): Define.
> (find_placeholder): Declare.
> * tree.c (struct replace_placeholders_t): Add exp member.
> (replace_placeholders_r): Don't walk into ctors with
> CONSTRUCTOR_PLACEHOLDER_BOUNDARY flag set, unless they are equal to
> d->exp. Replace PLACEHOLDER_EXPR with unshare_expr (x) rather than x.
> (replace_placeholders): Initialize data.exp.
> (find_placeholders_r, find_placeholders): New functions.
> * typeck2.c (process_init_constructor_record,
> process_init_constructor_union): Set CONSTRUCTOR_PLACEHOLDER_BOUNDARY
> if adding NSDMI on which find_placeholder returns true.
> * call.c (build_over_call): Don't call replace_placeholders here.
> * cp-gimplify.c (cp_genericize_r): Set TARGET_EXPR_NO_ELIDE on
> TARGET_EXPRs with CONSTRUCTOR_PLACEHOLDER_BOUNDARY set on
> TARGET_EXPR_INITIAL.
> (cp_fold): Copy over CONSTRUCTOR_PLACEHOLDER_BOUNDARY bit to new
> ctor.
>
> * g++.dg/cpp1y/pr79937-1.C: New test.
> * g++.dg/cpp1y/pr79937-2.C: New test.
> * g++.dg/cpp1y/pr79937-3.C: New test.
> * g++.dg/cpp1y/pr79937-4.C: New test.
> * g++.dg/cpp1y/pr82410.C: New test.
>
> --- gcc/tree.h.jj 2018-02-22 12:37:02.566387732 +0100
> +++ gcc/tree.h 2018-03-15 20:42:57.968858551 +0100
> @@ -1197,6 +1197,9 @@ extern tree maybe_wrap_with_location (tr
> #define TARGET_EXPR_SLOT(NODE) TREE_OPERAND_CHECK_CODE (NODE, TARGET_EXPR, 0)
> #define TARGET_EXPR_INITIAL(NODE) TREE_OPERAND_CHECK_CODE (NODE, TARGET_EXPR, 1)
> #define TARGET_EXPR_CLEANUP(NODE) TREE_OPERAND_CHECK_CODE (NODE, TARGET_EXPR, 2)
> +/* Don't elide the initialization of TARGET_EXPR_SLOT for this TARGET_EXPR
> + on rhs of MODIFY_EXPR. */
> +#define TARGET_EXPR_NO_ELIDE(NODE) (TARGET_EXPR_CHECK (NODE)->base.private_flag)
>
> /* DECL_EXPR accessor. This gives access to the DECL associated with
> the given declaration statement. */
> --- gcc/gimplify.c.jj 2018-01-18 21:28:49.743301440 +0100
> +++ gcc/gimplify.c 2018-03-15 21:06:15.290828320 +0100
> @@ -5211,6 +5211,8 @@ gimplify_modify_expr_rhs (tree *expr_p,
> tree init = TARGET_EXPR_INITIAL (*from_p);
>
> if (init
> + && (TREE_CODE (*expr_p) != MODIFY_EXPR
> + || !TARGET_EXPR_NO_ELIDE (*from_p))
> && !VOID_TYPE_P (TREE_TYPE (init)))
> {
> *from_p = init;
> --- gcc/cp/cp-tree.h.jj 2018-03-15 18:44:21.646300907 +0100
> +++ gcc/cp/cp-tree.h 2018-03-15 20:40:49.280818226 +0100
> @@ -425,6 +425,7 @@ extern GTY(()) tree cp_global_trees[CPTI
> DECL_VTABLE_OR_VTT_P (in VAR_DECL)
> FUNCTION_RVALUE_QUALIFIED (in FUNCTION_TYPE, METHOD_TYPE)
> CALL_EXPR_REVERSE_ARGS (in CALL_EXPR, AGGR_INIT_EXPR)
> + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (in CONSTRUCTOR)
> 6: IDENTIFIER_REPO_CHOSEN (in IDENTIFIER_NODE)
> DECL_CONSTRUCTION_VTABLE_P (in VAR_DECL)
> TYPE_MARKED_P (in _TYPE)
> @@ -4144,6 +4145,12 @@ more_aggr_init_expr_args_p (const aggr_i
> #define CONSTRUCTOR_C99_COMPOUND_LITERAL(NODE) \
> (TREE_LANG_FLAG_3 (CONSTRUCTOR_CHECK (NODE)))
>
> +/* True if this CONSTRUCTOR contains PLACEHOLDER_EXPRs referencing the
> + CONSTRUCTOR's type not nested inside another CONSTRUCTOR marked with
> + CONSTRUCTOR_PLACEHOLDER_BOUNDARY. */
> +#define CONSTRUCTOR_PLACEHOLDER_BOUNDARY(NODE) \
> + (TREE_LANG_FLAG_5 (CONSTRUCTOR_CHECK (NODE)))
> +
> #define DIRECT_LIST_INIT_P(NODE) \
> (BRACE_ENCLOSED_INITIALIZER_P (NODE) && CONSTRUCTOR_IS_DIRECT_INIT (NODE))
>
> @@ -7021,6 +7028,7 @@ extern tree array_type_nelts_top (tree)
> extern tree break_out_target_exprs (tree);
> extern tree build_ctor_subob_ref (tree, tree, tree);
> extern tree replace_placeholders (tree, tree, bool * = NULL);
> +extern bool find_placeholders (tree);
> extern tree get_type_decl (tree);
> extern tree decl_namespace_context (tree);
> extern bool decl_anon_ns_mem_p (const_tree);
> --- gcc/cp/tree.c.jj 2018-03-15 18:44:21.661300889 +0100
> +++ gcc/cp/tree.c 2018-03-15 20:49:03.518973106 +0100
> @@ -3096,6 +3096,7 @@ build_ctor_subob_ref (tree index, tree t
> struct replace_placeholders_t
> {
> tree obj; /* The object to be substituted for a PLACEHOLDER_EXPR. */
> + tree exp; /* The outermost exp. */
> bool seen; /* Whether we've encountered a PLACEHOLDER_EXPR. */
> hash_set<tree> *pset; /* To avoid walking same trees multiple times. */
> };
> @@ -3124,7 +3125,7 @@ replace_placeholders_r (tree* t, int* wa
> TREE_TYPE (x));
> x = TREE_OPERAND (x, 0))
> gcc_assert (TREE_CODE (x) == COMPONENT_REF);
> - *t = x;
> + *t = unshare_expr (x);
> *walk_subtrees = false;
> d->seen = true;
> }
> @@ -3134,7 +3135,12 @@ replace_placeholders_r (tree* t, int* wa
> {
> constructor_elt *ce;
> vec<constructor_elt,va_gc> *v = CONSTRUCTOR_ELTS (*t);
> - if (d->pset->add (*t))
> + /* Don't walk into CONSTRUCTOR_PLACEHOLDER_BOUNDARY ctors
> + other than the d->exp one, those have PLACEHOLDER_EXPRs
> + related to another object. */
> + if ((CONSTRUCTOR_PLACEHOLDER_BOUNDARY (*t)
> + && *t != d->exp)
> + || d->pset->add (*t))
> {
> *walk_subtrees = false;
> return NULL_TREE;
> @@ -3192,16 +3198,58 @@ replace_placeholders (tree exp, tree obj
> return exp;
>
> tree *tp = &exp;
> - hash_set<tree> pset;
> - replace_placeholders_t data = { obj, false, &pset };
> + /* Use exp instead of *(type *)&exp. */
This comment should have been removed with the code it described. OK
with that change.
> if (TREE_CODE (exp) == TARGET_EXPR)
> tp = &TARGET_EXPR_INITIAL (exp);
> + hash_set<tree> pset;
> + replace_placeholders_t data = { obj, *tp, false, &pset };
> cp_walk_tree (tp, replace_placeholders_r, &data, NULL);
> if (seen_p)
> *seen_p = data.seen;
> return exp;
> }
>
> +/* Callback function for find_placeholders. */
> +
> +static tree
> +find_placeholders_r (tree *t, int *walk_subtrees, void *)
> +{
> + if (TYPE_P (*t) || TREE_CONSTANT (*t))
> + {
> + *walk_subtrees = false;
> + return NULL_TREE;
> + }
> +
> + switch (TREE_CODE (*t))
> + {
> + case PLACEHOLDER_EXPR:
> + return *t;
> +
> + case CONSTRUCTOR:
> + if (CONSTRUCTOR_PLACEHOLDER_BOUNDARY (*t))
> + *walk_subtrees = false;
> + break;
> +
> + default:
> + break;
> + }
> +
> + return NULL_TREE;
> +}
> +
> +/* Return true if EXP contains a PLACEHOLDER_EXPR. Don't walk into
> + ctors with CONSTRUCTOR_PLACEHOLDER_BOUNDARY flag set. */
> +
> +bool
> +find_placeholders (tree exp)
> +{
> + /* This is only relevant for C++14. */
> + if (cxx_dialect < cxx14)
> + return false;
> +
> + return cp_walk_tree_without_duplicates (&exp, find_placeholders_r, NULL);
> +}
> +
> /* Similar to `build_nt', but for template definitions of dependent
> expressions */
>
> --- gcc/cp/typeck2.c.jj 2018-03-15 18:44:21.661300889 +0100
> +++ gcc/cp/typeck2.c 2018-03-15 20:40:49.282818227 +0100
> @@ -1470,6 +1470,9 @@ process_init_constructor_record (tree ty
> }
> /* C++14 aggregate NSDMI. */
> next = get_nsdmi (field, /*ctor*/false, complain);
> + if (!CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init)
> + && find_placeholders (next))
> + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = 1;
> }
> else if (type_build_ctor_call (TREE_TYPE (field)))
> {
> @@ -1608,10 +1611,11 @@ process_init_constructor_union (tree typ
> if (TREE_CODE (field) == FIELD_DECL
> && DECL_INITIAL (field) != NULL_TREE)
> {
> - CONSTRUCTOR_APPEND_ELT (CONSTRUCTOR_ELTS (init),
> - field,
> - get_nsdmi (field, /*in_ctor=*/false,
> - complain));
> + tree val = get_nsdmi (field, /*in_ctor=*/false, complain);
> + if (!CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init)
> + && find_placeholders (val))
> + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = 1;
> + CONSTRUCTOR_APPEND_ELT (CONSTRUCTOR_ELTS (init), field, val);
> break;
> }
> }
> --- gcc/cp/call.c.jj 2018-03-11 17:48:36.359061434 +0100
> +++ gcc/cp/call.c 2018-03-15 21:07:31.781961783 +0100
> @@ -8164,8 +8164,6 @@ build_over_call (struct z_candidate *can
> {
> arg = cp_build_fold_indirect_ref (arg);
> val = build2 (MODIFY_EXPR, TREE_TYPE (to), to, arg);
> - /* Handle NSDMI that refer to the object being initialized. */
> - replace_placeholders (arg, to);
> }
> else
> {
> --- gcc/cp/cp-gimplify.c.jj 2018-03-07 22:51:58.742478684 +0100
> +++ gcc/cp/cp-gimplify.c 2018-03-15 21:19:27.353619519 +0100
> @@ -1515,6 +1515,13 @@ cp_genericize_r (tree *stmt_p, int *walk
> }
> break;
>
> + case TARGET_EXPR:
> + if (TARGET_EXPR_INITIAL (stmt)
> + && TREE_CODE (TARGET_EXPR_INITIAL (stmt)) == CONSTRUCTOR
> + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (TARGET_EXPR_INITIAL (stmt)))
> + TARGET_EXPR_NO_ELIDE (stmt) = 1;
> + break;
> +
> default:
> if (IS_TYPE_OR_DECL_P (stmt))
> *walk_subtrees = 0;
> @@ -2469,7 +2476,11 @@ cp_fold (tree x)
> }
> }
> if (nelts)
> - x = build_constructor (TREE_TYPE (x), nelts);
> + {
> + x = build_constructor (TREE_TYPE (x), nelts);
> + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (x)
> + = CONSTRUCTOR_PLACEHOLDER_BOUNDARY (org_x);
> + }
> break;
> }
> case TREE_VEC:
> --- gcc/testsuite/g++.dg/cpp1y/pr79937-1.C.jj 2018-03-15 20:40:49.283818227 +0100
> +++ gcc/testsuite/g++.dg/cpp1y/pr79937-1.C 2018-03-15 20:40:49.282818227 +0100
> @@ -0,0 +1,23 @@
> +// PR c++/79937
> +// { dg-do run { target c++14 } }
> +
> +struct C {};
> +
> +struct X {
> + unsigned i;
> + unsigned n = i;
> +};
> +
> +C
> +bar (X x)
> +{
> + if (x.i != 1 || x.n != 1)
> + __builtin_abort ();
> + return {};
> +}
> +
> +int
> +main ()
> +{
> + C c = bar (X {1});
> +}
> --- gcc/testsuite/g++.dg/cpp1y/pr79937-2.C.jj 2018-03-15 20:40:49.283818227 +0100
> +++ gcc/testsuite/g++.dg/cpp1y/pr79937-2.C 2018-03-15 20:40:49.283818227 +0100
> @@ -0,0 +1,24 @@
> +// PR c++/79937
> +// { dg-do run { target c++14 } }
> +
> +struct C {};
> +
> +struct X {
> + unsigned i;
> + unsigned n = i;
> + unsigned m = i;
> +};
> +
> +C
> +bar (X x)
> +{
> + if (x.i != 1 || x.n != 2 || x.m != 1)
> + __builtin_abort ();
> + return {};
> +}
> +
> +int
> +main ()
> +{
> + C c = bar (X {1, X {2}.n});
> +}
> --- gcc/testsuite/g++.dg/cpp1y/pr79937-3.C.jj 2018-03-15 20:40:49.283818227 +0100
> +++ gcc/testsuite/g++.dg/cpp1y/pr79937-3.C 2018-03-15 20:40:49.283818227 +0100
> @@ -0,0 +1,24 @@
> +// PR c++/79937
> +// { dg-do run { target c++14 } }
> +
> +struct X {
> + unsigned i;
> + unsigned n = i;
> + unsigned m = i;
> +};
> +
> +X
> +bar (X x)
> +{
> + if (x.i != 1 || x.n != 2 || x.m != 1)
> + __builtin_abort ();
> + return x;
> +}
> +
> +int
> +main ()
> +{
> + X x = bar (X {1, X {2}.n});
> + if (x.i != 1 || x.n != 2 || x.m != 1)
> + __builtin_abort ();
> +}
> --- gcc/testsuite/g++.dg/cpp1y/pr79937-4.C.jj 2018-03-15 20:40:49.283818227 +0100
> +++ gcc/testsuite/g++.dg/cpp1y/pr79937-4.C 2018-03-15 20:40:49.283818227 +0100
> @@ -0,0 +1,32 @@
> +// PR c++/79937
> +// { dg-do run { target c++14 } }
> +
> +struct X {
> + unsigned i;
> + unsigned n = i;
> +};
> +
> +X
> +bar (X x)
> +{
> + return x;
> +}
> +
> +struct Y
> +{
> + static Y bar (Y y) { return y; }
> + unsigned i;
> + unsigned n = bar (Y{2,i}).n;
> +};
> +
> +int
> +main ()
> +{
> + X x { 1, bar (X{2}).n };
> + if (x.n != 2)
> + __builtin_abort ();
> +
> + Y y { 1 };
> + if (y.n != 1)
> + __builtin_abort ();
> +}
> --- gcc/testsuite/g++.dg/cpp1y/pr82410.C.jj 2018-03-15 20:40:49.284818227 +0100
> +++ gcc/testsuite/g++.dg/cpp1y/pr82410.C 2018-03-15 20:40:49.283818227 +0100
> @@ -0,0 +1,16 @@
> +// PR c++/82410
> +// { dg-do compile { target c++14 } }
> +
> +int
> +main ()
> +{
> + struct A {};
> + struct S
> + {
> + int & p;
> + int x = p;
> + operator A () { return {}; }
> + };
> + int l;
> + [] (A) {} (S{l});
> +}
>
>
> Jakub