Re: [RFA]: Fix for pending breakpoints in manually loaded/unloaded shlibs

[Jeff Johnston <jjohnstn@redhat.com>]

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

The updated patch is attached.  The testcase had to be slightly modified to 
match the new warning message format.

2004-08-23  Jeff Johnston  <jjohnstn@redhat.com>

         * observer.sh: Add struct so_list declaration.
         * Makefile.in: Add dependencies on observer.h for solib.c and
         breakpoint.c.
         * breakpoint.c (disable_breakpoints_in_unloaded_shlib): New
         function.
         (_initialize_breakpoint): Register
         disable_breakpoints_in_unloaded_shlib as an observer of the
         "solib unloaded" observation event.
         (re_enable_breakpoints_in_shlibs): For bp_shlib_disabled breakpoints,
         call decode_line_1 so unfound breakpoint errors are silent.
         * solib.c (update_solib_list): When a solib is discovered to have
         been unloaded by the program, notify all observers of the
         "solib unloaded" observation event.


2004-08-23  Jeff Johnston  <jjohnstn@redhat.com>

         * gdb.base/unload.exp: Fix expected warning message to match
         latest format.


Ok to commit?

-- Jeff J.

Jeff Johnston wrote:
> Daniel Jacobowitz wrote:
> 
>> On Wed, Aug 18, 2004 at 03:22:22PM -0400, Jeff Johnston wrote:
>>
>>> Daniel Jacobowitz wrote:
>>>
>>>> On Wed, Aug 11, 2004 at 04:12:07PM -0400, Jeff Johnston wrote:
>>>>
>>>>> +    if (so_name +        && !strcmp (so_name, solib->so_name))
>>>>> +          {
>>>>> +        b->enable_state = bp_shlib_disabled;
>>>>> +        /* At this point, we cannot rely on remove_breakpoint
>>>>> +           succeeding so we must mark the breakpoint as not inserted
>>>>> +           to prevent future errors occurring in 
>>>>> remove_breakpoints.  */
>>>>> +        b->loc->inserted = 0;
>>>>> +        if (!disabled_shlib_breaks)
>>>>> +          {
>>>>> +        target_terminal_ours_for_output ();
>>>>> +        warning ("Temporarily disabling unloaded shared library 
>>>>> breakpoints:");
>>>>> +          }
>>>>> +        disabled_shlib_breaks = 1;
>>>>> +        warning ("breakpoint #%d ", b->number);
>>>>
>>>>
>>>>
>>>> I think you're missing a space after the colon, in the first warning.
>>>> Also, this use of multiple warning() statements is neither i18n
>>>> friendly nor MI/GUI friendly - you may get a separate dialog box for
>>>> each.  I believe other places do this with sprintf; still not 100% i18n
>>>> friendly, but avoids the MI/GUI problems.  I can't find an example
>>>> offhand.
>>>>
>>>
>>> What you do want to see so I don't waste my time on this.  As you 
>>> already know, this routine was copied from the routine which disables 
>>> shared library breakpoints in breakpoint.c.  Is it sufficient to just 
>>> issue the warning that I am temporarily disabling unloaded shared 
>>> library breakpoints and not spell out each breakpoint in turn?  I can 
>>> see this as really annoying and pointless to an end-user if there are 
>>> hundreds or thousands of breakpoints.
>>
>>
>>
>> That's a good idea.  How about this?
>>
>>     target_terminal_ours_for_output ();
>>     warning ("Temporarily disabling breakpoints for unloaded shared 
>> library \"%s\",
>>          so_name);
>>
> 
> Yes, that's exactly what I had in mind.  Consider it done plus your 
> other comments.  Eli, in light of what Daniel and Andrew have said 
> regarding the value of having an observer, may I repost with changes and 
> check the code in?
> 
> -- Jeff J.
> 
> 

[-- Attachment #2: unload.patch2 --]
[-- Type: text/plain, Size: 6724 bytes --]

Index: doc/observer.texi
===================================================================
RCS file: /cvs/src/src/gdb/doc/observer.texi,v
retrieving revision 1.7
diff -u -p -r1.7 observer.texi
--- doc/observer.texi	21 May 2004 16:04:03 -0000	1.7
+++ doc/observer.texi	23 Aug 2004 21:27:18 -0000
@@ -90,3 +90,8 @@ at the entry-point instruction.  For @sa
 @value{GDBN} calls this observer immediately after connecting to the
 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.
+@end deftypefun
+
Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.607
diff -u -p -r1.607 Makefile.in
--- Makefile.in	8 Aug 2004 19:27:09 -0000	1.607
+++ Makefile.in	23 Aug 2004 21:27:19 -0000
@@ -1718,7 +1718,7 @@ breakpoint.o: breakpoint.c $(defs_h) $(s
 	$(gdb_string_h) $(demangle_h) $(annotate_h) $(symfile_h) \
 	$(objfiles_h) $(source_h) $(linespec_h) $(completer_h) $(gdb_h) \
 	$(ui_out_h) $(cli_script_h) $(gdb_assert_h) $(block_h) \
-	$(gdb_events_h)
+	$(gdb_events_h) $(observer_h) $(solist_h)
 bsd-kvm.o: bsd-kvm.c $(defs_h) $(cli_cmds_h) $(command_h) $(frame_h) \
 	$(regcache_h) $(target_h) $(value_h) $(gdb_assert_h) $(readline_h) \
 	$(bsd_kvm_h)
@@ -2450,7 +2450,8 @@ solib-aix5.o: solib-aix5.c $(defs_h) $(g
 solib.o: solib.c $(defs_h) $(gdb_string_h) $(symtab_h) $(bfd_h) $(symfile_h) \
 	$(objfiles_h) $(gdbcore_h) $(command_h) $(target_h) $(frame_h) \
 	$(gdb_regex_h) $(inferior_h) $(environ_h) $(language_h) $(gdbcmd_h) \
-	$(completer_h) $(filenames_h) $(exec_h) $(solist_h) $(readline_h)
+	$(completer_h) $(filenames_h) $(exec_h) $(solist_h) $(readline_h) \
+	$(observer_h)
 solib-frv.o: solib-frv.c $(defs_h) $(gdb_string_h) $(inferior_h) \
 	$(gdbcore_h) $(solist_h) $(frv_tdep_h) $(objfiles_h) $(symtab_h) \
 	$(language_h) $(command_h) $(gdbcmd_h) $(elf_frv_h)
Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.179
diff -u -p -r1.179 breakpoint.c
--- breakpoint.c	28 Jul 2004 17:26:26 -0000	1.179
+++ breakpoint.c	23 Aug 2004 21:27:19 -0000
@@ -49,6 +49,8 @@
 #include "cli/cli-script.h"
 #include "gdb_assert.h"
 #include "block.h"
+#include "solist.h"
+#include "observer.h"
 
 #include "gdb-events.h"
 
@@ -4388,6 +4390,46 @@ disable_breakpoints_in_shlibs (int silen
   }
 }
 
+/* Disable any breakpoints that are in in an unloaded shared library.  Only
+   apply to enabled breakpoints, disabled ones can just stay disabled.  */
+
+void
+disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
+{
+  struct breakpoint *b;
+  int disabled_shlib_breaks = 0;
+
+#if defined (PC_SOLIB)
+  /* See also: insert_breakpoints, under DISABLE_UNSETTABLE_BREAK.  */
+  ALL_BREAKPOINTS (b)
+  {
+    if ((b->loc->loc_type == bp_loc_hardware_breakpoint
+	|| b->loc->loc_type == bp_loc_software_breakpoint)
+	&& breakpoint_enabled (b) 
+	&& !b->loc->duplicate)
+      {
+	char *so_name = PC_SOLIB (b->loc->address);
+	if (so_name 
+	    && !strcmp (so_name, solib->so_name))
+          {
+	    b->enable_state = bp_shlib_disabled;
+	    /* At this point, we cannot rely on remove_breakpoint
+	       succeeding so we must mark the breakpoint as not inserted
+	       to prevent future errors occurring in remove_breakpoints.  */
+	    b->loc->inserted = 0;
+	    if (!disabled_shlib_breaks)
+	      {
+		target_terminal_ours_for_output ();
+		warning ("Temporarily disabling breakpoints for unloaded shared library \"%s\"",
+			  so_name);
+	      }
+	    disabled_shlib_breaks = 1;
+	  }
+      }
+  }
+#endif
+}
+
 /* Try to reenable any breakpoints in shared libraries.  */
 void
 re_enable_breakpoints_in_shlibs (void)
@@ -7100,6 +7142,8 @@ breakpoint_re_set_one (void *bint)
   struct breakpoint *b = (struct breakpoint *) bint;
   struct value *mark;
   int i;
+  int not_found;
+  int *not_found_ptr = NULL;
   struct symtabs_and_lines sals;
   char *s;
   enum enable_state save_enable;
@@ -7150,11 +7194,19 @@ breakpoint_re_set_one (void *bint)
       save_enable = b->enable_state;
       if (b->enable_state != bp_shlib_disabled)
         b->enable_state = bp_disabled;
+      else
+	/* If resetting a shlib-disabled breakpoint, we don't want to
+	   see an error message if it is not found since we will expect
+	   this to occur until the shared library is finally reloaded.
+	   We accomplish this by giving decode_line_1 a pointer to use
+	   for silent notification that the symbol is not found.  */
+	not_found_ptr = &not_found;
 
       set_language (b->language);
       input_radix = b->input_radix;
       s = b->addr_string;
-      sals = decode_line_1 (&s, 1, (struct symtab *) NULL, 0, (char ***) NULL, NULL);
+      sals = decode_line_1 (&s, 1, (struct symtab *) NULL, 0, (char ***) NULL,
+		            not_found_ptr);
       for (i = 0; i < sals.nelts; i++)
 	{
 	  resolve_sal_pc (&sals.sals[i]);
@@ -7755,6 +7807,10 @@ _initialize_breakpoint (void)
   static struct cmd_list_element *breakpoint_show_cmdlist;
   struct cmd_list_element *c;
 
+#ifdef SOLIB_ADD
+  observer_attach_solib_unloaded (disable_breakpoints_in_unloaded_shlib);
+#endif
+
   breakpoint_chain = 0;
   /* Don't bother to call set_breakpoint_count.  $bpnum isn't useful
      before a breakpoint is set.  */
Index: observer.sh
===================================================================
RCS file: /cvs/src/src/gdb/observer.sh,v
retrieving revision 1.3
diff -u -p -r1.3 observer.sh
--- observer.sh	7 May 2004 22:51:52 -0000	1.3
+++ observer.sh	23 Aug 2004 21:27:19 -0000
@@ -52,6 +52,7 @@ case $lang in
 
 struct observer;
 struct bpstats;
+struct so_list;
 EOF
         ;;
 esac
Index: solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.66
diff -u -p -r1.66 solib.c
--- solib.c	30 Jul 2004 19:17:19 -0000	1.66
+++ solib.c	23 Aug 2004 21:27:19 -0000
@@ -42,6 +42,7 @@
 #include "filenames.h"		/* for DOSish file names */
 #include "exec.h"
 #include "solist.h"
+#include "observer.h"
 #include "readline/readline.h"
 
 /* external data declarations */
@@ -478,6 +479,10 @@ 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.  */
+	  observer_notify_solib_unloaded (gdb);
+
 	  *gdb_link = gdb->next;
 
 	  /* Unless the user loaded it explicitly, free SO's objfile.  */

[-- Attachment #3: unloadtest.patch --]
[-- Type: text/plain, Size: 1138 bytes --]

Index: gdb.base/unload.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/unload.exp,v
retrieving revision 1.1
diff -u -p -r1.1 unload.exp
--- gdb.base/unload.exp	12 Aug 2004 20:22:59 -0000	1.1
+++ gdb.base/unload.exp	23 Aug 2004 21:30:34 -0000
@@ -126,7 +126,7 @@ Breakpoint.*, shrfunc1 \\\(x=3\\\).*unlo
 "running program"
 
 gdb_test "continue" \
-"Continuing.*y is 7.*warning: Temporarily disabling unloaded shared library breakpoints.*warning: breakpoint #.*Program exited normally." \
+"Continuing.*y is 7.*warning: Temporarily disabling breakpoints for.*unloadshr.sl.*Program exited normally." \
 "continuing to end of program"
 
 #
@@ -138,5 +138,6 @@ gdb_test "run" \
 "rerun to shared library breakpoint"
 
 gdb_test "continue" \
-"Continuing.*y is 7.*warning: Temporarily disabling unloaded shared library breakpoints.*warning: breakpoint #.*Program exited normally." \
-"continuing to end of program second time"
+"Continuing.*y is 7.*warning: Temporarily disabling breakpoints for.*unloadshr.sl.*Program exited normally." \
+"continuing to end of program"
+