On 2/17/19 3:34 AM, Jakub Jelinek wrote:
On Sat, Feb 16, 2019 at 08:51:33AM -1000, Jason Merrill wrote:
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.
For scalar types, as in all your testcases, we can evaluate the initializer
before the target, as C++17 wants. We probably still need your patch for
when type is a class.
If you are ok that the scalar vs. aggregate case will be handled
differently, I'm all for your patch, though I guess instead of that second
hunk it should change:
if (AGGREGATE_TYPE_P (type) || VECTOR_TYPE_P (type))
into:
if (!preeval)
and move the init = cxx_eval_constant_expression ... call into the
body of that if. I guess that means the scalar store will be handled right
even for unions then. Just wonder if similar to
if (*non_constant_p)
return t;
after target = cxx_eval_... we shouldn't have that for (both) init =
cxx_eval_... cases too.
Thanks, done.
The testcases can be all changed to work with say struct Z { int z; };
instead of int (or any other aggregate) and I think my patch or something
similar is needed.
But they would still be doing assignment, rather than initialization, so
they would still be preevaluated and work.
With unions, I think the most nasty case is when the union member to which
we want to store is active before an assignment, but is then made inactive
and later active again.
struct Z { int x, y; };
union W { Z a; long long w; };
W w {};
w.a = { 5, 0 }; // w.a becomes the active member
w.a = { (int) (w.w = 17LL + w.a.x), 2 };
So, if we don't preevaluate init, we look up w.a as { 5, 0 } active member
and try to store that, but in the meantime the init evaluation changes
active member to something different, which should invalidate w.a.
Here also we're looking at assignment. Here's a modification that still
breaks with my patch:
struct Z { int x, y; };
union W {
Z a; long long w;
constexpr W(): a({int(this->w = 42), 2}) {}
};
constexpr W w {};
static_assert (w.a.x == 42);
But it's not clear to me that the standard actually allows this. I
don't think changing the active member of a union in the mem-initializer
for another member is reasonable.
So, I'm going to apply this:
commit b5aa6e87a705496c38639b697317b0bd764dab30
Author: Jason Merrill <ja...@redhat.com>
Date: Fri Feb 15 13:09:33 2019 -1000
PR c++/89336 - multiple stores in constexpr stmt.
If we evaluate the RHS in the context of the LHS, that evaluation might
change the LHS in ways that mess with being able to store the value later.
So for assignment or scalar values, evaluate the RHS first.
* constexpr.c (cxx_eval_store_expression): Preevaluate scalar or
assigned value.
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index d946a797999..d413c6b9b27 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -3634,6 +3634,18 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
maybe_simplify_trivial_copy (target, init);
tree type = TREE_TYPE (target);
+ bool preeval = SCALAR_TYPE_P (type) || TREE_CODE (t) == MODIFY_EXPR;
+ if (preeval)
+ {
+ /* Evaluate the value to be stored without knowing what object it will be
+ stored in, so that any side-effects happen first. */
+ if (!SCALAR_TYPE_P (type))
+ new_ctx.ctor = new_ctx.object = NULL_TREE;
+ init = cxx_eval_constant_expression (&new_ctx, init, false,
+ non_constant_p, overflow_p);
+ if (*non_constant_p)
+ return t;
+ }
target = cxx_eval_constant_expression (ctx, target,
true,
non_constant_p, overflow_p);
@@ -3834,7 +3846,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
}
release_tree_vector (refs);
- if (AGGREGATE_TYPE_P (type) || VECTOR_TYPE_P (type))
+ if (!preeval)
{
/* Create a new CONSTRUCTOR in case evaluation of the initializer
wants to modify it. */
@@ -3843,21 +3855,20 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
*valp = build_constructor (type, NULL);
CONSTRUCTOR_NO_CLEARING (*valp) = no_zero_init;
}
- else if (TREE_CODE (*valp) == PTRMEM_CST)
- *valp = cplus_expand_constant (*valp);
new_ctx.ctor = *valp;
new_ctx.object = target;
+ init = cxx_eval_constant_expression (&new_ctx, init, false,
+ non_constant_p, overflow_p);
+ if (target == object)
+ /* The hash table might have moved since the get earlier. */
+ valp = ctx->values->get (object);
}
- init = cxx_eval_constant_expression (&new_ctx, init, false,
- non_constant_p, overflow_p);
/* Don't share a CONSTRUCTOR that might be changed later. */
init = unshare_constructor (init);
- if (target == object)
- /* The hash table might have moved since the get earlier. */
- valp = ctx->values->get (object);
- if (TREE_CODE (init) == CONSTRUCTOR)
+ if (*valp && TREE_CODE (*valp) == CONSTRUCTOR
+ && TREE_CODE (init) == CONSTRUCTOR)
{
/* An outer ctx->ctor might be pointing to *valp, so replace
its contents. */
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-89336-1.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-89336-1.C
new file mode 100644
index 00000000000..93fe16551a1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-89336-1.C
@@ -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, "");
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-89336-2.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-89336-2.C
new file mode 100644
index 00000000000..69889ffb2b4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-89336-2.C
@@ -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, "");
