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

["mikael at gcc dot gnu.org" <gcc-bugzilla@gcc.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.