Re: [PATCH] c++: constexpr init of union sub-aggr w/ base [PR105491]

[Patrick Palka <ppalka@redhat.com>]

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 <jason@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?

-- >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_;
-- 
2.36.1.203.g1bcf4f6271