Re: [RFA] Use gdb::unique_xmalloc_ptr when calling tilde_expand

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2017-08-04 15:44, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> The patch looks good to me.  I noted two formatting nits (that 
> were
> Simon> present before this patch), could you fix them before pushing?
> 
> I did this.  I don't really like fixing random things in the context of
> a patch.  Normally in fact I would expect patches containing unrelated
> changes like that to be rejected on the basis of not being
> single-purpose.

I agree with you on the idea, we just have different criteria about what 
unrelated means.  In this case since you're touching the source line 
inside, I would say that it's ok to fix the surrounding formatting at 
the same time.  Anyhow, not a big deal.

> Simon> Hmm, the old code discarded the cleanup and did an xstrdup, that 
> seems
> Simon> like a leak that you fixed :)
> 
> Yeah.  I found another leak as well, but I haven't sent the patch for
> that yet.
> 
> I noticed I sent the wrong patch -- it didn't include all the changes 
> to
> solib.c.  So, I'm re-sending it rather than checking it in.  The
> additional changes were part of the buildbot try run though; they were
> just an earlier commit I forgot to squash.

Ok, I assume everything except solib.c is unchanged, so I'll just take a 
look at solib.c.

> diff --git a/gdb/solib.c b/gdb/solib.c
> index 5b538eb..195183d 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -546,14 +546,10 @@ static int
>  solib_map_sections (struct so_list *so)
>  {
>    const struct target_so_ops *ops = solib_ops (target_gdbarch ());
> -  char *filename;
>    struct target_section *p;
> -  struct cleanup *old_chain;
> 
> -  filename = tilde_expand (so->so_name);
> -  old_chain = make_cleanup (xfree, filename);
> -  gdb_bfd_ref_ptr abfd (ops->bfd_open (filename));
> -  do_cleanups (old_chain);
> +  gdb::unique_xmalloc_ptr<char> filename (tilde_expand (so->so_name));
> +  gdb_bfd_ref_ptr abfd (ops->bfd_open (filename.get ()));
> 
>    if (abfd == NULL)
>      return 0;
> @@ -1301,23 +1297,22 @@ static void
>  reload_shared_libraries_1 (int from_tty)
>  {
>    struct so_list *so;
> -  struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
> 
>    if (print_symbol_loading_p (from_tty, 0, 0))
>      printf_unfiltered (_("Loading symbols for shared libraries.\n"));
> 
>    for (so = so_list_head; so != NULL; so = so->next)
>      {
> -      char *filename, *found_pathname = NULL;
> +      char *found_pathname = NULL;
>        int was_loaded = so->symbols_loaded;
>        symfile_add_flags add_flags = SYMFILE_DEFER_BP_RESET;
> 
>        if (from_tty)
>  	add_flags |= SYMFILE_VERBOSE;
> 
> -      filename = tilde_expand (so->so_original_name);
> -      make_cleanup (xfree, filename);
> -      gdb_bfd_ref_ptr abfd (solib_bfd_open (filename));
> +      gdb::unique_xmalloc_ptr<char> filename
> +	(tilde_expand (so->so_original_name));
> +      gdb_bfd_ref_ptr abfd (solib_bfd_open (filename.get ()));
>        if (abfd != NULL)
>  	{
>  	  found_pathname = xstrdup (bfd_get_filename (abfd.get ()));
> @@ -1364,8 +1359,6 @@ reload_shared_libraries_1 (int from_tty)
>  	      solib_read_symbols (so, add_flags);
>  	}
>      }
> -
> -  do_cleanups (old_chain);

There is still a cleanup in reload_shared_libraries_1, so I don't think 
the do_cleanups should be removed.

Simon