Re: [RFA]: Watchpoints per thread patch

Eli Zaretskii
2004-10-28T04:55:15Z

> 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}.