[PATCH v3] c++: ICE with temporary of class type in array DMI [PR109966]

[Marek Polacek <polacek@redhat.com>]

On Tue, Mar 12, 2024 at 06:26:14PM -0400, Jason Merrill wrote:
> On 3/12/24 11:56, Marek Polacek wrote:
> > On Tue, Mar 12, 2024 at 09:57:14AM -0400, Jason Merrill wrote:
> > > On 3/11/24 19:27, Marek Polacek wrote:
> > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/13?
> > > > 
> > > > -- >8 --
> > > > This ICE started with the fairly complicated r13-765.  We crash in
> > > > gimplify_var_or_parm_decl because a stray VAR_DECL leaked there.
> > > > The problem is ultimately that potential_prvalue_result_of wasn't
> > > > correctly handling arrays and replace_placeholders_for_class_temp_r
> > > > replaced a PLACEHOLDER_EXPR in a TARGET_EXPR which is used in the
> > > > context of copy elision.  If I have
> > > > 
> > > >     M m[2] = { M{""}, M{""} };
> > > > 
> > > > then we don't invoke the M(const M&) copy-ctor.  I think the fix is
> > > > to detect such a case in potential_prvalue_result_of.
> > > > 
> > > > 	PR c++/109966
> > > > 
> > > > gcc/cp/ChangeLog:
> > > > 
> > > > 	* typeck2.cc (potential_prvalue_result_of): Add walk_subtrees
> > > > 	parameter.  Handle initializing an array from a
> > > > 	brace-enclosed-initializer.
> > > > 	(replace_placeholders_for_class_temp_r): Pass walk_subtrees down to
> > > > 	potential_prvalue_result_of.
> > > > 
> > > > gcc/testsuite/ChangeLog:
> > > > 
> > > > 	* g++.dg/cpp1y/nsdmi-aggr20.C: New test.
> > > > 	* g++.dg/cpp1y/nsdmi-aggr21.C: New test.
> > > > ---
> > > >    gcc/cp/typeck2.cc                         | 27 ++++++++---
> > > >    gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr20.C | 17 +++++++
> > > >    gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr21.C | 59 +++++++++++++++++++++++
> > > >    3 files changed, 96 insertions(+), 7 deletions(-)
> > > >    create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr20.C
> > > >    create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr21.C
> > > > 
> > > > diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc
> > > > index 31198b2f9f5..8b99ce78e9a 100644
> > > > --- a/gcc/cp/typeck2.cc
> > > > +++ b/gcc/cp/typeck2.cc
> > > > @@ -1406,46 +1406,59 @@ digest_init_flags (tree type, tree init, int flags, tsubst_flags_t complain)
> > > >         A a = (A{});	      // initializer
> > > >         A a = (1, A{});	      // initializer
> > > >         A a = true ? A{} : A{};  // initializer
> > > > +     A arr[1] = { A{} };      // initializer
> > > >         auto x = A{}.x;	      // temporary materialization
> > > >         auto x = foo(A{});	      // temporary materialization
> > > >       FULL_EXPR is the whole expression, SUBOB is its TARGET_EXPR subobject.  */
> > > >    static bool
> > > > -potential_prvalue_result_of (tree subob, tree full_expr)
> > > > +potential_prvalue_result_of (tree subob, tree full_expr, int *walk_subtrees)
> > > >    {
> > > > +#define RECUR(t) potential_prvalue_result_of (subob, t, walk_subtrees)
> > > >      if (subob == full_expr)
> > > >        return true;
> > > >      else if (TREE_CODE (full_expr) == TARGET_EXPR)
> > > >        {
> > > >          tree init = TARGET_EXPR_INITIAL (full_expr);
> > > >          if (TREE_CODE (init) == COND_EXPR)
> > > > -	return (potential_prvalue_result_of (subob, TREE_OPERAND (init, 1))
> > > > -		|| potential_prvalue_result_of (subob, TREE_OPERAND (init, 2)));
> > > > +	return (RECUR (TREE_OPERAND (init, 1))
> > > > +		|| RECUR (TREE_OPERAND (init, 2)));
> > > >          else if (TREE_CODE (init) == COMPOUND_EXPR)
> > > > -	return potential_prvalue_result_of (subob, TREE_OPERAND (init, 1));
> > > > +	return RECUR (TREE_OPERAND (init, 1));
> > > >          /* ??? I don't know if this can be hit.  */
> > > >          else if (TREE_CODE (init) == PAREN_EXPR)
> > > >    	{
> > > >    	  gcc_checking_assert (false);
> > > > -	  return potential_prvalue_result_of (subob, TREE_OPERAND (init, 0));
> > > > +	  return RECUR (TREE_OPERAND (init, 0));
> > > >    	}
> > > >        }
> > > > +  /* The array case listed above.  */
> > > > +  else if (TREE_CODE (full_expr) == CONSTRUCTOR
> > > > +	   && TREE_CODE (TREE_TYPE (full_expr)) == ARRAY_TYPE)
> > > > +    for (constructor_elt &e: CONSTRUCTOR_ELTS (full_expr))
> > > > +      if (e.value == subob)
> > > > +	{
> > > > +	  *walk_subtrees = 0;
> > > 
> > > Why clear walk_subtrees?  Won't that mean we fail to replace any
> > > placeholders nested within an array element initializer?
> > 
> > Right.  I couldn't find a testcase where that would cause a problem
> > but I think I just wasn't inventive enough.
> > 
> > Originally, I was checking same_type_ignoring_top_level_qualifiers_p
> > but that's not going to work for code like
> > 
> >    struct N { N(M); };
> >    N arr[2] = { M{""}, M{""} };
> > 
> > or with operator M().  But I suppose I could just use can_convert
> > like below.  What do you think about that?
> 
> Basing this on the type seems unreliable, we're looking for where in the
> expression the TARGET_EXPR occurs, and there could be others of the same
> type elsewhere in the expression.
> 
> Why not loop over CONSTRUCTOR_ELTS like you did above, just without clearing
> walk_subtrees?

With the test with N(M) we are dealing with:

{TARGET_EXPR <D.2892, <<< Unknown tree: aggr_init_expr
  5
  __ct_comp 
  D.2892
  (struct N *) <<< Unknown tree: void_cst >>>
  TARGET_EXPR <D.2864, {.name=TARGET_EXPR <D.2854, <<< Unknown tree: aggr_init_expr
    5
    __ct_comp 
    D.2854
    (struct k *) <<< Unknown tree: void_cst >>>
    (const char *) "" >>>>, .j=NON_LVALUE_EXPR <42>, .i=(&<PLACEHOLDER_EXPR struct M>)->j}> >>>>}

where the TARGET_EXPR with slot=D.2864 is the problematic one.  So 
looping over CONSTRUCTOR_ELTS of the outer CONSTRUCTOR wouldn't find
it.  But you're of course right that just checking the type is fragile.

In the following patch, I'm taking a different tack.  I believe
we ought to use TARGET_EXPR_ELIDING_P.  The gimplify_arg bit I'm
talking about below is this:

      /* Also strip a TARGET_EXPR that would force an extra copy.  */
      if (TREE_CODE (*arg_p) == TARGET_EXPR)
        {
          tree init = TARGET_EXPR_INITIAL (*arg_p);
          if (init
              && !VOID_TYPE_P (TREE_TYPE (init)))
            *arg_p = init;
        }

Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/13?

-- >8 --
This ICE started with the fairly complicated r13-765.  We crash in
gimplify_var_or_parm_decl because a stray VAR_DECL leaked there.
The problem is ultimately that potential_prvalue_result_of wasn't
correctly handling arrays and replace_placeholders_for_class_temp_r
replaced a PLACEHOLDER_EXPR in a TARGET_EXPR which is used in the
context of copy elision.  If I have

  M m[2] = { M{""}, M{""} };

then we don't invoke the M(const M&) copy-ctor.

One part of the fix is to use TARGET_EXPR_ELIDING_P rather than
potential_prvalue_result_of.  That unfortunately doesn't handle the
case like

  struct N { N(M); };
  N arr[2] = { M{""}, M{""} };

because TARGET_EXPRs that initialize a function argument are not
marked TARGET_EXPR_ELIDING_P even though gimplify_arg drops such
TARGET_EXPRs on the floor.  We can use a pset to avoid replacing
placeholders in them.

I made an attempt to use set_target_expr_eliding in
convert_for_arg_passing but that regressed constexpr-diag1.C, and does
not seem like a prudent change in stage 4 anyway.

	PR c++/109966

gcc/cp/ChangeLog:

	* typeck2.cc (potential_prvalue_result_of): Remove.
	(replace_placeholders_for_class_temp_r): Check TARGET_EXPR_ELIDING_P.
	Use a pset.  Don't replace_placeholders in TARGET_EXPRs that initialize
	a function argument.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp1y/nsdmi-aggr20.C: New test.
	* g++.dg/cpp1y/nsdmi-aggr21.C: New test.
---
 gcc/cp/typeck2.cc                         | 55 ++++++---------------
 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr20.C | 17 +++++++
 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr21.C | 59 +++++++++++++++++++++++
 3 files changed, 92 insertions(+), 39 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr20.C
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr21.C

diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc
index 31198b2f9f5..4bf3201eedc 100644
--- a/gcc/cp/typeck2.cc
+++ b/gcc/cp/typeck2.cc
@@ -1399,41 +1399,6 @@ digest_init_flags (tree type, tree init, int flags, tsubst_flags_t complain)
   return digest_init_r (type, init, 0, flags, complain);
 }
 
-/* Return true if SUBOB initializes the same object as FULL_EXPR.
-   For instance:
-
-     A a = A{};		      // initializer
-     A a = (A{});	      // initializer
-     A a = (1, A{});	      // initializer
-     A a = true ? A{} : A{};  // initializer
-     auto x = A{}.x;	      // temporary materialization
-     auto x = foo(A{});	      // temporary materialization
-
-   FULL_EXPR is the whole expression, SUBOB is its TARGET_EXPR subobject.  */
-
-static bool
-potential_prvalue_result_of (tree subob, tree full_expr)
-{
-  if (subob == full_expr)
-    return true;
-  else if (TREE_CODE (full_expr) == TARGET_EXPR)
-    {
-      tree init = TARGET_EXPR_INITIAL (full_expr);
-      if (TREE_CODE (init) == COND_EXPR)
-	return (potential_prvalue_result_of (subob, TREE_OPERAND (init, 1))
-		|| potential_prvalue_result_of (subob, TREE_OPERAND (init, 2)));
-      else if (TREE_CODE (init) == COMPOUND_EXPR)
-	return potential_prvalue_result_of (subob, TREE_OPERAND (init, 1));
-      /* ??? I don't know if this can be hit.  */
-      else if (TREE_CODE (init) == PAREN_EXPR)
-	{
-	  gcc_checking_assert (false);
-	  return potential_prvalue_result_of (subob, TREE_OPERAND (init, 0));
-	}
-    }
-  return false;
-}
-
 /* Callback to replace PLACEHOLDER_EXPRs in a TARGET_EXPR (which isn't used
    in the context of guaranteed copy elision).  */
 
@@ -1441,11 +1406,13 @@ static tree
 replace_placeholders_for_class_temp_r (tree *tp, int *, void *data)
 {
   tree t = *tp;
-  tree full_expr = *static_cast<tree *>(data);
+  auto pset = static_cast<hash_set<tree> *>(data);
 
   /* We're looking for a TARGET_EXPR nested in the whole expression.  */
   if (TREE_CODE (t) == TARGET_EXPR
-      && !potential_prvalue_result_of (t, full_expr))
+      /* That serves as temporary materialization, not an initializer.  */
+      && !TARGET_EXPR_ELIDING_P (t)
+      && !pset->add (t))
     {
       tree init = TARGET_EXPR_INITIAL (t);
       while (TREE_CODE (init) == COMPOUND_EXPR)
@@ -1460,6 +1427,16 @@ replace_placeholders_for_class_temp_r (tree *tp, int *, void *data)
 	  gcc_checking_assert (!find_placeholders (init));
 	}
     }
+  /* TARGET_EXPRs initializing function arguments are not marked as eliding,
+     even though gimplify_arg drops them on the floor.  Don't go replacing
+     placeholders in them.  */
+  else if (TREE_CODE (t) == CALL_EXPR || TREE_CODE (t) == AGGR_INIT_EXPR)
+    for (int i = 0; i < call_expr_nargs (t); ++i)
+      {
+	tree arg = get_nth_callarg (t, i);
+	if (TREE_CODE (arg) == TARGET_EXPR)
+	  pset->add (arg);
+      }
 
   return NULL_TREE;
 }
@@ -1507,8 +1484,8 @@ digest_nsdmi_init (tree decl, tree init, tsubst_flags_t complain)
      temporary materialization does not occur when initializing an object
      from a prvalue of the same type, therefore we must not replace the
      placeholder with a temporary object so that it can be elided.  */
-  cp_walk_tree (&init, replace_placeholders_for_class_temp_r, &init,
-		nullptr);
+  hash_set<tree> pset;
+  cp_walk_tree (&init, replace_placeholders_for_class_temp_r, &pset, nullptr);
 
   return init;
 }
diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr20.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr20.C
new file mode 100644
index 00000000000..4796d861e83
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr20.C
@@ -0,0 +1,17 @@
+// PR c++/109966
+// { dg-do compile { target c++14 } }
+
+#define SA(X) static_assert ((X),#X)
+
+struct A {
+  int a;
+  int b = a;
+};
+
+struct B {
+  int x = 0;
+  int y[1]{A{x}.b};
+};
+
+constexpr B b = { };
+SA(b.y[0] == 0);
diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr21.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr21.C
new file mode 100644
index 00000000000..efec45bc1a8
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr21.C
@@ -0,0 +1,59 @@
+// PR c++/109966
+// { dg-do compile { target c++14 } }
+
+struct k {
+  k(const char *);
+};
+struct M {
+  k name;
+  int j = 42;
+  int i = j;
+};
+
+M m = M{""};
+
+struct S {
+  M arr1[1]{M{""}};
+  M a1[1] = { (M{""}) };
+  M a2[1] = { (true, M{""}) };
+  M a3[1] = { true ? M{""} : M{""} };
+  M arr2[2]{M{""}, M{""}};
+  M arr3[3]{M{""}, M{""}, M{""}};
+
+  M arr1e[1] = {M{""}};
+  M arr2e[2] = {M{""}, M{""}};
+  M arr3e[3] = {M{""}, M{""}, M{""}};
+
+  M arr1l[1] = { m };
+  M arr2l[2] = { m, m };
+  M arr3l[3] = { m, m, m };
+
+  M m1 = M{""};
+  M m2 = m;
+  M m3{M{""}};
+  M m4 = {M{""}};
+} o;
+
+struct N {
+  N(M);
+};
+
+struct Z {
+  N arr1[1]{ M{""} };
+  N arr2[2]{ M{""}, M{""} };
+  N arr1e[1] = { M{""} };
+  N arr2e[2] = { M{""}, M{""} };
+} z;
+
+struct Y {
+  k name;
+  int j = 42;
+  int i = j;
+  operator M();
+};
+
+struct W {
+  M arr1[1]{ Y{""} };
+  M arr2[2]{ Y{""}, Y{""} };
+  M arr3[3]{ Y{""}, Y{""}, Y{""} };
+} w;

base-commit: 6dbf0d252f69ab2924256e6778ba7dc55d5b6915
-- 
2.44.0