[RFAv3 2/2] Factorize macro definition code in macrotab.c

[RFAv3 2/2] Factorize macro definition code in macrotab.c

[Philippe Waroquiers]

When first fixing splay tree key leaks in macrotab.c, some duplicated code
logic was factorized.
The key leaks will be fixed in libiberty, but the code factorization
is better kept in any case.

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

	* macrotab.c (macro_define_internal): New function that
	factorizes macro_define_object_internal and macro_define_function
	code.
	(macro_define_object_internal): Use macro_define_internal.
	(macro_define_function): Likewise.
---
 gdb/macrotab.c | 59 +++++++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 29 deletions(-)

diff --git a/gdb/macrotab.c b/gdb/macrotab.c
index 7fcab4691b..f7178bdc25 100644
--- a/gdb/macrotab.c
+++ b/gdb/macrotab.c
@@ -743,21 +743,26 @@ 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;
 
   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,10 +779,23 @@ 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);
+  d = new_macro_definition (t, kind, argc, argv, replacement);
   splay_tree_insert (t->definitions, (splay_tree_key) k, (splay_tree_value) d);
 }
 
+/* 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
 macro_define_object (struct macro_source_file *source, int line,
 		     const char *name, const char *replacement)
@@ -802,29 +820,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)
-- 
2.20.1

[RFAv3 1/2] Fix leak of identifier in macro definition.

[Philippe Waroquiers]

Valgrind detects leaks like the following (gdb.base/macscp.exp).
This patch fixes 1 of the 3 leaks (the last one in the list below).

The remaining leaks are better fixed in splay_tree_remove  and
splay_tree_insert 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-26  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* macrocmd.c (extract_identifier): Return
	a gdb::unique_xmalloc_ptr<char> instead of a char *, and update
	callers.
---
 gdb/macrocmd.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

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 ());
 }
 
 
-- 
2.20.1

[RFAv3 0/2] Fix leaks in macro definition

[Philippe Waroquiers]

The first patch of this series fixes the leak of the identifier
in macro definition.
The second patch factorizes some code.

Compared to the previous version:
All the splay tree key leaks will be fixed via a change
in libiberty splay tree.

Re: [RFAv3 1/2] Fix leak of identifier in macro definition.

[Simon Marchi]

On 2019-01-26 17:31, Philippe Waroquiers wrote:
> Valgrind detects leaks like the following (gdb.base/macscp.exp).
> This patch fixes 1 of the 3 leaks (the last one in the list below).
> 
> The remaining leaks are better fixed in splay_tree_remove  and
> splay_tree_insert 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).

Thanks, this LGTM with a small nit to fix:

> @@ -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);

Declare name when assigning it, to avoid constructing an empty object 
and then assigning it.

>    if (! name)

Could you fix this to "name == NULL" or "name == nullptr" while at it?

Thanks!

Simon

Re: [RFAv3 2/2] Factorize macro definition code in macrotab.c

[Simon Marchi]

On 2019-01-26 17:31, Philippe Waroquiers wrote:
> When first fixing splay tree key leaks in macrotab.c, some duplicated 
> code
> logic was factorized.
> The key leaks will be fixed in libiberty, but the code factorization
> is better kept in any case.

LGTM, with just a nit:

> +/* 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);
> +}

Since it is a pointer parameter, could you change the "0" for "NULL" or 
"nullptr"?

Thanks,

Simon

Re: [RFAv3 2/2] Factorize macro definition code in macrotab.c

[Philippe Waroquiers]

On Tue, 2019-02-05 at 23:12 -0500, Simon Marchi wrote:
> On 2019-01-26 17:31, Philippe Waroquiers wrote:
> > When first fixing splay tree key leaks in macrotab.c, some duplicated 
> > code
> > logic was factorized.
> > The key leaks will be fixed in libiberty, but the code factorization
> > is better kept in any case.
> 
> LGTM, with just a nit:
> 
> > +/* 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);
> > +}
> 
> Since it is a pointer parameter, could you change the "0" for "NULL" or 
> "nullptr"?
Thanks for the review, I pushed the series after doing the fixes
in this and in the other mail.

Philippe