On Wed, 18 Mar 2020, Patrick Palka wrote:
> On Tue, 17 Mar 2020, Jason Merrill wrote:
>
> > On 3/16/20 1:39 PM, Patrick Palka wrote:
> > > In this PR, we are performing constexpr evaluation of a CONSTRUCTOR of
> > > type
> > > union U which looks like
> > >
> > > {.a=foo (&<PLACEHOLDER_EXPR union U>)}.
> > >
> > > Since the function foo takes a reference to the CONSTRUCTOR we're
> > > building,
> > > it
> > > could potentially modify the CONSTRUCTOR from under us. In particular
> > > since
> > > U
> > > is a union, the evaluation of a's initializer could change the active
> > > member
> > > from a to another member -- something which cxx_eval_bare_aggregate
> > > doesn't
> > > expect to happen.
> > >
> > > Upon further investigation, it turns out this issue is not limited to
> > > constructors of UNION_TYPE and not limited to cxx_eval_bare_aggregate
> > > either.
> > > For example, within cxx_eval_store_expression we may be evaluating an
> > > assignment
> > > such as (this comes from the test pr94066-2.C):
> > >
> > > ((union U *) this)->a = TARGET_EXPR <D.2163, foo ((union U *) this)>;
> >
> > I assume this is actually an INIT_EXPR, or we would have preevaluated and
> > not
> > had this problem.
>
> Yes exactly, I should have specified that the above is an INIT_EXPR and
> not a MODIFY_EXPR.
>
> >
> > > where evaluation of foo could change the active member of *this, which was
> > > set
> > > earlier in cxx_eval_store_expression to 'a'. And if U is a RECORD_TYPE,
> > > then
> > > evaluation of foo could add new fields to *this, thereby making stale the
> > > 'valp'
> > > pointer to the target constructor_elt through which we're later assigning.
> > >
> > > So in short, it seems that both cxx_eval_bare_aggregate and
> > > cxx_eval_store_expression do not anticipate that a constructor_elt's
> > > initializer
> > > could modify the underlying CONSTRUCTOR as a side-effect.
> >
> > Oof. If this is well-formed, it's because initialization of a doesn't
> > actually start until the return statement of foo, so we're probably wrong to
> > create a CONSTRUCTOR to hold the value of 'a' before evaluating foo.
> > Perhaps
> > init_subob_ctx shouldn't preemptively create a CONSTRUCTOR, and similarly
> > for
> > the cxx_eval_store_expression !preeval code.
>
> Hmm, I think I see what you mean. I'll look into this.
In cpp0x/constexpr-array12.C we have
struct A { int ar[3]; };
constexpr A a1 = { 0, a1.ar[0] };
the initializer for a1 is a CONSTRUCTOR with the form
{.ar={0, (int) VIEW_CONVERT_EXPR<const struct A>(a1).ar[0]}}
If we don't preemptively create a CONSTRUCTOR in cxx_eval_bare_aggregate
to hold the value of 'ar' before evaluating its initializer, then we
won't be able to resolve the 'a1.ar[0]' later on, and we will reject
this otherwise valid test case with an "accessing an uninitialized array
element" diagnostic. So it seems we need to continue creating a
CONSTRUCTOR in cxx_eval_bare_aggregate before evaluating the initializer
of an aggregate sub-object to handle self-referential CONSTRUCTORs like
the one above.
Then again, clang is going with rejecting the original testcase with the
following justification: https://bugs.llvm.org/show_bug.cgi?id=45133#c1
Should we follow suit?
>
> >
> > > To fix this problem, this patch makes cxx_eval_bare_aggregate and
> > > cxx_eval_store_expression recompute the pointer to the constructor_elt
> > > through
> > > which we're assigning, after the initializer has been evaluated.
> > >
> > > I am worried that the use of get_or_insert_ctor_field in
> > > cxx_eval_bare_aggregate
> > > might introduce quadratic behavior where there wasn't before. I wonder if
> > > there's a cheap heuristic we can use in cxx_eval_bare_aggregate to
> > > determine
> > > whether "self-modification" took place, and in which case we could avoid
> > > calling
> > > get_or_insert_ctor_field and do the fast thing we that we were doing
> > > before
> > > the
> > > patch.
> >
> > We could remember the (integer) index of the constructor element and verify
> > that our sub-ctors are still at those indices before doing a new search.
>
> Good idea, this should essentially eliminate the overhead of the second
> set of calls to get_or_insert_ctor_fields in cxx_eval_store_expression
> in the common case where the initializer doesn't mutate the underlying
> CONSTRUCTORs. I added this optimization to v2 of the patch, through a
> new 'pos_hint' parameter of get_or_insert_ctor_fields.
>
> As another optimization to get_or_insert_ctor_fields, we can first check
> whether the bit_position of INDEX is greater than the bit_position of
> the last constructor element of CTOR. In his case we can immediately
> append a new constructor element to CTOR, and avoid iterating over the
> whole TYPE_FIELDS chain and over CTOR. I think this optimization
> eliminates the potential quadratic behavior in cxx_eval_bare_aggregate
> in the common case.
>
> With these two optimizations, the added overhead of the recomputations
> added by this patch should be negligible.
>
> -- >8 --
>
> gcc/cp/ChangeLog:
>
> PR c++/94066
> * constexpr.c (get_or_insert_ctor_field): Split out (while adding
> handling for VECTOR_TYPEs, and common-case optimizations) from ...
> (cxx_eval_store_expression): ... here. Record the sequence of indexes
> into INDEXES that yields the suboject we're assigning to. Record the
> integer offsets of the constructor indexes we're assigning through into
> INDEX_POSITIONS. After evaluating the initializer of the store
> expression, recompute valp using INDEXES and using INDEX_POSITIONS as
> hints.
> (cxx_eval_bare_aggregate): Use get_or_insert_ctor_field to recompute the
> pointer to the constructor_elt we're assigning through after evaluating
> each initializer.
>
> gcc/testsuite/ChangeLog:
>
> PR c++/94066
> * g++.dg/cpp1y/pr94066.C: New test.
> * g++.dg/cpp1y/pr94066-2.C: New test.
> * g++.dg/cpp1y/pr94066-3.C: New test.
> * g++.dg/cpp1y/pr94066-4.C: New test.
> * g++.dg/cpp1y/pr94066-5.C: New test.
> ---
> gcc/cp/constexpr.c | 219 ++++++++++++++++---------
> gcc/testsuite/g++.dg/cpp1y/pr94066-2.C | 21 +++
> gcc/testsuite/g++.dg/cpp1y/pr94066-3.C | 21 +++
> gcc/testsuite/g++.dg/cpp1y/pr94066-4.C | 22 +++
> gcc/testsuite/g++.dg/cpp1y/pr94066-5.C | 18 ++
> gcc/testsuite/g++.dg/cpp1y/pr94066.C | 20 +++
> 6 files changed, 242 insertions(+), 79 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
> create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
> create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-4.C
> create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066-5.C
> create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr94066.C
>
> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
> index 192face9a3a..9ea64467661 100644
> --- a/gcc/cp/constexpr.c
> +++ b/gcc/cp/constexpr.c
> @@ -3144,6 +3144,107 @@ find_array_ctor_elt (tree ary, tree dindex, bool
> insert)
> return -1;
> }
>
> +/* Return a pointer to the constructor_elt of CTOR which matches INDEX. If
> no
> + matching constructor_elt exists, then add one to CTOR. Emit an
> appropriate
> + diagnostic if CTOR constructs a UNION_TYPE and adding INDEX would change
> the
> + active member of the union, unless QUIET is true. As an optimization,
> + if POS_HINT is non-negative then it is used as a guess for the (integer)
> + index of the matching constructor_elt within CTOR. */
> +
> +static constructor_elt *
> +get_or_insert_ctor_field (tree ctor, tree index, int pos_hint,
> + bool quiet, bool *non_constant_p)
> +{
> + tree type = TREE_TYPE (ctor);
> + if (TREE_CODE (type) == VECTOR_TYPE && index == NULL_TREE)
> + {
> + CONSTRUCTOR_APPEND_ELT (CONSTRUCTOR_ELTS (ctor), index, NULL_TREE);
> + return &CONSTRUCTOR_ELTS (ctor)->last();
> + }
> + else if (TREE_CODE (type) == ARRAY_TYPE || TREE_CODE (type) == VECTOR_TYPE)
> + {
> + HOST_WIDE_INT i = find_array_ctor_elt (ctor, index, /*insert*/true);
> + gcc_assert (i >= 0);
> + constructor_elt *cep = CONSTRUCTOR_ELT (ctor, i);
> + gcc_assert (cep->index == NULL_TREE
> + || TREE_CODE (cep->index) != RANGE_EXPR);
> + return cep;
> + }
> + else
> + {
> + gcc_assert (TREE_CODE (index) == FIELD_DECL);
> +
> + /* We must keep the CONSTRUCTOR's ELTS in FIELD order.
> + Usually we meet initializers in that order, but it is
> + possible for base types to be placed not in program
> + order. */
> + tree fields = TYPE_FIELDS (DECL_CONTEXT (index));
> + unsigned HOST_WIDE_INT idx = 0;
> + constructor_elt *cep = NULL;
> +
> + if (TREE_CODE (type) == UNION_TYPE && CONSTRUCTOR_NELTS (ctor)
> + && CONSTRUCTOR_ELT (ctor, 0)->index != index)
> + {
> + if (cxx_dialect < cxx2a)
> + {
> + if (!quiet)
> + error ("change of the active member of a union from %qD to %qD",
> + CONSTRUCTOR_ELT (ctor, 0)->index,
> + index);
> + *non_constant_p = true;
> + }
> + /* Changing active member. */
> + vec_safe_truncate (CONSTRUCTOR_ELTS (ctor), 0);
> + }
> + /* Check the hint. */
> + else if (pos_hint >= 0 && (unsigned)pos_hint < CONSTRUCTOR_NELTS (ctor)
> + && CONSTRUCTOR_ELT (ctor, pos_hint)->index == index)
> + {
> + cep = CONSTRUCTOR_ELT (ctor, pos_hint);
> + goto found;
> + }
> + /* If the bit offset of INDEX is at larger than that of the endmost
> + constructor_elt, then just immediately append a new constructor_elt to
> + the end of CTOR. */
> + else if (TREE_CODE (type) == RECORD_TYPE && CONSTRUCTOR_NELTS (ctor)
> + && tree_int_cst_compare (bit_position (index),
> + bit_position (CONSTRUCTOR_ELTS (ctor)
> + ->last().index)) > 0)
> + {
> + idx = CONSTRUCTOR_NELTS (ctor);
> + goto insert;
> + }
> +
> + for (; vec_safe_iterate (CONSTRUCTOR_ELTS (ctor), idx, &cep);
> + idx++, fields = DECL_CHAIN (fields))
> + {
> + if (index == cep->index)
> + goto found;
> +
> + /* The field we're initializing must be on the field
> + list. Look to see if it is present before the
> + field the current ELT initializes. */
> + for (; fields != cep->index; fields = DECL_CHAIN (fields))
> + if (index == fields)
> + goto insert;
> + }
> +
> + /* We fell off the end of the CONSTRUCTOR, so insert a new
> + entry at the end. */
> + insert:
> + {
> + constructor_elt ce = { index, NULL_TREE };
> +
> + vec_safe_insert (CONSTRUCTOR_ELTS (ctor), idx, ce);
> + cep = CONSTRUCTOR_ELT (ctor, idx);
> + }
> + found:;
> +
> + return cep;
> + }
> +}
> +
> +
> /* Under the control of CTX, issue a detailed diagnostic for
> an out-of-bounds subscript INDEX into the expression ARRAY. */
>
> @@ -3747,10 +3848,14 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx,
> tree t,
> {
> tree orig_value = value;
> init_subob_ctx (ctx, new_ctx, index, value);
> + int pos_hint = -1;
> if (new_ctx.ctor != ctx->ctor)
> - /* If we built a new CONSTRUCTOR, attach it now so that other
> - initializers can refer to it. */
> - CONSTRUCTOR_APPEND_ELT (*p, index, new_ctx.ctor);
> + {
> + /* If we built a new CONSTRUCTOR, attach it now so that other
> + initializers can refer to it. */
> + CONSTRUCTOR_APPEND_ELT (*p, index, new_ctx.ctor);
> + pos_hint = vec_safe_length (*p) - 1;
> + }
> tree elt = cxx_eval_constant_expression (&new_ctx, value,
> lval,
> non_constant_p, overflow_p);
> @@ -3784,14 +3889,15 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx,
> tree t,
> }
> else
> {
> - if (new_ctx.ctor != ctx->ctor)
> - {
> - /* We appended this element above; update the value. */
> - gcc_assert ((*p)->last().index == index);
> - (*p)->last().value = elt;
> - }
> - else
> - CONSTRUCTOR_APPEND_ELT (*p, index, elt);
> + /* The underlying CONSTRUCTOR might have been mutated by the
> + initializer, so recompute the location of the target
> + constructor_elt. */
> + iloc_sentinel ils (cp_expr_location (t));
> + constructor_elt *cep
> + = get_or_insert_ctor_field (ctx->ctor, index, pos_hint,
> + ctx->quiet, non_constant_p);
> + cep->value = elt;
> +
> /* Adding or replacing an element might change the ctor's flags. */
> TREE_CONSTANT (ctx->ctor) = constant_p;
> TREE_SIDE_EFFECTS (ctx->ctor) = side_effects_p;
> @@ -4566,7 +4672,8 @@ cxx_eval_store_expression (const constexpr_ctx *ctx,
> tree t,
> type = TREE_TYPE (object);
> bool no_zero_init = true;
>
> - releasing_vec ctors;
> + releasing_vec ctors, indexes;
> + auto_vec<int> index_positions;
> while (!refs->is_empty ())
> {
> if (*valp == NULL_TREE)
> @@ -4607,77 +4714,23 @@ cxx_eval_store_expression (const constexpr_ctx *ctx,
> tree t,
> subobjects will also be zero-initialized. */
> no_zero_init = CONSTRUCTOR_NO_CLEARING (*valp);
>
> - vec_safe_push (ctors, *valp);
> -
> enum tree_code code = TREE_CODE (type);
> type = refs->pop();
> tree index = refs->pop();
>
> - constructor_elt *cep = NULL;
> - if (code == ARRAY_TYPE)
> - {
> - HOST_WIDE_INT i
> - = find_array_ctor_elt (*valp, index, /*insert*/true);
> - gcc_assert (i >= 0);
> - cep = CONSTRUCTOR_ELT (*valp, i);
> - gcc_assert (cep->index == NULL_TREE
> - || TREE_CODE (cep->index) != RANGE_EXPR);
> - }
> - else
> - {
> - gcc_assert (TREE_CODE (index) == FIELD_DECL);
> -
> - /* We must keep the CONSTRUCTOR's ELTS in FIELD order.
> - Usually we meet initializers in that order, but it is
> - possible for base types to be placed not in program
> - order. */
> - tree fields = TYPE_FIELDS (DECL_CONTEXT (index));
> - unsigned HOST_WIDE_INT idx;
> -
> - if (code == UNION_TYPE && CONSTRUCTOR_NELTS (*valp)
> - && CONSTRUCTOR_ELT (*valp, 0)->index != index)
> - {
> - if (cxx_dialect < cxx2a)
> - {
> - if (!ctx->quiet)
> - error_at (cp_expr_loc_or_input_loc (t),
> - "change of the active member of a union "
> - "from %qD to %qD",
> - CONSTRUCTOR_ELT (*valp, 0)->index,
> - index);
> - *non_constant_p = true;
> - }
> - /* Changing active member. */
> - vec_safe_truncate (CONSTRUCTOR_ELTS (*valp), 0);
> - no_zero_init = true;
> - }
> + vec_safe_push (ctors, *valp);
> + vec_safe_push (indexes, index);
>
> - for (idx = 0;
> - vec_safe_iterate (CONSTRUCTOR_ELTS (*valp), idx, &cep);
> - idx++, fields = DECL_CHAIN (fields))
> - {
> - if (index == cep->index)
> - goto found;
> -
> - /* The field we're initializing must be on the field
> - list. Look to see if it is present before the
> - field the current ELT initializes. */
> - for (; fields != cep->index; fields = DECL_CHAIN (fields))
> - if (index == fields)
> - goto insert;
> - }
> + if (code == UNION_TYPE && CONSTRUCTOR_NELTS (*valp)
> + && CONSTRUCTOR_ELT (*valp, 0)->index != index)
> + no_zero_init = true;
>
> - /* We fell off the end of the CONSTRUCTOR, so insert a new
> - entry at the end. */
> - insert:
> - {
> - constructor_elt ce = { index, NULL_TREE };
> + iloc_sentinel ils (cp_expr_location (t));
> + constructor_elt *cep
> + = get_or_insert_ctor_field (*valp, index, /*pos_hint=*/-1,
> + ctx->quiet, non_constant_p);
> + index_positions.safe_push (cep - CONSTRUCTOR_ELTS (*valp)->address ());
>
> - vec_safe_insert (CONSTRUCTOR_ELTS (*valp), idx, ce);
> - cep = CONSTRUCTOR_ELT (*valp, idx);
> - }
> - found:;
> - }
> valp = &cep->value;
> }
>
> @@ -4758,9 +4811,17 @@ cxx_eval_store_expression (const constexpr_ctx *ctx,
> tree t,
> init = tinit;
> init = cxx_eval_constant_expression (&new_ctx, init, false,
> non_constant_p, overflow_p);
> - if (ctors->is_empty())
> - /* The hash table might have moved since the get earlier. */
> - valp = ctx->global->values.get (object);
> + /* The hash table might have moved since the get earlier. */
> + valp = ctx->global->values.get (object);
> + /* And the underlying CONSTRUCTORs might have been mutated by the
> + initializer, so we must recompute VALP. */
> + for (unsigned i = 0; i < vec_safe_length (indexes); i++)
> + {
> + constructor_elt *cep
> + = get_or_insert_ctor_field (*valp, indexes[i], index_positions[i],
> + /*quiet=*/true, non_constant_p);
> + valp = &cep->value;
> + }
> }
>
> /* Don't share a CONSTRUCTOR that might be changed later. */
> diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
> b/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
> new file mode 100644
> index 00000000000..ec8210a9d73
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
> @@ -0,0 +1,21 @@
> +// PR c++/94066
> +// { dg-do compile { target c++14 } }
> +
> +struct A { long x; };
> +
> +union U;
> +constexpr A foo(U *up);
> +
> +union U {
> + U() = default;
> + A a = foo(this); int y;
> +};
> +
> +constexpr A foo(U *up) {
> + up->y = 11; // { dg-error ".U::a. to .U::y." "" { target c++17_down } }
> + return {42};
> +}
> +
> +extern constexpr U u = {}; // { dg-error ".U::y. to .U::a." "" { target
> c++17_down } }
> +
> +static_assert(u.a.x == 42, ""); // { dg-error "non-constant" "" { target
> c++17_down } }
> diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
> b/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
> new file mode 100644
> index 00000000000..b68c975d569
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
> @@ -0,0 +1,21 @@
> +// PR c++/94066
> +// { dg-do compile { target c++14 } }
> +
> +struct A { long x; };
> +
> +struct U;
> +constexpr A foo(U *up);
> +
> +struct U {
> + A a = foo(this); int y;
> +};
> +
> +constexpr A foo(U *up) {
> + up->y = 11;
> + return {42};
> +}
> +
> +extern constexpr U u = {};
> +
> +static_assert(u.y == 11, "");
> +static_assert(u.a.x == 42, "");
> diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066-4.C
> b/gcc/testsuite/g++.dg/cpp1y/pr94066-4.C
> new file mode 100644
> index 00000000000..0362c939faf
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-4.C
> @@ -0,0 +1,22 @@
> +// PR c++/94066
> +// { dg-do compile { target c++14 } }
> +
> +struct A { long x; };
> +
> +struct U;
> +constexpr A foo(U *up);
> +
> +struct U {
> + U() = default;
> + int y; A a = foo(this);
> +};
> +
> +constexpr A foo(U *up) {
> + up->y = 11;
> + return {42};
> +}
> +
> +extern constexpr U u = {};
> +
> +static_assert(u.y == 11, "");
> +static_assert(u.a.x == 42, "");
> diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066-5.C
> b/gcc/testsuite/g++.dg/cpp1y/pr94066-5.C
> new file mode 100644
> index 00000000000..63069f3ea5f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-5.C
> @@ -0,0 +1,18 @@
> +// PR c++/94066
> +// { dg-do compile { target c++14 } }
> +
> +union U;
> +constexpr int foo(U *up);
> +
> +union U {
> + int a = foo(this); int y;
> +};
> +
> +constexpr int foo(U *up) {
> + up->y = 11;
> + return 42;
> +}
> +
> +extern constexpr U u = {}; // { dg-error ".U::y. to .U::a." "" { target
> c++17_down } }
> +
> +static_assert(u.a == 42, ""); // { dg-error "non-constant" "" { target
> c++17_down } }
> diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066.C
> b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
> new file mode 100644
> index 00000000000..11912199406
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
> @@ -0,0 +1,20 @@
> +// PR c++/94066
> +// { dg-do compile { target c++14 } }
> +
> +struct A { long x; };
> +
> +union U;
> +constexpr A foo(U *up);
> +
> +union U {
> + A a = foo(this); int y;
> +};
> +
> +constexpr A foo(U *up) {
> + up->y = 11; // { dg-error ".U::a. to .U::y." "" { target c++17_down } }
> + return {42};
> +}
> +
> +extern constexpr U u = {}; // { dg-error ".U::y. to .U::a." "" { target
> c++17_down } }
> +
> +static_assert(u.a.x == 42, ""); // { dg-error "non-constant" "" { target
> c++17_down } }
> --
> 2.26.0.rc1.11.g30e9940356
>
>
