* 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

(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).

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? 

(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.)

(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.

(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;
 

I am not going to have time for this right now.

Paul

I guess I'll take over then. Got a patch.

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

Fixed with r150823. Closing.