Eli Zaretskii wrote: >>Date: Wed, 27 Oct 2004 18:36:14 -0400 >>From: Jeff Johnston >>Cc: Andrew Cagney , 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 *); >> >>+ >>+/* 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 * 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 * observer.texi (new_thread): New observer.