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.
>
> > 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
