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

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

[Jeff Johnston]

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

The following patch fixes a problem with breakpoints set in shlibs that are 
manually loaded/unloaded by the program.  What currently happens is that pending 
breakpoints work properly for the first run of the program.  On the 2nd run, the 
resolved breakpoint(s) can end up at the start of the breakpoint list and is 
marked bp_shlib_disabled.  This is fine for a bit and we reach the breakpoint 
again when the shared library is loaded.  However, when we unload the 2nd time, 
there is trouble.  We eventually get a shlib_event from the dlclose() and we 
attempt to remove the breakpoint to step over it.  Unfortunately, we try and 
remove all breakpoints and we end attempting to remove a breakpoint that no 
longer exists (remember the breakpoint for the shared library routine is now at 
the start of the breakpoint list).  We fail trying to remove the first 
breakpoint and end up failing remove_breakpoints.  We subsequently keep running 
into the shlib_event breakpoint over and over again ad-infinitum.

To fix this, I have added an observer for a new event: solib_unloaded.  When 
update_solib_list discovers a shared library has been unloaded, it notifies all 
observers (initially this is just breakpoint.c).  Breakpoint.c sets up an 
observer to find all breakpoints in the removed shlib and mark them as 
non-inserted and bp_shlib_disabled.  This solves the problem.

I also added code to re_enable_breakpoints_in_shlibs to remove the error 
messages we get when we go and rerun the program (it attempts to find shlib 
breakpoints in every shared library that gets loaded).

I have included a new test case which exercises the scenario.

Ok to commit?

-- Jeff J.

2004-08-10  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-10  Jeff Johnston  <jjohnstn@redhat.com>

         * gdb.base/unload.exp: New test for breakpoints in dynamically
         loaded libraries.
         * gdb.base/unload.c: Ditto.
         * gdb.base/unloadshr.c: Ditto.



[-- Attachment #2: unload.patch --]
[-- Type: text/plain, Size: 6500 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	10 Aug 2004 18:47:47 -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	10 Aug 2004 18:47:48 -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)
 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	10 Aug 2004 18:47:48 -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,42 @@ 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;
+
+  /* See also: insert_breakpoints, under DISABLE_UNSETTABLE_BREAK. */
+  ALL_BREAKPOINTS (b)
+  {
+#if defined (PC_SOLIB)
+    if (((b->type == bp_breakpoint) ||
+	 (b->type == bp_hardware_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;
+	    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);
+	  }
+      }
+#endif
+  }
+}
 /* Try to reenable any breakpoints in shared libraries.  */
 void
 re_enable_breakpoints_in_shlibs (void)
@@ -7100,6 +7138,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 +7190,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 +7803,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	10 Aug 2004 18:47:48 -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	10 Aug 2004 18:47:48 -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: 8288 bytes --]

Index: gdb.base/unload.c
===================================================================
RCS file: gdb.base/unload.c
diff -N gdb.base/unload.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb.base/unload.c	10 Aug 2004 18:40:26 -0000
@@ -0,0 +1,60 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2004 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+ 
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software
+   Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+
+   Please email any bugs, comments, and/or additions to this file to:
+   bug-gdb@prep.ai.mit.edu  */
+
+#include <stdio.h>
+#include <dlfcn.h>
+
+int k = 0;
+
+#define SHLIB_NAME SHLIB_DIR "/unloadshr.sl"
+
+int main()
+{
+  void *handle;
+  int (*unloadshr) (int);
+  int y;
+  char *msg;
+
+  handle = dlopen (SHLIB_NAME, RTLD_LAZY);
+  msg = dlerror ();
+  
+  if (!handle)
+    {
+      fprintf (stderr, msg);
+      exit (1);
+    }
+
+  unloadshr = (int (*)(int))dlsym (handle, "shrfunc1");
+
+  if (!unloadshr)
+    {
+      fprintf (stderr, dlerror ());
+      exit (1);
+    }
+
+  y = (*unloadshr)(3);
+
+  printf ("y is %d\n", y);
+
+  dlclose (handle);
+
+  return 0;
+}
Index: gdb.base/unload.exp
===================================================================
RCS file: gdb.base/unload.exp
diff -N gdb.base/unload.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb.base/unload.exp	10 Aug 2004 18:40:26 -0000
@@ -0,0 +1,148 @@
+#   Copyright 2003, 2004
+#   Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+# 
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+# 
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  
+
+# Please email any bugs, comments, and/or additions to this file to:
+# bug-gdb@prep.ai.mit.edu
+
+# This file was created by Jeff Johnston. (jjohnstn@redhat.com)
+# The shared library compilation portion was copied from shlib-call.exp which was
+# written by Elena Zannoni (ezannoni@redhat.com).
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+#
+# test running programs
+#
+set prms_id 0
+set bug_id 0
+
+# are we on a target board?
+if ![isnative] then {
+    return 0
+}
+
+set testfile "unload"
+set libfile "unloadshr"
+set libsrcfile ${libfile}.c
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+set shlibdir ${objdir}/${subdir}
+
+if [get_compiler_info ${binfile}] {
+    return -1
+}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug "additional_flags=-DSHLIB_DIR\=\"${shlibdir}\"" "libs=-ldl"]] != "" } {
+     gdb_suppress_entire_file "Testcase compile failed, so all tests in this file will automatically fail."
+}
+
+# Build the shared libraries this test case needs.
+#
+
+if {$gcc_compiled == 0} {
+    if [istarget "hppa*-hp-hpux*"] then {
+	set additional_flags "additional_flags=+z"
+    } elseif { [istarget "mips-sgi-irix*"] } {
+	# Disable SGI compiler's implicit -Dsgi
+	set additional_flags "additional_flags=-Usgi"
+    } else {
+	# don't know what the compiler is...
+	set additional_flags ""
+    }
+} else {
+    if { ([istarget "powerpc*-*-aix*"]
+       || [istarget "rs6000*-*-aix*"]) } {
+	set additional_flags ""
+    } else {
+	set additional_flags "additional_flags=-fpic"
+    }
+}
+
+if {[gdb_compile "${srcdir}/${subdir}/${libsrcfile}" "${objdir}/${subdir}/${libfile}.o" object [list debug $additional_flags]] != ""} {
+     gdb_suppress_entire_file "Testcase compile failed, so all tests in this file will automatically fail."
+}
+
+if [istarget "hppa*-*-hpux*"] {
+    remote_exec build "ld -b ${objdir}/${subdir}/${libfile}.o -o ${objdir}/${subdir}/${libfile}.sl"
+} else {
+    set additional_flags "additional_flags=-shared"
+    if {[gdb_compile "${objdir}/${subdir}/${libfile}.o" "${objdir}/${subdir}/${libfile}.sl" executable [list debug $additional_flags]] != ""} {
+	gdb_suppress_entire_file "Testcase compile failed, so all tests in this file will automatically fail."
+    }
+}
+
+if { ($gcc_compiled 
+      &&  ([istarget "powerpc*-*-aix*"]
+	|| [istarget "rs6000*-*-aix*"] )) } {
+    set additional_flags "additional_flags=-L${objdir}/${subdir}"
+} elseif { [istarget "mips-sgi-irix*"] } {
+    set additional_flags "additional_flags=-rpath ${objdir}/${subdir}"
+} else {
+    set additional_flags ""
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if [target_info exists gdb_stub] {
+    gdb_step_for_stub;
+}
+
+#
+# Test setting a breakpoint in a dynamically loaded library which is
+# manually loaded and unloaded
+#
+
+gdb_test_multiple "break shrfunc1" "set pending breakpoint" {
+     -re ".*Make breakpoint pending.*y or \\\[n\\\]. $" {
+	    gdb_test "y" "Breakpoint.*shrfunc1.*pending." "set pending breakpoint"
+     }
+}
+
+gdb_test "info break" \
+    "Num Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+breakpoint     keep y.*PENDING.*shrfunc1.*" \
+"single pending breakpoint info"
+
+set unloadshr_line [gdb_get_line_number "unloadshr break" ${srcdir}/${subdir}/${libsrcfile}]
+
+gdb_test "run" \
+"Starting program.*unload.*
+Breakpoint.*at.*
+Pending breakpoint \"shrfunc1\" resolved.*
+Breakpoint.*, shrfunc1 \\\(x=3\\\).*unloadshr.c:$unloadshr_line.*" \
+"running program"
+
+gdb_test "continue" \
+"Continuing.*y is 7.*warning: Temporarily disabling unloaded shared library breakpoints.*warning: breakpoint #.*Program exited normally." \
+"continuing to end of program"
+
+#
+# Try to rerun program and verify that shared breakpoint is reset properly
+#
+
+gdb_test "run" \
+".*Breakpoint.*shrfunc1.*at.*unloadshr.c:$unloadshr_line.*" \
+"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"
Index: gdb.base/unloadshr.c
===================================================================
RCS file: gdb.base/unloadshr.c
diff -N gdb.base/unloadshr.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb.base/unloadshr.c	10 Aug 2004 18:40:26 -0000
@@ -0,0 +1,27 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2004 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+ 
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software
+   Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+
+   Please email any bugs, comments, and/or additions to this file to:
+   bug-gdb@prep.ai.mit.edu  */
+
+#include <stdio.h>
+
+int shrfunc1 (int x)
+{
+  return x + 4; /* unloadshr break */
+}

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

[Kevin Buettner]

On Tue, 10 Aug 2004 15:09:37 -0400
Jeff Johnston <jjohnstn@redhat.com> wrote:

> 2004-08-10  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.

Your changes to solib.c are approved.

Kevin

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

[Eli Zaretskii]

> Date: Tue, 10 Aug 2004 15:09:37 -0400
> From: Jeff Johnston <jjohnstn@redhat.com>
> 
> To fix this, I have added an observer for a new event: solib_unloaded.  When 
> update_solib_list discovers a shared library has been unloaded, it notifies all 
> observers (initially this is just breakpoint.c).  Breakpoint.c sets up an 
> observer to find all breakpoints in the removed shlib and mark them as 
> non-inserted and bp_shlib_disabled.  This solves the problem.

Could you please explain why this complicated mechanism is needed to
fix this problem?  Why cannot GDB directly mark the breakpoints when
the library is unloaded, instead of going the observer path?

> 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	10 Aug 2004 18:47:47 -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
> +

This part is approved (provided that the rest of the patch is
aproved).

> +/* Disable any breakpoints that are in in an unloaded shared library.  Only
                                       ^^^^^
A typo.

> +void
> +disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
> +{
> +  struct breakpoint *b;
> +  int disabled_shlib_breaks = 0;
> +
> +  /* See also: insert_breakpoints, under DISABLE_UNSETTABLE_BREAK. */
> +  ALL_BREAKPOINTS (b)
> +  {
> +#if defined (PC_SOLIB)

I think this #ifdef should be outside the loop: why loop at all if we
do nothing inside the loop body?

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

[Michael Chastain]

Hi Jeff,

What system(s) did you test this on?

For a shared library test the binutils version is important so
include that too.

Do you have access to any non-linux systems?  I think this needs to be
tested on at least one non-linux system before it goes in.  If you can't
do it, I can hop on an hpux machine and do that part.

  Please email any bugs, comments, and/or additions to this file to:
  bug-gdb@prep.ai.mit.edu  */

    This address is dead, please drop these lines.

All the gunk to build shared libraries cries out for some library
code in lib/gdb.exp or lib/dl-support.exp.  That's my job;
it's okay for you to keep cloning the code until I get to this.

Once I get past that the rest of unload.exp looks okay.

Michael C

===

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

         * gdb.base/unload.exp: New test for breakpoints in dynamically
         loaded libraries.
         * gdb.base/unload.c: Ditto.
         * gdb.base/unloadshr.c: Ditto.

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

[Jeff Johnston]

Michael Chastain wrote:
> Hi Jeff,
> 
> What system(s) did you test this on?
>

I tested on a Red Hat x86-linux system.  I am fixing a bug reported on Red Hat 
Linux.

> For a shared library test the binutils version is important so
> include that too.
> 

binutils-2.14.90.0.4-26.3

> Do you have access to any non-linux systems?  I think this needs to be
> tested on at least one non-linux system before it goes in.  If you can't
> do it, I can hop on an hpux machine and do that part.
>

If you could do that, that would be appreciated.

>   Please email any bugs, comments, and/or additions to this file to:
>   bug-gdb@prep.ai.mit.edu  */
> 
>     This address is dead, please drop these lines.
> 
> All the gunk to build shared libraries cries out for some library
> code in lib/gdb.exp or lib/dl-support.exp.  That's my job;
> it's okay for you to keep cloning the code until I get to this.
> 
> Once I get past that the rest of unload.exp looks okay.
> 

I'll remove the lines.  Thanks.

> Michael C
> 
> ===
> 
> 2004-08-10  Jeff Johnston  <jjohnstn@redhat.com>
> 
>          * gdb.base/unload.exp: New test for breakpoints in dynamically
>          loaded libraries.
>          * gdb.base/unload.c: Ditto.
>          * gdb.base/unloadshr.c: Ditto.
> 

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

[Jeff Johnston]

Eli Zaretskii wrote:
>>Date: Tue, 10 Aug 2004 15:09:37 -0400
>>From: Jeff Johnston <jjohnstn@redhat.com>
>>
>>To fix this, I have added an observer for a new event: solib_unloaded.  When 
>>update_solib_list discovers a shared library has been unloaded, it notifies all 
>>observers (initially this is just breakpoint.c).  Breakpoint.c sets up an 
>>observer to find all breakpoints in the removed shlib and mark them as 
>>non-inserted and bp_shlib_disabled.  This solves the problem.
> 
> 
> Could you please explain why this complicated mechanism is needed to
> fix this problem?  Why cannot GDB directly mark the breakpoints when
> the library is unloaded, instead of going the observer path?
>

Kind of damned if I do, damned if I don't :) I had talked over the problem with 
Andrew and he suggested the observer path as this is an event that other parts 
of gdb would be interested in knowing about.  IMHO, the mechanism is rather 
elegant, extensible, and simple to use.

Do you feel that this event isn't worthy of observation?

-- Jeff J.

> 
>>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	10 Aug 2004 18:47:47 -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
>>+
> 
> 
> This part is approved (provided that the rest of the patch is
> aproved).
> 
> 
>>+/* Disable any breakpoints that are in in an unloaded shared library.  Only
> 
>                                        ^^^^^
> A typo.
> 
> 
>>+void
>>+disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
>>+{
>>+  struct breakpoint *b;
>>+  int disabled_shlib_breaks = 0;
>>+
>>+  /* See also: insert_breakpoints, under DISABLE_UNSETTABLE_BREAK. */
>>+  ALL_BREAKPOINTS (b)
>>+  {
>>+#if defined (PC_SOLIB)
> 
> 
> I think this #ifdef should be outside the loop: why loop at all if we
> do nothing inside the loop body?
> 

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

[Andrew Cagney]

> Eli Zaretskii wrote:
> 
>>> Date: Tue, 10 Aug 2004 15:09:37 -0400
>>> From: Jeff Johnston <jjohnstn@redhat.com>
>>>
>>> To fix this, I have added an observer for a new event: solib_unloaded.  When update_solib_list discovers a shared library has been unloaded, it notifies all observers (initially this is just breakpoint.c).  Breakpoint.c sets up an observer to find all breakpoints in the removed shlib and mark them as non-inserted and bp_shlib_disabled.  This solves the problem.
>>
>>
>>
>> Could you please explain why this complicated mechanism is needed to
>> fix this problem?  Why cannot GDB directly mark the breakpoints when
>> the library is unloaded, instead of going the observer path?
>>
> 
> Kind of damned if I do, damned if I don't :) I had talked over the problem with Andrew and he suggested the observer path as this is an event that other parts of gdb would be interested in knowing about.  IMHO, the mechanism is rather elegant, extensible, and simple to use.

Right.  In addition to the breakpoints, the MI needs to know about these 
identical events (so that it can notify the gui of an shlib unload).

Andrew

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

[Daniel Jacobowitz]

On Tue, Aug 10, 2004 at 03:09:37PM -0400, Jeff Johnston wrote:
> The following patch fixes a problem with breakpoints set in shlibs that are 
> manually loaded/unloaded by the program.  What currently happens is that 
> pending breakpoints work properly for the first run of the program.  On the 
> 2nd run, the resolved breakpoint(s) can end up at the start of the 
> breakpoint list and is marked bp_shlib_disabled.  This is fine for a bit 
> and we reach the breakpoint again when the shared library is loaded.  
> However, when we unload the 2nd time, there is trouble.  We eventually get 
> a shlib_event from the dlclose() and we attempt to remove the breakpoint to 
> step over it.  Unfortunately, we try and remove all breakpoints and we end 
> attempting to remove a breakpoint that no longer exists (remember the 
> breakpoint for the shared library routine is now at the start of the 
> breakpoint list).  We fail trying to remove the first breakpoint and end up 
> failing remove_breakpoints.  We subsequently keep running into the 
> shlib_event breakpoint over and over again ad-infinitum.

I couldn't quite follow your explanation of the problem, but FWIW your
patch does make sense to me.

Please check it for coding style problems; I noticed a lot of operators
at the end of lines instead of the beginning of the next line.

> +#if defined (PC_SOLIB)
> +    if (((b->type == bp_breakpoint) ||
> +	 (b->type == bp_hardware_breakpoint)) &&
> +	breakpoint_enabled (b) &&
> +	!b->loc->duplicate)

You are just grabbing this from disable_breakpoints_in_shlibs, but the
b->type check is not correct.  Try
  (b->loc->type == bp_loc_hardware_breakpoint
   || b->loc->type == bp_loc_software_breakpoint)

[Conceptually, you want any breakpoint which corresponds to a code
address.]

Can this code be commonized rather than duplicated?

> +      {
> +	char *so_name = PC_SOLIB (b->loc->address);
> +	if (so_name &&
> +	    !strcmp (so_name, solib->so_name))
> +          {
> +	    b->enable_state = bp_shlib_disabled;
> +	    b->loc->inserted = 0;

Are we guaranteed that the breakpoint is not inserted right now?  This
is the only place in breakpoint.c that changes the inserted flag
directly other than initialization, insertion, and a hack for exec
following.

If you expect that the library has already been unmapped, so removing
it would fail, please add a comment saying so.

-- 
Daniel Jacobowitz

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

[Eli Zaretskii]

> Date: Wed, 11 Aug 2004 12:57:54 -0400
> From: Andrew Cagney <cagney@gnu.org>
> 
> Right.  In addition to the breakpoints, the MI needs to know about these 
> identical events (so that it can notify the gui of an shlib unload).

Not that I'm against this, but perhaps instead of implementing a core
GDB feature based on the observers (which, as far as I understand,
were invented for a different purpose), we should talk about two
separate changes: one that adds the observer, and another that fixes
the problem reported by Jeff.

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

[Jeff Johnston]

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

Daniel Jacobowitz wrote:
> On Tue, Aug 10, 2004 at 03:09:37PM -0400, Jeff Johnston wrote:
> 
>>The following patch fixes a problem with breakpoints set in shlibs that are 
>>manually loaded/unloaded by the program.  What currently happens is that 
>>pending breakpoints work properly for the first run of the program.  On the 
>>2nd run, the resolved breakpoint(s) can end up at the start of the 
>>breakpoint list and is marked bp_shlib_disabled.  This is fine for a bit 
>>and we reach the breakpoint again when the shared library is loaded.  
>>However, when we unload the 2nd time, there is trouble.  We eventually get 
>>a shlib_event from the dlclose() and we attempt to remove the breakpoint to 
>>step over it.  Unfortunately, we try and remove all breakpoints and we end 
>>attempting to remove a breakpoint that no longer exists (remember the 
>>breakpoint for the shared library routine is now at the start of the 
>>breakpoint list).  We fail trying to remove the first breakpoint and end up 
>>failing remove_breakpoints.  We subsequently keep running into the 
>>shlib_event breakpoint over and over again ad-infinitum.
> 
> 
> I couldn't quite follow your explanation of the problem, but FWIW your
> patch does make sense to me.
> 
> Please check it for coding style problems; I noticed a lot of operators
> at the end of lines instead of the beginning of the next line.
>

Noted.  Fixed in appended patch.

> 
>>+#if defined (PC_SOLIB)
>>+    if (((b->type == bp_breakpoint) ||
>>+	 (b->type == bp_hardware_breakpoint)) &&
>>+	breakpoint_enabled (b) &&
>>+	!b->loc->duplicate)
> 
> 
> You are just grabbing this from disable_breakpoints_in_shlibs, but the
> b->type check is not correct.  Try
>   (b->loc->type == bp_loc_hardware_breakpoint
>    || b->loc->type == bp_loc_software_breakpoint)
> 

Done.

> [Conceptually, you want any breakpoint which corresponds to a code
> address.]
> 
> Can this code be commonized rather than duplicated?
>

I don't see much value; the code is small, the test is now different, the input 
parameter is different, and there is resetting of the inserted flag.

> 
>>+      {
>>+	char *so_name = PC_SOLIB (b->loc->address);
>>+	if (so_name &&
>>+	    !strcmp (so_name, solib->so_name))
>>+          {
>>+	    b->enable_state = bp_shlib_disabled;
>>+	    b->loc->inserted = 0;
> 
> 
> Are we guaranteed that the breakpoint is not inserted right now?  This
> is the only place in breakpoint.c that changes the inserted flag
> directly other than initialization, insertion, and a hack for exec
> following.
> 
> If you expect that the library has already been unmapped, so removing
> it would fail, please add a comment saying so.
>

Putting remove_breakpoint() at that point does not work.  I have put the 
following comment:

+           /* 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.  */

Ok?

-- Jeff J.

[-- Attachment #2: unload.patch2 --]
[-- Type: text/plain, Size: 6744 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	11 Aug 2004 17:48:25 -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	11 Aug 2004 17:48:26 -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	11 Aug 2004 17:48:26 -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;
+
+  /* See also: insert_breakpoints, under DISABLE_UNSETTABLE_BREAK. */
+  ALL_BREAKPOINTS (b)
+  {
+#if defined (PC_SOLIB)
+    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 unloaded shared library breakpoints:");
+	      }
+	    disabled_shlib_breaks = 1;
+	    warning ("breakpoint #%d ", b->number);
+	  }
+      }
+#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	11 Aug 2004 17:48:26 -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	11 Aug 2004 17:48:26 -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.  */

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

[Andrew Cagney]

>>Date: Wed, 11 Aug 2004 12:57:54 -0400
>>> From: Andrew Cagney <cagney@gnu.org>
>>> 
>>> Right.  In addition to the breakpoints, the MI needs to know about these 
>>> identical events (so that it can notify the gui of an shlib unload).
> 
> 
> Not that I'm against this, but perhaps instead of implementing a core
> GDB feature based on the observers (which, as far as I understand,
> were invented for a different purpose), we should talk about two
> separate changes: one that adds the observer, and another that fixes
> the problem reported by Jeff.

Um, observers were introduced for this purpose - make it possible for 
anything, core or peripheral, to monitor changes in the inferior/GDB. 
Otherwize we end up with things like this.

	observer_notify_solib_unloaded (...);
	breakpoint_notify_solib_unloaded (...);

I'm currently investigating gdb.threads/static.exp failures and to fix 
that I think I'll need to add a further observer event.

Andrew

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

[Daniel Jacobowitz]

On Wed, Aug 11, 2004 at 04:42:24PM -0400, Andrew Cagney wrote:
> >>Date: Wed, 11 Aug 2004 12:57:54 -0400
> >>>From: Andrew Cagney <cagney@gnu.org>
> >>>
> >>>Right.  In addition to the breakpoints, the MI needs to know about these 
> >>>identical events (so that it can notify the gui of an shlib unload).
> >
> >
> >Not that I'm against this, but perhaps instead of implementing a core
> >GDB feature based on the observers (which, as far as I understand,
> >were invented for a different purpose), we should talk about two
> >separate changes: one that adds the observer, and another that fixes
> >the problem reported by Jeff.
> 
> Um, observers were introduced for this purpose - make it possible for 
> anything, core or peripheral, to monitor changes in the inferior/GDB. 
> Otherwize we end up with things like this.
> 
> 	observer_notify_solib_unloaded (...);
> 	breakpoint_notify_solib_unloaded (...);
> 
> I'm currently investigating gdb.threads/static.exp failures and to fix 
> that I think I'll need to add a further observer event.

Really?  I think thread-db needs to try to initialize at two points:
whenever a new shared library is loaded (converting the objfile hook to
an observer would be nice, but independent) and whenever an inferior is
created (conveniently we've got an observer for this already).

-- 
Daniel Jacobowitz

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

[Andrew Cagney]

>>I'm currently investigating gdb.threads/static.exp failures and to fix 
>>> that I think I'll need to add a further observer event.
> 
> 
> Really?

Really?

 > I think thread-db needs to try to initialize at two points:
> whenever a new shared library is loaded (converting the objfile hook to
> an observer would be nice, but independent) and whenever an inferior is
> created (conveniently we've got an observer for this already).

Think PIE.

For our purposes, both new executable loaded and new shlib loaded are 
the same event - there's been an objfile_loaded event.  There's no 
reason to differentiate them.

The new inferior event is orthogonal, and far more low level.

Andrew

(Of course this lets us eliminate one of the deprecated hooks)

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

[Andrew Cagney]

> To fix this, I have added an observer for a new event: solib_unloaded.  When update_solib_list discovers a shared library has been unloaded, it notifies all observers (initially this is just breakpoint.c).  Breakpoint.c sets up an observer to find all breakpoints in the removed shlib and mark them as non-inserted and bp_shlib_disabled.  This solves the problem.

The observer changes are ok (provided eli is ok with the doco description).

Andrew

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

[Eli Zaretskii]

> Date: Wed, 11 Aug 2004 16:42:24 -0400
> From: Andrew Cagney <cagney@gnu.org>
> 
> Um, observers were introduced for this purpose - make it possible for 
> anything, core or peripheral, to monitor changes in the inferior/GDB. 

But we don't want to _monitor_ anything in this case.  We want to fox
a specific, well-defined problem due to a specific course of events.

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

[Eli Zaretskii]

> Date: Wed, 11 Aug 2004 22:48:25 -0400
> From: Andrew Cagney <cagney@gnu.org>
> 
> The observer changes are ok (provided eli is ok with the doco description).

I already approved that part.

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

[Andrew Cagney]

>>> Date: Wed, 11 Aug 2004 16:42:24 -0400
>>> From: Andrew Cagney <cagney@gnu.org>
>>> 
>>> Um, observers were introduced for this purpose - make it possible for 
>>> anything, core or peripheral, to monitor changes in the inferior/GDB. 
> 
> 
> But we don't want to _monitor_ anything in this case.  We want to fox
> a specific, well-defined problem due to a specific course of events.

And that well defined course of events is called an shlib unload.  Lets 
abstract it as such.

Andrew

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

[Daniel Jacobowitz]

On Wed, Aug 11, 2004 at 06:19:07PM -0400, Andrew Cagney wrote:
> >>I'm currently investigating gdb.threads/static.exp failures and to fix 
> >>>that I think I'll need to add a further observer event.
> >
> >
> >Really?
> 
> Really?
> 
> > I think thread-db needs to try to initialize at two points:
> >whenever a new shared library is loaded (converting the objfile hook to
> >an observer would be nice, but independent) and whenever an inferior is
> >created (conveniently we've got an observer for this already).
> 
> Think PIE.

I am, in fact, thinking about pie.  Oh, you mean PIE...

> For our purposes, both new executable loaded and new shlib loaded are 
> the same event - there's been an objfile_loaded event.  There's no 
> reason to differentiate them.
> 
> The new inferior event is orthogonal, and far more low level.

I don't see how "new inferior" is any lower level than "new object".
In any case, thread-db wants to initialize when these two conditions
are true:
  (A) The inferior is running
and
  (B) The thread library has been loaded

So using an inferior created hook makes perfect sense to me...

-- 
Daniel Jacobowitz

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

[Michael Chastain]

I ran it on native hppa2.0w-hp-hpux11.11 and it barfed.

  Running /house/chastain/gdb/s1/gdb/testsuite/gdb.base/unload.exp ...
  gdb compile failed, /usr/ccs/bin/ld: Can't find library: "dl"
  WARNING: Testcase compile failed, so all tests in this file will automatically fail.

  ERROR: (timeout) GDB never initialized after 10 seconds.
  WARNING: remote_expect statement without a default case?!
  ... and so on ...

This is with hp ansi c B.11.11.28706.GP.

I looked at the hpux man page for dlopen and it looks similar to
the linux man page.  (And the linux man page says that their
interface comes from solaris).

The hpux man page for dlopen explicitly says "-ldl" but there is
no such library.

If I rip out the "-ldl" option" then the test script runs
and I get this:

  Running target unix
  Running /house/chastain/gdb/s1/gdb/testsuite/gdb.base/unload.exp ...
  PASS: gdb.base/unload.exp: set pending breakpoint
  PASS: gdb.base/unload.exp: single pending breakpoint info
  FAIL: gdb.base/unload.exp: running program
  FAIL: gdb.base/unload.exp: continuing to end of program
  FAIL: gdb.base/unload.exp: rerun to shared library breakpoint
  FAIL: gdb.base/unload.exp: continuing to end of program second time

Which is okay.  The test script is doing its job: making gdb ill.

I'll write up a little patch and send it to you.

Also I am working on a new "gdb_build" proc that will encapsulate all
this compiler-specific and target-specific mumbo-jumbo.  But for now
let's continue with keeping the logic in the test script, so you can
get the test script checked in.

Michael C

New observer objfile_mapped; was ...

[Andrew Cagney]

>>> Think PIE.
> 
> 
> I am, in fact, thinking about pie.  Oh, you mean PIE...
> 
> 
>>> For our purposes, both new executable loaded and new shlib loaded are 
>>> the same event - there's been an objfile_loaded event.  There's no 
>>> reason to differentiate them.
>>> 
>>> The new inferior event is orthogonal, and far more low level.
> 
> 
> I don't see how "new inferior" is any lower level than "new object".

At the time of inferior_created, nothing, zero zip is known about the 
inferior.  Things like the objfile loader and the shlib loader hang off 
of it.

> In any case, thread-db wants to initialize when these two conditions
> are true:
>   (A) The inferior is running
> and
>   (B) The thread library has been loaded
> 
> So using an inferior created hook makes perfect sense to me...

We're comparing:

-> inferior_created
	-> thread-db
-> solib loaded
	-> objfile mapped
		-> thread-db

with

-> inferior_created
	-> objfile mapped
		-> thread-db
-> shlib loaded
	-> objfile mapped
		-> thread-db

thread-db only needs to know when objfiles have been mapped in.

Andrew

Re: New observer objfile_mapped; was ...

[Daniel Jacobowitz]

On Thu, Aug 12, 2004 at 09:16:12AM -0400, Andrew Cagney wrote:
> We're comparing:
> 
> -> inferior_created
> 	-> thread-db
> -> solib loaded
> 	-> objfile mapped
> 		-> thread-db
> 
> with
> 
> -> inferior_created
> 	-> objfile mapped
> 		-> thread-db
> -> shlib loaded
> 	-> objfile mapped
> 		-> thread-db
> 
> thread-db only needs to know when objfiles have been mapped in.

Thanks, that makes better sense to me.

-- 
Daniel Jacobowitz

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

[Michael Chastain]

Okay, here's a patch for unload.exp to make it work on hpux 11.11.
I tested it on hpux 11.11 with both hp ansi c B.11.11.28706.GP and
gcc 3.3.4.  I didn't test it on any linux.

It will be easy to add more arms to the "switch" statement
as it gets tested on more operating systems.

Can you:

  throw this chunk in
  rip out the old bug-gdb address
  rip out the "-L" / "-rpath" leftovers (see below)
  re-test on i686-pc-linux-gnu
  re-post the patch (mention that you re-tested on i686-pc-linux-gnu)

Right before gdb_exit there is a chunk of code to set "-L" or "-rpath".
It looks dead because these values are never used after being set.
Can you rip that out?  Sorry I didn't catch that earlier.

Then I will re-test on native hppa2.0w-hp-hpux11.11 and that ought
to be good for approval.

Michael C

--- /house/chastain/u/unload.exp	2004-08-12 07:32:11.268088000 -0400
+++ unload.exp	2004-08-12 09:09:43.989638000 -0400
@@ -48,7 +48,14 @@
     return -1
 }
 
-if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug "additional_flags=-DSHLIB_DIR\=\"${shlibdir}\"" "libs=-ldl"]] != "" } {
+set dl_lib_flag ""
+switch -glob [istarget] {
+    "hppa*-hp-hpux*" { }
+    "*-*-linux*"     { set dl_lib_flag "libs=-ldl" }
+    default          { }
+}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug "additional_flags=-DSHLIB_DIR\=\"${shlibdir}\"" $dl_lib_flag]] != "" } {
      gdb_suppress_entire_file "Testcase compile failed, so all tests in this file will automatically fail."
 }
 

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

[Jeff Johnston]

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

Michael Chastain wrote:
> Okay, here's a patch for unload.exp to make it work on hpux 11.11.
> I tested it on hpux 11.11 with both hp ansi c B.11.11.28706.GP and
> gcc 3.3.4.  I didn't test it on any linux.
> 
> It will be easy to add more arms to the "switch" statement
> as it gets tested on more operating systems.
> 
> Can you:
> 
>   throw this chunk in
>   rip out the old bug-gdb address
>   rip out the "-L" / "-rpath" leftovers (see below)
>   re-test on i686-pc-linux-gnu
>   re-post the patch (mention that you re-tested on i686-pc-linux-gnu)
> 
> Right before gdb_exit there is a chunk of code to set "-L" or "-rpath".
> It looks dead because these values are never used after being set.
> Can you rip that out?  Sorry I didn't catch that earlier.
> 
> Then I will re-test on native hppa2.0w-hp-hpux11.11 and that ought
> to be good for approval.
>

Ok, see the attached patch.  The modified test runs fine on x86 linux.

Thanks,

-- Jeff J.

> Michael C
> 
> --- /house/chastain/u/unload.exp	2004-08-12 07:32:11.268088000 -0400
> +++ unload.exp	2004-08-12 09:09:43.989638000 -0400
> @@ -48,7 +48,14 @@
>      return -1
>  }
>  
> -if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug "additional_flags=-DSHLIB_DIR\=\"${shlibdir}\"" "libs=-ldl"]] != "" } {
> +set dl_lib_flag ""
> +switch -glob [istarget] {
> +    "hppa*-hp-hpux*" { }
> +    "*-*-linux*"     { set dl_lib_flag "libs=-ldl" }
> +    default          { }
> +}
> +
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug "additional_flags=-DSHLIB_DIR\=\"${shlibdir}\"" $dl_lib_flag]] != "" } {
>       gdb_suppress_entire_file "Testcase compile failed, so all tests in this file will automatically fail."
>  }
>  
> 

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

Index: gdb.base/unload.c
===================================================================
RCS file: gdb.base/unload.c
diff -N gdb.base/unload.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb.base/unload.c	12 Aug 2004 17:46:15 -0000
@@ -0,0 +1,57 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2004 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+ 
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software
+   Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. */
+
+#include <stdio.h>
+#include <dlfcn.h>
+
+int k = 0;
+
+#define SHLIB_NAME SHLIB_DIR "/unloadshr.sl"
+
+int main()
+{
+  void *handle;
+  int (*unloadshr) (int);
+  int y;
+  char *msg;
+
+  handle = dlopen (SHLIB_NAME, RTLD_LAZY);
+  msg = dlerror ();
+  
+  if (!handle)
+    {
+      fprintf (stderr, msg);
+      exit (1);
+    }
+
+  unloadshr = (int (*)(int))dlsym (handle, "shrfunc1");
+
+  if (!unloadshr)
+    {
+      fprintf (stderr, dlerror ());
+      exit (1);
+    }
+
+  y = (*unloadshr)(3);
+
+  printf ("y is %d\n", y);
+
+  dlclose (handle);
+
+  return 0;
+}
Index: gdb.base/unload.exp
===================================================================
RCS file: gdb.base/unload.exp
diff -N gdb.base/unload.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb.base/unload.exp	12 Aug 2004 17:46:15 -0000
@@ -0,0 +1,142 @@
+#   Copyright 2003, 2004
+#   Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+# 
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+# 
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  
+
+# This file was created by Jeff Johnston. (jjohnstn@redhat.com)
+# The shared library compilation portion was copied from shlib-call.exp which was
+# written by Elena Zannoni (ezannoni@redhat.com).
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+#
+# test running programs
+#
+set prms_id 0
+set bug_id 0
+
+# are we on a target board?
+if ![isnative] then {
+    return 0
+}
+
+set testfile "unload"
+set libfile "unloadshr"
+set libsrcfile ${libfile}.c
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+set shlibdir ${objdir}/${subdir}
+
+if [get_compiler_info ${binfile}] {
+    return -1
+}
+
+set dl_lib_flag ""
+switch -glob [istarget] {
+    "hppa*-hp-hpux*" { }
+    "*-*-linux*"     { set dl_lib_flag "libs=-ldl" }
+    default          { }
+}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug "additional_flags=-DSHLIB_DIR\=\"${shlibdir}\"" $dl_lib_flag]] != "" } {
+     gdb_suppress_entire_file "Testcase compile failed, so all tests in this file will automatically fail."
+}
+
+# Build the shared libraries this test case needs.
+#
+
+if {$gcc_compiled == 0} {
+    if [istarget "hppa*-hp-hpux*"] then {
+	set additional_flags "additional_flags=+z"
+    } elseif { [istarget "mips-sgi-irix*"] } {
+	# Disable SGI compiler's implicit -Dsgi
+	set additional_flags "additional_flags=-Usgi"
+    } else {
+	# don't know what the compiler is...
+	set additional_flags ""
+    }
+} else {
+    if { ([istarget "powerpc*-*-aix*"]
+       || [istarget "rs6000*-*-aix*"]) } {
+	set additional_flags ""
+    } else {
+	set additional_flags "additional_flags=-fpic"
+    }
+}
+
+if {[gdb_compile "${srcdir}/${subdir}/${libsrcfile}" "${objdir}/${subdir}/${libfile}.o" object [list debug $additional_flags]] != ""} {
+     gdb_suppress_entire_file "Testcase compile failed, so all tests in this file will automatically fail."
+}
+
+if [istarget "hppa*-*-hpux*"] {
+    remote_exec build "ld -b ${objdir}/${subdir}/${libfile}.o -o ${objdir}/${subdir}/${libfile}.sl"
+} else {
+    set additional_flags "additional_flags=-shared"
+    if {[gdb_compile "${objdir}/${subdir}/${libfile}.o" "${objdir}/${subdir}/${libfile}.sl" executable [list debug $additional_flags]] != ""} {
+	gdb_suppress_entire_file "Testcase compile failed, so all tests in this file will automatically fail."
+    }
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if [target_info exists gdb_stub] {
+    gdb_step_for_stub;
+}
+
+#
+# Test setting a breakpoint in a dynamically loaded library which is
+# manually loaded and unloaded
+#
+
+gdb_test_multiple "break shrfunc1" "set pending breakpoint" {
+     -re ".*Make breakpoint pending.*y or \\\[n\\\]. $" {
+	    gdb_test "y" "Breakpoint.*shrfunc1.*pending." "set pending breakpoint"
+     }
+}
+
+gdb_test "info break" \
+    "Num Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+breakpoint     keep y.*PENDING.*shrfunc1.*" \
+"single pending breakpoint info"
+
+set unloadshr_line [gdb_get_line_number "unloadshr break" ${srcdir}/${subdir}/${libsrcfile}]
+
+gdb_test "run" \
+"Starting program.*unload.*
+Breakpoint.*at.*
+Pending breakpoint \"shrfunc1\" resolved.*
+Breakpoint.*, shrfunc1 \\\(x=3\\\).*unloadshr.c:$unloadshr_line.*" \
+"running program"
+
+gdb_test "continue" \
+"Continuing.*y is 7.*warning: Temporarily disabling unloaded shared library breakpoints.*warning: breakpoint #.*Program exited normally." \
+"continuing to end of program"
+
+#
+# Try to rerun program and verify that shared breakpoint is reset properly
+#
+
+gdb_test "run" \
+".*Breakpoint.*shrfunc1.*at.*unloadshr.c:$unloadshr_line.*" \
+"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"
Index: gdb.base/unloadshr.c
===================================================================
RCS file: gdb.base/unloadshr.c
diff -N gdb.base/unloadshr.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb.base/unloadshr.c	12 Aug 2004 17:46:15 -0000
@@ -0,0 +1,24 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2004 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+ 
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software
+   Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. */
+
+#include <stdio.h>
+
+int shrfunc1 (int x)
+{
+  return x + 4; /* unloadshr break */
+}

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

[Eli Zaretskii]

> Date: Thu, 12 Aug 2004 08:10:28 -0400
> From: Andrew Cagney <cagney@gnu.org>
> > 
> > But we don't want to _monitor_ anything in this case.  We want to fox
> > a specific, well-defined problem due to a specific course of events.
> 
> And that well defined course of events is called an shlib unload.  Lets 
> abstract it as such.

Sorry, this is too terse for me to follow.  I'd appreciate if you
could elaborate a bit.

TIA

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

[Michael Chastain]

Works for me now on hpux 11.11, with both hp ansi c and gcc.

This patch is approved.  Party on!

Michael C

===

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

         * gdb.base/unload.exp: New test for breakpoints in dynamically
         loaded libraries.
         * gdb.base/unload.c: Ditto.
         * gdb.base/unloadshr.c: Ditto.

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

[Jeff Johnston]

Testsuite portion of patch checked in.  Thanks.

-- Jeff J.

Michael Chastain wrote:
> Works for me now on hpux 11.11, with both hp ansi c and gcc.
> 
> This patch is approved.  Party on!
> 
> Michael C
> 
> ===
> 
> 2004-08-10  Jeff Johnston  <jjohnstn@redhat.com>
> 
>          * gdb.base/unload.exp: New test for breakpoints in dynamically
>          loaded libraries.
>          * gdb.base/unload.c: Ditto.
>          * gdb.base/unloadshr.c: Ditto.
> 
> 

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

[Andrew Cagney]

>>Date: Thu, 12 Aug 2004 08:10:28 -0400
>>> From: Andrew Cagney <cagney@gnu.org>
>>
>>>> > 
>>>> > But we don't want to _monitor_ anything in this case.  We want to fox
>>>> > a specific, well-defined problem due to a specific course of events.
>>
>>> 
>>> And that well defined course of events is called an shlib unload.  Lets 
>>> abstract it as such.
> 
> 
> Sorry, this is too terse for me to follow.  I'd appreciate if you
> could elaborate a bit.

Here's a reference http://patterndigest.com/patterns/Observer.html
For our case the RealSubject is an shlib-unload.  solib.c doesn't need 
to know who is interested in this, just that there is something. 
Conversely, the breakpoint code, doesn't care about the course of events 
that lead to an shlib-unload, just that it occured.

Andrew

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

[Eli Zaretskii]

> Date: Thu, 12 Aug 2004 16:43:36 -0400
> From: Andrew Cagney <cagney@gnu.org>
> 
> Here's a reference http://patterndigest.com/patterns/Observer.html

I'm well aware of the observer pattern and its general usefulness.

What I'm not sure about is whether this specific case justifies an
introduction of a _new_ observer, when it could easily (or so it seems
to me) be fixed in another, more traditional, way.  Sorry if that
concern was unclear from my original wording.

> Conversely, the breakpoint code, doesn't care about the course of events 
> that lead to an shlib-unload, just that it occured.

Our breakpoint code is replete with things it cares about that happen
in other parts of the code.  I don't understand why this minor problem
justifies to be solved in such a different, non-minor way.

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

[Daniel Jacobowitz]

On Sat, Aug 14, 2004 at 02:47:13PM +0300, Eli Zaretskii wrote:
> > Date: Thu, 12 Aug 2004 16:43:36 -0400
> > From: Andrew Cagney <cagney@gnu.org>
> > 
> > Here's a reference http://patterndigest.com/patterns/Observer.html
> 
> I'm well aware of the observer pattern and its general usefulness.
> 
> What I'm not sure about is whether this specific case justifies an
> introduction of a _new_ observer, when it could easily (or so it seems
> to me) be fixed in another, more traditional, way.  Sorry if that
> concern was unclear from my original wording.
> 
> > Conversely, the breakpoint code, doesn't care about the course of events 
> > that lead to an shlib-unload, just that it occured.
> 
> Our breakpoint code is replete with things it cares about that happen
> in other parts of the code.  I don't understand why this minor problem
> justifies to be solved in such a different, non-minor way.

I think that using a new observer here improves clarity - for instance
it lets us easily identify all the points in the code where the
breakpoint module has hooks.  And, the observer seems generally useful;
breakpoint.c isn't the only thing that will be affected when a shared
library vanishes.

Does that work for you?

-- 
Daniel Jacobowitz

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

[Daniel Jacobowitz]

On Wed, Aug 11, 2004 at 04:12:07PM -0400, Jeff Johnston wrote:
> Ok?

Sorry to keep picking nits; while we discuss the issue of the new
observer I went over the patch again for minor problems.

> +/* 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;
> +
> +  /* See also: insert_breakpoints, under DISABLE_UNSETTABLE_BREAK. */

Two spaces after a period, please.

> +  ALL_BREAKPOINTS (b)
> +  {
> +#if defined (PC_SOLIB)

I think someone pointed out that you've #ifdef'd out the entire body of
this loop.  Might as well include the whole loop.  The #ifdef is nasty,
but that's a preexisting problem.

> +    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);

While I'm ranting about preexisting problems, it would be nice if
PC_SOLIB returned the solib, instead of just its name... but enh.

> +	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.

-- 
Daniel Jacobowitz

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

[Jeff Johnston]

Daniel Jacobowitz wrote:
> On Wed, Aug 11, 2004 at 04:12:07PM -0400, Jeff Johnston wrote:
> 
>>Ok?
> 
> 
> Sorry to keep picking nits; while we discuss the issue of the new
> observer I went over the patch again for minor problems.
> 
> 
>>+/* 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;
>>+
>>+  /* See also: insert_breakpoints, under DISABLE_UNSETTABLE_BREAK. */
> 
> 
> Two spaces after a period, please.
> 
> 
>>+  ALL_BREAKPOINTS (b)
>>+  {
>>+#if defined (PC_SOLIB)
> 
> 
> I think someone pointed out that you've #ifdef'd out the entire body of
> this loop.  Might as well include the whole loop.  The #ifdef is nasty,
> but that's a preexisting problem.
> 
> 
>>+    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);
> 
> 
> While I'm ranting about preexisting problems, it would be nice if
> PC_SOLIB returned the solib, instead of just its name... but enh.
> 
> 
>>+	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.

-- Jeff J.

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

[Daniel Jacobowitz]

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);

-- 
Daniel Jacobowitz

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

[Jeff Johnston]

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.

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

[Eli Zaretskii]

> Date: Wed, 18 Aug 2004 09:45:38 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> I think that using a new observer here improves clarity - for instance
> it lets us easily identify all the points in the code where the
> breakpoint module has hooks.  And, the observer seems generally useful;
> breakpoint.c isn't the only thing that will be affected when a shared
> library vanishes.

I'd like to see at least a couple more cases like this before I agree
that a generalization is in order.

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

[Eli Zaretskii]

> Date: Wed, 18 Aug 2004 16:03:56 -0400
> From: Jeff Johnston <jjohnstn@redhat.com>
> 
> 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?

I'm still unconvinced, but I won't veto it.

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

[Jeff Johnston]

[-- 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"
+

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

[Michael Chastain]

The test script is not quite okay.

The test script should match the output from the last released gdb,
gdb 6.2, as well as the current gdb.

When I compare gdb 6.2 with gdb HEAD, or with gdb 6.2.91 (when it
comes out), I use the same current test suite with both the old
and new gdb's.  I can't compare:

  (gdb 6.2, suite 6.2)
  (gdb 6.2.91, suite 6.2.91)
  
... because there are thousands of new tests and test name changes.
So I have to compare:

  (gdb 6.2, suite 6.2.91)
  (gdb 6.2.91, suite 6.2.91)

So it helps me if the test suite continues to match
the messages from the last released gdb.

If you're curious, you can see some of these comparison tables here:

  http://www.shout.net/~mec/sunday/2004-08-18/Compare-by-gdb-branch-HEAD.html

===

You can do this either by fuzzing the pattern up with some wild cards,
or with the "(...|...)" construct, or by using a gdb_test_multiple
with one arm for the old output and one arm for the new.
Personally I would go for gdb_test_multiple:

  set name "continuing to end of program"
  gdb_test_multiple "continue" $name {
    -re "Continuing.*y is 7.*warning: ... disabling unloaded shared library breakpoints ....*$gdb_prompt $" {
      # old gdb 6.2
      pass $name
    }
    -re "Continuing.*y is 7.*warning: ... disabling breakpoints for.*unloadshr.sl.* ... $gdb_prompt $" {
      # new gdb HEAD 2004-08-23
      pass $name
    }
  }

Also, what system did you test on?

===

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

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

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

[Jeff Johnston]

Michael Chastain wrote:
> The test script is not quite okay.
> 
> The test script should match the output from the last released gdb,
> gdb 6.2, as well as the current gdb.
> 

The test should behave the same because the gdb code that generates the message 
it is checking hasn't been committed yet.  On retrospect, perhaps I should not 
have committed the testcase in with this particular check in place (the test 
itself is a valid one regardless).  There has been some questions regarding 
whether I should be using an observer or not.  Knowing that, do I still need to 
do the following or can I check the change I attached in once I get final 
approval on the code and most importantly, the message to be issued?

> When I compare gdb 6.2 with gdb HEAD, or with gdb 6.2.91 (when it
> comes out), I use the same current test suite with both the old
> and new gdb's.  I can't compare:
> 
>   (gdb 6.2, suite 6.2)
>   (gdb 6.2.91, suite 6.2.91)
>   
> ... because there are thousands of new tests and test name changes.
> So I have to compare:
> 
>   (gdb 6.2, suite 6.2.91)
>   (gdb 6.2.91, suite 6.2.91)
> 
> So it helps me if the test suite continues to match
> the messages from the last released gdb.
> 
> If you're curious, you can see some of these comparison tables here:
> 
>   http://www.shout.net/~mec/sunday/2004-08-18/Compare-by-gdb-branch-HEAD.html
> 
> ===
> 
> You can do this either by fuzzing the pattern up with some wild cards,
> or with the "(...|...)" construct, or by using a gdb_test_multiple
> with one arm for the old output and one arm for the new.
> Personally I would go for gdb_test_multiple:
> 
>   set name "continuing to end of program"
>   gdb_test_multiple "continue" $name {
>     -re "Continuing.*y is 7.*warning: ... disabling unloaded shared library breakpoints ....*$gdb_prompt $" {
>       # old gdb 6.2
>       pass $name
>     }
>     -re "Continuing.*y is 7.*warning: ... disabling breakpoints for.*unloadshr.sl.* ... $gdb_prompt $" {
>       # new gdb HEAD 2004-08-23
>       pass $name
>     }
>   }
> 
> Also, what system did you test on?
> 
> ===
> 
> 2004-08-23  Jeff Johnston  <jjohnstn@redhat.com>
> 
> 	* gdb.base/unload.exp: Fix expected warning message to match
> 	latest format.
> 
> 

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

[Michael Chastain]

Jeff Johnston <jjohnstn@redhat.com> wrote:
> The test should behave the same because the gdb code that generates
> the message it is checking hasn't been committed yet.  On retrospect,
> perhaps I should not have committed the testcase in with this
> particular check in place (the test itself is a valid one regardless).
> There has been some questions regarding whether I should be using an
> observer or not.  Knowing that, do I still need to do the following or
> can I check the change I attached in once I get final approval on the
> code and most importantly, the message to be issued?

I don't quite follow you, but if you are asking: can you check in
a change to the test script before you check in a change to gdb:
in general, you can do that.

If the test script accepts both old+new messages, and the new message is
not wildly more complex than the old message, then testing with the old
message alone is good enough for getting the test script approved.
Just pop out the new patch and say how / what system you tested on.

If that's not what you mean, I'm confused.

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

[Jeff Johnston]

Michael Chastain wrote:
> Jeff Johnston <jjohnstn@redhat.com> wrote:
> 
>>The test should behave the same because the gdb code that generates
>>the message it is checking hasn't been committed yet.  On retrospect,
>>perhaps I should not have committed the testcase in with this
>>particular check in place (the test itself is a valid one regardless).
>>There has been some questions regarding whether I should be using an
>>observer or not.  Knowing that, do I still need to do the following or
>>can I check the change I attached in once I get final approval on the
>>code and most importantly, the message to be issued?
> 
> 
> I don't quite follow you, but if you are asking: can you check in
> a change to the test script before you check in a change to gdb:
> in general, you can do that.
> 
> If the test script accepts both old+new messages, and the new message is
> not wildly more complex than the old message, then testing with the old
> message alone is good enough for getting the test script approved.
> Just pop out the new patch and say how / what system you tested on.
> 
> If that's not what you mean, I'm confused.
>

The test fails now because gdb is not issuing the message that is currently in 
the test script.  The message in the test script is a new message which does not 
currently exist and never did in any gdb release.  When I get approval to check 
the gdb code in "and" change the test script, the test will succeed because it 
will match the actual message checked in.  I don't see a version issue of old 
gdb looked for x and new gdb looks for y.  Old gdb will always fail and new gdb 
will always work.  Thus, I was wondering if I still need to have checks for old 
and new messages in the script.

-- Jeff J.

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

[Michael Chastain]

Jeff Johnston <jjohnstn@redhat.com> wrote:
> The test fails now because gdb is not issuing the message that is
> currently in the test script.  The message in the test script is a new
> message which does not currently exist and never did in any gdb
> release.

Okay, that unconfuses me.

> Thus, I was wondering if I still need to have checks for old 
> and new messages in the script.

If gdb 6.2 didn't print a message, then you don't need to have it in the
script.  What I care about is comparing gdb 6.2 and gdb 6.2.91 with the
test suite that's going to be in gdb 6.2.91.

On the last spin I did with gdb 6.2 and the current version
of unload.exp (from gdb HEAD 2004-08-18), I got this:

  (gdb) PASS: gdb.base/unload.exp: running program
  continue
  Continuing.
  y is 7
  
  Program exited normally.
  (gdb) FAIL: gdb.base/unload.exp: continuing to end of program
  run
  Starting program: /tmp/migbat-testgdb-8amaeH24/test/gdb.base/unload 
  Warning:
  Cannot insert breakpoint 2.
  Error accessing memory address 0x400136cb: Input/output error.
  
  (gdb) FAIL: gdb.base/unload.exp: rerun to shared library breakpoint

Which is cool.  The test script is doing its job, making gdb barf.
That will be a genuine "before" FAIL in the tables, not a fake "before".

Maybe someday, omeone can even mine this data source and add more
"bugs fixed in this release" sections to NEWS.

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

[Andrew Cagney]

>>> Date: Wed, 18 Aug 2004 16:03:56 -0400
>>> From: Jeff Johnston <jjohnstn@redhat.com>
>>> 
>>> 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?
> 
> 
> I'm still unconvinced, but I won't veto it.

Jeff, the observer and use can be committed.

Eli, I'm not sure what you're looking for here:
> I'd like to see at least a couple more cases like this before I agree
> that a generalization is in order.
can you expand a little?

Andrew

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

[Jeff Johnston]

Andrew Cagney wrote:
>>>> Date: Wed, 18 Aug 2004 16:03:56 -0400
>>>> From: Jeff Johnston <jjohnstn@redhat.com>
>>>>
>>>> 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?
>>
>>
>>
>> I'm still unconvinced, but I won't veto it.
> 
> 
> Jeff, the observer and use can be committed.
>

Thanks.  Patches committed, including fix to the testcase.

> Eli, I'm not sure what you're looking for here:
> 
>> I'd like to see at least a couple more cases like this before I agree
>> that a generalization is in order.
> 
> can you expand a little?
> 
> Andrew
> 
> 
> 

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

[Michael Chastain]

Jeff Johnston <jjohnstn@redhat.com> wrote:
> Thanks.  Patches committed, including fix to the testcase.

Uh, I haven't approved that fix.

I wrote:

  If the test script accepts both old+new messages, and the new message is
  not wildly more complex than the old message, then testing with the old
  message alone is good enough for getting the test script approved.
  Just pop out the new patch and say how / what system you tested on.
   
You needed to send a fresh patch to gdb-patches, say how you tested it,
and then get it approved.  I'm sorry if this wasn't clear from my message.
Sometimes I do write too colloquially.

This is actually a live issue because the patch you committed has
a problem:

  -"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"

Now there are two tests with the identical name "continuing to end
of program", which leads to confusion.

Can you please: fix that; say which system you tested it on;
and submit a patch to gdb-patches?

Michael

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

[Jeff Johnston]

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

Michael Chastain wrote:
> Jeff Johnston <jjohnstn@redhat.com> wrote:
> 
>>Thanks.  Patches committed, including fix to the testcase.
> 
> 
> Uh, I haven't approved that fix.
> 
> I wrote:
> 
>   If the test script accepts both old+new messages, and the new message is
>   not wildly more complex than the old message, then testing with the old
>   message alone is good enough for getting the test script approved.
>   Just pop out the new patch and say how / what system you tested on.
>    
> You needed to send a fresh patch to gdb-patches, say how you tested it,
> and then get it approved.  I'm sorry if this wasn't clear from my message.
> Sometimes I do write too colloquially.
> 
> This is actually a live issue because the patch you committed has
> a problem:
> 
>   -"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"
> 
> Now there are two tests with the identical name "continuing to end
> of program", which leads to confusion.
> 
> Can you please: fix that; say which system you tested it on;
> and submit a patch to gdb-patches?
>

Oops, my bad.  Sorry about that.

Tested on x86-linux, ia64-linux, x86_64-linux, s390-linux.

Patch attached to fix problem above.

2004-09-01  Jeff Johnston  <jjohnstn@redhat.com>

         * gdb.base/unload.exp: Fix so messages aren't duplicated.


> Michael
> 

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

Index: gdb.base/unload.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/unload.exp,v
retrieving revision 1.2
diff -u -p -r1.2 unload.exp
--- gdb.base/unload.exp	1 Sep 2004 17:56:20 -0000	1.2
+++ gdb.base/unload.exp	1 Sep 2004 20:40:57 -0000
@@ -139,5 +139,5 @@ gdb_test "run" \
 
 gdb_test "continue" \
 "Continuing.*y is 7.*warning: Temporarily disabling breakpoints for.*unloadshr.sl.*Program exited normally." \
-"continuing to end of program"
+"continuing to end of program second time"
 

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

[Michael Chastain]

Approved.  Thanks.

Bugs happen.  The test suite is a headache; all the code is interpreted
and there's four layers of tools and libraries (tcl + expect + dejagnu
+ lib/gdb.exp); and there's no style guide.

> 2004-09-01  Jeff Johnston  <jjohnstn@redhat.com>
>
>          * gdb.base/unload.exp: Fix so messages aren't duplicated.

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

[Jeff Johnston]

Michael Chastain wrote:
> Approved.  Thanks.
>

Thanks.  Patch applied.

> Bugs happen.  The test suite is a headache; all the code is interpreted
> and there's four layers of tools and libraries (tcl + expect + dejagnu
> + lib/gdb.exp); and there's no style guide.
> 
> 
>>2004-09-01  Jeff Johnston  <jjohnstn@redhat.com>
>>
>>         * gdb.base/unload.exp: Fix so messages aren't duplicated.
> 
> 

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

[Eli Zaretskii]

> Date: Wed, 01 Sep 2004 11:13:38 -0400
> From: Andrew Cagney <cagney@gnu.org>
> Cc: Jeff Johnston <jjohnstn@redhat.com>, drow@false.org,
>         gdb-patches@sources.redhat.com
> 
> Eli, I'm not sure what you're looking for here:
> > I'd like to see at least a couple more cases like this before I agree
> > that a generalization is in order.
> can you expand a little?

I meant to say that I prefer not to make generalizations based on a
single case.

Anyway, the patch is committed now, so I don't see any reason to
continue arguing about it.