From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout.gmx.net (mout.gmx.net [212.227.17.20]) by sourceware.org (Postfix) with ESMTPS id BFE38385782D; Fri, 16 Feb 2024 21:19:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org BFE38385782D Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=gmx.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmx.de ARC-Filter: OpenARC Filter v1.0.0 sourceware.org BFE38385782D Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=212.227.17.20 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708118394; cv=none; b=aqBdp2OzrakwlAGrm2CayYmtjxkMkPBt0VK6UjP4XJFN2M6VElrMBZ1Gb7FRED8H+Sj/16eL19+UTjSCbbw3t2+PLEbnaA7i1VbFnAb7+Pei0AlQgVsVl7U96YLfdtyozwwzU71ihQTL2JN6zzfy/ifYLpF1Tsdxaozj7l+rDtk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708118394; c=relaxed/simple; bh=zNl+hNxlSb4ReAG5L6ZZLG0dAsj9jy4N+rvvRYHOhDc=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=wcQZj5gpW03pe1VKCzheuIHCvoAMIcZUVHuJYQjq5/kn7C/KqYnpg9dQTYTTsRvwVnSSn84AVuubbzU4jygeYKM2pvWShPkOrTLjlPvLTIMeaWNBS1xqT6CQYC8jtYi9ggOfHrjXJtih+p18VkRXo5AzDgaB29hv+iK8yj9c7y8= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.de; s=s31663417; t=1708118390; x=1708723190; i=anlauf@gmx.de; bh=zNl+hNxlSb4ReAG5L6ZZLG0dAsj9jy4N+rvvRYHOhDc=; h=X-UI-Sender-Class:Date:Subject:To:Cc:References:From: In-Reply-To; b=CdTmJwlYPY8aWamW140z3RWTF8gBmATT10PUeNv7fY1CD+/BiF5LkJwkvbU6cmsh ffE+JxYpWfZMvL2quCjC2n+zv17V+0TgItMTGS/bEFISsmm9RQisHmZgEkFl1lBRA 1+QgV7Nm3MM6fdvZNb+rY8uzKfSZIdMuC06fnQr/Uc8Q/5NQ3/GDZSYRt27VLN4gb Pw9ZHrj92vNW1AeMmjXbIgOl8MJkeZH6QKIzOdoeGyQeZQ2RmSsStgXSkEST5+eJ/ Sw62zK9Xhb/kv1VfAQCzDPQQBiETgOEsLJEG2KDHeB4qpa3PFQ43Ysxxdze5E70iC vsMpHC3CIF83kI0tGg== X-UI-Sender-Class: 724b4f7f-cbec-4199-ad4e-598c01a50d3a Received: from [192.168.178.29] ([79.232.154.23]) by mail.gmx.net (mrgmx105 [212.227.17.168]) with ESMTPSA (Nemesis) id 1M2wKq-1rXnpp3exw-003Pck; Fri, 16 Feb 2024 22:19:49 +0100 Message-ID: Date: Fri, 16 Feb 2024 22:19:48 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] Fortran: fix passing array component to polymorphic argument [PR105658] To: Peter Hill , gcc-patches@gcc.gnu.org Cc: fortran@gcc.gnu.org Newsgroups: gmane.comp.gcc.patches,gmane.comp.gcc.fortran References: Content-Language: en-US From: Harald Anlauf In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable X-Provags-ID: V03:K1:qt0v8LCvG3TODvpo/8Ixs79TdeqOrBeM7tj8yV4rVuxSpoqxs36 qx6jLhrWrowQQlWnoa6NIAWz5LIwGe99XbgvWEZj3CzSPMaa+OGWgQ3nYOQrxLKYHOuOx+I lS3S114CUf1i/RKkjnPWP/KgGaEMouQh6eZe2yAheA6cO+oOfgjC8ApLeGUsWkk2b7zpax/ seCOoyBBG/v0Qei8fQ08Q== UI-OutboundReport: notjunk:1;M01:P0:O4TZgQtXCyk=;dqJ7G0nmYM882LYmFJJZ1eHRVTZ BbcJ1++JtUMjqxRZNJMfhMHlKsyoOucl8Iey1x38EC8jiWY6pPtSFOxX+zY9J25lBYjpqapjO zvlnXS5VBJfrrjy+m0tQLG26mr1RMwICC5avV0QgJkeBDuuyL/vbhXxcxT3yDWq72oJvyJDaL CwdtHCNEZPYmTocbrMQejv21G/c0dH1akoCcbwdRwhobdnD9vt8m1Xgs3chV8vOvtV/SMst3h WaQ1sflntqMp37A1Djvsrfn6T2BNco5+GbxBYgBS+kT802E45HpAYYWNvJksvAxFvGCND1GyQ 3uQkWuUgeASdL9tsjvOOvEoLpTH9YfSv4GYVbQ7vIZ9Gv8wRLYXnTcX6P1ezm5uUe9niDamB3 Wb8beXOgIFKRF07yMpjKqUwUGof5XEuK2Qsea+17cP82wpG46exjnIXY1ROJ2+mOKADbR4yA6 vEXiE0emfUg6MCZ+opkS+rA3puwOPO0BsyJhUD5IVp2RM64gPv9tDATm6HlOE7NbQVoGcx0+M UyZovi+34aIoqwL9+JB++nVcJU8LQF0CDHIDJHPx97hTLoVyDPrrdUFiK44P+7iNZh7EE8XtT +qgGdnQVmJVOIgdwHw/NwY73UkJU3/4jQk99jMd8NGt54IxbWxPRH+b5V2jizosCNw3WTzfSY 2IuZBbr4tZ3jASigyKLlBOsHoItg9e9CL/WP3Bns3TqOhJRP6z75JzzfM9S7Rcr/5Mi4ftdhy BBSuh0CtO5xJ8zJe/pUMvfqUFOHpi17K0SbXaxwHMIK5LQPTY3g7eBIQz8ZL0h0s+SXkB97uI PQUMtBVmiuejKh8XhLP/91nvJR5jeTJvHjkaVYeT58M6E= X-Spam-Status: No, score=-12.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_LOW,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi Peter, thanks for your contribution to gfortran! You've found indeed a solution for a potentially annoying bug. Am 15.02.24 um 18:50 schrieb Peter Hill: > Dear all, > > The attached patch fixes PR105658 by forcing an array temporary to be > created. This is required when passing an array component, but this > didn't happen if the dummy argument was an unlimited polymorphic type. > > The problem bit of code is in `gfc_conv_expr_descriptor`, near L7828: > > subref_array_target =3D (is_subref_array (expr) > && (se->direct_byref > || expr->ts.type =3D=3D BT_CHARACTER)); > need_tmp =3D (gfc_ref_needs_temporary_p (expr->ref) > && !subref_array_target); > > where `need_tmp` is being evaluated to 0. The logic here isn't clear > to me, and this function is used in several places, which is why I > went with setting `parmse.force_tmp =3D 1` in `gfc_conv_procedure_call` > and using the same conditional as the later branch for the > non-polymorphic case (near the call to `gfc_conv_subref_array_arg`) > > If this patch is ok, please could someone commit it for me? This is my > first patch for GCC, so apologies in advance if the commit message is > missing something. Your patch mostly does the right thing. Note that when fsym is an unlimited polymorphic, some of its attributes are buried deep within its internal representation. I would also prefer to move the code to gfc_conv_intrinsic_to_class where it seems to fit better, like: diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc index a0593b76f18..db906caa52e 100644 =2D-- a/gcc/fortran/trans-expr.cc +++ b/gcc/fortran/trans-expr.cc @@ -1019,6 +1019,14 @@ gfc_conv_intrinsic_to_class (gfc_se *parmse, gfc_expr *e, tmp =3D gfc_typenode_for_spec (&class_ts); var =3D gfc_create_var (tmp, "class"); + /* Force a temporary for component or substring references. */ + if (unlimited_poly + && class_ts.u.derived->components->attr.dimension + && !class_ts.u.derived->components->attr.class_pointer + && !class_ts.u.derived->components->attr.allocatable + && is_subref_array (e)) + parmse->force_tmp =3D 1; + /* Set the vptr. */ ctree =3D gfc_class_vptr_get (var); (I am not entirely sure whether we need to exclude pointer and allocatable attributes here explicitly, given the constraints in F2023:15.5.2.6, but other may have an opinion, too. The above should be safe anyway.) > Tested on x86_64-pc-linux-gnu. > > The bug is present in gfortran back to 4.9, so should it also be backpor= ted? I think we'll target 14-mainline and might consider a backport to 13-branch. > Cheers, > Peter > > PR fortran/105658 > > gcc/fortran/ChangeLog > > * trans-expr.cc (gfc_conv_procedure_call): When passing an > array component reference of intrinsic type to a procedure > with an unlimited polymorphic dummy argument, a temporary > should be created. > > gcc/testsuite/ChangeLog > > * gfortran.dg/PR105658.f90: New test. > --- > gcc/fortran/trans-expr.cc | 8 ++++++++ > gcc/testsuite/gfortran.dg/PR105658.f90 | 25 +++++++++++++++++++++++++ > 2 files changed, 33 insertions(+) > create mode 100644 gcc/testsuite/gfortran.dg/PR105658.f90 > > diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc > index a0593b76f18..7fd3047c4e9 100644 > --- a/gcc/fortran/trans-expr.cc > +++ b/gcc/fortran/trans-expr.cc > @@ -6439,6 +6439,14 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol = * sym, > CLASS object for the unlimited polymorphic formal. */ > gfc_find_vtab (&e->ts); > gfc_init_se (&parmse, se); > + /* The actual argument is a component reference to an array > + of derived types, so we need to force creation of a > + temporary */ > + if (e->expr_type =3D=3D EXPR_VARIABLE > + && is_subref_array (e) > + && !(fsym && fsym->attr.pointer)) > + parmse.force_tmp =3D 1; > + > gfc_conv_intrinsic_to_class (&parmse, e, fsym->ts); > > } > diff --git a/gcc/testsuite/gfortran.dg/PR105658.f90 > b/gcc/testsuite/gfortran.dg/PR105658.f90 > new file mode 100644 > index 00000000000..407ee25f77c > --- /dev/null > +++ b/gcc/testsuite/gfortran.dg/PR105658.f90 > @@ -0,0 +1,25 @@ > +! { dg-do compile } > +! { dg-options "-Warray-temporaries" } > +! Test fix for incorrectly passing array component to unlimited > polymorphic procedure > + > +module test_PR105658_mod > + implicit none > + type :: foo > + integer :: member1 > + integer :: member2 > + end type foo > +contains > + subroutine print_poly(array) > + class(*), dimension(:), intent(in) :: array > + select type(array) > + type is (integer) > + print*, array > + end select > + end subroutine print_poly > + > + subroutine do_print(thing) > + type(foo), dimension(3), intent(in) :: thing > + call print_poly(thing%member1) ! { dg-warning "array temporary" } > + end subroutine do_print > + > +end module test_PR105658_mod One could extend this testcase to cover substrings as well: module test_PR105658_mod implicit none type :: foo integer :: member1 integer :: member2 end type foo contains subroutine print_poly(array) class(*), dimension(:), intent(in) :: array select type(array) type is (integer) print*, array type is (character(*)) print *, array end select end subroutine print_poly subroutine do_print(thing) type(foo), dimension(3), intent(in) :: thing type(foo), parameter :: y(3) =3D [foo(1,2),foo(3,4),foo(5,6)] integer :: i, j, uu(5,6) call print_poly(thing%member1) ! { dg-warning "array temporary" } call print_poly(y%member2) ! { dg-warning "array temporary" } call print_poly(y(1::2)%member2) ! { dg-warning "array temporary" } ! The following array sections work without temporaries uu =3D reshape([(((10*i+j),i=3D1,5),j=3D1,6)],[5,6]) print *, uu(2,2::2) call print_poly (uu(2,2::2)) ! no temp needed! print *, uu(1::2,6) call print_poly (uu(1::2,6)) ! no temp needed! end subroutine do_print subroutine do_print2(thing2) class(foo), dimension(:), intent(in) :: thing2 call print_poly (thing2% member2) ! { dg-warning "array temporary" } end subroutine do_print2 subroutine do_print3 () character(3) :: c(3) =3D ["abc","def","ghi"] call print_poly (c(1::2)) ! no temp needed! call print_poly (c(1::2)(2:3)) ! { dg-warning "array temporary" } end subroutine do_print3 end module test_PR105658_mod If you like, you can repackage the patch and sign it (see https://gcc.gnu.org/dco.html), and one of us will then commit it for you. Thanks! Harald From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ciao.gmane.io (ciao.gmane.io [116.202.254.214]) by sourceware.org (Postfix) with ESMTPS id 9E8723857702 for ; Fri, 16 Feb 2024 21:19:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9E8723857702 Authentication-Results: sourceware.org; dmarc=fail (p=quarantine dis=none) header.from=gmx.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=m.gmane-mx.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 9E8723857702 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=116.202.254.214 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708118399; cv=none; b=P3B26qr1KNAYU1Ny9i+c7TPsiolNGxW/hc/6PgMGP7GQdaeo6KOdph4peSYqGyj33oq9v46XtMmlOM50hSN0nCfNSigrIZ97QmRSUrKwrMJouUSkBZt0OgBvC05fyyCDJMFvAF3VfGWI0LChFHcPGcyep9pMSWjHzF0L0XMBOks= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708118399; c=relaxed/simple; bh=TGT8sllaIp0ZGapmYYuMm78fNkxy4oJ34ha0Kf2vOR0=; h=To:From:Subject:Date:Message-ID:Mime-Version; b=isRJ0ZQdWpyFuA+2GKo4JAjlZzbDylknLrzDPl3ulkFGJCjmxWYZ7msuiV7l+vpM7jKNwDZPRZqqKb4X3VXNAxb8J3brSQX7+fk+Vsq2umck/BU5XQRK/jcCcY+OU5gQLE05ywvqfmjgyyKg3H+yxBYyv61rTb5BFpiWYq7J61w= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from list by ciao.gmane.io with local (Exim 4.92) (envelope-from ) id 1rb5ci-0005jU-6M for gcc-patches@gcc.gnu.org; Fri, 16 Feb 2024 22:19:56 +0100 X-Injected-Via-Gmane: http://gmane.org/ To: gcc-patches@gcc.gnu.org From: Harald Anlauf Subject: Re: [PATCH] Fortran: fix passing array component to polymorphic argument [PR105658] Date: Fri, 16 Feb 2024 22:19:48 +0100 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit User-Agent: Mozilla Thunderbird Content-Language: en-US In-Reply-To: Cc: fortran@gcc.gnu.org X-Spam-Status: No, score=-9.1 required=5.0 tests=BAYES_00,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,KAM_SHORT,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Message-ID: <20240216211948.6W6R4J1_GAf10kPmzaPZ82HX0PqqwHCCOe8FbolG1Hk@z> Hi Peter, thanks for your contribution to gfortran! You've found indeed a solution for a potentially annoying bug. Am 15.02.24 um 18:50 schrieb Peter Hill: > Dear all, > > The attached patch fixes PR105658 by forcing an array temporary to be > created. This is required when passing an array component, but this > didn't happen if the dummy argument was an unlimited polymorphic type. > > The problem bit of code is in `gfc_conv_expr_descriptor`, near L7828: > > subref_array_target = (is_subref_array (expr) > && (se->direct_byref > || expr->ts.type == BT_CHARACTER)); > need_tmp = (gfc_ref_needs_temporary_p (expr->ref) > && !subref_array_target); > > where `need_tmp` is being evaluated to 0. The logic here isn't clear > to me, and this function is used in several places, which is why I > went with setting `parmse.force_tmp = 1` in `gfc_conv_procedure_call` > and using the same conditional as the later branch for the > non-polymorphic case (near the call to `gfc_conv_subref_array_arg`) > > If this patch is ok, please could someone commit it for me? This is my > first patch for GCC, so apologies in advance if the commit message is > missing something. Your patch mostly does the right thing. Note that when fsym is an unlimited polymorphic, some of its attributes are buried deep within its internal representation. I would also prefer to move the code to gfc_conv_intrinsic_to_class where it seems to fit better, like: diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc index a0593b76f18..db906caa52e 100644 --- a/gcc/fortran/trans-expr.cc +++ b/gcc/fortran/trans-expr.cc @@ -1019,6 +1019,14 @@ gfc_conv_intrinsic_to_class (gfc_se *parmse, gfc_expr *e, tmp = gfc_typenode_for_spec (&class_ts); var = gfc_create_var (tmp, "class"); + /* Force a temporary for component or substring references. */ + if (unlimited_poly + && class_ts.u.derived->components->attr.dimension + && !class_ts.u.derived->components->attr.class_pointer + && !class_ts.u.derived->components->attr.allocatable + && is_subref_array (e)) + parmse->force_tmp = 1; + /* Set the vptr. */ ctree = gfc_class_vptr_get (var); (I am not entirely sure whether we need to exclude pointer and allocatable attributes here explicitly, given the constraints in F2023:15.5.2.6, but other may have an opinion, too. The above should be safe anyway.) > Tested on x86_64-pc-linux-gnu. > > The bug is present in gfortran back to 4.9, so should it also be backported? I think we'll target 14-mainline and might consider a backport to 13-branch. > Cheers, > Peter > > PR fortran/105658 > > gcc/fortran/ChangeLog > > * trans-expr.cc (gfc_conv_procedure_call): When passing an > array component reference of intrinsic type to a procedure > with an unlimited polymorphic dummy argument, a temporary > should be created. > > gcc/testsuite/ChangeLog > > * gfortran.dg/PR105658.f90: New test. > --- > gcc/fortran/trans-expr.cc | 8 ++++++++ > gcc/testsuite/gfortran.dg/PR105658.f90 | 25 +++++++++++++++++++++++++ > 2 files changed, 33 insertions(+) > create mode 100644 gcc/testsuite/gfortran.dg/PR105658.f90 > > diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc > index a0593b76f18..7fd3047c4e9 100644 > --- a/gcc/fortran/trans-expr.cc > +++ b/gcc/fortran/trans-expr.cc > @@ -6439,6 +6439,14 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym, > CLASS object for the unlimited polymorphic formal. */ > gfc_find_vtab (&e->ts); > gfc_init_se (&parmse, se); > + /* The actual argument is a component reference to an array > + of derived types, so we need to force creation of a > + temporary */ > + if (e->expr_type == EXPR_VARIABLE > + && is_subref_array (e) > + && !(fsym && fsym->attr.pointer)) > + parmse.force_tmp = 1; > + > gfc_conv_intrinsic_to_class (&parmse, e, fsym->ts); > > } > diff --git a/gcc/testsuite/gfortran.dg/PR105658.f90 > b/gcc/testsuite/gfortran.dg/PR105658.f90 > new file mode 100644 > index 00000000000..407ee25f77c > --- /dev/null > +++ b/gcc/testsuite/gfortran.dg/PR105658.f90 > @@ -0,0 +1,25 @@ > +! { dg-do compile } > +! { dg-options "-Warray-temporaries" } > +! Test fix for incorrectly passing array component to unlimited > polymorphic procedure > + > +module test_PR105658_mod > + implicit none > + type :: foo > + integer :: member1 > + integer :: member2 > + end type foo > +contains > + subroutine print_poly(array) > + class(*), dimension(:), intent(in) :: array > + select type(array) > + type is (integer) > + print*, array > + end select > + end subroutine print_poly > + > + subroutine do_print(thing) > + type(foo), dimension(3), intent(in) :: thing > + call print_poly(thing%member1) ! { dg-warning "array temporary" } > + end subroutine do_print > + > +end module test_PR105658_mod One could extend this testcase to cover substrings as well: module test_PR105658_mod implicit none type :: foo integer :: member1 integer :: member2 end type foo contains subroutine print_poly(array) class(*), dimension(:), intent(in) :: array select type(array) type is (integer) print*, array type is (character(*)) print *, array end select end subroutine print_poly subroutine do_print(thing) type(foo), dimension(3), intent(in) :: thing type(foo), parameter :: y(3) = [foo(1,2),foo(3,4),foo(5,6)] integer :: i, j, uu(5,6) call print_poly(thing%member1) ! { dg-warning "array temporary" } call print_poly(y%member2) ! { dg-warning "array temporary" } call print_poly(y(1::2)%member2) ! { dg-warning "array temporary" } ! The following array sections work without temporaries uu = reshape([(((10*i+j),i=1,5),j=1,6)],[5,6]) print *, uu(2,2::2) call print_poly (uu(2,2::2)) ! no temp needed! print *, uu(1::2,6) call print_poly (uu(1::2,6)) ! no temp needed! end subroutine do_print subroutine do_print2(thing2) class(foo), dimension(:), intent(in) :: thing2 call print_poly (thing2% member2) ! { dg-warning "array temporary" } end subroutine do_print2 subroutine do_print3 () character(3) :: c(3) = ["abc","def","ghi"] call print_poly (c(1::2)) ! no temp needed! call print_poly (c(1::2)(2:3)) ! { dg-warning "array temporary" } end subroutine do_print3 end module test_PR105658_mod If you like, you can repackage the patch and sign it (see https://gcc.gnu.org/dco.html), and one of us will then commit it for you. Thanks! Harald