[PATCH v2] [gdb/symtab] Simplify memory management in parse_macro_definition

[PATCH v2] [gdb/symtab] Simplify memory management in parse_macro_definition

[Tom de Vries]

I noticed that parse_macro_definition does malloc and free, both for the
argv array and its elements.

Make memory management simpler by introducing a new class
vector_c_string (copied from temporary_macro_definition here [1]) that uses an
std::vector for the argv array.

Tested on x86_64-linux.

Co-Authored-By: Tom Tromey <tom@tromey.com>

[1] https://sourceware.org/pipermail/gdb-patches/2024-April/208456.html
---
 gdb/dwarf2/macro.c | 67 +++++++++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 24 deletions(-)

diff --git a/gdb/dwarf2/macro.c b/gdb/dwarf2/macro.c
index bc781c2cb92..e558a900a2c 100644
--- a/gdb/dwarf2/macro.c
+++ b/gdb/dwarf2/macro.c
@@ -36,6 +36,34 @@
 #include "complaints.h"
 #include "objfiles.h"
 
+/* Vector of C strings.  */
+
+struct vector_c_string
+{
+  vector_c_string () = default;
+  DISABLE_COPY_AND_ASSIGN (vector_c_string);
+
+  ~vector_c_string ()
+  {
+    free_vector_argv (argv);
+  }
+
+  /* Add element to vector.  */
+  void push_back (gdb::unique_xmalloc_ptr<char> &&value)
+  {
+    argv.push_back (value.release ());
+  }
+
+  /* De-allocate elements and clear vector.  */
+  void clear ()
+  {
+    free_vector_argv (argv);
+    argv.clear ();
+  }
+
+  std::vector<char *> argv;
+};
+
 static void
 dwarf2_macro_malformed_definition_complaint (const char *arg1)
 {
@@ -103,7 +131,7 @@ consume_improper_spaces (const char *p, const char *body)
 
 static void
 parse_macro_definition (struct macro_source_file *file, int line,
-			const char *body)
+			const char *body, vector_c_string &tmp)
 {
   const char *p;
 
@@ -159,15 +187,14 @@ parse_macro_definition (struct macro_source_file *file, int line,
 
   /* It's a function-like macro.  */
   gdb_assert (*p == '(');
-  std::string name (body, p - body);
-  int argc = 0;
-  int argv_size = 1;
-  char **argv = XNEWVEC (char *, argv_size);
 
+  std::string name (body, p - body);
   p++;
 
   p = consume_improper_spaces (p, body);
 
+  tmp.clear ();
+
   /* Parse the formal argument list.  */
   while (*p && *p != ')')
     {
@@ -181,14 +208,9 @@ parse_macro_definition (struct macro_source_file *file, int line,
 	dwarf2_macro_malformed_definition_complaint (body);
       else
 	{
-	  /* Make sure argv has room for the new argument.  */
-	  if (argc >= argv_size)
-	    {
-	      argv_size *= 2;
-	      argv = XRESIZEVEC (char *, argv, argv_size);
-	    }
-
-	  argv[argc++] = savestring (arg_start, p - arg_start);
+	  gdb::unique_xmalloc_ptr<char> arg (savestring (arg_start,
+							 p - arg_start));
+	  tmp.push_back (std::move (arg));
 	}
 
       p = consume_improper_spaces (p, body);
@@ -209,15 +231,15 @@ parse_macro_definition (struct macro_source_file *file, int line,
       if (*p == ' ')
 	/* Perfectly formed definition, no complaints.  */
 	macro_define_function (file, line, name.c_str (),
-			       argc, (const char **) argv,
-			       p + 1);
+			       tmp.argv.size (),
+			       (const char **) tmp.argv.data (), p + 1);
       else if (*p == '\0')
 	{
 	  /* Complain, but do define it.  */
 	  dwarf2_macro_malformed_definition_complaint (body);
 	  macro_define_function (file, line, name.c_str (),
-				 argc, (const char **) argv,
-				 p);
+				 tmp.argv.size (),
+				 (const char **) tmp.argv.data (), p);
 	}
       else
 	/* Just complain.  */
@@ -226,11 +248,6 @@ parse_macro_definition (struct macro_source_file *file, int line,
   else
     /* Just complain.  */
     dwarf2_macro_malformed_definition_complaint (body);
-
-  for (int i = 0; i < argc; i++)
-    xfree (argv[i]);
-
-  xfree (argv);
 }
 
 /* Skip some bytes from BYTES according to the form given in FORM.
@@ -446,6 +463,8 @@ dwarf_decode_macro_bytes (dwarf2_per_objfile *per_objfile,
   int at_commandline;
   const gdb_byte *opcode_definitions[256];
 
+  vector_c_string tmp;
+
   mac_ptr = dwarf_parse_macro_header (opcode_definitions, abfd, mac_ptr,
 				      &offset_size, section_is_gnu);
   if (mac_ptr == NULL)
@@ -563,7 +582,7 @@ dwarf_decode_macro_bytes (dwarf2_per_objfile *per_objfile,
 			   line, current_file->filename);
 	      }
 	    else if (is_define)
-	      parse_macro_definition (current_file, line, body);
+	      parse_macro_definition (current_file, line, body, tmp);
 	    else
 	      {
 		gdb_assert (macinfo_type == DW_MACRO_undef
@@ -627,7 +646,7 @@ dwarf_decode_macro_bytes (dwarf2_per_objfile *per_objfile,
 	      }
 
 	    if (macinfo_type == DW_MACRO_define_strx)
-	      parse_macro_definition (current_file, line, body);
+	      parse_macro_definition (current_file, line, body, tmp);
 	    else
 	      macro_undef (current_file, line, body);
 	   }

base-commit: bd54c881cd14af32f2347dab5ce51823ed631a88
-- 
2.35.3

[PING] [PATCH v2] [gdb/symtab] Simplify memory management in parse_macro_definition

[Tom de Vries]

On 6/26/24 08:32, Tom de Vries wrote:
> I noticed that parse_macro_definition does malloc and free, both for the
> argv array and its elements.
> 
> Make memory management simpler by introducing a new class
> vector_c_string (copied from temporary_macro_definition here [1]) that uses an
> std::vector for the argv array.
> 

Ping.

Thanks,
- Tom

> Tested on x86_64-linux.
> 
> Co-Authored-By: Tom Tromey <tom@tromey.com>
> 
> [1] https://sourceware.org/pipermail/gdb-patches/2024-April/208456.html
> ---
>   gdb/dwarf2/macro.c | 67 +++++++++++++++++++++++++++++-----------------
>   1 file changed, 43 insertions(+), 24 deletions(-)
> 
> diff --git a/gdb/dwarf2/macro.c b/gdb/dwarf2/macro.c
> index bc781c2cb92..e558a900a2c 100644
> --- a/gdb/dwarf2/macro.c
> +++ b/gdb/dwarf2/macro.c
> @@ -36,6 +36,34 @@
>   #include "complaints.h"
>   #include "objfiles.h"
>   
> +/* Vector of C strings.  */
> +
> +struct vector_c_string
> +{
> +  vector_c_string () = default;
> +  DISABLE_COPY_AND_ASSIGN (vector_c_string);
> +
> +  ~vector_c_string ()
> +  {
> +    free_vector_argv (argv);
> +  }
> +
> +  /* Add element to vector.  */
> +  void push_back (gdb::unique_xmalloc_ptr<char> &&value)
> +  {
> +    argv.push_back (value.release ());
> +  }
> +
> +  /* De-allocate elements and clear vector.  */
> +  void clear ()
> +  {
> +    free_vector_argv (argv);
> +    argv.clear ();
> +  }
> +
> +  std::vector<char *> argv;
> +};
> +
>   static void
>   dwarf2_macro_malformed_definition_complaint (const char *arg1)
>   {
> @@ -103,7 +131,7 @@ consume_improper_spaces (const char *p, const char *body)
>   
>   static void
>   parse_macro_definition (struct macro_source_file *file, int line,
> -			const char *body)
> +			const char *body, vector_c_string &tmp)
>   {
>     const char *p;
>   
> @@ -159,15 +187,14 @@ parse_macro_definition (struct macro_source_file *file, int line,
>   
>     /* It's a function-like macro.  */
>     gdb_assert (*p == '(');
> -  std::string name (body, p - body);
> -  int argc = 0;
> -  int argv_size = 1;
> -  char **argv = XNEWVEC (char *, argv_size);
>   
> +  std::string name (body, p - body);
>     p++;
>   
>     p = consume_improper_spaces (p, body);
>   
> +  tmp.clear ();
> +
>     /* Parse the formal argument list.  */
>     while (*p && *p != ')')
>       {
> @@ -181,14 +208,9 @@ parse_macro_definition (struct macro_source_file *file, int line,
>   	dwarf2_macro_malformed_definition_complaint (body);
>         else
>   	{
> -	  /* Make sure argv has room for the new argument.  */
> -	  if (argc >= argv_size)
> -	    {
> -	      argv_size *= 2;
> -	      argv = XRESIZEVEC (char *, argv, argv_size);
> -	    }
> -
> -	  argv[argc++] = savestring (arg_start, p - arg_start);
> +	  gdb::unique_xmalloc_ptr<char> arg (savestring (arg_start,
> +							 p - arg_start));
> +	  tmp.push_back (std::move (arg));
>   	}
>   
>         p = consume_improper_spaces (p, body);
> @@ -209,15 +231,15 @@ parse_macro_definition (struct macro_source_file *file, int line,
>         if (*p == ' ')
>   	/* Perfectly formed definition, no complaints.  */
>   	macro_define_function (file, line, name.c_str (),
> -			       argc, (const char **) argv,
> -			       p + 1);
> +			       tmp.argv.size (),
> +			       (const char **) tmp.argv.data (), p + 1);
>         else if (*p == '\0')
>   	{
>   	  /* Complain, but do define it.  */
>   	  dwarf2_macro_malformed_definition_complaint (body);
>   	  macro_define_function (file, line, name.c_str (),
> -				 argc, (const char **) argv,
> -				 p);
> +				 tmp.argv.size (),
> +				 (const char **) tmp.argv.data (), p);
>   	}
>         else
>   	/* Just complain.  */
> @@ -226,11 +248,6 @@ parse_macro_definition (struct macro_source_file *file, int line,
>     else
>       /* Just complain.  */
>       dwarf2_macro_malformed_definition_complaint (body);
> -
> -  for (int i = 0; i < argc; i++)
> -    xfree (argv[i]);
> -
> -  xfree (argv);
>   }
>   
>   /* Skip some bytes from BYTES according to the form given in FORM.
> @@ -446,6 +463,8 @@ dwarf_decode_macro_bytes (dwarf2_per_objfile *per_objfile,
>     int at_commandline;
>     const gdb_byte *opcode_definitions[256];
>   
> +  vector_c_string tmp;
> +
>     mac_ptr = dwarf_parse_macro_header (opcode_definitions, abfd, mac_ptr,
>   				      &offset_size, section_is_gnu);
>     if (mac_ptr == NULL)
> @@ -563,7 +582,7 @@ dwarf_decode_macro_bytes (dwarf2_per_objfile *per_objfile,
>   			   line, current_file->filename);
>   	      }
>   	    else if (is_define)
> -	      parse_macro_definition (current_file, line, body);
> +	      parse_macro_definition (current_file, line, body, tmp);
>   	    else
>   	      {
>   		gdb_assert (macinfo_type == DW_MACRO_undef
> @@ -627,7 +646,7 @@ dwarf_decode_macro_bytes (dwarf2_per_objfile *per_objfile,
>   	      }
>   
>   	    if (macinfo_type == DW_MACRO_define_strx)
> -	      parse_macro_definition (current_file, line, body);
> +	      parse_macro_definition (current_file, line, body, tmp);
>   	    else
>   	      macro_undef (current_file, line, body);
>   	   }
> 
> base-commit: bd54c881cd14af32f2347dab5ce51823ed631a88

Re: [PATCH v2] [gdb/symtab] Simplify memory management in parse_macro_definition

[Simon Marchi]

On 6/26/24 2:32 AM, Tom de Vries wrote:
> I noticed that parse_macro_definition does malloc and free, both for the
> argv array and its elements.
> 
> Make memory management simpler by introducing a new class
> vector_c_string (copied from temporary_macro_definition here [1]) that uses an
> std::vector for the argv array.

Is there a reason to keep using C strings here, instead of a let's say,
a vector of std::string?  I gave it a quick try, I have something that
builds, just need to test and polish it.

Here is it if you want to take a peek: https://review.lttng.org/c/binutils-gdb/+/13009

Simon

Re: [PATCH v2] [gdb/symtab] Simplify memory management in parse_macro_definition

[Tom de Vries]

On 7/24/24 18:07, Simon Marchi wrote:
> On 6/26/24 2:32 AM, Tom de Vries wrote:
>> I noticed that parse_macro_definition does malloc and free, both for the
>> argv array and its elements.
>>
>> Make memory management simpler by introducing a new class
>> vector_c_string (copied from temporary_macro_definition here [1]) that uses an
>> std::vector for the argv array.
> 
> Is there a reason to keep using C strings here, instead of a let's say,
> a vector of std::string?  I gave it a quick try, I have something that
> builds, just need to test and polish it.
> 

Hi Simon,

thanks for the review.

No, not really, my main goal here is the ability to do a return in 
parse_macro_definition without introducing a memory leak.

So that sounds fine to me.

Thanks,
- Tom

> Here is it if you want to take a peek: https://review.lttng.org/c/binutils-gdb/+/13009
> 
> Simon

Re: [PATCH v2] [gdb/symtab] Simplify memory management in parse_macro_definition

[Tom de Vries]

On 7/24/24 18:12, Tom de Vries wrote:
> On 7/24/24 18:07, Simon Marchi wrote:
>> On 6/26/24 2:32 AM, Tom de Vries wrote:
>>> I noticed that parse_macro_definition does malloc and free, both for the
>>> argv array and its elements.
>>>
>>> Make memory management simpler by introducing a new class
>>> vector_c_string (copied from temporary_macro_definition here [1]) 
>>> that uses an
>>> std::vector for the argv array.
>>
>> Is there a reason to keep using C strings here, instead of a let's say,
>> a vector of std::string?  I gave it a quick try, I have something that
>> builds, just need to test and polish it.
>>
> 
> Hi Simon,
> 
> thanks for the review.
> 
> No, not really, my main goal here is the ability to do a return in 
> parse_macro_definition without introducing a memory leak.
> 
> So that sounds fine to me.
> 

I've looked at the patch.

I've was thrown astray a bit about the macro_special_kind business, 
until I realized it was a pre-existing notion.

Anyway, patch LGTM.

Tested on x86_64-linux, no issues found.

Thanks,
- Tom


> Thanks,
> - Tom
> 
>> Here is it if you want to take a peek: 
>> https://review.lttng.org/c/binutils-gdb/+/13009
>>
>> Simon
> 

Re: [PATCH v2] [gdb/symtab] Simplify memory management in parse_macro_definition

[Simon Marchi]

On 7/29/24 12:13 PM, Tom de Vries wrote:
> On 7/24/24 18:12, Tom de Vries wrote:
>> On 7/24/24 18:07, Simon Marchi wrote:
>>> On 6/26/24 2:32 AM, Tom de Vries wrote:
>>>> I noticed that parse_macro_definition does malloc and free, both for the
>>>> argv array and its elements.
>>>>
>>>> Make memory management simpler by introducing a new class
>>>> vector_c_string (copied from temporary_macro_definition here [1]) that uses an
>>>> std::vector for the argv array.
>>>
>>> Is there a reason to keep using C strings here, instead of a let's say,
>>> a vector of std::string?  I gave it a quick try, I have something that
>>> builds, just need to test and polish it.
>>>
>>
>> Hi Simon,
>>
>> thanks for the review.
>>
>> No, not really, my main goal here is the ability to do a return in parse_macro_definition without introducing a memory leak.
>>
>> So that sounds fine to me.
>>
> 
> I've looked at the patch.
> 
> I've was thrown astray a bit about the macro_special_kind business, until I realized it was a pre-existing notion.

Yeah, I think it's a bit clearer with my change, we don't use that int
parameter for multiple purposes, including passing an enum.

> Anyway, patch LGTM.
> 
> Tested on x86_64-linux, no issues found.

For clarity, can you give your approval on the version I posted here?
Sorry, it wasn't clear that I posted it on the list.

https://inbox.sourceware.org/gdb-patches/20240724190833.69587-1-simon.marchi@efficios.com/#r

Simon