This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: Memory leak: d_growable_string_resize..cp_canonicalize_string
- From: Simon Marchi <simon dot marchi at polymtl dot ca>
- To: Alex Lindsay <alexlindsay239 at gmail dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Mon, 07 Aug 2017 17:53:29 +0200
- Subject: Re: Memory leak: d_growable_string_resize..cp_canonicalize_string
- Authentication-results: sourceware.org; auth=none
- References: <86mv7b20z2.fsf@gmail.com> <3941f94c-920d-5074-49da-5a913d1585ea@gmail.com>
On 2017-08-07 17:09, Alex Lindsay wrote:
This is my first attempt at contributing, so please be patient with
me...I've been encountering some large memory usage with gdb, so I've
been running it through valgrind.
Hi Alex,
Thanks for doing this!
For your future patches, I suggest you look into "git send-email", as it
will send the patch neatly formatted in a standard way.
Some discussion from the gdb-users
list is below. One leak occurs through this stack-trace:
==21225== 32 bytes in 1 blocks are definitely lost in loss record
6,063 of 10,949^M
==21225== at 0x4C2DB8F: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)^M
==21225== by 0x4C2FDEF: realloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)^M
==21225== by 0x76CB31: d_growable_string_resize
(cp-demangle.c:3963)^M
==21225== by 0x76CB31: d_growable_string_init (cp-demangle.c:3942)^M
==21225== by 0x76CB31: cplus_demangle_print (cp-demangle.c:4308)^M
==21225== by 0x4C9535: cp_comp_to_string(demangle_component*, int)
(cp-name-parser.y:1972)^M
==21225== by 0x53EF14: cp_canonicalize_string[abi:cxx11](char
const*) (cp-support.c:569)^M
==21225== by 0x561B75: dwarf2_canonicalize_name(char const*,
dwarf2_cu*, obstack*) [clone .isra.210] (dwarf2read.c:20159)^M
==21225== by 0x566B77: read_partial_die (dwarf2read.c:16264)
AFAICT, this occurs because we return a (char *) with
`cp_comp_to_string` and immediately copy its contents to a
std::string, never freeing the space pointed to by (char *). So my
patch for this is:
index df9a563508..f11d94f56c 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -566,7 +566,9 @@ cp_canonicalize_string (const char *string)
return std::string ();
estimated_len = strlen (string) * 2;
- std::string ret = cp_comp_to_string (info->tree, estimated_len);
+ char * ret_char = cp_comp_to_string (info->tree, estimated_len);
Remove the space after the *.
+ std::string ret = ret_char;
+ xfree (ret_char);
if (ret.empty ())
{
Let me know what else to do. In CONTRIBUTE, I read about ChangeLog
entries, so I can definitely make an addition there if deemed
appropriate.
Indeed, an appropriate ChangeLog entry for this patch could look like
this:
gdb/ChangeLog:
* cp-support.c (cp_canonicalize_string): Free return value of
cp_comp_to_string.
Note that the ChangeLog entries should describe _what_ changed and not
_why_. The why is explained is the commit message.
As for the patch itself, I think it's good in that it does fix the
problem, but there is a bit more refactoring that could be done to avoid
this kind of problem with this function in the future. Instead of
returning an allocated char*, it should probably return a
gdb::unique_xmalloc_ptr<char>. This will make sure the content gets
free'd without requiring the caller to free it explicitly. However,
doing such a change will probably have many ramifications, so if you are
not ready to do this it's perfectly fine. But if you are, feel free to
ask any questions you may have. The #gdb IRC channel is also good if
you need some more hands-on help.
Thanks,
Simon