[Bug fortran/92887] [F2008] Passing nullified/disassociated pointer or unalloc allocatable to OPTIONAL + VALUE dummy fails
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92887 anlauf at gcc dot gnu.org changed: What|Removed |Added Status|ASSIGNED|RESOLVED Resolution|--- |FIXED Target Milestone|--- |14.0 --- Comment #10 from anlauf at gcc dot gnu.org --- Fixed on mainline for gcc-14.
[Bug fortran/92887] [F2008] Passing nullified/disassociated pointer or unalloc allocatable to OPTIONAL + VALUE dummy fails
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92887 --- Comment #9 from CVS Commits --- The master branch has been updated by Harald Anlauf : https://gcc.gnu.org/g:413ac2c8608cd0378955af27f69e45274b025b32 commit r14-5111-g413ac2c8608cd0378955af27f69e45274b025b32 Author: Harald Anlauf Date: Wed Nov 1 22:55:36 2023 +0100 Fortran: passing of allocatable/pointer arguments to OPTIONAL+VALUE [PR92887] gcc/fortran/ChangeLog: PR fortran/92887 * trans-expr.cc (conv_cond_temp): Helper function for creation of a conditional temporary. (gfc_conv_procedure_call): Handle passing of allocatable or pointer actual argument to dummy with OPTIONAL + VALUE attribute. Actual arguments that are not allocated or associated are treated as not present. gcc/testsuite/ChangeLog: PR fortran/92887 * gfortran.dg/value_optional_1.f90: New test.
[Bug fortran/92887] [F2008] Passing nullified/disassociated pointer or unalloc allocatable to OPTIONAL + VALUE dummy fails
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92887 anlauf at gcc dot gnu.org changed: What|Removed |Added Assignee|unassigned at gcc dot gnu.org |anlauf at gcc dot gnu.org Status|NEW |ASSIGNED --- Comment #8 from anlauf at gcc dot gnu.org --- Cleaned up patch submitted: https://gcc.gnu.org/pipermail/fortran/2023-November/059883.html
[Bug fortran/92887] [F2008] Passing nullified/disassociated pointer or unalloc allocatable to OPTIONAL + VALUE dummy fails
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92887
--- Comment #7 from Mikael Morin ---
(In reply to anlauf from comment #6)
> (In reply to Mikael Morin from comment #5)
> > (In reply to anlauf from comment #4)
> > > @@ -6396,7 +6399,28 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol *
> > > sym,
> > > && fsym->ts.type != BT_CLASS
> > > && fsym->ts.type != BT_DERIVED)
> > > {
> > > - if (e->expr_type != EXPR_VARIABLE
> > > + /* F2018:15.5.2.12 Argument presence and
> > > +restrictions on arguments not present. */
> > > + if (e->expr_type == EXPR_VARIABLE
> > > + && (e->symtree->n.sym->attr.allocatable
> > > + || e->symtree->n.sym->attr.pointer))
> >
> > Beware of expressions like derived%alloc_comp or derived%pointer_comp which
> > don't match the above.
>
> Right. This is fixable by using
>
>
> && (gfc_expr_attr (e).allocatable
> || gfc_expr_attr (e).pointer))
>
> instead.
>
Sure. Easy. :-)
> > > @@ -7072,6 +7096,42 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol *
> > > sym,
> > > }
> > > }
> > >
> > > + /* F2023:15.5.3, 15.5.4: Actual argument expressions are evaluated
> > > + before they are associated and a procedure is executed. */
> > > + if (e && e->expr_type != EXPR_VARIABLE && !gfc_is_constant_expr
> > > (e))
> > > + {
> > > + /* Create temporary except for functions returning pointers that
> > > + can appear in a variable definition context. */
> >
> > Maybe explain *why* we have to create a temporary, that is some data
> > references may become undefined by the procedure call (intent(out) dummies)
> > so we have to evaluate values depending on them beforehand (PR 92178).
>
> That is one reason. Another one, also pointed out in PR92178 by Tobias'
> review
> of Steve's draft, is the first testcase at
>
> https://gcc.gnu.org/legacy-ml/gcc-patches/2019-10/msg01970.html
>
Not sure I understand. Looks like the same reason to me. Or maybe my words
are not clear enough.
> This is reminiscent to an issue reported for the MERGE intrinsic (pr107874,
> fixed so far, but there is a remaining issue in pr105371).
>
I don't see how pr107874 or pr105371 are related to this.
> > > + need_temp = true;
> > > + }
> > > +
> > > + if (need_temp)
> > > + {
> > > + if (cond_temp == NULL_TREE)
> > > + parmse.expr = gfc_evaluate_now (parmse.expr, );
> >
> > I'm not sure about this. The condition to set need_temp looks quite general
> > (especially it matches non-scalar cases, doesn't it?), but
> > gfc_conv_expr_reference should already take care of creating a variable, so
> > that a temporary is missing only for value dummies, I think. I would rather
> > move this to the place specific to value dummies.
>
> I agree in principle. The indentation level is already awful in the specific
> place, which calls for thoughts about refactoring that mega-loop over the
> arguments than currently spans far more than 1000 source code lines.
>
Yes. The situation is severely out of hand there. We could just outline the
for loop body to a separate function, but I'm not sure it would by us much.
We could use different classes defining the argument passing convention (by
reference, by value with an extra argument, with array container, with class
container, etc) and dispatch to the methods of those classes. But it's
difficult to have a global understanding of what is needed here.
> > > + else
> >
> > I would rather move the else part to the place above where cond_temp is set,
> > so that the code is easier to follow.
> >
> > > + {
> > > + /* "Conditional temporary" to handle variables that possibly
> > > + cannot be dereferenced. Use null value as fallback. */
> > > + tree dflt_temp;
> > > + gcc_assert (e->ts.type != BT_DERIVED && e->ts.type != BT_CLASS);
> > > + gcc_assert (e->rank == 0);
> > > + dflt_temp = gfc_create_var (TREE_TYPE (parmse.expr), "temp");
> > > + TREE_STATIC (dflt_temp) = 1;
> > > + TREE_CONSTANT (dflt_temp) = 1;
> > > + TREE_READONLY (dflt_temp) = 1;
> > > + DECL_INITIAL (dflt_temp) = build_zero_cst (TREE_TYPE (dflt_temp));
> > > + parmse.expr = fold_build3_loc (input_location, COND_EXPR,
> > > + TREE_TYPE (parmse.expr),
> > > + cond_temp,
> > > + parmse.expr, dflt_temp);
> > > + parmse.expr = gfc_evaluate_now (parmse.expr, );
> > > + }
> > > + }
> > > +
> > > +
> > >if (fsym && need_interface_mapping && e)
> > > gfc_add_interface_mapping (, fsym, , e);
> > >
>
> I agree that it was a bad idea to merge the patches for these two PRs just
> because temporaries are needed.
Well, the structure of the
[Bug fortran/92887] [F2008] Passing nullified/disassociated pointer or unalloc allocatable to OPTIONAL + VALUE dummy fails
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92887
--- Comment #6 from anlauf at gcc dot gnu.org ---
(In reply to Mikael Morin from comment #5)
> (In reply to anlauf from comment #4)
> >
> > I'll need broader feedback, so unless someone adds to this pr, I'll submit
> > the present patch - with testcases - to get attention.
> >
> Here you go:
>
> > diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc
> > index 45a984b6bdb..d9dcc11e5bd 100644
> > --- a/gcc/fortran/trans-expr.cc
> > +++ b/gcc/fortran/trans-expr.cc
>
> > @@ -6396,7 +6399,28 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol *
> > sym,
> > && fsym->ts.type != BT_CLASS
> > && fsym->ts.type != BT_DERIVED)
> > {
> > - if (e->expr_type != EXPR_VARIABLE
> > + /* F2018:15.5.2.12 Argument presence and
> > + restrictions on arguments not present. */
> > + if (e->expr_type == EXPR_VARIABLE
> > + && (e->symtree->n.sym->attr.allocatable
> > + || e->symtree->n.sym->attr.pointer))
>
> Beware of expressions like derived%alloc_comp or derived%pointer_comp which
> don't match the above.
Right. This is fixable by using
&& (gfc_expr_attr (e).allocatable
|| gfc_expr_attr (e).pointer))
instead.
> > @@ -7072,6 +7096,42 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol *
> > sym,
> > }
> > }
> >
> > + /* F2023:15.5.3, 15.5.4: Actual argument expressions are evaluated
> > +before they are associated and a procedure is executed. */
> > + if (e && e->expr_type != EXPR_VARIABLE && !gfc_is_constant_expr (e))
> > + {
> > + /* Create temporary except for functions returning pointers that
> > +can appear in a variable definition context. */
>
> Maybe explain *why* we have to create a temporary, that is some data
> references may become undefined by the procedure call (intent(out) dummies)
> so we have to evaluate values depending on them beforehand (PR 92178).
That is one reason. Another one, also pointed out in PR92178 by Tobias' review
of Steve's draft, is the first testcase at
https://gcc.gnu.org/legacy-ml/gcc-patches/2019-10/msg01970.html
This is reminiscent to an issue reported for the MERGE intrinsic (pr107874,
fixed so far, but there is a remaining issue in pr105371).
> > + if (e->expr_type != EXPR_FUNCTION
> > + || !(gfc_expr_attr (e).pointer || gfc_expr_attr (e).proc_pointer))
>
> Merge with the outer condition?
Yes. The above form was intended more for proof-of-concept and readability
than for coding standards.
> > + need_temp = true;
> > + }
> > +
> > + if (need_temp)
> > + {
> > + if (cond_temp == NULL_TREE)
> > + parmse.expr = gfc_evaluate_now (parmse.expr, );
>
> I'm not sure about this. The condition to set need_temp looks quite general
> (especially it matches non-scalar cases, doesn't it?), but
> gfc_conv_expr_reference should already take care of creating a variable, so
> that a temporary is missing only for value dummies, I think. I would rather
> move this to the place specific to value dummies.
I agree in principle. The indentation level is already awful in the specific
place, which calls for thoughts about refactoring that mega-loop over the
arguments than currently spans far more than 1000 source code lines.
> I think this PR is only about scalars with basic types, is there the same
> problem with derived types? with classes?
> I guess arrays are different as they are always by reference?
For the current documentation of the argument passing convention see:
https://gcc.gnu.org/onlinedocs/gfortran/Argument-passing-conventions.html
"For OPTIONAL dummy arguments, an absent argument is denoted by a NULL pointer,
except for scalar dummy arguments of intrinsic type which have the VALUE
attribute. For those, a hidden Boolean argument (logical(kind=C_bool),value) is
used to indicate whether the argument is present."
My understanding is that for these scalar arguments we do need something that
can be passed by value.
We currently do not support VALUE with array arguments (F2008+), character
of length > 1, and character actual arguments are broken unless they are
constants. There are several open PRs.
> > + else
>
> I would rather move the else part to the place above where cond_temp is set,
> so that the code is easier to follow.
>
> > + {
> > + /* "Conditional temporary" to handle variables that possibly
> > +cannot be dereferenced. Use null value as fallback. */
> > + tree dflt_temp;
> > + gcc_assert (e->ts.type != BT_DERIVED && e->ts.type != BT_CLASS);
> > + gcc_assert (e->rank == 0);
> > + dflt_temp = gfc_create_var (TREE_TYPE (parmse.expr), "temp");
> > + TREE_STATIC (dflt_temp) = 1;
> > +
[Bug fortran/92887] [F2008] Passing nullified/disassociated pointer or unalloc allocatable to OPTIONAL + VALUE dummy fails
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92887
Mikael Morin changed:
What|Removed |Added
CC||mikael at gcc dot gnu.org
--- Comment #5 from Mikael Morin ---
(In reply to anlauf from comment #4)
>
> I'll need broader feedback, so unless someone adds to this pr, I'll submit
> the present patch - with testcases - to get attention.
>
Here you go:
> diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc
> index 45a984b6bdb..d9dcc11e5bd 100644
> --- a/gcc/fortran/trans-expr.cc
> +++ b/gcc/fortran/trans-expr.cc
> @@ -6396,7 +6399,28 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
> && fsym->ts.type != BT_CLASS
> && fsym->ts.type != BT_DERIVED)
> {
> - if (e->expr_type != EXPR_VARIABLE
> + /* F2018:15.5.2.12 Argument presence and
> +restrictions on arguments not present. */
> + if (e->expr_type == EXPR_VARIABLE
> + && (e->symtree->n.sym->attr.allocatable
> + || e->symtree->n.sym->attr.pointer))
Beware of expressions like derived%alloc_comp or derived%pointer_comp which
don't match the above.
> + {
> + gfc_se argse;
> + gfc_init_se (, NULL);
> + argse.want_pointer = 1;
> + gfc_conv_expr (, e);
> + tmp = fold_convert (TREE_TYPE (argse.expr),
> + null_pointer_node);
> + tmp = fold_build2_loc (input_location, NE_EXPR,
> +logical_type_node,
> +argse.expr, tmp);
> + vec_safe_push (optionalargs,
> +fold_convert (boolean_type_node,
> + tmp));
> + need_temp = true;
> + cond_temp = tmp;
> + }
> + else if (e->expr_type != EXPR_VARIABLE
> || !e->symtree->n.sym->attr.optional
> || e->ref != NULL)
> vec_safe_push (optionalargs, boolean_true_node);
> @@ -7072,6 +7096,42 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
> }
> }
>
> + /* F2023:15.5.3, 15.5.4: Actual argument expressions are evaluated
> + before they are associated and a procedure is executed. */
> + if (e && e->expr_type != EXPR_VARIABLE && !gfc_is_constant_expr (e))
> + {
> + /* Create temporary except for functions returning pointers that
> + can appear in a variable definition context. */
Maybe explain *why* we have to create a temporary, that is some data references
may become undefined by the procedure call (intent(out) dummies) so we have to
evaluate values depending on them beforehand (PR 92178).
> + if (e->expr_type != EXPR_FUNCTION
> + || !(gfc_expr_attr (e).pointer || gfc_expr_attr (e).proc_pointer))
Merge with the outer condition?
> + need_temp = true;
> + }
> +
> + if (need_temp)
> + {
> + if (cond_temp == NULL_TREE)
> + parmse.expr = gfc_evaluate_now (parmse.expr, );
I'm not sure about this. The condition to set need_temp looks quite general
(especially it matches non-scalar cases, doesn't it?), but
gfc_conv_expr_reference should already take care of creating a variable, so
that a temporary is missing only for value dummies, I think. I would rather
move this to the place specific to value dummies.
I think this PR is only about scalars with basic types, is there the same
problem with derived types? with classes?
I guess arrays are different as they are always by reference?
> + else
I would rather move the else part to the place above where cond_temp is set, so
that the code is easier to follow.
> + {
> + /* "Conditional temporary" to handle variables that possibly
> + cannot be dereferenced. Use null value as fallback. */
> + tree dflt_temp;
> + gcc_assert (e->ts.type != BT_DERIVED && e->ts.type != BT_CLASS);
> + gcc_assert (e->rank == 0);
> + dflt_temp = gfc_create_var (TREE_TYPE (parmse.expr), "temp");
> + TREE_STATIC (dflt_temp) = 1;
> + TREE_CONSTANT (dflt_temp) = 1;
> + TREE_READONLY (dflt_temp) = 1;
> + DECL_INITIAL (dflt_temp) = build_zero_cst (TREE_TYPE (dflt_temp));
> + parmse.expr = fold_build3_loc (input_location, COND_EXPR,
> + TREE_TYPE (parmse.expr),
> +
[Bug fortran/92887] [F2008] Passing nullified/disassociated pointer or unalloc allocatable to OPTIONAL + VALUE dummy fails
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92887 anlauf at gcc dot gnu.org changed: What|Removed |Added Attachment #55356|0 |1 is obsolete|| --- Comment #4 from anlauf at gcc dot gnu.org --- Created attachment 55360 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55360=edit Patch v2, handles most of pr92887 and pr92178 This patch tries to gather temporary handling as needed by pr92887 and pr92178 and implements a simple "conditional temporary" for the case that we cannot dereference a disassociated pointer or unallocated allocatable (also only scalar version for now). There is still the issue for character variables and expression, which I think is independent of the present pr. I'll need broader feedback, so unless someone adds to this pr, I'll submit the present patch - with testcases - to get attention. Regtests ok here, BTW.
[Bug fortran/92887] [F2008] Passing nullified/disassociated pointer or unalloc allocatable to OPTIONAL + VALUE dummy fails
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92887
--- Comment #3 from anlauf at gcc dot gnu.org ---
When using the tentative patch
https://gcc.gnu.org/bugzilla/attachment.cgi?id=55333
and adding a hackish
else if (e && fsym && fsym->attr.value && e->expr_type == EXPR_VARIABLE)
{
parmse.expr = gfc_evaluate_now (parmse.expr, );
}
I see a temporary generated for the integer arguments, but not for the
character variable.
The above hackish addition is even not yet correct: we must not dereference
a disassociated pointer when copying to the temporary ...
[Bug fortran/92887] [F2008] Passing nullified/disassociated pointer or unalloc allocatable to OPTIONAL + VALUE dummy fails
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92887 --- Comment #2 from anlauf at gcc dot gnu.org --- Created attachment 55356 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55356=edit Partial patch for the presence attribute The attached patch generates a test for the presence that depends on the allocation or association status of the argument. An extended testcase however shows that we mishandle the VALUE attribute: we do not generate a proper temporary: program p implicit none (type, external) integer, allocatable :: aa integer, pointer :: pp character,allocatable :: ca character,pointer :: cp nullify (pp, cp) call sub (aa, pp, ca, cp) allocate (aa, pp, ca, cp) aa = 1; pp = 2; ca = "a"; cp = "b" call foo (3, 4, "a", "b") call foo (aa, pp, ca, cp) call bar (5, 6, "c", "d") call bar (aa, pp, ca, cp) call bla (7, 8, "e", "f") call bla (aa, pp, ca, cp) deallocate (aa, pp, ca, cp) contains subroutine sub (x, y, c, d) integer, value, optional :: x, y character, value, optional :: c, d if (present(x)) stop 1 if (present(y)) stop 2 if (present(c)) stop 3 if (present(d)) stop 4 end subroutine foo (x, y, c, d) integer, value, optional :: x, y character, value, optional :: c, d if (.not. present(x)) stop 1 if (.not. present(y)) stop 2 if (.not. present(c)) stop 3 if (.not. present(d)) stop 4 print *, "value+optional:", x, y, c, d end subroutine bar (x, y, c, d) integer, value :: x, y character, value :: c, d print *, "value :", x, y, c, d end subroutine bla (x, y, c, d) integer :: x, y character :: c, d print *, "no value :", x, y, c, d end end Executing this testcase shows junk for the character variable, and inspection of the tree dump shows that this is not limited to it...
[Bug fortran/92887] [F2008] Passing nullified/disassociated pointer or unalloc allocatable to OPTIONAL + VALUE dummy fails
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92887 anlauf at gcc dot gnu.org changed: What|Removed |Added Status|UNCONFIRMED |NEW Ever confirmed|0 |1 CC||anlauf at gcc dot gnu.org Last reconfirmed||2023-06-16 --- Comment #1 from anlauf at gcc dot gnu.org --- Confirmed.
