MI solib notification

MI solib notification

[Vladimir Prus]

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


This patch implements two new MI notifications,
=library-loaded and =library-unloaded. There's just one point of note --
identifications of shared libraries. In order for frontend to maintain
accurate list of shared libraries using notifications, there should be
a reliable way of matching unloaded library, as reported by =library-unloaded,
with a previously loaded library. Currently, GDB keeps two names associated
with a library, so_original_name which is reported by the target and so_name,
which is basically the name of the file where symbols are read from.

All internal comparisons of libraries for equality uses so_original_name, so
it makes sense that frontend use it too. However, it is technically possible
to load the same shared library twice, so ideally we use something more unique --
but this is extra work. So, MI notifications output three fields

	- id (documented as opaque)
	- target name
	- host name

presently, so_original_name is used for the first two, but in future, id might
become numeric, or it might look like libfoo.so@2. 

Comments? Is solib.c change OK? Are doc changes OK?

- Volodya



[-- Attachment #2: solib.diff --]
[-- Type: text/x-diff, Size: 7904 bytes --]

commit 8e9cd84e5a7b2302320b8a16ba7ba512057401b9
Author: Vladimir Prus <vladimir@codesourcery.com>
Date:   Fri Jan 30 20:50:50 2009 +0300

    Implement MI notification for library loading/unloading
    
    	gdb/doc/
    	* gdb.texinfo (GDB/MI Async Records): Document the
    	=library-loaded and =library-unloaded notifications.
    
    	gdb/
    	* mi/mi-interp.c (mi_solib_loaded, mi_solib_unloaded): New.
    	(mi_interpreter_init): Register the above.
    	* solib.c (clear_solib): Notify solib unload.
    
    	gdb/testsuite/
    	* gdb.mi/mi-nonstop.exp (notifs): Adjust for library notifications.
    	* gdb.mi/mi-nsintrall.exp (notifs): Likewise.
    	* gdb.mi/mi-nsmoribund.exp (notifs): Likewise.
    	* lib/mi-support.exp (library_loaded_re): New.
    	(mi_run_cmd, mi_send_resuming_command_raw): Adjust.

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index ffb29b4..d0e59a8 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -19535,6 +19535,15 @@ We suggest that in response to this notification, front ends
 highlight the selected thread and cause subsequent commands to apply to
 that thread.
 
+@item =library-loaded,id="@var{id}",target-name="@var{target-name}",host-name="@var{host-name}",low-address="@var{low}",high-address="@var{high}",symbols-loaded="@var{loaded}"
+Reports that a new library file was loaded by the program.  The
+@var{id} field is an opaque identifier of the library. For remote
+debugging case, @var{target-name} and @var{host-name} fields give the 
+name of the library file on the target, and on the host respectively.
+
+@item =library-unloaded,id="@var{id}",target-name="@var{target-name}",host-name="@var{host-name}"
+Reports that a library was loaded by the program.
+
 @end table
 
 @node GDB/MI Frame Information
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index f530176..8e2b230 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -34,6 +34,7 @@
 #include "mi-common.h"
 #include "observer.h"
 #include "gdbthread.h"
+#include "solist.h"
 
 /* These are the interpreter setup, etc. functions for the MI interpreter */
 static void mi_execute_command_wrapper (char *cmd);
@@ -58,6 +59,8 @@ static void mi_thread_exit (struct thread_info *t);
 static void mi_new_inferior (int pid);
 static void mi_inferior_exit (int pid);
 static void mi_on_resume (ptid_t ptid);
+static void mi_solib_loaded (struct so_list *solib);
+static void mi_solib_unloaded (struct so_list *solib);
 
 static void *
 mi_interpreter_init (int top_level)
@@ -86,6 +89,8 @@ mi_interpreter_init (int top_level)
       observer_attach_inferior_exit (mi_inferior_exit);
       observer_attach_normal_stop (mi_on_normal_stop);
       observer_attach_target_resumed (mi_on_resume);
+      observer_attach_solib_loaded (mi_solib_loaded);
+      observer_attach_solib_unloaded (mi_solib_unloaded);
     }
 
   return mi;
@@ -379,6 +384,31 @@ mi_on_resume (ptid_t ptid)
   gdb_flush (raw_stdout);
 }
 
+static void mi_solib_loaded (struct so_list *solib)
+{
+  struct mi_interp *mi = top_level_interpreter_data ();
+  target_terminal_ours ();
+  fprintf_unfiltered (mi->event_channel, 
+		      "library-loaded,id=\"%s\",target-name=\"%s\",host-name=\"%s\",low-address=\"0x%s\",high-address=\"0x%s\",symbols-loaded=\"%d\"", 
+		      solib->so_original_name, solib->so_original_name, 
+		      solib->so_name, 
+		      paddr (solib->addr_low), paddr (solib->addr_high), 
+		      solib->symbols_loaded);
+  gdb_flush (mi->event_channel);
+}
+
+static void mi_solib_unloaded (struct so_list *solib)
+{
+  struct mi_interp *mi = top_level_interpreter_data ();
+  target_terminal_ours ();
+  fprintf_unfiltered (mi->event_channel, 
+		      "library-unloaded,id=\"%s\",target-name=\"%s\",host-name=\"%s\"", 
+		      solib->so_original_name, solib->so_original_name, 
+		      solib->so_name);
+  gdb_flush (mi->event_channel);
+}
+
+
 extern initialize_file_ftype _initialize_mi_interp; /* -Wmissing-prototypes */
 
 void
diff --git a/gdb/solib.c b/gdb/solib.c
index cce4f7f..5a28292 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -908,6 +908,7 @@ clear_solib (void)
     {
       struct so_list *so = so_list_head;
       so_list_head = so->next;
+      observer_notify_solib_unloaded (so);
       if (so->abfd)
 	remove_target_sections (so->abfd);
       free_so (so);
diff --git a/gdb/testsuite/gdb.mi/mi-nonstop.exp b/gdb/testsuite/gdb.mi/mi-nonstop.exp
index 2521ae5..08952b0 100644
--- a/gdb/testsuite/gdb.mi/mi-nonstop.exp
+++ b/gdb/testsuite/gdb.mi/mi-nonstop.exp
@@ -63,7 +63,7 @@ mi_gdb_test "200-break-insert -t main" ".*"
 set created "=thread-created,id=\"$decimal\"\r\n"
 set running "\\*running,thread-id=\"$decimal\"\r\n"
 
-set notifs "($created)*($running)*"
+set notifs "($created)*($running)*($library_loaded_re)*"
 
 # Note: presently, we skip this test on non-native targets,
 # so 'run' is OK.  As soon as we start to run this on remote
diff --git a/gdb/testsuite/gdb.mi/mi-nsintrall.exp b/gdb/testsuite/gdb.mi/mi-nsintrall.exp
index b0d7b71..d34b08d 100644
--- a/gdb/testsuite/gdb.mi/mi-nsintrall.exp
+++ b/gdb/testsuite/gdb.mi/mi-nsintrall.exp
@@ -62,7 +62,7 @@ mi_gdb_test "200-break-insert -t main" ".*"
 set created "=thread-created,id=\"$decimal\"\r\n"
 set running "\\*running,thread-id=\"$decimal\"\r\n"
 
-set notifs "($created)*($running)*"
+set notifs "($created)*($running)*($library_loaded_re)*"
 
 # Note: presently, we skip this test on non-native targets,
 # so 'run' is OK.  As soon as we start to run this on remote
diff --git a/gdb/testsuite/gdb.mi/mi-nsmoribund.exp b/gdb/testsuite/gdb.mi/mi-nsmoribund.exp
index b836c5b..505fc84 100644
--- a/gdb/testsuite/gdb.mi/mi-nsmoribund.exp
+++ b/gdb/testsuite/gdb.mi/mi-nsmoribund.exp
@@ -62,7 +62,7 @@ mi_gdb_test "200-break-insert -t main" ".*"
 set created "=thread-created,id=\"$decimal\"\r\n"
 set running "\\*running,thread-id=\"$decimal\"\r\n"
 
-set notifs "($created)*($running)*"
+set notifs "($created)*($running)*($library_loaded_re)*"
 
 # Note: presently, we skip this test on non-native targets,
 # so 'run' is OK.  As soon as we start to run this on remote
diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index a85e373..be9b530 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -31,6 +31,7 @@ global mi_inferior_tty_name
 set MIFLAGS "-i=mi"
 
 set thread_selected_re "=thread-selected,id=\"\[0-9+\]\"\r\n"
+set library_loaded_re "=library-loaded\[^\n\]+\"\r\n"
 
 #
 # mi_gdb_exit -- exit the GDB, killing the target program if necessary
@@ -778,6 +779,7 @@ proc mi_run_cmd {args} {
     }
     global mi_gdb_prompt
     global thread_selected_re
+    global library_loaded_re
 
     if [target_info exists gdb_init_command] {
 	send_gdb "[target_info gdb_init_command]\n";
@@ -819,7 +821,7 @@ proc mi_run_cmd {args} {
 
     send_gdb "220-exec-run $args\n"
     gdb_expect {
-	-re "220\\^running\r\n(\\*running,thread-id=\"\[^\"\]+\"\r\n|=thread-created,id=\"1\",group-id=\"\[0-9\]+\"\r\n)*(${thread_selected_re})?${mi_gdb_prompt}" {
+	-re "220\\^running\r\n(\\*running,thread-id=\"\[^\"\]+\"\r\n|=thread-created,id=\"1\",group-id=\"\[0-9\]+\"\r\n)*(${library_loaded_re})*(${thread_selected_re})?${mi_gdb_prompt}" {
 	}
 	timeout {
 	    perror "Unable to start target"
@@ -1435,10 +1437,11 @@ proc mi_send_resuming_command_raw {command test} {
 
     global mi_gdb_prompt
     global thread_selected_re
+    global library_loaded_re
 
     send_gdb "$command\n"
     gdb_expect {
-        -re "\\^running\r\n\\*running,thread-id=\"\[^\"\]+\"\r\n($thread_selected_re)?${mi_gdb_prompt}" {
+        -re "\\^running\r\n\\*running,thread-id=\"\[^\"\]+\"\r\n($library_loaded_re)*($thread_selected_re)?${mi_gdb_prompt}" {
             # Note that lack of 'pass' call here -- this works around limitation
             # in DejaGNU xfail mechanism. mi-until.exp has this:
             #

Re: MI solib notification

[Daniel Jacobowitz]

On Fri, Jan 30, 2009 at 11:51:11PM +0200, Eli Zaretskii wrote:
> > From: Vladimir Prus <vladimir@codesourcery.com>
> > Date: Sat, 31 Jan 2009 00:10:46 +0300
> > 
> > Are doc changes OK?
> 
> Yes, thanks.  But I have a few comments:
> 
> > +@item =library-loaded,id="@var{id}",target-name="@var{target-name}",host-name="@var{host-name}",low-address="@var{low}",high-address="@var{high}",symbols-loaded="@var{loaded}"
> 
> This is a very long line, and the @table that it's part of has @code
> markup, which means TeX will not break this line.  We need to make it
> shorter.  One idea is this:
> 
>   @item =library-loaded,@var{info}
> 
> and then describe the contents of @var{info} below, perhaps as a
> separate @table.  WDYT?

MI output comes all on one line, but what we've done elsewhere in the
GDB/MI chapter is insert line breaks after some of the commas.  How
about that here too?  This form is easy to visually parse, once you're
used to the MI syntax.

-- 
Daniel Jacobowitz
CodeSourcery

Re: MI solib notification

[Eli Zaretskii]

> Date: Sun, 1 Feb 2009 12:53:09 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Vladimir Prus <vladimir@codesourcery.com>,
> 	gdb-patches@sources.redhat.com, nickrob@snap.net.nz
> 
> > > +@item =library-loaded,id="@var{id}",target-name="@var{target-name}",host-name="@var{host-name}",low-address="@var{low}",high-address="@var{high}",symbols-loaded="@var{loaded}"
> > 
> > This is a very long line, and the @table that it's part of has @code
> > markup, which means TeX will not break this line.  We need to make it
> > shorter.  One idea is this:
> > 
> >   @item =library-loaded,@var{info}
> > 
> > and then describe the contents of @var{info} below, perhaps as a
> > separate @table.  WDYT?
> 
> MI output comes all on one line, but what we've done elsewhere in the
> GDB/MI chapter is insert line breaks after some of the commas.  How
> about that here too?

That'd be okay as well, although I don't really understand why it is
better than my suggestion.

Re: MI solib notification

[Daniel Jacobowitz]

On Sun, Feb 01, 2009 at 08:22:28PM +0200, Eli Zaretskii wrote:
> That'd be okay as well, although I don't really understand why it is
> better than my suggestion.

It's not a big difference; I find it more natural to have the output
all grouped together and looking similar to GDB's expected output.

-- 
Daniel Jacobowitz
CodeSourcery

Re: MI solib notification

[Vladimir Prus]

On Sunday 01 February 2009 21:29:18 Daniel Jacobowitz wrote:
> On Sun, Feb 01, 2009 at 08:22:28PM +0200, Eli Zaretskii wrote:
> > That'd be okay as well, although I don't really understand why it is
> > better than my suggestion.
> 
> It's not a big difference; I find it more natural to have the output
> all grouped together and looking similar to GDB's expected output.

I'd prefer this approach to, because that's how the rest of MI docs do it.
But -- who to I force a break inside @item ?

- Volodya

Re: MI solib notification

[Eli Zaretskii]

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Tue, 17 Feb 2009 22:08:37 +0300
> Cc: Eli Zaretskii <eliz@gnu.org>,
>  gdb-patches@sources.redhat.com,
>  nickrob@snap.net.nz
> 
> On Sunday 01 February 2009 21:29:18 Daniel Jacobowitz wrote:
> > On Sun, Feb 01, 2009 at 08:22:28PM +0200, Eli Zaretskii wrote:
> > > That'd be okay as well, although I don't really understand why it is
> > > better than my suggestion.
> > 
> > It's not a big difference; I find it more natural to have the output
> > all grouped together and looking similar to GDB's expected output.
> 
> I'd prefer this approach to, because that's how the rest of MI docs do it.

Do we have such long @item's elsewhere in the MI docs?  I made a quick
scan and didn't find any, but perhaps I missed something.

> But -- who to I force a break inside @item ?

You can't, AFAIK.  Maybe you could use @itemx for all lines but the
first one, but that would be a kludge.

P.S. Please in the future show the original code/Texinfo that prompted
the response above: that was 2 and a half weeks ago, and I no longer
remembered what was this about, nor had the original message in my
mailbox.  I had to go to the archives to know what we were talking
about.

TIA

Re: MI solib notification

[Vladimir Prus]

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

On Tuesday 17 February 2009 22:28:13 Eli Zaretskii wrote:
> > From: Vladimir Prus <vladimir@codesourcery.com>
> > Date: Tue, 17 Feb 2009 22:08:37 +0300
> > Cc: Eli Zaretskii <eliz@gnu.org>,
> >  gdb-patches@sources.redhat.com,
> >  nickrob@snap.net.nz
> > 
> > On Sunday 01 February 2009 21:29:18 Daniel Jacobowitz wrote:
> > > On Sun, Feb 01, 2009 at 08:22:28PM +0200, Eli Zaretskii wrote:
> > > > That'd be okay as well, although I don't really understand why it is
> > > > better than my suggestion.
> > > 
> > > It's not a big difference; I find it more natural to have the output
> > > all grouped together and looking similar to GDB's expected output.
> > 
> > I'd prefer this approach to, because that's how the rest of MI docs do it.
> 
> Do we have such long @item's elsewhere in the MI docs?  I made a quick
> scan and didn't find any, but perhaps I missed something.

I don't think we do -- most other long examples are inside @smallexample

> > But -- who to I force a break inside @item ?
> 
> You can't, AFAIK.  

Ouch.

> Maybe you could use @itemx for all lines but the 
> first one, but that would be a kludge.

Well, worse. texinfo would render both quoted in '', so it will no longer
look anyway like MI output. Seems like we can directly list all fields
in @item. How about the attached, instead?

> P.S. Please in the future show the original code/Texinfo that prompted
> the response above: that was 2 and a half weeks ago, and I no longer
> remembered what was this about, nor had the original message in my
> mailbox.  I had to go to the archives to know what we were talking
> about.

Sorry, will quote texinfo in future.

- Volodya

[-- Attachment #2: solib_notifications.diff --]
[-- Type: text/x-diff, Size: 8906 bytes --]

commit c1091d19fe7c4b02bb5ea623761e4caa0d6fe537
Author: Vladimir Prus <vladimir@codesourcery.com>
Date:   Fri Jan 30 20:50:50 2009 +0300

    Implement MI notification for library loading/unloading
    
    	gdb/doc/
    	* gdb.texinfo (GDB/MI Async Records): Document the
    	=library-loaded and =library-unloaded notifications.
    
    	gdb/
    	* mi/mi-interp.c (mi_solib_loaded, mi_solib_unloaded): New.
    	(mi_interpreter_init): Register the above.
    	* solib.c (clear_solib): Notify solib unload.
    	* breakpoint.c (disable_breakpoints_in_unloaded_shlib): Do not
    	disable breakpoints on a.out targets.
    
    	gdb/testsuite/
    	* gdb.mi/mi-nonstop.exp (notifs): Adjust for library notifications.
    	* gdb.mi/mi-nsintrall.exp (notifs): Likewise.
    	* gdb.mi/mi-nsmoribund.exp (notifs): Likewise.
    	* lib/mi-support.exp (library_loaded_re): New.
    	(mi_run_cmd, mi_send_resuming_command_raw): Adjust.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 0caedec..bc7c29f 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4457,6 +4457,14 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
   struct bp_location *loc;
   int disabled_shlib_breaks = 0;
 
+  /* SunOS a.out shared libraries are always mapped, so do not
+     disable breakpoints; they will only be reported as unloaded
+     through clear_solib when GDB discards its shared library
+     list.  See clear_solib for more information.  */
+  if (exec_bfd != NULL
+      && bfd_get_flavour (exec_bfd) != bfd_target_aout_flavour)
+    return;
+
   ALL_BP_LOCATIONS (loc)
   {
     struct breakpoint *b = loc->owner;
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 6aad520..ce959f5 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -19912,6 +19912,22 @@ We suggest that in response to this notification, front ends
 highlight the selected thread and cause subsequent commands to apply to
 that thread.
 
+@item =library-loaded,...
+Reports that a new library file was loaded by the program.  This
+notification has 4 fields---@var{id}, @var{target-name},
+@var{host-name}, and @var{symbols-loaded}. The @var{id} field is an
+opaque identifier of the library.  For remote debugging case,
+@var{target-name} and @var{host-name} fields give the name of the
+library file on the target, and on the host respectively. For native
+debugging, both those fields have the same value. The
+@var{symbols-loaded} field reports if the debug symbols for this
+library are loaded.
+
+@item =library-unloaded,...
+Reports that a library was unloaded by the program. This notification
+has 3 fields---@var{id}, @var{target-name} and @var{host-name} with
+the same meaning as for the @code{=library-loaded} notification
+
 @end table
 
 @node GDB/MI Frame Information
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index 95dfdf4..d32638c 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -34,6 +34,7 @@
 #include "mi-common.h"
 #include "observer.h"
 #include "gdbthread.h"
+#include "solist.h"
 
 /* These are the interpreter setup, etc. functions for the MI interpreter */
 static void mi_execute_command_wrapper (char *cmd);
@@ -58,6 +59,8 @@ static void mi_thread_exit (struct thread_info *t);
 static void mi_new_inferior (int pid);
 static void mi_inferior_exit (int pid);
 static void mi_on_resume (ptid_t ptid);
+static void mi_solib_loaded (struct so_list *solib);
+static void mi_solib_unloaded (struct so_list *solib);
 
 static void *
 mi_interpreter_init (int top_level)
@@ -86,6 +89,8 @@ mi_interpreter_init (int top_level)
       observer_attach_inferior_exit (mi_inferior_exit);
       observer_attach_normal_stop (mi_on_normal_stop);
       observer_attach_target_resumed (mi_on_resume);
+      observer_attach_solib_loaded (mi_solib_loaded);
+      observer_attach_solib_unloaded (mi_solib_unloaded);
     }
 
   return mi;
@@ -408,6 +413,31 @@ mi_on_resume (ptid_t ptid)
   gdb_flush (raw_stdout);
 }
 
+static void mi_solib_loaded (struct so_list *solib)
+{
+  struct mi_interp *mi = top_level_interpreter_data ();
+  target_terminal_ours ();
+  fprintf_unfiltered (mi->event_channel, 
+		      "library-loaded,id=\"%s\",target-name=\"%s\",host-name=\"%s\",low-address=\"0x%s\",high-address=\"0x%s\",symbols-loaded=\"%d\"", 
+		      solib->so_original_name, solib->so_original_name, 
+		      solib->so_name, 
+		      paddr (solib->addr_low), paddr (solib->addr_high), 
+		      solib->symbols_loaded);
+  gdb_flush (mi->event_channel);
+}
+
+static void mi_solib_unloaded (struct so_list *solib)
+{
+  struct mi_interp *mi = top_level_interpreter_data ();
+  target_terminal_ours ();
+  fprintf_unfiltered (mi->event_channel, 
+		      "library-unloaded,id=\"%s\",target-name=\"%s\",host-name=\"%s\"", 
+		      solib->so_original_name, solib->so_original_name, 
+		      solib->so_name);
+  gdb_flush (mi->event_channel);
+}
+
+
 extern initialize_file_ftype _initialize_mi_interp; /* -Wmissing-prototypes */
 
 void
diff --git a/gdb/solib.c b/gdb/solib.c
index cce4f7f..5a28292 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -908,6 +908,7 @@ clear_solib (void)
     {
       struct so_list *so = so_list_head;
       so_list_head = so->next;
+      observer_notify_solib_unloaded (so);
       if (so->abfd)
 	remove_target_sections (so->abfd);
       free_so (so);
diff --git a/gdb/testsuite/gdb.mi/mi-nonstop.exp b/gdb/testsuite/gdb.mi/mi-nonstop.exp
index 2521ae5..08952b0 100644
--- a/gdb/testsuite/gdb.mi/mi-nonstop.exp
+++ b/gdb/testsuite/gdb.mi/mi-nonstop.exp
@@ -63,7 +63,7 @@ mi_gdb_test "200-break-insert -t main" ".*"
 set created "=thread-created,id=\"$decimal\"\r\n"
 set running "\\*running,thread-id=\"$decimal\"\r\n"
 
-set notifs "($created)*($running)*"
+set notifs "($created)*($running)*($library_loaded_re)*"
 
 # Note: presently, we skip this test on non-native targets,
 # so 'run' is OK.  As soon as we start to run this on remote
diff --git a/gdb/testsuite/gdb.mi/mi-nsintrall.exp b/gdb/testsuite/gdb.mi/mi-nsintrall.exp
index b0d7b71..d34b08d 100644
--- a/gdb/testsuite/gdb.mi/mi-nsintrall.exp
+++ b/gdb/testsuite/gdb.mi/mi-nsintrall.exp
@@ -62,7 +62,7 @@ mi_gdb_test "200-break-insert -t main" ".*"
 set created "=thread-created,id=\"$decimal\"\r\n"
 set running "\\*running,thread-id=\"$decimal\"\r\n"
 
-set notifs "($created)*($running)*"
+set notifs "($created)*($running)*($library_loaded_re)*"
 
 # Note: presently, we skip this test on non-native targets,
 # so 'run' is OK.  As soon as we start to run this on remote
diff --git a/gdb/testsuite/gdb.mi/mi-nsmoribund.exp b/gdb/testsuite/gdb.mi/mi-nsmoribund.exp
index b836c5b..505fc84 100644
--- a/gdb/testsuite/gdb.mi/mi-nsmoribund.exp
+++ b/gdb/testsuite/gdb.mi/mi-nsmoribund.exp
@@ -62,7 +62,7 @@ mi_gdb_test "200-break-insert -t main" ".*"
 set created "=thread-created,id=\"$decimal\"\r\n"
 set running "\\*running,thread-id=\"$decimal\"\r\n"
 
-set notifs "($created)*($running)*"
+set notifs "($created)*($running)*($library_loaded_re)*"
 
 # Note: presently, we skip this test on non-native targets,
 # so 'run' is OK.  As soon as we start to run this on remote
diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index a2aa629..f62c240 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -31,6 +31,7 @@ global mi_inferior_tty_name
 set MIFLAGS "-i=mi"
 
 set thread_selected_re "=thread-selected,id=\"\[0-9+\]\"\r\n"
+set library_loaded_re "=library-loaded\[^\n\]+\"\r\n"
 
 #
 # mi_gdb_exit -- exit the GDB, killing the target program if necessary
@@ -778,6 +779,7 @@ proc mi_run_cmd {args} {
     }
     global mi_gdb_prompt
     global thread_selected_re
+    global library_loaded_re
 
     if [target_info exists gdb_init_command] {
 	send_gdb "[target_info gdb_init_command]\n";
@@ -819,7 +821,7 @@ proc mi_run_cmd {args} {
 
     send_gdb "220-exec-run $args\n"
     gdb_expect {
-	-re "220\\^running\r\n(\\*running,thread-id=\"\[^\"\]+\"\r\n|=thread-created,id=\"1\",group-id=\"\[0-9\]+\"\r\n)*(${thread_selected_re})?${mi_gdb_prompt}" {
+	-re "220\\^running\r\n(\\*running,thread-id=\"\[^\"\]+\"\r\n|=thread-created,id=\"1\",group-id=\"\[0-9\]+\"\r\n)*(${library_loaded_re})*(${thread_selected_re})?${mi_gdb_prompt}" {
 	}
 	timeout {
 	    perror "Unable to start target"
@@ -1435,10 +1437,11 @@ proc mi_send_resuming_command_raw {command test} {
 
     global mi_gdb_prompt
     global thread_selected_re
+    global library_loaded_re
 
     send_gdb "$command\n"
     gdb_expect {
-        -re "\\^running\r\n\\*running,thread-id=\"\[^\"\]+\"\r\n($thread_selected_re)?${mi_gdb_prompt}" {
+        -re "\\^running\r\n\\*running,thread-id=\"\[^\"\]+\"\r\n($library_loaded_re)*($thread_selected_re)?${mi_gdb_prompt}" {
             # Note that lack of 'pass' call here -- this works around limitation
             # in DejaGNU xfail mechanism. mi-until.exp has this:
             #

Re: MI solib notification

[Eli Zaretskii]

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Tue, 17 Feb 2009 22:44:48 +0300
> Cc: drow@false.org,
>  gdb-patches@sources.redhat.com,
>  nickrob@snap.net.nz
> 
> How about the attached, instead?
> [...]
> +@item =library-loaded,...
> +Reports that a new library file was loaded by the program.  This
> +notification has 4 fields---@var{id}, @var{target-name},
> +@var{host-name}, and @var{symbols-loaded}. The @var{id} field is an
> +opaque identifier of the library.  For remote debugging case,
> +@var{target-name} and @var{host-name} fields give the name of the
> +library file on the target, and on the host respectively. For native
> +debugging, both those fields have the same value. The
> +@var{symbols-loaded} field reports if the debug symbols for this
> +library are loaded.

That's okay, but now I ask again why not do it like I suggested in the
first place, viz.:

    @item =library-loaded,@var{info}
    Reports that a new library file was loaded by the program.  @var{info}
    includes 4 fields:

    @table @code
    @item id="@var{id}"
    Opaque identifier of the library.
    @item target-name="@var{target-name}"
    @itemx host-name="@var{host-name}"
    For remote debugging case, @var{target-name} and @var{host-name}
    fields give the name of the library file on the target, and on the
    host respectively.  For native debugging, both those fields have the
    same value.
    ...

etc., you get the idea.  What you suggested now is very close to this,
but I think my suggestion makes it easier to read and grasp.

Also, there's one case in your text of only one space after a period
that ends a sentence.

Thanks.

Re: MI solib notification

[Vladimir Prus]

Eli Zaretskii wrote:

>> From: Vladimir Prus <vladimir@codesourcery.com>
>> Date: Tue, 17 Feb 2009 22:44:48 +0300
>> Cc: drow@false.org,
>>  gdb-patches@sources.redhat.com,
>>  nickrob@snap.net.nz
>> 
>> How about the attached, instead?
>> [...]
>> +@item =library-loaded,...
>> +Reports that a new library file was loaded by the program.  This
>> +notification has 4 fields---@var{id}, @var{target-name},
>> +@var{host-name}, and @var{symbols-loaded}. The @var{id} field is an
>> +opaque identifier of the library.  For remote debugging case,
>> +@var{target-name} and @var{host-name} fields give the name of the
>> +library file on the target, and on the host respectively. For native
>> +debugging, both those fields have the same value. The
>> +@var{symbols-loaded} field reports if the debug symbols for this
>> +library are loaded.
> 
> That's okay, but now I ask again why not do it like I suggested in the
> first place, viz.:
> 
>     @item =library-loaded,@var{info}
>     Reports that a new library file was loaded by the program.  @var{info}
>     includes 4 fields:
> 
>     @table @code
>     @item id="@var{id}"
>     Opaque identifier of the library.
>     @item target-name="@var{target-name}"
>     @itemx host-name="@var{host-name}"
>     For remote debugging case, @var{target-name} and @var{host-name}
>     fields give the name of the library file on the target, and on the
>     host respectively.  For native debugging, both those fields have the
>     same value.
>     ...
> 
> etc., you get the idea.  What you suggested now is very close to this,
> but I think my suggestion makes it easier to read and grasp.

I think the way you suggest is more complex. It introduces a new symbol 'info' 
that does not actually correspond to standalone entity in MI output and users
might begin to wonder what info actually is, and how it includes fields. Saying
that notification itself has 4 fields is more direct.

- Volodya

Re: MI solib notification

[Vladimir Prus]

Vladimir Prus wrote:

> Eli Zaretskii wrote:
> 
>>> From: Vladimir Prus <vladimir@codesourcery.com>
>>> Date: Tue, 17 Feb 2009 22:44:48 +0300
>>> Cc: drow@false.org,
>>>  gdb-patches@sources.redhat.com,
>>>  nickrob@snap.net.nz
>>> 
>>> How about the attached, instead?
>>> [...]
>>> +@item =library-loaded,...
>>> +Reports that a new library file was loaded by the program.  This
>>> +notification has 4 fields---@var{id}, @var{target-name},
>>> +@var{host-name}, and @var{symbols-loaded}. The @var{id} field is an
>>> +opaque identifier of the library.  For remote debugging case,
>>> +@var{target-name} and @var{host-name} fields give the name of the
>>> +library file on the target, and on the host respectively. For native
>>> +debugging, both those fields have the same value. The
>>> +@var{symbols-loaded} field reports if the debug symbols for this
>>> +library are loaded.
>> 
>> That's okay, but now I ask again why not do it like I suggested in the
>> first place, viz.:
>> 
>>     @item =library-loaded,@var{info}
>>     Reports that a new library file was loaded by the program.  @var{info}
>>     includes 4 fields:
>> 
>>     @table @code
>>     @item id="@var{id}"
>>     Opaque identifier of the library.
>>     @item target-name="@var{target-name}"
>>     @itemx host-name="@var{host-name}"
>>     For remote debugging case, @var{target-name} and @var{host-name}
>>     fields give the name of the library file on the target, and on the
>>     host respectively.  For native debugging, both those fields have the
>>     same value.
>>     ...
>> 
>> etc., you get the idea.  What you suggested now is very close to this,
>> but I think my suggestion makes it easier to read and grasp.
> 
> I think the way you suggest is more complex. It introduces a new symbol 'info'
> that does not actually correspond to standalone entity in MI output and users
> might begin to wonder what info actually is, and how it includes fields. Saying
> that notification itself has 4 fields is more direct.

Also, table is relatively more heavyweight mechanism to describe just 4 fields,
using 4 sentences total.

- Volodya

Re: MI solib notification

[Eli Zaretskii]

> From:  Vladimir Prus <vladimir@codesourcery.com>
> Date:  Wed, 18 Feb 2009 00:42:19 +0300
> 
> Also, table is relatively more heavyweight mechanism to describe just 4 fields,
> using 4 sentences total.

Well, we sometimes use it even for a single field, let alone for 4.
The advantage of a @table is that it makes the enumeration of several
items more explicit.

But I'm okay with your original text as well, as I said.

Re: MI solib notification

[Vladimir Prus]

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

On Wednesday 18 February 2009 07:12:21 Eli Zaretskii wrote:
> > From:  Vladimir Prus <vladimir@codesourcery.com>
> > Date:  Wed, 18 Feb 2009 00:42:19 +0300
> > 
> > Also, table is relatively more heavyweight mechanism to describe just 4 fields,
> > using 4 sentences total.
> 
> Well, we sometimes use it even for a single field, let alone for 4.
> The advantage of a @table is that it makes the enumeration of several
> items more explicit.
> 
> But I'm okay with your original text as well, as I said.

Ok, I've checked in that patch. I have applied a couple of code fixes:

	- Fixed "!=" vs. "==" messup Pedro has noticed
	- Removed the address fields from the notification, as discussed
	with Dan. The docs were already adjusted in the previous version
	of the patch, but code was not.

Thanks,
Volodya

[-- Attachment #2: final.diff --]
[-- Type: text/x-diff, Size: 8794 bytes --]

commit 60dac2a7376b6d8106d468abd902acb6d9534ae4
Author: Vladimir Prus <vladimir@codesourcery.com>
Date:   Fri Jan 30 20:50:50 2009 +0300

    Implement MI notification for library loading/unloading
    
    	gdb/doc/
    	* gdb.texinfo (GDB/MI Async Records): Document the
    	=library-loaded and =library-unloaded notifications.
    
    	gdb/
    	* mi/mi-interp.c (mi_solib_loaded, mi_solib_unloaded): New.
    	(mi_interpreter_init): Register the above.
    	* solib.c (clear_solib): Notify solib unload.
    	* breakpoint.c (disable_breakpoints_in_unloaded_shlib): Do not
    	disable breakpoints on a.out targets.
    
    	gdb/testsuite/
    	* gdb.mi/mi-nonstop.exp (notifs): Adjust for library notifications.
    	* gdb.mi/mi-nsintrall.exp (notifs): Likewise.
    	* gdb.mi/mi-nsmoribund.exp (notifs): Likewise.
    	* lib/mi-support.exp (library_loaded_re): New.
    	(mi_run_cmd, mi_send_resuming_command_raw): Adjust.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 0caedec..8a7057b 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4457,6 +4457,14 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
   struct bp_location *loc;
   int disabled_shlib_breaks = 0;
 
+  /* SunOS a.out shared libraries are always mapped, so do not
+     disable breakpoints; they will only be reported as unloaded
+     through clear_solib when GDB discards its shared library
+     list.  See clear_solib for more information.  */
+  if (exec_bfd != NULL
+      && bfd_get_flavour (exec_bfd) == bfd_target_aout_flavour)
+    return;
+
   ALL_BP_LOCATIONS (loc)
   {
     struct breakpoint *b = loc->owner;
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 6aad520..ce959f5 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -19912,6 +19912,22 @@ We suggest that in response to this notification, front ends
 highlight the selected thread and cause subsequent commands to apply to
 that thread.
 
+@item =library-loaded,...
+Reports that a new library file was loaded by the program.  This
+notification has 4 fields---@var{id}, @var{target-name},
+@var{host-name}, and @var{symbols-loaded}. The @var{id} field is an
+opaque identifier of the library.  For remote debugging case,
+@var{target-name} and @var{host-name} fields give the name of the
+library file on the target, and on the host respectively. For native
+debugging, both those fields have the same value. The
+@var{symbols-loaded} field reports if the debug symbols for this
+library are loaded.
+
+@item =library-unloaded,...
+Reports that a library was unloaded by the program. This notification
+has 3 fields---@var{id}, @var{target-name} and @var{host-name} with
+the same meaning as for the @code{=library-loaded} notification
+
 @end table
 
 @node GDB/MI Frame Information
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index 95dfdf4..65c1b4f 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -34,6 +34,7 @@
 #include "mi-common.h"
 #include "observer.h"
 #include "gdbthread.h"
+#include "solist.h"
 
 /* These are the interpreter setup, etc. functions for the MI interpreter */
 static void mi_execute_command_wrapper (char *cmd);
@@ -58,6 +59,8 @@ static void mi_thread_exit (struct thread_info *t);
 static void mi_new_inferior (int pid);
 static void mi_inferior_exit (int pid);
 static void mi_on_resume (ptid_t ptid);
+static void mi_solib_loaded (struct so_list *solib);
+static void mi_solib_unloaded (struct so_list *solib);
 
 static void *
 mi_interpreter_init (int top_level)
@@ -86,6 +89,8 @@ mi_interpreter_init (int top_level)
       observer_attach_inferior_exit (mi_inferior_exit);
       observer_attach_normal_stop (mi_on_normal_stop);
       observer_attach_target_resumed (mi_on_resume);
+      observer_attach_solib_loaded (mi_solib_loaded);
+      observer_attach_solib_unloaded (mi_solib_unloaded);
     }
 
   return mi;
@@ -408,6 +413,31 @@ mi_on_resume (ptid_t ptid)
   gdb_flush (raw_stdout);
 }
 
+static void
+mi_solib_loaded (struct so_list *solib)
+{
+  struct mi_interp *mi = top_level_interpreter_data ();
+  target_terminal_ours ();
+  fprintf_unfiltered (mi->event_channel, 
+		      "library-loaded,id=\"%s\",target-name=\"%s\",host-name=\"%s\",symbols-loaded=\"%d\"", 
+		      solib->so_original_name, solib->so_original_name, 
+		      solib->so_name, solib->symbols_loaded);
+  gdb_flush (mi->event_channel);
+}
+
+static void
+mi_solib_unloaded (struct so_list *solib)
+{
+  struct mi_interp *mi = top_level_interpreter_data ();
+  target_terminal_ours ();
+  fprintf_unfiltered (mi->event_channel, 
+		      "library-unloaded,id=\"%s\",target-name=\"%s\",host-name=\"%s\"", 
+		      solib->so_original_name, solib->so_original_name, 
+		      solib->so_name);
+  gdb_flush (mi->event_channel);
+}
+
+
 extern initialize_file_ftype _initialize_mi_interp; /* -Wmissing-prototypes */
 
 void
diff --git a/gdb/solib.c b/gdb/solib.c
index cce4f7f..5a28292 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -908,6 +908,7 @@ clear_solib (void)
     {
       struct so_list *so = so_list_head;
       so_list_head = so->next;
+      observer_notify_solib_unloaded (so);
       if (so->abfd)
 	remove_target_sections (so->abfd);
       free_so (so);
diff --git a/gdb/testsuite/gdb.mi/mi-nonstop.exp b/gdb/testsuite/gdb.mi/mi-nonstop.exp
index 2521ae5..08952b0 100644
--- a/gdb/testsuite/gdb.mi/mi-nonstop.exp
+++ b/gdb/testsuite/gdb.mi/mi-nonstop.exp
@@ -63,7 +63,7 @@ mi_gdb_test "200-break-insert -t main" ".*"
 set created "=thread-created,id=\"$decimal\"\r\n"
 set running "\\*running,thread-id=\"$decimal\"\r\n"
 
-set notifs "($created)*($running)*"
+set notifs "($created)*($running)*($library_loaded_re)*"
 
 # Note: presently, we skip this test on non-native targets,
 # so 'run' is OK.  As soon as we start to run this on remote
diff --git a/gdb/testsuite/gdb.mi/mi-nsintrall.exp b/gdb/testsuite/gdb.mi/mi-nsintrall.exp
index b0d7b71..d34b08d 100644
--- a/gdb/testsuite/gdb.mi/mi-nsintrall.exp
+++ b/gdb/testsuite/gdb.mi/mi-nsintrall.exp
@@ -62,7 +62,7 @@ mi_gdb_test "200-break-insert -t main" ".*"
 set created "=thread-created,id=\"$decimal\"\r\n"
 set running "\\*running,thread-id=\"$decimal\"\r\n"
 
-set notifs "($created)*($running)*"
+set notifs "($created)*($running)*($library_loaded_re)*"
 
 # Note: presently, we skip this test on non-native targets,
 # so 'run' is OK.  As soon as we start to run this on remote
diff --git a/gdb/testsuite/gdb.mi/mi-nsmoribund.exp b/gdb/testsuite/gdb.mi/mi-nsmoribund.exp
index b836c5b..505fc84 100644
--- a/gdb/testsuite/gdb.mi/mi-nsmoribund.exp
+++ b/gdb/testsuite/gdb.mi/mi-nsmoribund.exp
@@ -62,7 +62,7 @@ mi_gdb_test "200-break-insert -t main" ".*"
 set created "=thread-created,id=\"$decimal\"\r\n"
 set running "\\*running,thread-id=\"$decimal\"\r\n"
 
-set notifs "($created)*($running)*"
+set notifs "($created)*($running)*($library_loaded_re)*"
 
 # Note: presently, we skip this test on non-native targets,
 # so 'run' is OK.  As soon as we start to run this on remote
diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index a2aa629..f62c240 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -31,6 +31,7 @@ global mi_inferior_tty_name
 set MIFLAGS "-i=mi"
 
 set thread_selected_re "=thread-selected,id=\"\[0-9+\]\"\r\n"
+set library_loaded_re "=library-loaded\[^\n\]+\"\r\n"
 
 #
 # mi_gdb_exit -- exit the GDB, killing the target program if necessary
@@ -778,6 +779,7 @@ proc mi_run_cmd {args} {
     }
     global mi_gdb_prompt
     global thread_selected_re
+    global library_loaded_re
 
     if [target_info exists gdb_init_command] {
 	send_gdb "[target_info gdb_init_command]\n";
@@ -819,7 +821,7 @@ proc mi_run_cmd {args} {
 
     send_gdb "220-exec-run $args\n"
     gdb_expect {
-	-re "220\\^running\r\n(\\*running,thread-id=\"\[^\"\]+\"\r\n|=thread-created,id=\"1\",group-id=\"\[0-9\]+\"\r\n)*(${thread_selected_re})?${mi_gdb_prompt}" {
+	-re "220\\^running\r\n(\\*running,thread-id=\"\[^\"\]+\"\r\n|=thread-created,id=\"1\",group-id=\"\[0-9\]+\"\r\n)*(${library_loaded_re})*(${thread_selected_re})?${mi_gdb_prompt}" {
 	}
 	timeout {
 	    perror "Unable to start target"
@@ -1435,10 +1437,11 @@ proc mi_send_resuming_command_raw {command test} {
 
     global mi_gdb_prompt
     global thread_selected_re
+    global library_loaded_re
 
     send_gdb "$command\n"
     gdb_expect {
-        -re "\\^running\r\n\\*running,thread-id=\"\[^\"\]+\"\r\n($thread_selected_re)?${mi_gdb_prompt}" {
+        -re "\\^running\r\n\\*running,thread-id=\"\[^\"\]+\"\r\n($library_loaded_re)*($thread_selected_re)?${mi_gdb_prompt}" {
             # Note that lack of 'pass' call here -- this works around limitation
             # in DejaGNU xfail mechanism. mi-until.exp has this:
             #

[-- Attachment #3: adjust.diff --]
[-- Type: text/x-diff, Size: 1517 bytes --]

commit 2e809e5106502093f8f3bd3a1e070c82997ba69f
Author: Vladimir Prus <vladimir@codesourcery.com>
Date:   Wed Feb 18 09:39:34 2009 +0300

    Adjust solib notifications

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index bc7c29f..8a7057b 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4462,7 +4462,7 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
      through clear_solib when GDB discards its shared library
      list.  See clear_solib for more information.  */
   if (exec_bfd != NULL
-      && bfd_get_flavour (exec_bfd) != bfd_target_aout_flavour)
+      && bfd_get_flavour (exec_bfd) == bfd_target_aout_flavour)
     return;
 
   ALL_BP_LOCATIONS (loc)
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index 662dabb..65c1b4f 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -419,11 +419,9 @@ mi_solib_loaded (struct so_list *solib)
   struct mi_interp *mi = top_level_interpreter_data ();
   target_terminal_ours ();
   fprintf_unfiltered (mi->event_channel, 
-		      "library-loaded,id=\"%s\",target-name=\"%s\",host-name=\"%s\",low-address=\"0x%s\",high-address=\"0x%s\",symbols-loaded=\"%d\"", 
+		      "library-loaded,id=\"%s\",target-name=\"%s\",host-name=\"%s\",symbols-loaded=\"%d\"", 
 		      solib->so_original_name, solib->so_original_name, 
-		      solib->so_name, 
-		      paddr (solib->addr_low), paddr (solib->addr_high), 
-		      solib->symbols_loaded);
+		      solib->so_name, solib->symbols_loaded);
   gdb_flush (mi->event_channel);
 }
 

Re: MI solib notification

[Eli Zaretskii]

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Wed, 18 Feb 2009 10:31:02 +0300
> Cc: gdb-patches@sources.redhat.com
> 
> > But I'm okay with your original text as well, as I said.
> 
> Ok, I've checked in that patch.

Thanks.

> +@var{host-name}, and @var{symbols-loaded}. The @var{id} field is an
                                            ^^
One more blank here, please.

> +library file on the target, and on the host respectively. For native
                                                           ^^
Likewise.

> +debugging, both those fields have the same value. The
                                                   ^^
Likewise.

> +Reports that a library was unloaded by the program. This notification
                                                     ^^
Likewise.

Re: MI solib notification

[Vladimir Prus]

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

On Wednesday 18 February 2009 21:46:06 Eli Zaretskii wrote:
> > From: Vladimir Prus <vladimir@codesourcery.com>
> > Date: Wed, 18 Feb 2009 10:31:02 +0300
> > Cc: gdb-patches@sources.redhat.com
> > 
> > > But I'm okay with your original text as well, as I said.
> > 
> > Ok, I've checked in that patch.
> 
> Thanks.
> 
> > +@var{host-name}, and @var{symbols-loaded}. The @var{id} field is an
>                                             ^^
> One more blank here, please.
> 
> > +library file on the target, and on the host respectively. For native
>                                                            ^^
> Likewise.
> 
> > +debugging, both those fields have the same value. The
>                                                    ^^
> Likewise.
> 
> > +Reports that a library was unloaded by the program. This notification
>                                                      ^^
> Likewise.

I've checked in the below follow-up.

Thanks,
Volodya


[-- Attachment #2: spaces.diff --]
[-- Type: text/x-diff, Size: 1984 bytes --]

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/doc/ChangeLog,v
retrieving revision 1.866
diff -u -p -r1.866 ChangeLog
--- ChangeLog	18 Feb 2009 07:28:33 -0000	1.866
+++ ChangeLog	18 Feb 2009 19:43:24 -0000
@@ -1,5 +1,10 @@
 2009-02-18  Vladimir Prus  <vladimir@codesourcery.com>
 
+	* gdb.texinfo (GDB/MI Async Records): Add double-spaces
+	between sentences.
+
+2009-02-18  Vladimir Prus  <vladimir@codesourcery.com>
+
 	* gdb.texinfo (GDB/MI Async Records): Document the
 	=library-loaded and =library-unloaded notifications.
 
Index: gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.559
diff -u -p -r1.559 gdb.texinfo
--- gdb.texinfo	18 Feb 2009 07:28:33 -0000	1.559
+++ gdb.texinfo	18 Feb 2009 19:43:26 -0000
@@ -19915,16 +19915,16 @@ that thread.
 @item =library-loaded,...
 Reports that a new library file was loaded by the program.  This
 notification has 4 fields---@var{id}, @var{target-name},
-@var{host-name}, and @var{symbols-loaded}. The @var{id} field is an
+@var{host-name}, and @var{symbols-loaded}.  The @var{id} field is an
 opaque identifier of the library.  For remote debugging case,
 @var{target-name} and @var{host-name} fields give the name of the
-library file on the target, and on the host respectively. For native
-debugging, both those fields have the same value. The
+library file on the target, and on the host respectively.  For native
+debugging, both those fields have the same value.  The
 @var{symbols-loaded} field reports if the debug symbols for this
 library are loaded.
 
 @item =library-unloaded,...
-Reports that a library was unloaded by the program. This notification
+Reports that a library was unloaded by the program.  This notification
 has 3 fields---@var{id}, @var{target-name} and @var{host-name} with
 the same meaning as for the @code{=library-loaded} notification
 

Re: MI solib notification

[Eli Zaretskii]

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Wed, 18 Feb 2009 22:50:39 +0300
> Cc: pedro@codesourcery.com,
>  gdb-patches@sources.redhat.com
> 
> I've checked in the below follow-up.

Thank you.

Re: MI solib notification

[Eli Zaretskii]

> From:  Vladimir Prus <vladimir@codesourcery.com>
> Date:  Wed, 18 Feb 2009 00:37:29 +0300
> 
> >     @item =library-loaded,@var{info}
> >     Reports that a new library file was loaded by the program.  @var{info}
> >     includes 4 fields:
> > 
> >     @table @code
> >     @item id="@var{id}"
> >     Opaque identifier of the library.
> >     @item target-name="@var{target-name}"
> >     @itemx host-name="@var{host-name}"
> >     For remote debugging case, @var{target-name} and @var{host-name}
> >     fields give the name of the library file on the target, and on the
> >     host respectively.  For native debugging, both those fields have the
> >     same value.
> >     ...
> > 
> > etc., you get the idea.  What you suggested now is very close to this,
> > but I think my suggestion makes it easier to read and grasp.
> 
> I think the way you suggest is more complex. It introduces a new symbol 'info' 
> that does not actually correspond to standalone entity in MI output and users
> might begin to wonder what info actually is, and how it includes fields. Saying
> that notification itself has 4 fields is more direct.

OK, but then how about using a @table to describe those 4 fields?

Re: MI solib notification

[Daniel Jacobowitz]

On Tue, Feb 17, 2009 at 10:44:48PM +0300, Vladimir Prus wrote:
> commit c1091d19fe7c4b02bb5ea623761e4caa0d6fe537
> Author: Vladimir Prus <vladimir@codesourcery.com>
> Date:   Fri Jan 30 20:50:50 2009 +0300
> 
>     Implement MI notification for library loading/unloading
>     
>     	gdb/doc/
>     	* gdb.texinfo (GDB/MI Async Records): Document the
>     	=library-loaded and =library-unloaded notifications.
>     
>     	gdb/
>     	* mi/mi-interp.c (mi_solib_loaded, mi_solib_unloaded): New.
>     	(mi_interpreter_init): Register the above.
>     	* solib.c (clear_solib): Notify solib unload.
>     	* breakpoint.c (disable_breakpoints_in_unloaded_shlib): Do not
>     	disable breakpoints on a.out targets.
>     
>     	gdb/testsuite/
>     	* gdb.mi/mi-nonstop.exp (notifs): Adjust for library notifications.
>     	* gdb.mi/mi-nsintrall.exp (notifs): Likewise.
>     	* gdb.mi/mi-nsmoribund.exp (notifs): Likewise.
>     	* lib/mi-support.exp (library_loaded_re): New.
>     	(mi_run_cmd, mi_send_resuming_command_raw): Adjust.

The code portions of this are OK now, thanks.

> +static void mi_solib_loaded (struct so_list *solib)

> +static void mi_solib_unloaded (struct so_list *solib)

Newline after void please.

-- 
Daniel Jacobowitz
CodeSourcery

Re: MI solib notification

[Pedro Alves]

On Tuesday 17 February 2009 19:44:48, Vladimir Prus wrote:
> @@ -4457,6 +4457,14 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
>    struct bp_location *loc;
>    int disabled_shlib_breaks = 0;
>  
> +  /* SunOS a.out shared libraries are always mapped, so do not
> +     disable breakpoints; they will only be reported as unloaded
> +     through clear_solib when GDB discards its shared library
> +     list.  See clear_solib for more information.  */
> +  if (exec_bfd != NULL
> +      && bfd_get_flavour (exec_bfd) != bfd_target_aout_flavour)
> +    return;
> +

Isn't this check backwards?

-- 
Pedro Alves

Re: MI solib notification

[Pedro Alves]

On Tuesday 17 February 2009 19:44:48, Vladimir Prus wrote:
> @@ -4457,6 +4457,14 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
>    struct bp_location *loc;
>    int disabled_shlib_breaks = 0;
>  
> +  /* SunOS a.out shared libraries are always mapped, so do not
> +     disable breakpoints; they will only be reported as unloaded
> +     through clear_solib when GDB discards its shared library
> +     list.  See clear_solib for more information.  */
> +  if (exec_bfd != NULL
> +      && bfd_get_flavour (exec_bfd) != bfd_target_aout_flavour)
> +    return;
> +

Isn't this check backwards?

-- 
Pedro Alves

Re: MI solib notification

[Daniel Jacobowitz]

On Sat, Jan 31, 2009 at 12:10:46AM +0300, Vladimir Prus wrote:
> +static void mi_solib_loaded (struct so_list *solib)
> +{
> +  struct mi_interp *mi = top_level_interpreter_data ();
> +  target_terminal_ours ();
> +  fprintf_unfiltered (mi->event_channel, 
> +		      "library-loaded,id=\"%s\",target-name=\"%s\",host-name=\"%s\",low-address=\"0x%s\",high-address=\"0x%s\",symbols-loaded=\"%d\"", 
> +		      solib->so_original_name, solib->so_original_name, 
> +		      solib->so_name, 
> +		      paddr (solib->addr_low), paddr (solib->addr_high), 
> +		      solib->symbols_loaded);
> +  gdb_flush (mi->event_channel);
> +}

Do existing clients use addr_low / addr_high from "info shared"?
If so, do you know what they use it for?

These fields make sense for SVR4 models, like Linux and BSD shared
libraries, where shared libraries get a single chunk of address space.
But they don't make sense for some DLL systems which load the text and
data separately, or for kernel modules where each section can get a
different load offset.  We should either report the boundaries of
the first contiguous piece, which will not cover the whole library,
or else the highest and lowest address, which may cover bits of
some other library.

> diff --git a/gdb/solib.c b/gdb/solib.c
> index cce4f7f..5a28292 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -908,6 +908,7 @@ clear_solib (void)
>      {
>        struct so_list *so = so_list_head;
>        so_list_head = so->next;
> +      observer_notify_solib_unloaded (so);
>        if (so->abfd)
>  	remove_target_sections (so->abfd);
>        free_so (so);

What sort of effect does this have on the existing hooks?  There are
two users of this observer; the bsd-uthread.c one looks like it will
be fine, but this might make
breakpoint.c:disable_breakpoints_in_unloaded_shlib very chatty when
you rerun the program.

-- 
Daniel Jacobowitz
CodeSourcery

Re: MI solib notification

[Vladimir Prus]

On Sunday 01 February 2009 21:04:23 Daniel Jacobowitz wrote:
> On Sat, Jan 31, 2009 at 12:10:46AM +0300, Vladimir Prus wrote:
> > +static void mi_solib_loaded (struct so_list *solib)
> > +{
> > +  struct mi_interp *mi = top_level_interpreter_data ();
> > +  target_terminal_ours ();
> > +  fprintf_unfiltered (mi->event_channel, 
> > +		      "library-loaded,id=\"%s\",target-name=\"%s\",host-name=\"%s\",low-address=\"0x%s\",high-address=\"0x%s\",symbols-loaded=\"%d\"", 
> > +		      solib->so_original_name, solib->so_original_name, 
> > +		      solib->so_name, 
> > +		      paddr (solib->addr_low), paddr (solib->addr_high), 
> > +		      solib->symbols_loaded);
> > +  gdb_flush (mi->event_channel);
> > +}
> 
> Do existing clients use addr_low / addr_high from "info shared"?
> If so, do you know what they use it for?
> 
> These fields make sense for SVR4 models, like Linux and BSD shared
> libraries, where shared libraries get a single chunk of address space.
> But they don't make sense for some DLL systems which load the text and
> data separately, or for kernel modules where each section can get a
> different load offset.  We should either report the boundaries of
> the first contiguous piece, which will not cover the whole library,
> or else the highest and lowest address, which may cover bits of
> some other library.

Eclipse does use this, in particular consider this bit of code:

	public boolean hasSharedLibChanged(SharedLibrary lib, MIShared miLib) {
		return !miLib.getName().equals(lib.getFileName()) ||
			!MIFormat.getBigInteger(miLib.getFrom()).equals(lib.getStartAddress())   ||
		    !MIFormat.getBigInteger(miLib.getTo()).equals(lib.getEndAddress()) ||
			miLib.isRead() != lib.areSymbolsLoaded();
	}

Now, we know this is never going to happen in practice, when using GDB, and this is
GDB-specific code, so maybe we can drop addresses from the GDB output and then DSF
folks will make sure they don't use start/end addresses? 

> > diff --git a/gdb/solib.c b/gdb/solib.c
> > index cce4f7f..5a28292 100644
> > --- a/gdb/solib.c
> > +++ b/gdb/solib.c
> > @@ -908,6 +908,7 @@ clear_solib (void)
> >      {
> >        struct so_list *so = so_list_head;
> >        so_list_head = so->next;
> > +      observer_notify_solib_unloaded (so);
> >        if (so->abfd)
> >  	remove_target_sections (so->abfd);
> >        free_so (so);
> 
> What sort of effect does this have on the existing hooks?  There are
> two users of this observer; the bsd-uthread.c one looks like it will
> be fine, but this might make
> breakpoint.c:disable_breakpoints_in_unloaded_shlib very chatty when
> you rerun the program.

Well, not necessary, due to this code in clear_solib:


903       if (exec_bfd != NULL
904           && bfd_get_flavour (exec_bfd) != bfd_target_aout_flavour)
905         disable_breakpoints_in_shlibs ();

So, I think we'll only get additional chatter on a.out targets -- do we care?

- Volodya

Re: MI solib notification

[Daniel Jacobowitz]

On Thu, Feb 12, 2009 at 12:12:52PM +0300, Vladimir Prus wrote:
> Eclipse does use this, in particular consider this bit of code:
> 
> 	public boolean hasSharedLibChanged(SharedLibrary lib, MIShared miLib) {
> 		return !miLib.getName().equals(lib.getFileName()) ||
> 			!MIFormat.getBigInteger(miLib.getFrom()).equals(lib.getStartAddress())   ||
> 		    !MIFormat.getBigInteger(miLib.getTo()).equals(lib.getEndAddress()) ||
> 			miLib.isRead() != lib.areSymbolsLoaded();
> 	}
> 
> Now, we know this is never going to happen in practice, when using GDB, and this is
> GDB-specific code, so maybe we can drop addresses from the GDB output and then DSF
> folks will make sure they don't use start/end addresses? 

I see.  This is code that parses the CLI output, right?  Let's do as
you suggest: not generate the addresses in MI notifications, and make
GDB responsible for detecting this change in the future.

> Well, not necessary, due to this code in clear_solib:
> 
> 
> 903       if (exec_bfd != NULL
> 904           && bfd_get_flavour (exec_bfd) != bfd_target_aout_flavour)
> 905         disable_breakpoints_in_shlibs ();
> 
> So, I think we'll only get additional chatter on a.out targets -- do we care?

== vs != ?  I think this runs everywhere except a.out.

-- 
Daniel Jacobowitz
CodeSourcery

Re: MI solib notification

[Vladimir Prus]

On Thursday 12 February 2009 16:22:47 Daniel Jacobowitz wrote:
> On Thu, Feb 12, 2009 at 12:12:52PM +0300, Vladimir Prus wrote:
> > Eclipse does use this, in particular consider this bit of code:
> > 
> > 	public boolean hasSharedLibChanged(SharedLibrary lib, MIShared miLib) {
> > 		return !miLib.getName().equals(lib.getFileName()) ||
> > 			!MIFormat.getBigInteger(miLib.getFrom()).equals(lib.getStartAddress())   ||
> > 		    !MIFormat.getBigInteger(miLib.getTo()).equals(lib.getEndAddress()) ||
> > 			miLib.isRead() != lib.areSymbolsLoaded();
> > 	}
> > 
> > Now, we know this is never going to happen in practice, when using GDB, and this is
> > GDB-specific code, so maybe we can drop addresses from the GDB output and then DSF
> > folks will make sure they don't use start/end addresses? 
> 
> I see.  This is code that parses the CLI output, right?  Let's do as
> you suggest: not generate the addresses in MI notifications, and make
> GDB responsible for detecting this change in the future.

OK.

> > Well, not necessary, due to this code in clear_solib:
> > 
> > 
> > 903       if (exec_bfd != NULL
> > 904           && bfd_get_flavour (exec_bfd) != bfd_target_aout_flavour)
> > 905         disable_breakpoints_in_shlibs ();
> > 
> > So, I think we'll only get additional chatter on a.out targets -- do we care?
> 
> == vs != ?  I think this runs everywhere except a.out.

Yes, this runs everywhere except a.out and marks all breakpoints in solibs
disabled, without printing any messages. *Then* we look over all solibs
and call observer, but since all breakpoints are already disabled,
breakpoint.c:disable_breakpoints_in_unloaded_shlib won't do anything,
and won't say anything either.

- Volodya

Re: MI solib notification

[Daniel Jacobowitz]

On Thu, Feb 12, 2009 at 06:01:54PM +0300, Vladimir Prus wrote:
> Yes, this runs everywhere except a.out and marks all breakpoints in solibs
> disabled, without printing any messages. *Then* we look over all solibs
> and call observer, but since all breakpoints are already disabled,
> breakpoint.c:disable_breakpoints_in_unloaded_shlib won't do anything,
> and won't say anything either.

I see.  The comment in clear_solibs means that we will break SunOS
a.out shared libraries if we do this; they were previously not
disabled, since they won't ever be re-enabled.  So I suggest not
making the observer call on a.out either.  Otherwise OK.

-- 
Daniel Jacobowitz
CodeSourcery

Re: MI solib notification

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

>> Yes, this runs everywhere except a.out and marks all breakpoints in solibs
>> disabled, without printing any messages. *Then* we look over all solibs
>> and call observer, but since all breakpoints are already disabled,
>> breakpoint.c:disable_breakpoints_in_unloaded_shlib won't do anything,
>> and won't say anything either.

Daniel> I see.  The comment in clear_solibs means that we will break SunOS
Daniel> a.out shared libraries if we do this; they were previously not
Daniel> disabled, since they won't ever be re-enabled.  So I suggest not
Daniel> making the observer call on a.out either.  Otherwise OK.

I actually already approved this hunk in another thread -- Paul
Pluzhnikov's recent patch.

My rationale for approving it was that for this observer to be
reliable, it has to be called every place an so is freed.  I was even
thinking the call should probably be pushed into free_so.

Now I'm thinking I don't understand this code as well as I thought I
did :}.  I hope I didn't cause too much trouble.

Tom

Re: MI solib notification

[Daniel Jacobowitz]

On Thu, Feb 12, 2009 at 10:35:29AM -0700, Tom Tromey wrote:
> I actually already approved this hunk in another thread -- Paul
> Pluzhnikov's recent patch.

Oh, interesting.

> My rationale for approving it was that for this observer to be
> reliable, it has to be called every place an so is freed.  I was even
> thinking the call should probably be pushed into free_so.
> 
> Now I'm thinking I don't understand this code as well as I thought I
> did :}.  I hope I didn't cause too much trouble.

I agree with your rationale.  I almost suggested we push the aout
check down into disable_breakpoints_in_unloaded_shlib... what do you
think?

-- 
Daniel Jacobowitz
CodeSourcery

Re: MI solib notification

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

Daniel> I agree with your rationale.  I almost suggested we push the aout
Daniel> check down into disable_breakpoints_in_unloaded_shlib... what do you
Daniel> think?

It sounds reasonable to me.
Pedro also suggested this... great minds :-)

Tom

Re: MI solib notification

[Vladimir Prus]

On Thursday 12 February 2009 22:58:37 Tom Tromey wrote:
> >>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:
> 
> Daniel> I agree with your rationale.  I almost suggested we push the aout
> Daniel> check down into disable_breakpoints_in_unloaded_shlib... what do you
> Daniel> think?
> 
> It sounds reasonable to me.
> Pedro also suggested this... great minds :-)

Can you write down the comment that disable_breakpoints_in_unloaded_shlib
would have to contain to explain all this? ;-) I assume it will go like this:

	Don't emit any message here for a.out, because for all the other
	targets some other code calls some other function that arranges for
	this message to not be printed when rerunning the program...

In other words, the problem with checking for a.out is that it deeply depends on
intricate details of some other code. I think disable_breakpoints_in_unloaded_shlib
can either do:

	1. Check for target_has_execution or similar
	2. Check for a new parameter to the observer, like 'mass_murder'

- Volodya


	

> 
> Tom
> 

Re: MI solib notification

[Daniel Jacobowitz]

On Thu, Feb 12, 2009 at 11:11:04PM +0300, Vladimir Prus wrote:
> Can you write down the comment that disable_breakpoints_in_unloaded_shlib
> would have to contain to explain all this? ;-) I assume it will go like this:
> 
> 	Don't emit any message here for a.out, because for all the other
> 	targets some other code calls some other function that arranges for
> 	this message to not be printed when rerunning the program...
> 
> In other words, the problem with checking for a.out is that it deeply depends on
> intricate details of some other code.

It's not about the message, but about the disabling.  I mean:

  /* SunOS a.out shared libraries are always mapped, so do not
     disable breakpoints; they will only be reported as unloaded
     through clear_solib when GDB discards its shared library
     list.  See clear_solib for more information.  */

-- 
Daniel Jacobowitz
CodeSourcery

Re: MI solib notification

[Vladimir Prus]

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

On Thursday 12 February 2009 23:17:44 Daniel Jacobowitz wrote:
> On Thu, Feb 12, 2009 at 11:11:04PM +0300, Vladimir Prus wrote:
> > Can you write down the comment that disable_breakpoints_in_unloaded_shlib
> > would have to contain to explain all this? ;-) I assume it will go like this:
> > 
> > 	Don't emit any message here for a.out, because for all the other
> > 	targets some other code calls some other function that arranges for
> > 	this message to not be printed when rerunning the program...
> > 
> > In other words, the problem with checking for a.out is that it deeply depends on
> > intricate details of some other code.
> 
> It's not about the message, but about the disabling.  I mean:
> 
>   /* SunOS a.out shared libraries are always mapped, so do not
>      disable breakpoints; they will only be reported as unloaded
>      through clear_solib when GDB discards its shared library
>      list.  See clear_solib for more information.  */

For avoidance of doubt, is the attached OK code-wize?

- Volodya

[-- Attachment #2: solib_notifications.diff --]
[-- Type: text/x-diff, Size: 8601 bytes --]

commit 266230fb5ea8f192ec5725d23716ff2949a13ce0
Author: Vladimir Prus <vladimir@codesourcery.com>
Date:   Fri Jan 30 20:50:50 2009 +0300

    Implement MI notification for library loading/unloading
    
    	gdb/doc/
    	* gdb.texinfo (GDB/MI Async Records): Document the
    	=library-loaded and =library-unloaded notifications.
    
    	gdb/
    	* mi/mi-interp.c (mi_solib_loaded, mi_solib_unloaded): New.
    	(mi_interpreter_init): Register the above.
    	* solib.c (clear_solib): Notify solib unload.
    	* breakpoint.c (disable_breakpoints_in_unloaded_shlib): Do not
    	disable breakpoints on a.out targets.
    
    	gdb/testsuite/
    	* gdb.mi/mi-nonstop.exp (notifs): Adjust for library notifications.
    	* gdb.mi/mi-nsintrall.exp (notifs): Likewise.
    	* gdb.mi/mi-nsmoribund.exp (notifs): Likewise.
    	* lib/mi-support.exp (library_loaded_re): New.
    	(mi_run_cmd, mi_send_resuming_command_raw): Adjust.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 0caedec..9b48f24 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4457,6 +4457,13 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
   struct bp_location *loc;
   int disabled_shlib_breaks = 0;
 
+  /* We should not disable breakpoints on a.out targets.
+     See comment on solib.c:clear_solib for a detailed
+     explanation.  */
+  if (exec_bfd != NULL
+      && bfd_get_flavour (exec_bfd) != bfd_target_aout_flavour)
+    return;
+
   ALL_BP_LOCATIONS (loc)
   {
     struct breakpoint *b = loc->owner;
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 6aad520..805b834 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -19912,6 +19912,15 @@ We suggest that in response to this notification, front ends
 highlight the selected thread and cause subsequent commands to apply to
 that thread.
 
+@item =library-loaded,id="@var{id}",target-name="@var{target-name}",host-name="@var{host-name}",low-address="@var{low}",high-address="@var{high}",symbols-loaded="@var{loaded}"
+Reports that a new library file was loaded by the program.  The
+@var{id} field is an opaque identifier of the library. For remote
+debugging case, @var{target-name} and @var{host-name} fields give the 
+name of the library file on the target, and on the host respectively.
+
+@item =library-unloaded,id="@var{id}",target-name="@var{target-name}",host-name="@var{host-name}"
+Reports that a library was loaded by the program.
+
 @end table
 
 @node GDB/MI Frame Information
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index 95dfdf4..d32638c 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -34,6 +34,7 @@
 #include "mi-common.h"
 #include "observer.h"
 #include "gdbthread.h"
+#include "solist.h"
 
 /* These are the interpreter setup, etc. functions for the MI interpreter */
 static void mi_execute_command_wrapper (char *cmd);
@@ -58,6 +59,8 @@ static void mi_thread_exit (struct thread_info *t);
 static void mi_new_inferior (int pid);
 static void mi_inferior_exit (int pid);
 static void mi_on_resume (ptid_t ptid);
+static void mi_solib_loaded (struct so_list *solib);
+static void mi_solib_unloaded (struct so_list *solib);
 
 static void *
 mi_interpreter_init (int top_level)
@@ -86,6 +89,8 @@ mi_interpreter_init (int top_level)
       observer_attach_inferior_exit (mi_inferior_exit);
       observer_attach_normal_stop (mi_on_normal_stop);
       observer_attach_target_resumed (mi_on_resume);
+      observer_attach_solib_loaded (mi_solib_loaded);
+      observer_attach_solib_unloaded (mi_solib_unloaded);
     }
 
   return mi;
@@ -408,6 +413,31 @@ mi_on_resume (ptid_t ptid)
   gdb_flush (raw_stdout);
 }
 
+static void mi_solib_loaded (struct so_list *solib)
+{
+  struct mi_interp *mi = top_level_interpreter_data ();
+  target_terminal_ours ();
+  fprintf_unfiltered (mi->event_channel, 
+		      "library-loaded,id=\"%s\",target-name=\"%s\",host-name=\"%s\",low-address=\"0x%s\",high-address=\"0x%s\",symbols-loaded=\"%d\"", 
+		      solib->so_original_name, solib->so_original_name, 
+		      solib->so_name, 
+		      paddr (solib->addr_low), paddr (solib->addr_high), 
+		      solib->symbols_loaded);
+  gdb_flush (mi->event_channel);
+}
+
+static void mi_solib_unloaded (struct so_list *solib)
+{
+  struct mi_interp *mi = top_level_interpreter_data ();
+  target_terminal_ours ();
+  fprintf_unfiltered (mi->event_channel, 
+		      "library-unloaded,id=\"%s\",target-name=\"%s\",host-name=\"%s\"", 
+		      solib->so_original_name, solib->so_original_name, 
+		      solib->so_name);
+  gdb_flush (mi->event_channel);
+}
+
+
 extern initialize_file_ftype _initialize_mi_interp; /* -Wmissing-prototypes */
 
 void
diff --git a/gdb/solib.c b/gdb/solib.c
index cce4f7f..5a28292 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -908,6 +908,7 @@ clear_solib (void)
     {
       struct so_list *so = so_list_head;
       so_list_head = so->next;
+      observer_notify_solib_unloaded (so);
       if (so->abfd)
 	remove_target_sections (so->abfd);
       free_so (so);
diff --git a/gdb/testsuite/gdb.mi/mi-nonstop.exp b/gdb/testsuite/gdb.mi/mi-nonstop.exp
index 2521ae5..08952b0 100644
--- a/gdb/testsuite/gdb.mi/mi-nonstop.exp
+++ b/gdb/testsuite/gdb.mi/mi-nonstop.exp
@@ -63,7 +63,7 @@ mi_gdb_test "200-break-insert -t main" ".*"
 set created "=thread-created,id=\"$decimal\"\r\n"
 set running "\\*running,thread-id=\"$decimal\"\r\n"
 
-set notifs "($created)*($running)*"
+set notifs "($created)*($running)*($library_loaded_re)*"
 
 # Note: presently, we skip this test on non-native targets,
 # so 'run' is OK.  As soon as we start to run this on remote
diff --git a/gdb/testsuite/gdb.mi/mi-nsintrall.exp b/gdb/testsuite/gdb.mi/mi-nsintrall.exp
index b0d7b71..d34b08d 100644
--- a/gdb/testsuite/gdb.mi/mi-nsintrall.exp
+++ b/gdb/testsuite/gdb.mi/mi-nsintrall.exp
@@ -62,7 +62,7 @@ mi_gdb_test "200-break-insert -t main" ".*"
 set created "=thread-created,id=\"$decimal\"\r\n"
 set running "\\*running,thread-id=\"$decimal\"\r\n"
 
-set notifs "($created)*($running)*"
+set notifs "($created)*($running)*($library_loaded_re)*"
 
 # Note: presently, we skip this test on non-native targets,
 # so 'run' is OK.  As soon as we start to run this on remote
diff --git a/gdb/testsuite/gdb.mi/mi-nsmoribund.exp b/gdb/testsuite/gdb.mi/mi-nsmoribund.exp
index b836c5b..505fc84 100644
--- a/gdb/testsuite/gdb.mi/mi-nsmoribund.exp
+++ b/gdb/testsuite/gdb.mi/mi-nsmoribund.exp
@@ -62,7 +62,7 @@ mi_gdb_test "200-break-insert -t main" ".*"
 set created "=thread-created,id=\"$decimal\"\r\n"
 set running "\\*running,thread-id=\"$decimal\"\r\n"
 
-set notifs "($created)*($running)*"
+set notifs "($created)*($running)*($library_loaded_re)*"
 
 # Note: presently, we skip this test on non-native targets,
 # so 'run' is OK.  As soon as we start to run this on remote
diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index a2aa629..f62c240 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -31,6 +31,7 @@ global mi_inferior_tty_name
 set MIFLAGS "-i=mi"
 
 set thread_selected_re "=thread-selected,id=\"\[0-9+\]\"\r\n"
+set library_loaded_re "=library-loaded\[^\n\]+\"\r\n"
 
 #
 # mi_gdb_exit -- exit the GDB, killing the target program if necessary
@@ -778,6 +779,7 @@ proc mi_run_cmd {args} {
     }
     global mi_gdb_prompt
     global thread_selected_re
+    global library_loaded_re
 
     if [target_info exists gdb_init_command] {
 	send_gdb "[target_info gdb_init_command]\n";
@@ -819,7 +821,7 @@ proc mi_run_cmd {args} {
 
     send_gdb "220-exec-run $args\n"
     gdb_expect {
-	-re "220\\^running\r\n(\\*running,thread-id=\"\[^\"\]+\"\r\n|=thread-created,id=\"1\",group-id=\"\[0-9\]+\"\r\n)*(${thread_selected_re})?${mi_gdb_prompt}" {
+	-re "220\\^running\r\n(\\*running,thread-id=\"\[^\"\]+\"\r\n|=thread-created,id=\"1\",group-id=\"\[0-9\]+\"\r\n)*(${library_loaded_re})*(${thread_selected_re})?${mi_gdb_prompt}" {
 	}
 	timeout {
 	    perror "Unable to start target"
@@ -1435,10 +1437,11 @@ proc mi_send_resuming_command_raw {command test} {
 
     global mi_gdb_prompt
     global thread_selected_re
+    global library_loaded_re
 
     send_gdb "$command\n"
     gdb_expect {
-        -re "\\^running\r\n\\*running,thread-id=\"\[^\"\]+\"\r\n($thread_selected_re)?${mi_gdb_prompt}" {
+        -re "\\^running\r\n\\*running,thread-id=\"\[^\"\]+\"\r\n($library_loaded_re)*($thread_selected_re)?${mi_gdb_prompt}" {
             # Note that lack of 'pass' call here -- this works around limitation
             # in DejaGNU xfail mechanism. mi-until.exp has this:
             #

Re: MI solib notification

[Pedro Alves]

On Thursday 12 February 2009 15:13:37, Daniel Jacobowitz wrote:
> On Thu, Feb 12, 2009 at 06:01:54PM +0300, Vladimir Prus wrote:
> > Yes, this runs everywhere except a.out and marks all breakpoints in solibs
> > disabled, without printing any messages. *Then* we look over all solibs
> > and call observer, but since all breakpoints are already disabled,
> > breakpoint.c:disable_breakpoints_in_unloaded_shlib won't do anything,
> > and won't say anything either.
> 
> I see.  The comment in clear_solibs means that we will break SunOS
> a.out shared libraries if we do this; they were previously not
> disabled, since they won't ever be re-enabled.  So I suggest not
> making the observer call on a.out either.  Otherwise OK.

Sorry to pitch in this late, but, doesn't it sound wrong when
we end up conditionally calling observers for the benefit of
some specific observer?  When we do that, we're likelly to end
up adding some new observer in the future, for a different
purpose, and realise that it isn't getting called in some
situation.

I'd prefer in these cases to see us move in the direction where
the observer itself has the needed information to decide if it
should skip its whatever-action, or, to add a new
notification.

It seems there's a bit of overloading going on here:

- MI wants to know whenever GDB removes a shared library from
it's lists.  Be that when the inferior unloads the shared library,
or after the inferior having exited.  Because we currently
leave the shared list intact after the inferior having exited, and
only clear it on the subsequent re-run, this even means that
MI will get an =thread-group-exited notification when the inferior
exits, and only when the inferior is re-run, will MI output this
new =library-unloaded notification.  So, this shows that "unloaded"
means unloaded from gdb, not unloaded from the inferior, unless that
is fixed to be made consistent.  E.g., core files when they're closed
do clear the solist.

- The breakpoints.c:disable_breakpoints_in_unloaded_shlib observer
currently is notified whenever the inferior unloads an solib, but
not when we clear the list to start over.  If we want it to get
the notification also in that case, can't we push the a.out checks
down there instead?  If needed, we could add a parameter to the
notification, or come up with a new notification.

-- 
Pedro Alves

Re: MI solib notification

[Pedro Alves]

On Thursday 12 February 2009 15:13:37, Daniel Jacobowitz wrote:
> On Thu, Feb 12, 2009 at 06:01:54PM +0300, Vladimir Prus wrote:
> > Yes, this runs everywhere except a.out and marks all breakpoints in solibs
> > disabled, without printing any messages. *Then* we look over all solibs
> > and call observer, but since all breakpoints are already disabled,
> > breakpoint.c:disable_breakpoints_in_unloaded_shlib won't do anything,
> > and won't say anything either.
> 
> I see.  The comment in clear_solibs means that we will break SunOS
> a.out shared libraries if we do this; they were previously not
> disabled, since they won't ever be re-enabled.  So I suggest not
> making the observer call on a.out either.  Otherwise OK.

Sorry to pitch in this late, but, doesn't it sound wrong when
we end up conditionally calling observers for the benefit of
some specific observer?  When we do that, we're likelly to end
up adding some new observer in the future, for a different
purpose, and realise that it isn't getting called in some
situation.

I'd prefer in these cases to see us move in the direction where
the observer itself has the needed information to decide if it
should skip its whatever-action, or, to add a new
notification.

It seems there's a bit of overloading going on here:

- MI wants to know whenever GDB removes a shared library from
it's lists.  Be that when the inferior unloads the shared library,
or after the inferior having exited.  Because we currently
leave the shared list intact after the inferior having exited, and
only clear it on the subsequent re-run, this even means that
MI will get an =thread-group-exited notification when the inferior
exits, and only when the inferior is re-run, will MI output this
new =library-unloaded notification.  So, this shows that "unloaded"
means unloaded from gdb, not unloaded from the inferior, unless that
is fixed to be made consistent.  E.g., core files when they're closed
do clear the solist.

- The breakpoints.c:disable_breakpoints_in_unloaded_shlib observer
currently is notified whenever the inferior unloads an solib, but
not when we clear the list to start over.  If we want it to get
the notification also in that case, can't we push the a.out checks
down there instead?  If needed, we could add a parameter to the
notification, or come up with a new notification.

-- 
Pedro Alves

Re: MI solib notification

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> I'd prefer in these cases to see us move in the direction where
Pedro> the observer itself has the needed information to decide if it
Pedro> should skip its whatever-action, or, to add a new
Pedro> notification.

I completely agree.  Also, I'd like us to agree that this is a rule
that we'll apply in the future.  FWIW my impression is that we have
been moving in this direction already.

Tom

Re: MI solib notification

[Daniel Jacobowitz]

On Thu, Feb 12, 2009 at 12:43:29PM -0700, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Pedro> I'd prefer in these cases to see us move in the direction where
> Pedro> the observer itself has the needed information to decide if it
> Pedro> should skip its whatever-action, or, to add a new
> Pedro> notification.
> 
> I completely agree.  Also, I'd like us to agree that this is a rule
> that we'll apply in the future.  FWIW my impression is that we have
> been moving in this direction already.

I agree too; I was clearly wrong here.

-- 
Daniel Jacobowitz
CodeSourcery

Re: MI solib notification

[Joel Brobecker]

> I completely agree.  Also, I'd like us to agree that this is a rule
> that we'll apply in the future.  FWIW my impression is that we have
> been moving in this direction already.

Fully agreed.

-- 
Joel

Re: MI solib notification

[Daniel Jacobowitz]

One more question: should this show up in -list-features?

-- 
Daniel Jacobowitz
CodeSourcery