On 3/20/20 9:49 AM, Patrick Palka wrote:
On Thu, 19 Mar 2020, Jason Merrill wrote:
On 3/19/20 2:06 PM, Patrick Palka via Gcc-patches wrote:
On Thu, 19 Mar 2020, Marek Polacek wrote:
On Thu, Mar 19, 2020 at 01:06:35PM -0400, Patrick Palka via Gcc-patches
wrote:
On Thu, 19 Mar 2020, Patrick Palka wrote:
This patch adds a check to detect changing the active union member
during
initialization of the union. It uses the CONSTRUCTOR_NO_CLEARING flag
as a
proxy for whether the non-empty CONSTRUCTOR of UNION_TYPE we're
assigning to in
cxx_eval_store_expression is in the process of being initialized,
which seems to
work well.
If we can't rely on CONSTRUCTOR_NO_CLEARING to be set iff a CONSTRUCTOR
is in the process of being initialized, then here's an alternative patch
for consideration, that detects this UB in an indirect way and after the
fact.
Yeah, I'm not sure if that would work well, especially in C++20 where we
sometimes don't clear it:
/* The result of a constexpr function must be completely initialized.
However, in C++20, a constexpr constructor doesn't necessarily have
to initialize all the fields, so we don't clear
CONSTRUCTOR_NO_CLEARING
in order to detect reading an unitialized object in constexpr
instead
of value-initializing it. (reduced_constant_expression_p is
expected to
take care of clearing the flag.) */
if (TREE_CODE (result) == CONSTRUCTOR
&& (cxx_dialect < cxx2a
|| !DECL_CONSTRUCTOR_P (fun)))
clear_no_implicit_zero (result);
and rely on reduced_constant_expression_p to clear it.
I see, thanks. Here's a reproducer for the issue you pointed out, which
is a valid testcase but gets rejected with the proposed patch:
union U
{
int x;
char y;
};
constexpr bool
baz ()
{
U u;
u.x = 3;
u.y = 7;
return (u.y == 7);
}
static_assert (baz ());
CONSTRUCTOR_NO_CLEARING is set for 'u' and is not cleared after its
constructor returns, and so the check yields a false positive for the
assignment to u.y. That's unfortunate...
We should probably clear the flag when we assign to u.x because once we give a
value to one union member, the union has a value.
That works well. Further testing revealed a couple of more issues
caused by the new hunk
@@ -3751,6 +3751,11 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx, tree
t,
/* If we built a new CONSTRUCTOR, attach it now so that other
initializers can refer to it. */
CONSTRUCTOR_APPEND_ELT (*p, index, new_ctx.ctor);
+ else if (TREE_CODE (type) == UNION_TYPE)
+ /* If we're constructing a union, set the active union member now so
+ that we can later detect if the initializer attempts to activate
+ another member. */
+ CONSTRUCTOR_APPEND_ELT (*p, index, NULL_TREE);
tree elt = cxx_eval_constant_expression (&new_ctx, value,
lval,
non_constant_p, overflow_p);
where other routines aren't prepared to handle a NULL constructor union
element. They are cxx_eval_component_reference (fixed by emitting a
different error message given a NULL constructor elt) and
cxx_eval_bare_aggregate itself (fixed by not re-reducing a CONSTRUCTOR
returned by lookup_placeholder).
As a drive-by this patch adds a check in reduced_constant_expression_p
to correctly return false for an empty CONSTRUCTOR of UNION_TYPE. This
allows us to correctly diagnose the uninitialized use in
constexpr-union4.C where we weren't before.
-- >8 --
gcc/cp/ChangeLog:
PR c++/94066
* constexpr.c (reduced_constant_expression_p) [CONSTRUCTOR]: Properly
handle unions without an initializer.
(cxx_eval_component_reference): Emit a different diagnostic when the
constructor element corresponding to a union member is NULL.
(cxx_eval_bare_aggregate): When constructing a union, always set the
active union member before evaluating the initializer. Relax assertion
that verifies the index of the constructor element we're initializing
hasn't been changed.
(cxx_eval_store_expression): Diagnose changing the active union member
while the union is in the process of being initialized. After setting
an active union member, clear CONSTRUCTOR_NO_CLEARING on the underlying
CONSTRUCTOR.
(cxx_eval_constant_expression) [PLACEHOLDER_EXPR]: Don't re-reduce a
CONSTRUCTOR returned by lookup_placeholder.
gcc/testsuite/ChangeLog:
PR c++/94066
* g++.dg/cpp1y/constexpr-union2.C: New test.
* g++.dg/cpp1y/constexpr-union3.C: New test.
* g++.dg/cpp1y/constexpr-union4.C: New test.
* g++.dg/cpp1y/constexpr-union5.C: New test.
* 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/cpp2a/constexpr-union1.C: New test.
---
gcc/cp/constexpr.c | 69 ++++++++++++++++---
gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C | 9 +++
gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C | 9 +++
gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C | 9 +++
gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C | 15 ++++
gcc/testsuite/g++.dg/cpp1y/pr94066-2.C | 19 +++++
gcc/testsuite/g++.dg/cpp1y/pr94066-3.C | 16 +++++
gcc/testsuite/g++.dg/cpp1y/pr94066.C | 18 +++++
gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C | 18 +++++
9 files changed, 173 insertions(+), 9 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C
create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C
create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C
create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C
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.C
create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 192face9a3a..6284ab25b7d 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -2591,10 +2591,17 @@ reduced_constant_expression_p (tree t)
return false;
else if (cxx_dialect >= cxx2a
/* An ARRAY_TYPE doesn't have any TYPE_FIELDS. */
- && (TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE
- /* A union only initializes one member. */
- || TREE_CODE (TREE_TYPE (t)) == UNION_TYPE))
+ && TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE)
field = NULL_TREE;
+ else if (cxx_dialect >= cxx2a
+ && TREE_CODE (TREE_TYPE (t)) == UNION_TYPE)
+ {
+ if (CONSTRUCTOR_NELTS (t) == 0)
+ /* An initialized union has a constructor element. */
+ return false;
+ /* And it only initializes one member. */
+ field = NULL_TREE;
+ }
else
field = next_initializable_field (TYPE_FIELDS (TREE_TYPE (t)));
}
@@ -3446,8 +3453,14 @@ cxx_eval_component_reference (const constexpr_ctx *ctx,
tree t,
{
/* DR 1188 says we don't have to deal with this. */
if (!ctx->quiet)
- error ("accessing %qD member instead of initialized %qD member in "
- "constant expression", part, CONSTRUCTOR_ELT (whole, 0)->index);
+ {
+ constructor_elt *cep = CONSTRUCTOR_ELT (whole, 0);
+ if (cep->value == NULL_TREE)
+ error ("accessing uninitialized member %qD", part);
+ else
+ error ("accessing %qD member instead of initialized %qD member in "
+ "constant expression", part, cep->index);
+ }
*non_constant_p = true;
return t;
}
@@ -3751,6 +3764,11 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx, tree
t,
/* If we built a new CONSTRUCTOR, attach it now so that other
initializers can refer to it. */
CONSTRUCTOR_APPEND_ELT (*p, index, new_ctx.ctor);
+ else if (TREE_CODE (type) == UNION_TYPE)
+ /* If we're constructing a union, set the active union member now so
+ that we can later detect if the initializer attempts to activate
+ another member. */
+ CONSTRUCTOR_APPEND_ELT (*p, index, NULL_TREE);
tree elt = cxx_eval_constant_expression (&new_ctx, value,
lval,
non_constant_p, overflow_p);
@@ -3784,7 +3802,13 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx, tree
t,
}
else
{
- if (new_ctx.ctor != ctx->ctor)
+ if (TREE_CODE (type) == UNION_TYPE
+ && (*p)->last().index != index)
+ /* The initializer may have erroneously changed the active union
+ member that we're initializing. */
+ gcc_assert (*non_constant_p);
+ else if (new_ctx.ctor != ctx->ctor
+ || TREE_CODE (type) == UNION_TYPE)
{
/* We appended this element above; update the value. */
gcc_assert ((*p)->last().index == index);
@@ -4567,6 +4591,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree
t,
bool no_zero_init = true;
releasing_vec ctors;
+ bool changed_active_union_member_p = false;
while (!refs->is_empty ())
{
if (*valp == NULL_TREE)
@@ -4647,6 +4672,19 @@ cxx_eval_store_expression (const constexpr_ctx *ctx,
tree t,
index);
*non_constant_p = true;
}
+ else if (TREE_CODE (t) == MODIFY_EXPR
+ && CONSTRUCTOR_NO_CLEARING (*valp))
+ {
+ /* Diagnose changing the active union member while the union
+ is in the process of being initialized. */
Hmm, could we also detect this situation by noticing that the current
active constructor element has NULL_TREE value?
+ if (!ctx->quiet)
+ error_at (cp_expr_loc_or_input_loc (t),
+ "change of the active member of a union "
+ "from %qD to %qD during initialization",
+ CONSTRUCTOR_ELT (*valp, 0)->index,
+ index);
+ *non_constant_p = true;
+ }
/* Changing active member. */
vec_safe_truncate (CONSTRUCTOR_ELTS (*valp), 0);
no_zero_init = true;
@@ -4675,6 +4713,10 @@ cxx_eval_store_expression (const constexpr_ctx *ctx,
tree t,
vec_safe_insert (CONSTRUCTOR_ELTS (*valp), idx, ce);
cep = CONSTRUCTOR_ELT (*valp, idx);
+
+ if (code == UNION_TYPE)
+ /* Record that we've changed an active union member. */
+ changed_active_union_member_p = true;
}
found:;
}
@@ -4805,13 +4847,17 @@ cxx_eval_store_expression (const constexpr_ctx *ctx,
tree t,
unsigned i;
bool c = TREE_CONSTANT (init);
bool s = TREE_SIDE_EFFECTS (init);
- if (!c || s)
+ if (!c || s || changed_active_union_member_p)
FOR_EACH_VEC_ELT (*ctors, i, elt)
{
if (!c)
TREE_CONSTANT (elt) = false;
if (s)
TREE_SIDE_EFFECTS (elt) = true;
+ /* Clear CONSTRUCTOR_NO_CLEARING since we've activated a member of
+ this union. */
+ if (TREE_CODE (TREE_TYPE (elt)) == UNION_TYPE)
+ CONSTRUCTOR_NO_CLEARING (elt) = false;
}
if (*non_constant_p)
@@ -6133,8 +6179,13 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx,
tree t,
case PLACEHOLDER_EXPR:
/* Use of the value or address of the current object. */
if (tree ctor = lookup_placeholder (ctx, lval, TREE_TYPE (t)))
- return cxx_eval_constant_expression (ctx, ctor, lval,
- non_constant_p, overflow_p);
+ {
+ if (TREE_CODE (ctor) == CONSTRUCTOR)
+ return ctor;
+ else
+ return cxx_eval_constant_expression (ctx, ctor, lval,
+ non_constant_p, overflow_p);
+ }
/* A placeholder without a referent. We can get here when
checking whether NSDMIs are noexcept, or in massage_init_elt;
just say it's non-constant for now. */
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C
b/gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C
new file mode 100644
index 00000000000..7a6a818742b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union2.C
@@ -0,0 +1,9 @@
+// { dg-do compile { target c++14 } }
+
+union U
+{
+ char *x = &y;
+ char y;
+};
+
+constexpr U u = {};
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C
b/gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C
new file mode 100644
index 00000000000..5cf62e46cb5
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union3.C
@@ -0,0 +1,9 @@
+// { dg-do compile { target c++14 } }
+
+union U
+{
+ int x = (x = x + 1);
+ char y;
+};
+
+constexpr U u = {}; // { dg-error "accessing uninitialized member" }
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C
b/gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C
new file mode 100644
index 00000000000..3e44a1378f3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union4.C
@@ -0,0 +1,9 @@
+// { dg-do compile { target c++14 } }
+
+union U
+{
+ int x = y;
+ char y;
+};
+
+constexpr U u = {}; // { dg-error "accessing uninitialized member" }
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C
b/gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C
new file mode 100644
index 00000000000..55fe9fa2f0b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union5.C
@@ -0,0 +1,15 @@
+// { 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->a++;
+ return {42};
+}
+
+extern constexpr U u = {}; // { dg-error "accessing uninitialized member" }
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..1c00b650961
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-2.C
@@ -0,0 +1,19 @@
+// 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'" }
+ return {42};
+}
+
+extern constexpr U u = {};
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..175018acf86
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr94066-3.C
@@ -0,0 +1,16 @@
+// 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; // { dg-error "'U::a' to 'U::y'" }
+ return {42};
+}
+
+extern constexpr U u = {};
diff --git a/gcc/testsuite/g++.dg/cpp1y/pr94066.C
b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
new file mode 100644
index 00000000000..6725c8c737f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr94066.C
@@ -0,0 +1,18 @@
+// 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'" }
+ return {42};
+}
+
+extern constexpr U u = {};
diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C
b/gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C
new file mode 100644
index 00000000000..c38167ad798
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-union1.C
@@ -0,0 +1,18 @@
+// { dg-do compile { target c++2a } }
+
+union U
+{
+ int x;
+ char y;
+};
+
+constexpr bool
+baz ()
+{
+ U u;
+ u.x = 3;
+ u.y = 7;
+ return (u.y == 7);
+}
+
+static_assert (baz ());
