Re: [PATCH 01/10] change gdb to refcount bfd everywhere

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> The comments are present in both and neither is a reference, they
Jan> are already out of sync.

I don't really follow, but I also don't mind changing the comments --
they ought to be clear for everybody, not just me.  How about this?

/* Increment the reference count of ABFD.  It is fine for ABFD to be
   NULL; in this case the function does nothing.  */

void gdb_bfd_ref (struct bfd *abfd);

/* Decrement the reference count of ABFD.  If this is the last
   reference, ABFD will be freed.  If ABFD is NULL, this function does
   nothing.  */

void gdb_bfd_unref (struct bfd *abfd);


Jan> I personally disagree about the returned value from gdb_bfd_ref
Jan> being useful, it makes the code more magic IMO (plus it is not much
Jan> compatible with the narrow GNU Coding Standard code formatting).

I made this change.  I'll test the patch and send it tomorrow.

Then I went a bit further and wrote a follow-up patch that wraps the
various BFD-opening functions with gdb equivalents.  These initialize
the refcount and stash the filename.

E.g.:

    /* A wrapper for bfd_fopen that initializes the gdb-specific reference
       count and calls gdb_bfd_stash_filename.  */

    bfd *gdb_bfd_fopen (const char *, const char *, const char *, int);

I'll send this patch separately tomorrow.  I'm curious to know what you
think.  I find it cleans up the code quite a bit, I wish I'd done this
from the beginning.

>> @@ -193,9 +194,9 @@ allocate_objfile (bfd *abfd, int flags)
[...]
>> -  objfile->obfd = gdb_bfd_ref (abfd);
>> +  objfile->obfd = abfd;

Jan> Caller could gdb_bfd_unref its reference but YMMV.

I will look into this tomorrow.

Jan> This all pain would not exist with C++.

Yeah, well...

>> @@ -2519,14 +2512,10 @@ reread_symbols (void)
>> to close the descriptor but BFD lacks a way of closing the
>> BFD without closing the descriptor.  */
>> obfd_filename = bfd_get_filename (objfile->obfd);
>> -	  if (!bfd_close (objfile->obfd))
>> -	    error (_("Can't close BFD for %s: %s"), objfile->name,
>> -		   bfd_errmsg (bfd_get_error ()));
>> +	  gdb_bfd_unref (objfile->obfd);
>> objfile-> obfd = bfd_open_maybe_remote (obfd_filename);
>> if (objfile->obfd == NULL)
>> error (_("Can't open %s to read symbols."), objfile->name);
>> -	  else
>> -	    objfile->obfd = gdb_bfd_ref (objfile->obfd);

Jan> Why isn't gdb_bfd_ref missing here?

bfd_open_maybe_remote returns a new reference.

I wonder if I should rename it to gdb_bfd_open_maybe_remote, for
consistency.  What do you think?  I'm inclined to do it.

Tom