Patchwork [RFA] Remove two cleanups using std::string

login
register
mail settings
Submitter Tom Tromey
Date March 12, 2018, 10:38 p.m.
Message ID <20180312223829.7108-1-tom@tromey.com>
Download mbox | patch
Permalink /patch/26295/
State New
Headers show

Comments

Tom Tromey - March 12, 2018, 10:38 p.m.
This patches removes cleanups from a couple of spots by using
std::string rather than manual memory management.

Regression tested by the buildbot, though note that I don't believe
the buildbot actually exercises the machoread code.

gdb/ChangeLog
2018-03-12  Tom Tromey  <tom@tromey.com>

	* machoread.c (macho_check_dsym): Change filenamep to a
	std::string*.
	(macho_symfile_read): Update.
	* symfile.c (load_command): Use std::string.
---
 gdb/ChangeLog   |  7 +++++++
 gdb/machoread.c | 12 +++++-------
 gdb/symfile.c   | 44 ++++++++++++++------------------------------
 3 files changed, 26 insertions(+), 37 deletions(-)
Simon Marchi - March 13, 2018, 2:37 a.m.
On 2018-03-12 06:38 PM, Tom Tromey wrote:
> This patches removes cleanups from a couple of spots by using
> std::string rather than manual memory management.
> 
> Regression tested by the buildbot, though note that I don't believe
> the buildbot actually exercises the machoread code.
> 
> gdb/ChangeLog
> 2018-03-12  Tom Tromey  <tom@tromey.com>
> 
> 	* machoread.c (macho_check_dsym): Change filenamep to a
> 	std::string*.
> 	(macho_symfile_read): Update.
> 	* symfile.c (load_command): Use std::string.
> ---
>  gdb/ChangeLog   |  7 +++++++
>  gdb/machoread.c | 12 +++++-------
>  gdb/symfile.c   | 44 ++++++++++++++------------------------------
>  3 files changed, 26 insertions(+), 37 deletions(-)
> 
> diff --git a/gdb/machoread.c b/gdb/machoread.c
> index b270675d61..ab4410c925 100644
> --- a/gdb/machoread.c
> +++ b/gdb/machoread.c
> @@ -750,12 +750,12 @@ macho_symfile_read_all_oso (VEC (oso_el) **oso_vector_ptr,
>  #define DSYM_SUFFIX ".dSYM/Contents/Resources/DWARF/"
>  
>  /* Check if a dsym file exists for OBJFILE.  If so, returns a bfd for it
> -   and return *FILENAMEP with its original xmalloc-ated filename.
> +   and return *FILENAMEP with its original filename.
>     Return NULL if no valid dsym file is found (FILENAMEP is not used in
>     such case).  */
>  
>  static gdb_bfd_ref_ptr
> -macho_check_dsym (struct objfile *objfile, char **filenamep)
> +macho_check_dsym (struct objfile *objfile, std::string *filenamep)
>  {
>    size_t name_len = strlen (objfile_name (objfile));
>    size_t dsym_len = strlen (DSYM_SUFFIX);
> @@ -804,7 +804,7 @@ macho_check_dsym (struct objfile *objfile, char **filenamep)
>  	       objfile_name (objfile));
>        return NULL;
>      }
> -  *filenamep = xstrdup (dsym_filename);
> +  *filenamep = std::string (dsym_filename);
>    return dsym_bfd;
>  }
>  
> @@ -821,7 +821,7 @@ macho_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
>       be in the executable.  */
>    if (bfd_get_file_flags (abfd) & (EXEC_P | DYNAMIC))
>      {
> -      char *dsym_filename;
> +      std::string dsym_filename;
>  
>        /* Process the normal symbol table first.  */
>        storage_needed = bfd_get_symtab_upper_bound (objfile->obfd);
> @@ -865,8 +865,6 @@ macho_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
>  	{
>            struct bfd_section *asect, *dsect;
>  
> -	  make_cleanup (xfree, dsym_filename);
> -
>  	  if (mach_o_debug_level > 0)
>  	    printf_unfiltered (_("dsym file found\n"));
>  
> @@ -882,7 +880,7 @@ macho_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
>              }
>  
>  	  /* Add the dsym file as a separate file.  */
> -          symbol_file_add_separate (dsym_bfd.get (), dsym_filename,
> +          symbol_file_add_separate (dsym_bfd.get (), dsym_filename.c_str (),
>  				    symfile_flags, objfile);
>  
>  	  /* Don't try to read dwarf2 from main file or shared libraries.  */
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 58747d545b..f714845a6e 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -1789,8 +1789,6 @@ find_sym_fns (bfd *abfd)
>  static void
>  load_command (const char *arg, int from_tty)
>  {
> -  struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
> -
>    dont_repeat ();
>  
>    /* The user might be reloading because the binary has changed.  Take
> @@ -1798,40 +1796,28 @@ load_command (const char *arg, int from_tty)
>    reopen_exec_file ();
>    reread_symbols ();
>  
> +  std::string temp;
>    if (arg == NULL)
>      {
> -      const char *parg;
> -      int count = 0;
> +      const char *parg, *prev;
>  
> -      parg = arg = get_exec_file (1);
> +      arg = get_exec_file (1);
>  
> -      /* Count how many \ " ' tab space there are in the name.  */
> +      /* We may need to quote this string so buildargv can pull it
> +	 apart.  */
> +      prev = parg = arg;
>        while ((parg = strpbrk (parg, "\\\"'\t ")))
>  	{
> -	  parg++;
> -	  count++;
> +	  temp.append (prev, parg - prev);
> +	  prev = parg++;
> +	  temp.push_back ('\\');
>  	}
> -
> -      if (count)
> +      /* If we have not copied anything yet, then we didn't see a
> +	 character to quote, and we can just leave ARG unchanged.  */
> +      if (!temp.empty ())
>  	{
> -	  /* We need to quote this string so buildargv can pull it apart.  */
> -	  char *temp = (char *) xmalloc (strlen (arg) + count + 1 );
> -	  char *ptemp = temp;
> -	  const char *prev;
> -
> -	  make_cleanup (xfree, temp);
> -
> -	  prev = parg = arg;
> -	  while ((parg = strpbrk (parg, "\\\"'\t ")))
> -	    {
> -	      strncpy (ptemp, prev, parg - prev);
> -	      ptemp += parg - prev;
> -	      prev = parg++;
> -	      *ptemp++ = '\\';
> -	    }
> -	  strcpy (ptemp, prev);
> -
> -	  arg = temp;
> +	  temp.append (prev);
> +	  arg = temp.c_str ();
>  	}
>      }
>  
> @@ -1840,8 +1826,6 @@ load_command (const char *arg, int from_tty)
>    /* After re-loading the executable, we don't really know which
>       overlays are mapped any more.  */
>    overlay_cache_invalid = 1;
> -
> -  do_cleanups (cleanup);
>  }
>  
>  /* This version of "load" should be usable for any target.  Currently
> 

LGTM.  I would be more confident if that non-trivial escaping code was
unit-tested, but I tried it by hand and it seems to do what it's supposed
to do.

Simon
Tom Tromey - March 13, 2018, 11:40 p.m.
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> LGTM.  I would be more confident if that non-trivial escaping code was
Simon> unit-tested, but I tried it by hand and it seems to do what it's supposed
Simon> to do.

Yeah.  I looked at this briefly but it seemed like a lot of work for not
a huge benefit.  Maybe if some other code was doing the same thing, it
would make sense to pull this logic out into its own function.

Tom
Simon Marchi - March 14, 2018, 1:32 a.m.
On 2018-03-13 07:40 PM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> LGTM.  I would be more confident if that non-trivial escaping code was
> Simon> unit-tested, but I tried it by hand and it seems to do what it's supposed
> Simon> to do.
> 
> Yeah.  I looked at this briefly but it seemed like a lot of work for not
> a huge benefit.  Maybe if some other code was doing the same thing, it
> would make sense to pull this logic out into its own function.

Agreed.

Patch

diff --git a/gdb/machoread.c b/gdb/machoread.c
index b270675d61..ab4410c925 100644
--- a/gdb/machoread.c
+++ b/gdb/machoread.c
@@ -750,12 +750,12 @@  macho_symfile_read_all_oso (VEC (oso_el) **oso_vector_ptr,
 #define DSYM_SUFFIX ".dSYM/Contents/Resources/DWARF/"
 
 /* Check if a dsym file exists for OBJFILE.  If so, returns a bfd for it
-   and return *FILENAMEP with its original xmalloc-ated filename.
+   and return *FILENAMEP with its original filename.
    Return NULL if no valid dsym file is found (FILENAMEP is not used in
    such case).  */
 
 static gdb_bfd_ref_ptr
-macho_check_dsym (struct objfile *objfile, char **filenamep)
+macho_check_dsym (struct objfile *objfile, std::string *filenamep)
 {
   size_t name_len = strlen (objfile_name (objfile));
   size_t dsym_len = strlen (DSYM_SUFFIX);
@@ -804,7 +804,7 @@  macho_check_dsym (struct objfile *objfile, char **filenamep)
 	       objfile_name (objfile));
       return NULL;
     }
-  *filenamep = xstrdup (dsym_filename);
+  *filenamep = std::string (dsym_filename);
   return dsym_bfd;
 }
 
@@ -821,7 +821,7 @@  macho_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
      be in the executable.  */
   if (bfd_get_file_flags (abfd) & (EXEC_P | DYNAMIC))
     {
-      char *dsym_filename;
+      std::string dsym_filename;
 
       /* Process the normal symbol table first.  */
       storage_needed = bfd_get_symtab_upper_bound (objfile->obfd);
@@ -865,8 +865,6 @@  macho_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
 	{
           struct bfd_section *asect, *dsect;
 
-	  make_cleanup (xfree, dsym_filename);
-
 	  if (mach_o_debug_level > 0)
 	    printf_unfiltered (_("dsym file found\n"));
 
@@ -882,7 +880,7 @@  macho_symfile_read (struct objfile *objfile, symfile_add_flags symfile_flags)
             }
 
 	  /* Add the dsym file as a separate file.  */
-          symbol_file_add_separate (dsym_bfd.get (), dsym_filename,
+          symbol_file_add_separate (dsym_bfd.get (), dsym_filename.c_str (),
 				    symfile_flags, objfile);
 
 	  /* Don't try to read dwarf2 from main file or shared libraries.  */
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 58747d545b..f714845a6e 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1789,8 +1789,6 @@  find_sym_fns (bfd *abfd)
 static void
 load_command (const char *arg, int from_tty)
 {
-  struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
-
   dont_repeat ();
 
   /* The user might be reloading because the binary has changed.  Take
@@ -1798,40 +1796,28 @@  load_command (const char *arg, int from_tty)
   reopen_exec_file ();
   reread_symbols ();
 
+  std::string temp;
   if (arg == NULL)
     {
-      const char *parg;
-      int count = 0;
+      const char *parg, *prev;
 
-      parg = arg = get_exec_file (1);
+      arg = get_exec_file (1);
 
-      /* Count how many \ " ' tab space there are in the name.  */
+      /* We may need to quote this string so buildargv can pull it
+	 apart.  */
+      prev = parg = arg;
       while ((parg = strpbrk (parg, "\\\"'\t ")))
 	{
-	  parg++;
-	  count++;
+	  temp.append (prev, parg - prev);
+	  prev = parg++;
+	  temp.push_back ('\\');
 	}
-
-      if (count)
+      /* If we have not copied anything yet, then we didn't see a
+	 character to quote, and we can just leave ARG unchanged.  */
+      if (!temp.empty ())
 	{
-	  /* We need to quote this string so buildargv can pull it apart.  */
-	  char *temp = (char *) xmalloc (strlen (arg) + count + 1 );
-	  char *ptemp = temp;
-	  const char *prev;
-
-	  make_cleanup (xfree, temp);
-
-	  prev = parg = arg;
-	  while ((parg = strpbrk (parg, "\\\"'\t ")))
-	    {
-	      strncpy (ptemp, prev, parg - prev);
-	      ptemp += parg - prev;
-	      prev = parg++;
-	      *ptemp++ = '\\';
-	    }
-	  strcpy (ptemp, prev);
-
-	  arg = temp;
+	  temp.append (prev);
+	  arg = temp.c_str ();
 	}
     }
 
@@ -1840,8 +1826,6 @@  load_command (const char *arg, int from_tty)
   /* After re-loading the executable, we don't really know which
      overlays are mapped any more.  */
   overlay_cache_invalid = 1;
-
-  do_cleanups (cleanup);
 }
 
 /* This version of "load" should be usable for any target.  Currently