[Patch, Fortran, PR58586, v3] ICE with derived type with allocatable component passed by value

[Andre Vehreschild]

Hi Mikael, hi all,

Mikael, thanks for the review so far. I have inserted the changes requested and
updated this patch to trunk. For the e->expr_type == EXPR_FUNCTION I have
choose !DECL_P(parmse.expr) to not only prevent VAR_DECLs aliasing, but also
prevent aliasing for PARM_DECLs and similar.

Bootstraps and regtests ok on x86_64-linux-gnu/F21. 

Note, this patch also fixes the regression in alloc_comp_constructor_1.f90
introduced by my patch for pr44672, v4. The memory loss is also fixed again.

Ok for trunk?

Regards,
	Andre

On Sun, 26 Apr 2015 12:22:56 +0200
Mikael Morin <mikael.morin@sfr.fr> wrote:

> 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


-- 
Andre Vehreschild * Email: vehre ad gmx dot de 
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: pr58586_3.clog
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20150505/27f0684c/attachment.ksh>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr58586_3.patch
Type: text/x-patch
Size: 8707 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20150505/27f0684c/attachment.bin>