Hi!
As the following testcases shows, cxx_eval_store_expression mishandles the
case when constexpr evaluation of the rhs (init) modifies part of the ctor
that the store stores into.
Except for unions (see below) I believe it is fine the way the outer refs
are handled, because we advance into another CONSTRUCTOR and if the pointer
to that is reallocated or memmoved somewhere else, it shouldn't matter.
For the last CONSTRUCTOR, we set valp to
&CONSTRUCTOR_ELT (*valp, something)->value
but CONSTRUCTOR_ELTS is not a linked list, but vector, which can be
reallocated if we need more elements, or vec_safe_insert some earlier
element memmoves further elts later in the same vector.
The likely case is still that nothing has changed in between, so this patch
just quickly verifies if that is the case (by comparing
CONSTRUCTOR_ELT (ctor, 0) with the previously saved value of that and by
checking if at the spot in the vector is the expected index). If that is
the case, it doesn't do anything else, otherwise it updates the valp
pointer.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Note, at least by my reading of the standard, the union case seems to be
mishandled (and the patch doesn't change anything on that). The union
member being stored should IMHO become active after evaluating both the
lhs and rhs, but before the actual store, while the current code
invalidates the previously active member already before evaluating the rhs
(if it is different from the upcoming one). I think
constexpr int foo () {
union U { int a; long b; };
union V { union U u; short v; };
V w {};
w.u.a = w.v = w.u.b = 5L;
return w.u.a;
}
static_assert (foo () == 5, "");
should be valid (though clang++ rejects it as well). If it is indeed valid,
it is not going to be very easy to implement properly, I guess we shouldn't
if (last_code == UNION_TYPE && CONSTRUCTOR_NELTS (*valp)
&& CONSTRUCTOR_ELT (*valp, 0)->index != index)
/* Changing active member. */
vec_safe_truncate (CONSTRUCTOR_ELTS (*valp), 0);
early, on the other side, at least with the way it currently works we want
to allocate a new CONSTRUCTOR if we are going to store to a further
component of the union member and we likely need to store it somewhere
for the benefit of the rhs constexpr evaluation, because I think even
when some member is not active before the outer store_expression, it can
become active during the rhs evaluation. Say:
union U { int a[5]; long b; };
union V { union U u; short v; };
V w {};
w.v = 5L; // Make v the active member
w.u.a[3] = w.u.a[1] = w.v;
We can't invalidate w.v before handling the rhs, but if we create a new
CONSTRUCTOR for w.u for the w.u.a[3] = assignment and we'd create a different
one for the w.u.a[1] = assignment, then w.u.a[1] wouldn't show in w.
I wonder if we shouldn't temporarily allow the UNION_TYPE CONSTRUCTOR_ELTS
to contain more than one element, where e.g. the first one would be always
the currently active one (that is what we'd use for any reads) and after
that would be other (preferrably only empty CONSTRUCTORs containing at most
other CONSTRUCTORs but no real fields), which we'd use when we'd want to
activate something union member. Plus remember in store_expression if
a particular union CONSTRUCTOR had at most one element before and kill all
the other ones at the end at that point. Though, I admit it is not very
well thought out.
2019-02-13 Jakub Jelinek <ja...@redhat.com>
PR c++/89336
* constexpr.c (cxx_eval_store_expression): Recompute valp after
constexpr evaluation of init if CONSTRUCTOR_ELTS have been
reallocated or earlier elements inserted.
* g++.dg/cpp1y/constexpr-89336-1.C: New test.
* g++.dg/cpp1y/constexpr-89336-2.C: New test.
--- gcc/cp/constexpr.c.jj 2019-02-12 21:48:51.000000000 +0100
+++ gcc/cp/constexpr.c 2019-02-13 20:01:25.101100373 +0100
@@ -3733,6 +3733,10 @@ cxx_eval_store_expression (const constex
bool no_zero_init = true;
vec<tree,va_gc> *ctors = make_tree_vector ();
+ unsigned HOST_WIDE_INT last_idx = 0;
+ tree last_index = NULL_TREE;
+ enum tree_code last_code = ERROR_MARK;
+ constructor_elt *last_celt = NULL;
while (!refs->is_empty())
{
if (*valp == NULL_TREE)
@@ -3775,12 +3779,12 @@ cxx_eval_store_expression (const constex
vec_safe_push (ctors, *valp);
- enum tree_code code = TREE_CODE (type);
+ last_code = TREE_CODE (type);
type = refs->pop();
tree index = refs->pop();
constructor_elt *cep = NULL;
- if (code == ARRAY_TYPE)
+ if (last_code == ARRAY_TYPE)
{
HOST_WIDE_INT i
= find_array_ctor_elt (*valp, index, /*insert*/true);
@@ -3799,7 +3803,7 @@ cxx_eval_store_expression (const constex
tree fields = TYPE_FIELDS (DECL_CONTEXT (index));
unsigned HOST_WIDE_INT idx;
- if (code == UNION_TYPE && CONSTRUCTOR_NELTS (*valp)
+ if (last_code == UNION_TYPE && CONSTRUCTOR_NELTS (*valp)
&& CONSTRUCTOR_ELT (*valp, 0)->index != index)
/* Changing active member. */
vec_safe_truncate (CONSTRUCTOR_ELTS (*valp), 0);
@@ -3830,6 +3834,9 @@ cxx_eval_store_expression (const constex
}
found:;
}
+ last_celt = CONSTRUCTOR_ELT (*valp, 0);
+ last_idx = cep - last_celt;
+ last_index = cep->index;
valp = &cep->value;
}
release_tree_vector (refs);
@@ -3856,6 +3863,40 @@ cxx_eval_store_expression (const constex
if (target == object)
/* The hash table might have moved since the get earlier. */
valp = ctx->values->get (object);
+ /* The above cxx_eval_constant_expression call might have added further
+ ctor elements before the *valp one, or might have added some elements
+ after it and reallocate the vector. In that case, need to recompute
+ valp. In all the cases, the element we've added above should still
+ be there. */
+ else if (!vec_safe_is_empty (ctors)
+ && (CONSTRUCTOR_ELT (ctors->last (), 0) != last_celt
+ || last_celt[last_idx].index != last_index))
+ {
+ tree ctor = ctors->last ();
+ constructor_elt *cep = NULL;
+ if (last_code == ARRAY_TYPE)
+ {
+ HOST_WIDE_INT i
+ = find_array_ctor_elt (ctor, last_index, /*insert*/false);
+ gcc_assert (i >= 0);
+ cep = CONSTRUCTOR_ELT (ctor, i);
+ gcc_assert (TREE_CODE (cep->index) != RANGE_EXPR);
+ }
+ else if (last_code == UNION_TYPE)
+ {
+ cep = CONSTRUCTOR_ELT (ctor, 0);
+ cep->index = last_index;
+ }
+ else
+ {
+ for (unsigned HOST_WIDE_INT idx = last_idx;
+ vec_safe_iterate (CONSTRUCTOR_ELTS (ctor), idx, &cep); idx++)
+ if (last_index == cep->index)
+ break;
+ gcc_assert (cep);
+ }
+ valp = &cep->value;
+ }
if (TREE_CODE (init) == CONSTRUCTOR)
{
--- gcc/testsuite/g++.dg/cpp1y/constexpr-89336-1.C.jj 2019-02-13
20:12:54.836796658 +0100
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-89336-1.C 2019-02-13
20:07:27.645154780 +0100
@@ -0,0 +1,35 @@
+// PR c++/89336
+// { dg-do compile { target c++14 } }
+
+template <typename T, int N> struct A {
+ T a[N];
+ constexpr T &operator[] (int x) { return a[x]; }
+ constexpr const T &operator[] (int x) const { return a[x]; }
+};
+
+constexpr A<int, 16>
+foo ()
+{
+ A<int, 16> r{};
+ for (int i = 0; i < 6; ++i)
+ r[i + 8] = r[i] = i + 1;
+ return r;
+}
+
+constexpr auto x = foo ();
+static_assert (x[0] == 1, "");
+static_assert (x[1] == 2, "");
+static_assert (x[2] == 3, "");
+static_assert (x[3] == 4, "");
+static_assert (x[4] == 5, "");
+static_assert (x[5] == 6, "");
+static_assert (x[6] == 0, "");
+static_assert (x[7] == 0, "");
+static_assert (x[8] == 1, "");
+static_assert (x[9] == 2, "");
+static_assert (x[10] == 3, "");
+static_assert (x[11] == 4, "");
+static_assert (x[12] == 5, "");
+static_assert (x[13] == 6, "");
+static_assert (x[14] == 0, "");
+static_assert (x[15] == 0, "");
--- gcc/testsuite/g++.dg/cpp1y/constexpr-89336-2.C.jj 2019-02-13
20:22:44.059172675 +0100
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-89336-2.C 2019-02-13
20:22:03.683835843 +0100
@@ -0,0 +1,56 @@
+// PR c++/89336
+// { dg-do compile { target c++14 } }
+
+constexpr int
+foo ()
+{
+ int a[16] = {};
+ int r = 0;
+ a[15] = a[14] = a[13] = a[12] = a[11] = a[10] = a[9] = a[8]
+ = a[7] = a[6] = a[5] = a[4] = a[3] = a[2] = a[1] = a[0] = 5;
+ for (int i = 0; i < 16; ++i)
+ r += a[i];
+ return r;
+}
+
+static_assert (foo () == 16 * 5, "");
+
+struct A { int a, b, c, d, e, f, g, h, i, j, k, l, m, n, o, p; };
+
+constexpr int
+bar ()
+{
+ A a {};
+ a.p = a.o = a.n = a.m = a.l = a.k = a.j = a.i
+ = a.h = a.g = a.f = a.e = a.d = a.c = a.b = a.a = 8;
+ return a.a + a.b + a.c + a.d + a.e + a.f + a.g + a.h
+ + a.i + a.j + a.k + a.l + a.m + a.n + a.o + a.p;
+}
+
+static_assert (bar () == 16 * 8, "");
+
+constexpr int
+baz ()
+{
+ int a[16] = {};
+ int r = 0;
+ a[0] = a[1] = a[2] = a[3] = a[4] = a[5] = a[6] = a[7]
+ = a[8] = a[9] = a[10] = a[11] = a[12] = a[13] = a[14] = a[15] = 7;
+ for (int i = 0; i < 16; ++i)
+ r += a[i];
+ return r;
+}
+
+static_assert (baz () == 16 * 7, "");
+
+constexpr int
+qux ()
+{
+ A a {};
+ a.a = a.b = a.c = a.d = a.e = a.f = a.g = a.h
+ = a.i = a.j = a.k = a.l = a.m = a.n = a.o = a.p = 6;
+ return a.a + a.b + a.c + a.d + a.e + a.f + a.g + a.h
+ + a.i + a.j + a.k + a.l + a.m + a.n + a.o + a.p;
+}
+
+static_assert (qux () == 16 * 6, "");
Jakub
