[RFC] Watchpoint on an unloaded shared library(3)

[Emi SUZUKI <emi-suzuki at tjsys dot co dot jp>]

Hello members, 

This is the third issue (but would not be the last, unfortunately) I
mentioned at the mail:

  http://sourceware.org/ml/gdb-patches/2008-11/msg00538.html

Note that I use the same program code shown in the above mail to
reproduce it.  

---- 
Issue 3:

$ ./x86-gdb dl-test
GNU gdb (GDB) 6.8.50.20090106-cvs
Copyright (C) 2009 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-pc-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
(gdb) set can-use-hw-watchpoints 0
(gdb) start
Temporary breakpoint 1 at 0x80484f5: file dl-test.c, line 11.
Starting program: /home/suzuki/test/dl-test

Temporary breakpoint 1, main () at dl-test.c:11
11        void *handle = NULL;
(gdb) next 2
16        if ((sample = dlsym(handle, "sample")) == NULL)
(gdb) watch sample_glob
Watchpoint 2: sample_glob
(gdb) continue
Continuing.
sample of shared library
Watchpoint 2: sample_glob

Old value = 1
New value = 2
sample () at sample.c:10
10      }
(gdb) continue
Continuing.
Watchpoint 2: sample_glob

Old value = 2
New value = <unreadable>
0x00be0821 in munmap () from /lib/ld-linux.so.2
(gdb) continue
Continuing.
Segmentation fault

$ 
-----------

The segfault would occur here:

static int
watchpoint_check (void *p)
{
  bpstat bs = (bpstat) p;
  struct breakpoint *b;
  struct frame_info *fr;
  int within_current_scope;

  b = bs->breakpoint_at->owner;

  if (b->exp_valid_block == NULL)
    within_current_scope = 1;
  else
    {
      ....
    }

  if (within_current_scope)
    {
      /* We use value_{,free_to_}mark because it could be a
         *long* time before we return to the command level and
         call free_all_values.  We can't call free_all_values because
         we might be in the middle of evaluating a function call.  */

      struct value *mark = value_mark ();
      struct value *new_val;

      fetch_watchpoint_value (b->exp, &new_val, NULL, NULL);

b->exp for software watchpoints would be updated when a new shared
library is loaded and the user enables disabled ones, but not when a
shared library is unloaded.  So fetch_watchpoint_value (b->exp,....)
would refer to symtabs which is already freed after the shared
library is unloaded.  

For breakpoints, we use disable_breakpoints_in_unloaded_shlib to check
if the breakpoints refer to the unloaded library.  And we should call
update_watchpoint for each watchpoints to check if the expression
they refer to is to be removed.  But actually calling update_watchpoint 
in disable_breakpoints_in_unloaded_shlib does not work, because we
remove the symtabs of the unloaded library after we notify that the
shared library is unloaded: 

update_solib_list ()
{
    .....

      /* If it's not on the inferior's list, remove it from GDB's tables.  */
      else
	{
	  /* Notify any observer that the shared object has been
	     unloaded before we remove it from GDB's tables.  */
	  observer_notify_solib_unloaded (gdb);

	  *gdb_link = gdb->next;

	  /* Unless the user loaded it explicitly, free SO's objfile.  */
	  if (gdb->objfile && ! (gdb->objfile->flags & OBJF_USERLOADED))
	    free_objfile (gdb->objfile);

However, when the symtabs for the shared library is automatically
loaded, we notify the observers of load of a shared library *before*
loading its symtabs (Note that observer_notify_solib_load is called in
update_solib_list): 

solib_add ()
{
  ....

  update_solib_list (from_tty, target);

  /* Walk the list of currently loaded shared libraries, and read
     symbols for any that match the pattern --- or any whose symbols
     aren't already loaded, if no pattern was given.  */
  {
    int any_matches = 0;
    int loaded_any_symbols = 0;

    for (gdb = so_list_head; gdb; gdb = gdb->next)
      if (! pattern || re_exec (gdb->so_name))
	{
          /* Normally, we would read the symbols from that library
             only if READSYMS is set.  However, we're making a small
             exception for the pthread library, because we sometimes
             need the library symbols to be loaded in order to provide
             thread support (x86-linux for instance).  */
          const int add_this_solib =
            (readsyms || libpthread_solib_p (gdb));

	  any_matches = 1;
	  if (add_this_solib && solib_read_symbols (gdb, from_tty))
	    loaded_any_symbols = 1;
	}

So I think it is more consistent to remove auto-loaded symbols for an
unloaded shared library before notifying the unload.  I also confirmed
that the other observers for solib_unloaded do not expect that symtabs
for that library still exist.  

I made a patch under the idea above.  If you apply it, the output
around the issue would change like the below.  
----- 
(gdb) c
Continuing.
Watchpoint 2: sample_glob

Old value = 2
New value = <unreadable>
0x00be0821 in munmap () from /lib/ld-linux.so.2
(gdb) c
Continuing.
warning: Disabling watchpoints for unloaded shared library "./libsample.so"

Program exited normally.
(gdb) info breakpoints
Num     Type           Disp Enb Address    What
2       watchpoint     keep n              sample_glob
        breakpoint already hit 2 times
(gdb) 
------

Do you think it makes sense?


2008-01-13  Emi Suzuki  <emi-suzuki@tjsys.co.jp>

	* solib.c (update_solib_list): Call solib_unloaded observers
	after removing symtabs of an unloaded shared library.  
	* breakpoint.c (disable_breakpoints_in_unloaded_shlib): Call
	update_watchpoint for removing the reference to symbols of an
	unloaded shared library.  

diff -ruNp gdb.orig/gdb/breakpoint.c gdb/gdb/breakpoint.c
--- gdb.orig/gdb/breakpoint.c	2009-01-13 16:47:42.000000000 +0900
+++ gdb/gdb/breakpoint.c	2009-01-13 17:11:31.000000000 +0900
@@ -4457,10 +4457,10 @@ disable_breakpoints_in_unloaded_shlib (s
 {
   struct bp_location *loc;
   int disabled_shlib_breaks = 0;
+  struct breakpoint *b;
 
   ALL_BP_LOCATIONS (loc)
   {
-    struct breakpoint *b = loc->owner;
     if ((loc->loc_type == bp_loc_hardware_breakpoint
 	 || loc->loc_type == bp_loc_software_breakpoint)
 	&& !loc->shlib_disabled)
@@ -4482,8 +4482,35 @@ disable_breakpoints_in_unloaded_shlib (s
 		target_terminal_ours_for_output ();
 		warning (_("Temporarily disabling breakpoints for unloaded shared library \"%s\""),
 			  so_name);
+		disabled_shlib_breaks = 1;
+	      }
+	  }
+      }
+  }
+
+  disabled_shlib_breaks = 0;
+  ALL_BREAKPOINTS (b)
+  {
+    struct gdb_exception e;
+
+    if ((is_hardware_watchpoint (b) || b->type == bp_watchpoint)
+	&& b->enable_state == bp_enabled)
+      {
+	TRY_CATCH (e, RETURN_MASK_ALL)
+	  {
+	    update_watchpoint (b, 1 /* reparse */);
+	  }
+	if (e.reason < 0)
+	  {
+	    b->enable_state = bp_disabled;
+
+	    if (!disabled_shlib_breaks)
+	      {
+		target_terminal_ours_for_output ();
+		warning (_("Disabling watchpoints for unloaded shared library \"%s\""),
+			 solib->so_name);
+		disabled_shlib_breaks = 1;
 	      }
-	    disabled_shlib_breaks = 1;
 	  }
       }
   }
diff -ruNp gdb.orig/gdb/solib.c gdb/gdb/solib.c
--- gdb.orig/gdb/solib.c	2009-01-13 16:47:42.000000000 +0900
+++ gdb/gdb/solib.c	2009-01-13 17:11:31.000000000 +0900
@@ -587,12 +587,6 @@ 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 shared object has been
-	     unloaded before we remove it from GDB's tables.  */
-	  observer_notify_solib_unloaded (gdb);
-
-	  *gdb_link = gdb->next;
-
 	  /* Unless the user loaded it explicitly, free SO's objfile.  */
 	  if (gdb->objfile && ! (gdb->objfile->flags & OBJF_USERLOADED))
 	    free_objfile (gdb->objfile);
@@ -601,6 +595,12 @@ update_solib_list (int from_tty, struct 
 	     sections from so->abfd; remove them.  */
 	  remove_target_sections (gdb->abfd);
 
+	  /* Notify any observer that the shared object has been
+	     unloaded before we remove it from GDB's tables.  */
+	  observer_notify_solib_unloaded (gdb);
+
+	  *gdb_link = gdb->next;
+
 	  free_so (gdb);
 	  gdb = *gdb_link;
 	}

My best regards, 
-- 
Emi SUZUKI / emi-suzuki at tjsys.co.jp