Re: Solibs and objfile BFD ownership

[Paul Pluzhnikov <ppluzhnikov@google.com>]

[-- Attachment #1: Type: text/plain, Size: 1196 bytes --]

On Wed, Aug 19, 2009 at 3:11 PM, Tom Tromey<tromey@redhat.com> wrote:

> Paul>      *p_refcount = 2;  /* Both solib and objfile refer to this abfd.  */
> Paul>      bfd_usrdata (so->abfd) = p_refcount;
>
> I haven't had time to read this patch fully and try to understand it.
>
> It just struck me from a first reading that it would be simpler, and
> more conventional, to have a "gdb_bfd_ref" function, to go along with
> gdb_bfd_unref, which we call whenever we assign a 'bfd*' to a field
> somewhere.

It would be slightly wasteful to set refcount to 1 everywhere, since solib
is the only place where sharing could (currently) occur.

But maybe it *is* a good idea anyway. Here is a proposed fix.
Tested on Linux/x86_64 with no new failures. Also tested to fix the crash
I observed.

Thanks,
-- 
Paul Pluzhnikov

2009-08-19  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* objfiles.h (gdb_bfd_ref): New prototype.
	* objfiles.c (gdb_bfd_ref): New function.
	(allocate_objfile): Call it.
	(gdb_bfd_unref): Adjust assertion.
	* solib.c (solib_map_sections): Add reference.
	(symbol_add_stub): Don't add reference here.
	* symfile.c (reread_symbols): Add reference.

[-- Attachment #2: gdb-bfd-ownership-20090819.txt --]
[-- Type: text/plain, Size: 4362 bytes --]

Index: objfiles.h
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.h,v
retrieving revision 1.61
diff -u -p -u -r1.61 objfiles.h
--- objfiles.h	4 Aug 2009 18:46:05 -0000	1.61
+++ objfiles.h	19 Aug 2009 23:30:43 -0000
@@ -505,6 +505,7 @@ extern void set_objfile_data (struct obj
 extern void *objfile_data (struct objfile *objfile,
 			   const struct objfile_data *data);
 
+extern struct bfd *gdb_bfd_ref (struct bfd *abfd);
 extern void gdb_bfd_unref (struct bfd *abfd);
 \f
 
Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.92
diff -u -p -u -r1.92 objfiles.c
--- objfiles.c	17 Aug 2009 11:16:13 -0000	1.92
+++ objfiles.c	19 Aug 2009 23:30:43 -0000
@@ -181,7 +181,7 @@ allocate_objfile (bfd *abfd, int flags)
      that any data that is reference is saved in the per-objfile data
      region. */
 
-  objfile->obfd = abfd;
+  objfile->obfd = gdb_bfd_ref (abfd);
   if (objfile->name != NULL)
     {
       xfree (objfile->name);
@@ -1062,7 +1062,26 @@ objfiles_changed (void)
   objfiles_changed_p = 1;  /* Rebuild section map next time we need it.  */
 }
 
-/* Unreference and possibly close abfd.  */
+/* Add reference to ABFD.  Returns ABFD.  */
+struct bfd *
+gdb_bfd_ref (struct bfd *abfd)
+{
+  int *p_refcount = bfd_usrdata (abfd);
+
+  if (p_refcount != NULL)
+    {
+      *p_refcount += 1;
+      return abfd;
+    }
+
+  p_refcount = xmalloc (sizeof (*p_refcount));
+  *p_refcount = 1;
+  bfd_usrdata (abfd) = p_refcount;
+
+  return abfd;
+}
+
+/* Unreference and possibly close ABFD.  */
 void
 gdb_bfd_unref (struct bfd *abfd)
 {
@@ -1074,16 +1093,14 @@ gdb_bfd_unref (struct bfd *abfd)
 
   p_refcount = bfd_usrdata (abfd);
 
-  /* Valid range for p_refcount: NULL (single owner), or a pointer
-     to int counter, which has a value of 1 (single owner) or 2 (shared).  */
-  gdb_assert (p_refcount == NULL || *p_refcount == 1 || *p_refcount == 2);
+  /* Valid range for p_refcount: a pointer to int counter, which has a
+     value of 1 (single owner) or 2 (shared).  */
+  gdb_assert (*p_refcount == 1 || *p_refcount == 2);
+
+  *p_refcount -= 1;
+  if (*p_refcount > 0)
+    return;
 
-  if (p_refcount != NULL)
-    {
-      *p_refcount -= 1;
-      if (*p_refcount > 0)
-	return;
-    }
   xfree (p_refcount);
   bfd_usrdata (abfd) = NULL;  /* Paranoia.  */
 
Index: solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.124
diff -u -p -u -r1.124 solib.c
--- solib.c	10 Aug 2009 22:09:22 -0000	1.124
+++ solib.c	19 Aug 2009 23:30:44 -0000
@@ -361,7 +361,7 @@ solib_map_sections (void *arg)
   do_cleanups (old_chain);
 
   /* Leave bfd open, core_xfer_memory and "info files" need it.  */
-  so->abfd = abfd;
+  so->abfd = gdb_bfd_ref (abfd);
 
   /* copy full path name into so_name, so that later symbol_file_add
      can find it */
@@ -444,7 +444,6 @@ static void
 symbol_add_stub (struct so_list *so, int flags)
 {
   struct section_addr_info *sap;
-  int *p_refcount;
 
   /* Have we already loaded this shared object?  */
   ALL_OBJFILES (so->objfile)
@@ -457,10 +456,6 @@ symbol_add_stub (struct so_list *so, int
                                                     so->sections_end);
 
   so->objfile = symbol_file_add_from_bfd (so->abfd, flags, sap, OBJF_SHARED);
-  p_refcount = xmalloc (sizeof (*p_refcount));
-  *p_refcount = 2;  /* Both solib and objfile refer to this abfd.  */
-  bfd_usrdata (so->abfd) = p_refcount;
-
   free_section_addr_info (sap);
 
   return;
Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.243
diff -u -p -u -r1.243 symfile.c
--- symfile.c	17 Aug 2009 20:09:38 -0000	1.243
+++ symfile.c	19 Aug 2009 23:30:45 -0000
@@ -2333,6 +2333,8 @@ reread_symbols (void)
 		objfile->obfd = bfd_openr (obfd_filename, gnutarget);
 	      if (objfile->obfd == NULL)
 		error (_("Can't open %s to read symbols."), objfile->name);
+	      else
+		objfile->obfd = gdb_bfd_ref (objfile->obfd);
 	      /* 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,