[RFA]: Watchpoints per thread patch

[RFA]: Watchpoints per thread patch

[Jeff Johnston]

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

The following patch adds needed support for the ia64 and s390 platforms.  For 
these platforms, watchpoints need to be inserted / removed on each thread so as 
to work across all threads.  The patch adds support for detecting this at 
configuration time and setting a new flag WATCHPOINTS_PER_THREAD.  This flag is 
used when inserting/removing watchpoints and when a new thread event occurs.

This patch in itself does not give these platforms threaded watchpoint support 
to pass the watchthreads.exp test case, but I am breaking up my bigger patch 
that works for x86, x86_64, and ia64.  It still fails on the S390 but gets part 
marks because threaded watchpoints actually trigger properly however the S390 
cannot determine which watchpoint gets triggered when multiple watchpoint events 
occur. On ia64, the watchthreads.exp test case will fail differently than before 
because the low-level register and watchpoint code does not properly calculate 
the LWP for accessing registers so we end up eating through the maximum number 
of watchpoints quicker than anticipated.  I have a subsequent patch for getting 
the LWP reliably, but this subsequent patch may be made unnecessary depending on 
what Daniel does with ptids and the thread-db layer.  Regardless of Daniel's 
redesign, the watchpoints still must be inserted/removed on each thread.

Ok to commit?

-- Jeff J.

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

         * configure.host: For ia64 and s390 platforms, support watchpoints
         per thread.
         * configure.in: Set WATCHPOINTS_PER_THREAD flag if platform
         has watchpoints per thread.
	* config.in: Regenerated.
	* configure: Ditto.
         * thread.c (restore_current_thread): Add new silent flag argument.
         Do not issue message if switching threads and the silent flag is
         set.  Change all existing callers to call with new argument.
         Add call to find_thread_pid to safeguard against accessing an
         incomplete or invalid thread list item.
         (thread_switch_and_call): New function.
         * breakpoint.h: Define incomplete struct thread_info type.
         (insert_watchpoints_for_new_thread): New prototype.
         * breakpoint.c (insert_watchpoints_for_new_thread): New function.
         (insert_one_watchpoint): Ditto.
         (insert_watchpoint): Ditto.
         (remove_one_watchpoint): Ditto.
         (remove_watchpoint): Ditto.
         (remove_breakpoint): Call remove_watchpoint instead of
         target_remove_watchpoint.
         (insert_bp_location): Call insert_watchpoint instead of
         target_insert_watchpoint.
         (print_it_typical): Support watchpoints occurring at the same
         time as thread events.
         * thread-db.c (attach_thread)[WATCHPOINTS_PER_THREAD]: Call
         insert_watchpoints_for_new_thread.


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

Index: configure.host
===================================================================
RCS file: /cvs/src/src/gdb/configure.host,v
retrieving revision 1.84
diff -u -p -r1.84 configure.host
--- configure.host	1 Sep 2004 20:46:41 -0000	1.84
+++ configure.host	19 Oct 2004 22:59:47 -0000
@@ -179,3 +179,17 @@ hppa*-*-linux*)
 	gdb_host_long_double_format=0
 	;;
 esac
+
+# Set various watchpoint flags based on host/cpu
+
+case "${host}" in
+ia64-*-*)
+	gdb_host_watchpoints_per_thread=1
+	;;
+s390*-*-*)
+	gdb_host_watchpoints_per_thread=1
+	;;
+*)
+	gdb_host_watchpoints_per_thread=
+	;;
+esac
Index: configure.in
===================================================================
RCS file: /cvs/src/src/gdb/configure.in,v
retrieving revision 1.176
diff -u -p -r1.176 configure.in
--- configure.in	10 Oct 2004 15:55:49 -0000	1.176
+++ configure.in	19 Oct 2004 22:59:47 -0000
@@ -1414,6 +1414,11 @@ AC_DEFINE_UNQUOTED(GDB_HOST_FLOAT_FORMAT
 AC_DEFINE_UNQUOTED(GDB_HOST_DOUBLE_FORMAT,$gdb_host_double_format,[Host double floatformat])
 AC_DEFINE_UNQUOTED(GDB_HOST_LONG_DOUBLE_FORMAT,$gdb_host_long_double_format,[Host long double floatformat])
 
+# List of host watchpoint flags.
+if test "x$gdb_host_watchpoints_per_thread" != "x"; then
+    AC_DEFINE_UNQUOTED(WATCHPOINTS_PER_THREAD,$gdb_host_watchpoints_per_thread,[Host watchpoints are thread-specific])
+fi
+
 # target_subdir is used by the testsuite to find the target libraries.
 target_subdir=
 if test "${host}" != "${target}"; then
Index: thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.38
diff -u -p -r1.38 thread.c
--- thread.c	25 Aug 2004 15:18:05 -0000	1.38
+++ thread.c	19 Oct 2004 22:59:47 -0000
@@ -61,7 +61,7 @@ static void thread_apply_all_command (ch
 static int thread_alive (struct thread_info *);
 static void info_threads_command (char *, int);
 static void thread_apply_command (char *, int);
-static void restore_current_thread (ptid_t);
+static void restore_current_thread (ptid_t, int);
 static void switch_to_thread (ptid_t ptid);
 static void prune_threads (void);
 
@@ -468,34 +468,38 @@ switch_to_thread (ptid_t ptid)
 }
 
 static void
-restore_current_thread (ptid_t ptid)
+restore_current_thread (ptid_t ptid, int silent)
 {
-  if (!ptid_equal (ptid, inferior_ptid))
+  if (!ptid_equal (ptid, inferior_ptid) &&
+      find_thread_pid (ptid))
     {
       switch_to_thread (ptid);
-      print_stack_frame (get_current_frame (), 1, SRC_LINE);
+      if (!silent)
+	print_stack_frame (get_current_frame (), 1, SRC_LINE);
     }
 }
 
 struct current_thread_cleanup
 {
   ptid_t inferior_ptid;
+  int silent;
 };
 
 static void
 do_restore_current_thread_cleanup (void *arg)
 {
   struct current_thread_cleanup *old = arg;
-  restore_current_thread (old->inferior_ptid);
+  restore_current_thread (old->inferior_ptid, old->silent);
   xfree (old);
 }
 
 static struct cleanup *
-make_cleanup_restore_current_thread (ptid_t inferior_ptid)
+make_cleanup_restore_current_thread (ptid_t inferior_ptid, int silent)
 {
   struct current_thread_cleanup *old
     = xmalloc (sizeof (struct current_thread_cleanup));
   old->inferior_ptid = inferior_ptid;
+  old->silent = silent;
   return make_cleanup (do_restore_current_thread_cleanup, old);
 }
 
@@ -519,7 +523,7 @@ thread_apply_all_command (char *cmd, int
   if (cmd == NULL || *cmd == '\000')
     error ("Please specify a command following the thread ID list");
 
-  old_chain = make_cleanup_restore_current_thread (inferior_ptid);
+  old_chain = make_cleanup_restore_current_thread (inferior_ptid, 0);
 
   /* It is safe to update the thread list now, before
      traversing it for "thread apply all".  MVS */
@@ -560,7 +564,7 @@ thread_apply_command (char *tidlist, int
   if (*cmd == '\000')
     error ("Please specify a command following the thread ID list");
 
-  old_chain = make_cleanup_restore_current_thread (inferior_ptid);
+  old_chain = make_cleanup_restore_current_thread (inferior_ptid, 0);
 
   /* Save a copy of the command in case it is clobbered by
      execute_command */
@@ -616,6 +620,47 @@ thread_apply_command (char *tidlist, int
   do_cleanups (old_chain);
 }
 
+int
+thread_switch_and_call (int thread_num, int (*callback) (void *), void *args)
+{
+  struct thread_info *tp;
+  struct cleanup *old_chain;
+  int rc = -1;
+
+  old_chain = make_cleanup_restore_current_thread (inferior_ptid, 1);
+
+  if (thread_num == -1)
+    {
+      if (thread_list != NULL)
+	{
+	  for (tp = thread_list; tp; tp = tp->next)
+	    {
+	      if (thread_alive (tp))
+		{
+		  switch_to_thread (tp->ptid);
+		  rc = (*callback) (args);
+		  if (rc)
+		    break;
+		}
+	    }
+	}
+      else
+	rc = (*callback) (args);
+    }
+  else
+    {
+      for (tp = thread_list; tp; tp = tp->next)
+	if (tp->num == thread_num && thread_alive (tp))
+	  {
+	    switch_to_thread (tp->ptid);
+	    rc = (*callback) (args);
+	  }
+    }
+
+  do_cleanups (old_chain);
+  return rc;
+}
+
 /* Switch to the specified thread.  Will dispatch off to thread_apply_command
    if prefix of arg is `apply'.  */
 
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	19 Oct 2004 22:59:47 -0000
@@ -30,6 +30,7 @@
 
 struct value;
 struct block;
+struct thread_info;
 
 /* This is the maximum number of bytes a breakpoint instruction can take.
    Feel free to increase it.  It's just used in a few places to size
@@ -662,6 +663,7 @@ extern void rwatch_command_wrapper (char
 extern void tbreak_command (char *, int);
 
 extern int insert_breakpoints (void);
+extern int insert_watchpoints_for_new_thread (struct thread_info *);
 
 extern int remove_breakpoints (void);
 
Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.183
diff -u -p -r1.183 breakpoint.c
--- breakpoint.c	8 Oct 2004 17:30:46 -0000	1.183
+++ breakpoint.c	19 Oct 2004 22:59:48 -0000
@@ -748,6 +748,124 @@ insert_catchpoint (struct ui_out *uo, vo
   return 0;
 }
 
+struct target_watchpoint_args
+{
+  CORE_ADDR addr;
+  int len;
+  int type;
+};
+
+static int
+insert_one_watchpoint (void *data)
+{
+  struct target_watchpoint_args *args = (struct target_watchpoint_args *)data;
+
+  return target_insert_watchpoint (args->addr, args->len, args->type);
+}
+
+static int
+insert_watchpoint (struct bp_location *bpt, CORE_ADDR addr, int len, int type)
+{
+  int val;
+  struct target_watchpoint_args args;
+
+  args.addr = addr;
+  args.len = len;
+  args.type = type;
+
+  /* If watchpoints do not apply to all threads automatically, we have to insert
+     and delete them for every thread.  Otherwise, we can insert or delete them
+     once from any thread.  */
+#ifdef WATCHPOINTS_PER_THREAD
+  val = thread_switch_and_call (bpt->owner->thread, &insert_one_watchpoint, &args);
+#else
+  val = insert_one_watchpoint (&args);
+#endif
+
+  return val;
+}
+
+int
+insert_watchpoints_for_new_thread (struct thread_info *ti)
+{
+  struct bp_location *b;
+  int val = 0;
+  int return_val = 0;
+  struct ui_file *tmp_error_stream = mem_fileopen ();
+  make_cleanup_ui_file_delete (tmp_error_stream);
+
+  /* Explicitly mark the warning -- this will only be printed if
+     there was an error.  */
+  fprintf_unfiltered (tmp_error_stream, "Warning:\n");
+
+  ALL_BP_LOCATIONS (b)
+    {
+      /* Skip disabled breakpoints.  */
+      if (!breakpoint_enabled (b->owner))
+	continue;
+
+      /* For every active watchpoint, we need to insert the watchpoint on the new
+	 thread.  */
+      if ((b->loc_type == bp_loc_hardware_watchpoint
+	   || b->owner->type == bp_watchpoint))
+	{
+	  struct value *v = b->owner->val_chain;
+
+	  /* Look at each value on the value chain.  */
+	  for (; v; v = v->next)
+	    {
+	      /* If it's a memory location, and GDB actually needed
+		 its contents to evaluate the expression, then we
+		 must watch it.  */
+	      if (VALUE_LVAL (v) == lval_memory
+		  && ! VALUE_LAZY (v))
+		{
+		  struct type *vtype = check_typedef (VALUE_TYPE (v));
+		  
+		  /* We only watch structs and arrays if user asked
+		     for it explicitly, never if they just happen to
+		     appear in the middle of some value chain.  */
+		  if (v == b->owner->val_chain
+		      || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
+			  && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
+		    {
+		      CORE_ADDR addr;
+		      int len, type;
+		      struct target_watchpoint_args args;
+		      
+		      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;
+		      args.addr = addr;
+		      args.len = len;
+		      args.type = type;
+		      val = thread_switch_and_call (ti->num, &insert_one_watchpoint, &args); 
+		    }
+		}
+	    }
+	}
+
+      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)
@@ -982,7 +1100,7 @@ insert_bp_location (struct bp_location *
 		      else if (bpt->owner->type == bp_access_watchpoint)
 			type = hw_access;
 
-		      val = target_insert_watchpoint (addr, len, type);
+		      val = insert_watchpoint (bpt, addr, len, type);
 		      if (val == -1)
 			{
 			  /* Don't exit the loop, try to insert
@@ -1404,6 +1522,36 @@ detach_breakpoints (int pid)
 }
 
 static int
+remove_one_watchpoint (void *data)
+{
+  struct target_watchpoint_args *args = (struct target_watchpoint_args *)data;
+
+  return target_remove_watchpoint (args->addr, args->len, args->type);
+}
+
+static int
+remove_watchpoint (struct bp_location *bpt, CORE_ADDR addr, int len, int type)
+{
+  int val;
+  struct target_watchpoint_args args;
+
+  args.addr = addr;
+  args.len = len;
+  args.type = type;
+
+  /* If watchpoints do not apply to all threads automatically, we have to insert
+     and delete them for every thread.  Otherwise, we can insert or delete them
+     once from any thread.  */
+#ifdef WATCHPOINTS_PER_THREAD
+  val = thread_switch_and_call (bpt->owner->thread, &remove_one_watchpoint, &args);
+#else
+  val = remove_one_watchpoint (&args);
+#endif
+
+  return val;
+}
+
+static int
 remove_breakpoint (struct bp_location *b, insertion_state_t is)
 {
   int val;
@@ -1512,7 +1660,7 @@ remove_breakpoint (struct bp_location *b
 		  else if (b->owner->type == bp_access_watchpoint)
 		    type = hw_access;
 
-		  val = target_remove_watchpoint (addr, len, type);
+		  val = remove_watchpoint (b, addr, len, type);
 		  if (val == -1)
 		    b->inserted = 1;
 		  val = 0;
@@ -2122,8 +2270,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: thread-db.c
===================================================================
RCS file: /cvs/src/src/gdb/thread-db.c,v
retrieving revision 1.46
diff -u -p -r1.46 thread-db.c
--- thread-db.c	8 Oct 2004 20:29:56 -0000	1.46
+++ thread-db.c	19 Oct 2004 22:59:48 -0000
@@ -762,6 +762,13 @@ attach_thread (ptid_t ptid, const td_thr
   ATTACH_LWP (BUILD_LWP (ti_p->ti_lid, GET_PID (ptid)), 0);
 #endif
 
+  /* For some platforms, watchpoints must be applied to each and every
+     thread.  When a new thread is created, all current watchpoints need
+     to be inserted.  */
+#ifdef WATCHPOINTS_PER_THREAD
+  insert_watchpoints_for_new_thread (tp);
+#endif
+
   /* Enable thread event reporting for this thread.  */
   err = td_thr_event_enable_p (th_p, 1);
   if (err != TD_OK)

Re: [RFA]: Watchpoints per thread patch

[Eli Zaretskii]

> Date: Tue, 19 Oct 2004 19:56:57 -0400
> From: Jeff Johnston <jjohnstn@redhat.com>
> 
> The following patch adds needed support for the ia64 and s390 platforms.  For 
> these platforms, watchpoints need to be inserted / removed on each thread so as 
> to work across all threads.

The patch is fine with me, provided that you take care of the
following minor issues:

> +# Set various watchpoint flags based on host/cpu
> +
> +case "${host}" in
> +ia64-*-*)
> +	gdb_host_watchpoints_per_thread=1

This is not very autoconfish, but I guess there's no better way to
test for this, is there?

> +int
> +thread_switch_and_call (int thread_num, int (*callback) (void *), void *args)

IMHO, the name of this function is misleading; a better name would
something like map_threads or apply_to_all_threads.

> +int
> +insert_watchpoints_for_new_thread (struct thread_info *ti)

This function is only neded if WATCHPOINTS_PER_THREAD is defined, so I
think the function's definition itself should be conditioned by
WATCHPOINTS_PER_THREAD.

Re: [RFA]: Watchpoints per thread patch

[Mark Kettenis]

   Date: Tue, 19 Oct 2004 19:56:57 -0400
   From: Jeff Johnston <jjohnstn@redhat.com>

   The following patch adds needed support for the ia64 and s390
   platforms.  For these platforms, watchpoints need to be inserted /
   removed on each thread so as to work across all threads.  The patch
   adds support for detecting this at configuration time and setting a
   new flag WATCHPOINTS_PER_THREAD.  This flag is used when
   inserting/removing watchpoints and when a new thread event occurs.

Jeff, this is almost certainly the wrong way to implement this.

Firstly you're treating this as a generic feature of the ia64 and s390
platforms, while in fact it's a feature of the Linux kernel.  It's
true that we don't really support anything but Linux on ia64 and s390,
but that will certainly change in the near feature when FreeBSD/ia64
support will be integrated.

Secondly, you're setting the flag whenever the host is ia64 or s390.
Have you thought about what will happen if you build a cross-debugger
on one of these platforms?

Thirdly, I'm not really charmed about your choice to do this waith a
global #define that you set in configure.  Eli already told you that
it's not very autoconfy, but it also causes us to conditionally
compile parts of the code only on certain platforms.  This is not the
direction in which we're heading.  Have you thought about implementing
an observer for new threads and using that to set the breakpoints in a
new thread?

Cheers,

Mark

Re: [RFA]: Watchpoints per thread patch

[Jeff Johnston]

Mark Kettenis wrote:
>    Date: Tue, 19 Oct 2004 19:56:57 -0400
>    From: Jeff Johnston <jjohnstn@redhat.com>
> 
>    The following patch adds needed support for the ia64 and s390
>    platforms.  For these platforms, watchpoints need to be inserted /
>    removed on each thread so as to work across all threads.  The patch
>    adds support for detecting this at configuration time and setting a
>    new flag WATCHPOINTS_PER_THREAD.  This flag is used when
>    inserting/removing watchpoints and when a new thread event occurs.
> 
> Jeff, this is almost certainly the wrong way to implement this.
> 
> Firstly you're treating this as a generic feature of the ia64 and s390
> platforms, while in fact it's a feature of the Linux kernel.  It's
> true that we don't really support anything but Linux on ia64 and s390,
> but that will certainly change in the near feature when FreeBSD/ia64
> support will be integrated.
> 
> Secondly, you're setting the flag whenever the host is ia64 or s390.
> Have you thought about what will happen if you build a cross-debugger
> on one of these platforms?
>

Hmm, aside from the observer suggestion below, would it have been ok if the 
autoconf test were to test for linux as well?  There is another quirk with ia64 
which I have also used a flag solution for because it affects the behavior of 
the former lin-lwp, now renamed.

> Thirdly, I'm not really charmed about your choice to do this waith a
> global #define that you set in configure.  Eli already told you that
> it's not very autoconfy, but it also causes us to conditionally
> compile parts of the code only on certain platforms.  This is not the
> direction in which we're heading.  Have you thought about implementing
> an observer for new threads and using that to set the breakpoints in a
> new thread?
> 

The observer would certainly be cleaner for handling the new thread aspect of 
the problem as the observer could be kept within the respective linux nat files. 
  There is still the problem of removing and inserting watchpoints per thread 
during a step.  I could embed the thread looping in the lower level ia64 and 
s390 insert/remove linux watchpoint code.  Does that seem reasonable or is that 
considered overloading the to_xxx_watchpoint functions?

-- Jeff J.

Re: [RFA]: Watchpoints per thread patch

[Andrew Cagney]

Jeff Johnston wrote:
> The following patch adds needed support for the ia64 and s390 
> platforms.  For these platforms, watchpoints need to be inserted / 
> removed on each thread so as to work across all threads.  The patch adds 
> support for detecting this at configuration time and setting a new flag 
> WATCHPOINTS_PER_THREAD.  This flag is used when inserting/removing 
> watchpoints and when a new thread event occurs.
> 
> This patch in itself does not give these platforms threaded watchpoint 
> support to pass the watchthreads.exp test case, but I am breaking up my 
> bigger patch that works for x86, x86_64, and ia64.  It still fails on 
> the S390 but gets part marks because threaded watchpoints actually 
> trigger properly however the S390 cannot determine which watchpoint gets 
> triggered when multiple watchpoint events occur. On ia64, the 
> watchthreads.exp test case will fail differently than before because the 
> low-level register and watchpoint code does not properly calculate the 
> LWP for accessing registers so we end up eating through the maximum 
> number of watchpoints quicker than anticipated.  I have a subsequent 
> patch for getting the LWP reliably, but this subsequent patch may be 
> made unnecessary depending on what Daniel does with ptids and the 
> thread-db layer.  Regardless of Daniel's redesign, the watchpoints still 
> must be inserted/removed on each thread.

The controversy here appears to be with the target dependant code being 
added to breakpoint.c.

For the target's that have the problem, modify target_insert_watchpoint 
to include the code your adding.  That way, instead of:

> +  /* If watchpoints do not apply to all threads automatically, we have to insert
> +     and delete them for every thread.  Otherwise, we can insert or delete them
> +     once from any thread.  */
> +#ifdef WATCHPOINTS_PER_THREAD
> +  val = thread_switch_and_call (bpt->owner->thread, &insert_one_watchpoint, &args);
> +#else
> +  val = insert_one_watchpoint (&args);
> +#endif

the underlying target (ia64-linux-nat, ...) can locally override the 
method and handle the problem.  The code's the same, but how it is wired 
up is different

Sound reasonable to all?

Andrew

Re: [RFA]: Watchpoints per thread patch

[Daniel Jacobowitz]

On Wed, Oct 20, 2004 at 01:27:15PM -0400, Andrew Cagney wrote:
> the underlying target (ia64-linux-nat, ...) can locally override the 
> method and handle the problem.  The code's the same, but how it is wired 
> up is different
> 
> Sound reasonable to all?

I think that sounds pretty good.  Hopefully the changes involved will
be pretty small, since I imagine that most GNU/Linux targets with
hardware watchpoints will want it.

-- 
Daniel Jacobowitz

Re: [RFA]: Watchpoints per thread patch

[Eli Zaretskii]

> Date: Wed, 20 Oct 2004 13:27:15 -0400
> From: Andrew Cagney <cagney@gnu.org>
> Cc: gdb-patches@sources.redhat.com
> 
> The controversy here appears to be with the target dependant code being 
> added to breakpoint.c.

Is this really target-dependant?  What happens on other targets that
support threads and watchpoints?

Re: [RFA]: Watchpoints per thread patch

[Jeff Johnston]

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

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

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

Ok to commit?

-- Jeff J.

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

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

doc/ChangeLog:

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

         * observer.texi (new_thread): New observer.


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

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

Re: [RFA]: Watchpoints per thread patch

[Daniel Jacobowitz]

On Wed, Oct 27, 2004 at 06:36:14PM -0400, Jeff Johnston wrote:
> +      /* For every active watchpoint, we need to insert the watchpoint on 
> +         the new thread.  */
> +      if ((b->loc_type == bp_loc_hardware_watchpoint
> +	   || b->owner->type == bp_watchpoint))

Do you need bp_watchpoint here?  That's going to be a software
watchpoint.

> @@ -757,11 +759,16 @@ attach_thread (ptid_t ptid, const td_thr
>    if (ti_p->ti_state == TD_THR_UNKNOWN || ti_p->ti_state == TD_THR_ZOMBIE)
>      return;			/* A zombie thread -- do not attach.  */
>  
> +  new_ptid = BUILD_LWP (ti_p->ti_lid, GET_PID (ptid));
> +
>    /* Under GNU/Linux, we have to attach to each and every thread.  */
>  #ifdef ATTACH_LWP
> -  ATTACH_LWP (BUILD_LWP (ti_p->ti_lid, GET_PID (ptid)), 0);
> +  ATTACH_LWP (new_ptid, 0);
>  #endif
>  
> +  /* Inform any observers of new attached thread.  */
> +  observer_notify_new_thread (new_ptid);
> +
>    /* Enable thread event reporting for this thread.  */
>    err = td_thr_event_enable_p (th_p, 1);
>    if (err != TD_OK)

Is there somewhere in the core threading code we could do this, rather
than in a GNU/Linux target file?

-- 
Daniel Jacobowitz

Re: [RFA]: Watchpoints per thread patch

[Jeff Johnston]

Daniel Jacobowitz wrote:
> On Wed, Oct 27, 2004 at 06:36:14PM -0400, Jeff Johnston wrote:
> 
>>+      /* For every active watchpoint, we need to insert the watchpoint on 
>>+         the new thread.  */
>>+      if ((b->loc_type == bp_loc_hardware_watchpoint
>>+	   || b->owner->type == bp_watchpoint))
> 
> 
> Do you need bp_watchpoint here?  That's going to be a software
> watchpoint.
>

No, it should only be hardware watchpoints.  I will remove this.

> 
>>@@ -757,11 +759,16 @@ attach_thread (ptid_t ptid, const td_thr
>>   if (ti_p->ti_state == TD_THR_UNKNOWN || ti_p->ti_state == TD_THR_ZOMBIE)
>>     return;			/* A zombie thread -- do not attach.  */
>> 
>>+  new_ptid = BUILD_LWP (ti_p->ti_lid, GET_PID (ptid));
>>+
>>   /* Under GNU/Linux, we have to attach to each and every thread.  */
>> #ifdef ATTACH_LWP
>>-  ATTACH_LWP (BUILD_LWP (ti_p->ti_lid, GET_PID (ptid)), 0);
>>+  ATTACH_LWP (new_ptid, 0);
>> #endif
>> 
>>+  /* Inform any observers of new attached thread.  */
>>+  observer_notify_new_thread (new_ptid);
>>+
>>   /* Enable thread event reporting for this thread.  */
>>   err = td_thr_event_enable_p (th_p, 1);
>>   if (err != TD_OK)
> 
> 
> Is there somewhere in the core threading code we could do this, rather
> than in a GNU/Linux target file?
> 

Were you thinking of add_thread()?  If so, we would have to move the calls to 
add_thread so they never occur before an attach because the low-level observers 
will need the thread already attached.

-- Jeff J.

Re: [RFA]: Watchpoints per thread patch

[Eli Zaretskii]

> Date: Wed, 27 Oct 2004 18:36:14 -0400
> From: Jeff Johnston <jjohnstn@redhat.com>
> Cc: Andrew Cagney <cagney@gnu.org>, gdb-patches@sources.redhat.com
> 
> The attached patch is the rework of my original attempt.  It no longer uses 
> configuration or magic defines.  Per Mark's suggestion, it uses an observer to 
> handle inserting watchpoints on a new thread and only the low-level code knows 
> about inserting/removing watchpoints on all threads.
> 
> Ok to commit?

A few comments:

> +/* External function to insert all existing watchpoints on a newly
> +   attached thread.  IWPFN is a callback function to perform
> +   the target insert watchpoint.  This function is used to support
> +   platforms whereby a watchpoint must be inserted/removed on each
> +   individual thread (e.g. ia64-linux and s390-linux).  For
> +   ia64 and s390 linux, this function is called via a new thread
> +   observer.  */

In this comment, the word "whereby" should be replaced by "where", I
think.

> --- target.h	8 Oct 2004 20:29:55 -0000	1.65
> +++ target.h	27 Oct 2004 21:43:51 -0000
> @@ -178,6 +178,15 @@ extern char *target_signal_to_name (enum
>  /* Given a name (SIGHUP, etc.), return its signal.  */
>  enum target_signal target_signal_from_name (char *);
>  \f
> +
> +/* Watchpoint specification.  */
> +struct target_watchpoint
> +  {
> +    CORE_ADDR addr;
> +    int len;
> +    int type;
> +  };
> +

Why do we put on target.h, which is a general header, a definition of
a struct used only on certain platforms?

> --- doc/observer.texi	1 Sep 2004 17:59:37 -0000	1.8
> +++ doc/observer.texi	27 Oct 2004 21:43:51 -0000
> @@ -95,3 +95,7 @@ inferior, and before any information on 
>  The specified shared library has been discovered to be unloaded.
>  @end deftypefun
>  
> +@deftypefun void new_thread (ptid_t @var{ptid})
> +A new thread has been attached to.

"A new thread has been attached" to what?  The description of the
observer should at least reference the argument @var{ptid}.

Re: [RFA]: Watchpoints per thread patch

[Daniel Jacobowitz]

On Wed, Oct 27, 2004 at 07:17:13PM -0400, Jeff Johnston wrote:
> Were you thinking of add_thread()?  If so, we would have to move the calls 
> to add_thread so they never occur before an attach because the low-level 
> observers will need the thread already attached.

Oh, that's a good point.  Do you think that's a reasonable change to
make?

-- 
Daniel Jacobowitz

Re: [RFA]: Watchpoints per thread patch

[Jeff Johnston]

Daniel Jacobowitz wrote:
> On Wed, Oct 27, 2004 at 07:17:13PM -0400, Jeff Johnston wrote:
> 
>>Were you thinking of add_thread()?  If so, we would have to move the calls 
>>to add_thread so they never occur before an attach because the low-level 
>>observers will need the thread already attached.
> 
> 
> Oh, that's a good point.  Do you think that's a reasonable change to
> make?
>

It is a can of worms.  I can move the add_thread call in attach_thread easily 
enough, but there are other calls to add_thread strewn about.  For example, 
corelow.c calls add_thread as does infrun.c when it finds a new process.  I 
certainly don't see it being valid for either of these scenarios to 
insert/remove all watchpoints.  My personal preference would be to leave it 
where it is for now.

-- Jeff J.

Re: [RFA]: Watchpoints per thread patch

[Daniel Jacobowitz]

On Thu, Oct 28, 2004 at 03:47:22PM -0400, Jeff Johnston wrote:
> Daniel Jacobowitz wrote:
> >On Wed, Oct 27, 2004 at 07:17:13PM -0400, Jeff Johnston wrote:
> >
> >>Were you thinking of add_thread()?  If so, we would have to move the 
> >>calls to add_thread so they never occur before an attach because the 
> >>low-level observers will need the thread already attached.
> >
> >
> >Oh, that's a good point.  Do you think that's a reasonable change to
> >make?
> >
> 
> It is a can of worms.  I can move the add_thread call in attach_thread 
> easily enough, but there are other calls to add_thread strewn about.  For 
> example, corelow.c calls add_thread as does infrun.c when it finds a new 
> process.  I certainly don't see it being valid for either of these 
> scenarios to insert/remove all watchpoints.  My personal preference would 
> be to leave it where it is for now.

There are two separate questions here:
  - When do we need to be adding and removing watchpoints from threads
    on GNU/Linux?
  - When should an observer named "new_thread" be called?

If it's not valid to do the former action at all the latter points,
then it's not the right observer to be using.

The code in infrun is never reached for native GNU/Linux threads, btw;
I'm not sure which targets if any do reach it.  I don't believe remote
GNU/Linux threads do either.

-- 
Daniel Jacobowitz

Re: [RFA]: Watchpoints per thread patch

[Jeff Johnston]

Daniel Jacobowitz wrote:
> On Thu, Oct 28, 2004 at 03:47:22PM -0400, Jeff Johnston wrote:
> 
>>Daniel Jacobowitz wrote:
>>
>>>On Wed, Oct 27, 2004 at 07:17:13PM -0400, Jeff Johnston wrote:
>>>
>>>
>>>>Were you thinking of add_thread()?  If so, we would have to move the 
>>>>calls to add_thread so they never occur before an attach because the 
>>>>low-level observers will need the thread already attached.
>>>
>>>
>>>Oh, that's a good point.  Do you think that's a reasonable change to
>>>make?
>>>
>>
>>It is a can of worms.  I can move the add_thread call in attach_thread 
>>easily enough, but there are other calls to add_thread strewn about.  For 
>>example, corelow.c calls add_thread as does infrun.c when it finds a new 
>>process.  I certainly don't see it being valid for either of these 
>>scenarios to insert/remove all watchpoints.  My personal preference would 
>>be to leave it where it is for now.
> 
> 
> There are two separate questions here:
>   - When do we need to be adding and removing watchpoints from threads
>     on GNU/Linux?
>   - When should an observer named "new_thread" be called?
> 
> If it's not valid to do the former action at all the latter points,
> then it's not the right observer to be using.
>

It could easily be called new_linux_thread or new_lwp_thread.

> The code in infrun is never reached for native GNU/Linux threads, btw;
> I'm not sure which targets if any do reach it.  I don't believe remote
> GNU/Linux threads do either.
> 

If you feel that add_thread will be safe in other scenarios, I am more than 
happy to do the change, but I will only be testing linux.

-- Jeff J.

Re: [RFA]: Watchpoints per thread patch

[Jeff Johnston]

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

Eli Zaretskii wrote:
>>Date: Wed, 27 Oct 2004 18:36:14 -0400
>>From: Jeff Johnston <jjohnstn@redhat.com>
>>Cc: Andrew Cagney <cagney@gnu.org>, gdb-patches@sources.redhat.com
>>
>>The attached patch is the rework of my original attempt.  It no longer uses 
>>configuration or magic defines.  Per Mark's suggestion, it uses an observer to 
>>handle inserting watchpoints on a new thread and only the low-level code knows 
>>about inserting/removing watchpoints on all threads.
>>
>>Ok to commit?
> 
> 
> A few comments:
> 
> 
>>+/* External function to insert all existing watchpoints on a newly
>>+   attached thread.  IWPFN is a callback function to perform
>>+   the target insert watchpoint.  This function is used to support
>>+   platforms whereby a watchpoint must be inserted/removed on each
>>+   individual thread (e.g. ia64-linux and s390-linux).  For
>>+   ia64 and s390 linux, this function is called via a new thread
>>+   observer.  */
> 
> 
> In this comment, the word "whereby" should be replaced by "where", I
> think.
>

Ok, done.

> 
>>--- target.h	8 Oct 2004 20:29:55 -0000	1.65
>>+++ target.h	27 Oct 2004 21:43:51 -0000
>>@@ -178,6 +178,15 @@ extern char *target_signal_to_name (enum
>> /* Given a name (SIGHUP, etc.), return its signal.  */
>> enum target_signal target_signal_from_name (char *);
>> \f
>>+
>>+/* Watchpoint specification.  */
>>+struct target_watchpoint
>>+  {
>>+    CORE_ADDR addr;
>>+    int len;
>>+    int type;
>>+  };
>>+
> 
> 
> Why do we put on target.h, which is a general header, a definition of
> a struct used only on certain platforms?
> 

I have moved this to linux-nat.h and renamed it struct linux_watchpoint.

> 
>>--- doc/observer.texi	1 Sep 2004 17:59:37 -0000	1.8
>>+++ doc/observer.texi	27 Oct 2004 21:43:51 -0000
>>@@ -95,3 +95,7 @@ inferior, and before any information on 
>> The specified shared library has been discovered to be unloaded.
>> @end deftypefun
>> 
>>+@deftypefun void new_thread (ptid_t @var{ptid})
>>+A new thread has been attached to.
> 
> 
> "A new thread has been attached" to what?  The description of the
> observer should at least reference the argument @var{ptid}.
>

I have changed the definition to be more meaningful now that it is being used 
generically by all.

On Thu, Oct 28, 2004 at 03:47:22PM -0400, Jeff Johnston wrote:

 >> Daniel Jacobowitz wrote:
 >
 >>> >On Wed, Oct 27, 2004 at 07:17:13PM -0400, Jeff Johnston wrote:
 >>> >
 >>
 >>>> >>Were you thinking of add_thread()?  If so, we would have to move the
 >>>> >>calls to add_thread so they never occur before an attach because the
 >>>> >>low-level observers will need the thread already attached.
 >>
 >>> >
 >>> >
 >>> >Oh, that's a good point.  Do you think that's a reasonable change to
 >>> >make?
 >>> >
 >
 >>

Daniel, I have moved the observer notification to add_thread as suggested.  To 
do this, I had to move a few things in attach_thread.  As well, I had to add 
another part of my full patch in because the ptid that add_thread knows about is 
in the wrong format (pid, 0, tid).  The low-level insert watchpoint code needs 
the lwp so I have added in my target_get_lwp change.  I realize you have plans 
to change how the ptid is kept, but until that is fleshed out, this change is 
required.  I used a call-back to allow breakpoint.c which knows how to handle 
watchpoints to communicate with the low-level linux code that knows how to 
insert/remove a watchpoint.

I have tested on ia64 and s390 linux.  The ia64 requires another patch to make 
it pass the watchthreads.exp test.  S390 linux can't pass that test without 
additional hardware support, however, I will note that s390 is properly 
recognizing hardware watchpoints on threads which is a major step forward.

Ok to commit?

2004-11-04  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 thread observer.
         * linux-nat.h (struct linux_watchpoint): New structure.
         * thread-db.c (attach_thread): Add the thread after attaching
         to it.  Before allocating the thread private info area, check
         to see if it has already been allocated.
         (thread_db_get_info): Allocate a thread private info area if
         one doesn't already exist.
         (init_thread_db_ops): Point to_get_lwp to lwp_from_thread function.
         * thread.c (add_thread): Notify any observers of a new thread event.
         * s390-nat.c (s390_tid): New function.
         (s390_inferior_tid): Change to call s390_tid.
         (s390_remove_one_watchpoint): New function.
         (s390_remove_watchpoint_callback): Ditto.
         (s390_remove_watchpoint): Change to iterate through lwps and
         remove the specified watchpoint for each thread.
         (s390_insert_one_watchpoint): New function.
         (s390_insert_watchpoint_callback): Ditto.
         (s390_insert_watchpoint): Change to iterate through lwps and
         insert the specified watchpoint on each thread.
         (s390_new_thread): New thread observer.
         (_initialize_s390_nat): New function.  Initialize
         new thread observer.
         * target.c (return_ptid): New static function.
         (update_current_target): Add support for new to_get_lwp.
         (init_dummy_target): Ditto.
         * target.h (struct target_ops): Add to_get_lwp.
         (target_get_lwp): New macro.


doc/ChangeLog:

2004-11-04  Jeff Johnston  <jjohnstn@redhat.com>

         * observer.texi (new_thread): New observer.






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

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.184
diff -u -p -r1.184 breakpoint.c
--- breakpoint.c	29 Oct 2004 20:23:04 -0000	1.184
+++ breakpoint.c	4 Nov 2004 18:19:11 -0000
@@ -748,6 +748,91 @@ insert_catchpoint (struct ui_out *uo, vo
   return 0;
 }
 
+/* External function to insert all existing watchpoints on a newly
+   attached thread.  IWPFN is a callback function to perform
+   the target insert watchpoint.  This function is used to support
+   platforms 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
+	   || b->owner->type == bp_watchpoint))
+	{
+	  struct value *v = b->owner->val_chain;
+
+	  /* Look at each value on the value chain.  */
+	  for (; v; v = v->next)
+	    {
+	      /* If it's a memory location, and GDB actually needed
+		 its contents to evaluate the expression, then we
+		 must watch it.  */
+	      if (VALUE_LVAL (v) == lval_memory
+		  && ! VALUE_LAZY (v))
+		{
+		  struct type *vtype = check_typedef (VALUE_TYPE (v));
+		  
+		  /* We only watch structs and arrays if user asked
+		     for it explicitly, never if they just happen to
+		     appear in the middle of some value chain.  */
+		  if (v == b->owner->val_chain
+		      || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
+			  && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
+		    {
+		      CORE_ADDR addr;
+		      int len, type;
+		      
+		      addr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
+		      len = TYPE_LENGTH (VALUE_TYPE (v));
+		      type = hw_write;
+		      if (b->owner->type == bp_read_watchpoint)
+			type = hw_read;
+		      else if (b->owner->type == bp_access_watchpoint)
+			type = hw_access;
+		      val = (*iwpfn) (new_thread, addr, len, type); 
+		    }
+		}
+	    }
+	}
+
+      if (val)
+	return_val = val;
+    }
+
+  /* Failure to insert a watchpoint on any memory value in the
+     value chain brings us here.  */
+  if (return_val)
+    {
+      fprintf_unfiltered (tmp_error_stream,
+			  "%s\n",
+			  "Could not insert hardware watchpoints on new thread."); 
+      target_terminal_ours_for_output ();
+      error_stream (tmp_error_stream);
+    }
+  return return_val;
+}
+
 /* Helper routine: free the value chain for a breakpoint (watchpoint).  */
 
 static void free_valchain (struct bp_location *b)
@@ -2122,8 +2207,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	4 Nov 2004 18:19:11 -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	4 Nov 2004 18:19:11 -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,13 +663,41 @@ 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)
 {
   CORE_ADDR psr;
   int tid;
   struct siginfo siginfo;
-  ptid_t ptid = inferior_ptid;
+  ptid_t ptid = target_get_lwp (inferior_ptid);
 
   tid = TIDGET(ptid);
   if (tid == 0)
@@ -674,3 +735,31 @@ ia64_linux_xfer_unwind_table (struct tar
 {
   return syscall (__NR_getunwind, readbuf, len);
 }
+
+/* Internal callback for insert_watchpoints_for_new_thread to call.
+   The PTID will be in the thread-level format and must be
+   translated to the lwp-level format before calling
+   ia64_linux_insert_one_watchpoint to insert any watchpoint.  */
+static int
+ia64_linux_insert_watchpoint_for_thread (ptid_t ptid, CORE_ADDR addr, 
+		                         int len, int rw)
+{
+  ptid_t lwp_ptid = target_get_lwp (ptid);
+  return ia64_linux_insert_one_watchpoint (lwp_ptid, addr, len, rw);
+}
+  
+/* 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_watchpoint_for_thread);
+}
+
+void
+_initialize_ia64_linux_nat (void)
+{
+  observer_attach_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	4 Nov 2004 18:19:11 -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: 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	4 Nov 2004 18:19:12 -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,18 +114,32 @@ 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.  */
+    {
+      /* In some cases, we have a thread-level format ptid which
+         must be converted to an lwp-level format ptid.  */
+      ptid_t lwp_ptid = target_get_lwp (ptid);
+      tid = TIDGET (lwp_ptid);
+      if (tid == 0)
+        tid = PIDGET (lwp_ptid); /* Not a threaded program.  */
+    }
 
   return tid;
 }
 
+/* Find the TID for the current inferior thread to use with ptrace.  */
+static int
+s390_inferior_tid (void)
+{
+  return s390_tid (inferior_ptid);
+}
+
 /* Fetch all general-purpose registers from process/thread TID and
    store their values in GDB's register cache.  */
 static void
@@ -269,9 +285,9 @@ s390_stopped_by_watchpoint (void)
 }
 
 static void
-s390_fix_watch_points (void)
+s390_fix_watch_points (ptid_t ptid)
 {
-  int tid = s390_inferior_tid ();
+  int tid = s390_tid (ptid);
 
   per_struct per_info;
   ptrace_area parea;
@@ -308,8 +324,9 @@ s390_fix_watch_points (void)
     perror_with_name ("Couldn't modify watchpoint status");
 }
 
-int
-s390_insert_watchpoint (CORE_ADDR addr, int len)
+/* Insert a specified watchpoint on a specified thread.  */
+static int
+s390_insert_one_watchpoint (ptid_t ptid, CORE_ADDR addr, int len, int type)
 {
   struct watch_area *area = xmalloc (sizeof (struct watch_area));
   if (!area)
@@ -321,12 +338,36 @@ s390_insert_watchpoint (CORE_ADDR addr, 
   area->next = watch_base;
   watch_base = area;
 
-  s390_fix_watch_points ();
+  s390_fix_watch_points (ptid);
   return 0;
 }
 
+/* Callback routine to use with iterate_over_lwps to insert a specified
+   watchpoint on all threads.  */
+static int
+s390_insert_watchpoint_callback (struct lwp_info *lwp, void *data)
+{
+  struct linux_watchpoint *args = (struct linux_watchpoint *)data;
+
+  return s390_insert_one_watchpoint (lwp->ptid, args->addr, args->len, 0);
+}
+
+/* Insert a specified watchpoint on all threads.  */
 int
-s390_remove_watchpoint (CORE_ADDR addr, int len)
+s390_insert_watchpoint (CORE_ADDR addr, int len)
+{
+  struct linux_watchpoint args;
+
+  args.addr = addr;
+  args.len = len;
+  /* For the S390, a watchpoint must be inserted/removed for each
+     thread so we iterate over the list of existing lwps.  */
+  return iterate_over_lwps (&s390_insert_watchpoint_callback, &args);
+}
+
+/* Remove a specified watchpoint from a specified thread.  */
+static int
+s390_remove_one_watchpoint (ptid_t ptid, CORE_ADDR addr, int len)
 {
   struct watch_area *area, **parea;
 
@@ -346,10 +387,32 @@ s390_remove_watchpoint (CORE_ADDR addr, 
   *parea = area->next;
   xfree (area);
 
-  s390_fix_watch_points ();
+  s390_fix_watch_points (ptid);
   return 0;
 }
 
+/* Callback routine to use with iterate_over_lwps to remove a specified
+   watchpoint from all threads.  */
+static int
+s390_remove_watchpoint_callback (struct lwp_info *lwp, void *data)
+{
+  struct linux_watchpoint *args = (struct linux_watchpoint *)data;
+
+  return s390_remove_one_watchpoint (lwp->ptid, args->addr, args->len);
+}
+
+/* Remove a specified watchpoint from all threads.  */
+int
+s390_remove_watchpoint (CORE_ADDR addr, int len)
+{
+  struct linux_watchpoint args;
+
+  args.addr = addr;
+  args.len = len;
+  /* For the S390, a watchpoint must be inserted/removed for each
+     thread so we iterate over the list of existing lwps.  */
+  return iterate_over_lwps (&s390_remove_watchpoint_callback, &args);
+}
 
 int
 kernel_u_size (void)
@@ -357,3 +420,18 @@ kernel_u_size (void)
   return sizeof (struct user);
 }
 
+/* New thread observer that inserts all existing watchpoints on the
+   new thread.  */
+static void
+s390_new_thread (ptid_t ptid)
+{
+  insert_watchpoints_for_new_thread (ptid, &s390_insert_one_watchpoint);
+}
+
+void
+_initialize_s390_nat (void)
+{
+  observer_attach_new_thread (s390_new_thread);
+}
+
+
Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.90
diff -u -p -r1.90 target.c
--- target.c	8 Oct 2004 20:29:55 -0000	1.90
+++ target.c	4 Nov 2004 18:19:12 -0000
@@ -61,6 +61,8 @@ static int return_one (void);
 
 static int return_minus_one (void);
 
+static ptid_t return_ptid (ptid_t ptid);
+
 void target_ignore (void);
 
 static void target_command (char *, int);
@@ -430,6 +432,7 @@ update_current_target (void)
       INHERIT (to_notice_signals, t);
       INHERIT (to_thread_alive, t);
       INHERIT (to_find_new_threads, t);
+      INHERIT (to_get_lwp, t);
       INHERIT (to_pid_to_str, t);
       INHERIT (to_extra_thread_info, t);
       INHERIT (to_stop, t);
@@ -606,6 +609,9 @@ update_current_target (void)
   de_fault (to_find_new_threads, 
 	    (void (*) (void)) 
 	    target_ignore);
+  de_fault (to_get_lwp,
+            (ptid_t (*) (ptid_t))
+            return_ptid);
   de_fault (to_extra_thread_info, 
 	    (char *(*) (struct thread_info *)) 
 	    return_zero);
@@ -1541,6 +1547,12 @@ return_minus_one (void)
   return -1;
 }
 
+static ptid_t
+return_ptid (ptid_t ptid)
+{
+  return ptid;
+}
+
 /*
  * Resize the to_sections pointer.  Also make sure that anyone that
  * was holding on to an old value of it gets updated.
@@ -1793,6 +1805,7 @@ init_dummy_target (void)
   dummy_target.to_find_memory_regions = dummy_find_memory_regions;
   dummy_target.to_make_corefile_notes = dummy_make_corefile_notes;
   dummy_target.to_xfer_partial = default_xfer_partial;
+  dummy_target.to_get_lwp = return_ptid;
   dummy_target.to_magic = OPS_MAGIC;
 }
 \f
Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/target.h,v
retrieving revision 1.65
diff -u -p -r1.65 target.h
--- target.h	8 Oct 2004 20:29:55 -0000	1.65
+++ target.h	4 Nov 2004 18:19:12 -0000
@@ -372,6 +372,7 @@ struct target_ops
     void (*to_notice_signals) (ptid_t ptid);
     int (*to_thread_alive) (ptid_t ptid);
     void (*to_find_new_threads) (void);
+    ptid_t (*to_get_lwp) (ptid_t ptid);
     char *(*to_pid_to_str) (ptid_t);
     char *(*to_extra_thread_info) (struct thread_info *);
     void (*to_stop) (void);
@@ -802,6 +803,10 @@ extern void target_load (char *arg, int 
 #define target_find_new_threads() \
      (*current_target.to_find_new_threads) (); \
 
+/* Get the lwp for a thread.  */
+#define target_get_lwp(ptid) \
+     (*current_target.to_get_lwp) (ptid); \
+
 /* Make target stop in a continuable fashion.  (For instance, under
    Unix, this should act like SIGSTOP).  This function is normally
    used by GUIs to implement a stop button.  */
Index: thread-db.c
===================================================================
RCS file: /cvs/src/src/gdb/thread-db.c,v
retrieving revision 1.46
diff -u -p -r1.46 thread-db.c
--- thread-db.c	8 Oct 2004 20:29:56 -0000	1.46
+++ thread-db.c	4 Nov 2004 18:19:12 -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>
@@ -333,6 +334,17 @@ thread_db_get_info (struct thread_info *
 {
   td_err_e err;
 
+  /* Allocate a private area if needed.  This can occur for ia64 or
+     s390 linux which must insert breakpoints on newly found threads.
+     The observer for a new thread event will be called on an add_thread
+     call before the private area has been set up and will need to
+     get the lwp for the new ptid using this function.  */
+  if (!thread_info->private)
+    {
+      thread_info->private = xmalloc (sizeof (struct private_thread_info));
+      memset (thread_info->private, 0, sizeof (struct private_thread_info));
+    }
+
   if (thread_info->private->ti_valid)
     return &thread_info->private->ti;
 
@@ -746,14 +758,6 @@ attach_thread (ptid_t ptid, const td_thr
 
   check_thread_signals ();
 
-  /* Add the thread to GDB's thread list.  */
-  tp = add_thread (ptid);
-  tp->private = xmalloc (sizeof (struct private_thread_info));
-  memset (tp->private, 0, sizeof (struct private_thread_info));
-
-  if (verbose)
-    printf_unfiltered ("[New %s]\n", target_pid_to_str (ptid));
-
   if (ti_p->ti_state == TD_THR_UNKNOWN || ti_p->ti_state == TD_THR_ZOMBIE)
     return;			/* A zombie thread -- do not attach.  */
 
@@ -762,6 +766,21 @@ attach_thread (ptid_t ptid, const td_thr
   ATTACH_LWP (BUILD_LWP (ti_p->ti_lid, GET_PID (ptid)), 0);
 #endif
 
+  /* Add the thread to GDB's thread list.  
+     We do this after attaching so any observers of a new
+     thread event can perform PTRACE operations on the thread
+     if needed.  An observer may end up allocating the
+     private info area, so check first.  */
+  tp = add_thread (ptid);
+  if (!tp->private)
+    {
+      tp->private = xmalloc (sizeof (struct private_thread_info));
+      memset (tp->private, 0, sizeof (struct private_thread_info));
+    }
+
+  if (verbose)
+    printf_unfiltered ("[New %s]\n", target_pid_to_str (ptid));
+
   /* Enable thread event reporting for this thread.  */
   err = td_thr_event_enable_p (th_p, 1);
   if (err != TD_OK)
@@ -1346,6 +1365,7 @@ init_thread_db_ops (void)
   thread_db_ops.to_mourn_inferior = thread_db_mourn_inferior;
   thread_db_ops.to_thread_alive = thread_db_thread_alive;
   thread_db_ops.to_find_new_threads = thread_db_find_new_threads;
+  thread_db_ops.to_get_lwp = lwp_from_thread;
   thread_db_ops.to_pid_to_str = thread_db_pid_to_str;
   thread_db_ops.to_stratum = thread_stratum;
   thread_db_ops.to_has_thread_control = tc_schedlock;
Index: thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.39
diff -u -p -r1.39 thread.c
--- thread.c	29 Oct 2004 20:23:13 -0000	1.39
+++ thread.c	4 Nov 2004 18:19:12 -0000
@@ -130,6 +130,10 @@ add_thread (ptid_t ptid)
   tp->num = ++highest_thread_num;
   tp->next = thread_list;
   thread_list = tp;
+
+  /* Inform any observers of the new thread.  */
+  observer_notify_new_thread (ptid);
+
   return tp;
 }
 
Index: doc/observer.texi
===================================================================
RCS file: /cvs/src/src/gdb/doc/observer.texi,v
retrieving revision 1.8
diff -u -p -r1.8 observer.texi
--- doc/observer.texi	1 Sep 2004 17:59:37 -0000	1.8
+++ doc/observer.texi	4 Nov 2004 18:19:12 -0000
@@ -95,3 +95,7 @@ inferior, and before any information on 
 The specified shared library has been discovered to be unloaded.
 @end deftypefun
 
+@deftypefun void new_thread (ptid_t @var{ptid})
+A new thread described by @var{ptid} has been detected by gdb.
+@end deftypefun
+

Re: [RFA]: Watchpoints per thread patch

[Eli Zaretskii]

> Date: Thu, 04 Nov 2004 13:25:32 -0500
> From: Jeff Johnston <jjohnstn@redhat.com>
> Cc: drow@false.org, cagney@gnu.org, gdb-patches@sources.redhat.com
> 
> Ok to commit?

Fine with me, thanks.

Re: [RFA]: Watchpoints per thread patch

[Daniel Jacobowitz]

On Thu, Nov 04, 2004 at 01:25:32PM -0500, Jeff Johnston wrote:
> Daniel, I have moved the observer notification to add_thread as suggested.  
> To do this, I had to move a few things in attach_thread.  As well, I had to 
> add another part of my full patch in because the ptid that add_thread knows 
> about is in the wrong format (pid, 0, tid).  The low-level insert 
> watchpoint code needs the lwp so I have added in my target_get_lwp change.  
> I realize you have plans to change how the ptid is kept, but until that is 
> fleshed out, this change is required.  I used a call-back to allow 
> breakpoint.c which knows how to handle watchpoints to communicate with the 
> low-level linux code that knows how to insert/remove a watchpoint.

I don't want to add target_get_lwp only to remove it a couple weeks
later!  I don't think this patch is appropriate for 6.3; for the
mainline, we have plenty of time, so please wait a little longer.

My plan is to first rename thread-db.c to mark it as clearly GNU/Linux
specific, as I was asked to do, and then change the format of ptid_t
used by thread-db to make finding the LWP trivial.  It should not take
long.  I will try to post the first part tomorrow.

> +      /* For every active watchpoint, we need to insert the watchpoint on 
> +         the new thread.  */
> +      if ((b->loc_type == bp_loc_hardware_watchpoint
> +	   || b->owner->type == bp_watchpoint))

You were going to fix this bit.

Also, I have been thinking about your approach.  You are hooking native
code in via an observer; observers bypass the target stack.  It worked
while you were only calling this observer from the GNU/Linux native
support in thread-db.c.  But now it will mess up if you use a native
ia64 debugger connected to a remote target, in which case we should
never enter the ia64-nat code - there's nothing to ptrace.

The observer could move back to thread-db, but as Andrew keeps
reminding me, there are situations where we could take advantage of
thread-db for things like core files.  This is a target stack activity;
I think we need to find a way to do it without violating the
abstraction of the target stack.

-- 
Daniel Jacobowitz

Re: [RFA]: Watchpoints per thread patch

[Andrew Cagney]

Daniel Jacobowitz wrote:
> On Thu, Nov 04, 2004 at 01:25:32PM -0500, Jeff Johnston wrote:
> 
>>Daniel, I have moved the observer notification to add_thread as suggested.  
>>To do this, I had to move a few things in attach_thread.  As well, I had to 
>>add another part of my full patch in because the ptid that add_thread knows 
>>about is in the wrong format (pid, 0, tid).  The low-level insert 
>>watchpoint code needs the lwp so I have added in my target_get_lwp change.  
>>I realize you have plans to change how the ptid is kept, but until that is 
>>fleshed out, this change is required.  I used a call-back to allow 
>>breakpoint.c which knows how to handle watchpoints to communicate with the 
>>low-level linux code that knows how to insert/remove a watchpoint.
> 
> 
> I don't want to add target_get_lwp only to remove it a couple weeks
> later!  I don't think this patch is appropriate for 6.3; for the
> mainline, we have plenty of time, so please wait a little longer.

You're correct, it definitly isn't appropriate for 6.3.  However, it is 
appropriate for mainline.  Lets get this patch off the table (and have 
working watchpoints), that way we're in a position to better focus on 
just the refactorings you talk of.  Especially since, this work gives us 
a working test case that we can refactor against.

Sound reasonable?

> You were going to fix this bit.
> 
> Also, I have been thinking about your approach.  You are hooking native
> code in via an observer; observers bypass the target stack.  It worked
> while you were only calling this observer from the GNU/Linux native
> support in thread-db.c.  But now it will mess up if you use a native
> ia64 debugger connected to a remote target, in which case we should
> never enter the ia64-nat code - there's nothing to ptrace.

Jeff,

I'd use the cludge based on what is found in remote.c:
   if (!current_target.to_shortname ||
       strcmp (current_target.to_shortname, "remote") != 0)
     error ("command can only be used with remote target");
we need to solve the problem daniel describes but that involves 
structural change.

> The observer could move back to thread-db, but as Andrew keeps
> reminding me, there are situations where we could take advantage of
> thread-db for things like core files.  This is a target stack activity;
> I think we need to find a way to do it without violating the
> abstraction of the target stack.

There are two concurrency abstractions:
	- light-weight-processes
		system level
	- threads
		user level
burried in the target stack, here the code is manipulating the former, 
thread-db manipulates the latter.  That the abstractions are not clearly 
separated is a design flaw, but not one we're going to immediatly fix here.

Andrew

Re: [RFA]: Watchpoints per thread patch

[Daniel Jacobowitz]

On Fri, Nov 05, 2004 at 11:52:25AM -0500, Andrew Cagney wrote:
> Daniel Jacobowitz wrote:
> >I don't want to add target_get_lwp only to remove it a couple weeks
> >later!  I don't think this patch is appropriate for 6.3; for the
> >mainline, we have plenty of time, so please wait a little longer.
> 
> You're correct, it definitly isn't appropriate for 6.3.  However, it is 
> appropriate for mainline.  Lets get this patch off the table (and have 
> working watchpoints), that way we're in a position to better focus on 
> just the refactorings you talk of.  Especially since, this work gives us 
> a working test case that we can refactor against.
> 
> Sound reasonable?

Andrew, I'm confused.  Aren't you the maintainer who is historically
most likely to jump on contributors for kludging around missing
infrastructure?  I think we should solve this correctly, not with (so
far) two majorly incorrect hacks.

And we've already got working watchpoints.  This is for multi-threaded
hardware watchpoints, which have never worked right in GDB and thus
seem to me like a new feature.

> >You were going to fix this bit.
> >
> >Also, I have been thinking about your approach.  You are hooking native
> >code in via an observer; observers bypass the target stack.  It worked
> >while you were only calling this observer from the GNU/Linux native
> >support in thread-db.c.  But now it will mess up if you use a native
> >ia64 debugger connected to a remote target, in which case we should
> >never enter the ia64-nat code - there's nothing to ptrace.
> 
> Jeff,
> 
> I'd use the cludge based on what is found in remote.c:
>   if (!current_target.to_shortname ||
>       strcmp (current_target.to_shortname, "remote") != 0)
>     error ("command can only be used with remote target");
> we need to solve the problem daniel describes but that involves 
> structural change.

There are plenty of other ways to solve this.  This entire operation
should go through the target stack. My cleanups, and yours, are
bringing us to the day when target vector inheritance can be used for
GNU/Linux; then we can do this properly. The precedent of using
observers to support target-stack-specific code is a horrible one that
we should avoid.

Then targets which need this operation can supply a
target_fixup_watchpoints_for_new_thread, and the observer can live in
core code instead of each target.  Doesn't that seem like a better
solution?

-- 
Daniel Jacobowitz

Re: [RFA]: Watchpoints per thread patch

[Andrew Cagney]

Daniel Jacobowitz wrote:
> On Fri, Nov 05, 2004 at 11:52:25AM -0500, Andrew Cagney wrote:
> 
>>Daniel Jacobowitz wrote:
>>
>>>I don't want to add target_get_lwp only to remove it a couple weeks
>>>later!  I don't think this patch is appropriate for 6.3; for the
>>>mainline, we have plenty of time, so please wait a little longer.
>>
>>You're correct, it definitly isn't appropriate for 6.3.  However, it is 
>>appropriate for mainline.  Lets get this patch off the table (and have 
>>working watchpoints), that way we're in a position to better focus on 
>>just the refactorings you talk of.  Especially since, this work gives us 
>>a working test case that we can refactor against.
>>
>>Sound reasonable?
> 
> 
> Andrew, I'm confused.  Aren't you the maintainer who is historically
> most likely to jump on contributors for kludging around missing
> infrastructure?  I think we should solve this correctly, not with (so
> far) two majorly incorrect hacks.
> 
> And we've already got working watchpoints.  This is for multi-threaded
> hardware watchpoints, which have never worked right in GDB and thus
> seem to me like a new feature.

Given our already overcommitted backlog: breakpoints on C++ 
constructors, breakpoints on inline code, DW_OP_piece, i18n, multi-arch 
solib, ....; how realistic is it that we'll, in addition, manage to both 
refactor the linux code base (I know this will be slow as I've been 
working on it) and also add multi-threaded watchpoints, all in the 6.4 
time frame?

Let concentrate on clearing existing backlog, and not add another 
promise to the list.

Andrew

Re: [RFA]: Watchpoints per thread patch

[Daniel Jacobowitz]

On Mon, Nov 08, 2004 at 04:32:23PM -0500, Andrew Cagney wrote:
> Given our already overcommitted backlog: breakpoints on C++ 
> constructors, breakpoints on inline code, DW_OP_piece, i18n, multi-arch 
> solib, ....; how realistic is it that we'll, in addition, manage to both 
> refactor the linux code base (I know this will be slow as I've been 
> working on it) and also add multi-threaded watchpoints, all in the 6.4 
> time frame?
> 
> Let concentrate on clearing existing backlog, and not add another 
> promise to the list.

*sarcasm*

You're right.  That's an excellent plan.  Let's just drop the
multithreaded watchpoint patch, then, if it will never make it
to the front of the backlog.

*sarcasm off*

I don't think that "we have too many other plans" is a reason to accept
obviously broken code, which abuses the observer mechanism and would
make further cleanups of the breakpoint code and target stack more
difficult.

On top of that, we've already _started_ on i18n, DW_OP_piece, and
refactoring the GNU/Linux code base from that list.  So I think it's
actually pretty likely that the refactoring will get done.  Compare
to multiarching solib and breakpoints on inline functions, neither of
which has had any progress in some years.

Sorry, Jeff.  If we want this feature for GDB 6.4, then we'll have to
get there the normal way.  I think we can do it.

-- 
Daniel Jacobowitz

Re: [RFA]: Watchpoints per thread patch

[Andrew Cagney]

Daniel Jacobowitz wrote:
> On Mon, Nov 08, 2004 at 04:32:23PM -0500, Andrew Cagney wrote:
> 
>>Given our already overcommitted backlog: breakpoints on C++ 
>>constructors, breakpoints on inline code, DW_OP_piece, i18n, multi-arch 
>>solib, ....; how realistic is it that we'll, in addition, manage to both 
>>refactor the linux code base (I know this will be slow as I've been 
>>working on it) and also add multi-threaded watchpoints, all in the 6.4 
>>time frame?
>>
>>Let concentrate on clearing existing backlog, and not add another 
>>promise to the list.
> 
> 
> *sarcasm*
> 
> You're right.  That's an excellent plan.  Let's just drop the
> multithreaded watchpoint patch, then, if it will never make it
> to the front of the backlog.

> *sarcasm off*

Looks like I touched a raw nerve, eh?

Well let me touch another one.  Ask any serious developer trying to use 
GDB and they'll tell you bluntly ``we sux'', and the things I listed 
(along with multi-threaded watchpoints) are why ``we sux''.

Can we sux a lttle less and at least support multi-threaded watchpoints?

--

The obvious solution here is to accept a simplified version of the 
patch, as that way we at least get the feature into 6.4.

Re: [RFA]: Watchpoints per thread patch

[Daniel Jacobowitz]

On Mon, Nov 08, 2004 at 09:19:25PM -0500, Andrew Cagney wrote:
> Daniel Jacobowitz wrote:
> >On Mon, Nov 08, 2004 at 04:32:23PM -0500, Andrew Cagney wrote:
> >
> >>Given our already overcommitted backlog: breakpoints on C++ 
> >>constructors, breakpoints on inline code, DW_OP_piece, i18n, multi-arch 
> >>solib, ....; how realistic is it that we'll, in addition, manage to both 
> >>refactor the linux code base (I know this will be slow as I've been 
> >>working on it) and also add multi-threaded watchpoints, all in the 6.4 
> >>time frame?
> >>
> >>Let concentrate on clearing existing backlog, and not add another 
> >>promise to the list.
> >
> >
> >*sarcasm*
> >
> >You're right.  That's an excellent plan.  Let's just drop the
> >multithreaded watchpoint patch, then, if it will never make it
> >to the front of the backlog.
> 
> >*sarcasm off*
> 
> Looks like I touched a raw nerve, eh?
> 
> Well let me touch another one.  Ask any serious developer trying to use 
> GDB and they'll tell you bluntly ``we sux'', and the things I listed 
> (along with multi-threaded watchpoints) are why ``we sux''.
> 
> Can we sux a lttle less and at least support multi-threaded watchpoints?

Yes, you touched a raw nerve.  You touched a raw nerve where you are
attempting to hold contributions from different contributors to
different standards.  For instance, you blocked vsyscall support for
months because you objected to the quality and design of the code; I
felt it was of satisfactory quality, but you and I already know that we
disagree about many aspects of software design.

I think that Jeff's patch, with your suggested kludge, is of much worse
quality than that was.  It's a gross hack around a mechanism that has
enough existing problems.  I do not want it added to GDB, whose average
quality level is quite bad enough already.

> The obvious solution here is to accept a simplified version of the 
> patch, as that way we at least get the feature into 6.4.

Not at all.  The obvious solution is the same solution we've used
before: for a developer who cares about this feature to put their
attention on the necessary cleanups.  I've already volunteered to
resolve the problem of finding the LWP ID for Jeff. Even if it takes me
a while it'll be plenty of time before 6.4. You're already working on
the GNU/Linux native target vector cleanup; I'll help. If we want to
make GDB into _less_ of a pile of crap then adding _more_ crap code to
it is not the right way to do it!

-- 
Daniel Jacobowitz

Re: [RFA]: Watchpoints per thread patch

[Eli Zaretskii]

> Date: Mon, 8 Nov 2004 21:33:06 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Jeff Johnston <jjohnstn@redhat.com>, Eli Zaretskii <eliz@gnu.org>,
> 	gdb-patches@sources.redhat.com
> 
> The obvious solution is the same solution we've used
> before: for a developer who cares about this feature to put their
> attention on the necessary cleanups.

I agree with this principle.

Re: [RFA]: Watchpoints per thread patch

[Andrew Cagney]

Daniel Jacobowitz wrote:
> On Mon, Nov 08, 2004 at 09:19:25PM -0500, Andrew Cagney wrote:
> 
>>Daniel Jacobowitz wrote:
>>
>>>On Mon, Nov 08, 2004 at 04:32:23PM -0500, Andrew Cagney wrote:
>>>
>>>
>>>>Given our already overcommitted backlog: breakpoints on C++ 
>>>>constructors, breakpoints on inline code, DW_OP_piece, i18n, multi-arch 
>>>>solib, ....; how realistic is it that we'll, in addition, manage to both 
>>>>refactor the linux code base (I know this will be slow as I've been 
>>>>working on it) and also add multi-threaded watchpoints, all in the 6.4 
>>>>time frame?
>>>>
>>>>Let concentrate on clearing existing backlog, and not add another 
>>>>promise to the list.
>>>
>>>
>>>*sarcasm*
>>>
>>>You're right.  That's an excellent plan.  Let's just drop the
>>>multithreaded watchpoint patch, then, if it will never make it
>>>to the front of the backlog.
>>
>>>*sarcasm off*
>>
>>Looks like I touched a raw nerve, eh?
>>
>>Well let me touch another one.  Ask any serious developer trying to use 
>>GDB and they'll tell you bluntly ``we sux'', and the things I listed 
>>(along with multi-threaded watchpoints) are why ``we sux''.
>>
>>Can we sux a lttle less and at least support multi-threaded watchpoints?
> 
> 
> Yes, you touched a raw nerve.  You touched a raw nerve where you are
> attempting to hold contributions from different contributors to
> different standards.  For instance, you blocked vsyscall support for
> months because you objected to the quality and design of the code; I
> felt it was of satisfactory quality, but you and I already know that we
> disagree about many aspects of software design.

Er, my objection to vsyscall was technical.  Attach didn't work then, 
does it work now?  No, so we've got a cludge.

Andrew

Re: [RFA]: Watchpoints per thread patch

[Daniel Jacobowitz]

On Tue, Nov 09, 2004 at 10:10:00AM -0500, Andrew Cagney wrote:
> Daniel Jacobowitz wrote:
> >Yes, you touched a raw nerve.  You touched a raw nerve where you are
> >attempting to hold contributions from different contributors to
> >different standards.  For instance, you blocked vsyscall support for
> >months because you objected to the quality and design of the code; I
> >felt it was of satisfactory quality, but you and I already know that we
> >disagree about many aspects of software design.
> 
> Er, my objection to vsyscall was technical.  Attach didn't work then, 
> does it work now?  No, so we've got a cludge.

My objections to this patch are also technical.  A non-technical
objection would be "I don't like Jeff's hairstyle, so I'm rejecting
this patch".  "This is a terrible abuse of the observer mechanism"
is a technical objection.

As for vsyscall, my memory may be faulty.  It happens to me a lot
lately.  I was thinking of the delay while the observers mechanism was
extended, and the partial xfer mechanism created.  What do you mean by
"attach doesn't work now"?

-- 
Daniel Jacobowitz

Re: [RFA]: Watchpoints per thread patch

[Jeff Johnston]

Daniel Jacobowitz wrote:
> On Mon, Nov 08, 2004 at 09:19:25PM -0500, Andrew Cagney wrote:
> 
>>Daniel Jacobowitz wrote:
>>
>>>On Mon, Nov 08, 2004 at 04:32:23PM -0500, Andrew Cagney wrote:
>>>
>>>
>>>>Given our already overcommitted backlog: breakpoints on C++ 
>>>>constructors, breakpoints on inline code, DW_OP_piece, i18n, multi-arch 
>>>>solib, ....; how realistic is it that we'll, in addition, manage to both 
>>>>refactor the linux code base (I know this will be slow as I've been 
>>>>working on it) and also add multi-threaded watchpoints, all in the 6.4 
>>>>time frame?
>>>>
>>>>Let concentrate on clearing existing backlog, and not add another 
>>>>promise to the list.
>>>
>>>
>>>*sarcasm*
>>>
>>>You're right.  That's an excellent plan.  Let's just drop the
>>>multithreaded watchpoint patch, then, if it will never make it
>>>to the front of the backlog.
>>
>>>*sarcasm off*
>>
>>Looks like I touched a raw nerve, eh?
>>
>>Well let me touch another one.  Ask any serious developer trying to use 
>>GDB and they'll tell you bluntly ``we sux'', and the things I listed 
>>(along with multi-threaded watchpoints) are why ``we sux''.
>>
>>Can we sux a lttle less and at least support multi-threaded watchpoints?
> 
> 
> Yes, you touched a raw nerve.  You touched a raw nerve where you are
> attempting to hold contributions from different contributors to
> different standards.  For instance, you blocked vsyscall support for
> months because you objected to the quality and design of the code; I
> felt it was of satisfactory quality, but you and I already know that we
> disagree about many aspects of software design.
> 
> I think that Jeff's patch, with your suggested kludge, is of much worse
> quality than that was.  It's a gross hack around a mechanism that has
> enough existing problems.  I do not want it added to GDB, whose average
> quality level is quite bad enough already.
> 
> 
>>The obvious solution here is to accept a simplified version of the 
>>patch, as that way we at least get the feature into 6.4.
> 
> 
> Not at all.  The obvious solution is the same solution we've used
> before: for a developer who cares about this feature to put their
> attention on the necessary cleanups.  I've already volunteered to
> resolve the problem of finding the LWP ID for Jeff. Even if it takes me
> a while it'll be plenty of time before 6.4. You're already working on
> the GNU/Linux native target vector cleanup; I'll help. If we want to
> make GDB into _less_ of a pile of crap then adding _more_ crap code to
> it is not the right way to do it!
> 

Time out here for a second.  I have been modifying this patch according to 
"your" comments.  I have had a design that had no observers and one that kept 
the observation isolated to the linux code.

One key issue of my latest patch you seem to object to is the fact that I now 
have to massage the ptid.  This was not necessary in the previous design where I 
was observing within the linux code where the lwp was readily available.  We 
both know the low-level code is fundamentally wrong in its assumption regarding 
the ptids.  They cannot be assumed to be in PID, LWP, 0 format.  We get lucky 
with register accesses only because the thread-db code is flushing registers in 
the lwp format.  It is not documented and when low-level code accesses ptids 
outside of thread-db, it is wrong.  Watchpoints are in the this boat because 
they are accessed by breakpoint.c and infrun.c where the ptid is in the wrong 
format (PID, 0, TID).

I feel your objection to temporarily massaging these ptids as thread-db.c does 
is unreasonable.  We need to think of the end-user.  The amount of code added is 
small and it is trivial to remove this code once the preferred solution is in 
place.  There is currently no work-around to solving thread bugs involving 
memory corruption.

If you have a fix planned soon regarding the ptid format, I have absolutely no 
objection to waiting for it.  However, if you can't get around to this for a 
while due to other commitments or it is going to take some hashing out on the 
list, let's stop punishing the end-user and do something helpful while we work 
things out proper.

-- Jeff J.

Re: [RFA]: Watchpoints per thread patch

[Daniel Jacobowitz]

On Tue, Nov 09, 2004 at 02:06:07PM -0500, Jeff Johnston wrote:
> Time out here for a second.  I have been modifying this patch according to 
> "your" comments.  I have had a design that had no observers and one that 
> kept the observation isolated to the linux code.

The design without observers had plenty of other problems, e.g. it also
broke remote debugging.

My suggestion about putting the observer in add_thread was a bad one.
I've never claimed to be an infallible lord of development.  A
new_thread observer does indeed belong in add_thread, but is not
suitable for your use; and I didn't understand why until later.

We could use a Linux native specific observer, or handle this through
the target stack.  I think handling it through the target stack makes
more sense, but I haven't sketched out what the target method would
look like.  If other GDB developers think that the precedent of a
native-code-only observer isn't a bad one, then maybe we should go back
to your previous placement of the observer and give it a Linux specific
name.  This will be aided by renaming thread-db to be clearly Linux
native code.

> One key issue of my latest patch you seem to object to is the fact that I 
> now have to massage the ptid.  This was not necessary in the previous 
> design where I was observing within the linux code where the lwp was 
> readily available.  We both know the low-level code is fundamentally wrong 
> in its assumption regarding the ptids.  They cannot be assumed to be in 
> PID, LWP, 0 format.  We get lucky with register accesses only because the 
> thread-db code is flushing registers in the lwp format.  It is not 
> documented and when low-level code accesses ptids outside of thread-db, it 
> is wrong.  Watchpoints are in the this boat because they are accessed by 
> breakpoint.c and infrun.c where the ptid is in the wrong format (PID, 0, 
> TID).
> 
> I feel your objection to temporarily massaging these ptids as thread-db.c 
> does is unreasonable.  We need to think of the end-user.  The amount of 
> code added is small and it is trivial to remove this code once the 
> preferred solution is in place.  There is currently no work-around to 
> solving thread bugs involving memory corruption.
> 
> If you have a fix planned soon regarding the ptid format, I have absolutely 
> no objection to waiting for it.  However, if you can't get around to this 
> for a while due to other commitments or it is going to take some hashing 
> out on the list, let's stop punishing the end-user and do something helpful 
> while we work things out proper.

Jeff, I've already posted the thread-db change to make this
unnecessary.  I was asked to rename the file first, and I've posted
that too.

-- 
Daniel Jacobowitz

Re: [RFA]: Watchpoints per thread patch

[Jim Blandy]

> > If you have a fix planned soon regarding the ptid format, I have absolutely 
> > no objection to waiting for it.  However, if you can't get around to this 
> > for a while due to other commitments or it is going to take some hashing 
> > out on the list, let's stop punishing the end-user and do something helpful 
> > while we work things out proper.
> 
> Jeff, I've already posted the thread-db change to make this
> unnecessary.  I was asked to rename the file first, and I've posted
> that too.

For those of us trying desperately to keep score, may I ask for a
recap of Our Story Thus Far?

- What are the specific pieces of work needed to do this The Right Way?
- Which have been done already, or at least posted?
- How long will the rest take?
- Who has volunteered to do them?

I'd like to hear Daniel's, Andrew's, and Jeff's answers.

Re: [RFA]: Watchpoints per thread patch

[Jeff Johnston]

Daniel Jacobowitz wrote:
> On Tue, Nov 09, 2004 at 02:06:07PM -0500, Jeff Johnston wrote:
> 
>>Time out here for a second.  I have been modifying this patch according to 
>>"your" comments.  I have had a design that had no observers and one that 
>>kept the observation isolated to the linux code.
> 
> 
> The design without observers had plenty of other problems, e.g. it also
> broke remote debugging.
> 
> My suggestion about putting the observer in add_thread was a bad one.
> I've never claimed to be an infallible lord of development.  A
> new_thread observer does indeed belong in add_thread, but is not
> suitable for your use; and I didn't understand why until later.
> 

Fair enough, but it was certainly uncalled for to categorize my patch(s) as
"crap" as I was attempting to implement your suggested changes.

> We could use a Linux native specific observer, or handle this through
> the target stack.  I think handling it through the target stack makes
> more sense, but I haven't sketched out what the target method would
> look like.  If other GDB developers think that the precedent of a
> native-code-only observer isn't a bad one, then maybe we should go back
> to your previous placement of the observer and give it a Linux specific
> name.  This will be aided by renaming thread-db to be clearly Linux
> native code.
>

Ok.  I'll wait and see if anybody else has objections.  Eli, I understand that I 
would have to rename the observer and change its definition appropriately.

> 
>>One key issue of my latest patch you seem to object to is the fact that I 
>>now have to massage the ptid.  This was not necessary in the previous 
>>design where I was observing within the linux code where the lwp was 
>>readily available.  We both know the low-level code is fundamentally wrong 
>>in its assumption regarding the ptids.  They cannot be assumed to be in 
>>PID, LWP, 0 format.  We get lucky with register accesses only because the 
>>thread-db code is flushing registers in the lwp format.  It is not 
>>documented and when low-level code accesses ptids outside of thread-db, it 
>>is wrong.  Watchpoints are in the this boat because they are accessed by 
>>breakpoint.c and infrun.c where the ptid is in the wrong format (PID, 0, 
>>TID).
>>
>>I feel your objection to temporarily massaging these ptids as thread-db.c 
>>does is unreasonable.  We need to think of the end-user.  The amount of 
>>code added is small and it is trivial to remove this code once the 
>>preferred solution is in place.  There is currently no work-around to 
>>solving thread bugs involving memory corruption.
>>
>>If you have a fix planned soon regarding the ptid format, I have absolutely 
>>no objection to waiting for it.  However, if you can't get around to this 
>>for a while due to other commitments or it is going to take some hashing 
>>out on the list, let's stop punishing the end-user and do something helpful 
>>while we work things out proper.
> 
> 
> Jeff, I've already posted the thread-db change to make this
> unnecessary.  I was asked to rename the file first, and I've posted
> that too.
>

Ok, my misunderstanding.  Like I said, if something is currently in the works, I 
have no problem in waiting.

-- Jeff J.

Re: [RFA]: Watchpoints per thread patch

[Daniel Jacobowitz]

On Tue, Nov 09, 2004 at 03:48:32PM -0500, Jeff Johnston wrote:
> Daniel Jacobowitz wrote:
> >On Tue, Nov 09, 2004 at 02:06:07PM -0500, Jeff Johnston wrote:
> >
> >>Time out here for a second.  I have been modifying this patch according 
> >>to "your" comments.  I have had a design that had no observers and one 
> >>that kept the observation isolated to the linux code.
> >
> >
> >The design without observers had plenty of other problems, e.g. it also
> >broke remote debugging.
> >
> >My suggestion about putting the observer in add_thread was a bad one.
> >I've never claimed to be an infallible lord of development.  A
> >new_thread observer does indeed belong in add_thread, but is not
> >suitable for your use; and I didn't understand why until later.
> >
> 
> Fair enough, but it was certainly uncalled for to categorize my patch(s) as
> "crap" as I was attempting to implement your suggested changes.

I think it was entirely called for; it just reflected as much upon my
suggestion as on your patch.  I apologize for giving bad advice.

-- 
Daniel Jacobowitz

Re: [RFA]: Watchpoints per thread patch

[Jeff Johnston]

Jim Blandy wrote:
>>>If you have a fix planned soon regarding the ptid format, I have absolutely 
>>>no objection to waiting for it.  However, if you can't get around to this 
>>>for a while due to other commitments or it is going to take some hashing 
>>>out on the list, let's stop punishing the end-user and do something helpful 
>>>while we work things out proper.
>>
>>Jeff, I've already posted the thread-db change to make this
>>unnecessary.  I was asked to rename the file first, and I've posted
>>that too.
> 
> 
> For those of us trying desperately to keep score, may I ask for a
> recap of Our Story Thus Far?
> 
> - What are the specific pieces of work needed to do this The Right Way?
> - Which have been done already, or at least posted?
> - How long will the rest take?
> - Who has volunteered to do them?
> 
> I'd like to hear Daniel's, Andrew's, and Jeff's answers.
> 
>

Jim,

   I think this is settled now.  I am waiting for Daniel's patch to ptids to be 
posted and accepted.  Then, I will modify my previous patch over top of this.

-- Jeff J.

Re: [RFA]: Watchpoints per thread patch

[Jim Blandy]

Jeff Johnston <jjohnstn@redhat.com> writes:

> Jim Blandy wrote:
> >>> If you have a fix planned soon regarding the ptid format, I have
> >>> absolutely no objection to waiting for it.  However, if you can't
> >>> get around to this for a while due to other commitments or it is
> >>> going to take some hashing out on the list, let's stop punishing
> >>> the end-user and do something helpful while we work things out
> >>> proper.
> >>
> >>Jeff, I've already posted the thread-db change to make this
> >>unnecessary.  I was asked to rename the file first, and I've posted
> >>that too.
> > For those of us trying desperately to keep score, may I ask for a
> > recap of Our Story Thus Far?
> > - What are the specific pieces of work needed to do this The Right
> > Way?
> > - Which have been done already, or at least posted?
> > - How long will the rest take?
> > - Who has volunteered to do them?
> > I'd like to hear Daniel's, Andrew's, and Jeff's answers.
> >
> 
> Jim,
> 
>    I think this is settled now.  I am waiting for Daniel's patch to
> ptids to be posted and accepted.  Then, I will modify my previous
> patch over top of this.

Okay, that's how it looked to me, too.  Thanks.

Re: [RFA]: Watchpoints per thread patch

[Eli Zaretskii]

> Date: Tue, 9 Nov 2004 14:31:24 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Andrew Cagney <cagney@gnu.org>, Eli Zaretskii <eliz@gnu.org>,
> 	gdb-patches@sources.redhat.com
> 
> We could use a Linux native specific observer, or handle this through
> the target stack.  I think handling it through the target stack makes
> more sense, but I haven't sketched out what the target method would
> look like.  If other GDB developers think that the precedent of a
> native-code-only observer isn't a bad one, then maybe we should go back
> to your previous placement of the observer and give it a Linux specific
> name.

Is there any significant difference between native-code-only observers
and the other kind?  Could you elaborate?

Anyway, I said in the past several times that I don't really like to
use the observers too much.  The reason for that is that with a
mechanism such as this, which is like hooks in Emacs or interrupt
handlers in the old DOS days, you often get in trouble once more than
one observer is hooked to some event: the order of the the observers'
invocation might matter, and AFAIK we do not have any way to control
that (nor would we know what order is ``right'', even if we had a way
to control it).

So in general, if there's a reasonably good design that avoids using
observers, I'd favor that.

Re: [RFA]: Watchpoints per thread patch

[Eli Zaretskii]

> Date: Tue, 09 Nov 2004 15:48:32 -0500
> From: Jeff Johnston <jjohnstn@redhat.com>
> Cc: Andrew Cagney <cagney@gnu.org>, Eli Zaretskii <eliz@gnu.org>,
>         gdb-patches@sources.redhat.com
> 
> > We could use a Linux native specific observer, or handle this through
> > the target stack.  I think handling it through the target stack makes
> > more sense, but I haven't sketched out what the target method would
> > look like.  If other GDB developers think that the precedent of a
> > native-code-only observer isn't a bad one, then maybe we should go back
> > to your previous placement of the observer and give it a Linux specific
> > name.  This will be aided by renaming thread-db to be clearly Linux
> > native code.
> >
> 
> Ok.  I'll wait and see if anybody else has objections.  Eli, I understand that I 
> would have to rename the observer and change its definition appropriately.

Yes, if you decide to modify the observer's name to a
GNU/Linux-specific one.

Re: [RFA]: Watchpoints per thread patch

[Jeff Johnston]

Eli Zaretskii wrote:
>>Date: Tue, 09 Nov 2004 15:48:32 -0500
>>From: Jeff Johnston <jjohnstn@redhat.com>
>>Cc: Andrew Cagney <cagney@gnu.org>, Eli Zaretskii <eliz@gnu.org>,
>>        gdb-patches@sources.redhat.com
>>
>>
>>>We could use a Linux native specific observer, or handle this through
>>>the target stack.  I think handling it through the target stack makes
>>>more sense, but I haven't sketched out what the target method would
>>>look like.  If other GDB developers think that the precedent of a
>>>native-code-only observer isn't a bad one, then maybe we should go back
>>>to your previous placement of the observer and give it a Linux specific
>>>name.  This will be aided by renaming thread-db to be clearly Linux
>>>native code.
>>>
>>
>>Ok.  I'll wait and see if anybody else has objections.  Eli, I understand that I 
>>would have to rename the observer and change its definition appropriately.
> 
> 
> Yes, if you decide to modify the observer's name to a
> GNU/Linux-specific one.
> 

Yes, that is what I mean to do.

-- Jeff J.

Re: [RFA]: Watchpoints per thread patch

[Andrew Cagney]

> As for vsyscall, my memory may be faulty.  It happens to me a lot
> lately.  I was thinking of the delay while the observers mechanism was
> extended, and the partial xfer mechanism created.  What do you mean by
> "attach doesn't work now"?

        /* FIXME: cagney/2004-05-06: Should not require an existing
           BFD when trying to create a run-time BFD of the VSYSCALL
           page in the inferior.  Unfortunately that's the current
           interface so for the moment bail.  Introducing a
           ``bfd_runtime'' (a BFD created using the loaded image) file
           format should fix this.  */
         {
           warning ("could not load vsyscall page because no executable 
was specified");
           warning ("try using the \"file\" command first");
           return;
         }

Re: [RFA]: Watchpoints per thread patch

[Andrew Cagney]

> Jim,
> 
>   I think this is settled now.  I am waiting for Daniel's patch to ptids 
> to be posted and accepted.  Then, I will modify my previous patch over 
> top of this.

Thanks.

Andrew

Re: [RFA]: Watchpoints per thread patch

[Ulrich Weigand]

Jeff Johnston wrote:

>        (s390_remove_one_watchpoint): New function.
>        (s390_remove_watchpoint_callback): Ditto.
>        (s390_remove_watchpoint): Change to iterate through lwps and
>        remove the specified watchpoint for each thread.
>        (s390_insert_one_watchpoint): New function.
>        (s390_insert_watchpoint_callback): Ditto.
>        (s390_insert_watchpoint): Change to iterate through lwps and
>        insert the specified watchpoint on each thread.

Note that s390_insert/remove_watchpoint simply manipulate a global
data structure holding current watch points; only the routine
s390_fix_watch_points actually changes per-thread kernel state.

So it would appear simpler to only call s390_fix_watch_points
in a per-thread loop, and leave the higher-level functions alone.
(When a new thread attaches, you only need to run s390_fix_watch_points
on the new thread as well.)


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