* [C++ PATCH] Fix cxx_eval_store_expression (PR c++/89336)
@ 2019-02-13 23:02 Jakub Jelinek
2019-02-16 18:51 ` Jason Merrill
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Jelinek @ 2019-02-13 23:02 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches
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 <jakub@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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [C++ PATCH] Fix cxx_eval_store_expression (PR c++/89336)
2019-02-13 23:02 [C++ PATCH] Fix cxx_eval_store_expression (PR c++/89336) Jakub Jelinek
@ 2019-02-16 18:51 ` Jason Merrill
2019-02-17 13:34 ` Jakub Jelinek
0 siblings, 1 reply; 9+ messages in thread
From: Jason Merrill @ 2019-02-16 18:51 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 2744 bytes --]
On 2/13/19 6:02 PM, Jakub Jelinek wrote:
> 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.
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.
> 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
This testcase will also work if we evaluate scalar init first. The
difficult case is when type is a class, something like
struct A { int i; constexpr A(): i(42) { } };
struct B { A a; };
struct C { A a; };
union U { B b; C c; constexpr U(): b() {} };
constexpr int f()
{
U u;
u.c.a = u.b.a;
return u.c.a.i;
}
static_assert (f() == 42);
But actually, we should be able to evaluate init first in class
assignment cases as well, making this also work; we only need to have
the target available first for initialization, in case the constructor
refers to the object by another path. We likely still need your patch
for that case, though I'm having trouble coming up with a testcase.
[-- Attachment #2: preeval.dif --]
[-- Type: text/plain, Size: 1453 bytes --]
commit b7cf563e1715909221f783747bb5e194dd38d803
Author: Jason Merrill <jason@redhat.com>
Date: Fri Feb 15 13:09:33 2019 -1000
preeval
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 923763faa0a..ea1e999620e 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -3634,6 +3634,16 @@ 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);
+ }
target = cxx_eval_constant_expression (ctx, target,
true,
non_constant_p, overflow_p);
@@ -3849,8 +3859,9 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
new_ctx.object = target;
}
- init = cxx_eval_constant_expression (&new_ctx, init, false,
- non_constant_p, overflow_p);
+ if (!preeval)
+ 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)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [C++ PATCH] Fix cxx_eval_store_expression (PR c++/89336)
2019-02-16 18:51 ` Jason Merrill
@ 2019-02-17 13:34 ` Jakub Jelinek
2019-02-19 1:01 ` Jason Merrill
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Jelinek @ 2019-02-17 13:34 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches
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.
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.
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.
Jakub
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [C++ PATCH] Fix cxx_eval_store_expression (PR c++/89336)
2019-02-17 13:34 ` Jakub Jelinek
@ 2019-02-19 1:01 ` Jason Merrill
2019-02-19 14:01 ` Jakub Jelinek
0 siblings, 1 reply; 9+ messages in thread
From: Jason Merrill @ 2019-02-19 1:01 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 2591 bytes --]
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:
[-- Attachment #2: 89336.diff --]
[-- Type: text/x-patch, Size: 5563 bytes --]
commit b5aa6e87a705496c38639b697317b0bd764dab30
Author: Jason Merrill <jason@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, "");
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [C++ PATCH] Fix cxx_eval_store_expression (PR c++/89336)
2019-02-19 1:01 ` Jason Merrill
@ 2019-02-19 14:01 ` Jakub Jelinek
2019-02-19 21:28 ` Jason Merrill
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Jelinek @ 2019-02-19 14:01 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches
On Mon, Feb 18, 2019 at 03:01:14PM -1000, Jason Merrill wrote:
> 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.
There is in [expr.const]/2:
an lvalue-to-rvalue conversion (7.1) that is applied to a glvalue that refers to a non-active member of a
union or a subobject thereof;
an assignment expression (8.18) or invocation of an assignment operator (15.8) that would change the
active member of a union;
in C++17 it seems, so maybe my union testcases are accepts-invalid.
This has been introduced in P0137R1 and removed again in P1330R0. Does that
mean e.g. following is valid in C++14, invalid in C++17 and valid again in
C++20? Or has one of the above papers changed retroactively previous
standards?
// PR c++/89336
// { dg-do compile { target c++14 } }
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, "");
constexpr int
bar ()
{
union U { int a[5]; long b; };
union V { union U u; short v; };
V w {};
w.v = 5;
w.u.a[3] = w.u.a[1] = w.v;
return w.u.a[1] + w.u.a[3];
}
static_assert (bar () == 10, "");
struct Z { int x, y; };
constexpr Z
baz ()
{
union W { Z a; long long w; };
W w {};
w.a = { 5, 0 };
w.a = { (int) (w.w = 17LL + w.a.x), 2 };
return w.a;
}
static_assert (baz ().x == 22, "");
static_assert (baz ().y == 2, "");
Jakub
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [C++ PATCH] Fix cxx_eval_store_expression (PR c++/89336)
2019-02-19 14:01 ` Jakub Jelinek
@ 2019-02-19 21:28 ` Jason Merrill
2019-02-19 22:03 ` Jakub Jelinek
0 siblings, 1 reply; 9+ messages in thread
From: Jason Merrill @ 2019-02-19 21:28 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches List
On Tue, Feb 19, 2019 at 4:00 AM Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Feb 18, 2019 at 03:01:14PM -1000, Jason Merrill wrote:
> > 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.
>
> There is in [expr.const]/2:
>
> an lvalue-to-rvalue conversion (7.1) that is applied to a glvalue that refers to a non-active member of a
> union or a subobject thereof;
>
> an assignment expression (8.18) or invocation of an assignment operator (15.8) that would change the
> active member of a union;
>
> in C++17 it seems, so maybe my union testcases are accepts-invalid.
> This has been introduced in P0137R1 and removed again in P1330R0. Does that
> mean e.g. following is valid in C++14, invalid in C++17 and valid again in
> C++20? Or has one of the above papers changed retroactively previous
> standards?
Before P0137 I believe foo and bar were arguably undefined.
Jason
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [C++ PATCH] Fix cxx_eval_store_expression (PR c++/89336)
2019-02-19 21:28 ` Jason Merrill
@ 2019-02-19 22:03 ` Jakub Jelinek
2019-02-20 7:47 ` Jakub Jelinek
2019-02-20 21:12 ` Jason Merrill
0 siblings, 2 replies; 9+ messages in thread
From: Jakub Jelinek @ 2019-02-19 22:03 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches List
On Tue, Feb 19, 2019 at 11:28:22AM -1000, Jason Merrill wrote:
> On Tue, Feb 19, 2019 at 4:00 AM Jakub Jelinek <jakub@redhat.com> wrote:
> > On Mon, Feb 18, 2019 at 03:01:14PM -1000, Jason Merrill wrote:
> > > 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.
> >
> > There is in [expr.const]/2:
> >
> > an lvalue-to-rvalue conversion (7.1) that is applied to a glvalue that refers to a non-active member of a
> > union or a subobject thereof;
> >
> > an assignment expression (8.18) or invocation of an assignment operator (15.8) that would change the
> > active member of a union;
> >
> > in C++17 it seems, so maybe my union testcases are accepts-invalid.
> > This has been introduced in P0137R1 and removed again in P1330R0. Does that
> > mean e.g. following is valid in C++14, invalid in C++17 and valid again in
> > C++20? Or has one of the above papers changed retroactively previous
> > standards?
>
> Before P0137 I believe foo and bar were arguably undefined.
I see, before that it was:
"an lvalue-to-rvalue conversion (4.1) or modification (5.18, 5.2.6, 5.3.2) that is applied
to a glvalue that refers to a non-active member of a union or a subobject thereof;"
So, do we want something like this then (or free all vectors immediately
there and return immediately)?
2019-02-19 Jakub Jelinek <jakub@redhat.com>
PR c++/89336
* constexpr.c (cxx_eval_store_expression): Diagnose changing of active
union member for -std=c++17 and earlier.
* g++.dg/cpp1y/constexpr-89336-3.C: New test.
--- gcc/cp/constexpr.c.jj 2019-02-19 10:26:09.567962395 +0100
+++ gcc/cp/constexpr.c 2019-02-19 22:53:45.702983814 +0100
@@ -3890,8 +3890,20 @@ cxx_eval_store_expression (const constex
if (code == UNION_TYPE && CONSTRUCTOR_NELTS (*valp)
&& CONSTRUCTOR_ELT (*valp, 0)->index != index)
- /* Changing active member. */
- vec_safe_truncate (CONSTRUCTOR_ELTS (*valp), 0);
+ {
+ if (cxx_dialect < cxx2a)
+ {
+ if (!ctx->quiet)
+ error_at (cp_expr_loc_or_loc (t, input_location),
+ "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);
+ }
for (idx = 0;
vec_safe_iterate (CONSTRUCTOR_ELTS (*valp), idx, &cep);
--- gcc/testsuite/g++.dg/cpp1y/constexpr-89336-3.C.jj 2019-02-19 14:13:19.990627715 +0100
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-89336-3.C 2019-02-19 22:59:02.294799026 +0100
@@ -0,0 +1,46 @@
+// PR c++/89336
+// { dg-do compile { target c++14 } }
+
+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; // { dg-error "change of the active member of a union from" "" { target c++17_down } }
+ return w.u.a;
+}
+
+static_assert (foo () == 5, ""); // { dg-error "non-constant condition for static assertion" "" { target c++17_down } }
+ // { dg-message "expansion of" "" { target c++17_down } .-1 }
+
+constexpr int
+bar ()
+{
+ union U { int a[5]; long b; };
+ union V { union U u; short v; };
+ V w {};
+ w.v = 5;
+ w.u.a[3] = w.u.a[1] = w.v; // { dg-error "change of the active member of a union from" "" { target c++17_down } }
+ return w.u.a[1] + w.u.a[3];
+}
+
+static_assert (bar () == 10, ""); // { dg-error "non-constant condition for static assertion" "" { target c++17_down } }
+ // { dg-message "expansion of" "" { target c++17_down } .-1 }
+
+struct Z { int x, y; };
+
+constexpr Z
+baz ()
+{
+ union W { Z a; long long w; };
+ W w {};
+ w.a = { 5, 0 };
+ w.a = { (int) (w.w = 17LL + w.a.x), 2 }; // { dg-error "change of the active member of a union from" "" { target c++17_down } }
+ return w.a;
+}
+
+static_assert (baz ().x == 22, ""); // { dg-error "non-constant condition for static assertion" "" { target c++17_down } }
+ // { dg-message "expansion of" "" { target c++17_down } .-1 }
+static_assert (baz ().y == 2, ""); // { dg-error "non-constant condition for static assertion" "" { target c++17_down } }
+ // { dg-message "expansion of" "" { target c++17_down } .-1 }
Jakub
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [C++ PATCH] Fix cxx_eval_store_expression (PR c++/89336)
2019-02-19 22:03 ` Jakub Jelinek
@ 2019-02-20 7:47 ` Jakub Jelinek
2019-02-20 21:12 ` Jason Merrill
1 sibling, 0 replies; 9+ messages in thread
From: Jakub Jelinek @ 2019-02-20 7:47 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches List
On Tue, Feb 19, 2019 at 11:03:05PM +0100, Jakub Jelinek wrote:
> > Before P0137 I believe foo and bar were arguably undefined.
>
> I see, before that it was:
> "an lvalue-to-rvalue conversion (4.1) or modification (5.18, 5.2.6, 5.3.2) that is applied
> to a glvalue that refers to a non-active member of a union or a subobject thereof;"
>
> So, do we want something like this then (or free all vectors immediately
> there and return immediately)?
Bootstrapped/regtested now on x86_64-linux and i686-linux.
> 2019-02-19 Jakub Jelinek <jakub@redhat.com>
>
> PR c++/89336
> * constexpr.c (cxx_eval_store_expression): Diagnose changing of active
> union member for -std=c++17 and earlier.
>
> * g++.dg/cpp1y/constexpr-89336-3.C: New test.
>
> --- gcc/cp/constexpr.c.jj 2019-02-19 10:26:09.567962395 +0100
> +++ gcc/cp/constexpr.c 2019-02-19 22:53:45.702983814 +0100
> @@ -3890,8 +3890,20 @@ cxx_eval_store_expression (const constex
>
> if (code == UNION_TYPE && CONSTRUCTOR_NELTS (*valp)
> && CONSTRUCTOR_ELT (*valp, 0)->index != index)
> - /* Changing active member. */
> - vec_safe_truncate (CONSTRUCTOR_ELTS (*valp), 0);
> + {
> + if (cxx_dialect < cxx2a)
> + {
> + if (!ctx->quiet)
> + error_at (cp_expr_loc_or_loc (t, input_location),
> + "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);
> + }
>
> for (idx = 0;
> vec_safe_iterate (CONSTRUCTOR_ELTS (*valp), idx, &cep);
> --- gcc/testsuite/g++.dg/cpp1y/constexpr-89336-3.C.jj 2019-02-19 14:13:19.990627715 +0100
> +++ gcc/testsuite/g++.dg/cpp1y/constexpr-89336-3.C 2019-02-19 22:59:02.294799026 +0100
> @@ -0,0 +1,46 @@
> +// PR c++/89336
> +// { dg-do compile { target c++14 } }
> +
> +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; // { dg-error "change of the active member of a union from" "" { target c++17_down } }
> + return w.u.a;
> +}
> +
> +static_assert (foo () == 5, ""); // { dg-error "non-constant condition for static assertion" "" { target c++17_down } }
> + // { dg-message "expansion of" "" { target c++17_down } .-1 }
> +
> +constexpr int
> +bar ()
> +{
> + union U { int a[5]; long b; };
> + union V { union U u; short v; };
> + V w {};
> + w.v = 5;
> + w.u.a[3] = w.u.a[1] = w.v; // { dg-error "change of the active member of a union from" "" { target c++17_down } }
> + return w.u.a[1] + w.u.a[3];
> +}
> +
> +static_assert (bar () == 10, ""); // { dg-error "non-constant condition for static assertion" "" { target c++17_down } }
> + // { dg-message "expansion of" "" { target c++17_down } .-1 }
> +
> +struct Z { int x, y; };
> +
> +constexpr Z
> +baz ()
> +{
> + union W { Z a; long long w; };
> + W w {};
> + w.a = { 5, 0 };
> + w.a = { (int) (w.w = 17LL + w.a.x), 2 }; // { dg-error "change of the active member of a union from" "" { target c++17_down } }
> + return w.a;
> +}
> +
> +static_assert (baz ().x == 22, ""); // { dg-error "non-constant condition for static assertion" "" { target c++17_down } }
> + // { dg-message "expansion of" "" { target c++17_down } .-1 }
> +static_assert (baz ().y == 2, ""); // { dg-error "non-constant condition for static assertion" "" { target c++17_down } }
> + // { dg-message "expansion of" "" { target c++17_down } .-1 }
>
Jakub
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [C++ PATCH] Fix cxx_eval_store_expression (PR c++/89336)
2019-02-19 22:03 ` Jakub Jelinek
2019-02-20 7:47 ` Jakub Jelinek
@ 2019-02-20 21:12 ` Jason Merrill
1 sibling, 0 replies; 9+ messages in thread
From: Jason Merrill @ 2019-02-20 21:12 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches List
On 2/19/19 12:03 PM, Jakub Jelinek wrote:
> On Tue, Feb 19, 2019 at 11:28:22AM -1000, Jason Merrill wrote:
>> On Tue, Feb 19, 2019 at 4:00 AM Jakub Jelinek <jakub@redhat.com> wrote:
>>> On Mon, Feb 18, 2019 at 03:01:14PM -1000, Jason Merrill wrote:
>>>> 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.
>>>
>>> There is in [expr.const]/2:
>>>
>>> an lvalue-to-rvalue conversion (7.1) that is applied to a glvalue that refers to a non-active member of a
>>> union or a subobject thereof;
>>>
>>> an assignment expression (8.18) or invocation of an assignment operator (15.8) that would change the
>>> active member of a union;
>>>
>>> in C++17 it seems, so maybe my union testcases are accepts-invalid.
>>> This has been introduced in P0137R1 and removed again in P1330R0. Does that
>>> mean e.g. following is valid in C++14, invalid in C++17 and valid again in
>>> C++20? Or has one of the above papers changed retroactively previous
>>> standards?
>>
>> Before P0137 I believe foo and bar were arguably undefined.
>
> I see, before that it was:
> "an lvalue-to-rvalue conversion (4.1) or modification (5.18, 5.2.6, 5.3.2) that is applied
> to a glvalue that refers to a non-active member of a union or a subobject thereof;"
>
> So, do we want something like this then (or free all vectors immediately
> there and return immediately)?
>
> 2019-02-19 Jakub Jelinek <jakub@redhat.com>
>
> PR c++/89336
> * constexpr.c (cxx_eval_store_expression): Diagnose changing of active
> union member for -std=c++17 and earlier.
OK. I don't think it's important to return immediately.
Jason
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-02-20 21:06 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-13 23:02 [C++ PATCH] Fix cxx_eval_store_expression (PR c++/89336) Jakub Jelinek
2019-02-16 18:51 ` Jason Merrill
2019-02-17 13:34 ` Jakub Jelinek
2019-02-19 1:01 ` Jason Merrill
2019-02-19 14:01 ` Jakub Jelinek
2019-02-19 21:28 ` Jason Merrill
2019-02-19 22:03 ` Jakub Jelinek
2019-02-20 7:47 ` Jakub Jelinek
2019-02-20 21:12 ` Jason Merrill
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).