Re: PR82943 - Suggested patch to fix
Hello and Happy New Year! I wanted to follow up on this patch I made to address PR82943 for GFortran. Has anyone had a chance to review it? Thanks, Alexander Westbrooks On Thu, Jun 29, 2023 at 10:38 PM Alexander Westbrooks wrote: > Hello, > > I have finished my testing, and updated my patch and relevant Changelogs. > I added 4 new tests and all the existing tests in the current testsuite > for gfortran passed or failed as expected. Do I need to attach the test > results here? > > The platform I tested on was a Docker container running in Docker Desktop, > running the "mcr.microsoft.com/devcontainers/universal:2-linux" image. > > I also made sure that my code changes followed the coding standards. > Please let me know if there is anything else that I need to do. I don't > have write-access to the repository. > > Thanks, > > Alexander > > On Wed, Jun 28, 2023 at 4:14 PM Harald Anlauf wrote: > >> Hi Alex, >> >> welcome to the gfortran community. It is great that you are trying >> to get actively involved. >> >> You already did quite a few things right: patches shall be sent to >> the gcc-patches ML, but Fortran reviewers usually notice them only >> where they are copied to the fortran ML. >> >> There are some general recommendations on the formatting of C code, >> like indentation, of the patches, and of the commit log entries. >> >> Regarding coding standards, see https://www.gnu.org/prep/standards/ . >> >> Regarding testcases, a recommendation is to have a look at >> existing testcases, e.g. in gcc/testsuite/gfortran.dg/, and then >> decide if the testcase shall test the compile-time or run-time >> behaviour, and add the necessary dejagnu directives. >> >> You should also verify if your patch passes regression testing. >> For changes to gfortran, it is usually sufficient to run >> >> make check-fortran -j >> >> where is the number of parallel tests. >> You would need to report also the platform where you tested on. >> >> There is also a legal issue to consider before non-trivial patches can >> be accepted for incorporation: https://gcc.gnu.org/contribute.html#legal >> >> If your patch is accepted and if you do not have write-access to the >> repository, one of the maintainers will likely take care of it. >> If you become a regular contributor, you will probably want to consider >> getting write access. >> >> Cheers, >> Harald >> >> >> >> On 6/24/23 19:17, Alexander Westbrooks via Gcc-patches wrote: >> > Hello, >> > >> > I am new to the GFortran community. Over the past two weeks I created a >> > patch that should fix PR82943 for GFortran. I have attached it to this >> > email. The patch allows the code below to compile successfully. I am >> > working on creating test cases next, but I am new to the process so it >> may >> > take me some time. After I make test cases, do I email them to you as >> well? >> > Do I need to make a pull-request on github in order to get the patch >> > reviewed? >> > >> > Thank you, >> > >> > Alexander Westbrooks >> > >> > module testmod >> > >> > public :: foo >> > >> > type, public :: tough_lvl_0(a, b) >> > integer, kind :: a = 1 >> > integer, len :: b >> > contains >> > procedure :: foo >> > end type >> > >> > type, public, EXTENDS(tough_lvl_0) :: tough_lvl_1 (c) >> > integer, len :: c >> > contains >> > procedure :: bar >> > end type >> > >> > type, public, EXTENDS(tough_lvl_1) :: tough_lvl_2 (d) >> > integer, len :: d >> > contains >> > procedure :: foobar >> > end type >> > >> > contains >> > subroutine foo(this) >> > class(tough_lvl_0(1,*)), intent(inout) :: this >> > end subroutine >> > >> > subroutine bar(this) >> > class(tough_lvl_1(1,*,*)), intent(inout) :: this >> > end subroutine >> > >> > subroutine foobar(this) >> > class(tough_lvl_2(1,*,*,*)), intent(inout) :: this >> > end subroutine >> > >> > end module >> > >> > PROGRAM testprogram >> > USE testmod >> > >> > TYPE(tough_lvl_0(1,5)) :: test_pdt_0 >> > TYPE(tough_lvl_1(1,5,6)) :: test_pdt_1 >> > TYPE(tough_lvl_2(1,5,6,7)) :: test_pdt_2 >> > >> > CALL test_pdt_0%foo() >> > >> > CALL test_pdt_1%foo() >> > CALL test_pdt_1%bar() >> > >> > CALL test_pdt_2%foo() >> > CALL test_pdt_2%bar() >> > CALL test_pdt_2%foobar() >> > >> > >> > END PROGRAM testprogram >> >>
Re: PR82943 - Suggested patch to fix
On 1/20/24 10:46 AM, Alexander Westbrooks wrote: Hello and Happy New Year! I wanted to follow up on this patch I made to address PR82943 for GFortran. Has anyone had a chance to review it? Thanks, Alexander Westbrooks Inserting myself in here just a little bit. I will apply and test today if I can. Paul is unavailable for a few weeks. Harald can chime in. Do you have commit rights for gcc? Your efforts are appreciated. Regards, Jerry
Re: PR82943 - Suggested patch to fix
On 1/20/24 11:08 AM, Jerry D wrote: On 1/20/24 10:46 AM, Alexander Westbrooks wrote: Hello and Happy New Year! I wanted to follow up on this patch I made to address PR82943 for GFortran. Has anyone had a chance to review it? Thanks, Alexander Westbrooks Inserting myself in here just a little bit. I will apply and test today if I can. Paul is unavailable for a few weeks. Harald can chime in. Do you have commit rights for gcc? Your efforts are appreciated. Regards, Jerry I did send you an invite to our Mattermost workspace. I did apply your patch. Some comments. The ChangeLog files are auto generated so do not get manually changed with a patch. The push process to the gcc git repository will generate those from the git commit log. If the git commit log is not formatted correctly the push will be rejected. The patch attempts to create a PDT_33.f03 test case. There is one already there so probably rename that one. In resolve.cc There was a compile error that looked like an extra '&&' in the conditional. I deleted that and all compiled ok and all regression tested OK, including your new test cases and the existing PDT_33.f03 case. I did not try your new test case yet for PDT_33. So next steps are walk you through using the git scripts to make commits with the logs formatted correctly. (assuming no one else chimes in with any other comment about code changes themselves.. Regards, Jerry
Re: PR82943 - Suggested patch to fix
Am 20.01.24 um 20:08 schrieb Jerry D: On 1/20/24 10:46 AM, Alexander Westbrooks wrote: Hello and Happy New Year! I wanted to follow up on this patch I made to address PR82943 for GFortran. Has anyone had a chance to review it? Thanks, Alexander Westbrooks Inserting myself in here just a little bit. I will apply and test today if I can. Paul is unavailable for a few weeks. Harald can chime in. Do you have commit rights for gcc? I am not aware of Alex having a copyright assignment on file, or a DCO certificate, and the patch is not signed off. But I might be wrong. Your efforts are appreciated. Regards, Jerry
[Fortran] half-cycle trig functions and atan[d] fixes
All, I have attached a new patch to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113152 which addresses the following issues. PR113152 -- implement half-cycle trigonometric functions PR113412 -- better error message for atan(y,x) PR113413 -- implement atand(y,x) The patch clocks in at 3488 lines, so will not be attached here. There are a few items that may need some explanation. First, some (if not all) of the half-cycle functions are contained in ISO/IEC TS 18661-4, IEEE754-2008, and now Fortran 2023. The patch assumes that an OS's libm may/will contain cospi(), etc. (see for example FreeBSD's libm). If these are not in libm, then fallback routines are provided by libgfortran. The fallback routines have seen limited testing, so your mileage may vary. Second, I have no idea what REAL17 entails; however, I provide untested routines in trigpi_fallback2.F90 where I use REAL(16) in the declarations (yikes). Third, MPFR provides half-cycle functions in version 4.2.0 (and later). For simplicifation, the patch checks for an adequate MPFR version, if an older version is found fallback algorithms are used. These have had limited testing. The patch appears to pass regression testing. Hopefully, someone will find time to test and commit the patch. The remainder is an attempt to write ChangeLog entries. gcc/fortran: * gfortran.h (gfc_isym_id): Add GFC_ISYM_ACOSPI, GFC_ISYM_ASINPI, GFC_ISYM_ATANPI, GFC_ISYM_ATAN2PI, GFC_ISYM_COSPI, GFC_ISYM_SINPI, and GFC_ISYM_TANPI * intrinsic.cc (do_check): Whitespace and typo in comments. (add_functions): Add two-argument form of ATAND. Add half-cycle trigonometric functions ACOSPI, ASINPI, ATANPI, ATAN2PI, COSPI, SINPI, and TANPI. (sort_actual): Generate sensible error messages for two argument versions of ATAN, ATAND, and ATANPI. * intrinsic.h: New prototypes for gfc_simplify_acospi, gfc_simplify_asinpi, gfc_simplify_atanpi, gfc_simplify_atan2pi, gfc_simplify_cospi, gfc_simplify_sinpi, gfc_simplify_tanpi, gfc_resolve_acospi, gfc_resolve_asinpi, gfc_resolve_atanpi, gfc_resolve_atan2pi, gfc_resolve_cospi, gfc_resolve_sinpi, and gfc_resolve_tanpi * intrinsic.texi: Document new functions ACOSPI, ASINPI, ATANPI, ATAN2PI, COSPI, SINPI, and TANPI. Put the ATAN* family of functions in lexigraphical order. * iresolve.cc (gfc_resolve_acospi, gfc_resolve_asinpi, gfc_resolve_atanpi, gfc_resolve_atan2pi, gfc_resolve_cospi, gfc_resolve_sinpi, gfc_resolve_tanpi): New functions. * simplify.cc (gfc_simplify_acospi, gfc_simplify_asinpi, gfc_simplify_atanpi, gfc_simplify_atan2pi, gfc_simplify_cospi, gfc_simplify_sinpi, gfc_simplify_tanpi): New functions. Introduce MPFR_HALF_CYCLE macros to use MPFR half-cycle functions if available. * trans-intrinsic.cc: Declare asinpi, acospi, atanpi, atan2pi, sinpi, cospi, and tanpi as LIB_FUNCTION's. libgfortran: *Makefile.am: New files trigpi.c, trigpi_fallback1.c, and trigpi_fallback2.F90 * configure.ac: Check libm for float, double, and long double functions for asinpi, acospi, atanpi, atan2pi, sinpi, cospi, and tanpi * Makefile.in: Regenerated. * config.h.in: Ditto. * configure: Ditto. * gfortran.map: Add GFORTRAN_14 section to expose the library symbols. * trigpi.c: Fallback implementations of the half-cycle trigonometric functions. * trigpi_fallback1.c: Fallback functions for float, double, and long double routines if libm does provide them. * trigpi_fallback2.F90: REAL(16)/REAL(17?) fallback implementations of the half-cycle trigonometric functions. -- Steve
Re: PR82943 - Suggested patch to fix
On 1/20/24 12:08 PM, Harald Anlauf wrote: Am 20.01.24 um 20:08 schrieb Jerry D: On 1/20/24 10:46 AM, Alexander Westbrooks wrote: Hello and Happy New Year! I wanted to follow up on this patch I made to address PR82943 for GFortran. Has anyone had a chance to review it? Thanks, Alexander Westbrooks Inserting myself in here just a little bit. I will apply and test today if I can. Paul is unavailable for a few weeks. Harald can chime in. Do you have commit rights for gcc? I am not aware of Alex having a copyright assignment on file, or a DCO certificate, and the patch is not signed off. But I might be wrong. --- snip --- I do not mind committing this but need clarifications regarding the copyright (copyleft?) rules in this case. In the past we have allowed small contributions like this. This may be a little more than minor. Regardless it appears to do the job! Jerry
Re: PR82943 - Suggested patch to fix
Am 20.01.24 um 21:37 schrieb Jerry D: On 1/20/24 12:08 PM, Harald Anlauf wrote: Am 20.01.24 um 20:08 schrieb Jerry D: On 1/20/24 10:46 AM, Alexander Westbrooks wrote: Hello and Happy New Year! I wanted to follow up on this patch I made to address PR82943 for GFortran. Has anyone had a chance to review it? Thanks, Alexander Westbrooks Inserting myself in here just a little bit. I will apply and test today if I can. Paul is unavailable for a few weeks. Harald can chime in. Do you have commit rights for gcc? I am not aware of Alex having a copyright assignment on file, or a DCO certificate, and the patch is not signed off. But I might be wrong. --- snip --- I do not mind committing this but need clarifications regarding the copyright (copyleft?) rules in this case. In the past we have allowed small contributions like this. This may be a little more than minor. It is. This is why I pointed to: https://gcc.gnu.org/dco.html Regardless it appears to do the job! Jerry
[PATCH] Fortran: passing of optional scalar arguments with VALUE attribute [PR113377]
Dear all,
here's the first part of an attempt to fix issues with optional
dummy arguments as actual arguments to optional dummies. This patch
rectifies the case of scalar dummies with the VALUE attribute,
which in gfortran's argument passing convention are passed on the
stack when they are of intrinsic type, and have a hidden variable
for the presence status.
The testcase tries to cover valid combinations of actual and dummy
argument. A few tests that are not standard-conforming but would
still work with gfortran (due to the argument passing convention)
are left there but commented out with a pointer to the standard
(thanks, Mikael!).
Regtested on x86_64-pc-linux-gnu. OK for mainline?
Thanks,
Harald
From f6a65138391c902d2782973665059d7d059a50d1 Mon Sep 17 00:00:00 2001
From: Harald Anlauf
Date: Sat, 20 Jan 2024 22:18:02 +0100
Subject: [PATCH] Fortran: passing of optional scalar arguments with VALUE
attribute [PR113377]
gcc/fortran/ChangeLog:
PR fortran/113377
* trans-expr.cc (gfc_conv_procedure_call): Fix handling of optional
scalar arguments of intrinsic type with the VALUE attribute.
gcc/testsuite/ChangeLog:
PR fortran/113377
* gfortran.dg/optional_absent_9.f90: New test.
---
gcc/fortran/trans-expr.cc | 5 +
.../gfortran.dg/optional_absent_9.f90 | 324 ++
2 files changed, 329 insertions(+)
create mode 100644 gcc/testsuite/gfortran.dg/optional_absent_9.f90
diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc
index 9dd1f4086f4..2f47a75955c 100644
--- a/gcc/fortran/trans-expr.cc
+++ b/gcc/fortran/trans-expr.cc
@@ -6526,6 +6526,10 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
gfc_init_se (&argse, NULL);
argse.want_pointer = 1;
gfc_conv_expr (&argse, e);
+ if (e->symtree->n.sym->attr.dummy
+&& POINTER_TYPE_P (TREE_TYPE (argse.expr)))
+ argse.expr = gfc_build_addr_expr (NULL_TREE,
+argse.expr);
cond = fold_convert (TREE_TYPE (argse.expr),
null_pointer_node);
cond = fold_build2_loc (input_location, NE_EXPR,
@@ -7256,6 +7260,7 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
&& e->symtree->n.sym->attr.optional
&& (((e->rank != 0 && elemental_proc)
|| e->representation.length || e->ts.type == BT_CHARACTER
+ || (e->rank == 0 && e->symtree->n.sym->attr.value)
|| (e->rank != 0
&& (fsym == NULL
|| (fsym->as
diff --git a/gcc/testsuite/gfortran.dg/optional_absent_9.f90 b/gcc/testsuite/gfortran.dg/optional_absent_9.f90
new file mode 100644
index 000..495a6c00d7f
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/optional_absent_9.f90
@@ -0,0 +1,324 @@
+! { dg-do run }
+! PR fortran/113377
+!
+! Test passing of missing optional scalar dummies of intrinsic type
+
+module m_int
+ implicit none
+contains
+ subroutine test_int ()
+integer :: k = 1
+call one (k)
+call one_val (k)
+call one_all (k)
+call one_ptr (k)
+ end
+
+ subroutine one (i, j)
+integer, intent(in) :: i
+integer ,optional :: j
+integer, allocatable :: aa
+integer, pointer :: pp => NULL()
+if (present (j)) error stop "j is present"
+call two (i, j)
+call two_val (i, j)
+call two (i, aa)
+call two (i, pp)
+ end
+
+ subroutine one_val (i, j)
+integer, intent(in) :: i
+integer, value, optional :: j
+if (present (j)) error stop "j is present"
+call two (i, j)
+call two_val (i, j)
+ end
+
+ subroutine one_all (i, j)
+integer, intent(in) :: i
+integer, allocatable,optional :: j
+if (present (j)) error stop "j is present"
+! call two (i, j) ! invalid per F2018:15.5.2.12, par. 3, clause 8
+! call two_val (i, j) ! invalid (*) F2018:15.5.2.12, par. 3, clause 8
+call two_all (i, j)
+ end
+! (*) gfortran argument passing conventions ("scalar dummy arguments of type
+! INTEGER, LOGICAL, REAL, COMPLEX, and CHARACTER(len=1) with VALUE attribute
+! pass the presence status separately") may still allow this case pass
+
+ subroutine one_ptr (i, j)
+integer, intent(in) :: i
+integer, pointer,optional :: j
+if (present (j)) error stop "j is present"
+! call two (i, j) ! invalid per F2018:15.5.2.12, par. 3, clause 7
+! call two_val (i, j) ! invalid (*) F2018:15.5.2.12, par. 3, clause 7
+call two_ptr (i, j)
+ end
+
+ subroutine two (i, j)
+integer, intent(in) :: i
+integer, intent(in), optional :: j
+if (present (j)) error stop 11
+ end
+
+ subroutine two_val (i, j)
+integer, intent(in) :: i
+integer, value, optional :: j
+if (present (j)) error stop 12
+ end
+
+ subroutine two_all (i, j)
+integer, intent(in) :: i
+integer, allocatable,optional :: j
+if (present (j)) error stop 13
+ end
+
+ subroutine two_ptr (i, j)
+integer, intent(in) :: i
+
Re: PR82943 - Suggested patch to fix
Based on what I am reading here, I can either do the DCO path or the copy right form path? Or is it both, where I send in the copy right forms and then on every commit I put the DCO? On Sat, Jan 20, 2024 at 3:40 PM Harald Anlauf wrote: > Am 20.01.24 um 21:37 schrieb Jerry D: > > On 1/20/24 12:08 PM, Harald Anlauf wrote: > >> Am 20.01.24 um 20:08 schrieb Jerry D: > >>> On 1/20/24 10:46 AM, Alexander Westbrooks wrote: > Hello and Happy New Year! > > I wanted to follow up on this patch I made to address PR82943 for > GFortran. Has anyone had a chance to review it? > > Thanks, > > Alexander Westbrooks > > >>> > >>> Inserting myself in here just a little bit. I will apply and test > today > >>> if I can. Paul is unavailable for a few weeks. Harald can chime in. > >>> > >>> Do you have commit rights for gcc? > >> > >> I am not aware of Alex having a copyright assignment on file, > >> or a DCO certificate, and the patch is not signed off. > >> But I might be wrong. > >> > > --- snip --- > > > > I do not mind committing this but need clarifications regarding the > > copyright (copyleft?) rules in this case. In the past we have allowed > > small contributions like this. This may be a little more than minor. > > It is. This is why I pointed to: > > https://gcc.gnu.org/dco.html > > > Regardless it appears to do the job! > > > > Jerry > > > > > >
