Patchwork [Fotran] Fix PR 4755 - Do not free + reallocate a variable that is already allocated.

login
register
mail settings
Submitter Daniel Carrera
Date July 22, 2011, 6:54 p.m.
Message ID <4E29C76C.1070409@gmail.com>
Download mbox | patch
Permalink /patch/106378/
State New
Headers show

Comments

Daniel Carrera - July 22, 2011, 6:54 p.m.
Hello,

This is a simple patch that fixes PR 4755. Currently the ALLOCATE 
statement will free and re-allocate an already allocated scalar. The 
Fortran standard says that this is an error. The attached patch fixes 
the problem.

I am also attaching two tree dumps of the same program, compiled before 
and after the application of this patch. The test program is this:

program test
     integer, allocatable :: A(:), B[:]
     integer :: stat
     character(len=33) :: str
     allocate(A(1), B[*], stat=stat)
end program


If you compare the "before" and "after" files you'll see that with this 
patch GCC no longer tries to free and reallocate A(1). You will also 
notice that the block for B[*] didn't change. The block for B[*] is 
already correct in trunk and doesn't need changing.

Here is my ChangeLog:


2011-07-22  Daniel Carrera  <dcarrera@gmail.com>

	* trans.c (gfc__allocate_allocatable): [PR 4755] Do not fix
	and the reallocate a variable that is already allocated.
Daniel Carrera - July 22, 2011, 6:55 p.m.
Ooops... error in my ChangeLog:  "fix" -> "free"

2011-07-22  Daniel Carrera  <dcarrera@gmail.com>

	* trans.c (gfc__allocate_allocatable): [PR 4755] Do not free
	and the reallocate a variable that is already allocated.

On 07/22/2011 08:54 PM, Daniel Carrera wrote:
> Hello,
>
> This is a simple patch that fixes PR 4755. Currently the ALLOCATE
> statement will free and re-allocate an already allocated scalar. The
> Fortran standard says that this is an error. The attached patch fixes
> the problem.
>
> I am also attaching two tree dumps of the same program, compiled before
> and after the application of this patch. The test program is this:
>
> program test
> integer, allocatable :: A(:), B[:]
> integer :: stat
> character(len=33) :: str
> allocate(A(1), B[*], stat=stat)
> end program
>
>
> If you compare the "before" and "after" files you'll see that with this
> patch GCC no longer tries to free and reallocate A(1). You will also
> notice that the block for B[*] didn't change. The block for B[*] is
> already correct in trunk and doesn't need changing.
>
> Here is my ChangeLog:
>
>
> 2011-07-22 Daniel Carrera <dcarrera@gmail.com>
>
> * trans.c (gfc__allocate_allocatable): [PR 4755] Do not fix
> and the reallocate a variable that is already allocated.
Tobias Burnus - July 22, 2011, 8:09 p.m.
Hi Daniel,

Daniel Carrera:
> This is a simple patch that fixes PR 4755.

It's PR 49755 - the 9 is missing in the number ...

> 2011-07-22  Daniel Carrera <dcarrera@gmail.com>
>
>     * trans.c (gfc__allocate_allocatable): [PR 4755] Do not fix
>     and the reallocate a variable that is already allocated.

Instead of "[PR 4755]" use: <tab>PR <component>/<number>. Namely:

> 2011-07-22  Daniel Carrera <dcarrera@gmail.com>
>
>     PR fortran/49755
>     * trans.c (gfc__allocate_allocatable): [PR 4755] Do not free
>     and the reallocate a variable that is already allocated.

Please also include the test case from the PR in the patch - with added 
"! { dg-do run }" and also containing "! PR fortran/49755".

However, the bug is not fully fixed. If you try the testcase in the 
bugreport, you will see that it still fails - the stat= is correctly 
set, but the array shape is wrong.

The problem is that the "a.dim" setting should be done after allocation 
in the inner loop - also the a.offset is misplaced. If possible, one 
should also move the "D.1585" into the inner "else" block, which would 
save one from using the following code in case the memory is already 
allocated:

                   D.1585 = a.data;
                   ...
               a.data = D.1585;

The promised dump, cf. testcase in the PR.

         a.dim[0].lbound = 1;
         a.dim[0].ubound = 5;
         a.dim[0].stride = 1;
         a.dim[1].lbound = 1;
         a.dim[1].ubound = 5;
         a.dim[1].stride = 5;
         overflow.4 = 0;
         if ((logical(kind=4)) __builtin_expect ((integer(kind=8)) 
(overflow.4 != 0), 0))
             stat.3 = 5014;
         else
           {
               void * restrict D.1585;

               if ((logical(kind=4)) __builtin_expect ((integer(kind=8)) 
(a.data != 0B), 0))
                 {
                   D.1585 = a.data;
                   stat.3 = 5014;
                 }
              else
                ....
               a.data = D.1585;
             }
           }
         a.offset = -6;
         if (stat.3 != 0) goto L.3;
         L.3:

Tobias
Daniel Carrera - July 23, 2011, 8:47 a.m.
Hi Tobias,

Just a quick update so you know what I'm doing.

On 07/22/2011 10:09 PM, Tobias Burnus wrote:
> However, the bug is not fully fixed. If you try the testcase in the
> bugreport, you will see that it still fails - the stat= is correctly
> set, but the array shape is wrong.

Working on it...


> The problem is that the "a.dim" setting should be done after allocation
> in the inner loop - also the a.offset is misplaced. If possible, one
> should also move the "D.1585" into the inner "else" block, which would
> save one from using the following code in case the memory is already
> allocated:
>
> D.1585 = a.data;
> ...
> a.data = D.1585;

I'm working on this right now. I see a few instances of code like this 
in the output tree. I think there is an opportunity to make the output 
tree much cleaner and maybe also make the GCC internals cleaner.

I hope to have something to show you soon-ish.

Daniel.

Patch

diff -r c8b6eb02738a gcc/fortran/trans.c
--- a/gcc/fortran/trans.c	Fri Jul 22 09:21:49 2011 +0000
+++ b/gcc/fortran/trans.c	Fri Jul 22 20:36:23 2011 +0200
@@ -775,13 +775,8 @@  gfc_allocate_allocatable (stmtblock_t * 
       stmtblock_t set_status_block;
 
       gfc_start_block (&set_status_block);
-      tmp = build_call_expr_loc (input_location,
-			     built_in_decls[BUILT_IN_FREE], 1,
-			     fold_convert (pvoid_type_node, mem));
-      gfc_add_expr_to_block (&set_status_block, tmp);
 
-      tmp = gfc_allocate_using_malloc (&set_status_block, size, status);
-      gfc_add_modify (&set_status_block, res, fold_convert (type, tmp));
+      gfc_add_modify (&set_status_block, res, fold_convert (type, mem));
 
       gfc_add_modify (&set_status_block, status,
 		      build_int_cst (status_type, LIBERROR_ALLOCATION));