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.
commit b7cf563e1715909221f783747bb5e194dd38d803
Author: Jason Merrill <ja...@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)
