Re: [Patch, Fortran, PR58586, v2] ICE with derived type with allocatable component passed by value

[Mikael Morin <mikael.morin@sfr.fr>]

Hello,

I'm reviewing the original patch only for now.
The added bits in v2 will have to wait.

Le 23/04/2015 20:00, Andre Vehreschild a écrit :
>>> diff --git a/gcc/fortran/trans-expr.c b/gcc/fortran/trans-expr.c
>>> index 9e6432f..80dfed1 100644
>>> --- a/gcc/fortran/trans-expr.c
>>> +++ b/gcc/fortran/trans-expr.c
>>> @@ -5344,8 +5344,19 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol *
>>> sym, && (e->expr_type != EXPR_VARIABLE && !e->rank))
>>>          {
>>>  	  int parm_rank;
>>> -	  tmp = build_fold_indirect_ref_loc (input_location,
>>> -					 parmse.expr);
>>> +	  /* It is known the e returns a structure type with at least one
>>> +	     allocatable component.  When e is a function, ensure that the
>>> +	     function is called once only by using a temporary variable.
>>> */
>>> +	  if (e->expr_type == EXPR_FUNCTION)
>>> +	    parmse.expr = gfc_evaluate_now_loc (input_location,
>>> +						parmse.expr, &se->pre);
>> You need not limit this to functions only.
>> I think you can even do this without condition.
> 
> Yes, one could do that, but then an unnecessary temporary variable in the - for
> my taste - already too clobbered pseudo code is introduced. Furthermore, is the
> penalty on doing the check for a function negligible. I therefore have not
> changed that.

All right; would you mind writing it either
	if (e->expr_type != EXPR_VARIABLE)
or
	if (!DECL_P (parmse.expr))
or
	if (!VAR_P (parmse.expr))
instead?

> 
>>> +	  if (POINTER_TYPE_P (TREE_TYPE (parmse.expr)))
>> This distinguishes arguments with/without value attribute, right?
>> I think it's better to use the frontend information here (fsym->attr.value).
> 
> Changed to check for value.

Please check fsym && fsym->attr.value
and add the following testcase (it fails with the patch).


module m
  type :: t
    integer, allocatable :: comp
  end type
  type :: u
    type(t), allocatable :: comp
  end type
end module m

  use m
  call sub(u())
end


OK with these changes.

> 
>> Ah, and don't forget to provide a ChangeLog entry with it.
> 
> The Changelog entry comes in an additional attachment. 
> 
It doesn't appear inline in my mailer as its content type is
application/octet-stream, so I missed it.  Sorry.

Thanks for the patch.  I will review the rest later.

Mikael