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

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Wed, 18 Jul 2012 21:31:59 +0200, Tom Tromey wrote:
> --- /dev/null
> +++ b/gdb/gdb_bfd.c
> @@ -0,0 +1,90 @@
[...]
> +/* Add reference to ABFD.  Returns ABFD.  */
> +
> +struct bfd *
> +gdb_bfd_ref (struct bfd *abfd)
[...]
> +/* Unreference and possibly close ABFD.  */
> +
> +void
> +gdb_bfd_unref (struct bfd *abfd)
[...]
> --- /dev/null
> +++ b/gdb/gdb_bfd.h
> @@ -0,0 +1,35 @@
[...]
> +/* Acquire a new reference to ABFD.  Returns ABFD for convenience.
> +   It is fine for ABFD to be NULL; in this case the function does
> +   nothing and returns NULL.  */
> +
> +struct bfd *gdb_bfd_ref (struct bfd *abfd);
> +
> +/* Release a reference to 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);

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


> diff --git a/gdb/jit.c b/gdb/jit.c
> index 568d17b..6a1425c 100644
> --- a/gdb/jit.c
> +++ b/gdb/jit.c
> @@ -861,6 +862,7 @@ jit_bfd_try_read_symtab (struct jit_code_entry *code_entry,
>        puts_unfiltered (_("Error opening JITed symbol file, ignoring it.\n"));
>        return;
>      }
> +  nbfd = gdb_bfd_ref (nbfd);

I personally disagree about the returned value from gdb_bfd_ref being useful,
it makes the code more magic IMO (plus it is not much compatible with the
narrow GNU Coding Standard code formatting).  But when it is already this way
then this gdb_bfd_ref wrapped should have been around
bfd_open_from_target_memory above as gdb_bfd_ref can handle NULL parameter.


> @@ -193,9 +194,9 @@ allocate_objfile (bfd *abfd, int flags)
>  
>    /* Update the per-objfile information that comes from the bfd, ensuring
>       that any data that is reference is saved in the per-objfile data
> -     region.  */
> +     region.  Note that we steal a reference to ABFD.  */
>  
> -  objfile->obfd = gdb_bfd_ref (abfd);
> +  objfile->obfd = abfd;

Caller could gdb_bfd_unref its reference but YMMV.


> @@ -794,10 +795,10 @@ add_vmap (LdInfo *ldi)
>      {
>        warning (_("\"%s\": not in executable format: %s."),
>  	       objname, bfd_errmsg (bfd_get_error ()));
> -      bfd_close (abfd);
> +      gdb_bfd_unref (abfd);
>        return NULL;
>      }
> -  obj = allocate_objfile (vp->bfd, 0);
> +  obj = allocate_objfile (gdb_bfd_ref (vp->bfd), 0);

This code is a bit magic due to it.  map_vmap is missing gdb_bfd_ref despite
it creates new reference etc.

This all pain would not exist with C++.


> @@ -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);

Why isn't gdb_bfd_ref missing here?


>  	  /* bfd_openr sets cacheable to true, which is what we want.  */
>  	  if (!bfd_check_format (objfile->obfd, bfd_object))
>  	    error (_("Can't read symbols from %s: %s."), objfile->name,


Thanks,
Jan