[RFA]: Modified Watchthreads Patch

[RFA]: Modified Watchthreads Patch

[Jeff Johnston]

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

The following is a modified version of my thread watchpoint patch from 
October/November.  It removes the code I had used to switch between lwp ptids 
and thread ptids now that Daniel's lwp patch is in place.  It uses the former 
version of my observer that is linux-specific and is activated in attach_thread 
in linux-thread-db.c.  Eli, I renamed the observer as asked to indicate this.

I also addressed Ulrich's comments regarding simplifying the S390 code and using 
the s390_fix_watch_points call to actually put the watchpoints on the new thread.

Ulrich/Daniel can you take a look to verify everything is in place.  Daniel, I 
realize that this touches files that are currently in patch state for you.  I 
have no problem waiting for your latest patch to apply and retrofitting my 
changes at check-in if necessary.

As I mentioned before, more is required to get ia64 threaded watchpoints to 
work.  For S390, this change allows it to set and recognize threaded watchpoints.

-- Jeff J.

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

         * breakpoint.c (insert_watchpoints_for_new_thread): New function.
         (print_it_typical): Do not issue an error for bp_thread_event
         if a subsequent event is on the chain.
         * 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_stopped_data_address): Call target_get_lwp to ensure
         the ptid has its lwp field filled in.
         (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 linux thread observer.
         * linux-nat.h (struct linux_watchpoint): New structure.
         * linux-thread-db.c (attach_thread): Notify all linux new thread
         observers.
         * s390-nat.c (s390_tid): 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_watchpoint_callback): New function.
         (s390_insert_watchpoint): Change to iterate through lwps and
         insert the specified watchpoint on each thread.
         (s390_linux_new_thread): New linux thread observer.
         (_initialize_s390_nat): New function.  Initialize
         new linux thread observer.
         * Makefile.in (s390-nat.o, ia64-linux-nat.o): Add new header file
         dependencies.

doc/ChangeLog

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

         * observer.texi (linux_new_thread): New observer.



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

Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.676
diff -u -p -r1.676 Makefile.in
--- Makefile.in	7 Dec 2004 22:17:59 -0000	1.676
+++ Makefile.in	9 Dec 2004 23:18:41 -0000
@@ -2059,7 +2059,8 @@ ia64-aix-nat.o: ia64-aix-nat.c $(defs_h)
 	$(objfiles_h) $(gdb_stat_h)
 ia64-aix-tdep.o: ia64-aix-tdep.c $(defs_h)
 ia64-linux-nat.o: ia64-linux-nat.c $(defs_h) $(gdb_string_h) $(inferior_h) \
-	$(target_h) $(gdbcore_h) $(regcache_h) $(gdb_wait_h) $(gregset_h)
+	$(target_h) $(gdbcore_h) $(regcache_h) $(gdb_wait_h) $(gregset_h) \
+	$(linux_nat_h) $(observer_h)
 ia64-linux-tdep.o: ia64-linux-tdep.c $(defs_h) $(ia64_tdep_h) \
 	$(arch_utils_h) $(gdbcore_h) $(regcache_h)
 ia64-tdep.o: ia64-tdep.c $(defs_h) $(inferior_h) $(gdbcore_h) \
@@ -2440,7 +2441,7 @@ rs6000-tdep.o: rs6000-tdep.c $(defs_h) $
 	$(ppc_tdep_h) $(gdb_assert_h) $(dis_asm_h) $(trad_frame_h) \
 	$(frame_unwind_h) $(frame_base_h)
 s390-nat.o: s390-nat.c $(defs_h) $(tm_h) $(regcache_h) $(inferior_h) \
-	$(s390_tdep_h)
+	$(s390_tdep_h) $(observer_h) $(linux_nat_h)
 s390-tdep.o: s390-tdep.c $(defs_h) $(arch_utils_h) $(frame_h) $(inferior_h) \
 	$(symtab_h) $(target_h) $(gdbcore_h) $(gdbcmd_h) $(objfiles_h) \
 	$(tm_h) $(__bfd_bfd_h) $(floatformat_h) $(regcache_h) \
Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.186
diff -u -p -r1.186 breakpoint.c
--- breakpoint.c	1 Dec 2004 06:54:56 -0000	1.186
+++ breakpoint.c	9 Dec 2004 23:18:42 -0000
@@ -738,6 +738,90 @@ 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 where 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)
+	{
+	  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)
@@ -2112,8 +2196,13 @@ print_it_typical (bpstat bs)
       break;
 
     case bp_thread_event:
-      /* Not sure how we will get here. 
-	 GDB should not stop for these breakpoints.  */
+      /* We can only get here legitimately if something further on the bs 
+	 list has caused the stop status to be noisy.  A valid example
+	 of this is a new thread event and a software watchpoint have
+	 both occurred.  */
+      if (bs->next)
+        return PRINT_UNKNOWN;
+
       printf_filtered ("Thread Event Breakpoint: gdb should not stop!\n");
       return PRINT_NOTHING;
       break;
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	9 Dec 2004 23:18:42 -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	9 Dec 2004 23:18:42 -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 linux_watchpoint *args = (struct linux_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 linux_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 linux_watchpoint *args = (struct linux_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 linux_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,19 @@ 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.  */
+static 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_linux_new_thread (ia64_linux_new_thread);
+}
+    
Index: linux-nat.h
===================================================================
RCS file: /cvs/src/src/gdb/linux-nat.h,v
retrieving revision 1.6
diff -u -p -r1.6 linux-nat.h
--- linux-nat.h	29 Mar 2004 18:07:14 -0000	1.6
+++ linux-nat.h	9 Dec 2004 23:18:42 -0000
@@ -63,6 +63,14 @@ struct lwp_info
   struct lwp_info *next;
 };
 
+/* Watchpoint description.  */
+struct linux_watchpoint
+{
+  CORE_ADDR addr;
+  int len;
+  int type;
+};
+
 /* Read/write to target memory via the Linux kernel's "proc file
    system".  */
 struct mem_attrib;
Index: linux-thread-db.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-thread-db.c,v
retrieving revision 1.2
diff -u -p -r1.2 linux-thread-db.c
--- linux-thread-db.c	8 Dec 2004 15:10:30 -0000	1.2
+++ linux-thread-db.c	9 Dec 2004 23:18:42 -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>
@@ -713,6 +714,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
@@ -748,11 +750,18 @@ 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
 
+  /* Notify any observers of a new linux thread.  This
+     would include any linux platforms that have to insert hardware
+     watchpoints on every thread.  */
+  observer_notify_linux_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: 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	9 Dec 2004 23:18:42 -0000
@@ -27,6 +27,8 @@
 #include "inferior.h"
 
 #include "s390-tdep.h"
+#include "linux-nat.h"
+#include "observer.h"
 
 #include <asm/ptrace.h>
 #include <sys/ptrace.h>
@@ -112,14 +114,14 @@ 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;
 }
@@ -203,7 +205,7 @@ store_fpregs (int tid, int regnum)
 void
 fetch_inferior_registers (int regnum)
 {
-  int tid = s390_inferior_tid ();
+  int tid = s390_tid (inferior_ptid);
 
   if (regnum == -1 
       || (regnum < S390_NUM_REGS && regmap_gregset[regnum] != -1))
@@ -219,7 +221,7 @@ fetch_inferior_registers (int regnum)
 void
 store_inferior_registers (int regnum)
 {
-  int tid = s390_inferior_tid ();
+  int tid = s390_tid (inferior_ptid);
 
   if (regnum == -1 
       || (regnum < S390_NUM_REGS && regmap_gregset[regnum] != -1))
@@ -261,7 +263,7 @@ s390_stopped_by_watchpoint (void)
   parea.len = sizeof (per_lowcore);
   parea.process_addr = (addr_t) & per_lowcore;
   parea.kernel_addr = offsetof (struct user_regs_struct, per_info.lowcore);
-  if (ptrace (PTRACE_PEEKUSR_AREA, s390_inferior_tid (), &parea) < 0)
+  if (ptrace (PTRACE_PEEKUSR_AREA, s390_tid (inferior_ptid), &parea) < 0)
     perror_with_name ("Couldn't retrieve watchpoint status");
 
   return per_lowcore.perc_storage_alteration == 1
@@ -269,9 +271,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,6 +310,16 @@ s390_fix_watch_points (void)
     perror_with_name ("Couldn't modify watchpoint status");
 }
 
+/* Callback routine to use with iterate_over_lwps to insert a specified
+   watchpoint on all threads.  */
+static int
+s390_insert_watchpoint_callback (struct lwp_info *lwp, void *data)
+{
+  s390_fix_watch_points (lwp->ptid);
+  return 0;
+}
+
+/* Insert a specified watchpoint on all threads.  */
 int
 s390_insert_watchpoint (CORE_ADDR addr, int len)
 {
@@ -321,10 +333,24 @@ s390_insert_watchpoint (CORE_ADDR addr, 
   area->next = watch_base;
   watch_base = area;
 
-  s390_fix_watch_points ();
+  /* For the S390, a watchpoint must be inserted/removed for each
+     thread so we iterate over the list of existing lwps.  */
+  if (iterate_over_lwps (&s390_insert_watchpoint_callback, NULL))
+    return -1;
+
+  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)
+{
+  s390_fix_watch_points (lwp->ptid);
   return 0;
 }
 
+/* Remove a specified watchpoint from all threads.  */
 int
 s390_remove_watchpoint (CORE_ADDR addr, int len)
 {
@@ -346,7 +372,11 @@ s390_remove_watchpoint (CORE_ADDR addr, 
   *parea = area->next;
   xfree (area);
 
-  s390_fix_watch_points ();
+  /* For the S390, a watchpoint must be inserted/removed for each
+     thread so we iterate over the list of existing lwps.  */
+  if (iterate_over_lwps (&s390_remove_watchpoint_callback, NULL))
+    return -1;
+
   return 0;
 }
 
@@ -357,3 +387,19 @@ kernel_u_size (void)
   return sizeof (struct user);
 }
 
+/* New thread observer that inserts all existing watchpoints on the
+   new thread.  */
+static void
+s390_linux_new_thread (ptid_t ptid)
+{
+  /* Add existing watchpoints to new thread.  */
+  s390_fix_watch_points (ptid);
+}
+
+void
+_initialize_s390_nat (void)
+{
+  observer_attach_linux_new_thread (s390_linux_new_thread);
+}
+
+

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

Index: observer.texi
===================================================================
RCS file: /cvs/src/src/gdb/doc/observer.texi,v
retrieving revision 1.8
diff -u -p -r1.8 observer.texi
--- observer.texi	1 Sep 2004 17:59:37 -0000	1.8
+++ observer.texi	9 Dec 2004 23:31:58 -0000
@@ -95,3 +95,8 @@ inferior, and before any information on 
 The specified shared library has been discovered to be unloaded.
 @end deftypefun
 
+@deftypefun void linux_new_thread (ptid_t @var{ptid})
+A new linux thread described by @var{ptid} has been officially attached
+to by gdb.
+@end deftypefun
+

Re: [RFA]: Modified Watchthreads Patch

[Eli Zaretskii]

> Date: Thu, 09 Dec 2004 18:36:13 -0500
> From: Jeff Johnston <jjohnstn@redhat.com>
> 
> The following is a modified version of my thread watchpoint patch from 
> October/November.  It removes the code I had used to switch between lwp ptids 
> and thread ptids now that Daniel's lwp patch is in place.  It uses the former 
> version of my observer that is linux-specific and is activated in attach_thread 
> in linux-thread-db.c.  Eli, I renamed the observer as asked to indicate this.

Thanks.

>          * breakpoint.c (insert_watchpoints_for_new_thread): New function.
>          (print_it_typical): Do not issue an error for bp_thread_event
>          if a subsequent event is on the chain.
>          * breakpoint.h (insert_watchpoints_for_new_thread): New prototype.

Hmm... the new function insert_watchpoints_for_new_thread is called
only by ia64_linux_new_thread.  Is there any policy for functions that
are only used by a single port?  Do we care that all the other GDB
builds will get a useless function compiled into them?  Should we
perhaps #ifdef it away conditioned on some symbol?

> +@deftypefun void linux_new_thread (ptid_t @var{ptid})
> +A new linux thread described by @var{ptid} has been officially attached
> +to by gdb.
> +@end deftypefun

What does it mean ``officially attached''?  Can a thread be attached
to ``unofficially''?

Re: [RFA]: Modified Watchthreads Patch

[Daniel Jacobowitz]

On Fri, Dec 10, 2004 at 02:20:39PM +0200, Eli Zaretskii wrote:
> Hmm... the new function insert_watchpoints_for_new_thread is called
> only by ia64_linux_new_thread.  Is there any policy for functions that
> are only used by a single port?  Do we care that all the other GDB
> builds will get a useless function compiled into them?  Should we
> perhaps #ifdef it away conditioned on some symbol?

Let's not.  Conditional compilation is bad... if it were more than a
single function, we could move it into its own file.

However, I think ia64_linux_new_thread's use should be taken as an
example.  If I understand Jeff's patch correctly, a number of other
targets with hardware watchpoints will need it also.

-- 
Daniel Jacobowitz

Re: [RFA]: Modified Watchthreads Patch

[Jeff Johnston]

Daniel Jacobowitz wrote:
> On Fri, Dec 10, 2004 at 02:20:39PM +0200, Eli Zaretskii wrote:
> 
>>Hmm... the new function insert_watchpoints_for_new_thread is called
>>only by ia64_linux_new_thread.  Is there any policy for functions that
>>are only used by a single port?  Do we care that all the other GDB
>>builds will get a useless function compiled into them?  Should we
>>perhaps #ifdef it away conditioned on some symbol?
> 
> 
> Let's not.  Conditional compilation is bad... if it were more than a
> single function, we could move it into its own file.
> 
> However, I think ia64_linux_new_thread's use should be taken as an
> example.  If I understand Jeff's patch correctly, a number of other
> targets with hardware watchpoints will need it also.
> 

Originally, S390 also shared this code as it too has to insert watchpoints on 
all threads.  However, it stores its own list of watchpoints so it doesn't 
require breakpoint.c to go through the breakpoint list and find them anymore.

I had thought of making the function ia64-only after simplifying S390 but I 
figured there would be in all likelihood more platforms in the future that would 
need it.

Either way is fine with me.

-- Jeff J.

Re: [RFA]: Modified Watchthreads Patch

[Michael Snyder]

Jeff Johnston wrote:

> Originally, S390 also shared this code as it too has to insert 
> watchpoints on all threads.  However, it stores its own list of 
> watchpoints so it doesn't require breakpoint.c to go through the 
> breakpoint list and find them anymore.

Maybe breakpoint.h should export a method for traversing
the breakpoint list (as objfiles.h does for the objfiles
list).

Re: [RFA]: Modified Watchthreads Patch

[jjohnstn]

On Thu, 23 Dec 2004, Michael Snyder wrote:

> Jeff Johnston wrote:
> 
> > Originally, S390 also shared this code as it too has to insert 
> > watchpoints on all threads.  However, it stores its own list of 
> > watchpoints so it doesn't require breakpoint.c to go through the 
> > breakpoint list and find them anymore.
> 
> Maybe breakpoint.h should export a method for traversing
> the breakpoint list (as objfiles.h does for the objfiles
> list).
>

In my particular case, there is more than just traversal logic.
I didn't want that to leave breakpoint.c as I felt that future
changes to the value chain logic or breakpoint struct logic
would be best encapsulated there.  Does that seem reasonable?

-- Jeff J.
 
> 
> 

Re: [RFA]: Modified Watchthreads Patch

[Eli Zaretskii]

> Date: Fri, 10 Dec 2004 08:31:16 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Jeff Johnston <jjohnstn@redhat.com>, gdb-patches@sources.redhat.com
> 
> On Fri, Dec 10, 2004 at 02:20:39PM +0200, Eli Zaretskii wrote:
> > Hmm... the new function insert_watchpoints_for_new_thread is called
> > only by ia64_linux_new_thread.  Is there any policy for functions that
> > are only used by a single port?  Do we care that all the other GDB
> > builds will get a useless function compiled into them?  Should we
> > perhaps #ifdef it away conditioned on some symbol?
> 
> Let's not.  Conditional compilation is bad...

I asked several questions.  It sounds like you only replied to the
last one.

If possible, I'd like to hear opinions or official policy, if there is
one, on the other questions.

> However, I think ia64_linux_new_thread's use should be taken as an
> example.  If I understand Jeff's patch correctly, a number of other
> targets with hardware watchpoints will need it also.

Which ones, and how do they get along now?

Re: [RFA]: Modified Watchthreads Patch

[Daniel Jacobowitz]

On Sat, Dec 11, 2004 at 12:50:11AM +0200, Eli Zaretskii wrote:
> > Date: Fri, 10 Dec 2004 08:31:16 -0500
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: Jeff Johnston <jjohnstn@redhat.com>, gdb-patches@sources.redhat.com
> > 
> > On Fri, Dec 10, 2004 at 02:20:39PM +0200, Eli Zaretskii wrote:
> > > Hmm... the new function insert_watchpoints_for_new_thread is called
> > > only by ia64_linux_new_thread.  Is there any policy for functions that
> > > are only used by a single port?  Do we care that all the other GDB
> > > builds will get a useless function compiled into them?  Should we
> > > perhaps #ifdef it away conditioned on some symbol?
> > 
> > Let's not.  Conditional compilation is bad...
> 
> I asked several questions.  It sounds like you only replied to the
> last one.

(1) I don't know of any policy.

(2) Well, I don't care.  I think it's relatively harmless and we
already include a large number of "useless" functions to simplify
design.

(3) We've been trying to move away from conditional compilation within
core files for several years, if I understand correctly.

> If possible, I'd like to hear opinions or official policy, if there is
> one, on the other questions.

GDB does not have much in the way of "official" policy.  This is the
closest thing from gdbint:

@cindex portability
Insertion of new @code{#ifdef}'s will be frowned upon.  It's much better
to write the code portably than to conditionalize it for various systems.


> > However, I think ia64_linux_new_thread's use should be taken as an
> > example.  If I understand Jeff's patch correctly, a number of other
> > targets with hardware watchpoints will need it also.
> 
> Which ones, and how do they get along now?

They don't work.  I don't know any target on which multi-threaded
watchpoints work; do you?

-- 
Daniel Jacobowitz

Re: [RFA]: Modified Watchthreads Patch

[Jeff Johnston]

Eli Zaretskii wrote:
>>Date: Thu, 09 Dec 2004 18:36:13 -0500
>>From: Jeff Johnston <jjohnstn@redhat.com>
>>
>>The following is a modified version of my thread watchpoint patch from 
>>October/November.  It removes the code I had used to switch between lwp ptids 
>>and thread ptids now that Daniel's lwp patch is in place.  It uses the former 
>>version of my observer that is linux-specific and is activated in attach_thread 
>>in linux-thread-db.c.  Eli, I renamed the observer as asked to indicate this.
> 
> 
> Thanks.
> 
> 
>>         * breakpoint.c (insert_watchpoints_for_new_thread): New function.
>>         (print_it_typical): Do not issue an error for bp_thread_event
>>         if a subsequent event is on the chain.
>>         * breakpoint.h (insert_watchpoints_for_new_thread): New prototype.
> 
> 
> Hmm... the new function insert_watchpoints_for_new_thread is called
> only by ia64_linux_new_thread.  Is there any policy for functions that
> are only used by a single port?  Do we care that all the other GDB
> builds will get a useless function compiled into them?  Should we
> perhaps #ifdef it away conditioned on some symbol?
> 
> 
>>+@deftypefun void linux_new_thread (ptid_t @var{ptid})
>>+A new linux thread described by @var{ptid} has been officially attached
>>+to by gdb.
>>+@end deftypefun
> 
> 
> What does it mean ``officially attached''?  Can a thread be attached
> to ``unofficially''?
>

I'm referring to the act of gdb recognizing the thread.  The function itself is 
called attach_thread but it has a #ifdef governing whether a low-level ATTACH is 
required or not.  Gdb now recognizes it has "attached" to the thread whether a 
physical attach is needed or not.  I can drop the "officially" qualifier if it 
is confusing.

-- Jeff J.

Re: [RFA]: Modified Watchthreads Patch

[Eli Zaretskii]

> Date: Fri, 10 Dec 2004 14:04:40 -0500
> From: Jeff Johnston <jjohnstn@redhat.com>
> Cc: gdb-patches@sources.redhat.com
> 
> > What does it mean ``officially attached''?  Can a thread be attached
> > to ``unofficially''?
> 
> I'm referring to the act of gdb recognizing the thread.  The
> function itself is called attach_thread but it has a #ifdef
> governing whether a low-level ATTACH is required or not.  Gdb now
> recognizes it has "attached" to the thread whether a physical attach
> is needed or not.  I can drop the "officially" qualifier if it is
> confusing.

It should either be dropped or replaced with an explanation of when
exactly the thread attachment causes the observer to be invoked.  I'd
prefer the latter (i.e. a more detailed explanation), but if that is
impractical because too many irrelevant details are involved, then
let's just drop the qualifier.

Re: [RFA]: Modified Watchthreads Patch

[Michael Snyder]

Eli Zaretskii wrote:

> Hmm... the new function insert_watchpoints_for_new_thread is called
> only by ia64_linux_new_thread.  Is there any policy for functions that
> are only used by a single port?  Do we care that all the other GDB
> builds will get a useless function compiled into them?  Should we
> perhaps #ifdef it away conditioned on some symbol?

Good grief -- they all get coff, dwarf, elf, stabs, and mdebug 
readers... what's a function here or there?   ;-)

Re: [RFA]: Modified Watchthreads Patch

[Eli Zaretskii]

> Date: Thu, 23 Dec 2004 14:30:54 -0800
> From: Michael Snyder <msnyder@redhat.com>
> CC: Jeff Johnston <jjohnstn@redhat.com>, gdb-patches@sources.redhat.com
> 
> Good grief -- they all get coff, dwarf, elf, stabs, and mdebug 
> readers... what's a function here or there?   ;-)

Not all the readers are just ballast.  For example, the DJGPP
development tools support 3 debug formats: COFF, stabs, and DWARF2,
and 2 executable formats: COFF and stubbed COFF.

And I do think we should have some machinery in place to determine at
configure time what readers will be compiled in.

Re: [RFA]: Modified Watchthreads Patch

[Daniel Jacobowitz]

On Thu, Dec 09, 2004 at 06:36:13PM -0500, Jeff Johnston wrote:
> The following is a modified version of my thread watchpoint patch from 
> October/November.  It removes the code I had used to switch between lwp 
> ptids and thread ptids now that Daniel's lwp patch is in place.  It uses 
> the former version of my observer that is linux-specific and is activated 
> in attach_thread in linux-thread-db.c.  Eli, I renamed the observer as 
> asked to indicate this.
> 
> I also addressed Ulrich's comments regarding simplifying the S390 code and 
> using the s390_fix_watch_points call to actually put the watchpoints on the 
> new thread.
> 
> Ulrich/Daniel can you take a look to verify everything is in place.  
> Daniel, I realize that this touches files that are currently in patch state 
> for you.  I have no problem waiting for your latest patch to apply and 
> retrofitting my changes at check-in if necessary.
> 
> As I mentioned before, more is required to get ia64 threaded watchpoints to 
> work.  For S390, this change allows it to set and recognize threaded 
> watchpoints.

Two formatting comments: please replace "linux" in comments with
"GNU/Linux", and please check copyright years on the modified files.

On the technical side, two questions:

1) I can see that it will be a bit of work to rearrange i386-linux to
use this, but it should be doable.  Do you know offhand of any
i386-specific problems other than inserting watchpoints for all
threads?

2) What should to_stopped_by_watchpoint do in the presence of multiple
threads?  It looks like it relies on inferior_ptid being the thread
which stopped at a watchpoint; I'm worried that that may not be
consistently true in a heavily threaded application.  Maybe it should
iterate over all threads.

The to_stopped_data_address has its own problems with threads; but the
case of handling hitting two watchpoints at once, I think, we can leave
for another day.

This is looking very good so far!

-- 
Daniel Jacobowitz

Re: [RFA]: Modified Watchthreads Patch

[Jeff Johnston]

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

Daniel Jacobowitz wrote:
> On Thu, Dec 09, 2004 at 06:36:13PM -0500, Jeff Johnston wrote:
> 
>>The following is a modified version of my thread watchpoint patch from 
>>October/November.  It removes the code I had used to switch between lwp 
>>ptids and thread ptids now that Daniel's lwp patch is in place.  It uses 
>>the former version of my observer that is linux-specific and is activated 
>>in attach_thread in linux-thread-db.c.  Eli, I renamed the observer as 
>>asked to indicate this.
>>
>>I also addressed Ulrich's comments regarding simplifying the S390 code and 
>>using the s390_fix_watch_points call to actually put the watchpoints on the 
>>new thread.
>>
>>Ulrich/Daniel can you take a look to verify everything is in place.  
>>Daniel, I realize that this touches files that are currently in patch state 
>>for you.  I have no problem waiting for your latest patch to apply and 
>>retrofitting my changes at check-in if necessary.
>>
>>As I mentioned before, more is required to get ia64 threaded watchpoints to 
>>work.  For S390, this change allows it to set and recognize threaded 
>>watchpoints.
> 
> 
> Two formatting comments: please replace "linux" in comments with
> "GNU/Linux", and please check copyright years on the modified files.
> 

Done.  I have reattached the code patch with these fixes.

> On the technical side, two questions:
> 
> 1) I can see that it will be a bit of work to rearrange i386-linux to
> use this, but it should be doable.  Do you know offhand of any
> i386-specific problems other than inserting watchpoints for all
> threads?
>

Actually, with i386/x86-64 I discovered that the debug registers are global in 
scope for the setting of watchpoints (i.e. I didn't have to use the observer). 
The status register, however, is thread-specific for reporting them.  I have 
gotten the watchthreads.exp testcase working for both platforms.  Your lwp fix 
helps a lot with this.  We call TIDGET()/PIDGET() in the low-level code which 
used to get called in the wrong ptid mode so we kept checking the main-thread 
for the watchpoint.

> 2) What should to_stopped_by_watchpoint do in the presence of multiple
> threads?  It looks like it relies on inferior_ptid being the thread
> which stopped at a watchpoint; I'm worried that that may not be
> consistently true in a heavily threaded application.  Maybe it should
> iterate over all threads.
>

It works fine for the watchthreads.exp test once all the mechanisms are in place 
(I have a few more patches to go).  We don't want to iterate over all threads 
unless we know the platform has a problem.  Otherwise, we won't be able to pin 
down a specific watchpoint triggered with the thread/source line that triggered 
it.  Is there a valid scenario where inferior_ptid should not be the thread for 
the signal chosen by the low-level linux-nat code?  If not, I would prefer to 
treat that as a bug that requires pinning down.

> The to_stopped_data_address has its own problems with threads; but the
> case of handling hitting two watchpoints at once, I think, we can leave
> for another day.
> 

The multiple watchpoint scenario occurs in watchthreads.exp.  For ia64, it is a 
real pain which I will describe in my next patch.

> This is looking very good so far!
> 

Thanks.

-- Jeff J.

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

Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.676
diff -u -p -r1.676 Makefile.in
--- Makefile.in	7 Dec 2004 22:17:59 -0000	1.676
+++ Makefile.in	10 Dec 2004 19:57:07 -0000
@@ -2059,7 +2059,8 @@ ia64-aix-nat.o: ia64-aix-nat.c $(defs_h)
 	$(objfiles_h) $(gdb_stat_h)
 ia64-aix-tdep.o: ia64-aix-tdep.c $(defs_h)
 ia64-linux-nat.o: ia64-linux-nat.c $(defs_h) $(gdb_string_h) $(inferior_h) \
-	$(target_h) $(gdbcore_h) $(regcache_h) $(gdb_wait_h) $(gregset_h)
+	$(target_h) $(gdbcore_h) $(regcache_h) $(gdb_wait_h) $(gregset_h) \
+	$(linux_nat_h) $(observer_h)
 ia64-linux-tdep.o: ia64-linux-tdep.c $(defs_h) $(ia64_tdep_h) \
 	$(arch_utils_h) $(gdbcore_h) $(regcache_h)
 ia64-tdep.o: ia64-tdep.c $(defs_h) $(inferior_h) $(gdbcore_h) \
@@ -2440,7 +2441,7 @@ rs6000-tdep.o: rs6000-tdep.c $(defs_h) $
 	$(ppc_tdep_h) $(gdb_assert_h) $(dis_asm_h) $(trad_frame_h) \
 	$(frame_unwind_h) $(frame_base_h)
 s390-nat.o: s390-nat.c $(defs_h) $(tm_h) $(regcache_h) $(inferior_h) \
-	$(s390_tdep_h)
+	$(s390_tdep_h) $(observer_h) $(linux_nat_h)
 s390-tdep.o: s390-tdep.c $(defs_h) $(arch_utils_h) $(frame_h) $(inferior_h) \
 	$(symtab_h) $(target_h) $(gdbcore_h) $(gdbcmd_h) $(objfiles_h) \
 	$(tm_h) $(__bfd_bfd_h) $(floatformat_h) $(regcache_h) \
Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.186
diff -u -p -r1.186 breakpoint.c
--- breakpoint.c	1 Dec 2004 06:54:56 -0000	1.186
+++ breakpoint.c	10 Dec 2004 19:57:07 -0000
@@ -738,6 +738,90 @@ 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 where a watchpoint must be inserted/removed on each
+   individual thread (e.g. ia64-linux and s390-linux).  For
+   ia64 and s390 GNU/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)
+	{
+	  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)
@@ -2112,8 +2196,13 @@ print_it_typical (bpstat bs)
       break;
 
     case bp_thread_event:
-      /* Not sure how we will get here. 
-	 GDB should not stop for these breakpoints.  */
+      /* We can only get here legitimately if something further on the bs 
+	 list has caused the stop status to be noisy.  A valid example
+	 of this is a new thread event and a software watchpoint have
+	 both occurred.  */
+      if (bs->next)
+        return PRINT_UNKNOWN;
+
       printf_filtered ("Thread Event Breakpoint: gdb should not stop!\n");
       return PRINT_NOTHING;
       break;
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	10 Dec 2004 19:57:07 -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 GNU/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	10 Dec 2004 19:57:07 -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 linux_watchpoint *args = (struct linux_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 linux_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 linux_watchpoint *args = (struct linux_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 linux_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,19 @@ 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.  */
+static 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_linux_new_thread (ia64_linux_new_thread);
+}
+    
Index: linux-nat.h
===================================================================
RCS file: /cvs/src/src/gdb/linux-nat.h,v
retrieving revision 1.6
diff -u -p -r1.6 linux-nat.h
--- linux-nat.h	29 Mar 2004 18:07:14 -0000	1.6
+++ linux-nat.h	10 Dec 2004 19:57:07 -0000
@@ -63,6 +63,14 @@ struct lwp_info
   struct lwp_info *next;
 };
 
+/* Watchpoint description.  */
+struct linux_watchpoint
+{
+  CORE_ADDR addr;
+  int len;
+  int type;
+};
+
 /* Read/write to target memory via the Linux kernel's "proc file
    system".  */
 struct mem_attrib;
Index: linux-thread-db.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-thread-db.c,v
retrieving revision 1.2
diff -u -p -r1.2 linux-thread-db.c
--- linux-thread-db.c	8 Dec 2004 15:10:30 -0000	1.2
+++ linux-thread-db.c	10 Dec 2004 19:57:08 -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>
@@ -713,6 +714,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
@@ -748,11 +750,18 @@ 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
 
+  /* Notify any observers of a new GNU/Linux thread.  This
+     would include any GNU/Linux platforms that have to insert hardware
+     watchpoints on every thread.  */
+  observer_notify_linux_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: 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	10 Dec 2004 19:57:08 -0000
@@ -1,5 +1,5 @@
 /* S390 native-dependent code for GDB, the GNU debugger.
-   Copyright 2001, 2003 Free Software Foundation, Inc
+   Copyright 2001, 2003, 2004 Free Software Foundation, Inc
 
    Contributed by D.J. Barrow (djbarrow@de.ibm.com,barrow_dj@yahoo.com)
    for IBM Deutschland Entwicklung GmbH, IBM Corporation.
@@ -27,6 +27,8 @@
 #include "inferior.h"
 
 #include "s390-tdep.h"
+#include "linux-nat.h"
+#include "observer.h"
 
 #include <asm/ptrace.h>
 #include <sys/ptrace.h>
@@ -112,14 +114,14 @@ 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;
 }
@@ -203,7 +205,7 @@ store_fpregs (int tid, int regnum)
 void
 fetch_inferior_registers (int regnum)
 {
-  int tid = s390_inferior_tid ();
+  int tid = s390_tid (inferior_ptid);
 
   if (regnum == -1 
       || (regnum < S390_NUM_REGS && regmap_gregset[regnum] != -1))
@@ -219,7 +221,7 @@ fetch_inferior_registers (int regnum)
 void
 store_inferior_registers (int regnum)
 {
-  int tid = s390_inferior_tid ();
+  int tid = s390_tid (inferior_ptid);
 
   if (regnum == -1 
       || (regnum < S390_NUM_REGS && regmap_gregset[regnum] != -1))
@@ -261,7 +263,7 @@ s390_stopped_by_watchpoint (void)
   parea.len = sizeof (per_lowcore);
   parea.process_addr = (addr_t) & per_lowcore;
   parea.kernel_addr = offsetof (struct user_regs_struct, per_info.lowcore);
-  if (ptrace (PTRACE_PEEKUSR_AREA, s390_inferior_tid (), &parea) < 0)
+  if (ptrace (PTRACE_PEEKUSR_AREA, s390_tid (inferior_ptid), &parea) < 0)
     perror_with_name ("Couldn't retrieve watchpoint status");
 
   return per_lowcore.perc_storage_alteration == 1
@@ -269,9 +271,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,6 +310,16 @@ s390_fix_watch_points (void)
     perror_with_name ("Couldn't modify watchpoint status");
 }
 
+/* Callback routine to use with iterate_over_lwps to insert a specified
+   watchpoint on all threads.  */
+static int
+s390_insert_watchpoint_callback (struct lwp_info *lwp, void *data)
+{
+  s390_fix_watch_points (lwp->ptid);
+  return 0;
+}
+
+/* Insert a specified watchpoint on all threads.  */
 int
 s390_insert_watchpoint (CORE_ADDR addr, int len)
 {
@@ -321,10 +333,24 @@ s390_insert_watchpoint (CORE_ADDR addr, 
   area->next = watch_base;
   watch_base = area;
 
-  s390_fix_watch_points ();
+  /* For the S390, a watchpoint must be inserted/removed for each
+     thread so we iterate over the list of existing lwps.  */
+  if (iterate_over_lwps (&s390_insert_watchpoint_callback, NULL))
+    return -1;
+
+  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)
+{
+  s390_fix_watch_points (lwp->ptid);
   return 0;
 }
 
+/* Remove a specified watchpoint from all threads.  */
 int
 s390_remove_watchpoint (CORE_ADDR addr, int len)
 {
@@ -346,7 +372,11 @@ s390_remove_watchpoint (CORE_ADDR addr, 
   *parea = area->next;
   xfree (area);
 
-  s390_fix_watch_points ();
+  /* For the S390, a watchpoint must be inserted/removed for each
+     thread so we iterate over the list of existing lwps.  */
+  if (iterate_over_lwps (&s390_remove_watchpoint_callback, NULL))
+    return -1;
+
   return 0;
 }
 
@@ -357,3 +387,19 @@ kernel_u_size (void)
   return sizeof (struct user);
 }
 
+/* New thread observer that inserts all existing watchpoints on the
+   new thread.  */
+static void
+s390_linux_new_thread (ptid_t ptid)
+{
+  /* Add existing watchpoints to new thread.  */
+  s390_fix_watch_points (ptid);
+}
+
+void
+_initialize_s390_nat (void)
+{
+  observer_attach_linux_new_thread (s390_linux_new_thread);
+}
+
+

Re: [RFA]: Modified Watchthreads Patch

[Daniel Jacobowitz]

On Fri, Dec 10, 2004 at 03:02:41PM -0500, Jeff Johnston wrote:
> >On the technical side, two questions:
> >
> >1) I can see that it will be a bit of work to rearrange i386-linux to
> >use this, but it should be doable.  Do you know offhand of any
> >i386-specific problems other than inserting watchpoints for all
> >threads?
> >
> 
> Actually, with i386/x86-64 I discovered that the debug registers are global 
> in scope for the setting of watchpoints (i.e. I didn't have to use the 
> observer). The status register, however, is thread-specific for reporting 
> them.  I have gotten the watchthreads.exp testcase working for both 
> platforms.  Your lwp fix helps a lot with this.  We call TIDGET()/PIDGET() 
> in the low-level code which used to get called in the wrong ptid mode so we 
> kept checking the main-thread for the watchpoint.

Er... do you know why the debug registers are global, and what kernel
is this with?  They look thread-specific to me (kernel 2.6.10-rc1). 
They are accessible using PEEKUSR/POKEUSR for each thread, and
__switch_to updates them at context switches.

> >2) What should to_stopped_by_watchpoint do in the presence of multiple
> >threads?  It looks like it relies on inferior_ptid being the thread
> >which stopped at a watchpoint; I'm worried that that may not be
> >consistently true in a heavily threaded application.  Maybe it should
> >iterate over all threads.
> >
> 
> It works fine for the watchthreads.exp test once all the mechanisms are in 
> place (I have a few more patches to go).  We don't want to iterate over all 
> threads unless we know the platform has a problem.  Otherwise, we won't be 
> able to pin down a specific watchpoint triggered with the thread/source 
> line that triggered it.  Is there a valid scenario where inferior_ptid 
> should not be the thread for the signal chosen by the low-level linux-nat 
> code?  If not, I would prefer to treat that as a bug that requires pinning 
> down.

We can delay this issue, then.  I am concerned about losing watchpoints
when other events are active, e.g. a thread event breakpoint or dlopen
breakpoint and a read watchpoint.  I'm sure GDB gets this wrong
already.

Please fix the whitespace at the end of s390-nat.c.  Otherwise, this is
approved if Ulrich is OK with the S390 bits; let's give him a chance to
comment.

-- 
Daniel Jacobowitz

Re: [RFA]: Modified Watchthreads Patch

[Jeff Johnston]

Daniel Jacobowitz wrote:
> On Fri, Dec 10, 2004 at 03:02:41PM -0500, Jeff Johnston wrote:
> 
>>>On the technical side, two questions:
>>>
>>>1) I can see that it will be a bit of work to rearrange i386-linux to
>>>use this, but it should be doable.  Do you know offhand of any
>>>i386-specific problems other than inserting watchpoints for all
>>>threads?
>>>
>>
>>Actually, with i386/x86-64 I discovered that the debug registers are global 
>>in scope for the setting of watchpoints (i.e. I didn't have to use the 
>>observer). The status register, however, is thread-specific for reporting 
>>them.  I have gotten the watchthreads.exp testcase working for both 
>>platforms.  Your lwp fix helps a lot with this.  We call TIDGET()/PIDGET() 
>>in the low-level code which used to get called in the wrong ptid mode so we 
>>kept checking the main-thread for the watchpoint.
> 
> 
> Er... do you know why the debug registers are global, and what kernel
> is this with?  They look thread-specific to me (kernel 2.6.10-rc1). 
> They are accessible using PEEKUSR/POKEUSR for each thread, and
> __switch_to updates them at context switches.
>

I am simply speaking from experience with the RHEL3 kernel.  I got it working 
without touching the insert/remove logic.  I am currently retrofitting new 
changes into the mainline gdb that are much "cleaner" than my previous fixes.  I 
haven't tried x86 on the latest kernel, but I am in the midst of putting 
together an uber-patch with the stuff here plus some other things needed for 
each platform.  IA64 is already finished and running watchthreads.exp on a 
next-release kernel.  I am about to start x86 so I will keep in mind your 
comment.  I'll let you know either way what I had to do to get it working.

> 
>>>2) What should to_stopped_by_watchpoint do in the presence of multiple
>>>threads?  It looks like it relies on inferior_ptid being the thread
>>>which stopped at a watchpoint; I'm worried that that may not be
>>>consistently true in a heavily threaded application.  Maybe it should
>>>iterate over all threads.
>>>
>>
>>It works fine for the watchthreads.exp test once all the mechanisms are in 
>>place (I have a few more patches to go).  We don't want to iterate over all 
>>threads unless we know the platform has a problem.  Otherwise, we won't be 
>>able to pin down a specific watchpoint triggered with the thread/source 
>>line that triggered it.  Is there a valid scenario where inferior_ptid 
>>should not be the thread for the signal chosen by the low-level linux-nat 
>>code?  If not, I would prefer to treat that as a bug that requires pinning 
>>down.
> 
> 
> We can delay this issue, then.  I am concerned about losing watchpoints
> when other events are active, e.g. a thread event breakpoint or dlopen
> breakpoint and a read watchpoint.  I'm sure GDB gets this wrong
> already.
> 
> Please fix the whitespace at the end of s390-nat.c.  Otherwise, this is
> approved if Ulrich is OK with the S390 bits; let's give him a chance to
> comment.
> 

Great.  Will make the white-space change and wait for Ulrich.

-- Jeff J.

Re: [RFA]: Modified Watchthreads Patch

[Jeff Johnston]

Jeff Johnston wrote:
> Daniel Jacobowitz wrote:
> 
>> On Fri, Dec 10, 2004 at 03:02:41PM -0500, Jeff Johnston wrote:
>>
>>>> On the technical side, two questions:
>>>>
>>>> 1) I can see that it will be a bit of work to rearrange i386-linux to
>>>> use this, but it should be doable.  Do you know offhand of any
>>>> i386-specific problems other than inserting watchpoints for all
>>>> threads?
>>>>
>>>
>>> Actually, with i386/x86-64 I discovered that the debug registers are 
>>> global in scope for the setting of watchpoints (i.e. I didn't have to 
>>> use the observer). The status register, however, is thread-specific 
>>> for reporting them.  I have gotten the watchthreads.exp testcase 
>>> working for both platforms.  Your lwp fix helps a lot with this.  We 
>>> call TIDGET()/PIDGET() in the low-level code which used to get called 
>>> in the wrong ptid mode so we kept checking the main-thread for the 
>>> watchpoint.
>>
>>
>>
>> Er... do you know why the debug registers are global, and what kernel
>> is this with?  They look thread-specific to me (kernel 2.6.10-rc1). 
>> They are accessible using PEEKUSR/POKEUSR for each thread, and
>> __switch_to updates them at context switches.
>>
> 
> I am simply speaking from experience with the RHEL3 kernel.  I got it 
> working without touching the insert/remove logic.  I am currently 
> retrofitting new changes into the mainline gdb that are much "cleaner" 
> than my previous fixes.  I haven't tried x86 on the latest kernel, but I 
> am in the midst of putting together an uber-patch with the stuff here 
> plus some other things needed for each platform.  IA64 is already 
> finished and running watchthreads.exp on a next-release kernel.  I am 
> about to start x86 so I will keep in mind your comment.  I'll let you 
> know either way what I had to do to get it working.
>

Interesting results.  Applying my previous patch and just changing the FIXME 
code in dr_get_register and dr_set_register to use the standard:

tid = TIDGET (inferior_ptid);
if (tid == 0)
   tid = PIDGET (inferior_ptid);

allows watchthreads.exp to work for both x86 and x86_64.  For x86, I used an fc3 
system with a 2.6.9-1.667smp kernel.  I had to use an RHEL3 2.4 kernel for x86-64.

The test sets two watchpoints that will be triggered once in the main thread and 
thereafter in two distinct threads.  The test verifies that each value is 
incremented as it should in the correct thread.  It makes sure there are no 
missing jumps.  I have witnessed multiple watchpoint events and thread creation 
events requiring processing at the same time (i.e. deferred events required) and 
it handles these correctly.

I tried an experiment and broke in the thread function in one of the threads.  I 
then watched a variable that can only be triggered in a separate thread.  That 
also worked which was cool.

As I observed before, the actual watchpoint only needs to be set on one thread 
and it will trigger in other threads.  I can send you the additional patch if 
you want to experiment with it.  I am still waiting for a machine with the 
latest RH kernel on it.  I'll let you know if that works the same.

-- Jeff J.

Re: [RFA]: Modified Watchthreads Patch

[Daniel Jacobowitz]

On Fri, Dec 10, 2004 at 06:52:37PM -0500, Jeff Johnston wrote:
> Interesting results.  Applying my previous patch and just changing the 
> FIXME code in dr_get_register and dr_set_register to use the standard:
> 
> tid = TIDGET (inferior_ptid);
> if (tid == 0)
>   tid = PIDGET (inferior_ptid);
> 
> allows watchthreads.exp to work for both x86 and x86_64.  For x86, I used 
> an fc3 system with a 2.6.9-1.667smp kernel.  I had to use an RHEL3 2.4 
> kernel for x86-64.

As Eli says, there are some design issues with the rest of the x86
watchpoint code that make me nervous about doing this.

> The test sets two watchpoints that will be triggered once in the main 
> thread and thereafter in two distinct threads.  The test verifies that each 
> value is incremented as it should in the correct thread.  It makes sure 
> there are no missing jumps.  I have witnessed multiple watchpoint events 
> and thread creation events requiring processing at the same time (i.e. 
> deferred events required) and it handles these correctly.
> 
> I tried an experiment and broke in the thread function in one of the 
> threads.  I then watched a variable that can only be triggered in a 
> separate thread.  That also worked which was cool.
> 
> As I observed before, the actual watchpoint only needs to be set on one 
> thread and it will trigger in other threads.  I can send you the additional 
> patch if you want to experiment with it.  I am still waiting for a machine 
> with the latest RH kernel on it.  I'll let you know if that works the same.

I'm glad to hear that it works for you.  However, since the kernel
sources that I have here patently say that it shouldn't, we need to
understand why.  Do the RHEL kernels have local changes to watchpoint
support?  arch/i386/kernel/ptrace.c and arch/i386/kernel/process.c
should have all the relevant bits.

-- 
Daniel Jacobowitz

Re: [RFA]: Modified Watchthreads Patch

[Jeff Johnston]

Daniel Jacobowitz wrote:
> On Fri, Dec 10, 2004 at 06:52:37PM -0500, Jeff Johnston wrote:
> 
>>Interesting results.  Applying my previous patch and just changing the 
>>FIXME code in dr_get_register and dr_set_register to use the standard:
>>
>>tid = TIDGET (inferior_ptid);
>>if (tid == 0)
>>  tid = PIDGET (inferior_ptid);
>>
>>allows watchthreads.exp to work for both x86 and x86_64.  For x86, I used 
>>an fc3 system with a 2.6.9-1.667smp kernel.  I had to use an RHEL3 2.4 
>>kernel for x86-64.
> 
> 
> As Eli says, there are some design issues with the rest of the x86
> watchpoint code that make me nervous about doing this.
> 
> 
>>The test sets two watchpoints that will be triggered once in the main 
>>thread and thereafter in two distinct threads.  The test verifies that each 
>>value is incremented as it should in the correct thread.  It makes sure 
>>there are no missing jumps.  I have witnessed multiple watchpoint events 
>>and thread creation events requiring processing at the same time (i.e. 
>>deferred events required) and it handles these correctly.
>>
>>I tried an experiment and broke in the thread function in one of the 
>>threads.  I then watched a variable that can only be triggered in a 
>>separate thread.  That also worked which was cool.
>>
>>As I observed before, the actual watchpoint only needs to be set on one 
>>thread and it will trigger in other threads.  I can send you the additional 
>>patch if you want to experiment with it.  I am still waiting for a machine 
>>with the latest RH kernel on it.  I'll let you know if that works the same.
> 
> 
> I'm glad to hear that it works for you.  However, since the kernel
> sources that I have here patently say that it shouldn't, we need to
> understand why.  Do the RHEL kernels have local changes to watchpoint
> support?  arch/i386/kernel/ptrace.c and arch/i386/kernel/process.c
> should have all the relevant bits.
>

There's no comments in the kernel patches (try not to laugh) and I don't know by 
looking at the patches whether any of them are affecting the watchpoint support. 
  I will ask one of the kernel folks here if they know of any local patch that 
might do this.  There are no changes to ptrace.c in the patches I have for fc3.

-- Jeff J.

Re: [RFA]: Modified Watchthreads Patch

[Eli Zaretskii]

> Date: Fri, 10 Dec 2004 18:52:37 -0500
> From: Jeff Johnston <jjohnstn@redhat.com>
> Cc: Daniel Jacobowitz <drow@false.org>, gdb-patches@sources.redhat.com
> 
> Interesting results.  Applying my previous patch and just changing the FIXME 
> code in dr_get_register and dr_set_register to use the standard:
> 
> tid = TIDGET (inferior_ptid);
> if (tid == 0)
>    tid = PIDGET (inferior_ptid);
> 
> allows watchthreads.exp to work for both x86 and x86_64.  For x86, I used an fc3 
> system with a 2.6.9-1.667smp kernel.  I had to use an RHEL3 2.4 kernel for x86-64.
> 
> The test sets two watchpoints that will be triggered once in the main thread and 
> thereafter in two distinct threads.  The test verifies that each value is 
> incremented as it should in the correct thread.  It makes sure there are no 
> missing jumps.  I have witnessed multiple watchpoint events and thread creation 
> events requiring processing at the same time (i.e. deferred events required) and 
> it handles these correctly.
> 
> I tried an experiment and broke in the thread function in one of the threads.  I 
> then watched a variable that can only be triggered in a separate thread.  That 
> also worked which was cool.

While I'm happy it worked for you, please be sure to test somewhat
more complex uses of watchpoints, viz.:

 . Setting several watchpoints on the same data (perhaps with
   different conditions), in the same thread and in several different
   threads.

 . use rwatch and awatch, not only watch, commands to put
   watchpoints.  As you might know, GDB handles data-write watchpoints
   with a different branch of code than what it uses for data-read and
   data-access watchpoints; we need to test both branches to be really
   sure the code works for threaded programs.

For single-threaded programs, the watchpoint support in i386-nat.c
works correctly in the above two classes of complex usage.  In
particular, it allows you to set an unlimited amount of watchpoints of
the same type on the same data, even though there are only 4 debug
registers we can use on x86.  We need to be sure that code doesn't
break for multi-threaded programs.

> As I observed before, the actual watchpoint only needs to be set on
> one thread and it will trigger in other threads.

One issue we should discuss is do we really want this behavior?  Do we
really want GDB to stop the inferior when another thread hits the
watchpoint that we set in a specific thread, or do we want a
watchpoint to break only in the thread in which it was set?  There are
valid arguments for both alternatives, and we never came to any
resolution of this issue, since back when it was discussed, the state
of support in GDB for multi-threaded programs was in flux.

Re: [RFA]: Modified Watchthreads Patch

[Daniel Jacobowitz]

On Sat, Dec 11, 2004 at 01:31:29PM +0200, Eli Zaretskii wrote:
> > Date: Fri, 10 Dec 2004 18:52:37 -0500
> > From: Jeff Johnston <jjohnstn@redhat.com>
> > Cc: Daniel Jacobowitz <drow@false.org>, gdb-patches@sources.redhat.com

[A lot of stuff I completely agree with deleted.]

> > As I observed before, the actual watchpoint only needs to be set on
> > one thread and it will trigger in other threads.
> 
> One issue we should discuss is do we really want this behavior?  Do we
> really want GDB to stop the inferior when another thread hits the
> watchpoint that we set in a specific thread, or do we want a
> watchpoint to break only in the thread in which it was set?  There are
> valid arguments for both alternatives, and we never came to any
> resolution of this issue, since back when it was discussed, the state
> of support in GDB for multi-threaded programs was in flux.

Here's a proposal; I'm thinking it out as I write it, so if you can
break it you get to keep all the shiny pieces.


- The GDB core needs to continue to support watchpoints (hardware
breakpoints; et cetera) triggering in an unexpected thread.

Rationale: some targets won't support any other way.  For instance
page protection based watchpoints on GNU/Linux would probably apply to
all threads.

- As an optimization, the interfaces for inserting and removing
watchpoints could specify a thread to apply the watchpoint to.
The target could, if it supports this operation, apply the watchpoint
to only that thread.

How useful would this be?  For hardware read or write watchpoints,
very.  For "normal" hardware watchpoints (bp_hardware_watchpoint), I
think almost not at all.  GDB relies on knowing the value of all
watched locations to decide whether the watchpoint "triggered" and
should stop the program, right?  If other threads modify the watched
location, GDB would report the watchpoint the next time we stop,
because the value would appear to change.  And consider a scenario like
this:

  Thread A		Thread B

    *p = 0
			 *p = 1
    *p = 0
			 *p = 1
    *p = 0
			 *p = 1
    *p = 0

Assuming that the program didn't stop for any other reason, and that
hardware watchpoints trigger after the write is executed, "watch *p
thread A" would never trigger, because the value would always appear to
be the same.  This fails my usual spot-check for a useful user
interface design, because it is difficult to explain or justify it
without resorting to an explanation of the implementation's
limitations.

If we want thread-specific watchpoint insertion, I think that we need
to either restrict it to rwatch/awatch, or come up with a more useful
behavior for software and hardware watchpoints than the one I've
thought of.  Can you think of one?

-- 
Daniel Jacobowitz

Re: [RFA]: Modified Watchthreads Patch

[Eli Zaretskii]

> Date: Sat, 11 Dec 2004 11:52:37 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Jeff Johnston <jjohnstn@redhat.com>, gdb-patches@sources.redhat.com
> 
> - The GDB core needs to continue to support watchpoints (hardware
> breakpoints; et cetera) triggering in an unexpected thread.

Agreed.

> Rationale: some targets won't support any other way.  For instance
> page protection based watchpoints on GNU/Linux would probably apply to
> all threads.

Another, even better (IMHO) rationale: one important reason for using
watchpoints is to find what code accesses some specific data; when we
use watchpoints for this, we more often than not do not know what
thread will access the data.

> - As an optimization, the interfaces for inserting and removing
> watchpoints could specify a thread to apply the watchpoint to.
> The target could, if it supports this operation, apply the watchpoint
> to only that thread.

This is also needed, for situations where we do know which thread we
are interested in, and other threads hit the same watchpoint too
frequently.

> How useful would this be?  For hardware read or write watchpoints,
> very.  For "normal" hardware watchpoints (bp_hardware_watchpoint), I
> think almost not at all.  GDB relies on knowing the value of all
> watched locations to decide whether the watchpoint "triggered" and
> should stop the program, right?

Yes, but I think (and have said it several times in the past) that
this difference in handling bp_hardware_watchpoint and
bp_read_watchpoint/bp_access_watchpoint is a Bad Thing and we should
rewrite it so that all the types of hardware-assisted watchpoints are
handled in the same way.  The current code that handles
bp_hardware_watchpoint is simply the same code that handled software
bp_watchpoint, which to me doesn't make sense, because at least on
x86, the hardware tells us exactly what address was written to.

(However, note that some platforms that cannot implement
target_stopped_data_address actually take advantage of the different
treatment of bp_hardware_watchpoint to implement `watch' even though
`rwatch' and `awatch' are not implemented.  If we decide to make the
treatment of all hardware-assisted watchpoints similar, we should be
careful not to break those platforms.)

> If other threads modify the watched
> location, GDB would report the watchpoint the next time we stop,
> because the value would appear to change.  And consider a scenario like
> this:
> 
>   Thread A		Thread B
> 
>     *p = 0
> 			 *p = 1
>     *p = 0
> 			 *p = 1
>     *p = 0
> 			 *p = 1
>     *p = 0
> 
> Assuming that the program didn't stop for any other reason, and that
> hardware watchpoints trigger after the write is executed

(I note in parens that on x86, watchpoints trigger _before_ the write
is executed.  Not sure if it matters here.)

> "watch *p thread A" would never trigger, because the value would
> always appear to be the same.

With current handling of bp_hardware_watchpoint, yes.  With the
handling I would like to see, one that is the same as for
bp_read_watchpoint/bp_access_watchpoint, no: the hardware will tell us
that the address was written to!  We just need to be smarter about the
funky logic we have now in breakpoint.c that decides whether or not to
announce the watchpoint, based on whether the value changed or not.

> If we want thread-specific watchpoint insertion, I think that we need
> to either restrict it to rwatch/awatch

It doesn't make sense, from the user point of view, that awatch
catches data writes which watch does not.  That's because, to the
user, awatch is a superset of rwatch and watch (when the latter is
hardware-assisted).

> or come up with a more useful behavior for software and hardware
> watchpoints than the one I've thought of.  Can you think of one?

I think there's nothing wrong with the model you suggested, it's just
that our handling of bp_hardware_watchpoint is wrong.  Assuming we
change it along the lines I suggested above, do you see any further
problems with extending watchpoints to threaded programs?

One aspect where a kernel can help us with multi-threaded programs is
if it tells us not only that a watchpoint has been hit, but also what
thread hit that watchpoint.  Are there any kernel-level features that
can be used to find that out?

Re: [RFA]: Modified Watchthreads Patch

[Daniel Jacobowitz]

On Sat, Dec 11, 2004 at 07:52:08PM +0200, Eli Zaretskii wrote:
> > Date: Sat, 11 Dec 2004 11:52:37 -0500
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: Jeff Johnston <jjohnstn@redhat.com>, gdb-patches@sources.redhat.com
> > 
> > - The GDB core needs to continue to support watchpoints (hardware
> > breakpoints; et cetera) triggering in an unexpected thread.
> 
> Agreed.
> 
> > Rationale: some targets won't support any other way.  For instance
> > page protection based watchpoints on GNU/Linux would probably apply to
> > all threads.
> 
> Another, even better (IMHO) rationale: one important reason for using
> watchpoints is to find what code accesses some specific data; when we
> use watchpoints for this, we more often than not do not know what
> thread will access the data.

That's just a watchpoint without an explicit thread specified.  That's
the default when you say "watch foo".

> Yes, but I think (and have said it several times in the past) that
> this difference in handling bp_hardware_watchpoint and
> bp_read_watchpoint/bp_access_watchpoint is a Bad Thing and we should
> rewrite it so that all the types of hardware-assisted watchpoints are
> handled in the same way.  The current code that handles
> bp_hardware_watchpoint is simply the same code that handled software
> bp_watchpoint, which to me doesn't make sense, because at least on
> x86, the hardware tells us exactly what address was written to.
> 
> (However, note that some platforms that cannot implement
> target_stopped_data_address actually take advantage of the different
> treatment of bp_hardware_watchpoint to implement `watch' even though
> `rwatch' and `awatch' are not implemented.  If we decide to make the
> treatment of all hardware-assisted watchpoints similar, we should be
> careful not to break those platforms.)

OK.  We can fix this.

> > Assuming that the program didn't stop for any other reason, and that
> > hardware watchpoints trigger after the write is executed
> 
> (I note in parens that on x86, watchpoints trigger _before_ the write
> is executed.  Not sure if it matters here.)

How do we get the "new value" for a watchpoint, then?  Do we step over
the instruction?

> > or come up with a more useful behavior for software and hardware
> > watchpoints than the one I've thought of.  Can you think of one?
> 
> I think there's nothing wrong with the model you suggested, it's just
> that our handling of bp_hardware_watchpoint is wrong.  Assuming we
> change it along the lines I suggested above, do you see any further
> problems with extending watchpoints to threaded programs?

This is "extending watchpoints to specific threads" in my
understanding, not to "threaded programs" - obviously a useful feature,
but I think a different one.  Conceptually, our watchpoints already
work for threaded programs.  In practice, Jeff found that on GNU/Linux
they didn't work, and that's what he's fixing, not the "specific
threads" enhancement.  Does that make sense?

> One aspect where a kernel can help us with multi-threaded programs is
> if it tells us not only that a watchpoint has been hit, but also what
> thread hit that watchpoint.  Are there any kernel-level features that
> can be used to find that out?

Right now GDB assumes that this is reflected in inferior_ptid.  At
least for GNU/Linux, this is a reasonable assumption.  The watchpoint
is reported as an event (SIGTRAP); an event has to come from a specific
thread, and it will come from the thread which generated the
trap/fault/etc.

-- 
Daniel Jacobowitz

Re: [RFA]: Modified Watchthreads Patch

[Eli Zaretskii]

> Date: Sat, 11 Dec 2004 13:02:36 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: jjohnstn@redhat.com, gdb-patches@sources.redhat.com
> 
> > Another, even better (IMHO) rationale: one important reason for using
> > watchpoints is to find what code accesses some specific data; when we
> > use watchpoints for this, we more often than not do not know what
> > thread will access the data.
> 
> That's just a watchpoint without an explicit thread specified.  That's
> the default when you say "watch foo".

Yes, but what is the difference between unexpected thread and any
thread?  In practice, it means you will need to stop on any thread,
right?

> > > Assuming that the program didn't stop for any other reason, and that
> > > hardware watchpoints trigger after the write is executed
> > 
> > (I note in parens that on x86, watchpoints trigger _before_ the write
> > is executed.  Not sure if it matters here.)
> 
> How do we get the "new value" for a watchpoint, then?  Do we step over
> the instruction?

Oops, sorry, I managed to completely confuse myself.  You are right,
watchpoints trigger after the data has been written.

> This is "extending watchpoints to specific threads" in my
> understanding, not to "threaded programs"

Okay, I agree.  But whetever the name, I think such an extension
should be a useful one.

Re: [RFA]: Modified Watchthreads Patch

[Daniel Jacobowitz]

On Sat, Dec 11, 2004 at 09:06:12PM +0200, Eli Zaretskii wrote:
> > Date: Sat, 11 Dec 2004 13:02:36 -0500
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: jjohnstn@redhat.com, gdb-patches@sources.redhat.com
> > 
> > > Another, even better (IMHO) rationale: one important reason for using
> > > watchpoints is to find what code accesses some specific data; when we
> > > use watchpoints for this, we more often than not do not know what
> > > thread will access the data.
> > 
> > That's just a watchpoint without an explicit thread specified.  That's
> > the default when you say "watch foo".
> 
> Yes, but what is the difference between unexpected thread and any
> thread?  In practice, it means you will need to stop on any thread,
> right?

Right.  I was thinking about the tiny bit of code needed to detect
that a watchpoint + hitting thread combination that the user doesn't
care about has caused this stop, and quietly resume the inferior.

It sounds like we're in agreement again :-)

-- 
Daniel Jacobowitz

Re: [RFA]: Modified Watchthreads Patch

[Eli Zaretskii]

> Date: Sat, 11 Dec 2004 14:08:20 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: jjohnstn@redhat.com, gdb-patches@sources.redhat.com
> 
> It sounds like we're in agreement again :-)

Which is good, don't you think? ;-)

Re: [RFA]: Modified Watchthreads Patch

[Mark Kettenis]

   Date: Sat, 11 Dec 2004 13:02:36 -0500
   From: Daniel Jacobowitz <drow@false.org>

   On Sat, Dec 11, 2004 at 07:52:08PM +0200, Eli Zaretskii wrote:
   > > Date: Sat, 11 Dec 2004 11:52:37 -0500
   > > From: Daniel Jacobowitz <drow@false.org>
   > > Cc: Jeff Johnston <jjohnstn@redhat.com>, gdb-patches@sources.redhat.com
   > > 
   > > - The GDB core needs to continue to support watchpoints (hardware
   > > breakpoints; et cetera) triggering in an unexpected thread.
   > 
   > Agreed.
   > 
   > > Rationale: some targets won't support any other way.  For instance
   > > page protection based watchpoints on GNU/Linux would probably apply to
   > > all threads.
   > 
   > Another, even better (IMHO) rationale: one important reason for using
   > watchpoints is to find what code accesses some specific data; when we
   > use watchpoints for this, we more often than not do not know what
   > thread will access the data.

   That's just a watchpoint without an explicit thread specified.  That's
   the default when you say "watch foo".

   > Yes, but I think (and have said it several times in the past) that
   > this difference in handling bp_hardware_watchpoint and
   > bp_read_watchpoint/bp_access_watchpoint is a Bad Thing and we should
   > rewrite it so that all the types of hardware-assisted watchpoints are
   > handled in the same way.  The current code that handles
   > bp_hardware_watchpoint is simply the same code that handled software
   > bp_watchpoint, which to me doesn't make sense, because at least on
   > x86, the hardware tells us exactly what address was written to.
   > 
   > (However, note that some platforms that cannot implement
   > target_stopped_data_address actually take advantage of the different
   > treatment of bp_hardware_watchpoint to implement `watch' even though
   > `rwatch' and `awatch' are not implemented.  If we decide to make the
   > treatment of all hardware-assisted watchpoints similar, we should be
   > careful not to break those platforms.)

Yes, page-protection assisted watchpoints in their most primitive form
might cause spurious events when you access memory that is in the same
page as the variable being watched.  GDB will have to check whether
the variable actually changed.

   OK.  We can fix this.

But for the reason given above `watch' always will be a bit different
than `rwatch' and `awatch'.

   > > Assuming that the program didn't stop for any other reason, and that
   > > hardware watchpoints trigger after the write is executed
   > 
   > (I note in parens that on x86, watchpoints trigger _before_ the write
   > is executed.  Not sure if it matters here.)

This is in general true for page-protection assisted watchpoints to.
At least it is for HP-UX.  You get a page fault which is reported as
SIGBUS or SIGSEGV or something like that.  But I guess that for real
hardware watchpoints it is possible for them to trigger after the
write is executed.  And for page-protection assisted watchpoints that
are implemented fully in the kernel this is possible behaviour.  I
think this is what happens on Solaris, but I'm not completely sure.

   How do we get the "new value" for a watchpoint, then?  Do we step over
   the instruction?

In general yes.  This is what HAVE_STEPPABLE_WATCHPOINT,
HAVE_NONSTEPPABLE_WATCHPOINT and HAVE_CONTINUABLE_WATCHPOINT are
dealing with.

   > > or come up with a more useful behavior for software and hardware
   > > watchpoints than the one I've thought of.  Can you think of one?
   > 
   > I think there's nothing wrong with the model you suggested, it's just
   > that our handling of bp_hardware_watchpoint is wrong.  Assuming we
   > change it along the lines I suggested above, do you see any further
   > problems with extending watchpoints to threaded programs?

   This is "extending watchpoints to specific threads" in my
   understanding, not to "threaded programs" - obviously a useful feature,
   but I think a different one.  Conceptually, our watchpoints already
   work for threaded programs.  In practice, Jeff found that on GNU/Linux
   they didn't work, and that's what he's fixing, not the "specific
   threads" enhancement.  Does that make sense?

It does to me.  If there are problems they're most likely in
platform-dependent code (and i386-nat.c *is* platform-dependent code
even if it's used on a number of different platforms).

   > One aspect where a kernel can help us with multi-threaded programs is
   > if it tells us not only that a watchpoint has been hit, but also what
   > thread hit that watchpoint.  Are there any kernel-level features that
   > can be used to find that out?

   Right now GDB assumes that this is reflected in inferior_ptid.  At
   least for GNU/Linux, this is a reasonable assumption.  The watchpoint
   is reported as an event (SIGTRAP); an event has to come from a specific
   thread, and it will come from the thread which generated the
   trap/fault/etc.

I think this is a valid assumption.  Hitting a "hardware" watchpoint
will always trigger some sort of trap (be it a memory fault or some
special hardware interrupt).  A Unix-like kernel should know what
"thread" was active at that point, and be able to report that.

Mark

Re: [RFA]: Modified Watchthreads Patch

[Mark Kettenis]

   Date: Fri, 10 Dec 2004 18:52:37 -0500
   From: Jeff Johnston <jjohnstn@redhat.com>

   As I observed before, the actual watchpoint only needs to be set on
   one thread and it will trigger in other threads.  I can send you
   the additional patch if you want to experiment with it.

This is highly suspicious.  I think it means that there is a bug in
the kernel.  Of course that's in the eye of the beholder since the
there is no documentation whatsoever how watchpoints should work in
the Linux kernel.  I think that, before we change GDB, we should
insists that this gets properly documented, and agreed on by the
kernel people.

   I am still waiting for a machine with the latest RH kernel on it.
   I'll let you know if that works the same.

What the latest RH kernel does should be mostly irrelevant to the GDB
project.  What matters is what the official Linux kernel does.  We
should realize that not everybody is using a kernel that was released
just a week ago.  GDB should also work on older kernels (we've
previously stated that it will work on version 2.0 and later).  Of
course we don't support all features on those kernels, but we
shouldn't break features that worked on those kernels before.  It
seems to me the i386 watchpoint issue has a high-risk to break older
kernels.

Mark

Re: [RFA]: Modified Watchthreads Patch

[Eli Zaretskii]

> Date: Sat, 11 Dec 2004 15:49:09 +0100 (CET)
> From: Mark Kettenis <kettenis@gnu.org>
> CC: jjohnstn@redhat.com, drow@false.org, gdb-patches@sources.redhat.com
> 
> there is no documentation whatsoever how watchpoints should work in
> the Linux kernel.  I think that, before we change GDB, we should
> insists that this gets properly documented, and agreed on by the
> kernel people.

I agree.

> What the latest RH kernel does should be mostly irrelevant to the GDB
> project.  What matters is what the official Linux kernel does.  We
> should realize that not everybody is using a kernel that was released
> just a week ago.  GDB should also work on older kernels (we've
> previously stated that it will work on version 2.0 and later).  Of
> course we don't support all features on those kernels, but we
> shouldn't break features that worked on those kernels before.  It
> seems to me the i386 watchpoint issue has a high-risk to break older
> kernels.

100% agreement.

Re: [RFA]: Modified Watchthreads Patch

[Daniel Jacobowitz]

On Fri, Dec 10, 2004 at 03:37:29PM -0500, Daniel Jacobowitz wrote:
> Please fix the whitespace at the end of s390-nat.c.  Otherwise, this is
> approved if Ulrich is OK with the S390 bits; let's give him a chance to
> comment.

Let's hold off while we discuss the observers issue.

Jeff, I've been thinking about this patch, and another problem occured
to me.  You're using a "new thread" event, but you're not iterating
over threads - you're iterating over LWPs.  So whether or not we want
to use an observer for this action, it's in the wrong conceptual place;
on recent systems we should be able to debug multi-threaded programs
that do not use libpthread with some degree of success.  TLS won't
work, of course, since that's library-supported... but most of the rest
of what libthread_db is not necessary.  The code for this on the GDB
side is not completely in place yet but I'm working up to it - that's
one of the goals of the revamped Linux target_ops.

-- 
Daniel Jacobowitz

Re: [RFA]: Modified Watchthreads Patch

[Mark Kettenis]

   Date: Fri, 10 Dec 2004 20:28:38 -0500
   From: Daniel Jacobowitz <drow@false.org>

   On Fri, Dec 10, 2004 at 03:37:29PM -0500, Daniel Jacobowitz wrote:
   > Please fix the whitespace at the end of s390-nat.c.  Otherwise, this is
   > approved if Ulrich is OK with the S390 bits; let's give him a chance to
   > comment.

   Let's hold off while we discuss the observers issue.

   Jeff, I've been thinking about this patch, and another problem occured
   to me.  You're using a "new thread" event, but you're not iterating
   over threads - you're iterating over LWPs.  So whether or not we want
   to use an observer for this action, it's in the wrong conceptual place;
   on recent systems we should be able to debug multi-threaded programs
   that do not use libpthread with some degree of success.  TLS won't
   work, of course, since that's library-supported... but most of the rest
   of what libthread_db is not necessary.  The code for this on the GDB
   side is not completely in place yet but I'm working up to it - that's
   one of the goals of the revamped Linux target_ops.

Yes.  In principle for a platform that has a 1x1 threads
implementation, the only thing your threads layer should do is
translating between the thread ID's the programmer sees and the lwp
ID's used by the kernel.

Unfortunately for Linux this isn't completely true...

Mark

Re: [RFA]: Modified Watchthreads Patch

[Eli Zaretskii]

> Date: Fri, 10 Dec 2004 14:10:15 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gdb-patches@sources.redhat.com
> 
> 1) I can see that it will be a bit of work to rearrange i386-linux to
> use this, but it should be doable.  Do you know offhand of any
> i386-specific problems other than inserting watchpoints for all
> threads?

The design of the x86 watchpoint support explicitly assumes that
watchpoints are not thread-local.  If we want to lift that limitation,
I think the x86-specific code needs to be redesigned.  Someone who
knows way more than I do about x86 threads and how the debug registers
are handled by the relevant kernels in the presence of threads, should
present a clean replacement design that deals with thread-local
watchpoints.  Small modifications like inserting watchpoints for all
threads and other similar patchwork will simply not cut it, IMHO.

Observe:

> 2) What should to_stopped_by_watchpoint do in the presence of multiple
> threads?  It looks like it relies on inferior_ptid being the thread
> which stopped at a watchpoint; I'm worried that that may not be
> consistently true in a heavily threaded application.  Maybe it should
> iterate over all threads.
> 
> The to_stopped_data_address has its own problems with threads; but the
> case of handling hitting two watchpoints at once, I think, we can leave
> for another day.

These two are just the tip of the iceberg, but already you discovered
that the two cornerstones of the GDB watchpoint support do not work
reliably in multithreaded programs.  We should redesign the x86
watchpoint support instead of taking the evolutionary approach, which
will leave us with messy, unmaintainable, and buggy code.

Re: [RFA]: Modified Watchthreads Patch

[Daniel Jacobowitz]

On Sat, Dec 11, 2004 at 01:00:08AM +0200, Eli Zaretskii wrote:
> > Date: Fri, 10 Dec 2004 14:10:15 -0500
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: gdb-patches@sources.redhat.com
> > 
> > 1) I can see that it will be a bit of work to rearrange i386-linux to
> > use this, but it should be doable.  Do you know offhand of any
> > i386-specific problems other than inserting watchpoints for all
> > threads?
> 
> The design of the x86 watchpoint support explicitly assumes that
> watchpoints are not thread-local.  If we want to lift that limitation,
> I think the x86-specific code needs to be redesigned.  Someone who
> knows way more than I do about x86 threads and how the debug registers
> are handled by the relevant kernels in the presence of threads, should
> present a clean replacement design that deals with thread-local
> watchpoints.  Small modifications like inserting watchpoints for all
> threads and other similar patchwork will simply not cut it, IMHO.

Does the i386 native watchpoint support work on any existing target
with multiple threads?  I think this is a more accurate description of
the assumptions, even though it's from i386-linux-nat.c:

  /* FIXME: kettenis/2001-01-29: It's not clear what we should do with
     multi-threaded processes here.  For now, pretend there is just
     one thread.  */

> Observe:
> 
> > 2) What should to_stopped_by_watchpoint do in the presence of multiple
> > threads?  It looks like it relies on inferior_ptid being the thread
> > which stopped at a watchpoint; I'm worried that that may not be
> > consistently true in a heavily threaded application.  Maybe it should
> > iterate over all threads.
> > 
> > The to_stopped_data_address has its own problems with threads; but the
> > case of handling hitting two watchpoints at once, I think, we can leave
> > for another day.
> 
> These two are just the tip of the iceberg, but already you discovered
> that the two cornerstones of the GDB watchpoint support do not work
> reliably in multithreaded programs.  We should redesign the x86
> watchpoint support instead of taking the evolutionary approach, which
> will leave us with messy, unmaintainable, and buggy code.

That is not a problem with the i386 native support for watchpoints; it
is a problem with the core GDB interfaces for watchpoints, a much
bigger problem.  If you don't think we can support multi-threaded
watchpoints in GDB without doing this redesign first, do you object to
Jeff's current patch?

To get a useful level of support from the i386 watchpoint code, in
fact, looks pretty easy.  Most of it would be local to the existing
low-level support routines which are implemented in an i386-linux
specific file.  I can't say any more than that, since Jeff hasn't
posted his patch yet.

-- 
Daniel Jacobowitz

Re: [RFA]: Modified Watchthreads Patch

[Eli Zaretskii]

> Date: Fri, 10 Dec 2004 18:06:03 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: jjohnstn@redhat.com, gdb-patches@sources.redhat.com
> 
> Does the i386 native watchpoint support work on any existing target
> with multiple threads?

I don't know, but if it does, it's more through luck than through
correct code.

> That is not a problem with the i386 native support for watchpoints; it
> is a problem with the core GDB interfaces for watchpoints

I don't know if it is a problem with the GDB interface; it might be.
For the x86 native code, I _know_ it's a problem, because the code was
designed and written assuming that watchpoints are not thread-local.

I'm not opposed to rethinking the core GDB watchpoint interface as
well, but doing that is a much harder job, because we need to analyze
how various debug interfaces on various platforms can help us know
what thread hit the watchpoint.  Only then we can come up with a
viable design.

> If you don't think we can support multi-threaded watchpoints in GDB
> without doing this redesign first, do you object to Jeff's current
> patch?

You may recall that I objected to using observers for this in the
first place, but my objections were voted down.  Given that, I don't
find it useful to object to Jeff's current patch.  But I will
encourage any attempts to refactor the watchpoint interfaces and x86
native code to accomodate multi-threaded programs, even if in the
meantime we approve some changes that patch around the inappropriate
design to get thread-local watchpoints on some platforms.

> To get a useful level of support from the i386 watchpoint code, in
> fact, looks pretty easy.

It may be easy, but I think we still need to talk about the design.
The debug registers mirroring in i386-nat.c clearly assume that debug
registers are all global.  This might not work at all with threads.

Re: [RFA]: Modified Watchthreads Patch

[Daniel Jacobowitz]

On Sat, Dec 11, 2004 at 01:30:52AM +0200, Eli Zaretskii wrote:
> You may recall that I objected to using observers for this in the
> first place, but my objections were voted down.  Given that, I don't
> find it useful to object to Jeff's current patch.  But I will
> encourage any attempts to refactor the watchpoint interfaces and x86
> native code to accomodate multi-threaded programs, even if in the
> meantime we approve some changes that patch around the inappropriate
> design to get thread-local watchpoints on some platforms.

By whom?  I waited to review the revised version until you had a chance
to comment on the continued use of observers.  The last conclusion I
remember from the previous discussion was that I would prefer to use
the target stack, but didn't have a strong enough preference to object
to an observer.  Jeff asked you if a renamed observer was acceptable,
and you said that it was.

If there's been a miscommunication, if you still object to this use of
the observers, please, say so now.  We can discuss alternatives.  Before
the patch goes in is the best time.

> > To get a useful level of support from the i386 watchpoint code, in
> > fact, looks pretty easy.
> 
> It may be easy, but I think we still need to talk about the design.
> The debug registers mirroring in i386-nat.c clearly assume that debug
> registers are all global.  This might not work at all with threads.

Yes, definitely that would need discussion.

-- 
Daniel Jacobowitz

Re: [RFA]: Modified Watchthreads Patch

[Eli Zaretskii]

> Date: Fri, 10 Dec 2004 18:37:00 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: jjohnstn@redhat.com, gdb-patches@sources.redhat.com
> 
> I waited to review the revised version until you had a chance to
> comment on the continued use of observers.

I still object to the use of observers for a purpose such as this one.
My objections are a bit philosophical, though.

Basically, I think that observers are a last-resort mechanism for
anything that is part of the GDB infrastructure.  It's like hooks or
callbacks--you don't normally expect a program internals to use
callbacks that it provides for higher-level application code.

Put another way, using a mechanism such as observers for internal code
means we leave our internal structure not entirely defined.  We design
the internals, so we ought to know what needs to be done where and
when.  For example, this particular usage of an observer means that we
don't really know in advance that watchpoint insertion needs to be
done for each thread when it is being attached.  Do we really want to
say that we don't know what we are doing in our own program?

Callbacks is something you generally provide for application code that
is not part of the program.  For example, if we ever get to the point
of having libgdb that could be linked into other applications to
create customized debuggers, observers will be an important mechanism
to let libgdb users hook into internal GDB operations without hacking
the code.

In addition, proliferation of observers' use will sooner or later
raise the issue of the order of the observer invocation, since we lack
a machinery for invoking a series of observers in a controlled manner:
we cannot control the order of their invocation and we cannot tell GDB
to stop invoking any additional observers.  The current machinery
assumes that each observer is orthogonal to others in its side
effects; what if this assumption doesn't hold?

> Jeff asked you if a renamed observer was acceptable, and you said
> that it was.

That's because I didn't want to restart a dispute where I was already
told that the party line was that it was okay to use observers for
such puproses (or any puproses, really).

> If there's been a miscommunication, if you still object to this use of
> the observers, please, say so now.  We can discuss alternatives.

Well, the obvious alternative is to call the function that needs to
insert the watchpoints for a thread that has been attached right where
the thread attachment is dealt with.

Re: [RFA]: Modified Watchthreads Patch

[Mark Kettenis]

   Date: Sat, 11 Dec 2004 13:19:03 +0200
   From: "Eli Zaretskii" <eliz@gnu.org>

   > Date: Fri, 10 Dec 2004 18:37:00 -0500
   > From: Daniel Jacobowitz <drow@false.org>
   > Cc: jjohnstn@redhat.com, gdb-patches@sources.redhat.com
   > 
   > I waited to review the revised version until you had a chance to
   > comment on the continued use of observers.

   I still object to the use of observers for a purpose such as this one.
   My objections are a bit philosophical, though.

   Basically, I think that observers are a last-resort mechanism for
   anything that is part of the GDB infrastructure.  It's like hooks or
   callbacks--you don't normally expect a program internals to use
   callbacks that it provides for higher-level application code.

I somewhat disagree here.  GDB is modularized.  Depending on the
configuration, some parts may be present, other parts may not.
Observers provide a very useful mechanism for interaction between
modules without the need to know exactly which modules are present or
not.  But...

   Put another way, using a mechanism such as observers for internal code
   means we leave our internal structure not entirely defined.  We design
   the internals, so we ought to know what needs to be done where and
   when.  For example, this particular usage of an observer means that we
   don't really know in advance that watchpoint insertion needs to be
   done for each thread when it is being attached.  Do we really want to
   say that we don't know what we are doing in our own program?

...indeed.  It's a bit strange that there's a need for a
Linux-specific observer.  The Linux-specific code should know the
details isn't it?  Unfortunately the situation is a bit murky.  The
implementation of hardware (or kernel-assisted) watchpoints is very
platform-dependent.  And on top of that, because of the poor
thread-debugging interface that Linux has, we never really know what
threads/lwps actaully exist within a process.  This may call for
unorthodox ways of handling things.

Personally I think that it's better to declare watchpoints in
multi-threaded programs as unsupported.  Then add a sane interface for
debugging threads and watchpoints to the kernel, before revisiting the
issue in GDB.  I mean, it's like the Linux kernel is no longer Open
Source.

Adding hacks around hacks, like we've been doing to support threads on
Linux for quite some time now is defenitely not a good idea.

   In addition, proliferation of observers' use will sooner or later
   raise the issue of the order of the observer invocation, since we lack
   a machinery for invoking a series of observers in a controlled manner:
   we cannot control the order of their invocation and we cannot tell GDB
   to stop invoking any additional observers.  The current machinery
   assumes that each observer is orthogonal to others in its side
   effects; what if this assumption doesn't hold?

I think we should take a different viewpoint here.  The current
machinery doesn't *assume* that each observer is orthogonal, it's the
*definition* of the interface.  If an observer makes the assumption
that another observer has done its "thing", then that's a programming
error.  This error could be fixed by introducing another notification
that documents that the "thing" has been taken care of.

Mark

Re: [RFA]: Modified Watchthreads Patch

[Daniel Jacobowitz]

On Sat, Dec 11, 2004 at 03:33:52PM +0100, Mark Kettenis wrote:
>    Put another way, using a mechanism such as observers for internal code
>    means we leave our internal structure not entirely defined.  We design
>    the internals, so we ought to know what needs to be done where and
>    when.  For example, this particular usage of an observer means that we
>    don't really know in advance that watchpoint insertion needs to be
>    done for each thread when it is being attached.  Do we really want to
>    say that we don't know what we are doing in our own program?
> 
> ...indeed.  It's a bit strange that there's a need for a
> Linux-specific observer.  The Linux-specific code should know the
> details isn't it?  Unfortunately the situation is a bit murky.  The
> implementation of hardware (or kernel-assisted) watchpoints is very
> platform-dependent.  And on top of that, because of the poor
> thread-debugging interface that Linux has, we never really know what
> threads/lwps actaully exist within a process.  This may call for
> unorthodox ways of handling things.
> 
> Personally I think that it's better to declare watchpoints in
> multi-threaded programs as unsupported.  Then add a sane interface for
> debugging threads and watchpoints to the kernel, before revisiting the
> issue in GDB.  I mean, it's like the Linux kernel is no longer Open
> Source.
> 
> Adding hacks around hacks, like we've been doing to support threads on
> Linux for quite some time now is defenitely not a good idea.

Mark, would you please stop saying this?  I don't believe it to be true
any more.  If you think it's still accurate, please point me at
specific hacks around hacks, and let's see if we can get rid of them
now.

The kernel implementation of hardware watchpoints is very
platform-dependent because the hardware implementation of hardware
watchpoints is very platform-dependent.  If I had a list of all current
and future platforms to work with, maybe I could come up with a common
interface - but I doubt it.  I would expect that they would always be
per-thread on any Linux target with true hardware watchpoints.  Of
course, they'll be process global if they're implemented by page
protections; this is something I've been meaning to implement for
Linux for years, but never had the time for.

If I were designing a new operating system from scratch, and I wanted
to architect a thread debugging interface, it wouldn't look anything
like the Linux kernel's does.  That much is obvious.  However, the two
reasons for that are that I know a lot more about debuggers (and spend
a lot more of my time working on debuggers) than I do about high
performance threading, and that I wouldn't have the historical
architecture of the kernel to contend with.  Without thousands of
man-hours to redo the kernel internals, and probably user-visible
incompatible changes, there aren't a whole lot of options.

Moreover, what we have right now is extremely rich, and reasonably well
defined.  It is not well-documented, since no one's been motivated to
sit down and write a treatise on it, but I know how it is supposed to
work - that's why I knew to raise a flag to Jeff's "process global"
setting of debug registers, because there are almost no process global
actions in Linux ptrace.

It allows for catching the creation and destruction of threads.  This
was one of its big historical holes, which I filled - because the
kernel IS still open.

It allows for finding a list of existing threads when attaching - I
admit, this is one of the big warts.  It's not very straightforward
for LinuxThreads, but it can be done without resorting to thread_db.
For modern threading (NPTL) it's very easy.

I admit there are some peculiarities related to stopping all threads.
But most of them are related to very real situations that we want to be
able to debug: two threads receiving a signal at the same time, hitting
different breakpoints at the same time, et cetera.  Life with threads
is just more complicated.  Some platforms do the complicated bits in
the kernel, and Linux chose to expose an LWP-oriented interface rather
than a whole-process oriented interface so we have to do the
complicated bits in userspace.  That is not going to change, because
the Linux design philosophy for threading is that they are just a
special kind of process; Linux has no concept of "the whole process"
and will not be adding one.  This has been discussed from time to time
on the linux-kernel list.  [There is some correlation to the POSIX
threading concept of a process, for the purpose of POSIX-compliant
signal delivery, but that's the extent of it.]

And I'm busily (at work) improving platform support for NPTL; one of my
goals is to someday rip all the LinuxThreads support code out of GDB. 
But it's going to be a long time before that's a viable option - at
least a couple of years from now.  That's no more friendly than
dropping support for all but the newest kernel.  And we'll need some
of that code to provide quality support for debugging multiple
processes simultaneously, or to support debugging applications which
use clone() directly.

-- 
Daniel Jacobowitz

Re: [RFA]: Modified Watchthreads Patch

[Eli Zaretskii]

> Date: Sat, 11 Dec 2004 11:36:52 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: eliz@gnu.org, jjohnstn@redhat.com, gdb-patches@sources.redhat.com
> 
> The kernel implementation of hardware watchpoints is very
> platform-dependent because the hardware implementation of hardware
> watchpoints is very platform-dependent.

??? Are we still talking about x86?  If so, there are only 2 possible
mechanisms I know about: (1) debug registers and (2) page-level
protection.  Are there any others?  Am I missing something?

Of these two, i386-nat.c currently only supports the first one.  If we
restrict ourselves for a moment to that single mechanism only, what
platform dependencies are we talking about, as far as the _hardware_
implementation of hardware-assisted watchpoints is concerned?  Am I
missing something?

> I would expect that they would always be
> per-thread on any Linux target with true hardware watchpoints.  Of
> course, they'll be process global if they're implemented by page
> protections

The implementation that uses debug registers can be either global or
local, depending on some bit in one of the registers.  One thing GDB
should know about a platform is how that platform fiddles with that
bit.

Re: [RFA]: Modified Watchthreads Patch

[Daniel Jacobowitz]

On Sat, Dec 11, 2004 at 07:27:26PM +0200, Eli Zaretskii wrote:
> > Date: Sat, 11 Dec 2004 11:36:52 -0500
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: eliz@gnu.org, jjohnstn@redhat.com, gdb-patches@sources.redhat.com
> > 
> > The kernel implementation of hardware watchpoints is very
> > platform-dependent because the hardware implementation of hardware
> > watchpoints is very platform-dependent.
> 
> ??? Are we still talking about x86?  If so, there are only 2 possible
> mechanisms I know about: (1) debug registers and (2) page-level
> protection.  Are there any others?  Am I missing something?

No, sorry.  Mark made general comments about "Linux" so that's what I
was responding to.

The range of implementations of MIPS hardware watchpoints, for
instance, are somewhat different than the i386 ones.  You've got the
same two options (that is, if your platform has appropriate debug
registers), but the features offered by the debug registers can vary. 
So can their globalness.  For instance, a platform which only placed
watchpoints on physical addresses instead of virtual addresses would
probably not offer thread-specific setting of the watchpoint registers.
[Do any platforms do that, and also support SMP?  I sure hope not.]

> > I would expect that they would always be
> > per-thread on any Linux target with true hardware watchpoints.  Of
> > course, they'll be process global if they're implemented by page
> > protections
> 
> The implementation that uses debug registers can be either global or
> local, depending on some bit in one of the registers.  One thing GDB
> should know about a platform is how that platform fiddles with that
> bit.

For the Linux kernel, it doesn't matter what you set in that bit.  It
has to clear the registers at task switch anyway, for security reasons.
I imagine this is true on most other protected, multi-tasking OS's
also; am I missing something?

-- 
Daniel Jacobowitz

Re: [RFA]: Modified Watchthreads Patch

[Eli Zaretskii]

> Date: Sat, 11 Dec 2004 12:37:57 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: kettenis@gnu.org, jjohnstn@redhat.com, gdb-patches@sources.redhat.com
> 
> For the Linux kernel, it doesn't matter what you set in that bit.  It
> has to clear the registers at task switch anyway, for security reasons.

This might mean that, in practice, we will need to have some GDB code
to produce an illusion of thread-specific watchpoints on the x86.
That is, the hardware will stop the inferior whenever _any_ thread
hits the watchtpoint, and then GDB will take control, and figure out
whether it needs to stop or continue.

Re: [RFA]: Modified Watchthreads Patch

[Daniel Jacobowitz]

On Sat, Dec 11, 2004 at 08:05:30PM +0200, Eli Zaretskii wrote:
> > Date: Sat, 11 Dec 2004 12:37:57 -0500
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: kettenis@gnu.org, jjohnstn@redhat.com, gdb-patches@sources.redhat.com
> > 
> > For the Linux kernel, it doesn't matter what you set in that bit.  It
> > has to clear the registers at task switch anyway, for security reasons.
> 
> This might mean that, in practice, we will need to have some GDB code
> to produce an illusion of thread-specific watchpoints on the x86.
> That is, the hardware will stop the inferior whenever _any_ thread
> hits the watchtpoint, and then GDB will take control, and figure out
> whether it needs to stop or continue.

Actually, I believe it means the opposite.  This is why the registers
are supposed to be specific to individual tasks.  Remember that in
Linux, a task is an LWP, not a "process" - a somewhat mythical beast
under Linux.  The hardware will stop the particular task that hits the
watchpoint; it's GDB that has to stop the other LWPs.

-- 
Daniel Jacobowitz

Re: [RFA]: Modified Watchthreads Patch

[Mark Kettenis]

   Date: Sat, 11 Dec 2004 11:36:52 -0500
   From: Daniel Jacobowitz <drow@false.org>

   > Adding hacks around hacks, like we've been doing to support threads on
   > Linux for quite some time now is defenitely not a good idea.

   Mark, would you please stop saying this?  I don't believe it to be true
   any more.  If you think it's still accurate, please point me at
   specific hacks around hacks, and let's see if we can get rid of them
   now.

Sorry Daniel, I know you've done some really good work with regard to
threads in the kernel.  I guess I'm still somewhat frustrated about
the situation back when I wrote the initial Linux LWP layer.  Back
then the attitude of the kernel developers was basically: "We won't
complicate the kernel with support for debuggers, solve everything
from userland!".

That said, there is just too much code in linux-nat.c.  If you compare
the code necessary to implement to_wait and to_resume that's there
with the amount of code in inf-ttrace.c, you see what I mean.  Most of
the code is present because we need to stop each thread individually
by sending it a SIGSTOP.  Things become so much simpler if the kernel
would provide an interface to stop them all in one go that doesn't
interfere with signal delivery...

   I admit there are some peculiarities related to stopping all threads.
   But most of them are related to very real situations that we want to be
   able to debug: two threads receiving a signal at the same time, hitting
   different breakpoints at the same time, et cetera.  Life with threads
   is just more complicated.  Some platforms do the complicated bits in
   the kernel, and Linux chose to expose an LWP-oriented interface rather
   than a whole-process oriented interface so we have to do the
   complicated bits in userspace.  That is not going to change, because
   the Linux design philosophy for threading is that they are just a
   special kind of process; Linux has no concept of "the whole process"
   and will not be adding one.  This has been discussed from time to time
   on the linux-kernel list.  [There is some correlation to the POSIX
   threading concept of a process, for the purpose of POSIX-compliant
   signal delivery, but that's the extent of it.]

I still think this is wrong.  The very fact that these proceses share
a virtual memory space means that they're grouped together.  The
kernel shouldn't deny that.  But even if folks don't want to support
freezing that memory space atomically (at least to the observer), we
really need a way to stop each process individually that doesn't
interfere with signal delivery.  I sincerely believe that we'll keep
seeing thread-related problems if it isn't possible to stop threads
while keeping all signals pending.

   And I'm busily (at work) improving platform support for NPTL; one of my
   goals is to someday rip all the LinuxThreads support code out of GDB. 
   But it's going to be a long time before that's a viable option - at
   least a couple of years from now.  That's no more friendly than
   dropping support for all but the newest kernel.  And we'll need some
   of that code to provide quality support for debugging multiple
   processes simultaneously, or to support debugging applications which
   use clone() directly.

Your work is very much appreciated.

Mark

Re: [RFA]: Modified Watchthreads Patch

[Daniel Jacobowitz]

On Sat, Dec 11, 2004 at 07:49:35PM +0100, Mark Kettenis wrote:
>    Date: Sat, 11 Dec 2004 11:36:52 -0500
>    From: Daniel Jacobowitz <drow@false.org>
> 
>    > Adding hacks around hacks, like we've been doing to support threads on
>    > Linux for quite some time now is defenitely not a good idea.
> 
>    Mark, would you please stop saying this?  I don't believe it to be true
>    any more.  If you think it's still accurate, please point me at
>    specific hacks around hacks, and let's see if we can get rid of them
>    now.
> 
> Sorry Daniel, I know you've done some really good work with regard to
> threads in the kernel.  I guess I'm still somewhat frustrated about
> the situation back when I wrote the initial Linux LWP layer.  Back
> then the attitude of the kernel developers was basically: "We won't
> complicate the kernel with support for debuggers, solve everything
> from userland!".

There's still some of that attitude, but when we can demonstrate the
value of a kernel change we can get it implemented.  That's how
fork/clone events came to be.

> That said, there is just too much code in linux-nat.c.  If you compare
> the code necessary to implement to_wait and to_resume that's there
> with the amount of code in inf-ttrace.c, you see what I mean.  Most of
> the code is present because we need to stop each thread individually
> by sending it a SIGSTOP.  Things become so much simpler if the kernel
> would provide an interface to stop them all in one go that doesn't
> interfere with signal delivery...

Unfortunately, I think this is basically impossible.  I spent several
weeks talking to Roland (who has a better head for the signal delivery
issues than I do - and who didn't think it was impossible, so maybe
he's right :-) about how to do this.

The problem is that many threads could be executing right now; there is
no way to stop them before any pending signals are delivered,
especially if they're running on other processors.

Comparing the code size in inf-ttrace.c and linux-nat.c is unfair. 
Remember what I said (below) about the different choice of models? 
There's probably as much or more code in the HP/UX kernel to stop
multiple threads and queue their (possibly multiple) pending events
for ttrace.  On HP/UX the code lives in the kernel where you can't see
it; on Linux it lives in GDB.  I consider fitting the nat code into
three times the size of inf-ttrace a triumph.  Of course, (A) it can be
slimmed down a lot (more below) and (B) inf-ttrace implements page
protection watchpoints also.

>    I admit there are some peculiarities related to stopping all threads.
>    But most of them are related to very real situations that we want to be
>    able to debug: two threads receiving a signal at the same time, hitting
>    different breakpoints at the same time, et cetera.  Life with threads
>    is just more complicated.  Some platforms do the complicated bits in
>    the kernel, and Linux chose to expose an LWP-oriented interface rather
>    than a whole-process oriented interface so we have to do the
>    complicated bits in userspace.  That is not going to change, because
>    the Linux design philosophy for threading is that they are just a
>    special kind of process; Linux has no concept of "the whole process"
>    and will not be adding one.  This has been discussed from time to time
>    on the linux-kernel list.  [There is some correlation to the POSIX
>    threading concept of a process, for the purpose of POSIX-compliant
>    signal delivery, but that's the extent of it.]
> 
> I still think this is wrong.  The very fact that these proceses share
> a virtual memory space means that they're grouped together.  The
> kernel shouldn't deny that.  But even if folks don't want to support
> freezing that memory space atomically (at least to the observer), we
> really need a way to stop each process individually that doesn't
> interfere with signal delivery.  I sincerely believe that we'll keep
> seeing thread-related problems if it isn't possible to stop threads
> while keeping all signals pending.

This is less difficult.  The biggest wart in linux-nat, IMO, is the
code to backtrack off of breakpoints and signals and arrange to re-hit
them later; I intend to rip it out in the next couple of months. 
Gdbserver is an existence proof that it isn't necessary.

After that, I might investigate a PTRACE_STOP that stops a single
thread without queueing a SIGSTOP; however, I have looked at this
before, and there's just no good way to do it inside the kernel. I
don't agree that it's necessary, though.  It might be better to send a
gdb-chosen real-time signal to the process, because of the RT signal
properties of queueing and not displacing signals sent by other
processes using the same number.  It doesn't need to stop on our
signal; just on some signal.

Remember, the "pending" signal may already have been delivered by the
time the observer asks to stop it.  Making the kernel turn back time
would be a bit of a trick!

-- 
Daniel Jacobowitz

Re: [RFA]: Modified Watchthreads Patch

[Eli Zaretskii]

> Date: Sat, 11 Dec 2004 15:33:52 +0100 (CET)
> From: Mark Kettenis <kettenis@gnu.org>
> CC: drow@false.org, jjohnstn@redhat.com, gdb-patches@sources.redhat.com
> 
> Personally I think that it's better to declare watchpoints in
> multi-threaded programs as unsupported.  Then add a sane interface for
> debugging threads and watchpoints to the kernel, before revisiting the
> issue in GDB.  I mean, it's like the Linux kernel is no longer Open
> Source.

I wasn't aware the situation was that bad, but if it is, I cannot
agree more.

>    In addition, proliferation of observers' use will sooner or later
>    raise the issue of the order of the observer invocation, since we lack
>    a machinery for invoking a series of observers in a controlled manner:
>    we cannot control the order of their invocation and we cannot tell GDB
>    to stop invoking any additional observers.  The current machinery
>    assumes that each observer is orthogonal to others in its side
>    effects; what if this assumption doesn't hold?
> 
> I think we should take a different viewpoint here.  The current
> machinery doesn't *assume* that each observer is orthogonal, it's the
> *definition* of the interface.

First, this definition should be spelled out in the documentation.
But even if it is, it's dangerous to rely on programmers working
independently to never step on each other's feet.

Re: [RFA]: Modified Watchthreads Patch

[Daniel Jacobowitz]

On Sat, Dec 11, 2004 at 01:19:03PM +0200, Eli Zaretskii wrote:
> > Date: Fri, 10 Dec 2004 18:37:00 -0500
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: jjohnstn@redhat.com, gdb-patches@sources.redhat.com
> > 
> > I waited to review the revised version until you had a chance to
> > comment on the continued use of observers.
> 
> I still object to the use of observers for a purpose such as this one.
> My objections are a bit philosophical, though.

That's a fine kind of objection :-)

> Basically, I think that observers are a last-resort mechanism for
> anything that is part of the GDB infrastructure.  It's like hooks or
> callbacks--you don't normally expect a program internals to use
> callbacks that it provides for higher-level application code.
> 
> Put another way, using a mechanism such as observers for internal code
> means we leave our internal structure not entirely defined.  We design
> the internals, so we ought to know what needs to be done where and
> when.  For example, this particular usage of an observer means that we
> don't really know in advance that watchpoint insertion needs to be
> done for each thread when it is being attached.  Do we really want to
> say that we don't know what we are doing in our own program?

As Mark said, the idea is that GDB is modular.  If we knew that every
time the GNU/Linux native support code was used, the architecture
native support code would need to munge watchpoints in this way, and
there were few other platforms for which it was true, then using an
observer would be silly.  But we aren't supposed to "know" what
architecture is in use when we compile linux-nat.c, and this action is
a property of the architecture.

Are there really any current uses of observers which meet your
definition above?

That said, it looks like all Linux targets (with the exception of the
current x86 mystery) have this behavior.  That gives me at least two
more ideas on how to implement it.

1)  Wait for my target vector inheritance patch to go in.  Have the
target override either to_wait or to_resume - probably to_resume.  In
the overridden version, iterate over all LWPs and make sure
watchpoints are correctly inserted for them all.  Disadvantage: we
shouldn't need to iterate over the entire LWP list for this.  But there
are enough places in GDB that don't scale easily to huge LWP lists that
I can't imagine this one being a problem in the next ten years.

2) Provide a GNU/Linux specific hook, not using the observer mechanism,
in the same way we've been connecting architectures to other individual
modules of GDB.  Implement linux_set_new_thread_watchpoints_callback,
which would be functionally similar to this observer, but have a better
defined purpose and use.

Are either of these better?

> In addition, proliferation of observers' use will sooner or later
> raise the issue of the order of the observer invocation, since we lack
> a machinery for invoking a series of observers in a controlled manner:
> we cannot control the order of their invocation and we cannot tell GDB
> to stop invoking any additional observers.  The current machinery
> assumes that each observer is orthogonal to others in its side
> effects; what if this assumption doesn't hold?

Yes, this is a very real problem.

-- 
Daniel Jacobowitz

Re: [RFA]: Modified Watchthreads Patch

[Eli Zaretskii]

> Date: Sat, 11 Dec 2004 11:11:37 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: jjohnstn@redhat.com, gdb-patches@sources.redhat.com
> 
> Are there really any current uses of observers which meet your
> definition above?

I'm unsure which definition you refer to.

> 1)  Wait for my target vector inheritance patch to go in.  Have the
> target override either to_wait or to_resume - probably to_resume.  In
> the overridden version, iterate over all LWPs and make sure
> watchpoints are correctly inserted for them all.  Disadvantage: we
> shouldn't need to iterate over the entire LWP list for this.  But there
> are enough places in GDB that don't scale easily to huge LWP lists that
> I can't imagine this one being a problem in the next ten years.
> 
> 2) Provide a GNU/Linux specific hook, not using the observer mechanism,
> in the same way we've been connecting architectures to other individual
> modules of GDB.  Implement linux_set_new_thread_watchpoints_callback,
> which would be functionally similar to this observer, but have a better
> defined purpose and use.
> 
> Are either of these better?

Either one of them is better.

Re: [RFA]: Modified Watchthreads Patch

[Daniel Jacobowitz]

On Sat, Dec 11, 2004 at 06:54:53PM +0200, Eli Zaretskii wrote:
> > Date: Sat, 11 Dec 2004 11:11:37 -0500
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: jjohnstn@redhat.com, gdb-patches@sources.redhat.com
> > 
> > Are there really any current uses of observers which meet your
> > definition above?
> 
> I'm unsure which definition you refer to.

Let me try to clarify then... this is what you said:

> Basically, I think that observers are a last-resort mechanism for
> anything that is part of the GDB infrastructure.  It's like hooks or
> callbacks--you don't normally expect a program internals to use
> callbacks that it provides for higher-level application code.
>
> Put another way, using a mechanism such as observers for internal code
> means we leave our internal structure not entirely defined.  We design
> the internals, so we ought to know what needs to be done where and
> when.  For example, this particular usage of an observer means that we
> don't really know in advance that watchpoint insertion needs to be
> done for each thread when it is being attached.  Do we really want to
> say that we don't know what we are doing in our own program?

I think that every current use of observers is in this sense "we don't
really know in advance what needs to be done".  For instance, we've got
observer_notify_inferior_created, which is uesd for actions that we
don't know statically will be necessary at inferior creation - vsyscall
DSO loading on targets which have one, and some HP/UX specific code
that I don't recall the purpose of.

Or consider target_changed, which is attached by the frame code (always
part of GDB!) and the regcache (likewise!) and notified by valops.c
(likewise!).

I think this is a fine use of observers; one "module" of GDB wants to
be notified when an event occurs in another.

> > 1)  Wait for my target vector inheritance patch to go in.  Have the
> > target override either to_wait or to_resume - probably to_resume.  In
> > the overridden version, iterate over all LWPs and make sure
> > watchpoints are correctly inserted for them all.  Disadvantage: we
> > shouldn't need to iterate over the entire LWP list for this.  But there
> > are enough places in GDB that don't scale easily to huge LWP lists that
> > I can't imagine this one being a problem in the next ten years.
> > 
> > 2) Provide a GNU/Linux specific hook, not using the observer mechanism,
> > in the same way we've been connecting architectures to other individual
> > modules of GDB.  Implement linux_set_new_thread_watchpoints_callback,
> > which would be functionally similar to this observer, but have a better
> > defined purpose and use.
> > 
> > Are either of these better?
> 
> Either one of them is better.

Great!  Jeff, Mark, do you have opinions on either (or other
suggestions)?

Observe, we're back to the core question of the role of observers here.
I prefer #2 to #1.  But #2 is _functionally_ equivalent to providing an
observer named linux_enable_watchpoints_for_new_threads.  In one case
it would have to be documented in observers.texi and support functions
would be autogenerated; in the other case it would probably be
documented in comments, and bunch of support functions would have to be
written by hand, instead of being generated by the observers shell script.

-- 
Daniel Jacobowitz

Re: [RFA]: Modified Watchthreads Patch

[Eli Zaretskii]

> Date: Sat, 11 Dec 2004 12:32:56 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: jjohnstn@redhat.com, gdb-patches@sources.redhat.com
> 
> I think that every current use of observers is in this sense "we don't
> really know in advance what needs to be done".  For instance, we've got
> observer_notify_inferior_created, which is uesd for actions that we
> don't know statically will be necessary at inferior creation - vsyscall
> DSO loading on targets which have one, and some HP/UX specific code
> that I don't recall the purpose of.
> 
> Or consider target_changed, which is attached by the frame code (always
> part of GDB!) and the regcache (likewise!) and notified by valops.c
> (likewise!).

What about solib_unloaded?

> Observe, we're back to the core question of the role of observers here.
> I prefer #2 to #1.  But #2 is _functionally_ equivalent to providing an
> observer named linux_enable_watchpoints_for_new_threads.

It is functionally equivalent, but ideologically different: it's a
detail of GDB internals as opposed to a general-purpose extension
mechanism.

As for its documentation, it boils down to a couple of sentences, so I
don't think it's a big deal.

Re: [RFA]: Modified Watchthreads Patch

[Daniel Jacobowitz]

On Sat, Dec 11, 2004 at 08:00:54PM +0200, Eli Zaretskii wrote:
> > Date: Sat, 11 Dec 2004 12:32:56 -0500
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: jjohnstn@redhat.com, gdb-patches@sources.redhat.com
> > 
> > I think that every current use of observers is in this sense "we don't
> > really know in advance what needs to be done".  For instance, we've got
> > observer_notify_inferior_created, which is uesd for actions that we
> > don't know statically will be necessary at inferior creation - vsyscall
> > DSO loading on targets which have one, and some HP/UX specific code
> > that I don't recall the purpose of.
> > 
> > Or consider target_changed, which is attached by the frame code (always
> > part of GDB!) and the regcache (likewise!) and notified by valops.c
> > (likewise!).
> 
> What about solib_unloaded?

Same deal, almost.  It's used by the common breakpoint code.  It's
notified by the "common" solib code - it's not actually common, but
we're trying to make it so.

> > Observe, we're back to the core question of the role of observers here.
> > I prefer #2 to #1.  But #2 is _functionally_ equivalent to providing an
> > observer named linux_enable_watchpoints_for_new_threads.
> 
> It is functionally equivalent, but ideologically different: it's a
> detail of GDB internals as opposed to a general-purpose extension
> mechanism.

I do not consider GDB's observer mechanism to be a general-purpose
extension mechanism.  Some day, if there is a revival of libgdb, then
maybe it will be usable as one - and at that point, perhaps we'll need
a second copy with different events.  I think the current observers are
a mechanism for nominally independent internal modules of GDB to
communicate.

Are "breakpoint" and "shared library" any more independent than
"common Linux support" and "some particular Linux backend"?  Maybe so,
but the line seems pretty shaky to me.  Certainly the breakpoint module
has a lot of code to handle shared libraries, and the shared library
code makes use of breakpoints.

-- 
Daniel Jacobowitz

Re: [RFA]: Modified Watchthreads Patch

[Jeff Johnston]

Daniel Jacobowitz wrote:
> On Sat, Dec 11, 2004 at 06:54:53PM +0200, Eli Zaretskii wrote:
> 
>>>Date: Sat, 11 Dec 2004 11:11:37 -0500
>>>From: Daniel Jacobowitz <drow@false.org>
>>>Cc: jjohnstn@redhat.com, gdb-patches@sources.redhat.com
>>>
>>>Are there really any current uses of observers which meet your
>>>definition above?
>>
>>I'm unsure which definition you refer to.
> 
> 
> Let me try to clarify then... this is what you said:
> 
> 
>>Basically, I think that observers are a last-resort mechanism for
>>anything that is part of the GDB infrastructure.  It's like hooks or
>>callbacks--you don't normally expect a program internals to use
>>callbacks that it provides for higher-level application code.
>>
>>Put another way, using a mechanism such as observers for internal code
>>means we leave our internal structure not entirely defined.  We design
>>the internals, so we ought to know what needs to be done where and
>>when.  For example, this particular usage of an observer means that we
>>don't really know in advance that watchpoint insertion needs to be
>>done for each thread when it is being attached.  Do we really want to
>>say that we don't know what we are doing in our own program?
> 
> 
> I think that every current use of observers is in this sense "we don't
> really know in advance what needs to be done".  For instance, we've got
> observer_notify_inferior_created, which is uesd for actions that we
> don't know statically will be necessary at inferior creation - vsyscall
> DSO loading on targets which have one, and some HP/UX specific code
> that I don't recall the purpose of.
> 
> Or consider target_changed, which is attached by the frame code (always
> part of GDB!) and the regcache (likewise!) and notified by valops.c
> (likewise!).
> 
> I think this is a fine use of observers; one "module" of GDB wants to
> be notified when an event occurs in another.
> 
> 
>>>1)  Wait for my target vector inheritance patch to go in.  Have the
>>>target override either to_wait or to_resume - probably to_resume.  In
>>>the overridden version, iterate over all LWPs and make sure
>>>watchpoints are correctly inserted for them all.  Disadvantage: we
>>>shouldn't need to iterate over the entire LWP list for this.  But there
>>>are enough places in GDB that don't scale easily to huge LWP lists that
>>>I can't imagine this one being a problem in the next ten years.
>>>
>>>2) Provide a GNU/Linux specific hook, not using the observer mechanism,
>>>in the same way we've been connecting architectures to other individual
>>>modules of GDB.  Implement linux_set_new_thread_watchpoints_callback,
>>>which would be functionally similar to this observer, but have a better
>>>defined purpose and use.
>>>
>>>Are either of these better?
>>
>>Either one of them is better.
> 
> 
> Great!  Jeff, Mark, do you have opinions on either (or other
> suggestions)?
> 
> Observe, we're back to the core question of the role of observers here.
> I prefer #2 to #1.  But #2 is _functionally_ equivalent to providing an
> observer named linux_enable_watchpoints_for_new_threads.  In one case
> it would have to be documented in observers.texi and support functions
> would be autogenerated; in the other case it would probably be
> documented in comments, and bunch of support functions would have to be
> written by hand, instead of being generated by the observers shell script.
> 

Sorry, I should have responded to this ages ago.  I prefer #2.  I assume the 
hook resides in the target vector or have you got some other idea in mind?

-- Jeff J.

Re: [RFA]: Modified Watchthreads Patch

[Daniel Jacobowitz]

On Thu, Jan 13, 2005 at 02:22:45PM -0500, Jeff Johnston wrote:
> Daniel Jacobowitz wrote:
> >On Sat, Dec 11, 2004 at 06:54:53PM +0200, Eli Zaretskii wrote:
> >
> >>>Date: Sat, 11 Dec 2004 11:11:37 -0500
> >>>From: Daniel Jacobowitz <drow@false.org>
> >>>Cc: jjohnstn@redhat.com, gdb-patches@sources.redhat.com
> >>>
> >>>Are there really any current uses of observers which meet your
> >>>definition above?
> >>
> >>I'm unsure which definition you refer to.
> >
> >
> >Let me try to clarify then... this is what you said:
> >
> >
> >>Basically, I think that observers are a last-resort mechanism for
> >>anything that is part of the GDB infrastructure.  It's like hooks or
> >>callbacks--you don't normally expect a program internals to use
> >>callbacks that it provides for higher-level application code.
> >>
> >>Put another way, using a mechanism such as observers for internal code
> >>means we leave our internal structure not entirely defined.  We design
> >>the internals, so we ought to know what needs to be done where and
> >>when.  For example, this particular usage of an observer means that we
> >>don't really know in advance that watchpoint insertion needs to be
> >>done for each thread when it is being attached.  Do we really want to
> >>say that we don't know what we are doing in our own program?
> >
> >
> >I think that every current use of observers is in this sense "we don't
> >really know in advance what needs to be done".  For instance, we've got
> >observer_notify_inferior_created, which is uesd for actions that we
> >don't know statically will be necessary at inferior creation - vsyscall
> >DSO loading on targets which have one, and some HP/UX specific code
> >that I don't recall the purpose of.
> >
> >Or consider target_changed, which is attached by the frame code (always
> >part of GDB!) and the regcache (likewise!) and notified by valops.c
> >(likewise!).
> >
> >I think this is a fine use of observers; one "module" of GDB wants to
> >be notified when an event occurs in another.
> >
> >
> >>>1)  Wait for my target vector inheritance patch to go in.  Have the
> >>>target override either to_wait or to_resume - probably to_resume.  In
> >>>the overridden version, iterate over all LWPs and make sure
> >>>watchpoints are correctly inserted for them all.  Disadvantage: we
> >>>shouldn't need to iterate over the entire LWP list for this.  But there
> >>>are enough places in GDB that don't scale easily to huge LWP lists that
> >>>I can't imagine this one being a problem in the next ten years.
> >>>
> >>>2) Provide a GNU/Linux specific hook, not using the observer mechanism,
> >>>in the same way we've been connecting architectures to other individual
> >>>modules of GDB.  Implement linux_set_new_thread_watchpoints_callback,
> >>>which would be functionally similar to this observer, but have a better
> >>>defined purpose and use.
> >>>
> >>>Are either of these better?
> >>
> >>Either one of them is better.
> >
> >
> >Great!  Jeff, Mark, do you have opinions on either (or other
> >suggestions)?
> >
> >Observe, we're back to the core question of the role of observers here.
> >I prefer #2 to #1.  But #2 is _functionally_ equivalent to providing an
> >observer named linux_enable_watchpoints_for_new_threads.  In one case
> >it would have to be documented in observers.texi and support functions
> >would be autogenerated; in the other case it would probably be
> >documented in comments, and bunch of support functions would have to be
> >written by hand, instead of being generated by the observers shell script.
> >
> 
> Sorry, I should have responded to this ages ago.  I prefer #2.  I assume 
> the hook resides in the target vector or have you got some other idea in 
> mind?

I believe I was waiting for further feedback from Eli on the role of
observers within GDB.  That's why I never got back to you.  Sorry.

No, it would not reside in the target vector.  I had something like
dwarf2_frame_set_signal_frame_p in mind.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFA]: Modified Watchthreads Patch

[Eli Zaretskii]

> Date: Thu, 10 Feb 2005 14:58:39 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sources.redhat.com
> 
> > >>>1)  Wait for my target vector inheritance patch to go in.  Have the
> > >>>target override either to_wait or to_resume - probably to_resume.  In
> > >>>the overridden version, iterate over all LWPs and make sure
> > >>>watchpoints are correctly inserted for them all.  Disadvantage: we
> > >>>shouldn't need to iterate over the entire LWP list for this.  But there
> > >>>are enough places in GDB that don't scale easily to huge LWP lists that
> > >>>I can't imagine this one being a problem in the next ten years.
> > >>>
> > >>>2) Provide a GNU/Linux specific hook, not using the observer mechanism,
> > >>>in the same way we've been connecting architectures to other individual
> > >>>modules of GDB.  Implement linux_set_new_thread_watchpoints_callback,
> > >>>which would be functionally similar to this observer, but have a better
> > >>>defined purpose and use.
> > >>>
> > >>>Are either of these better?
> > >>
> > >>Either one of them is better.
> > >
> > >
> > >Great!  Jeff, Mark, do you have opinions on either (or other
> > >suggestions)?
> > >
> > >Observe, we're back to the core question of the role of observers here.
> > >I prefer #2 to #1.  But #2 is _functionally_ equivalent to providing an
> > >observer named linux_enable_watchpoints_for_new_threads.  In one case
> > >it would have to be documented in observers.texi and support functions
> > >would be autogenerated; in the other case it would probably be
> > >documented in comments, and bunch of support functions would have to be
> > >written by hand, instead of being generated by the observers shell script.
> > >
> > 
> > Sorry, I should have responded to this ages ago.  I prefer #2.  I assume 
> > the hook resides in the target vector or have you got some other idea in 
> > mind?
> 
> I believe I was waiting for further feedback from Eli on the role of
> observers within GDB.

Perhaps I misunderstood, but the above 2 alternatives don't use
observers.  And since these are Linux-specific issues, I left it to
Daniel and you to select the best alternative.

In other words, I don't think you need any input from me to decide how
to solve this.  Am I missing something?

Re: [RFA]: Modified Watchthreads Patch

[Daniel Jacobowitz]

On Fri, Feb 11, 2005 at 07:03:20PM +0200, Eli Zaretskii wrote:
> Perhaps I misunderstood, but the above 2 alternatives don't use
> observers.  And since these are Linux-specific issues, I left it to
> Daniel and you to select the best alternative.
> 
> In other words, I don't think you need any input from me to decide how
> to solve this.  Am I missing something?

No, you aren't.  For this problem, we're set.  However, I'd like to
discuss the design principle for a bit, so that we know how to approach
this problem in the future.  I found the message I was thinking of.  I
wrote:

  <quote>

  > > Observe, we're back to the core question of the role of observers here.
  > > I prefer #2 to #1.  But #2 is _functionally_ equivalent to providing an
  > > observer named linux_enable_watchpoints_for_new_threads.
  >
  > It is functionally equivalent, but ideologically different: it's a
  > detail of GDB internals as opposed to a general-purpose extension
  > mechanism.

  I do not consider GDB's observer mechanism to be a general-purpose
  extension mechanism.  Some day, if there is a revival of libgdb, then
  maybe it will be usable as one - and at that point, perhaps we'll need
  a second copy with different events.  I think the current observers are
  a mechanism for nominally independent internal modules of GDB to
  communicate.

  Are "breakpoint" and "shared library" any more independent than
  "common Linux support" and "some particular Linux backend"?  Maybe so,
  but the line seems pretty shaky to me.  Certainly the breakpoint module
  has a lot of code to handle shared libraries, and the shared library
  code makes use of breakpoints.

  </quote>

My current feeling is that either:
  (A) we should use the same mechanism to create two sets of hooks,
      one to be considered a general extension mechanism and the other
      an internal modularity aid;
or
  (B) we should define the observers as the latter rather than the
      former, and declare the former niche unfilled.

Otherwise there's just a lot of wasted code duplication.

What do you think?  Am I way off on the role of observers?  Since we
don't "support" plugging arbitrary code into them, I have trouble
considering them an extension mechanism.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFA]: Modified Watchthreads Patch

[Eli Zaretskii]

> Date: Fri, 11 Feb 2005 12:17:51 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: jjohnstn@redhat.com, gdb-patches@sources.redhat.com
> 
> My current feeling is that either:
>   (A) we should use the same mechanism to create two sets of hooks,
>       one to be considered a general extension mechanism and the other
>       an internal modularity aid;
> or
>   (B) we should define the observers as the latter rather than the
>       former, and declare the former niche unfilled.
> 
> Otherwise there's just a lot of wasted code duplication.
> 
> What do you think?  Am I way off on the role of observers?  Since we
> don't "support" plugging arbitrary code into them, I have trouble
> considering them an extension mechanism.

I initially thought that observers were introduced as an extension
mechanism.  But it begins to look like they are intended to be used as
in the C++ observer paradigm.  I don't like that, since GDB is not a
C++ program, and because using observers in GDB obfuscates the flow of
control and data and makes the GDB code harder to understand.  But it
looks like mine is the minority opinion, judging by past discussions
of this.

Re: [RFA]: Modified Watchthreads Patch

[Ulrich Weigand]

Daniel Jacobowitz wrote:

>Please fix the whitespace at the end of s390-nat.c.  Otherwise, this is
>approved if Ulrich is OK with the S390 bits; let's give him a chance to
>comment.

The s390 changes are fine with me, thanks!

I'd only ask to remove the bits relating to s390 in the comment
preceding insert_watchpoints_for_new_thread; they are no longer
accurate.


Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

--
  Dr. Ulrich Weigand
  Linux for S/390 Design & Development
  IBM Deutschland Entwicklung GmbH, Schoenaicher Str. 220, 71032 Boeblingen
  Phone: +49-7031/16-3727   ---   Email: Ulrich.Weigand@de.ibm.com