[RFA] elfread.c (elf_symtab_read): Stop memory leak.

[RFA] elfread.c (elf_symtab_read): Stop memory leak.

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 5 bytes --]

OK?


[-- Attachment #2: elfread.txt --]
[-- Type: text/plain, Size: 1123 bytes --]

2011-03-05  Michael Snyder  <msnyder@vmware.com>

	* elfread.c (elf_symtab_read): Stop memory leak.

Index: elfread.c
===================================================================
RCS file: /cvs/src/src/gdb/elfread.c,v
retrieving revision 1.103
diff -u -p -r1.103 elfread.c
--- elfread.c	7 Jan 2011 19:36:16 -0000	1.103
+++ elfread.c	5 Mar 2011 20:33:20 -0000
@@ -242,6 +242,7 @@ elf_symtab_read (struct objfile *objfile
   char *filesymname = "";
   struct dbx_symfile_info *dbx = objfile->deprecated_sym_stab_info;
   int stripped = (bfd_get_symcount (objfile->obfd) == 0);
+  struct cleanup *back_to = make_cleanup (null_cleanup, NULL);
 
   for (i = 0; i < number_of_symbols; i++)
     {
@@ -464,6 +465,7 @@ elf_symtab_read (struct objfile *objfile
 				     * max_index));
 			  sectinfo = (struct stab_section_info *)
 			    xmalloc (size);
+			  make_cleanup (xfree, sectinfo);
 			  memset (sectinfo, 0, size);
 			  sectinfo->num_sections = max_index;
 			  if (filesym == NULL)
@@ -572,6 +574,7 @@ elf_symtab_read (struct objfile *objfile
 	    }
 	}
     }
+  do_cleanups (back_to);
 }
 
 struct build_id

Re: [RFA] elfread.c (elf_symtab_read): Stop memory leak.

[Joel Brobecker]

> 2011-03-05  Michael Snyder  <msnyder@vmware.com>
> 
> 	* elfread.c (elf_symtab_read): Stop memory leak.

I think that's OK.  A little more nervous than usual, as I had
to look through a fair bit of code.  But you did run this change
past the testsuite, right?

Note that we could possibly be using alloca to avoid the use
of heap allocation.  However, as I have learnt the hard way
in the past, it's a really bad idea to do so in a loop (one
can exhaust the stack very effectively that way).

> Index: elfread.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/elfread.c,v
> retrieving revision 1.103
> diff -u -p -r1.103 elfread.c
> --- elfread.c	7 Jan 2011 19:36:16 -0000	1.103
> +++ elfread.c	5 Mar 2011 20:33:20 -0000
> @@ -242,6 +242,7 @@ elf_symtab_read (struct objfile *objfile
>    char *filesymname = "";
>    struct dbx_symfile_info *dbx = objfile->deprecated_sym_stab_info;
>    int stripped = (bfd_get_symcount (objfile->obfd) == 0);
> +  struct cleanup *back_to = make_cleanup (null_cleanup, NULL);
>  
>    for (i = 0; i < number_of_symbols; i++)
>      {
> @@ -464,6 +465,7 @@ elf_symtab_read (struct objfile *objfile
>  				     * max_index));
>  			  sectinfo = (struct stab_section_info *)
>  			    xmalloc (size);
> +			  make_cleanup (xfree, sectinfo);
>  			  memset (sectinfo, 0, size);
>  			  sectinfo->num_sections = max_index;
>  			  if (filesym == NULL)
> @@ -572,6 +574,7 @@ elf_symtab_read (struct objfile *objfile
>  	    }
>  	}
>      }
> +  do_cleanups (back_to);
>  }
>  
>  struct build_id


-- 
Joel

Re: [RFA] elfread.c (elf_symtab_read): Stop memory leak.

[Michael Snyder]

Joel Brobecker wrote:
>> 2011-03-05  Michael Snyder  <msnyder@vmware.com>
>>
>> 	* elfread.c (elf_symtab_read): Stop memory leak.
> 
> I think that's OK.  A little more nervous than usual, as I had
> to look through a fair bit of code.  But you did run this change
> past the testsuite, right?

Not one by one, but I batch them up, yes.


> Note that we could possibly be using alloca to avoid the use
> of heap allocation.  However, as I have learnt the hard way
> in the past, it's a really bad idea to do so in a loop (one
> can exhaust the stack very effectively that way).

Thanks, committed.

> 
>> Index: elfread.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/elfread.c,v
>> retrieving revision 1.103
>> diff -u -p -r1.103 elfread.c
>> --- elfread.c	7 Jan 2011 19:36:16 -0000	1.103
>> +++ elfread.c	5 Mar 2011 20:33:20 -0000
>> @@ -242,6 +242,7 @@ elf_symtab_read (struct objfile *objfile
>>    char *filesymname = "";
>>    struct dbx_symfile_info *dbx = objfile->deprecated_sym_stab_info;
>>    int stripped = (bfd_get_symcount (objfile->obfd) == 0);
>> +  struct cleanup *back_to = make_cleanup (null_cleanup, NULL);
>>  
>>    for (i = 0; i < number_of_symbols; i++)
>>      {
>> @@ -464,6 +465,7 @@ elf_symtab_read (struct objfile *objfile
>>  				     * max_index));
>>  			  sectinfo = (struct stab_section_info *)
>>  			    xmalloc (size);
>> +			  make_cleanup (xfree, sectinfo);
>>  			  memset (sectinfo, 0, size);
>>  			  sectinfo->num_sections = max_index;
>>  			  if (filesym == NULL)
>> @@ -572,6 +574,7 @@ elf_symtab_read (struct objfile *objfile
>>  	    }
>>  	}
>>      }
>> +  do_cleanups (back_to);
>>  }
>>  
>>  struct build_id
> 
> 

Re: [RFA] elfread.c (elf_symtab_read): Stop memory leak.

[Joel Brobecker]

> > 2011-03-05  Michael Snyder  <msnyder@vmware.com>
> > 
> > 	* elfread.c (elf_symtab_read): Stop memory leak.
> 
> I think that's OK.  A little more nervous than usual, as I had
> to look through a fair bit of code.  But you did run this change
> past the testsuite, right?

I'm going to revert this patch, because I don't think it's right.
It's causing GDB to crash while loading symbols from ld.so on
sparc-solaris. Reviewing the patch again, I can't understand what
I was thinking when I reviewed it, because the data is referenced
by the objfile, thusly:

  struct dbx_symfile_info *dbx = objfile->deprecated_sym_stab_info;
  [...]
          if (sectinfo != NULL)
            {
              sectinfo->next = dbx->stab_section_info;
              dbx->stab_section_info = sectinfo;
              sectinfo = NULL;
            }

There may very well be a memory leak, but it's a lot less contained
than Michael probably thought. I don't know the code well enough
to really be sure how we'd be leaking, and risk a fix.  Perhaps
the sectinfo list should be released in:

    /* Perform any local cleanups required when we are done with a particular
       objfile.  I.E, we are in the process of discarding all symbol information
       for an objfile, freeing up all memory held for it, and unlinking the
       objfile struct from the global list of known objfiles.  */
    
    static void
    elf_symfile_finish (struct objfile *objfile)
    {
      if (objfile->deprecated_sym_stab_info != NULL)
        {
          xfree (objfile->deprecated_sym_stab_info);
        }
    
      dwarf2_free_objfile (objfile);
    }

A call to free_elfinfo before the call to xfree might do:

    /* This cleans up the objfile's deprecated_sym_stab_info pointer, and
       the chain of stab_section_info's, that might be dangling from
       it.  */
    
    static void
    free_elfinfo (void *objp)
    {

-- 
Joel

Re: [RFA] elfread.c (elf_symtab_read): Stop memory leak.

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 417 bytes --]

> > > 2011-03-05  Michael Snyder  <msnyder@vmware.com>
> > > 
> > > 	* elfread.c (elf_symtab_read): Stop memory leak.
> > 
> > I think that's OK.  A little more nervous than usual, as I had
> > to look through a fair bit of code.  But you did run this change
> > past the testsuite, right?
> 
> I'm going to revert this patch, because I don't think it's right.

This is what I applied on HEAD and branch...

-- 
Joel

[-- Attachment #2: elfread.diff --]
[-- Type: text/x-diff, Size: 1543 bytes --]

commit 86a08e4d3dcf7c0ff728083ae2b02f83ef836a09
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Tue May 3 17:55:06 2011 +0200

    Revert "elfread.c (elf_symtab_read): Stop memory leak"
    
    It turns out that this change is not correct, and it causes a crash
    on sparc-solaris while trying to load ld.so.  This is because the
    memory is actually still referenced after elf_symtab_read completes.
    
    gdb/ChangeLog:
    
            * elfread.c (elf_symtab_read): Revert the previous change
            that tried to stop a memory leak.

diff --git a/gdb/elfread.c b/gdb/elfread.c
index b9cfa13..f36c93b 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -257,7 +257,6 @@ elf_symtab_read (struct objfile *objfile, int type,
   char *filesymname = "";
   struct dbx_symfile_info *dbx = objfile->deprecated_sym_stab_info;
   int stripped = (bfd_get_symcount (objfile->obfd) == 0);
-  struct cleanup *back_to = make_cleanup (null_cleanup, NULL);
 
   for (i = 0; i < number_of_symbols; i++)
     {
@@ -482,7 +481,6 @@ elf_symtab_read (struct objfile *objfile, int type,
 				  + (sizeof (CORE_ADDR) * max_index));
 			  sectinfo = (struct stab_section_info *)
 			    xmalloc (size);
-			  make_cleanup (xfree, sectinfo);
 			  memset (sectinfo, 0, size);
 			  sectinfo->num_sections = max_index;
 			  if (filesym == NULL)
@@ -591,7 +589,6 @@ elf_symtab_read (struct objfile *objfile, int type,
 	    }
 	}
     }
-  do_cleanups (back_to);
 }
 
 /* Build minimal symbols named `function@got.plt' (see SYMBOL_GOT_PLT_SUFFIX)