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

[Patrick Palka <ppalka@redhat.com>]

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.

-- >8 --

gcc/cp/ChangeLog:

	* constexpr.cc (reduced_constant_expression_p): Replace use of
	next_initializable_field with custom predicate.  Refactor to
	remove the use of goto.
	* decl.cc (next_initializable_field): Skip over vptr fields.
	Document that this function is to be used only for aggregate
	classes.
---
 gcc/cp/constexpr.cc | 65 ++++++++++++++++++++++++++-------------------
 gcc/cp/decl.cc      | 15 +++++------
 2 files changed, 44 insertions(+), 36 deletions(-)

diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
index 9b1e71857fc..d1cd556591c 100644
--- a/gcc/cp/constexpr.cc
+++ b/gcc/cp/constexpr.cc
@@ -3016,7 +3016,6 @@ reduced_constant_expression_p (tree t)
 
     case CONSTRUCTOR:
       /* And we need to handle PTRMEM_CST wrapped in a CONSTRUCTOR.  */
-      tree field;
       if (CONSTRUCTOR_NO_CLEARING (t))
 	{
 	  if (TREE_CODE (TREE_TYPE (t)) == VECTOR_TYPE)
@@ -3041,42 +3040,54 @@ reduced_constant_expression_p (tree t)
 		}
 	      if (find_array_ctor_elt (t, max) == -1)
 		return false;
-	      goto ok;
 	    }
-	  else if (cxx_dialect >= cxx20
-		   && TREE_CODE (TREE_TYPE (t)) == UNION_TYPE)
+	  else if (TREE_CODE (TREE_TYPE (t)) == UNION_TYPE)
 	    {
-	      if (CONSTRUCTOR_NELTS (t) == 0)
+	      if (CONSTRUCTOR_NELTS (t) != 1)
 		/* An initialized union has a constructor element.  */
 		return false;
-	      /* And it only initializes one member.  */
-	      field = NULL_TREE;
+	      if (!reduced_constant_expression_p (CONSTRUCTOR_ELT (t, 0)->value))
+		return false;
 	    }
 	  else
-	    field = next_initializable_field (TYPE_FIELDS (TREE_TYPE (t)));
+	    {
+	      /* Similar to the predicate used in next_initializable_field,
+		 except that we additionally skip over empty types (for which
+		 we don't require an initializer), and we always consider base
+		 class fields (not just in C++17 mode) and vptr fields.  */
+	      auto ignorable_field_p = [] (tree field) {
+		if (!field)
+		  return false;
+		return (TREE_CODE (field) != FIELD_DECL
+			|| DECL_UNNAMED_BIT_FIELD (field)
+			|| (DECL_ARTIFICIAL (field)
+			    && !DECL_FIELD_IS_BASE (field)
+			    && !DECL_VIRTUAL_P (field))
+			|| is_really_empty_class (TREE_TYPE (field),
+						  /*ignore_vptr*/false));
+	      };
+	      tree field = TYPE_FIELDS (TREE_TYPE (t));
+	      for (auto &e: CONSTRUCTOR_ELTS (t))
+		{
+		  if (!reduced_constant_expression_p (e.value))
+		    return false;
+		  while (e.index != field && ignorable_field_p (field))
+		    field = DECL_CHAIN (field);
+		  if (e.index == field)
+		    field = DECL_CHAIN (field);
+		  else
+		    return false;
+		}
+	      while (ignorable_field_p (field))
+		field = DECL_CHAIN (field);
+	      if (field)
+		return false;
+	    }
 	}
       else
-	field = NULL_TREE;
-      for (auto &e: CONSTRUCTOR_ELTS (t))
-	{
-	  /* If VAL is null, we're in the middle of initializing this
-	     element.  */
+	for (auto &e: CONSTRUCTOR_ELTS (t))
 	  if (!reduced_constant_expression_p (e.value))
 	    return false;
-	  /* Empty class field may or may not have an initializer.  */
-	  for (; field && e.index != field;
-	       field = next_initializable_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));
-	}
-      /* There could be a non-empty field at the end.  */
-      for (; field; field = next_initializable_field (DECL_CHAIN (field)))
-	if (!is_really_empty_class (TREE_TYPE (field), /*ignore_vptr*/false))
-	  return false;
-ok:
       if (CONSTRUCTOR_NO_CLEARING (t))
 	/* All the fields are initialized.  */
 	CONSTRUCTOR_NO_CLEARING (t) = false;
diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index c9110db796a..c564e5e596d 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -6382,10 +6382,10 @@ struct reshape_iter
 
 static tree reshape_init_r (tree, reshape_iter *, tree, tsubst_flags_t);
 
-/* 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 can be
-   initialized.  If there are no more such fields, the return value
-   will be NULL.  */
+/* FIELD is an element of TYPE_FIELDS of an aggregate class, or NULL.  In the
+   former case, the value returned is the next FIELD_DECL (possibly FIELD
+   itself) that can be initialized.  If there are no more such fields, the
+   return value will be NULL.  */
 
 tree
 next_initializable_field (tree field)
@@ -6394,11 +6394,8 @@ next_initializable_field (tree field)
 	 && (TREE_CODE (field) != FIELD_DECL
 	     || DECL_UNNAMED_BIT_FIELD (field)
 	     || (DECL_ARTIFICIAL (field)
-		 /* In C++17, don't skip base class fields.  */
-		 && !(cxx_dialect >= cxx17 && DECL_FIELD_IS_BASE (field))
-		 /* Don't skip vptr fields.  We might see them when we're
-		    called from reduced_constant_expression_p.  */
-		 && !DECL_VIRTUAL_P (field))))
+		 /* In C++17, aggregates can have base classes.  */
+		 && !(cxx_dialect >= cxx17 && DECL_FIELD_IS_BASE (field)))))
     field = DECL_CHAIN (field);
 
   return field;
-- 
2.36.0.63.gf5aaf72f1b