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

Simon Marchi
2018-03-13T02:37:47Z

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