[Patch, fortran] PR65792 - allocation of scalar elemental function with structure constructor fails

[Patch, fortran] PR65792 - allocation of scalar elemental function with structure constructor fails

[Paul Richard Thomas]

[-- Attachment #1: Type: text/plain, Size: 932 bytes --]

Dear All,

This patch exploits Mikael's identification of the location of the
problem and his testcase. It requires Andre's patch for PR59678 as a
prerequisite. It is almost certainly a bit over-conservative but it
seems to be free of any memory leakage. The ChangeLog and the comments
describe it sufficiently for review.

OK for trunk and 5.x?

Paul

2015-04-26  Mikael Morin  <mikael@gcc.gnu.org
        Paul Thomas  <pault@gcc.gnu.org>

    PR fortran/65792
    * trans-expr.c (gfc_trans_subcomponent_assign): Always assign
    the expression component to the destination. In addition, if
    the component has allocatable components, copy them and
    deallocate those of the expression, if it is not a variable.
    The expression is fixed if not a variable to prevent multiple
    evaluations.

2015-04-26  Mikael Morin  <mikael@gcc.gnu.org>

    PR fortran/65792
    * gfortran.dg/derived_constructor_components_5: New test

[-- Attachment #2: submit.diff --]
[-- Type: text/plain, Size: 1944 bytes --]

Index: gcc/fortran/trans-expr.c
===================================================================
*** gcc/fortran/trans-expr.c	(revision 222439)
--- gcc/fortran/trans-expr.c	(working copy)
*************** gfc_trans_subcomponent_assign (tree dest
*** 7062,7080 ****
      {
        if (expr->expr_type != EXPR_STRUCTURE)
  	{
  	  gfc_init_se (&se, NULL);
  	  gfc_conv_expr (&se, expr);
  	  gfc_add_block_to_block (&block, &se.pre);
  	  if (cm->ts.u.derived->attr.alloc_comp
! 	      && expr->expr_type == EXPR_VARIABLE)
  	    {
  	      tmp = gfc_copy_alloc_comp (cm->ts.u.derived, se.expr,
  					 dest, expr->rank);
  	      gfc_add_expr_to_block (&block, tmp);
  	    }
- 	  else
- 	    gfc_add_modify (&block, dest,
- 			    fold_convert (TREE_TYPE (dest), se.expr));
  	  gfc_add_block_to_block (&block, &se.post);
  	}
        else
--- 7062,7091 ----
      {
        if (expr->expr_type != EXPR_STRUCTURE)
  	{
+ 	  tree dealloc = NULL_TREE;
  	  gfc_init_se (&se, NULL);
  	  gfc_conv_expr (&se, expr);
  	  gfc_add_block_to_block (&block, &se.pre);
+ 	  /* Prevent repeat evaluations in gfc_copy_alloc_comp by fixing the
+ 	     expression in  a temporary variable and deallocate is allocatable
+ 	     components the copy to the result.  */
  	  if (cm->ts.u.derived->attr.alloc_comp
! 	      && expr->expr_type != EXPR_VARIABLE)
! 	    {
! 	      se.expr = gfc_evaluate_now (se.expr, &block);
! 	      dealloc = gfc_deallocate_alloc_comp (cm->ts.u.derived, se.expr,
! 						   expr->rank);
! 	    }
! 	  gfc_add_modify (&block, dest,
! 			  fold_convert (TREE_TYPE (dest), se.expr));
! 	  if (cm->ts.u.derived->attr.alloc_comp)
  	    {
  	      tmp = gfc_copy_alloc_comp (cm->ts.u.derived, se.expr,
  					 dest, expr->rank);
  	      gfc_add_expr_to_block (&block, tmp);
+ 	      if (dealloc != NULL_TREE)
+ 		gfc_add_expr_to_block (&block, dealloc);
  	    }
  	  gfc_add_block_to_block (&block, &se.post);
  	}
        else

[-- Attachment #3: derived_constructor_comps_5.f90 --]
[-- Type: text/plain, Size: 1647 bytes --]

! { dg-do run }
!
! PR fortran/65792
! The evaluation of the argument in the call to new_prt_spec2
! failed to properly initialize the comp component.
! While the array contents were properly copied, the array bounds remained
! uninitialized.
!
! Contributed by Dominique D'Humieres <dominiq@lps.ens.fr>

program main
  implicit none

  integer, parameter :: n = 2

  type :: string_t
     character(LEN=1), dimension(:), allocatable :: chars
  end type string_t

  type :: string_container_t
     type(string_t) :: comp
  end type string_container_t

  type(string_t) :: prt_in, tmp, tmpa(n)
  type(string_container_t) :: tmpc, tmpca(n)
  integer :: i, j, k

  do i=1,2

! scalar elemental function with structure constructor
     prt_in = string_t(["D"])
     tmpc = new_prt_spec2 (string_container_t(prt_in))
     if (any(tmpc%comp%chars .ne. ["D"])) call abort
     deallocate (prt_in%chars)
     deallocate(tmpc%comp%chars)
! Check that function arguments are OK too
     tmpc = new_prt_spec2 (string_container_t(new_str_t(["h","e","l","l","o"])))
     if (any(tmpc%comp%chars .ne. ["h","e","l","l","o"])) call abort
     deallocate(tmpc%comp%chars)

  end do

contains

  impure elemental function new_prt_spec2 (name) result (prt_spec)
    type(string_container_t), intent(in) :: name
    type(string_container_t) :: prt_spec
    prt_spec = name
  end function new_prt_spec2


  function new_str_t (name) result (prt_spec)
    character (*), intent(in), dimension (:) :: name
    type(string_t) :: prt_spec
    prt_spec = string_t(name)
  end function new_str_t

end program main

Re: [Patch, fortran] PR65792 - allocation of scalar elemental function with structure constructor fails

[Steve Kargl]

On Sun, Apr 26, 2015 at 08:35:06PM +0200, Paul Richard Thomas wrote:
>
> --- 7062,7091 ----
>       {
>         if (expr->expr_type != EXPR_STRUCTURE)
>   	{
> + 	  tree dealloc = NULL_TREE;
>   	  gfc_init_se (&se, NULL);
>   	  gfc_conv_expr (&se, expr);
>   	  gfc_add_block_to_block (&block, &se.pre);
> + 	  /* Prevent repeat evaluations in gfc_copy_alloc_comp by fixing the
> + 	     expression in  a temporary variable and deallocate is allocatable
> + 	     components the copy to the result.  */

Can you take a second shot at this comment?  The "and ..." portions
seems to be a little muddled.

OK with after comment fix.

-- 
Steve

Re: [Patch, fortran] PR65792 - allocation of scalar elemental function with structure constructor fails

[Paul Richard Thomas]

Dear Steve,

Thanks for the review. I THINK that I know what I meant in the comment
:-) I will commit tomorrow night.

Cheers

Paul

On 26 April 2015 at 20:53, Steve Kargl <sgk@troutmask.apl.washington.edu> wrote:
> On Sun, Apr 26, 2015 at 08:35:06PM +0200, Paul Richard Thomas wrote:
>>
>> --- 7062,7091 ----
>>       {
>>         if (expr->expr_type != EXPR_STRUCTURE)
>>       {
>> +       tree dealloc = NULL_TREE;
>>         gfc_init_se (&se, NULL);
>>         gfc_conv_expr (&se, expr);
>>         gfc_add_block_to_block (&block, &se.pre);
>> +       /* Prevent repeat evaluations in gfc_copy_alloc_comp by fixing the
>> +          expression in  a temporary variable and deallocate is allocatable
>> +          components the copy to the result.  */
>
> Can you take a second shot at this comment?  The "and ..." portions
> seems to be a little muddled.
>
> OK with after comment fix.
>
> --
> Steve



-- 
Outside of a dog, a book is a man's best friend. Inside of a dog it's
too dark to read.

Groucho Marx

Re: [Patch, fortran] PR65792 - allocation of scalar elemental function with structure constructor fails

[Paul Richard Thomas]

Dear All,

By the time I went to commit, something had changed and the patch
caused a regression. I presume that the version that I had of Andre's
patch was not the same as the one committed. I'll cast an eye over it
this weekend and see if I can understand what gives.

Cheers

paul

On 27 April 2015 at 22:42, Paul Richard Thomas
<paul.richard.thomas@gmail.com> wrote:
> Dear Steve,
>
> Thanks for the review. I THINK that I know what I meant in the comment
> :-) I will commit tomorrow night.
>
> Cheers
>
> Paul
>
> On 26 April 2015 at 20:53, Steve Kargl <sgk@troutmask.apl.washington.edu> wrote:
>> On Sun, Apr 26, 2015 at 08:35:06PM +0200, Paul Richard Thomas wrote:
>>>
>>> --- 7062,7091 ----
>>>       {
>>>         if (expr->expr_type != EXPR_STRUCTURE)
>>>       {
>>> +       tree dealloc = NULL_TREE;
>>>         gfc_init_se (&se, NULL);
>>>         gfc_conv_expr (&se, expr);
>>>         gfc_add_block_to_block (&block, &se.pre);
>>> +       /* Prevent repeat evaluations in gfc_copy_alloc_comp by fixing the
>>> +          expression in  a temporary variable and deallocate is allocatable
>>> +          components the copy to the result.  */
>>
>> Can you take a second shot at this comment?  The "and ..." portions
>> seems to be a little muddled.
>>
>> OK with after comment fix.
>>
>> --
>> Steve
>
>
>
> --
> Outside of a dog, a book is a man's best friend. Inside of a dog it's
> too dark to read.
>
> Groucho Marx



-- 
Outside of a dog, a book is a man's best friend. Inside of a dog it's
too dark to read.

Groucho Marx

Re: [Patch, fortran] PR65792 - allocation of scalar elemental function with structure constructor fails

[Mikael Morin]

[-- Attachment #1: Type: text/plain, Size: 478 bytes --]

Le 01/05/2015 20:25, Paul Richard Thomas a écrit :
> Dear All,
> 
> By the time I went to commit, something had changed and the patch
> caused a regression. I presume that the version that I had of Andre's
> patch was not the same as the one committed. I'll cast an eye over it
> this weekend and see if I can understand what gives.
> 
Hello Paul,

to get things moving again, I propose the attached fix to your patch.
Tested on alloc_comp_deep_copy_1 only for now.

Mikael




[-- Attachment #2: pr65792_fix.diff --]
[-- Type: text/x-patch, Size: 572 bytes --]

diff --git a/gcc/fortran/trans-expr.c b/gcc/fortran/trans-expr.c
index 6cf5fe2..532f4b7 100644
--- a/gcc/fortran/trans-expr.c
+++ b/gcc/fortran/trans-expr.c
@@ -7078,7 +7078,8 @@ gfc_trans_subcomponent_assign (tree dest, gfc_component * cm, gfc_expr * expr,
 	    }
 	  gfc_add_modify (&block, dest,
 			  fold_convert (TREE_TYPE (dest), se.expr));
-	  if (cm->ts.u.derived->attr.alloc_comp)
+	  if (cm->ts.u.derived->attr.alloc_comp
+	      && expr->expr_type != EXPR_NULL)
 	    {
 	      tmp = gfc_copy_alloc_comp (cm->ts.u.derived, se.expr,
 					 dest, expr->rank);

*ping* Re: [Patch, fortran] PR65792 - allocation of scalar elemental function with structure constructor fails

[Mikael Morin]

Le 09/05/2015 15:12, Mikael Morin a écrit :
> Le 01/05/2015 20:25, Paul Richard Thomas a écrit :
>> Dear All,
>>
>> By the time I went to commit, something had changed and the patch
>> caused a regression. I presume that the version that I had of Andre's
>> patch was not the same as the one committed. I'll cast an eye over it
>> this weekend and see if I can understand what gives.
>>
> Hello Paul,
> 
> to get things moving again, I propose the attached fix to your patch.
> Tested on alloc_comp_deep_copy_1 only for now.

Hello Paul,

any news?

Mikael

Re: *ping* Re: [Patch, fortran] PR65792 - allocation of scalar elemental function with structure constructor fails

[Paul Richard Thomas]

Dear Mikael, dear all,

I am struggling to find time for gfortran at present because of the
change in my personal circumstances. I am still working full time as a
consultant, whilst trying to sell the house and fix up all those
things that l'Administration Francaise makes so "interesting" (Having
had quasi-diplomatic status, there are all sorts of issues involved in
cars, pensions and... yes, the 2014 tax return).  Things will improve
on a timescale of three months because I am expecting an acceptable
offer for the house, next week.

In the meantime, to show willing, I will attend to this right now :-)

Cheers

Paul

On 15 May 2015 at 19:01, Mikael Morin <mikael.morin@sfr.fr> wrote:
> Le 09/05/2015 15:12, Mikael Morin a écrit :
>> Le 01/05/2015 20:25, Paul Richard Thomas a écrit :
>>> Dear All,
>>>
>>> By the time I went to commit, something had changed and the patch
>>> caused a regression. I presume that the version that I had of Andre's
>>> patch was not the same as the one committed. I'll cast an eye over it
>>> this weekend and see if I can understand what gives.
>>>
>> Hello Paul,
>>
>> to get things moving again, I propose the attached fix to your patch.
>> Tested on alloc_comp_deep_copy_1 only for now.
>
> Hello Paul,
>
> any news?
>
> Mikael



-- 
Outside of a dog, a book is a man's best friend. Inside of a dog it's
too dark to read.

Groucho Marx

Re: *ping* Re: [Patch, fortran] PR65792 - allocation of scalar elemental function with structure constructor fails

[Paul Richard Thomas]

Dear Mikael,

With your tweak it bootstrapped and regtested OK. Also changed the
comment after Steve's pointing out that the last sentence was
incomprehensible:-)

Committed to trunk as revision 223234.

Cheers

Paul

2015-05-16  Mikael Morin  <mikael@gcc.gnu.org
        Paul Thomas  <pault@gcc.gnu.org>

    PR fortran/65792
    * trans-expr.c (gfc_trans_subcomponent_assign): Always assign
    the expression component to the destination. In addition, if
    the component has allocatable components, copy them and
    deallocate those of the expression, if it is not a variable.
    The expression is fixed if not a variable to prevent multiple
    evaluations.

2015-05-16  Mikael Morin  <mikael@gcc.gnu.org>

    PR fortran/65792
    * gfortran.dg/derived_constructor_components_5: New test


On 15 May 2015 at 19:01, Mikael Morin <mikael.morin@sfr.fr> wrote:
> Le 09/05/2015 15:12, Mikael Morin a écrit :
>> Le 01/05/2015 20:25, Paul Richard Thomas a écrit :
>>> Dear All,
>>>
>>> By the time I went to commit, something had changed and the patch
>>> caused a regression. I presume that the version that I had of Andre's
>>> patch was not the same as the one committed. I'll cast an eye over it
>>> this weekend and see if I can understand what gives.
>>>
>> Hello Paul,
>>
>> to get things moving again, I propose the attached fix to your patch.
>> Tested on alloc_comp_deep_copy_1 only for now.
>
> Hello Paul,
>
> any news?
>
> Mikael



-- 
Outside of a dog, a book is a man's best friend. Inside of a dog it's
too dark to read.

Groucho Marx