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. */
