diff mbox series

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

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

Commit Message

Marek Polacek March 12, 2024, 3:56 p.m. UTC 
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?

dg.exp passed, full regtest running.

-- >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): Handle initializing an
	array from a brace-enclosed-initializer.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp1y/nsdmi-aggr20.C: New test.
	* g++.dg/cpp1y/nsdmi-aggr21.C: New test.
---
 gcc/cp/typeck2.cc                         | 18 +++++--
 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr20.C | 17 +++++++
 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr21.C | 59 +++++++++++++++++++++++
 3 files changed, 90 insertions(+), 4 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: ef79c64cb5762c86ee04ddfcedb7fe31eaa3bac8

Comments

Jason Merrill March 12, 2024, 10:26 p.m. UTC | #1 
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?

Jason
diff mbox series

Patch

diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc
index 31198b2f9f5..21d4f42ae20 100644
--- a/gcc/cp/typeck2.cc
+++ b/gcc/cp/typeck2.cc
@@ -1406,6 +1406,7 @@  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
 
@@ -1414,24 +1415,33 @@  digest_init_flags (tree type, tree init, int flags, tsubst_flags_t complain)
 static bool
 potential_prvalue_result_of (tree subob, tree full_expr)
 {
+#define RECUR(t) potential_prvalue_result_of (subob, t)
   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
+	   && can_convert (strip_array_types (TREE_TYPE (full_expr)),
+			   TREE_TYPE (subob), tf_none))
+      return true;
+
   return false;
+#undef RECUR
 }
 
 /* Callback to replace PLACEHOLDER_EXPRs in a TARGET_EXPR (which isn't used
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;