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