RFA: fix minor memory leak in symfile.c

RFA: fix minor memory leak in symfile.c

[Tom Tromey]

I found a small memory leak while running valgrind on gdb.

While auditing other callers of build_id_bfd_get, I found a use of
'free', so I fixed that as well.  (Perhaps we ought to poison "free"?)

Built & regtested on x86-64 (compile farm).
I also verified the leak before- and after- on x86 F8.

Please review.

thanks,
Tom

:ADDPATCH symbols:

2008-09-13  Tom Tromey  <tromey@redhat.com>

	* symfile.c (build_id_verify): Free 'found'.
	(find_separate_debug_file): Use xfree, not free.

diff --git a/gdb/symfile.c b/gdb/symfile.c
index d067d2b..ed36497 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1235,6 +1235,9 @@ build_id_verify (const char *filename, struct build_id *check)
   if (!bfd_close (abfd))
     warning (_("cannot close \"%s\": %s"), filename,
 	     bfd_errmsg (bfd_get_error ()));
+
+  xfree (found);
+
   return retval;
 }
 
@@ -1363,7 +1366,7 @@ find_separate_debug_file (struct objfile *objfile)
       char *build_id_name;
 
       build_id_name = build_id_to_debug_filename (build_id);
-      free (build_id);
+      xfree (build_id);
       /* Prevent looping on a stripped .debug file.  */
       if (build_id_name != NULL && strcmp (build_id_name, objfile->name) == 0)
         {

Re: RFA: fix minor memory leak in symfile.c

[Daniel Jacobowitz]

On Sat, Sep 13, 2008 at 10:58:39AM -0600, Tom Tromey wrote:
> 2008-09-13  Tom Tromey  <tromey@redhat.com>
> 
> 	* symfile.c (build_id_verify): Free 'found'.
> 	(find_separate_debug_file): Use xfree, not free.

:REVIEWMAIL:

OK.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFA: fix minor memory leak in symfile.c

[Joel Brobecker]

:REVIEWMAIL:

> While auditing other callers of build_id_bfd_get, I found a use of
> 'free', so I fixed that as well.  (Perhaps we ought to poison "free"?)

I think that's a good idea, since I don't think there is any case
besides the xfree implementation where we want to call free. Same
for malloc as well.  But I'm not very familiar with the pros and
cons of this GCC pragma.

> 2008-09-13  Tom Tromey  <tromey@redhat.com>
> 
> 	* symfile.c (build_id_verify): Free 'found'.
> 	(find_separate_debug_file): Use xfree, not free.

Looks good :).

-- 
Joel

Re: RFA: fix minor memory leak in symfile.c

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

>> While auditing other callers of build_id_bfd_get, I found a use of
>> 'free', so I fixed that as well.  (Perhaps we ought to poison "free"?)

Joel> I think that's a good idea, since I don't think there is any case
Joel> besides the xfree implementation where we want to call free. Same
Joel> for malloc as well.  But I'm not very familiar with the pros and
Joel> cons of this GCC pragma.

It works in the lexer, so it is fairly primitive.

I tried it and ran into a few problems.  vec.h uses the token
'free'.  struct dict_vector has a field named 'free'.  And, bison uses
'free', but IIRC we #define it to xfree somewhere.

So, I think this probably isn't worth pursuing.

I did find a few stray uses of free.  I can send that patch if you
like.  I'm not even sure if this matters -- I suppose the
justification for xfree is not as strong as that for xmalloc.

Tom

Re: RFA: fix minor memory leak in symfile.c

[Joel Brobecker]

> So, I think this probably isn't worth pursuing.

I agree.

> I did find a few stray uses of free.  I can send that patch if you
> like.  I'm not even sure if this matters -- I suppose the
> justification for xfree is not as strong as that for xmalloc.

Yes, in practice xfree doesn't do anything more than free. I looked
at the code, and it only contains a check against NULL, right now.
The C standard requires that free does nothing when passed a NULL
pointer.  So unless we're building GDB with a screwy C compiler,
it's strictly equivalent to calling free.

Still, I think it'd be nice to get rid of these. In desperate
situations, one might want to locally hack these to track memory
issues...

-- 
Joel

Re: RFA: fix minor memory leak in symfile.c

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> The C standard requires that free does nothing when passed a NULL
Joel> pointer.  So unless we're building GDB with a screwy C compiler,
Joel> it's strictly equivalent to calling free.

My understanding is that on all systems in active use, free(NULL)
works fine.  Jim Meyering recently removed if (x) free (x) from a
bunch of GNU programs...

Joel> Still, I think it'd be nice to get rid of these. In desperate
Joel> situations, one might want to locally hack these to track memory
Joel> issues...

Here's the patch.  I'm regtesting it now.

I also changed a use of asprintf, because gdbint.texinfo clearly
states that this function is not to be used.  (It is the only use in
gdb.)

Ok if it succeeds?

Note that, if you should ever actually have to do this, you should
expect to have to fix gdb first anyway.  In the absence of enforcement
I am sure that more uses of free will sneak in.

Tom

2008-09-13  Tom Tromey  <tromey@redhat.com>

	* varobj.c (varobj_set_display_format): Use xfree.
	* tracepoint.c (stringify_collection_list): Use xfree.
	* remote-fileio.c (remote_fileio_reset): Use xfree.
	* mipsread.c (read_alphacoff_dynamic_symtab): Use xfree.
	* dfp.c (decimal_from_floating): Use xfree, xstrprintf.  Don't use
	asprintf.
	* cp-support.c (mangled_name_to_comp): Use xfree.

diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index df48f60..f50d8fd 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -167,7 +167,7 @@ mangled_name_to_comp (const char *mangled_name, int options,
 
   if (ret == NULL)
     {
-      free (demangled_name);
+      xfree (demangled_name);
       return NULL;
     }
 
diff --git a/gdb/dfp.c b/gdb/dfp.c
index 9816d27..bbaf9aa 100644
--- a/gdb/dfp.c
+++ b/gdb/dfp.c
@@ -235,16 +235,12 @@ void
 decimal_from_floating (struct value *from, gdb_byte *to, int len)
 {
   char *buffer;
-  int ret;
 
-  ret = asprintf (&buffer, "%.30" DOUBLEST_PRINT_FORMAT,
-		  value_as_double (from));
-  if (ret < 0)
-    error (_("Error in memory allocation for conversion to decimal float."));
+  buffer = xstrprintf ("%.30" DOUBLEST_PRINT_FORMAT, value_as_double (from));
 
   decimal_from_string (to, len, buffer);
 
-  free (buffer);
+  xfree (buffer);
 }
 
 /* Converts a decimal float of LEN bytes to a double value.  */
diff --git a/gdb/mipsread.c b/gdb/mipsread.c
index fdd8634..1e88f81 100644
--- a/gdb/mipsread.c
+++ b/gdb/mipsread.c
@@ -217,13 +217,13 @@ read_alphacoff_dynamic_symtab (struct section_offsets *section_offsets,
   dyninfo_secsize = bfd_get_section_size (si.dyninfo_sect);
   got_secsize = bfd_get_section_size (si.got_sect);
   sym_secptr = xmalloc (sym_secsize);
-  cleanups = make_cleanup (free, sym_secptr);
+  cleanups = make_cleanup (xfree, sym_secptr);
   str_secptr = xmalloc (str_secsize);
-  make_cleanup (free, str_secptr);
+  make_cleanup (xfree, str_secptr);
   dyninfo_secptr = xmalloc (dyninfo_secsize);
-  make_cleanup (free, dyninfo_secptr);
+  make_cleanup (xfree, dyninfo_secptr);
   got_secptr = xmalloc (got_secsize);
-  make_cleanup (free, got_secptr);
+  make_cleanup (xfree, got_secptr);
 
   if (!bfd_get_section_contents (abfd, si.sym_sect, sym_secptr,
 				 (file_ptr) 0, sym_secsize))
diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
index 93665c9..05a78be 100644
--- a/gdb/remote-fileio.c
+++ b/gdb/remote-fileio.c
@@ -1398,7 +1398,7 @@ remote_fileio_reset (void)
     }
   if (remote_fio_data.fd_map)
     {
-      free (remote_fio_data.fd_map);
+      xfree (remote_fio_data.fd_map);
       remote_fio_data.fd_map = NULL;
       remote_fio_data.fd_map_size = 0;
     }
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 671a63a..99bd17f 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -1457,7 +1457,7 @@ stringify_collection_list (struct collection_list *list, char *string)
 
   if (ndx == 0)
     {
-      free (str_list);
+      xfree (str_list);
       return NULL;
     }
   else
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 3a0bf5e..12b644f 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -693,7 +693,7 @@ varobj_set_display_format (struct varobj *var,
   if (varobj_value_is_changeable_p (var) 
       && var->value && !value_lazy (var->value))
     {
-      free (var->print_value);
+      xfree (var->print_value);
       var->print_value = value_get_print_value (var->value, var->format);
     }
 

Re: RFA: fix minor memory leak in symfile.c

[Joel Brobecker]

> My understanding is that on all systems in active use, free(NULL)
> works fine.  Jim Meyering recently removed if (x) free (x) from a
> bunch of GNU programs...

We might end up doing it in GDB as well, not sure what the others think.

> I also changed a use of asprintf, because gdbint.texinfo clearly
> states that this function is not to be used.  (It is the only use in
> gdb.)

Thanks!

> 2008-09-13  Tom Tromey  <tromey@redhat.com>
> 
> 	* varobj.c (varobj_set_display_format): Use xfree.
> 	* tracepoint.c (stringify_collection_list): Use xfree.
> 	* remote-fileio.c (remote_fileio_reset): Use xfree.
> 	* mipsread.c (read_alphacoff_dynamic_symtab): Use xfree.
> 	* dfp.c (decimal_from_floating): Use xfree, xstrprintf.  Don't use
> 	asprintf.
> 	* cp-support.c (mangled_name_to_comp): Use xfree.
>
> Ok if it succeeds?

Yes. Thanks for doing this.

-- 
Joel

Re: RFA: fix minor memory leak in symfile.c

[Mark Kettenis]

> Date: Sat, 13 Sep 2008 16:03:27 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> 
> > My understanding is that on all systems in active use, free(NULL)
> > works fine.  Jim Meyering recently removed if (x) free (x) from a
> > bunch of GNU programs...
> 
> We might end up doing it in GDB as well, not sure what the others think.

I don't see why we should change our policy to use xfree(), but the
main reason why we didn't rely on free(NULL) doing nothing was SunOS
4.  There's probably no point in supporting GDB on that platform
anymore, so I think requiring a C89 libc would be reasonable.

Mark

Re: RFA: fix minor memory leak in symfile.c

[Thiago Jung Bauermann]

El sáb, 13-09-2008 a las 16:51 -0600, Tom Tromey escribió:
> I also changed a use of asprintf, because gdbint.texinfo clearly
> states that this function is not to be used.  (It is the only use in
> gdb.)

When I added that use of asprintf, I checked that libiberty provides it
if the underlying OS doesn't. I thought we could use anything covered by
libiberty. Maybe not?
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Re: RFA: fix minor memory leak in symfile.c

[Tom Tromey]

>>>>> "Thiago" == Thiago Jung Bauermann <bauerman@br.ibm.com> writes:

>> I also changed a use of asprintf, because gdbint.texinfo clearly
>> states that this function is not to be used.  (It is the only use in
>> gdb.)

Thiago> When I added that use of asprintf, I checked that libiberty provides it
Thiago> if the underlying OS doesn't. I thought we could use anything covered by
Thiago> libiberty. Maybe not?

No, I'm sure that is ok -- this just means that gdbint.texinfo is out
of date.

Tom

Re: RFA: fix minor memory leak in symfile.c

[Joel Brobecker]

> When I added that use of asprintf, I checked that libiberty provides it
> if the underlying OS doesn't. I thought we could use anything covered by
> libiberty. Maybe not?

Not sure. I don't think that it is a portability issue, but rather
to provide an interface where any issues causes an error to be thrown.
That way, no need to check the pointer returned, nor the status code,
and thus it's not possible to forget. I think it's the same as xmalloc.

-- 
Joel

Re: RFA: fix minor memory leak in symfile.c

[Daniel Jacobowitz]

On Wed, Sep 17, 2008 at 06:04:18PM -0700, Joel Brobecker wrote:
> > When I added that use of asprintf, I checked that libiberty provides it
> > if the underlying OS doesn't. I thought we could use anything covered by
> > libiberty. Maybe not?
> 
> Not sure. I don't think that it is a portability issue, but rather
> to provide an interface where any issues causes an error to be thrown.
> That way, no need to check the pointer returned, nor the status code,
> and thus it's not possible to forget. I think it's the same as xmalloc.

Yes - thus xstrprintf, I believe.

-- 
Daniel Jacobowitz
CodeSourcery