[Bug fortran/40877] New: memory leaks with gfc_charlen?

[Bug fortran/40877] New: memory leaks with gfc_charlen?

[janus at gcc dot gnu dot org]

This PR is a reminder to check some places where a new gfc_charlen is allocated
without properly setting it into a namelist (ns->cl_list). Each of them may
potentially produce a memory leak (though I'm not completely sure):

 * expr.c (simplify_const_ref)
 * symbol.c (gfc_set_default_type, generate_isocbinding_symbol)
 * trans-decl.c (create_function_arglist)

I noticed them while working on PR40822, where I also introduced the function
gfc_new_charlen, which should be used instead of gfc_get_charlen to avoid
memory leaks.


-- 
           Summary: memory leaks with gfc_charlen?
           Product: gcc
           Version: 4.5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: fortran
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: janus at gcc dot gnu dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40877

[Bug fortran/40877] memory leaks with gfc_charlen?

[pault at gcc dot gnu dot org]

------- Comment #1 from pault at gcc dot gnu dot org  2009-08-05 08:57 -------
 * expr.c (simplify_const_ref)
 * symbol.c (gfc_set_default_type, generate_isocbinding_symbol)

These two produce leaks.

 * trans-decl.c (create_function_arglist)

This is OK - the new cl is threaded into the list.

Thanks for spotting that.

Cheers

Paul


-- 

pault at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |pault at gcc dot gnu dot org
                   |dot org                     |
             Status|UNCONFIRMED                 |ASSIGNED
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2009-08-05 08:57:08
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40877

[Bug fortran/40877] memory leaks with gfc_charlen?

[janus at gcc dot gnu dot org]

------- Comment #2 from janus at gcc dot gnu dot org  2009-08-05 11:27 -------
(In reply to comment #1)
>  * trans-decl.c (create_function_arglist)
> 
> This is OK - the new cl is threaded into the list.

Actually I think that this is not done properly. The code in question is:

              gfc_charlen *cl, *cl2;
              cl = f->sym->ts.cl;
              f->sym->ts.cl = gfc_get_charlen();
              f->sym->ts.cl->length = cl->length;
              f->sym->ts.cl->backend_decl = cl->backend_decl;
              f->sym->ts.cl->length_from_typespec = cl->length_from_typespec;
              f->sym->ts.cl->resolved = cl->resolved;
              cl2 = f->sym->ts.cl->next;
              f->sym->ts.cl->next = cl;
              cl->next = cl2;

First problem: cl2 is always NULL, since f->sym->ts.cl gets assigned a fresh
gfc_charlen.

Second problem: The new charlen is inserted in front of the old one, and not
behind it.

Let's say before this code is executed we have a linked list of charlen
structures like this:

something -> cl -> ...

After the code is executed this will look like:

something---
           |
new_cl -> cl -> NULL

where 'something' still points to 'cl', so 'new_cl' is not reachable from
within the linked list (and also cl->next is lost).


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40877

[Bug fortran/40877] memory leaks with gfc_charlen?

[janus at gcc dot gnu dot org]

------- Comment #3 from janus at gcc dot gnu dot org  2009-08-05 11:44 -------
The code from trans-decl.c (create_function_arglist) cited in comment #2 was
committed by Tobias as r148517 just a few weeks ago, as a fix for PR 40383.

Tobias, do you remember any of your thoughts when writing this? 


-- 

janus at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |burnus at gcc dot gnu dot
                   |                            |org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40877

[Bug fortran/40877] memory leaks with gfc_charlen?

[burnus at gcc dot gnu dot org]

------- Comment #4 from burnus at gcc dot gnu dot org  2009-08-05 12:50 -------
(In reply to comment #3)
> Tobias, do you remember any of your thoughts when writing this? 

Well, they are essentially written in the patch email (linked from the PR
40383):
   http://gcc.gnu.org/ml/gcc-patches/2009-06/msg01147.html

The test case was:
  subroutine sub(a,b)
     character(4), optional :: a, b  ! Known compile-time length
which translates into:
  sub (a, b, a_, b_)
where "a_" and "b_" are not used in the function (as the character length is
known) except for the bounds checking. In that case one needs to access the
TREE (or backend declaration) for "a_" and for "b_".

However, as "a" and "b" are the same, they share the same "cl" - and the same
backend type declaration (which is fine). Before Daniel's bounds check, the
"a_" and "b_" TREEs were generated but not anywhere saved, as they are not
needed. Daniel's patch simply added a pointer to a TREE to "cl" which points at
that hidden argument. As there is only one "cl" shared by both "a" and "b", one
could only access "b_" - that way one would effectively only check one of the
arguments (incomplete check). However, if optional arguments are involved, that
argument might be missing and "b_" is 0, which turns it into a wrong diagnostic
for "a".

One solution would be to create two "cl" - one for "a" and one for "b".
However, that would also create two type declaration TREEs even though "a" and
"b" have exactly the same type. Already at present the middle end has problems
with different TREE types for variables which have the same Fortran type. 
(Maybe I am wrong and we fixed this meanwhile and gfortran re-uses the TREE,
but I do not think so.)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40877

[Bug fortran/40877] memory leaks with gfc_charlen?

[janus at gcc dot gnu dot org]

------- Comment #5 from janus at gcc dot gnu dot org  2009-08-05 13:21 -------
(In reply to comment #4)
> (In reply to comment #3)
> > Tobias, do you remember any of your thoughts when writing this? 
> 
> Well, they are essentially written in the patch email (linked from the PR

Yeah, ok, the general idea sounds fine. What I meant was that the way in which
you copy the cl seems to create a memory leak (cf. comment #2), since the newly
created cl is not reachable from any ns->cl_list.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40877

[Bug fortran/40877] memory leaks with gfc_charlen?

[janus at gcc dot gnu dot org]

------- Comment #6 from janus at gcc dot gnu dot org  2009-08-05 14:50 -------
(In reply to comment #2)
> >  * trans-decl.c (create_function_arglist)
> > 
> > This is OK - the new cl is threaded into the list.
> 
> Actually I think that this is not done properly.

The following should fix it:

Index: gcc/fortran/trans-decl.c
===================================================================
--- gcc/fortran/trans-decl.c    (revision 150482)
+++ gcc/fortran/trans-decl.c    (working copy)
@@ -1796,16 +1796,15 @@ create_function_arglist (gfc_symbol * sy
              /* This can happen if the same type is used for multiple
                 arguments. We need to copy cl as otherwise
                 cl->passed_length gets overwritten.  */
-             gfc_charlen *cl, *cl2;
+             gfc_charlen *cl;
              cl = f->sym->ts.cl;
              f->sym->ts.cl = gfc_get_charlen();
              f->sym->ts.cl->length = cl->length;
              f->sym->ts.cl->backend_decl = cl->backend_decl;
              f->sym->ts.cl->length_from_typespec = cl->length_from_typespec;
              f->sym->ts.cl->resolved = cl->resolved;
-             cl2 = f->sym->ts.cl->next;
-             f->sym->ts.cl->next = cl;
-              cl->next = cl2;
+             f->sym->ts.cl->next = cl->next;
+              cl->next = f->sym->ts.cl;
             }
          f->sym->ts.cl->passed_length = length;



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40877

[Bug fortran/40877] memory leaks with gfc_charlen?

[pault at gcc dot gnu dot org]

------- Comment #7 from pault at gcc dot gnu dot org  2009-08-05 15:00 -------
I am not going to have time for this right now.

Paul


-- 

pault at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|pault at gcc dot gnu dot org|unassigned at gcc dot gnu
                   |                            |dot org
             Status|ASSIGNED                    |NEW


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40877

[Bug fortran/40877] memory leaks with gfc_charlen?

[janus at gcc dot gnu dot org]

------- Comment #8 from janus at gcc dot gnu dot org  2009-08-05 20:25 -------
I guess I'll take over then. Got a patch.


-- 

janus at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |janus at gcc dot gnu dot org
                   |dot org                     |
             Status|NEW                         |ASSIGNED
   Last reconfirmed|2009-08-05 08:57:08         |2009-08-05 20:25:46
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40877

[Bug fortran/40877] memory leaks with gfc_charlen?

[janus at gcc dot gnu dot org]

------- Comment #9 from janus at gcc dot gnu dot org  2009-08-17 09:11 -------
Subject: Bug 40877

Author: janus
Date: Mon Aug 17 09:11:00 2009
New Revision: 150823

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=150823
Log:
2009-08-17  Janus Weil  <janus@gcc.gnu.org>

        PR fortran/40877
        * array.c (gfc_resolve_character_array_constructor): Add NULL argument
        to gfc_new_charlen.
        * decl.c (add_init_expr_to_sym,variable_decl,match_char_spec,
        gfc_match_implicit): Ditto.
        * expr.c (simplify_const_ref): Fix memory leak.
        (gfc_simplify_expr): Add NULL argument to gfc_new_charlen.
        * gfortran.h (gfc_new_charlen): Modified prototype.
        * iresolve.c (check_charlen_present,gfc_resolve_char_achar): Add NULL
        argument to gfc_new_charlen.
        * module.c (mio_charlen): Ditto.
        * resolve.c (gfc_resolve_substring_charlen,
        gfc_resolve_character_operator,fixup_charlen): Ditto.
        (resolve_fl_derived,resolve_symbol): Add argument to gfc_charlen.
        * symbol.c (gfc_new_charlen): Add argument 'old_cl' (to make a copy of
        an existing charlen).
        (gfc_set_default_type,generate_isocbinding_symbol): Fix memory leak.
        (gfc_copy_formal_args_intr): Add NULL argument to gfc_new_charlen.
        * trans-decl.c (create_function_arglist): Fix memory leak.


Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/array.c
    trunk/gcc/fortran/decl.c
    trunk/gcc/fortran/expr.c
    trunk/gcc/fortran/gfortran.h
    trunk/gcc/fortran/iresolve.c
    trunk/gcc/fortran/module.c
    trunk/gcc/fortran/resolve.c
    trunk/gcc/fortran/symbol.c
    trunk/gcc/fortran/trans-decl.c


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40877

[Bug fortran/40877] memory leaks with gfc_charlen?

[janus at gcc dot gnu dot org]

------- Comment #10 from janus at gcc dot gnu dot org  2009-08-17 09:14 -------
Fixed with r150823. Closing.


-- 

janus at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40877