diff mbox series

c++: Fix ICE due to folding a call to constructor on cdtor_returns_this arches (aka arm32) [PR113083]

Message ID ZdhaZlZjIuf9TYgW@tucnak
State Unresolved
Headers
Series c++: Fix ICE due to folding a call to constructor on cdtor_returns_this arches (aka arm32) [PR113083] |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Jakub Jelinek Feb. 23, 2024, 8:42 a.m. UTC 
  Hi!

When targetm.cxx.cdtor_returns_this () (aka on arm32 TARGET_AAPCS_BASED)
constructor is supposed to return this pointer, but when we cp_fold such
a call, we don't take that into account and just INIT_EXPR the object,
so we can later ICE during gimplification, because the expression doesn't
have the right type.

Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux and
tested with a cross to armv7-linux-gnueabi on the testcase, but
unfortunately there are no 32-bit arm boxes in cfarm and arm32 is gone from
Fedora for quite some time as well, so I have no easy way to test this.
Christophe, do you think you could test this?  Thanks.

2024-02-23  Jakub Jelinek  <jakub@redhat.com>

	PR c++/113083
	* cp-gimplify.cc (cp_fold): For targetm.cxx.cdtor_returns_this ()
	wrap r into a COMPOUND_EXPR and return folded CALL_EXPR_ARG (x, 0).

	* g++.dg/cpp0x/constexpr-113083.C: New test.


	Jakub

Comments

Christophe Lyon Feb. 23, 2024, 9:13 a.m. UTC | #1 
On Fri, 23 Feb 2024 at 09:42, Jakub Jelinek <jakub@redhat.com> wrote:
>
> Hi!
>
> When targetm.cxx.cdtor_returns_this () (aka on arm32 TARGET_AAPCS_BASED)
> constructor is supposed to return this pointer, but when we cp_fold such
> a call, we don't take that into account and just INIT_EXPR the object,
> so we can later ICE during gimplification, because the expression doesn't
> have the right type.
>
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux and
> tested with a cross to armv7-linux-gnueabi on the testcase, but
> unfortunately there are no 32-bit arm boxes in cfarm and arm32 is gone from
> Fedora for quite some time as well, so I have no easy way to test this.
> Christophe, do you think you could test this?  Thanks.

Hi Jakub,

Sadly our precommit CI could not apply your patch automatically (as
you can see in patchwork).

I'll test your patch manually.

Thanks,

Christophe

>
> 2024-02-23  Jakub Jelinek  <jakub@redhat.com>
>
>         PR c++/113083
>         * cp-gimplify.cc (cp_fold): For targetm.cxx.cdtor_returns_this ()
>         wrap r into a COMPOUND_EXPR and return folded CALL_EXPR_ARG (x, 0).
>
>         * g++.dg/cpp0x/constexpr-113083.C: New test.
>
> --- gcc/cp/cp-gimplify.cc.jj    2024-02-22 21:45:09.663430066 +0100
> +++ gcc/cp/cp-gimplify.cc       2024-02-22 22:30:23.481428242 +0100
> @@ -3412,9 +3412,15 @@ cp_fold (tree x, fold_flags_t flags)
>             if (DECL_CONSTRUCTOR_P (callee))
>               {
>                 loc = EXPR_LOCATION (x);
> -               tree s = build_fold_indirect_ref_loc (loc,
> -                                                     CALL_EXPR_ARG (x, 0));
> +               tree a = CALL_EXPR_ARG (x, 0);
> +               bool return_this = targetm.cxx.cdtor_returns_this ();
> +               if (return_this)
> +                 a = cp_save_expr (a);
> +               tree s = build_fold_indirect_ref_loc (loc, a);
>                 r = cp_build_init_expr (s, r);
> +               if (return_this)
> +                 r = build2_loc (loc, COMPOUND_EXPR, TREE_TYPE (x), r,
> +                                 fold_convert_loc (loc, TREE_TYPE (x), a));
>               }
>             x = r;
>             break;
> --- gcc/testsuite/g++.dg/cpp0x/constexpr-113083.C.jj    2024-01-13 00:05:00.077372302 +0100
> +++ gcc/testsuite/g++.dg/cpp0x/constexpr-113083.C       2024-02-22 22:20:20.622618992 +0100
> @@ -0,0 +1,16 @@
> +// PR c++/113083
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-Os" }
> +
> +struct A { constexpr A (); };
> +
> +void
> +foo ()
> +{
> +  A b;
> +}
> +
> +constexpr
> +A::A ()
> +{
> +}
>
>         Jakub
>
Christophe Lyon Feb. 23, 2024, 1:53 p.m. UTC | #2 
On Fri, 23 Feb 2024 at 10:13, Christophe Lyon
<christophe.lyon@linaro.org> wrote:
>
> On Fri, 23 Feb 2024 at 09:42, Jakub Jelinek <jakub@redhat.com> wrote:
> >
> > Hi!
> >
> > When targetm.cxx.cdtor_returns_this () (aka on arm32 TARGET_AAPCS_BASED)
> > constructor is supposed to return this pointer, but when we cp_fold such
> > a call, we don't take that into account and just INIT_EXPR the object,
> > so we can later ICE during gimplification, because the expression doesn't
> > have the right type.
> >
> > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux and
> > tested with a cross to armv7-linux-gnueabi on the testcase, but
> > unfortunately there are no 32-bit arm boxes in cfarm and arm32 is gone from
> > Fedora for quite some time as well, so I have no easy way to test this.
> > Christophe, do you think you could test this?  Thanks.
>
> Hi Jakub,
>
> Sadly our precommit CI could not apply your patch automatically (as
> you can see in patchwork).
>
> I'll test your patch manually.
>

I can now confirm that the new test passes on arm (native
armv8l-unknown-linux-gnueabihf), and no regression.

Thanks,

Christophe

> Thanks,
>
> Christophe
>
> >
> > 2024-02-23  Jakub Jelinek  <jakub@redhat.com>
> >
> >         PR c++/113083
> >         * cp-gimplify.cc (cp_fold): For targetm.cxx.cdtor_returns_this ()
> >         wrap r into a COMPOUND_EXPR and return folded CALL_EXPR_ARG (x, 0).
> >
> >         * g++.dg/cpp0x/constexpr-113083.C: New test.
> >
> > --- gcc/cp/cp-gimplify.cc.jj    2024-02-22 21:45:09.663430066 +0100
> > +++ gcc/cp/cp-gimplify.cc       2024-02-22 22:30:23.481428242 +0100
> > @@ -3412,9 +3412,15 @@ cp_fold (tree x, fold_flags_t flags)
> >             if (DECL_CONSTRUCTOR_P (callee))
> >               {
> >                 loc = EXPR_LOCATION (x);
> > -               tree s = build_fold_indirect_ref_loc (loc,
> > -                                                     CALL_EXPR_ARG (x, 0));
> > +               tree a = CALL_EXPR_ARG (x, 0);
> > +               bool return_this = targetm.cxx.cdtor_returns_this ();
> > +               if (return_this)
> > +                 a = cp_save_expr (a);
> > +               tree s = build_fold_indirect_ref_loc (loc, a);
> >                 r = cp_build_init_expr (s, r);
> > +               if (return_this)
> > +                 r = build2_loc (loc, COMPOUND_EXPR, TREE_TYPE (x), r,
> > +                                 fold_convert_loc (loc, TREE_TYPE (x), a));
> >               }
> >             x = r;
> >             break;
> > --- gcc/testsuite/g++.dg/cpp0x/constexpr-113083.C.jj    2024-01-13 00:05:00.077372302 +0100
> > +++ gcc/testsuite/g++.dg/cpp0x/constexpr-113083.C       2024-02-22 22:20:20.622618992 +0100
> > @@ -0,0 +1,16 @@
> > +// PR c++/113083
> > +// { dg-do compile { target c++11 } }
> > +// { dg-options "-Os" }
> > +
> > +struct A { constexpr A (); };
> > +
> > +void
> > +foo ()
> > +{
> > +  A b;
> > +}
> > +
> > +constexpr
> > +A::A ()
> > +{
> > +}
> >
> >         Jakub
> >
Jason Merrill Feb. 23, 2024, 5:52 p.m. UTC | #3 
On 2/23/24 08:53, Christophe Lyon wrote:
> On Fri, 23 Feb 2024 at 10:13, Christophe Lyon
> <christophe.lyon@linaro.org> wrote:
>>
>> On Fri, 23 Feb 2024 at 09:42, Jakub Jelinek <jakub@redhat.com> wrote:
>>>
>>> Hi!
>>>
>>> When targetm.cxx.cdtor_returns_this () (aka on arm32 TARGET_AAPCS_BASED)
>>> constructor is supposed to return this pointer, but when we cp_fold such
>>> a call, we don't take that into account and just INIT_EXPR the object,
>>> so we can later ICE during gimplification, because the expression doesn't
>>> have the right type.
>>>
>>> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux and
>>> tested with a cross to armv7-linux-gnueabi on the testcase, but
>>> unfortunately there are no 32-bit arm boxes in cfarm and arm32 is gone from
>>> Fedora for quite some time as well, so I have no easy way to test this.
>>> Christophe, do you think you could test this?  Thanks.
>>
>> Hi Jakub,
>>
>> Sadly our precommit CI could not apply your patch automatically (as
>> you can see in patchwork).
>>
>> I'll test your patch manually.
>>
> 
> I can now confirm that the new test passes on arm (native
> armv8l-unknown-linux-gnueabihf), and no regression.

The patch is OK.

Jason
diff mbox series

Patch

--- gcc/cp/cp-gimplify.cc.jj	2024-02-22 21:45:09.663430066 +0100
+++ gcc/cp/cp-gimplify.cc	2024-02-22 22:30:23.481428242 +0100
@@ -3412,9 +3412,15 @@  cp_fold (tree x, fold_flags_t flags)
 	    if (DECL_CONSTRUCTOR_P (callee))
 	      {
 		loc = EXPR_LOCATION (x);
-		tree s = build_fold_indirect_ref_loc (loc,
-						      CALL_EXPR_ARG (x, 0));
+		tree a = CALL_EXPR_ARG (x, 0);
+		bool return_this = targetm.cxx.cdtor_returns_this ();
+		if (return_this)
+		  a = cp_save_expr (a);
+		tree s = build_fold_indirect_ref_loc (loc, a);
 		r = cp_build_init_expr (s, r);
+		if (return_this)
+		  r = build2_loc (loc, COMPOUND_EXPR, TREE_TYPE (x), r,
+				  fold_convert_loc (loc, TREE_TYPE (x), a));
 	      }
 	    x = r;
 	    break;
--- gcc/testsuite/g++.dg/cpp0x/constexpr-113083.C.jj	2024-01-13 00:05:00.077372302 +0100
+++ gcc/testsuite/g++.dg/cpp0x/constexpr-113083.C	2024-02-22 22:20:20.622618992 +0100
@@ -0,0 +1,16 @@ 
+// PR c++/113083
+// { dg-do compile { target c++11 } }
+// { dg-options "-Os" }
+
+struct A { constexpr A (); };
+
+void
+foo ()
+{
+  A b;
+}
+
+constexpr
+A::A ()
+{
+}