Solibs and objfile BFD ownership

Solibs and objfile BFD ownership

[Daniel Jacobowitz]

I'm seeing ARM test failures caused by this patch:

2009-03-05  Paul Pluzhnikov  <ppluzhnikov@google.com>

        * printcmd.c (do_one_display): Reparse exp_string.
        (display_uses_solib_p): New function.
        (clear_dangling_display_expressions): New function.
        (_initialize_printcmd): Add observer.
        * solib.c (no_shared_libraries): Swap order of calls to
        clear_solib and objfile_purge_solibs.

Specifically this bit:

  /* The order of the two routines below is important: clear_solib notifies
     the solib_unloaded observers, and some of these observers might need
     access to their associated objfiles.  Therefore, we can not purge the
     solibs' objfiles before clear_solib has been called.  */

  clear_solib ();
  objfile_purge_solibs ();

The problem with doing things in this order is that both the solib and
the objfile have a reference to the same BFD.  The solib is
responsible for releasing it (OBJF_KEEPBFD).  arm_objfile_data_cleanup
accesses objfile->obfd during free_objfile, to find the number
of sections; at that point it's already been free'd.

Any thoughts?  I could change the ARM code to work around this, but
that means banning access to objfile->obfd during a not well specified
range of functions.  Handing off the ownership of the BFD seems like
it would be messy.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Solibs and objfile BFD ownership

[Paul Pluzhnikov]

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

On Tue, Jul 28, 2009 at 8:40 AM, Daniel Jacobowitz<drow@false.org> wrote:

>  clear_solib ();
>  objfile_purge_solibs ();
>
> The problem with doing things in this order is that both the solib and
> the objfile have a reference to the same BFD.  The solib is
> responsible for releasing it (OBJF_KEEPBFD).  arm_objfile_data_cleanup
> accesses objfile->obfd during free_objfile, to find the number
> of sections; at that point it's already been free'd.

That is a problem.

> Any thoughts?

There doesn't appear to be a correct order of destruction :-(

Attached patch restores the original order, while still keeping
solib_unloaded observers from accessing danglig objfiles.

I do not know if there is a better way to fix this.

Tested on Linux/x86_64 with no new regressions.

Thanks,
-- 
Paul Pluzhnikov

2009-07-29  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* solib.c (clear_solib): Rename to clear_solib_1
	(clear_solib): New function.
	(no_shared_libraries): Adjust.

[-- Attachment #2: gdb-clear-solib-20090729.txt --]
[-- Type: text/plain, Size: 2102 bytes --]

Index: solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.122
diff -u -p -u -r1.122 solib.c
--- solib.c	17 Jul 2009 17:08:23 -0000	1.122
+++ solib.c	29 Jul 2009 23:39:11 -0000
@@ -893,10 +893,8 @@ solib_name_from_address (CORE_ADDR addre
   return (0);
 }
 
-/* Called by free_all_symtabs */
-
-void
-clear_solib (void)
+static void
+clear_solib_1 (int notify_observers)
 {
   struct target_so_ops *ops = solib_ops (target_gdbarch);
 
@@ -928,7 +926,8 @@ clear_solib (void)
     {
       struct so_list *so = so_list_head;
       so_list_head = so->next;
-      observer_notify_solib_unloaded (so);
+      if (notify_observers)
+        observer_notify_solib_unloaded (so);
       if (so->abfd)
 	remove_target_sections (so->abfd);
       free_so (so);
@@ -937,6 +936,15 @@ clear_solib (void)
   ops->clear_solib ();
 }
 
+/* Called by free_all_symtabs */
+
+void
+clear_solib (void)
+{
+  clear_solib_1 (1);
+}
+
+
 /* GLOBAL FUNCTION
 
    solib_create_inferior_hook -- shared library startup support
@@ -1018,13 +1026,20 @@ sharedlibrary_command (char *args, int f
 void
 no_shared_libraries (char *ignored, int from_tty)
 {
-  /* The order of the two routines below is important: clear_solib notifies
-     the solib_unloaded observers, and some of these observers might need
-     access to their associated objfiles.  Therefore, we can not purge the
-     solibs' objfiles before clear_solib has been called.  */
+  struct so_list *so = so_list_head;
+
+  /* We notify all solib_unloaded observers before destroying any objfiles,
+     because some observers need access to their associated objfiles.  */
+
+  for (so = so_list_head; so; so = so->next)
+    observer_notify_solib_unloaded (so);
+
+  /* The order of the two routines below is important: clear_solib_1
+     will free/close objfile->obfd (solib owns it), but objfile_purge_solibs
+     needs access to it (at least on ARM, see arm_objfile_data_cleanup).  */
 
-  clear_solib ();
   objfile_purge_solibs ();
+  clear_solib_1 (0);
 }
 
 static void

Re: Solibs and objfile BFD ownership

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> There doesn't appear to be a correct order of destruction :-(
[...]
Paul> I do not know if there is a better way to fix this.

It seems to me that there are not many choices.  Either we must pick a
single owner, pick an order, implement something like reference counting
for BFDs, or have a separate function to clean up the BFDs.

Tom

Re: Solibs and objfile BFD ownership

[Paul Pluzhnikov]

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

On Thu, Jul 30, 2009 at 9:15 AM, Tom Tromey<tromey@redhat.com> wrote:

> It seems to me that there are not many choices.  Either we must pick a
> single owner, pick an order, implement something like reference counting
> for BFDs, or have a separate function to clean up the BFDs.

Right.

Conveniently, 'struct bfd' has usrdata, which is unused in GDB.
Here is a stab at reference-counted sharing of BFDs between solib and
objfile.

-- 
Paul Pluzhnikov


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

	* objfiles.h (OBJF_KEEPBFD): Delete.
	(gdb_bfd_unref): New prototype.
	* objfiles.c (gdb_bfd_unref): New function.
	(free_objfile): Call gdb_bfd_unref.
	* solib.c (free_so): Likewise.
	(symbol_add_stub): Set refcount.

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

Index: objfiles.h
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.h,v
retrieving revision 1.60
diff -u -p -u -r1.60 objfiles.h
--- objfiles.h	22 Jul 2009 19:21:31 -0000	1.60
+++ objfiles.h	4 Aug 2009 00:40:55 -0000
@@ -414,12 +414,6 @@ struct objfile
 
 #define OBJF_USERLOADED	(1 << 3)	/* User loaded */
 
-/* The bfd of this objfile is used outside of the objfile (e.g. by solib).
-   Do not try to free it.  */
-
-#define OBJF_KEEPBFD	(1 << 4)	/* Do not delete bfd */
-
-
 /* The object file that the main symbol table was loaded from (e.g. the
    argument to the "symbol-file" or "file" command).  */
 
@@ -510,6 +504,8 @@ extern void set_objfile_data (struct obj
 			      const struct objfile_data *data, void *value);
 extern void *objfile_data (struct objfile *objfile,
 			   const struct objfile_data *data);
+
+extern void gdb_bfd_unref (struct bfd *abfd);
 \f
 
 /* Traverse all object files.  ALL_OBJFILES_SAFE works even if you delete
Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.88
diff -u -p -u -r1.88 objfiles.c
--- objfiles.c	28 Jul 2009 16:39:06 -0000	1.88
+++ objfiles.c	4 Aug 2009 00:40:55 -0000
@@ -452,16 +452,7 @@ free_objfile (struct objfile *objfile)
   /* Discard any data modules have associated with the objfile.  */
   objfile_free_data (objfile);
 
-  /* We always close the bfd, unless the OBJF_KEEPBFD flag is set.  */
-
-  if (objfile->obfd != NULL && !(objfile->flags & OBJF_KEEPBFD))
-    {
-      char *name = bfd_get_filename (objfile->obfd);
-      if (!bfd_close (objfile->obfd))
-	warning (_("cannot close \"%s\": %s"),
-		 name, bfd_errmsg (bfd_get_error ()));
-      xfree (name);
-    }
+  gdb_bfd_unref (objfile->obfd);
 
   /* Remove it from the chain of all objfiles. */
 
@@ -1020,3 +1011,31 @@ objfiles_changed (void)
 {
   objfiles_changed_p = 1;  /* Rebuild section map next time we need it.  */
 }
+
+/* Unreference and possibly close abfd.  */
+void
+gdb_bfd_unref (struct bfd *abfd)
+{
+  int *p_refcount;
+  char *name;
+
+  if (abfd == NULL)
+    return;
+
+  p_refcount = abfd->usrdata;
+  if (p_refcount != NULL)
+    {
+      gdb_assert (*p_refcount > 0);
+      *p_refcount -= 1;
+      if (*p_refcount > 0)
+	return;
+    }
+  xfree (p_refcount);
+  abfd->usrdata = NULL;  /* Paranoia.  */
+
+  name = bfd_get_filename (abfd);
+  if (!bfd_close (abfd))
+    warning (_("cannot close \"%s\": %s"),
+	     name, bfd_errmsg (bfd_get_error ()));
+  xfree (name);
+}
Index: solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.122
diff -u -p -u -r1.122 solib.c
--- solib.c	17 Jul 2009 17:08:23 -0000	1.122
+++ solib.c	4 Aug 2009 00:40:55 -0000
@@ -421,21 +421,11 @@ void
 free_so (struct so_list *so)
 {
   struct target_so_ops *ops = solib_ops (target_gdbarch);
-  char *bfd_filename = 0;
 
   if (so->sections)
     xfree (so->sections);
-      
-  if (so->abfd)
-    {
-      bfd_filename = bfd_get_filename (so->abfd);
-      if (! bfd_close (so->abfd))
-	warning (_("cannot close \"%s\": %s"),
-		 bfd_filename, bfd_errmsg (bfd_get_error ()));
-    }
 
-  if (bfd_filename)
-    xfree (bfd_filename);
+  gdb_bfd_unref (so->abfd);
 
   ops->free_so (so);
 
@@ -454,6 +444,7 @@ 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)
@@ -465,8 +456,11 @@ symbol_add_stub (struct so_list *so, int
   sap = build_section_addr_info_from_section_table (so->sections,
                                                     so->sections_end);
 
-  so->objfile = symbol_file_add_from_bfd (so->abfd, flags,
-					  sap, OBJF_SHARED | OBJF_KEEPBFD);
+  so->objfile = symbol_file_add_from_bfd (so->abfd, flags, sap, OBJF_SHARED);
+  p_refcount = malloc (sizeof (*p_refcount));
+  *p_refcount = 2;  /* Both solib and objfile refer to this abfd.  */
+  so->abfd->usrdata = p_refcount;
+
   free_section_addr_info (sap);
 
   return;

Re: Solibs and objfile BFD ownership

[Daniel Jacobowitz]

On Mon, Aug 03, 2009 at 05:49:54PM -0700, Paul Pluzhnikov wrote:
> Conveniently, 'struct bfd' has usrdata, which is unused in GDB.
> Here is a stab at reference-counted sharing of BFDs between solib and
> objfile.

I can't see a better solution, so let's use this.

A comment somewhere about the usage of the usrdata field would be
nice.  I gather that NULL and 1 behave the same?

> +  p_refcount = malloc (sizeof (*p_refcount));

xmalloc

-- 
Daniel Jacobowitz
CodeSourcery

Re: Solibs and objfile BFD ownership

[Paul Pluzhnikov]

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

On Tue, Aug 4, 2009 at 7:53 AM, Daniel Jacobowitz<drow@false.org> wrote:

> A comment somewhere about the usage of the usrdata field would be
> nice.  I gather that NULL and 1 behave the same?

Yes. I added comment to gdb_bfd_unref.
Updated patch attached. Tested on Linux/x86_64 with no new failures.

Daniel, could you confirm that this fixes the ARM failures?

Thanks,
-- 
Paul Pluzhnikov

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

       * objfiles.h (OBJF_KEEPBFD): Delete.
       (gdb_bfd_unref): New prototype.
       * objfiles.c (gdb_bfd_unref): New function.
       (free_objfile): Call gdb_bfd_unref.
       * solib.c (free_so): Likewise.
       (symbol_add_stub): Set refcount.

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

Index: objfiles.h
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.h,v
retrieving revision 1.60
diff -u -p -u -r1.60 objfiles.h
--- objfiles.h	22 Jul 2009 19:21:31 -0000	1.60
+++ objfiles.h	4 Aug 2009 17:22:33 -0000
@@ -414,12 +414,6 @@ struct objfile
 
 #define OBJF_USERLOADED	(1 << 3)	/* User loaded */
 
-/* The bfd of this objfile is used outside of the objfile (e.g. by solib).
-   Do not try to free it.  */
-
-#define OBJF_KEEPBFD	(1 << 4)	/* Do not delete bfd */
-
-
 /* The object file that the main symbol table was loaded from (e.g. the
    argument to the "symbol-file" or "file" command).  */
 
@@ -510,6 +504,8 @@ extern void set_objfile_data (struct obj
 			      const struct objfile_data *data, void *value);
 extern void *objfile_data (struct objfile *objfile,
 			   const struct objfile_data *data);
+
+extern void gdb_bfd_unref (struct bfd *abfd);
 \f
 
 /* Traverse all object files.  ALL_OBJFILES_SAFE works even if you delete
Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.88
diff -u -p -u -r1.88 objfiles.c
--- objfiles.c	28 Jul 2009 16:39:06 -0000	1.88
+++ objfiles.c	4 Aug 2009 17:22:33 -0000
@@ -452,16 +452,7 @@ free_objfile (struct objfile *objfile)
   /* Discard any data modules have associated with the objfile.  */
   objfile_free_data (objfile);
 
-  /* We always close the bfd, unless the OBJF_KEEPBFD flag is set.  */
-
-  if (objfile->obfd != NULL && !(objfile->flags & OBJF_KEEPBFD))
-    {
-      char *name = bfd_get_filename (objfile->obfd);
-      if (!bfd_close (objfile->obfd))
-	warning (_("cannot close \"%s\": %s"),
-		 name, bfd_errmsg (bfd_get_error ()));
-      xfree (name);
-    }
+  gdb_bfd_unref (objfile->obfd);
 
   /* Remove it from the chain of all objfiles. */
 
@@ -1020,3 +1011,35 @@ objfiles_changed (void)
 {
   objfiles_changed_p = 1;  /* Rebuild section map next time we need it.  */
 }
+
+/* Unreference and possibly close abfd.  */
+void
+gdb_bfd_unref (struct bfd *abfd)
+{
+  int *p_refcount;
+  char *name;
+
+  if (abfd == NULL)
+    return;
+
+  p_refcount = abfd->usrdata;
+
+  /* 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);
+
+  if (p_refcount != NULL)
+    {
+      *p_refcount -= 1;
+      if (*p_refcount > 0)
+	return;
+    }
+  xfree (p_refcount);
+  abfd->usrdata = NULL;  /* Paranoia.  */
+
+  name = bfd_get_filename (abfd);
+  if (!bfd_close (abfd))
+    warning (_("cannot close \"%s\": %s"),
+	     name, bfd_errmsg (bfd_get_error ()));
+  xfree (name);
+}
Index: solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.122
diff -u -p -u -r1.122 solib.c
--- solib.c	17 Jul 2009 17:08:23 -0000	1.122
+++ solib.c	4 Aug 2009 17:22:33 -0000
@@ -421,21 +421,11 @@ void
 free_so (struct so_list *so)
 {
   struct target_so_ops *ops = solib_ops (target_gdbarch);
-  char *bfd_filename = 0;
 
   if (so->sections)
     xfree (so->sections);
-      
-  if (so->abfd)
-    {
-      bfd_filename = bfd_get_filename (so->abfd);
-      if (! bfd_close (so->abfd))
-	warning (_("cannot close \"%s\": %s"),
-		 bfd_filename, bfd_errmsg (bfd_get_error ()));
-    }
 
-  if (bfd_filename)
-    xfree (bfd_filename);
+  gdb_bfd_unref (so->abfd);
 
   ops->free_so (so);
 
@@ -454,6 +444,7 @@ 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)
@@ -465,8 +456,11 @@ symbol_add_stub (struct so_list *so, int
   sap = build_section_addr_info_from_section_table (so->sections,
                                                     so->sections_end);
 
-  so->objfile = symbol_file_add_from_bfd (so->abfd, flags,
-					  sap, OBJF_SHARED | OBJF_KEEPBFD);
+  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.  */
+  so->abfd->usrdata = p_refcount;
+
   free_section_addr_info (sap);
 
   return;

Re: Solibs and objfile BFD ownership

[Daniel Jacobowitz]

On Tue, Aug 04, 2009 at 10:37:24AM -0700, Paul Pluzhnikov wrote:
> On Tue, Aug 4, 2009 at 7:53 AM, Daniel Jacobowitz<drow@false.org> wrote:
> 
> > A comment somewhere about the usage of the usrdata field would be
> > nice.  I gather that NULL and 1 behave the same?
> 
> Yes. I added comment to gdb_bfd_unref.
> Updated patch attached. Tested on Linux/x86_64 with no new failures.
> 
> Daniel, could you confirm that this fixes the ARM failures?

Yes - looks much better with this patch!

> 2009-08-04  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
>        * objfiles.h (OBJF_KEEPBFD): Delete.
>        (gdb_bfd_unref): New prototype.
>        * objfiles.c (gdb_bfd_unref): New function.
>        (free_objfile): Call gdb_bfd_unref.
>        * solib.c (free_so): Likewise.
>        (symbol_add_stub): Set refcount.

OK.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Solibs and objfile BFD ownership

[Paul Pluzhnikov]

On Tue, Aug 4, 2009 at 11:40 AM, Daniel Jacobowitz<drow@false.org> wrote:

>> Daniel, could you confirm that this fixes the ARM failures?
> Yes - looks much better with this patch!

So committed. Thanks,
-- 
Paul Pluzhnikov

Re: Solibs and objfile BFD ownership

[Paul Pluzhnikov]

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

On Tue, Aug 4, 2009 at 11:47 AM, Paul Pluzhnikov<ppluzhnikov@google.com> wrote:

> So committed. Thanks,

I seem to have hit a rough patch with my patches :-(

The BFD refcounting patch from 2009-08-04 causes GDB to crash when I attach
to a process with many solibs, then (while GDB is reading solib symbols)
change my mind about attaching and hit Control-C, then 'run'.

This is happening because in symbol_add_stub refcount may not be set:

   so->objfile = symbol_file_add_from_bfd (so->abfd, flags, sap, OBJF_SHARED);

---> QUIT could be executed deep inside symbol_file_add_from_bfd, and
---> bfd_userdata below is never set.

   p_refcount = xmalloc (sizeof (*p_refcount));
   *p_refcount = 2;  /* Both solib and objfile refer to this abfd.  */
   bfd_usrdata (so->abfd) = p_refcount;

Later, we re-enter symbol_add_stub, and this:

   ALL_OBJFILES (so->objfile)
     {
       if (strcmp (so->objfile->name, so->so_name) == 0)
         return;

re-connects the so with the objfile, but never sets the bfd_usrdata.

Later still (during execution of 'run'), we go through clear_solib and
objfile_purge_solibs, and the latter crashes trying to bfd_close the abfd
which has already been bfd_close()d by the former.

Here is a proposed patch. Tested on Linux/x86_64 with no new failures.

Thanks,
-- 
Paul Pluzhnikov

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

	* solib.c (set_ref_count): New function.
	(symbol_add_stub): Call it.

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

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	17 Aug 2009 22:27:07 -0000
@@ -440,26 +440,53 @@ master_so_list (void)
   return so_list_head;
 }
 
+/* Set reference count on ABFD to COUNT.  */
+
+static void
+set_ref_count (struct bfd *abfd, int count)
+{
+  int *p_refcount = (int *) xmalloc (sizeof (*p_refcount));
+  *p_refcount = count;
+
+  gdb_assert (bfd_usrdata (abfd) == NULL);
+  bfd_usrdata (abfd) = p_refcount;
+}
+
 static void
 symbol_add_stub (struct so_list *so, int flags)
 {
   struct section_addr_info *sap;
-  int *p_refcount;
+  struct objfile *objfile;
 
   /* Have we already loaded this shared object?  */
-  ALL_OBJFILES (so->objfile)
+  ALL_OBJFILES (objfile)
     {
-      if (strcmp (so->objfile->name, so->so_name) == 0)
-	return;
+      if (strcmp (objfile->name, so->so_name) == 0)
+	{
+	  if (objfile != so->objfile)
+	    {
+	      /* This could happen when symbol_file_add_from_bfd
+		 below is interrupted.  */
+
+	      gdb_assert (so->objfile == NULL);
+	      gdb_assert (bfd_usrdata (so->abfd) == NULL);
+
+	      so->objfile = objfile;  /* Reconnect.  */
+
+	      /* Both solib and objfile refer to this abfd.  */
+	      set_ref_count (so->abfd, 2);
+	    }
+	  return;
+	}
     }
 
   sap = build_section_addr_info_from_section_table (so->sections,
                                                     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;
+
+  /* Both solib and objfile refer to this abfd.  */
+  set_ref_count (so->abfd, 2);
 
   free_section_addr_info (sap);
 

Re: Solibs and objfile BFD ownership

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> I seem to have hit a rough patch with my patches :-(

Yeah, that happens to me too.

Paul> This is happening because in symbol_add_stub refcount may not be set:
so-> objfile = symbol_file_add_from_bfd (so->abfd, flags, sap, OBJF_SHARED);
---> QUIT could be executed deep inside symbol_file_add_from_bfd, and
---> bfd_userdata below is never set.

Paul>    p_refcount = xmalloc (sizeof (*p_refcount));
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.

What do you think?

Tom

Re: Solibs and objfile BFD ownership

[Paul Pluzhnikov]

[-- 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,

Re: Solibs and objfile BFD ownership

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

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

Yeah, I suppose.  But it seems clearer and more future-proof to me.

Paul> 2009-08-19  Paul Pluzhnikov  <ppluzhnikov@google.com>
Paul> 	* objfiles.h (gdb_bfd_ref): New prototype.
Paul> 	* objfiles.c (gdb_bfd_ref): New function.
Paul> 	(allocate_objfile): Call it.
Paul> 	(gdb_bfd_unref): Adjust assertion.
Paul> 	* solib.c (solib_map_sections): Add reference.
Paul> 	(symbol_add_stub): Don't add reference here.
Paul> 	* symfile.c (reread_symbols): Add reference.

This is ok.  Thanks.

Paul> +  gdb_assert (*p_refcount == 1 || *p_refcount == 2);

This assertion is a bit strange, but I suppose it is harmless.  If we
ever want more sharing, we can always just remove it.  In the meantime I
suppose it is checking that we don't over-share somehow.

Tom

Re: Solibs and objfile BFD ownership

[Paul Pluzhnikov]

On Fri, Aug 21, 2009 at 10:29 AM, Tom Tromey<tromey@redhat.com> wrote:

> This is ok.   Thanks.

So committed.

> Paul> +   gdb_assert (*p_refcount == 1 || *p_refcount == 2);
>
> This assertion is a bit strange, but I suppose it is harmless.   If we
> ever want more sharing, we can always just remove it.   In the meantime I
> suppose it is checking that we don't over-share somehow.

The assertion was intended to catch somebody else in GDB using bfd_usrdata
for some other purpose (in a way I didn't find).

I think it's safe to leave it in for now.

Thanks,
-- 
Paul Pluzhnikov