diff mbox

[Fortran] PR 42647: Missed initialization/dealloc of allocatable scalar DT with allocatable component

Message ID AANLkTinDSKyehQcb76o-mOB8ADmf=uCqk_FZYrM9doKD@mail.gmail.com
State New
Headers show

Commit Message

Janus Weil Oct. 14, 2010, 2:21 p.m. UTC 
2010/10/13 Tobias Burnus <burnus@net-b.de>:
>  On 10/13/2010 02:08 PM, Janus Weil wrote:
>>
>> Self-review: Three things were still missing:
>> 1) Auto-deallocation of CLASS components.
>> 2) Auto-deallocation of "sub-components", i.e. stuff like a%b%c.
>> 3) Test cases.
>
> I have not yet had a closer look at the patch, but I think you should also
> take care of the DEALLOCATE statement. Example:
>
> type t
>  integer, allocatable :: p
> end type t
> type(t), allocatable :: a
>
> deallocate(a)
> end
>
> (Allocate left away to make dump cleaner.) The DEALLOCATE is translated into
> the following. You see many accesses to "a.p" which cause a segfault if "a"
> is not allocated - assume
>
>      if (a.p != 0B)
>        {
>          __builtin_free ((void *) a.p);
>        }
>      a.p = 0B;
>      if (a == 0B)         {
>          _gfortran_runtime_error_at
>        }       else         {
>          __builtin_free ((void *) a);
>        }
>      a = 0B;
>
> Hmm, I actually could not make the program crash - even though the dump
> looks wrong.

Yes, you're right. I need to restructure my patch a bit to make it
work also for DEALLOCATE.


> In any case the following program leaks memory (without the "if" line); with
> the "if" line it aborts as "istat" is not zero, without stat= it aborts with
> an error message stating that "a" is not allocated. Expected: No error, no
> valgrind error and istat == 0.
>
> type t
>  integer, allocatable :: p(:)
> end type t
> type(t), allocatable :: a
>
> allocate(a)
> allocate(a%p(10000))
>
> deallocate(a, stat=istat)
> if(istat /= 0) call abort()
> end

This is fixed by the following hunk:



Cheers,
Janus

Comments

Tobias Burnus Oct. 14, 2010, 8:26 p.m. UTC | #1 
Janus Weil wrote:
> 2010/10/13 Tobias Burnus<burnus@net-b.de>:
>> I have not yet had a closer look at the patch, but I think you should 
>> also take care of the DEALLOCATE statement. Example: [...]
> Yes, you're right. I need to restructure my patch a bit to make it
> work also for DEALLOCATE.
>
>> In any case the following program leaks memory (without the "if" line);
> This is fixed by the following hunk: [....]

Thanks - it fixes my test case and the one of PR 42647 comment 10 (after 
fixing the test-case bug).

Though after modifying my test a bit, it still fails. Thus, if you are 
looking for a failing test case with DEALLOCATE: The following crashes 
here with a segfault.

type t
   integer, allocatable :: p
end type t
type(t), allocatable :: a

allocate(a)
allocate(a%p)

deallocate(a, stat=istat)
if(istat /= 0) call abort()
deallocate(a, stat=istat)  ! << segfault
if(istat == 0) call abort()
end


Tobias
Janus Weil Oct. 23, 2010, 9:59 p.m. UTC | #2 
>> I have not yet had a closer look at the patch, but I think you should also
>> take care of the DEALLOCATE statement. Example:
>>
>> type t
>>  integer, allocatable :: p
>> end type t
>> type(t), allocatable :: a
>>
>> deallocate(a)
>> end
>>
>> (Allocate left away to make dump cleaner.) The DEALLOCATE is translated into
>> the following. You see many accesses to "a.p" which cause a segfault if "a"
>> is not allocated - assume
>>
>>      if (a.p != 0B)
>>        {
>>          __builtin_free ((void *) a.p);
>>        }
>>      a.p = 0B;
>>      if (a == 0B)         {
>>          _gfortran_runtime_error_at
>>        }       else         {
>>          __builtin_free ((void *) a);
>>        }
>>      a = 0B;
>>
>> Hmm, I actually could not make the program crash - even though the dump
>> looks wrong.
>
> Yes, you're right. I need to restructure my patch a bit to make it
> work also for DEALLOCATE.

After two unsuccessful attempts to refactor the patch in different
ways, I simply added a new function
'gfc_deallocate_scalar_with_status' to do the job. The attached
version of the patch fixes all of Tobias' test cases I think, plus it
is free of testsuite regressions.

I guess one should at least remove 'gfc_deallocate_scalar' and instead
use the more general 'gfc_deallocate_scalar_with_status' (I'll do that
soon, followed by another regtest).

In the meantime: Any further comments on this patch?

Cheers,
Janus
Tobias Burnus Oct. 24, 2010, 11:29 a.m. UTC | #3 
Janus Weil wrote:
> In the meantime: Any further comments on this patch?

No, I think it looks ok. (Looking at the generated code and glancing at 
the patch; I have not yet properly read the patch itself.)

  * * *

It does not solve the related issues of PR 43018 and PR 45451.

Regarding the former: For the following program, valgrind shows:
    Invalid read of size 8
       at 0x40087A: MAIN__ (alloc_comp_scalar_1.f90:8)
for the line "a1 = a2".

program main
   type :: container_t
     integer, allocatable :: entry
   end type container_t
   type(container_t) :: a2,a1
   call myAlloc()
   a2%entry = 1
   a1 = a2
contains
   subroutine myAlloc()
     allocate (a2%entry)
   end subroutine myAlloc
end program main

The dump looks quite unspectacular. Does anyone see the problem?

MAIN__ ()
{
   static struct container_t a2 = {};
   static struct container_t a1 = {};
   static void myalloc (void);

   try
     {
       a2.entry = 0B;
       a1.entry = 0B;
       myalloc ();
       *a2.entry = 1;
       {
         void * restrict D.1508;
         struct container_t D.1507;

         D.1507 = a1;
         a1 = a2;
         if ((void *) a2.entry != 0B)
           {
             D.1508 = (void * restrict) __builtin_malloc (8);
             a1.entry = (integer(kind=4) *) D.1508;
             __builtin_memcpy (a1.entry, a2.entry, 8);
           }
         else
           {
             a1.entry = 0B;
           }
         if (D.1507.entry != 0B)
           {
             __builtin_free ((void *) D.1507.entry);
           }
         D.1507.entry = 0B;
       }
     }
   finally
     {
       if (a1.entry != 0B)
         {
           __builtin_free ((void *) a1.entry);
         }
       a1.entry = 0B;
       if (a2.entry != 0B)
         {
           __builtin_free ((void *) a2.entry);
         }
       a2.entry = 0B;
     }
}

Tobias
Janus Weil Oct. 25, 2010, 1 p.m. UTC | #4 
2010/10/23 Janus Weil <janus@gcc.gnu.org>:
>>> I have not yet had a closer look at the patch, but I think you should also
>>> take care of the DEALLOCATE statement. Example:
>>>
>>> type t
>>>  integer, allocatable :: p
>>> end type t
>>> type(t), allocatable :: a
>>>
>>> deallocate(a)
>>> end
>>>
>>> (Allocate left away to make dump cleaner.) The DEALLOCATE is translated into
>>> the following. You see many accesses to "a.p" which cause a segfault if "a"
>>> is not allocated - assume
>>>
>>>      if (a.p != 0B)
>>>        {
>>>          __builtin_free ((void *) a.p);
>>>        }
>>>      a.p = 0B;
>>>      if (a == 0B)         {
>>>          _gfortran_runtime_error_at
>>>        }       else         {
>>>          __builtin_free ((void *) a);
>>>        }
>>>      a = 0B;
>>>
>>> Hmm, I actually could not make the program crash - even though the dump
>>> looks wrong.
>>
>> Yes, you're right. I need to restructure my patch a bit to make it
>> work also for DEALLOCATE.
>
> After two unsuccessful attempts to refactor the patch in different
> ways, I simply added a new function
> 'gfc_deallocate_scalar_with_status' to do the job. The attached
> version of the patch fixes all of Tobias' test cases I think, plus it
> is free of testsuite regressions.
>
> I guess one should at least remove 'gfc_deallocate_scalar' and instead
> use the more general 'gfc_deallocate_scalar_with_status' (I'll do that
> soon, followed by another regtest).

The attached version of the patch does this and is regression-free. Ok
for trunk?

Cheers,
Janus


2010-10-25  Janus Weil  <janus@gcc.gnu.org>

	PR fortran/42647
	* trans.h (gfc_deallocate_scalar_with_status): New prototype.
	* trans.c (gfc_deallocate_scalar_with_status): New function for
	deallocation of allocatable scalars.
	* trans-decl.c (gfc_trans_deferred_vars): Call it here ...
	* trans-array.c (structure_alloc_comps): ... and here.

2010-10-25  Janus Weil  <janus@gcc.gnu.org>

	PR fortran/42647
	* gfortran.dg/allocatable_scalar_9.f90: Extended.
	* gfortran.dg/allocatable_scalar_10.f90: New.
	* gfortran.dg/class_19.f03: Extended.
Tobias Burnus Oct. 26, 2010, 3:47 p.m. UTC | #5 
On 10/25/2010 03:00 PM, Janus Weil wrote:
> The attached version of the patch does this and is regression-free. Ok 
> for trunk?

OK, but you can consider to remove
+                 rank = c->as ? c->as->rank : 0;
and use "c->as->rank" directly as the code is now guarded by an if 
(attr.dimension).

Thanks for the patch!

Tobias

> 2010-10-25  Janus Weil<janus@gcc.gnu.org>
>
> 	PR fortran/42647
> 	* trans.h (gfc_deallocate_scalar_with_status): New prototype.
> 	* trans.c (gfc_deallocate_scalar_with_status): New function for
> 	deallocation of allocatable scalars.
> 	* trans-decl.c (gfc_trans_deferred_vars): Call it here ...
> 	* trans-array.c (structure_alloc_comps): ... and here.
>
> 2010-10-25  Janus Weil<janus@gcc.gnu.org>
>
> 	PR fortran/42647
> 	* gfortran.dg/allocatable_scalar_9.f90: Extended.
> 	* gfortran.dg/allocatable_scalar_10.f90: New.
> 	* gfortran.dg/class_19.f03: Extended.
Janus Weil Oct. 26, 2010, 5:42 p.m. UTC | #6 
2010/10/26 Tobias Burnus <burnus@net-b.de>:
> On 10/25/2010 03:00 PM, Janus Weil wrote:
>>
>> The attached version of the patch does this and is regression-free. Ok for
>> trunk?
>
> OK, but you can consider to remove
> +                 rank = c->as ? c->as->rank : 0;
> and use "c->as->rank" directly as the code is now guarded by an if
> (attr.dimension).

Committed with this change as r165973. Thanks for the review!

Cheers,
Janus



>> 2010-10-25  Janus Weil<janus@gcc.gnu.org>
>>
>>        PR fortran/42647
>>        * trans.h (gfc_deallocate_scalar_with_status): New prototype.
>>        * trans.c (gfc_deallocate_scalar_with_status): New function for
>>        deallocation of allocatable scalars.
>>        * trans-decl.c (gfc_trans_deferred_vars): Call it here ...
>>        * trans-array.c (structure_alloc_comps): ... and here.
>>
>> 2010-10-25  Janus Weil<janus@gcc.gnu.org>
>>
>>        PR fortran/42647
>>        * gfortran.dg/allocatable_scalar_9.f90: Extended.
>>        * gfortran.dg/allocatable_scalar_10.f90: New.
>>        * gfortran.dg/class_19.f03: Extended.
>
>
diff mbox

Patch

Index: gcc/fortran/trans-stmt.c
===================================================================
--- gcc/fortran/trans-stmt.c    (revision 165461)
+++ gcc/fortran/trans-stmt.c    (working copy)
@@ -4679,7 +4679,11 @@ 
          if (!(last && last->u.c.component->attr.pointer)
                && !(!last && expr->symtree->n.sym->attr.pointer))
            {
-             tmp = gfc_deallocate_alloc_comp (expr->ts.u.derived, se.expr,
+             if (!expr->rank)
+               tmp = build_fold_indirect_ref_loc (input_location, se.expr);
+             else
+               tmp = se.expr;
+             tmp = gfc_deallocate_alloc_comp (expr->ts.u.derived, tmp,
                                               expr->rank);
              gfc_add_expr_to_block (&se.pre, tmp);
            }