Patchwork [Fortran] Plug memory leaks; fix tree-check ICE for PR

login
register
mail settings
Submitter Tobias Burnus
Date Aug. 27, 2012, 12:14 p.m.
Message ID <503B649E.5040509@net-b.de>
Download mbox | patch
Permalink /patch/180187/
State New
Headers show

Comments

Tobias Burnus - Aug. 27, 2012, 12:14 p.m.
On 08/26/2012 08:12 PM, Tobias Burnus wrote:
> This patch fixes one ICE and several memory leaks. But there are more.
>

I have now committed the patch with the following additional patch
         * module.c (mio_symbol): Don't increase sym->refs for its
         use in sym->formal_ns->proc_name.

The patch for the ICE (PR54370) was committed as Rev. 190709, the rest 
(memory leakage, including PR41093)  as Rev. 190710.

The additional patch is required as otherwise sym->refs is too high for 
gfc_free_symbol - and gfc_free_namespace doesn't free (and thus 
decrement) ns->proc_name.


I have regtested and tested the patch with a couple of programs, I also 
checked some with valgrind; the leakage decreased quit a bit but there 
are still known leaks, cf. http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54384

I do hope that the patch didn't cause any regression, but I do not rule 
out problems in some special situations as the memory handling is not 
always that transparent.

Tobias
Mikael Morin - Aug. 27, 2012, 1:19 p.m.
On 27/08/2012 14:14, Tobias Burnus wrote:
> On 08/26/2012 08:12 PM, Tobias Burnus wrote:
>> This patch fixes one ICE and several memory leaks. But there are more.
>>
> 
> I have now committed the patch with the following additional patch
>         * module.c (mio_symbol): Don't increase sym->refs for its
>         use in sym->formal_ns->proc_name.
> 
> The patch for the ICE (PR54370) was committed as Rev. 190709, the rest
> (memory leakage, including PR41093)  as Rev. 190710.
> 
> The additional patch is required as otherwise sym->refs is too high for
> gfc_free_symbol - and gfc_free_namespace doesn't free (and thus
> decrement) ns->proc_name.
> 
> 
> I have regtested and tested the patch with a couple of programs, I also
> checked some with valgrind; the leakage decreased quit a bit but there
> are still known leaks, cf.
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54384
> 
> I do hope that the patch didn't cause any regression, but I do not rule
> out problems in some special situations as the memory handling is not
> always that transparent.
> 
> Tobias
> 
> 
> --- a/gcc/fortran/module.c
> +++ b/gcc/fortran/module.c
> @@ -3807,10 +3807,7 @@ mio_symbol (gfc_symbol *sym)
>      {
>        mio_namespace_ref (&sym->formal_ns);
>        if (sym->formal_ns)
> -       {
> -         sym->formal_ns->proc_name = sym;
> -         sym->refs++;
> -       }
> +       sym->formal_ns->proc_name = sym;
>      }
> 
>    /* Save/restore common block links.  */

Hello,

as you seem to be very much into memory issues, could you add comments
in gfortran.h telling which pointers account for reference counting?
As far as I remember for symbols, there are:
  gfc_symtree::n::sym;
  gfc_namespace::proc_name;

and for namespaces, there is:
  gfc_symbol::formal_ns;
what else? does it make sense at all to have it reference counted?

***

In the old patch at http://gcc.gnu.org/bugzilla/attachment.cgi?id=21016
I was using helper functions to do refcount updates, like below (for
namespaces, there was the same for symbols). They handled correctly rhs
== NULL and lhs == rhs, and took care of freeing the lhs (if necessary).
You could use something alike.

void
gfc_set_ns_ptr (gfc_namespace **dest, gfc_namespace *src)
{
  gfc_namespace *ns;

  if (src)
    src->refs++;

  ns = *dest;
  *dest = src;
  gfc_free_namespace (ns);
}

Mikael

Patch

--- a/gcc/fortran/module.c
+++ b/gcc/fortran/module.c
@@ -3807,10 +3807,7 @@  mio_symbol (gfc_symbol *sym)
      {
        mio_namespace_ref (&sym->formal_ns);
        if (sym->formal_ns)
-       {
-         sym->formal_ns->proc_name = sym;
-         sym->refs++;
-       }
+       sym->formal_ns->proc_name = sym;
      }

    /* Save/restore common block links.  */