Re: [RFA] Fix leaks in macro definitions.

[Simon Marchi <simon dot marchi at polymtl dot ca>]

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