[PATCH] c++: Fix constexpr evaluation of self-modifying CONSTRUCTORs [PR94066]
Patrick Palka
ppalka@redhat.com
Mon Mar 16 17:39:25 GMT 2020
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)>;
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.
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.
Tested on x86_64-pc-linux-gnu, but a full bootstrap is in progress. Does this
look like the right approach?
gcc/cp/ChangeLog:
PR c++/94066
* constexpr.c (get_or_insert_ctor_field): Split out (while adding
handling for VECTOR_TYPEs) from ...
(cxx_eval_store_expression): ... here. Record the sequence of indexes
into INDEXES that yields the suboject we're assigning to. After
evaluating the initializer of the store expression, recompute valp using
INDEXES.
(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 | 184 +++++++++++++++----------
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, 210 insertions(+), 76 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..9fcf9f33457 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -3144,6 +3144,88 @@ 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 it 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. */
+
+static constructor_elt *
+get_or_insert_ctor_field (tree ctor, tree index,
+ 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;
+
+ 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);
+ }
+
+ constructor_elt *cep = NULL;
+ for (idx = 0;
+ 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. */
@@ -3784,14 +3866,12 @@ 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);
+ iloc_sentinel ils (cp_expr_location (t));
+ constructor_elt *cep
+ = get_or_insert_ctor_field (ctx->ctor, index,
+ 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 +4646,7 @@ 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;
while (!refs->is_empty ())
{
if (*valp == NULL_TREE)
@@ -4607,77 +4687,21 @@ 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, ctx->quiet, non_constant_p);
- vec_safe_insert (CONSTRUCTOR_ELTS (*valp), idx, ce);
- cep = CONSTRUCTOR_ELT (*valp, idx);
- }
- found:;
- }
valp = &cep->value;
}
@@ -4758,9 +4782,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 may have been modified by the
+ initializer. */
+ for (unsigned i = 0; i < vec_safe_length (indexes); i++)
+ {
+ constructor_elt *cep
+ = get_or_insert_ctor_field (*valp, indexes[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
More information about the Gcc-patches
mailing list