Re: [RFA]: Watchpoints per thread patch

[Jeff Johnston <jjohnstn@redhat.com>]

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

Daniel Jacobowitz wrote:
> On Wed, Oct 20, 2004 at 01:27:15PM -0400, Andrew Cagney wrote:
> 
>>the underlying target (ia64-linux-nat, ...) can locally override the 
>>method and handle the problem.  The code's the same, but how it is wired 
>>up is different
>>
>>Sound reasonable to all?
> 
> 
> I think that sounds pretty good.  Hopefully the changes involved will
> be pretty small, since I imagine that most GNU/Linux targets with
> hardware watchpoints will want it.
> 

The attached patch is the rework of my original attempt.  It no longer uses 
configuration or magic defines.  Per Mark's suggestion, it uses an observer to 
handle inserting watchpoints on a new thread and only the low-level code knows 
about inserting/removing watchpoints on all threads.

Ok to commit?

-- Jeff J.

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

         * breakpoint.c (insert_watchpoints_for_new_thread): New function.
         * breakpoint.h (insert_watchpoints_for_new_thread): New prototype.
         * ia64-linux-nat.c (ia64_linux_insert_one_watchpoint): New function.
         (ia64_linux_insert_watchpoint_callback): Ditto.
         (ia64_linux_insert_watchpoint): Change to iterate through lwps
         and insert the specified watchpoint per thread.
         (ia64_linux_remove_one_watchpoint): New function.
         (ia64_linux_remove_watchpoint_callback): Ditto.
         (ia64_linux_remove_watchpoint): Change to iterate through lwps and
         remove the specified watchpoint for each thread.
         (ia64_linux_new_thread): New thread observer.
         (_initialize_ia64_linux_nat): New function.  Initialize
         new thread observer.
         * thread-db.c (attach_thread): Notify any observers of the new
         thread event.
         * s390-nat.c (s390_tid): New function.
         (s390_inferior_tid): Change to call s390_tid.
         (s390_remove_one_watchpoint): New function.
         (s390_remove_watchpoint_callback): Ditto.
         (s390_remove_watchpoint): Change to iterate through lwps and
         remove the specified watchpoint for each thread.
         (s390_insert_one_watchpoint): New function.
         (s390_insert_watchpoint_callback): Ditto.
         (s390_insert_watchpoint): Change to iterate through lwps and
         insert the specified watchpoint on each thread.
         (s390_new_thread): New thread observer.
         (_initialize_s390_nat): New function.  Initialize
         new thread observer.

doc/ChangeLog:

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

         * observer.texi (new_thread): New observer.


[-- Attachment #2: watchthreads3.patch --]
[-- Type: text/plain, Size: 14516 bytes --]

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.183
diff -u -p -r1.183 breakpoint.c
--- breakpoint.c	8 Oct 2004 17:30:46 -0000	1.183
+++ breakpoint.c	27 Oct 2004 21:43:50 -0000
@@ -748,6 +748,91 @@ insert_catchpoint (struct ui_out *uo, vo
   return 0;
 }
 
+/* External function to insert all existing watchpoints on a newly
+   attached thread.  IWPFN is a callback function to perform
+   the target insert watchpoint.  This function is used to support
+   platforms whereby a watchpoint must be inserted/removed on each
+   individual thread (e.g. ia64-linux and s390-linux).  For
+   ia64 and s390 linux, this function is called via a new thread
+   observer.  */
+int
+insert_watchpoints_for_new_thread (ptid_t new_thread, 
+				   insert_watchpoint_ftype *iwpfn)
+{
+  struct bp_location *b;
+  int val = 0;
+  int return_val = 0;
+  struct ui_file *tmp_error_stream = mem_fileopen ();
+  make_cleanup_ui_file_delete (tmp_error_stream);
+
+  /* Explicitly mark the warning -- this will only be printed if
+     there was an error.  */
+  fprintf_unfiltered (tmp_error_stream, "Warning:\n");
+
+  ALL_BP_LOCATIONS (b)
+    {
+      /* Skip disabled breakpoints.  */
+      if (!breakpoint_enabled (b->owner))
+	continue;
+
+      /* For every active watchpoint, we need to insert the watchpoint on 
+         the new thread.  */
+      if ((b->loc_type == bp_loc_hardware_watchpoint
+	   || b->owner->type == bp_watchpoint))
+	{
+	  struct value *v = b->owner->val_chain;
+
+	  /* Look at each value on the value chain.  */
+	  for (; v; v = v->next)
+	    {
+	      /* If it's a memory location, and GDB actually needed
+		 its contents to evaluate the expression, then we
+		 must watch it.  */
+	      if (VALUE_LVAL (v) == lval_memory
+		  && ! VALUE_LAZY (v))
+		{
+		  struct type *vtype = check_typedef (VALUE_TYPE (v));
+		  
+		  /* We only watch structs and arrays if user asked
+		     for it explicitly, never if they just happen to
+		     appear in the middle of some value chain.  */
+		  if (v == b->owner->val_chain
+		      || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
+			  && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
+		    {
+		      CORE_ADDR addr;
+		      int len, type;
+		      
+		      addr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
+		      len = TYPE_LENGTH (VALUE_TYPE (v));
+		      type = hw_write;
+		      if (b->owner->type == bp_read_watchpoint)
+			type = hw_read;
+		      else if (b->owner->type == bp_access_watchpoint)
+			type = hw_access;
+		      val = (*iwpfn) (new_thread, addr, len, type); 
+		    }
+		}
+	    }
+	}
+
+      if (val)
+	return_val = val;
+    }
+
+  /* Failure to insert a watchpoint on any memory value in the
+     value chain brings us here.  */
+  if (return_val)
+    {
+      fprintf_unfiltered (tmp_error_stream,
+			  "%s\n",
+			  "Could not insert hardware watchpoints on new thread."); 
+      target_terminal_ours_for_output ();
+      error_stream (tmp_error_stream);
+    }
+  return return_val;
+}
+
 /* Helper routine: free the value chain for a breakpoint (watchpoint).  */
 
 static void free_valchain (struct bp_location *b)
Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.34
diff -u -p -r1.34 breakpoint.h
--- breakpoint.h	13 May 2004 16:39:11 -0000	1.34
+++ breakpoint.h	27 Oct 2004 21:43:50 -0000
@@ -663,6 +663,12 @@ extern void tbreak_command (char *, int)
 
 extern int insert_breakpoints (void);
 
+/* The following provides a callback mechanism to insert watchpoints
+   for a new thread.  This is needed, for example, on ia64 linux.  */
+typedef int (insert_watchpoint_ftype) (ptid_t, CORE_ADDR, int, int);
+extern int insert_watchpoints_for_new_thread (ptid_t ptid,
+					      insert_watchpoint_ftype *fn);
+
 extern int remove_breakpoints (void);
 
 /* This function can be used to physically insert eventpoints from the
Index: ia64-linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/ia64-linux-nat.c,v
retrieving revision 1.26
diff -u -p -r1.26 ia64-linux-nat.c
--- ia64-linux-nat.c	13 Oct 2004 21:40:41 -0000	1.26
+++ ia64-linux-nat.c	27 Oct 2004 21:43:50 -0000
@@ -39,6 +39,8 @@
 
 #include <asm/ptrace_offsets.h>
 #include <sys/procfs.h>
+#include "observer.h"
+#include "linux-nat.h"
 
 /* Prototypes for supply_gregset etc. */
 #include "gregset.h"
@@ -559,8 +561,9 @@ is_power_of_2 (int val)
   return onecount <= 1;
 }
 
-int
-ia64_linux_insert_watchpoint (ptid_t ptid, CORE_ADDR addr, int len, int rw)
+/* Internal routine to insert one watchpoint for a specified thread.  */
+static int
+ia64_linux_insert_one_watchpoint (ptid_t ptid, CORE_ADDR addr, int len, int rw)
 {
   int idx;
   long dbr_addr, dbr_mask;
@@ -606,8 +609,38 @@ ia64_linux_insert_watchpoint (ptid_t pti
   return 0;
 }
 
+/* Internal callback routine which can be used via iterate_over_lwps
+   to insert a specific watchpoint from all active threads.  */
+static int
+ia64_linux_insert_watchpoint_callback (struct lwp_info *lwp, void *data)
+{
+  struct target_watchpoint *args = (struct target_watchpoint *)data;
+
+  return ia64_linux_insert_one_watchpoint (lwp->ptid, args->addr,
+		 		     	   args->len, args->type);
+}
+
+/* Insert a watchpoint for all threads.  */
 int
-ia64_linux_remove_watchpoint (ptid_t ptid, CORE_ADDR addr, int len)
+ia64_linux_insert_watchpoint (ptid_t ptid, CORE_ADDR addr, int len, int rw)
+{
+  struct target_watchpoint args;
+
+  args.addr = addr;
+  args.len = len;
+  args.type = rw;
+
+  /* For ia64, watchpoints must be inserted/removed on each thread so
+     we iterate over the lwp list.  */
+  if (iterate_over_lwps (&ia64_linux_insert_watchpoint_callback, &args))
+    return -1;
+
+  return 0;
+}
+
+/* Internal routine to remove one watchpoint for a specified thread.  */
+static int
+ia64_linux_remove_one_watchpoint (ptid_t ptid, CORE_ADDR addr, int len)
 {
   int idx;
   long dbr_addr, dbr_mask;
@@ -630,6 +663,34 @@ ia64_linux_remove_watchpoint (ptid_t pti
   return -1;
 }
 
+/* Internal callback routine which can be used via iterate_over_lwps
+   to remove a specific watchpoint from all active threads.  */
+static int
+ia64_linux_remove_watchpoint_callback (struct lwp_info *lwp, void *data)
+{
+  struct target_watchpoint *args = (struct target_watchpoint *)data;
+
+  return ia64_linux_remove_one_watchpoint (lwp->ptid, args->addr,
+		 		     	   args->len);
+}
+
+/* Remove a watchpoint for all threads.  */
+int
+ia64_linux_remove_watchpoint (ptid_t ptid, CORE_ADDR addr, int len)
+{
+  struct target_watchpoint args;
+
+  args.addr = addr;
+  args.len = len;
+
+  /* For ia64, watchpoints must be inserted/removed on each thread so
+     we iterate over the lwp list.  */
+  if (iterate_over_lwps (&ia64_linux_remove_watchpoint_callback, &args))
+    return -1;
+
+  return 0;
+}
+
 int
 ia64_linux_stopped_data_address (CORE_ADDR *addr_p)
 {
@@ -674,3 +735,18 @@ ia64_linux_xfer_unwind_table (struct tar
 {
   return syscall (__NR_getunwind, readbuf, len);
 }
+
+/* Observer function for a new thread attach.  We need to insert
+   existing watchpoints on the new thread.  */
+void
+ia64_linux_new_thread (ptid_t ptid)
+{
+  insert_watchpoints_for_new_thread (ptid, &ia64_linux_insert_one_watchpoint);
+}
+
+void
+_initialize_ia64_linux_nat (void)
+{
+  observer_attach_new_thread (ia64_linux_new_thread);
+}
+    
Index: s390-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/s390-nat.c,v
retrieving revision 1.12
diff -u -p -r1.12 s390-nat.c
--- s390-nat.c	18 Feb 2004 04:17:35 -0000	1.12
+++ s390-nat.c	27 Oct 2004 21:43:50 -0000
@@ -112,18 +112,25 @@ fill_fpregset (fpregset_t *regp, int reg
 			      ((char *)regp) + regmap_fpregset[i]);
 }
 
-/* Find the TID for the current inferior thread to use with ptrace.  */
+/* Find the TID for use with ptrace.  */
 static int
-s390_inferior_tid (void)
+s390_tid (ptid_t ptid)
 {
   /* GNU/Linux LWP ID's are process ID's.  */
-  int tid = TIDGET (inferior_ptid);
+  int tid = TIDGET (ptid);
   if (tid == 0)
-    tid = PIDGET (inferior_ptid); /* Not a threaded program.  */
+    tid = PIDGET (ptid); /* Not a threaded program.  */
 
   return tid;
 }
 
+/* Find the TID for the current inferior thread to use with ptrace.  */
+static int
+s390_inferior_tid (void)
+{
+  return s390_tid (inferior_ptid);
+}
+
 /* Fetch all general-purpose registers from process/thread TID and
    store their values in GDB's register cache.  */
 static void
@@ -269,9 +276,9 @@ s390_stopped_by_watchpoint (void)
 }
 
 static void
-s390_fix_watch_points (void)
+s390_fix_watch_points (ptid_t ptid)
 {
-  int tid = s390_inferior_tid ();
+  int tid = s390_tid (ptid);
 
   per_struct per_info;
   ptrace_area parea;
@@ -308,8 +315,9 @@ s390_fix_watch_points (void)
     perror_with_name ("Couldn't modify watchpoint status");
 }
 
-int
-s390_insert_watchpoint (CORE_ADDR addr, int len)
+/* Insert a specified watchpoint on a specified thread.  */
+static int
+s390_insert_one_watchpoint (ptid_t ptid, CORE_ADDR addr, int len, int type)
 {
   struct watch_area *area = xmalloc (sizeof (struct watch_area));
   if (!area)
@@ -321,12 +329,36 @@ s390_insert_watchpoint (CORE_ADDR addr, 
   area->next = watch_base;
   watch_base = area;
 
-  s390_fix_watch_points ();
+  s390_fix_watch_points (ptid);
   return 0;
 }
 
+/* Callback routine to use with iterate_over_lwps to insert a specified
+   watchpoint from all threads.  */
+static int
+s390_insert_watchpoint_callback (struct lwp_info *lwp, void *data)
+{
+  struct target_watchpoint *args = (struct target_watchpoint *)data;
+
+  return s390_insert_one_watchpoint (lwp->ptid, args->addr, args->len, 0);
+}
+
+/* Insert a specified watchpoint on all threads.  */
 int
-s390_remove_watchpoint (CORE_ADDR addr, int len)
+s390_insert_watchpoint (CORE_ADDR addr, int len)
+{
+  struct target_watchpoint args;
+
+  args.addr = addr;
+  args.len = len;
+  /* For the S390, a watchpoint must be inserted/removed for each
+     thread so we iterate over the list of existing lwps.  */
+  return iterate_over_lwps (&s390_insert_watchpoint_callback, &args);
+}
+
+/* Remove a specified watchpoint from a specified thread.  */
+static int
+s390_remove_one_watchpoint (ptid_t ptid, CORE_ADDR addr, int len)
 {
   struct watch_area *area, **parea;
 
@@ -346,10 +378,32 @@ s390_remove_watchpoint (CORE_ADDR addr, 
   *parea = area->next;
   xfree (area);
 
-  s390_fix_watch_points ();
+  s390_fix_watch_points (ptid);
   return 0;
 }
 
+/* Callback routine to use with iterate_over_lwps to remove a specified
+   watchpoint from all threads.  */
+static int
+s390_remove_watchpoint_callback (struct lwp_info *lwp, void *data)
+{
+  struct s390_watchpoint_args *args = (struct s390_watchpoint_args *)data;
+
+  return s390_remove_one_watchpoint (lwp->ptid, args->addr, args->len);
+}
+
+/* Remove a specified watchpoint from all threads.  */
+int
+s390_remove_watchpoint (CORE_ADDR addr, int len)
+{
+  struct s390_watchpoint_args args;
+
+  args.addr = addr;
+  args.len = len;
+  /* For the S390, a watchpoint must be inserted/removed for each
+     thread so we iterate over the list of existing lwps.  */
+  return iterate_over_lwps (&s390_remove_watchpoint_callback, &args);
+}
 
 int
 kernel_u_size (void)
@@ -357,3 +411,18 @@ kernel_u_size (void)
   return sizeof (struct user);
 }
 
+/* New thread observer that inserts all existing watchpoints on the
+   new thread.  */
+static int
+s390_new_thread (ptid_t ptid)
+{
+  return insert_watchpoints_for_new_thread (ptid, &s390_insert_one_watchpoint);
+}
+
+void
+_initialize_s390_nat (void)
+{
+  observer_attach_new_thread (s390_new_thread);
+}
+
+
Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/target.h,v
retrieving revision 1.65
diff -u -p -r1.65 target.h
--- target.h	8 Oct 2004 20:29:55 -0000	1.65
+++ target.h	27 Oct 2004 21:43:51 -0000
@@ -178,6 +178,15 @@ extern char *target_signal_to_name (enum
 /* Given a name (SIGHUP, etc.), return its signal.  */
 enum target_signal target_signal_from_name (char *);
 \f
+
+/* Watchpoint specification.  */
+struct target_watchpoint
+  {
+    CORE_ADDR addr;
+    int len;
+    int type;
+  };
+
 /* Request the transfer of up to LEN 8-bit bytes of the target's
    OBJECT.  The OFFSET, for a seekable object, specifies the starting
    point.  The ANNEX can be used to provide additional data-specific
Index: thread-db.c
===================================================================
RCS file: /cvs/src/src/gdb/thread-db.c,v
retrieving revision 1.46
diff -u -p -r1.46 thread-db.c
--- thread-db.c	8 Oct 2004 20:29:56 -0000	1.46
+++ thread-db.c	27 Oct 2004 21:43:51 -0000
@@ -34,6 +34,7 @@
 #include "target.h"
 #include "regcache.h"
 #include "solib-svr4.h"
+#include "observer.h"
 
 #ifdef HAVE_GNU_LIBC_VERSION_H
 #include <gnu/libc-version.h>
@@ -722,6 +723,7 @@ attach_thread (ptid_t ptid, const td_thr
 {
   struct thread_info *tp;
   td_err_e err;
+  ptid_t new_ptid;
 
   /* If we're being called after a TD_CREATE event, we may already
      know about this thread.  There are two ways this can happen.  We
@@ -757,11 +759,16 @@ attach_thread (ptid_t ptid, const td_thr
   if (ti_p->ti_state == TD_THR_UNKNOWN || ti_p->ti_state == TD_THR_ZOMBIE)
     return;			/* A zombie thread -- do not attach.  */
 
+  new_ptid = BUILD_LWP (ti_p->ti_lid, GET_PID (ptid));
+
   /* Under GNU/Linux, we have to attach to each and every thread.  */
 #ifdef ATTACH_LWP
-  ATTACH_LWP (BUILD_LWP (ti_p->ti_lid, GET_PID (ptid)), 0);
+  ATTACH_LWP (new_ptid, 0);
 #endif
 
+  /* Inform any observers of new attached thread.  */
+  observer_notify_new_thread (new_ptid);
+
   /* Enable thread event reporting for this thread.  */
   err = td_thr_event_enable_p (th_p, 1);
   if (err != TD_OK)
Index: doc/observer.texi
===================================================================
RCS file: /cvs/src/src/gdb/doc/observer.texi,v
retrieving revision 1.8
diff -u -p -r1.8 observer.texi
--- doc/observer.texi	1 Sep 2004 17:59:37 -0000	1.8
+++ doc/observer.texi	27 Oct 2004 21:43:51 -0000
@@ -95,3 +95,7 @@ inferior, and before any information on 
 The specified shared library has been discovered to be unloaded.
 @end deftypefun
 
+@deftypefun void new_thread (ptid_t @var{ptid})
+A new thread has been attached to.
+@end deftypefun
+