Re: [PATCH] Fortran: passing of optional scalar arguments with VALUE attribute [PR113377]
Hello,
Le 20/01/2024 à 22:58, Harald Anlauf a écrit :
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?
Well, not yet.
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);
The second part of the condition looks superfluous: if
argse.want_pointer was set, we can expect to get a pointer result.
But more important, I don't understand the need for this whole part, the
new test seems to pass without it.
And here is an example that regresses with it.
program p
type :: t
integer, allocatable :: c
end type
call s2(t())
contains
subroutine s1(a)
integer, value, optional :: a
if (present(a)) stop 1
end subroutine
subroutine s2(a)
type(t) :: a
call s1(a%c)
end subroutine
end program
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)
This looks good.
|| (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)
To be complete, you could check two_val(i, aa) and two_val(i, pp) as well.
Both seem to pass already without the patch, so not absolutely needed.
Your call.
+ end
+
I think the patch is OK with the first trans-expr.cc hunk removed.
Thanks.
Mikael
Re: [PATCH] Fortran: passing of optional scalar arguments with VALUE attribute [PR113377]
Hi Mikael!
Am 21.01.24 um 11:50 schrieb Mikael Morin:
Hello,
Le 20/01/2024 à 22:58, Harald Anlauf a écrit :
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?
Well, not yet.
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);
The second part of the condition looks superfluous: if
argse.want_pointer was set, we can expect to get a pointer result.
But more important, I don't understand the need for this whole part, the
new test seems to pass without it.
And here is an example that regresses with it.
program p
type :: t
integer, allocatable :: c
end type
call s2(t())
contains
subroutine s1(a)
integer, value, optional :: a
if (present(a)) stop 1
end subroutine
subroutine s2(a)
type(t) :: a
call s1(a%c)
end subroutine
end program
Thanks for this example! I've taken the liberty to add a slightly
extended version of it to the testcase.
I was taken astray by the attempt to handle the (invalid by the
standard) variant of passing an absent allocatable scalar to
an optional scalar dummy with the value attribute under since
we use a hidden variable for the present status. Without the
code above there is an unprotected pointer dereference.
I think that it still could be done, but it is probably not worth
it. So I followed your suggestion and removed that part.
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)
This looks good.
|| (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)
To be complete, you could check two_val(i, aa) and two_val(i, pp) as well.
Both seem to pass already without the patch, so not absolutely needed.
Your call.
It is already contained in testcase gfortran.dg/value_optional_1.f90,
(see call sub there), but then it may be helpful to have it here too.
Thus added.
+ end
+
I think the patch is OK with the first trans-expr.cc hunk removed.
Thanks.
That's what I have done and pushed as r14-8317-g68862e5c75ef0e.
Mikael
Thanks for the review!
Harald
Re: PR82943 - Suggested patch to fix
Am 20.01.24 um 23:42 schrieb Alexander Westbrooks: 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? I thought the text is pretty clear. As already mentioned, https://gcc.gnu.org/contribute.html#legal gives you the options. One of these options is: "Alternatively, a contributor can certify the Developer Certificate of Origin for their contribution by adding the Signed-off-by: tag to their submission." If you opt for this variant, https://gcc.gnu.org/dco.html tells you everything. Basically (but please read yourself): "The sign-off is a simple line at the end of the explanation for the patch, which certifies that you wrote it or otherwise have the right to pass it on as a free software patch. ..." [...] "then you just add a line saying: Signed-off-by: Random J Developer using your real name (sorry, no pseudonyms or anonymous contributions.) ..." I think this would be sufficient. 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
