[RFA] Introduce solib_loaded observer

[RFA] Introduce solib_loaded observer

[Mark Kettenis]

This patch introduces a new solib_loaded observer that parallels the
solub_unloaded observer that we already have.  I need this observer to
properly implement a threads stratum for FreeBSD and OpenBSD
user-space threads library support.  With this observer I'll also be
able to fix the problems we're having with statically linked programs
that use the threading library on Linux.  If you think about it a bit,
the problems with static thread libraries stem from the fact that we
abuse the now deprecated target_new_objfile_hook to get notified about
newly loaded libraries.  Unfortunately that hook is also called when
the main executable is loaded, which causes all kinds of trouble.

The patch below doesn't only implement the new observer.  The reason
for that is that when the solib_loaded observer is called, the symbols
for the new library haven't been loaded yet.  So I've split out the
code to read in those symbols out from solib_add() into a seperate
function, and made that public.

OK?

Mark


Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>
 
	* solib.h (struct so_list): Forward declaration.
	(solib_read_symbols): New prototype.
	* solib.c (solib_read_symbols): New function.
	(solib_add): Call solib_read_symbols to read in symbols.
	(update_solib_list): Call observer_notify_solib_loaded.


Index: doc/ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>

	* observer.texi (GDB Observers): Document "solib_loaded".


Index: solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.68
diff -u -p -r1.68 solib.c
--- solib.c 11 Sep 2004 10:24:50 -0000 1.68
+++ solib.c 8 Jan 2005 23:10:34 -0000
@@ -1,7 +1,8 @@
 /* Handle shared libraries for GDB, the GNU Debugger.
 
    Copyright 1990, 1991, 1992, 1993, 1994, 1995, 1996, 1997, 1998,
-   1999, 2000, 2001, 2002, 2003 Free Software Foundation, Inc.
+   1999, 2000, 2001, 2002, 2003, 2005
+   Free Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -369,6 +370,33 @@ symbol_add_stub (void *arg)
   return (1);
 }
 
+/* Read in symbols for shared object SO.  If FROM_TTY is non-zero, be
+   chatty about it.  Return non-zero if any symbols were actually
+   loaded.  */
+
+int
+solib_read_symbols (struct so_list *so, int from_tty)
+{
+  if (so->symbols_loaded)
+    {
+      if (from_tty)
+	printf_unfiltered ("Symbols already loaded for %s\n", so->so_name);
+    }
+  else
+    {
+      if (catch_errors (symbol_add_stub, so,
+			"Error while reading shared library symbols:\n",
+			RETURN_MASK_ALL))
+	{
+	  if (from_tty)
+	    printf_unfiltered ("Loaded symbols for %s\n", so->so_name);
+	  so->symbols_loaded = 1;
+	  return 1;
+	}
+    }
+
+  return 0;
+}
 
 /* LOCAL FUNCTION
 
@@ -479,8 +507,8 @@ update_solib_list (int from_tty, struct 
       /* If it's not on the inferior's list, remove it from GDB's tables.  */
       else
 	{
-	  /* Notify any observer that the SO has been unloaded
-	     before we remove it from the gdb tables.  */
+	  /* Notify any observer that the shared object has been
+	     unloaded before we remove it from GDB's tables.  */
 	  observer_notify_solib_unloaded (gdb);
 
 	  *gdb_link = gdb->next;
@@ -533,6 +561,10 @@ update_solib_list (int from_tty, struct 
 			  count * sizeof (i->sections[0]));
 		}
 	    }
+
+	  /* Notify any observer that the shared object has been
+             loaded now that we've added it to GDB's tables.  */
+	  observer_notify_solib_loaded (i);
 	}
     }
 }
@@ -584,27 +616,8 @@ solib_add (char *pattern, int from_tty, 
       if (! pattern || re_exec (gdb->so_name))
 	{
 	  any_matches = 1;
-
-	  if (gdb->symbols_loaded)
-	    {
-	      if (from_tty)
-		printf_unfiltered ("Symbols already loaded for %s\n",
-				   gdb->so_name);
-	    }
-	  else if (readsyms)
-	    {
-	      if (catch_errors
-		  (symbol_add_stub, gdb,
-		   "Error while reading shared library symbols:\n",
-		   RETURN_MASK_ALL))
-		{
-		  if (from_tty)
-		    printf_unfiltered ("Loaded symbols for %s\n",
-				       gdb->so_name);
-		  gdb->symbols_loaded = 1;
-		  loaded_any_symbols = 1;
-		}
-	    }
+	  if (readsyms && solib_read_symbols (gdb, from_tty))
+	    loaded_any_symbols = 1;
 	}
 
     if (from_tty && pattern && ! any_matches)
Index: solib.h
===================================================================
RCS file: /cvs/src/src/gdb/solib.h,v
retrieving revision 1.12
diff -u -p -r1.12 solib.h
--- solib.h 16 Dec 2003 21:46:10 -0000 1.12
+++ solib.h 8 Jan 2005 23:10:34 -0000
@@ -23,6 +23,7 @@
 #define SOLIB_H
 
 /* Forward decl's for prototypes */
+struct so_list;
 struct target_ops;
 
 /* Called when we free all symtabs, to free the shared library information
@@ -38,6 +39,7 @@ extern void clear_solib (void);
     solib_add (filename, from_tty, targ, readsyms)
 
 extern void solib_add (char *, int, struct target_ops *, int);
+extern int solib_read_symbols (struct so_list *, int);
 
 /* Function to be called when the inferior starts up, to discover the names
    of shared libraries that are dynamically linked, the base addresses to
Index: doc/observer.texi
===================================================================
RCS file: /cvs/src/src/gdb/doc/observer.texi,v
retrieving revision 1.8
diff -u -p -r1.8 observer.texi
--- doc/observer.texi 1 Sep 2004 17:59:37 -0000 1.8
+++ doc/observer.texi 8 Jan 2005 23:10:38 -0000
@@ -91,6 +91,12 @@ at the entry-point instruction.  For @sa
 inferior, and before any information on the inferior has been printed.
 @end deftypefun
 
+@deftypefun void solib_loaded (struct so_list *@var{solib})
+The specified shared library has been discovered to be loaded.  Note
+that when @value{GDBN} calls this observer, the library's symbols
+probably haven't been loaded yet.
+@end deftypefun
+
 @deftypefun void solib_unloaded (struct so_list *@var{solib})
 The specified shared library has been discovered to be unloaded.
 @end deftypefun

Re: [RFA] Introduce solib_loaded observer

[Daniel Jacobowitz]

On Sun, Jan 09, 2005 at 12:14:06AM +0100, Mark Kettenis wrote:
> This patch introduces a new solib_loaded observer that parallels the
> solub_unloaded observer that we already have.  I need this observer to
> properly implement a threads stratum for FreeBSD and OpenBSD
> user-space threads library support.  With this observer I'll also be
> able to fix the problems we're having with statically linked programs
> that use the threading library on Linux.  If you think about it a bit,
> the problems with static thread libraries stem from the fact that we
> abuse the now deprecated target_new_objfile_hook to get notified about
> newly loaded libraries.  Unfortunately that hook is also called when
> the main executable is loaded, which causes all kinds of trouble.

Actually, I've already posted patches for the Linux problem.  The basic
"abuse" of the target_new_objfile_hook isn't really the stumbling
block, rather, the attempt to use it for both running and non-running
programs.  The necessary fix was to use inferior_created.  Still, it'll
definitely be cleaner to use a solib_loaded hook there, and I look
forward to that.

I'll ping that patch after I get back from travelling next week.

> The patch below doesn't only implement the new observer.  The reason
> for that is that when the solib_loaded observer is called, the symbols
> for the new library haven't been loaded yet.  So I've split out the
> code to read in those symbols out from solib_add() into a seperate
> function, and made that public.

Could you explain why this is necessary, instead of calling the
observer after loading symbols?  I don't see notification at that point
as particularly useful.  Certainly the first thing the GNU/Linux code
would want to do would be to load the symbols.

-- 
Daniel Jacobowitz

Re: [RFA] Introduce solib_loaded observer

[Eli Zaretskii]

> Date: Sun, 9 Jan 2005 00:14:06 +0100 (CET)
> From: Mark Kettenis <mark.kettenis@xs4all.nl>
> 
> OK?

Okay for the gdb.texinfo part, except for this:

> +@deftypefun void solib_loaded (struct so_list *@var{solib})
> +The specified shared library has been discovered to be loaded.

It's probably better to say

  The shared library specified by @var{solib} has been loaded.

Re: [RFA] Introduce solib_loaded observer

[Mark Kettenis]

   Date: Sun, 09 Jan 2005 06:37:17 +0200
   From: "Eli Zaretskii" <eliz@gnu.org>

   > +@deftypefun void solib_loaded (struct so_list *@var{solib})
   > +The specified shared library has been discovered to be loaded.

   It's probably better to say

     The shared library specified by @var{solib} has been loaded.

Indeed, that's better.  I'll change the wording on the solib_unloaded
observer in a similar fashion.

Mark

Re: [RFA] Introduce solib_loaded observer

[Mark Kettenis]

   Date: Sat, 8 Jan 2005 19:01:04 -0500
   From: Daniel Jacobowitz <drow@false.org>

   On Sun, Jan 09, 2005 at 12:14:06AM +0100, Mark Kettenis wrote:
   > This patch introduces a new solib_loaded observer that parallels the
   > solub_unloaded observer that we already have.  I need this observer to
   > properly implement a threads stratum for FreeBSD and OpenBSD
   > user-space threads library support.  With this observer I'll also be
   > able to fix the problems we're having with statically linked programs
   > that use the threading library on Linux.  If you think about it a bit,
   > the problems with static thread libraries stem from the fact that we
   > abuse the now deprecated target_new_objfile_hook to get notified about
   > newly loaded libraries.  Unfortunately that hook is also called when
   > the main executable is loaded, which causes all kinds of trouble.

   Actually, I've already posted patches for the Linux problem.  The basic
   "abuse" of the target_new_objfile_hook isn't really the stumbling
   block, rather, the attempt to use it for both running and non-running
   programs.  The necessary fix was to use inferior_created.  Still, it'll
   definitely be cleaner to use a solib_loaded hook there, and I look
   forward to that.

I missed that patch.  But I'm doing the same thing for the BSD
user-level threads support.  Works like a charm.

   I'll ping that patch after I get back from travelling next week.

   > The patch below doesn't only implement the new observer.  The reason
   > for that is that when the solib_loaded observer is called, the symbols
   > for the new library haven't been loaded yet.  So I've split out the
   > code to read in those symbols out from solib_add() into a seperate
   > function, and made that public.

   Could you explain why this is necessary, instead of calling the
   observer after loading symbols?  I don't see notification at that point
   as particularly useful.  Certainly the first thing the GNU/Linux code
   would want to do would be to load the symbols.

Calling the observer after loading the symbols isn't possible.  You
can set "auto-solib-add" to 0, and then the symbols will never be
loaded at all.  So you'll always have to force loading the symbols
from within your observer anyway (but you only have to do so for the
threads library).  From a code perspective the point where the
notification is called is the most logical.  And that way it's less
likely that we see "auto-solib-add" related bugs ;-).

Mark

Re: [RFA] Introduce solib_loaded observer

[Daniel Jacobowitz]

On Sun, Jan 09, 2005 at 02:05:15PM +0100, Mark Kettenis wrote:
>    > The patch below doesn't only implement the new observer.  The reason
>    > for that is that when the solib_loaded observer is called, the symbols
>    > for the new library haven't been loaded yet.  So I've split out the
>    > code to read in those symbols out from solib_add() into a seperate
>    > function, and made that public.
> 
>    Could you explain why this is necessary, instead of calling the
>    observer after loading symbols?  I don't see notification at that point
>    as particularly useful.  Certainly the first thing the GNU/Linux code
>    would want to do would be to load the symbols.
> 
> Calling the observer after loading the symbols isn't possible.  You
> can set "auto-solib-add" to 0, and then the symbols will never be
> loaded at all.  So you'll always have to force loading the symbols
> from within your observer anyway (but you only have to do so for the
> threads library).  From a code perspective the point where the
> notification is called is the most logical.  And that way it's less
> likely that we see "auto-solib-add" related bugs ;-).

At the same time, I worry that it's going to be confusingly
inconsistent - for instance, I would have expected turning off
auto-solib-add to prevent loading symbols for libpthread!  Or at least,
loading of full symbols (all libthread_db on GNU/Linux really needs are
a couple of minsyms).

I don't think "isn't possible" is accurate; there are only two callers
of update_solib_list, and one of them wants to read the symbols.  That
could be pushed down into an argument to update_solib_list.  Would that
be better?

-- 
Daniel Jacobowitz

Re: [RFA] Introduce solib_loaded observer

[Mark Kettenis]

   Date: Sun, 9 Jan 2005 17:37:33 -0500
   From: Daniel Jacobowitz <drow@false.org>

   On Sun, Jan 09, 2005 at 02:05:15PM +0100, Mark Kettenis wrote:
   >
   > Calling the observer after loading the symbols isn't possible.  You
   > can set "auto-solib-add" to 0, and then the symbols will never be
   > loaded at all.  So you'll always have to force loading the symbols
   > from within your observer anyway (but you only have to do so for the
   > threads library).  From a code perspective the point where the
   > notification is called is the most logical.  And that way it's less
   > likely that we see "auto-solib-add" related bugs ;-).

   At the same time, I worry that it's going to be confusingly
   inconsistent - for instance, I would have expected turning off
   auto-solib-add to prevent loading symbols for libpthread!  Or at least,
   loading of full symbols (all libthread_db on GNU/Linux really needs are
   a couple of minsyms).

We should try to be as consistent as possible.  The current situation
is very inconsistent too: if you turn off auto-solib-add, you won't
get thread debugging support.  It's true that for debugging support
you usually only need a few minimal symbols.  I considered rolling my
own BFD-based lookup function, but I suspect that would result in a
serious performance hit because I'd lose the benefit of caching.

   I don't think "isn't possible" is accurate; there are only two callers
   of update_solib_list, and one of them wants to read the symbols.  That
   could be pushed down into an argument to update_solib_list.  Would that
   be better?

Because of the auto-solib-add issue I don't think it is, but given the
right arguments I think you can make me think differently.  What to
the others think of this?

Mark

Re: [RFA] Introduce solib_loaded observer

[Daniel Jacobowitz]

On Sun, Jan 09, 2005 at 11:58:18PM +0100, Mark Kettenis wrote:
>    Date: Sun, 9 Jan 2005 17:37:33 -0500
>    From: Daniel Jacobowitz <drow@false.org>
> 
>    On Sun, Jan 09, 2005 at 02:05:15PM +0100, Mark Kettenis wrote:
>    >
>    > Calling the observer after loading the symbols isn't possible.  You
>    > can set "auto-solib-add" to 0, and then the symbols will never be
>    > loaded at all.  So you'll always have to force loading the symbols
>    > from within your observer anyway (but you only have to do so for the
>    > threads library).  From a code perspective the point where the
>    > notification is called is the most logical.  And that way it's less
>    > likely that we see "auto-solib-add" related bugs ;-).
> 
>    At the same time, I worry that it's going to be confusingly
>    inconsistent - for instance, I would have expected turning off
>    auto-solib-add to prevent loading symbols for libpthread!  Or at least,
>    loading of full symbols (all libthread_db on GNU/Linux really needs are
>    a couple of minsyms).
> 
> We should try to be as consistent as possible.  The current situation
> is very inconsistent too: if you turn off auto-solib-add, you won't
> get thread debugging support.  It's true that for debugging support

Note that there's no other way to deliberately turn off thread
debugging at present.

> you usually only need a few minimal symbols.  I considered rolling my
> own BFD-based lookup function, but I suspect that would result in a
> serious performance hit because I'd lose the benefit of caching.

I doubt it would be that serious.  It'd be a bit tricky, of course, so
adding extra code for it seems like a shame.  Perhaps we could read in
just the minsyms...

> Because of the auto-solib-add issue I don't think it is, but given the
> right arguments I think you can make me think differently.  What to
> the others think of this?

I wonder if we can envision any other potential consumers for this
hook?  What would they want to do?

-- 
Daniel Jacobowitz

Re: [RFA] Introduce solib_loaded observer

[Andrew Cagney]

> +@deftypefun void solib_loaded (struct so_list *@var{solib})
> +The specified shared library has been discovered to be loaded.  Note
> +that when @value{GDBN} calls this observer, the library's symbols
> +probably haven't been loaded yet.
> +@end deftypefun
> +
>  @deftypefun void solib_unloaded (struct so_list *@var{solib})
>  The specified shared library has been discovered to be unloaded.
>  @end deftypefun

This is good.

Andrew

Re: [RFA] Introduce solib_loaded observer

[Kevin Buettner]

On Sun, 9 Jan 2005 23:58:18 +0100 (CET)
Mark Kettenis <mark.kettenis@xs4all.nl> wrote:

>    Date: Sun, 9 Jan 2005 17:37:33 -0500
>    From: Daniel Jacobowitz <drow@false.org>
> 
>    On Sun, Jan 09, 2005 at 02:05:15PM +0100, Mark Kettenis wrote:
>    >
>    > Calling the observer after loading the symbols isn't possible.  You
>    > can set "auto-solib-add" to 0, and then the symbols will never be
>    > loaded at all.  So you'll always have to force loading the symbols
>    > from within your observer anyway (but you only have to do so for the
>    > threads library).  From a code perspective the point where the
>    > notification is called is the most logical.  And that way it's less
>    > likely that we see "auto-solib-add" related bugs ;-).
> 
>    At the same time, I worry that it's going to be confusingly
>    inconsistent - for instance, I would have expected turning off
>    auto-solib-add to prevent loading symbols for libpthread!  Or at least,
>    loading of full symbols (all libthread_db on GNU/Linux really needs are
>    a couple of minsyms).
> 
> We should try to be as consistent as possible.  The current situation
> is very inconsistent too: if you turn off auto-solib-add, you won't
> get thread debugging support.  It's true that for debugging support
> you usually only need a few minimal symbols.  I considered rolling my
> own BFD-based lookup function, but I suspect that would result in a
> serious performance hit because I'd lose the benefit of caching.
> 
>    I don't think "isn't possible" is accurate; there are only two callers
>    of update_solib_list, and one of them wants to read the symbols.  That
>    could be pushed down into an argument to update_solib_list.  Would that
>    be better?
> 
> Because of the auto-solib-add issue I don't think it is, but given the
> right arguments I think you can make me think differently.  What to
> the others think of this?

IMO, setting auto-solib-add to 0 is useful in those situations where
the application has a large number of shared libraries.  Setting it to
0 will sometimes make it more expedient to debug such programs.  I
don't think that a GDB user wants to give up thread debugging when
setting auto-solib-add to 0.  Thus, I'm all in favor of Mark's current
strategy.

Mark, your patch is approved when you feel that useful discussion on
this matter has ended.  If Daniel convinces you that some other
strategy is more appropriate, then that patch is preapproved so long
as we don't give up the ability to debug threaded programs when
auto-solib-add is 0.

Kevin

Re: [RFA] Introduce solib_loaded observer

[Mark Kettenis]

   Date: Sun, 9 Jan 2005 18:06:29 -0500
   From: Daniel Jacobowitz <drow@false.org>

   On Sun, Jan 09, 2005 at 11:58:18PM +0100, Mark Kettenis wrote:
   >    Date: Sun, 9 Jan 2005 17:37:33 -0500
   >    From: Daniel Jacobowitz <drow@false.org>
   > 
   >    On Sun, Jan 09, 2005 at 02:05:15PM +0100, Mark Kettenis wrote:
   >    >
   >    > Calling the observer after loading the symbols isn't possible.  You
   >    > can set "auto-solib-add" to 0, and then the symbols will never be
   >    > loaded at all.  So you'll always have to force loading the symbols
   >    > from within your observer anyway (but you only have to do so for the
   >    > threads library).  From a code perspective the point where the
   >    > notification is called is the most logical.  And that way it's less
   >    > likely that we see "auto-solib-add" related bugs ;-).
   > 
   >    At the same time, I worry that it's going to be confusingly
   >    inconsistent - for instance, I would have expected turning off
   >    auto-solib-add to prevent loading symbols for libpthread!  Or at least,
   >    loading of full symbols (all libthread_db on GNU/Linux really needs are
   >    a couple of minsyms).
   > 
   > We should try to be as consistent as possible.  The current situation
   > is very inconsistent too: if you turn off auto-solib-add, you won't
   > get thread debugging support.  It's true that for debugging support

   Note that there's no other way to deliberately turn off thread
   debugging at present.

Then we should add a command to do that.

   > you usually only need a few minimal symbols.  I considered rolling my
   > own BFD-based lookup function, but I suspect that would result in a
   > serious performance hit because I'd lose the benefit of caching.

   I doubt it would be that serious.  It'd be a bit tricky, of course, so
   adding extra code for it seems like a shame.  Perhaps we could read in
   just the minsyms...

...perhaps another threads implementation needs full symbols.  The
bottom line is that the code probably needs to fiddle with symbols,
whether or not we call the observer before or after GDB would normally
load the symbols for a shared library.

   > Because of the auto-solib-add issue I don't think it is, but given the
   > right arguments I think you can make me think differently.  What to
   > the others think of this?

   I wonder if we can envision any other potential consumers for this
   hook?  What would they want to do?

The observer could be used to re-set breakpoints in shared libraries,
to implement bp_catch/bp_unload in a somewhat more generic fashion.
Some of these might need symbols some of them don't.

I'm still in favour of calling the observer *before*.  That way,
people using the observer are forced to ask the question whether they
want symbols to be loaded or not.

Mark

Re: [RFA] Introduce solib_loaded observer

[Daniel Jacobowitz]

On Tue, Jan 11, 2005 at 10:57:13PM +0100, Mark Kettenis wrote:
> I'm still in favour of calling the observer *before*.  That way,
> people using the observer are forced to ask the question whether they
> want symbols to be loaded or not.

Good enough for me.  Thanks for explaining.

-- 
Daniel Jacobowitz

Re: [RFA] Introduce solib_loaded observer

[Mark Kettenis]

   Date: Sun, 09 Jan 2005 06:37:17 +0200
   From: "Eli Zaretskii" <eliz@gnu.org>

   > +@deftypefun void solib_loaded (struct so_list *@var{solib})
   > +The specified shared library has been discovered to be loaded.

   It's probably better to say

     The shared library specified by @var{solib} has been loaded.

This is what I actually checked in:

Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>

	* observer.texi (GDB Observers): Document "solib_loaded".

Index: observer.texi
===================================================================
RCS file: /cvs/src/src/gdb/doc/observer.texi,v
retrieving revision 1.8
diff -u -p -r1.8 observer.texi
--- observer.texi 1 Sep 2004 17:59:37 -0000 1.8
+++ observer.texi 12 Jan 2005 20:45:47 -0000
@@ -91,7 +91,12 @@ at the entry-point instruction.  For @sa
 inferior, and before any information on the inferior has been printed.
 @end deftypefun
 
-@deftypefun void solib_unloaded (struct so_list *@var{solib})
-The specified shared library has been discovered to be unloaded.
+@deftypefun void solib_loaded (struct so_list *@var{solib})
+The shared library specified by @var{solib} has been loaded.  Note that
+when @value{GDBN} calls this observer, the library's symbols probably
+haven't been loaded yet.
 @end deftypefun
 
+@deftypefun void solib_unloaded (struct so_list *@var{solib})
+The shared library specified by @var{solib} has been unloaded.
+@end deftypefun

Re: [RFA] Introduce solib_loaded observer

[Eli Zaretskii]

> Date: Wed, 12 Jan 2005 21:49:13 +0100 (CET)
> From: Mark Kettenis <kettenis@gnu.org>
> CC: gdb-patches@sources.redhat.com
> 
>    Date: Sun, 09 Jan 2005 06:37:17 +0200
>    From: "Eli Zaretskii" <eliz@gnu.org>
> 
>    > +@deftypefun void solib_loaded (struct so_list *@var{solib})
>    > +The specified shared library has been discovered to be loaded.
> 
>    It's probably better to say
> 
>      The shared library specified by @var{solib} has been loaded.
> 
> This is what I actually checked in:
> 
> Index: ChangeLog
> from  Mark Kettenis  <kettenis@gnu.org>
> 
> 	* observer.texi (GDB Observers): Document "solib_loaded".

Thanks.