diff mbox series

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

Message ID ZfNro5HtgOJTEE4R@redhat.com
State New
Headers show
Series [v3] c++: ICE with temporary of class type in array DMI [PR109966] | expand

Commit Message

Marek Polacek March 14, 2024, 9:26 p.m. UTC 
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


base-commit: 6dbf0d252f69ab2924256e6778ba7dc55d5b6915

Comments

Marek Polacek March 19, 2024, 7:47 p.m. UTC | #1 
On Thu, Mar 14, 2024 at 05:26:59PM -0400, Marek Polacek wrote:
> @@ -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)

I just realized this could also check !TARGET_EXPR_ELIDING_P; there's no point
to adding an eliding TARGET_EXPR into the pset.

> +	  pset->add (arg);
> +      }

Marek
Jason Merrill April 12, 2024, 8:15 p.m. UTC | #2 
On 3/14/24 17:26, Marek Polacek wrote:
> 
> 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.

I tried the same thing to see what you mean, and that doesn't look like 
a regression to me, just a different (and more accurate) diagnostic.

But you're right that this patch is safer, and the other approach can 
wait for stage 1.  Will you queue that up?  In the mean time, this patch 
is OK.

> I just realized this could also check !TARGET_EXPR_ELIDING_P; there's no point
> to adding an eliding TARGET_EXPR into the pset.

...with this change.

Jason
Marek Polacek April 12, 2024, 9:41 p.m. UTC | #3 
On Fri, Apr 12, 2024 at 04:15:45PM -0400, Jason Merrill wrote:
> On 3/14/24 17:26, Marek Polacek wrote:
> > 
> > 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.
> 
> I tried the same thing to see what you mean, and that doesn't look like a
> regression to me, just a different (and more accurate) diagnostic.
> 
> But you're right that this patch is safer, and the other approach can wait
> for stage 1.  Will you queue that up?  In the mean time, this patch is OK.

Yeah, happy to; I've opened 114707 to remember.
 
> > I just realized this could also check !TARGET_EXPR_ELIDING_P; there's no point
> > to adding an eliding TARGET_EXPR into the pset.
> 
> ...with this change.

Thanks.

Marek
diff mbox series

Patch

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;