[RFA] Remove two cleanups using std::string

Message ID 20180312223829.7108-1-tom@tromey.com
State New
Headers show
Series
  • [RFA] Remove two cleanups using std::string
Related show

Commit Message

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(-)

-- 
2.13.6

Comments

Simon Marchi March 13, 2018, 2:37 a.m. | #1
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. | #2
>>>>> "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. | #3
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