[PATCH 1/2] [gdb/symtab] Remove dead code in parse_macro_definition

[PATCH 1/2] [gdb/symtab] Remove dead code in parse_macro_definition

[Tom de Vries]

In parse_macro_definition, there's a loop:
...
  for (p = body; *p; p++)
    if (*p == ' ' || *p == '(')
      break;
...
whose post-condition is:
...
  gdb_assert (*p == ' ' || *p == '(' || *p == '\0');
...

Consequently, in the following:
...
  if (*p == ' ' || *p == '\0')
    <BODY1>
  else if (*p == '(')
    <BODY2>
  else
    <BODY3>
...
BODY3 is dead code.

Remove it, and get rid of unnecessary indentation by using an early-exit:
....
  if (*p == ' ' || *p == '\0')
    {
      <BODY1>
      return;
    }

  gdb_assert (*p == '(');
  <BODY2>
...

Tested on aarch64-linux.
---
 gdb/dwarf2/macro.c | 117 ++++++++++++++++++++++-----------------------
 1 file changed, 56 insertions(+), 61 deletions(-)

diff --git a/gdb/dwarf2/macro.c b/gdb/dwarf2/macro.c
index a511d0a3b44..bc781c2cb92 100644
--- a/gdb/dwarf2/macro.c
+++ b/gdb/dwarf2/macro.c
@@ -154,88 +154,83 @@ parse_macro_definition (struct macro_source_file *file, int line,
 	}
 
       macro_define_object (file, line, name.c_str (), replacement);
+      return;
     }
-  else if (*p == '(')
-    {
-      /* It's a function-like macro.  */
-      std::string name (body, p - body);
-      int argc = 0;
-      int argv_size = 1;
-      char **argv = XNEWVEC (char *, argv_size);
-
-      p++;
 
-      p = consume_improper_spaces (p, body);
+  /* 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);
 
-      /* Parse the formal argument list.  */
-      while (*p && *p != ')')
-	{
-	  /* Find the extent of the current argument name.  */
-	  const char *arg_start = p;
+  p++;
 
-	  while (*p && *p != ',' && *p != ')' && *p != ' ')
-	    p++;
-
-	  if (! *p || p == arg_start)
-	    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);
-		}
+  p = consume_improper_spaces (p, body);
 
-	      argv[argc++] = savestring (arg_start, p - arg_start);
-	    }
+  /* Parse the formal argument list.  */
+  while (*p && *p != ')')
+    {
+      /* Find the extent of the current argument name.  */
+      const char *arg_start = p;
 
-	  p = consume_improper_spaces (p, body);
+      while (*p && *p != ',' && *p != ')' && *p != ' ')
+	p++;
 
-	  /* Consume the comma, if present.  */
-	  if (*p == ',')
+      if (! *p || p == arg_start)
+	dwarf2_macro_malformed_definition_complaint (body);
+      else
+	{
+	  /* Make sure argv has room for the new argument.  */
+	  if (argc >= argv_size)
 	    {
-	      p++;
-
-	      p = consume_improper_spaces (p, body);
+	      argv_size *= 2;
+	      argv = XRESIZEVEC (char *, argv, argv_size);
 	    }
+
+	  argv[argc++] = savestring (arg_start, p - arg_start);
 	}
 
-      if (*p == ')')
+      p = consume_improper_spaces (p, body);
+
+      /* Consume the comma, if present.  */
+      if (*p == ',')
 	{
 	  p++;
 
-	  if (*p == ' ')
-	    /* Perfectly formed definition, no complaints.  */
-	    macro_define_function (file, line, name.c_str (),
-				   argc, (const char **) argv,
-				   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);
-	    }
-	  else
-	    /* Just complain.  */
-	    dwarf2_macro_malformed_definition_complaint (body);
+	  p = consume_improper_spaces (p, body);
+	}
+    }
+
+  if (*p == ')')
+    {
+      p++;
+
+      if (*p == ' ')
+	/* Perfectly formed definition, no complaints.  */
+	macro_define_function (file, line, name.c_str (),
+			       argc, (const char **) argv,
+			       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);
 	}
       else
 	/* Just complain.  */
 	dwarf2_macro_malformed_definition_complaint (body);
-
-      {
-	int i;
-
-	for (i = 0; i < argc; i++)
-	  xfree (argv[i]);
-      }
-      xfree (argv);
     }
   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.

base-commit: 18b13d11d37c8b7e7f9d0548d54e0cb3a01e81fb
-- 
2.35.3

[PATCH 2/2] [gdb/symtab] Malloc/free less 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.

Improve this by:
- using an std::vector for the argv array,
- using an obstack for the elements, and
- sharing allocations between calls to parse_macro_definition.

Tested on aarch64-linux.
---
 gdb/dwarf2/macro.c | 60 +++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 22 deletions(-)

diff --git a/gdb/dwarf2/macro.c b/gdb/dwarf2/macro.c
index bc781c2cb92..83965627d5b 100644
--- a/gdb/dwarf2/macro.c
+++ b/gdb/dwarf2/macro.c
@@ -36,6 +36,31 @@
 #include "complaints.h"
 #include "objfiles.h"
 
+/* Vector of C strings.  Allows to push a string_view-like string while
+   managing the memory required for that.  */
+
+class vector_c_string : public std::vector<const char *>
+{
+public:
+  /* Push a copy of ARG[0..ARG_SIZE-1].  */
+  void push_back (const char *arg, size_t arg_size)
+  {
+    const char *dup = obstack_strndup (&m_obstack, arg, arg_size);
+    std::vector<const char *>::push_back (dup);
+  }
+
+  /* Clear the vector, and de-allocate the copies.  */
+  void clear ()
+  {
+    std::vector<const char *>::clear ();
+    m_obstack.clear ();
+  }
+
+private:
+  /* Used to allocate copies.  */
+  auto_obstack m_obstack;
+};
+
 static void
 dwarf2_macro_malformed_definition_complaint (const char *arg1)
 {
@@ -105,6 +130,9 @@ static void
 parse_macro_definition (struct macro_source_file *file, int line,
 			const char *body)
 {
+  /* By making this static we share allocation between invocations.  */
+  static vector_c_string argv;
+
   const char *p;
 
   /* The body string takes one of two forms.  For object-like macro
@@ -160,9 +188,8 @@ 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);
+
+  argv.clear ();
 
   p++;
 
@@ -181,14 +208,10 @@ 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);
+	  /* Push a zero-terminated copy of arg.  This wouldn't be
+	     necessary if macro_define_function handled string views
+	     args.  */
+	  argv.push_back (arg_start, p - arg_start);
 	}
 
       p = consume_improper_spaces (p, body);
@@ -208,16 +231,14 @@ 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);
+	macro_define_function (file, line, name.c_str (), argv.size (),
+			       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);
+	  macro_define_function (file, line, name.c_str (), argv.size (),
+				 argv.data (), p);
 	}
       else
 	/* Just complain.  */
@@ -226,11 +247,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.
-- 
2.35.3

Re: [PATCH 1/2] [gdb/symtab] Remove dead code in parse_macro_definition

[Alexandra Petlanova Hajkova]

[-- Attachment #1: Type: text/plain, Size: 951 bytes --]

On Mon, Jun 24, 2024 at 10:13 AM Tom de Vries <tdevries@suse.de> wrote:

> In parse_macro_definition, there's a loop:
> ...
>   for (p = body; *p; p++)
>     if (*p == ' ' || *p == '(')
>       break;
> ...
> whose post-condition is:
> ...
>   gdb_assert (*p == ' ' || *p == '(' || *p == '\0');
> ...
>
> Consequently, in the following:
> ...
>   if (*p == ' ' || *p == '\0')
>     <BODY1>
>   else if (*p == '(')
>     <BODY2>
>   else
>     <BODY3>
> ...
> BODY3 is dead code.
>
> Remove it, and get rid of unnecessary indentation by using an early-exit:
> ....
>   if (*p == ' ' || *p == '\0')
>     {
>       <BODY1>
>       return;
>     }
>
>   gdb_assert (*p == '(');
>   <BODY2>
> ...
>
> Tested on aarch64-linux.
> ---
>
>
Look reasonable. Also I can confirm this change does not cause any
resgrassions for ppc64le Fedora Rawhide.

 Reviewed-By: Alexandra Petlanova Hajkova <ahajkova@redhat.com>

Re: [PATCH 2/2] [gdb/symtab] Malloc/free less in parse_macro_definition

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> +/* Vector of C strings.  Allows to push a string_view-like string while
Tom> +   managing the memory required for that.  */
Tom> +
Tom> +class vector_c_string : public std::vector<const char *>

I think it's generally considered bad practice to inherit from std
types.

Tom> +  /* By making this static we share allocation between invocations.  */
Tom> +  static vector_c_string argv;

I think we should avoid static locals.  For one thing they inhibit our
ability to use threads.

If allocation is a performance issue or something then maybe the storage
can be hoisted to some more-outer function somewhere.

I wonder if this could share some code with:

https://inbox.sourceware.org/gdb-patches/20240426154439.94728-1-tromey@adacore.com/

Like maybe a different refactoring is in order.

Tom

Re: [PATCH 1/2] [gdb/symtab] Remove dead code in parse_macro_definition

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Remove it, and get rid of unnecessary indentation by using an early-exit:
Tom> ....
Tom>   if (*p == ' ' || *p == '\0')
Tom>     {
Tom>       <BODY1>
Tom>       return;
Tom>     }

Looks good.  Thank you.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 2/2] [gdb/symtab] Malloc/free less in parse_macro_definition

[Tom de Vries]

On 6/24/24 20:20, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> +/* Vector of C strings.  Allows to push a string_view-like string while
> Tom> +   managing the memory required for that.  */
> Tom> +
> Tom> +class vector_c_string : public std::vector<const char *>
> 
> I think it's generally considered bad practice to inherit from std
> types.
> 

Thanks for pointing that out.  I googled a bit, and understood that this 
is due to std::vector having a non-virtual destructor.

> Tom> +  /* By making this static we share allocation between invocations.  */
> Tom> +  static vector_c_string argv;
> 
> I think we should avoid static locals.  For one thing they inhibit our
> ability to use threads.
> 
> If allocation is a performance issue or something then maybe the storage
> can be hoisted to some more-outer function somewhere.
> 

Done.

> I wonder if this could share some code with:
> 
> https://inbox.sourceware.org/gdb-patches/20240426154439.94728-1-tromey@adacore.com/
> 
> Like maybe a different refactoring is in order.

I've borrowed from that patch, and submitted a v2 (named slightly 
differently) here ( 
https://sourceware.org/pipermail/gdb-patches/2024-June/210218.html ).

Thanks,
- Tom