[RFA] Fix leaks in macro definitions.
Commit Message
Valgrind detects leaks like the following (gdb.base/macsp.exp).
This patch fixes the 3 leaks.
Tested on debian/amd64, natively and under valgrind.
==22285== 64 (48 direct, 16 indirect) bytes in 1 blocks are definitely lost in loss record 737 of 3,377
==22285== at 0x4C2BE6D: malloc (vg_replace_malloc.c:309)
==22285== by 0x4049E7: xmalloc (common-utils.c:44)
==22285== by 0x533A20: new_macro_key(macro_table*, char const*, macro_source_file*, int) (macrotab.c:355)
==22285== by 0x53438B: macro_define_function(macro_source_file*, int, char const*, int, char const**, char const*) (macrotab.c:822)
==22285== by 0x52F945: macro_define_command(char const*, int) (macrocmd.c:409)
...
==22285== 128 (96 direct, 32 indirect) bytes in 2 blocks are definitely lost in loss record 1,083 of 3,377
==22285== at 0x4C2BE6D: malloc (vg_replace_malloc.c:309)
==22285== by 0x4049E7: xmalloc (common-utils.c:44)
==22285== by 0x533A20: new_macro_key(macro_table*, char const*, macro_source_file*, int) (macrotab.c:355)
==22285== by 0x534277: macro_define_object_internal(macro_source_file*, int, char const*, char const*, macro_special_kind) (macrotab.c:776)
==22285== by 0x52F7E0: macro_define_command(char const*, int) (macrocmd.c:414)
...
==22285== 177 bytes in 19 blocks are definitely lost in loss record 1,193 of 3,377
==22285== at 0x4C2BE6D: malloc (vg_replace_malloc.c:309)
==22285== by 0x4049E7: xmalloc (common-utils.c:44)
==22285== by 0x52F5BD: extract_identifier(char const**, int) (macrocmd.c:316)
==22285== by 0x52F77D: macro_define_command(char const*, int) (macrocmd.c:355)
gdb/ChangeLog
2019-01-15 Philippe Waroquiers <philippe.waroquiers@skynet.be>
* macrocmd.c (macro_define_command): Use a unique_xmalloc_ptr
to maintain name ownership.
* macrotab.c (macro_define_internal): New function that
factorizes macro_define_object_internal and macro_define_function
code. If the newly inserted node has replaced a existing node,
call macro_tree_delete_key to free the unused just allocated key.
(macro_define_object_internal): Use macro_define_internal.
(macro_define_function): Likewise.
(macro_undef): Free the key of the removed node.
---
gdb/macrocmd.c | 10 +++----
gdb/macrotab.c | 75 +++++++++++++++++++++++++++++---------------------
2 files changed, 48 insertions(+), 37 deletions(-)
Comments
On 2019-01-15 00:56, Philippe Waroquiers wrote:
> Valgrind detects leaks like the following (gdb.base/macsp.exp).
> This patch fixes the 3 leaks.
> Tested on debian/amd64, natively and under valgrind.
>
> ==22285== 64 (48 direct, 16 indirect) bytes in 1 blocks are definitely
> lost in loss record 737 of 3,377
> ==22285== at 0x4C2BE6D: malloc (vg_replace_malloc.c:309)
> ==22285== by 0x4049E7: xmalloc (common-utils.c:44)
> ==22285== by 0x533A20: new_macro_key(macro_table*, char const*,
> macro_source_file*, int) (macrotab.c:355)
> ==22285== by 0x53438B: macro_define_function(macro_source_file*,
> int, char const*, int, char const**, char const*) (macrotab.c:822)
> ==22285== by 0x52F945: macro_define_command(char const*, int)
> (macrocmd.c:409)
> ...
> ==22285== 128 (96 direct, 32 indirect) bytes in 2 blocks are
> definitely lost in loss record 1,083 of 3,377
> ==22285== at 0x4C2BE6D: malloc (vg_replace_malloc.c:309)
> ==22285== by 0x4049E7: xmalloc (common-utils.c:44)
> ==22285== by 0x533A20: new_macro_key(macro_table*, char const*,
> macro_source_file*, int) (macrotab.c:355)
> ==22285== by 0x534277:
> macro_define_object_internal(macro_source_file*, int, char const*,
> char const*, macro_special_kind) (macrotab.c:776)
> ==22285== by 0x52F7E0: macro_define_command(char const*, int)
> (macrocmd.c:414)
> ...
> ==22285== 177 bytes in 19 blocks are definitely lost in loss record
> 1,193 of 3,377
> ==22285== at 0x4C2BE6D: malloc (vg_replace_malloc.c:309)
> ==22285== by 0x4049E7: xmalloc (common-utils.c:44)
> ==22285== by 0x52F5BD: extract_identifier(char const**, int)
> (macrocmd.c:316)
> ==22285== by 0x52F77D: macro_define_command(char const*, int)
> (macrocmd.c:355)
>
> gdb/ChangeLog
> 2019-01-15 Philippe Waroquiers <philippe.waroquiers@skynet.be>
>
> * macrocmd.c (macro_define_command): Use a unique_xmalloc_ptr
> to maintain name ownership.
> * macrotab.c (macro_define_internal): New function that
> factorizes macro_define_object_internal and macro_define_function
> code. If the newly inserted node has replaced a existing node,
> call macro_tree_delete_key to free the unused just allocated key.
> (macro_define_object_internal): Use macro_define_internal.
> (macro_define_function): Likewise.
> (macro_undef): Free the key of the removed node.
> ---
> gdb/macrocmd.c | 10 +++----
> gdb/macrotab.c | 75 +++++++++++++++++++++++++++++---------------------
> 2 files changed, 48 insertions(+), 37 deletions(-)
>
> diff --git a/gdb/macrocmd.c b/gdb/macrocmd.c
> index 706a8353e2..c1c42fa910 100644
> --- a/gdb/macrocmd.c
> +++ b/gdb/macrocmd.c
> @@ -346,14 +346,13 @@ static void
> macro_define_command (const char *exp, int from_tty)
> {
> temporary_macro_definition new_macro;
> - char *name = NULL;
>
> if (!exp)
> error (_("usage: macro define NAME[(ARGUMENT-LIST)]
> [REPLACEMENT-LIST]"));
>
> skip_ws (&exp);
> - name = extract_identifier (&exp, 0);
> - if (! name)
> + gdb::unique_xmalloc_ptr<char> name (extract_identifier (&exp, 0));
> + if (name.get () == NULL)
Nit, you don't have to use ".get ()" here.
I think extract_identifier should return a gdb::unique_xmalloc_ptr.
This call site lower:
argv[new_macro.argc] = extract_identifier (&exp, 1);
can then use ".release ()".
> @@ -774,8 +780,28 @@ macro_define_object_internal (struct
> macro_source_file *source, int line,
> return;
>
> k = new_macro_key (t, name, source, line);
> - d = new_macro_definition (t, macro_object_like, kind, 0,
> replacement);
> - splay_tree_insert (t->definitions, (splay_tree_key) k,
> (splay_tree_value) d);
> + d = new_macro_definition (t, kind, argc, argv, replacement);
> + s = splay_tree_insert (t->definitions, (splay_tree_key) k,
> (splay_tree_value) d);
> + if ((struct macro_key *) s->key != k)
> + {
> + /* If the node inserted is not using k, it means we have a
> redefinition.
> + We must free the newly allocated k, as it will not be stored in
> + the splay tree. */
> + macro_tree_delete_key (k);
> + }
Would you consider that a bug in the splay tree implementation? Should
it use the new key and free the old one with the key deallocation
function?
> @@ -841,8 +850,10 @@ macro_undef (struct macro_source_file *source, int
> line,
> arguments like '-DFOO -UFOO -DFOO=2'. */
> if (source == key->start_file
> && line == key->start_line)
> - splay_tree_remove (source->table->definitions, n->key);
> -
> + {
> + splay_tree_remove (source->table->definitions, n->key);
> + macro_tree_delete_key (key);
> + }
> else
> {
> /* This function is the only place a macro's end-of-scope
Same here, should it be the splay tree code that deletes the key?
Simon
>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
Simon> Would you consider that a bug in the splay tree implementation?
Simon> Should it use the new key and free the old one with the key
Simon> deallocation function?
Yes, I would think so as well, given that we pass key- and
value-deletion functions to splay_tree_new_with_allocator.
Tom
On 2019-01-15 12:15, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
>
> Simon> Would you consider that a bug in the splay tree implementation?
> Simon> Should it use the new key and free the old one with the key
> Simon> deallocation function?
>
> Yes, I would think so as well, given that we pass key- and
> value-deletion functions to splay_tree_new_with_allocator.
Ok. Fixing this in the splay tree code would be a quite long task
(reviewing all usages in binutils-gdb and gcc, I don't think this code
is available externally?), so I am ok with this patch which fixes the
issue in the mean time. Tom?
Simon
>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
Simon> Ok. Fixing this in the splay tree code would be a quite long task
Simon> (reviewing all usages in binutils-gdb and gcc, I don't think this code
Simon> is available externally?), so I am ok with this patch which fixes the
Simon> issue in the mean time. Tom?
Actually, I just misunderstood the patch and/or the splay-tree API.
For some of the patch, the splay tree is doing the right thing.
This applies to the macro_define_* patches.
This though:
@@ -841,8 +850,10 @@ macro_undef (struct macro_source_file *source, int line,
arguments like '-DFOO -UFOO -DFOO=2'. */
if (source == key->start_file
&& line == key->start_line)
- splay_tree_remove (source->table->definitions, n->key);
-
+ {
+ splay_tree_remove (source->table->definitions, n->key);
+ macro_tree_delete_key (key);
+ }
else
{
/* This function is the only place a macro's end-of-scope
This one seems like it is definitely a splay-tree bug. The issue is
that it deletes a node but not the node's key.
I think it would be best by far to fix this in splay_tree_remove.
I agree it's hard, but working around this seems worse to me.
Tom
On Tue, 2019-01-15 at 10:15 -0700, Tom Tromey wrote:
> > > > > > "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
>
> Simon> Would you consider that a bug in the splay tree implementation?
> Simon> Should it use the new key and free the old one with the key
> Simon> deallocation function?
>
> Yes, I would think so as well, given that we pass key- and
> value-deletion functions to splay_tree_new_with_allocator.
>
> Tom
For sure, what splay-tree.h/.c is doing now is underspecified,
and is not very clean/orthogonal for value/key memory mgt.
However, fixing this implies to revisit all the users of the
splay tree.
If this library is only used by gcc and gdb, then probably
the change in libiberty can be coordinated so that gcc and
gdb code is changed in sync.
But if libiberty is used 'in the field' by various other code
basis, fixing the splay tree to internally release the keys
in the cases exposed in the gdb leak (and patch) will cause
(silent) heap corruptions (double free) in all the splay tree
users that are today properly freeing the keys.
Not really a nice backward compatible fix :).
Deciding for a fix in splay tree for sure implies
more discussions (no idea who knows where libiberty is used).
Thanks
Philippe
On Tue, 2019-01-15 at 11:50 -0500, Simon Marchi wrote:
> > + gdb::unique_xmalloc_ptr<char> name (extract_identifier (&exp, 0));
> > + if (name.get () == NULL)
>
> Nit, you don't have to use ".get ()" here.
One day, I will really have to learn some C++ :).
>
> I think extract_identifier should return a gdb::unique_xmalloc_ptr.
> This call site lower:
>
> argv[new_macro.argc] = extract_identifier (&exp, 1);
>
> can then use ".release ()".
Yes, that looks like a cleaner approach.
I will update the patch once the question about how to
handle the splay tree leak is clarified
i.e. fix in GDB, or (incompatible) fix in libiberty.
(see other mail with Tom).
Thanks
Philippe
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
Tom> I think it would be best by far to fix this in splay_tree_remove.
Tom> I agree it's hard, but working around this seems worse to me.
I looked, and there is one spot in gcc that passes a key deletion
function, and one spot in binutils-gdb (the spot in question).
The module in gcc (gcc/lto/lto.c) does not call splay_tree_remove.
So, I think landing the fix upstream in gcc should be totally fine.
Tom
>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
Philippe> Deciding for a fix in splay tree for sure implies
Philippe> more discussions (no idea who knows where libiberty is used).
This isn't typically a consideration for libiberty. It's not released
separately from the gcc and binutils-gdb trees, so if someone else out
there is using it, they need to keep watch over what they do when they
start using a new version.
Tom
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
Philippe> Deciding for a fix in splay tree for sure implies
Philippe> more discussions (no idea who knows where libiberty is used).
Tom> This isn't typically a consideration for libiberty. It's not released
Tom> separately from the gcc and binutils-gdb trees, so if someone else out
Tom> there is using it, they need to keep watch over what they do when they
Tom> start using a new version.
BTW, I can send a patch to gcc if you would prefer.
Tom
On Thu, 2019-01-17 at 15:24 -0700, Tom Tromey wrote:
> > > > > > "Tom" == Tom Tromey <tom@tromey.com> writes:
> > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
>
> Philippe> Deciding for a fix in splay tree for sure implies
> Philippe> more discussions (no idea who knows where libiberty is used).
>
> Tom> This isn't typically a consideration for libiberty. It's not released
> Tom> separately from the gcc and binutils-gdb trees, so if someone else out
> Tom> there is using it, they need to keep watch over what they do when they
> Tom> start using a new version.
>
> BTW, I can send a patch to gcc if you would prefer.
Yes, if the below patch is ok, it would be really nice if you could send
it to gcc:
https://sourceware.org/ml/gdb-patches/2019-01/msg00437.html
(of course, fix/update the patch as you wish/as needed).
Thanks
Philippe
>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
Tom > BTW, I can send a patch to gcc if you would prefer.
Philippe> Yes, if the below patch is ok, it would be really nice if you could send
Philippe> it to gcc:
Philippe> https://sourceware.org/ml/gdb-patches/2019-01/msg00437.html
Philippe> (of course, fix/update the patch as you wish/as needed).
I didn't see a patch below; but I had gone ahead & sent the patch
yesterday:
https://gcc.gnu.org/ml/gcc-patches/2019-01/msg01101.html
Tom
@@ -346,14 +346,13 @@ static void
macro_define_command (const char *exp, int from_tty)
{
temporary_macro_definition new_macro;
- char *name = NULL;
if (!exp)
error (_("usage: macro define NAME[(ARGUMENT-LIST)] [REPLACEMENT-LIST]"));
skip_ws (&exp);
- name = extract_identifier (&exp, 0);
- if (! name)
+ gdb::unique_xmalloc_ptr<char> name (extract_identifier (&exp, 0));
+ if (name.get () == NULL)
error (_("Invalid macro name."));
if (*exp == '(')
{
@@ -404,14 +403,15 @@ macro_define_command (const char *exp, int from_tty)
++exp;
skip_ws (&exp);
- macro_define_function (macro_main (macro_user_macros), -1, name,
+ macro_define_function (macro_main (macro_user_macros), -1, name.get (),
new_macro.argc, (const char **) new_macro.argv,
exp);
}
else
{
skip_ws (&exp);
- macro_define_object (macro_main (macro_user_macros), -1, name, exp);
+ macro_define_object (macro_main (macro_user_macros), -1, name.get (),
+ exp);
}
}
@@ -743,21 +743,27 @@ check_for_redefinition (struct macro_source_file *source, int line,
return 0;
}
-/* A helper function to define a new object-like macro. */
+/* A helper function to define a new object-like or function-like macro
+ according to KIND. When KIND is macro_object_like,
+ the macro_special_kind must be provided as ARGC, and ARGV must be NULL.
+ When KIND is macro_function_like, ARGC and ARGV are giving the function
+ arguments. */
static void
-macro_define_object_internal (struct macro_source_file *source, int line,
- const char *name, const char *replacement,
- enum macro_special_kind kind)
+macro_define_internal (struct macro_source_file *source, int line,
+ const char *name, enum macro_kind kind,
+ int argc, const char **argv,
+ const char *replacement)
{
struct macro_table *t = source->table;
struct macro_key *k = NULL;
struct macro_definition *d;
+ splay_tree_node s;
if (! t->redef_ok)
- k = check_for_redefinition (source, line,
- name, macro_object_like,
- 0, 0,
+ k = check_for_redefinition (source, line,
+ name, kind,
+ argc, argv,
replacement);
/* If we're redefining a symbol, and the existing key would be
@@ -774,8 +780,28 @@ macro_define_object_internal (struct macro_source_file *source, int line,
return;
k = new_macro_key (t, name, source, line);
- d = new_macro_definition (t, macro_object_like, kind, 0, replacement);
- splay_tree_insert (t->definitions, (splay_tree_key) k, (splay_tree_value) d);
+ d = new_macro_definition (t, kind, argc, argv, replacement);
+ s = splay_tree_insert (t->definitions, (splay_tree_key) k, (splay_tree_value) d);
+ if ((struct macro_key *) s->key != k)
+ {
+ /* If the node inserted is not using k, it means we have a redefinition.
+ We must free the newly allocated k, as it will not be stored in
+ the splay tree. */
+ macro_tree_delete_key (k);
+ }
+}
+
+/* A helper function to define a new object-like macro. */
+
+static void
+macro_define_object_internal (struct macro_source_file *source, int line,
+ const char *name, const char *replacement,
+ enum macro_special_kind special_kind)
+{
+ macro_define_internal (source, line,
+ name, macro_object_like,
+ special_kind, 0,
+ replacement);
}
void
@@ -802,29 +828,12 @@ macro_define_function (struct macro_source_file *source, int line,
const char *name, int argc, const char **argv,
const char *replacement)
{
- struct macro_table *t = source->table;
- struct macro_key *k = NULL;
- struct macro_definition *d;
-
- if (! t->redef_ok)
- k = check_for_redefinition (source, line,
- name, macro_function_like,
- argc, argv,
- replacement);
-
- /* See comments about duplicate keys in macro_define_object. */
- if (k && ! key_compare (k, name, source, line))
- return;
-
- /* We should also check here that all the argument names in ARGV are
- distinct. */
-
- k = new_macro_key (t, name, source, line);
- d = new_macro_definition (t, macro_function_like, argc, argv, replacement);
- splay_tree_insert (t->definitions, (splay_tree_key) k, (splay_tree_value) d);
+ macro_define_internal (source, line,
+ name, macro_function_like,
+ argc, argv,
+ replacement);
}
-
void
macro_undef (struct macro_source_file *source, int line,
const char *name)
@@ -841,8 +850,10 @@ macro_undef (struct macro_source_file *source, int line,
arguments like '-DFOO -UFOO -DFOO=2'. */
if (source == key->start_file
&& line == key->start_line)
- splay_tree_remove (source->table->definitions, n->key);
-
+ {
+ splay_tree_remove (source->table->definitions, n->key);
+ macro_tree_delete_key (key);
+ }
else
{
/* This function is the only place a macro's end-of-scope