diff mbox

[RFAv2] Fix leaks in macro definitions.

Message ID 20190119172802.25220-1-philippe.waroquiers@skynet.be
State New, archived
Headers

Commit Message

Philippe Waroquiers Jan. 19, 2019, 5:28 p.m. UTC 
  Valgrind detects leaks like the following (gdb.base/macscp.exp).
This patch fixes 2 of the 3 leaks.
The remaining leak is to be fixed in splay_tree_remove in libiberty.
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)

This is the second version of the patch.

Compared to first version, the changes are:
  * Handled the comment of Simon to have extract_identifier returning
    a unique_ptr.
  * Handled the comment of Tom that suggested rather to fix one of the
    leaks in splay-tree.c (which is a libiberty patch).

gdb/ChangeLog
2019-01-19  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* macrocmd.c (extract_identifier): Return
	a gdb::unique_xmalloc_ptr<char> instead of a char *, and update
	callers.
	* 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.
---
 gdb/macrocmd.c | 21 ++++++++-------
 gdb/macrotab.c | 69 ++++++++++++++++++++++++++++----------------------
 2 files changed, 49 insertions(+), 41 deletions(-)

Comments

Simon Marchi Jan. 21, 2019, 11:15 p.m. UTC | #1 
On 2019-01-19 12:28 p.m., Philippe Waroquiers wrote:
> @@ -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);


If this is really the expected behavior, I really think this should be documented in the
splay tree API.  It's really a trap everybody will fall into.

You patch LGTM from my point of view, but I'll let Tom give a final OK.

Simon
Tom Tromey Jan. 22, 2019, 8:33 a.m. UTC | #2 
>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:

Simon> On 2019-01-19 12:28 p.m., Philippe Waroquiers wrote:
>> @@ -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);

Simon> If this is really the expected behavior, I really think this should be documented in the
Simon> splay tree API.  It's really a trap everybody will fall into.

I guess I wasn't thinking, or maybe reading or both, clearly in the
previous round of splay-tree fixing.

It seems that the contract of splay_tree_insert must be that it takes
ownership of the key and the value.  So, it should indeed delete a
duplicate key here.

In gcc, this only affects lto.c, so I suppose that has a latent leak:

  unsigned HOST_WIDE_INT *idp = XCNEW (unsigned HOST_WIDE_INT);
  *idp = id;
  splay_tree_insert (t, (splay_tree_key) idp, (splay_tree_value) file_data);

... assuming duplicate keys can ever be seen, which I don't know.

Tom
diff mbox

Patch

diff --git a/gdb/macrocmd.c b/gdb/macrocmd.c
index 706a8353e2..7913c9871c 100644
--- a/gdb/macrocmd.c
+++ b/gdb/macrocmd.c
@@ -288,7 +288,7 @@  skip_ws (const char **expp)
    function will also allow "..." forms as used in varargs macro
    parameters.  */
 
-static char *
+static gdb::unique_xmalloc_ptr<char>
 extract_identifier (const char **expp, int is_parameter)
 {
   char *result;
@@ -317,7 +317,7 @@  extract_identifier (const char **expp, int is_parameter)
   memcpy (result, *expp, len);
   result[len] = '\0';
   *expp += len;
-  return result;
+  return gdb::unique_xmalloc_ptr<char> (result);
 }
 
 struct temporary_macro_definition : public macro_definition
@@ -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 == NULL)
     error (_("Invalid macro name."));
   if (*exp == '(')
     {
@@ -380,7 +379,7 @@  macro_define_command (const char *exp, int from_tty)
 	      /* Must update new_macro as well...  */
 	      new_macro.argv = (const char * const *) argv;
 	    }
-	  argv[new_macro.argc] = extract_identifier (&exp, 1);
+	  argv[new_macro.argc] = extract_identifier (&exp, 1).release ();
 	  if (! argv[new_macro.argc])
 	    error (_("Macro is missing an argument."));
 	  ++new_macro.argc;
@@ -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);
     }
 }
 
@@ -419,7 +419,7 @@  macro_define_command (const char *exp, int from_tty)
 static void
 macro_undef_command (const char *exp, int from_tty)
 {
-  char *name;
+  gdb::unique_xmalloc_ptr<char> name;
 
   if (!exp)
     error (_("usage: macro undef NAME"));
@@ -428,8 +428,7 @@  macro_undef_command (const char *exp, int from_tty)
   name = extract_identifier (&exp, 0);
   if (! name)
     error (_("Invalid macro name."));
-  macro_undef (macro_main (macro_user_macros), -1, name);
-  xfree (name);
+  macro_undef (macro_main (macro_user_macros), -1, name.get ());
 }
 
 
diff --git a/gdb/macrotab.c b/gdb/macrotab.c
index 7fcab4691b..99e2231089 100644
--- a/gdb/macrotab.c
+++ b/gdb/macrotab.c
@@ -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)