[RFC] Fix BFD leak in solib-darwin.c

  commit 192b62ce0b4bb5c61188f570e127a26d2c32f716 ("Use class to manage
BFD reference counts") changed darwin_get_dyld_bfd to use:

+	dyld_bfd.release ();

rather than

-      do_cleanups (cleanup);

However, using release here leaks the BFD.  Instead I believe reset
should be used instead.

I can't readily test this, so please take a look and let me know what
you think.

gdb/ChangeLog
2019-02-22  Tom Tromey  <tromey@adacore.com>

	* solib-darwin.c (darwin_get_dyld_bfd): Use reset, not release.
---
 gdb/ChangeLog      | 4 ++++
 gdb/solib-darwin.c | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)
  

On 2/22/19 8:36 AM, Tom Tromey wrote:
> commit 192b62ce0b4bb5c61188f570e127a26d2c32f716 ("Use class to manage
> BFD reference counts") changed darwin_get_dyld_bfd to use:
> 
> +	dyld_bfd.release ();
> 
> rather than
> 
> -      do_cleanups (cleanup);
> 
> However, using release here leaks the BFD.  Instead I believe reset
> should be used instead.
> 
> I can't readily test this, so please take a look and let me know what
> you think.
> 
> gdb/ChangeLog
> 2019-02-22  Tom Tromey  <tromey@adacore.com>
> 
> 	* solib-darwin.c (darwin_get_dyld_bfd): Use reset, not release.
> ---
>  gdb/ChangeLog      | 4 ++++
>  gdb/solib-darwin.c | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
> index d3060604bad..da410ad4660 100644
> --- a/gdb/solib-darwin.c
> +++ b/gdb/solib-darwin.c
> @@ -455,7 +455,7 @@ darwin_get_dyld_bfd ()
>        if (sub != NULL)
>  	dyld_bfd = sub;
>        else
> -	dyld_bfd.release ();
> +	dyld_bfd.reset (nullptr);
>      }
>    return dyld_bfd;
>  }

I haven't tested, but this LGTM.  Simon has noticed several instances of
this bug where release() was used instead of reset().  (The most recent one
was in the build-id separate debug file code that I think hasn't been
committed yet.)  It might be worth doing a quick sweep of current 'release'
calls (if there aren't too many) to check for other leaks.
  

On 02/22/2019 04:36 PM, Tom Tromey wrote:
>        if (sub != NULL)
>  	dyld_bfd = sub;
>        else
> -	dyld_bfd.release ();
> +	dyld_bfd.reset (nullptr);

These 4 lines are now equivalent to just doing:

   dyld_bfd = sub;

Thanks,
Pedro Alves
  

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> On 02/22/2019 04:36 PM, Tom Tromey wrote:
>> if (sub != NULL)
>> dyld_bfd = sub;
>> else
>> -	dyld_bfd.release ();
>> +	dyld_bfd.reset (nullptr);

Pedro> These 4 lines are now equivalent to just doing:

Pedro>    dyld_bfd = sub;

I made this change and I'm going to commit it shortly.

Tom
  

>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

Re-replying...

John> Simon has noticed several instances of this bug where release()
John> was used instead of reset().

I wonder whether we ought to mark the release method as
"warn_unused_result" to try to avoid future bugs like this.  Another
idea is to try to reduce the number of calls to release generally, by
changing more things to take rvalue references or the like.

Tom
  

On 2/26/19 12:18 PM, Tom Tromey wrote:
>>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:
> 
> Re-replying...
> 
> John> Simon has noticed several instances of this bug where release()
> John> was used instead of reset().
> 
> I wonder whether we ought to mark the release method as
> "warn_unused_result" to try to avoid future bugs like this.  Another
> idea is to try to reduce the number of calls to release generally, by
> changing more things to take rvalue references or the like.

I think the warning would be useful if it doesn't trigger a lot of false
positives.  I feel like it wouldn't today.  When I was grep'ing for
'release ()' I only focused on the calls that weren't assigning a value
to a result and I feel like there weren't many of those (and some were
assigning, just not obvious in the one line of grep context).
  

>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

John> I think the warning would be useful if it doesn't trigger a lot of false
John> positives.  I feel like it wouldn't today.  When I was grep'ing for
John> 'release ()' I only focused on the calls that weren't assigning a value
John> to a result and I feel like there weren't many of those (and some were
John> assigning, just not obvious in the one line of grep context).

Yeah.  I went ahead and did this... I had to fix up a few spots, nothing
too terrible.  It caught one bug, in varobj.c:

      if (pretty_printer == Py_None)
	pretty_printer.release ();

      install_visualizer (var->dynamic, NULL, pretty_printer.release ());

I think that first release should be .reset(nullptr) instead.

I'll send it through the buildbot and see what happens there.

Tom