Message ID | AANLkTinDSKyehQcb76o-mOB8ADmf=uCqk_FZYrM9doKD@mail.gmail.com |
---|---|
State | New |
Headers | show |
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
>> 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
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
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.
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.
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. > >
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); }