[Bug fortran/92887] [F2008] Passing nullified/disassociated pointer or unalloc allocatable to OPTIONAL + VALUE dummy fails

[Bug fortran/92887] [F2008] Passing nullified/disassociated pointer or unalloc allocatable to OPTIONAL + VALUE dummy fails

[anlauf at gcc dot gnu.org]

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.

[Bug fortran/92887] [F2008] Passing nullified/disassociated pointer or unalloc allocatable to OPTIONAL + VALUE dummy fails

[anlauf at gcc dot gnu.org]

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&action=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

[anlauf at gcc dot gnu.org]

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, &parmse.pre);
        }

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

[anlauf at gcc dot gnu.org]

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&action=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

[mikael at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92887

Mikael Morin <mikael at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mikael at gcc dot gnu.org

--- Comment #5 from Mikael Morin <mikael at gcc dot gnu.org> ---
(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 (&argse, NULL);
> +			    argse.want_pointer = 1;
> +			    gfc_conv_expr (&argse, 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, &parmse.pre);

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),
> +					     cond_temp,
> +					     parmse.expr, dflt_temp);
> +	      parmse.expr = gfc_evaluate_now (parmse.expr, &parmse.pre);
> +	    }
> +	}
> +
> +
>        if (fsym && need_interface_mapping && e)
>  	gfc_add_interface_mapping (&mapping, fsym, &parmse, e);
>

[Bug fortran/92887] [F2008] Passing nullified/disassociated pointer or unalloc allocatable to OPTIONAL + VALUE dummy fails

[anlauf at gcc dot gnu.org]

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, &parmse.pre);
> 
> 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;
> > +	      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, &parmse.pre);
> > +	    }
> > +	}
> > +
> > +
> >        if (fsym && need_interface_mapping && e)
> >  	gfc_add_interface_mapping (&mapping, fsym, &parmse, e);
> >

I agree that it was a bad idea to merge the patches for these two PRs just
because temporaries are needed.

[Bug fortran/92887] [F2008] Passing nullified/disassociated pointer or unalloc allocatable to OPTIONAL + VALUE dummy fails

[mikael at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92887

--- Comment #7 from Mikael Morin <mikael at gcc dot gnu.org> ---
(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, &parmse.pre);
> > 
> > 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, &parmse.pre);
> > > +	    }
> > > +	}
> > > +
> > > +
> > >        if (fsym && need_interface_mapping && e)
> > >  	gfc_add_interface_mapping (&mapping, fsym, &parmse, 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 existing code doesn't really help to guide changes. 
I thing we should try to make it possible to thing locally about a specific
case without caring about the rest.  But as new missing cases are discovered we
keep adding code, making it less and less local, and ... Well, we keep feeding
the monster.

[Bug fortran/92887] [F2008] Passing nullified/disassociated pointer or unalloc allocatable to OPTIONAL + VALUE dummy fails

[anlauf at gcc dot gnu.org]

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

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92887

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Harald Anlauf <anlauf@gcc.gnu.org>:

https://gcc.gnu.org/g:413ac2c8608cd0378955af27f69e45274b025b32

commit r14-5111-g413ac2c8608cd0378955af27f69e45274b025b32
Author: Harald Anlauf <anlauf@gmx.de>
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

[anlauf at gcc dot gnu.org]

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.