On 5/31/22 12:41, Patrick Palka wrote:
On Wed, 18 May 2022, Jason Merrill wrote:
On 5/17/22 12:34, Patrick Palka wrote:
On Sat, May 7, 2022 at 5:18 PM Jason Merrill <ja...@redhat.com> wrote:
On 5/6/22 16:46, Patrick Palka wrote:
On Fri, 6 May 2022, Jason Merrill wrote:
On 5/6/22 16:10, Patrick Palka wrote:
On Fri, 6 May 2022, Patrick Palka wrote:
On Fri, 6 May 2022, Jason Merrill wrote:
On 5/6/22 14:00, Patrick Palka wrote:
On Fri, 6 May 2022, Patrick Palka wrote:
On Fri, 6 May 2022, Jason Merrill wrote:
On 5/6/22 11:22, Patrick Palka wrote:
Here ever since r10-7313-gb599bf9d6d1e18,
reduced_constant_expression_p
in C++11/14 is rejecting the marked sub-aggregate
initializer
(of type
S)
W w = {.D.2445={.s={.D.2387={.m=0}, .b=0}}}
^
ultimately because said initializer has
CONSTRUCTOR_NO_CLEARING
set,
and
so the function proceeds to verify that all fields of S
are
initialized.
And before C++17 we don't expect to see base class
fields (since
next_initializable_field skips over the), so the base
class
initializer
causes r_c_e_p to return false.
That seems like the primary bug. I guess r_c_e_p
shouldn't be
using
next_initializable_field. Really that function should
only be
used for
aggregates.
I see, I'll try replacing it in r_c_e_p. Would that be in
addition
to
or instead of the clear_no_implicit_zero approach?
I'm testing the following, which uses a custom predicate
instead of
next_initializable_field in r_c_e_p.
Let's make it a public predicate, not internal to r_c_e_p.
Maybe it
could be
next_subobject_field, and the current next_initializable_field
change to
next_aggregate_field?
Will do.
Looks like the inner initializer {.D.2387={.m=0}, .b=0} is
formed
during
the subobject constructor call:
V::V (&((struct S *) this)->D.2120);
after the evaluation of which, 'result' in
cxx_eval_call_expression is
NULL
(presumably because it's a CALL_EXPR, not AGGR_INIT_EXPR?):
/* This can be null for a subobject constructor call, in
which case what we care about is the initialization
side-effects rather than the value. We could get at
the
value by evaluating *this, but we don't bother;
there's
no need to put such a call in the hash table. */
result = lval ? ctx->object : ctx->ctor;
so we end up not calling clear_no_implicit_zero for the inner
initializer
directly. We only call clear_no_implicit_zero after
evaluating the
AGGR_INIT_EXPR for outermost initializer (of type W).
Maybe for constructors we could call it on ctx->ctor instead of
result,
or
call r_c_e_p in C++20+?
But both ctx->ctor and ->object are NULL during a subobject
constructor
call (since we apparently clear these fields when entering a
STATEMENT_LIST):
So I tried instead obtaining the constructor by evaluating new_obj
via
--- a/gcc/cp/constexpr.cc
+++ b/gcc/cp/constexpr.cc
@@ -2993,6 +2988,9 @@ cxx_eval_call_expression (const
constexpr_ctx *ctx,
tree t,
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 (new_obj && DECL_CONSTRUCTOR_P (fun))
+ result = cxx_eval_constant_expression (ctx, new_obj,
/*lval=*/false,
+ non_constant_p,
overflow_p);
if (TREE_CODE (result) == CONSTRUCTOR
&& (cxx_dialect < cxx20
|| !DECL_CONSTRUCTOR_P (fun)))
but that seems to break e.g. g++.dg/cpp2a/constexpr-init12.C
because
after the subobject constructor call
S::S (&((struct W *) this)->s, NON_LVALUE_EXPR <8>);
the constructor for the subobject a.s in new_obj is still
completely
missing (I suppose because S::S doesn't initialize any of its
members)
so trying to obtain it causes us to complain too soon from
cxx_eval_component_reference:
constexpr-init12.C:16:24: in ‘constexpr’ expansion of ‘W(42)’
constexpr-init12.C:10:22: in ‘constexpr’ expansion of
‘((W*)this)->W::s.S::S(8)’
constexpr-init12.C:16:24: error: accessing uninitialized member
‘W::s’
16 | constexpr auto a = W(42); // { dg-error "not a constant
expression" }
| ^
It does seem dubious that we would clear the flag on an outer
ctor when
it's
still set on an inner ctor, should probably add an assert
somewhere.
Makes sense, not sure where the best place would be..
On second thought, if I'm understanding your suggestion correctly, I
don't think we can generally enforce such a property for
CONSTRUCTOR_NO_CLEARING, given how cxx_eval_store_expression uses it
for
unions:
union U {
struct { int x, y; } a;
} u;
u.a.x = 0;
Here after evaluating the assignment, the outer ctor for the union
will
have CONSTRUCTOR_NO_CLEARING cleared to indicate we finished
activating
the union member, but the inner ctor is certainly not fully
initialized
so it'll have CONSTRUCTOR_NO_CLEARING set still.
Why clear the flag on the union before the inner ctor is fully
initialized, if
the intent is to prevent changing the active member during
initialization?
In the loop over ctors in cxx_eval_store_expression, I'd think if we
encounter
a CONSTRUCTOR_NO_CLEARING ctor along the way, we shouldn't clear the
flag on
an outer ctor.
Wouldn't that prevent us from later activating a different member, which
is
allowed in C++20? It would cause us to reject e.g.
constexpr auto f() {
union U {
struct { int x, y; } a;
int b;
} u;
u.a.x = 0;
u.b = 0;
return u;
}
static_assert(f().b == 0);
even in C++20 with
<source>:7:7: error: change of the active member of a union from
‘f()::U::a’ to ‘f()::U::b’ during initialization
7 | u.b = 0;
| ~~~~^~~
because when evaluating the second assignment we'd see that the
CONSTRUCTOR_NO_CLEARING flag is still set on the union while it already
has an activated member.
Ah, makes sense.
On a related note, here's the patch that mechanically renames
next_initializable_field to next_aggregate_field and makes it skip over
vptr
fields, and defines next_subobject_field alongside it for use by
r_c_e_p.
Bootstrap and regtesting in progress.
OK if successful.
Thanks, committed as r13-211-g0c7bce0ac184c0. Would this patch be
suitable for backporting to 12? It seems safe enough. Alternatively
I guess we could backport the original workaround that tweaks
clear_no_implicit_zero.
Maybe for 12 just add next_subobject_field without touching any other users of
next_initializable_field?
Ah makes sense, like so?
OK.
-- >8 --
Subject: [PATCH] c++: constexpr init of union sub-aggr w/ base [PR105491]
...
NB: This minimal backport of r13-211-g0c7bce0ac184c0 for 12.2 just adds
next_subobject_field and makes reduced_constant_expression_p use it.
PR c++/105491
gcc/cp/ChangeLog:
* constexpr.cc (reduced_constant_expression_p): Use
next_subobject_field instead.
* cp-tree.h (next_subobject_field): Declare.
* decl.cc (next_subobject_field): Define.
gcc/testsuite/ChangeLog:
* g++.dg/cpp0x/constexpr-union7.C: New test.
* g++.dg/cpp0x/constexpr-union7a.C: New test.
* g++.dg/cpp2a/constinit17.C: New test.
(cherry picked from commit 0c7bce0ac184c057bacad9c8e615ce82923835fd)
---
gcc/cp/constexpr.cc | 8 +++----
gcc/cp/cp-tree.h | 1 +
gcc/cp/decl.cc | 19 +++++++++++++++
gcc/testsuite/g++.dg/cpp0x/constexpr-union7.C | 17 +++++++++++++
.../g++.dg/cpp0x/constexpr-union7a.C | 15 ++++++++++++
gcc/testsuite/g++.dg/cpp2a/constinit17.C | 24 +++++++++++++++++++
6 files changed, 80 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-union7.C
create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-union7a.C
create mode 100644 gcc/testsuite/g++.dg/cpp2a/constinit17.C
diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
index 47d5113ace2..1e3342adbb9 100644
--- a/gcc/cp/constexpr.cc
+++ b/gcc/cp/constexpr.cc
@@ -3053,7 +3053,7 @@ reduced_constant_expression_p (tree t)
field = NULL_TREE;
}
else
- field = next_initializable_field (TYPE_FIELDS (TREE_TYPE (t)));
+ field = next_subobject_field (TYPE_FIELDS (TREE_TYPE (t)));
}
else
field = NULL_TREE;
@@ -3065,15 +3065,15 @@ reduced_constant_expression_p (tree t)
return false;
/* Empty class field may or may not have an initializer. */
for (; field && e.index != field;
- field = next_initializable_field (DECL_CHAIN (field)))
+ field = next_subobject_field (DECL_CHAIN (field)))
if (!is_really_empty_class (TREE_TYPE (field),
/*ignore_vptr*/false))
return false;
if (field)
- field = next_initializable_field (DECL_CHAIN (field));
+ field = next_subobject_field (DECL_CHAIN (field));
}
/* There could be a non-empty field at the end. */
- for (; field; field = next_initializable_field (DECL_CHAIN (field)))
+ for (; field; field = next_subobject_field (DECL_CHAIN (field)))
if (!is_really_empty_class (TREE_TYPE (field), /*ignore_vptr*/false))
return false;
ok:
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index e9a3d09ac4c..63437415296 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -6884,6 +6884,7 @@ extern void initialize_artificial_var (tree,
vec<constructor_elt, va_gc> *);
extern tree check_var_type (tree, tree, location_t);
extern tree reshape_init (tree, tree, tsubst_flags_t);
extern tree next_initializable_field (tree);
+extern tree next_subobject_field (tree);
extern tree first_field (const_tree);
extern tree fndecl_declared_return_type (tree);
extern bool undeduced_auto_decl (tree);
diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index 2852093d624..af5eca71ba1 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -6409,6 +6409,25 @@ next_initializable_field (tree field)
return field;
}
+/* FIELD is an element of TYPE_FIELDS or NULL. In the former case, the value
+ returned is the next FIELD_DECL (possibly FIELD itself) that corresponds
+ to a subobject. If there are no more such fields, the return value will be
+ NULL. */
+
+tree
+next_subobject_field (tree field)
+{
+ while (field
+ && (TREE_CODE (field) != FIELD_DECL
+ || DECL_UNNAMED_BIT_FIELD (field)
+ || (DECL_ARTIFICIAL (field)
+ && !DECL_FIELD_IS_BASE (field)
+ && !DECL_VIRTUAL_P (field))))
+ field = DECL_CHAIN (field);
+
+ return field;
+}
+
/* Return true for [dcl.init.list] direct-list-initialization from
single element of enumeration with a fixed underlying type. */
diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-union7.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-union7.C
new file mode 100644
index 00000000000..b3147d9db50
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-union7.C
@@ -0,0 +1,17 @@
+// PR c++/105491
+// { dg-do compile { target c++11 } }
+
+struct V {
+ int m = 0;
+};
+struct S : V {
+ constexpr S(int) : b() { }
+ bool b;
+};
+struct W {
+ constexpr W() : s(0) { }
+ union {
+ S s;
+ };
+};
+constexpr W w;
diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-union7a.C
b/gcc/testsuite/g++.dg/cpp0x/constexpr-union7a.C
new file mode 100644
index 00000000000..b676e7d1748
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-union7a.C
@@ -0,0 +1,15 @@
+// PR c++/105491
+// { dg-do compile { target c++11 } }
+
+struct V {
+ int m = 0;
+};
+struct S : V {
+ constexpr S(int) : b() { }
+ bool b;
+};
+union W {
+ constexpr W() : s(0) { }
+ S s;
+};
+constexpr W w;
diff --git a/gcc/testsuite/g++.dg/cpp2a/constinit17.C
b/gcc/testsuite/g++.dg/cpp2a/constinit17.C
new file mode 100644
index 00000000000..6431654ac85
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/constinit17.C
@@ -0,0 +1,24 @@
+// PR c++/105491
+// { dg-do compile { target c++11 } }
+
+class Message {
+ virtual int GetMetadata();
+};
+class ProtobufCFileOptions : Message {
+public:
+ constexpr ProtobufCFileOptions(int);
+ bool no_generate_;
+ bool const_strings_;
+ bool use_oneof_field_name_;
+ bool gen_pack_helpers_;
+ bool gen_init_helpers_;
+};
+constexpr ProtobufCFileOptions::ProtobufCFileOptions(int)
+ : no_generate_(), const_strings_(), use_oneof_field_name_(),
+ gen_pack_helpers_(), gen_init_helpers_() {}
+struct ProtobufCFileOptionsDefaultTypeInternal {
+ constexpr ProtobufCFileOptionsDefaultTypeInternal() : _instance({}) {}
+ union {
+ ProtobufCFileOptions _instance;
+ };
+} __constinit _ProtobufCFileOptions_default_instance_;
