Re: [rfc, rfa/doc] Multi-threaded watchpoint improvements

[Eli Zaretskii <eliz at gnu dot org>]

> Date: Sun, 16 Sep 2007 14:39:49 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> I have (finally, sorry for the delay) finished revamping the
> multi-threaded watchpoint patches Jeff submitted ages ago and Luis
> resubmitted recently.  This message contains all the changes except
> for various native GNU/Linux target files.  It includes manual and
> gdbint changes describing what I've figured out to date.

Thanks (and sorry for the delay in reviewing: it was a busy week).

> Do these changes look OK?

The changes to the manuals are okay with me, except for the following:

> --- doc/gdbint.texinfo	5 Jul 2007 12:22:32 -0000	1.266
> +++ doc/gdbint.texinfo	16 Sep 2007 18:05:32 -0000
> @@ -660,15 +660,21 @@ section is mostly irrelevant for softwar
>  
>  When the inferior stops, @value{GDBN} tries to establish, among other
>  possible reasons, whether it stopped due to a watchpoint being hit.
> -For a data-write watchpoint, it does so by evaluating, for each
> -watchpoint, the expression whose value is being watched, and testing
> -whether the watched value has changed.  For data-read and data-access
> -watchpoints, @value{GDBN} needs the target to supply a primitive that
> -returns the address of the data that was accessed or read (see the
> -description of @code{target_stopped_data_address} below): if this
> -primitive returns a valid address, @value{GDBN} infers that a
> -watchpoint triggered if it watches an expression whose evaluation uses
> -that address.
> +It first uses @code{STOPPED_BY_WATCHPOINT} to see if any watchpoint
> +was hit.  If not, all watchpoint checking is skipped.
> +
> +Then @value{GDBN} calls @code{target_stopped_data_address} exactly
> +once.  This method returns the address of the watchpoint which
> +triggered, if the target can determine it.  The returned address is
> +compared to each watched memory address in each active watchpoint.
> +Data-read and data-access watchpoints rely on this; if the triggered
> +address is not available, read and access watchpoints will not work.
> +If the triggered address is available, @value{GDBN} will only check
> +data-write watchpoints which match the triggered address.  If it is
> +not available, @value{GDBN} will consider all data-write watchpoints.
> +For each data-write watchpoint that @value{GDBN} considers, it does so
> +by evaluating the expression whose value is being watched, and testing
> +whether the watched value has changed.

The old text clearly separated the description of what GDB does for
data-write watchpoints from what it does for date-read/data-access
watchpoints.  The new text confuses things, because it doesn't keep
that separation.  I suggest to rephrase the last paragraph as follows:

  Then @value{GDBN} calls @code{target_stopped_data_address} exactly
  once.  This method returns the address of the watchpoint which
  triggered, if the target can determine it.  If the triggered address
  is available, @value{GDBN} compares the address returned by this
  method with each watched memory address in each active watchpoint.
  For data-read and data-access watchpoints, @value{GDBN} announces
  every watchpoint that watches the triggered address as being hit.
  For this reason, data-read and data-access watchpoints
  @emph{require} that the triggered address be available; if not, read
  and access watchpoints will never be considered hit.  For data-write
  watchpoints, if the triggered address is available, @value{GDBN}
  considers only those watchpoints which match that address;
  otherwise, @value{GDBN} considers all data-write watchpoints.  For
  each data-write watchpoint that @value{GDBN} considers, it evaluates
  the expression whose value is being watched, and tests whether the
  watched value has changed.  Watchpoints whose watched values has
  changed are announced as hit.

Also, I don't understand the purpose of this sentence:

> +@value{GDBN} only supports process-wide watchpoints.

"Process-wide watchpoints'' as opposed to what?

The text then goes on like this:

>                                                       If the target
> +supports threads and per-thread debug registers, it should set the
> +per-thread debug registers for all threads to the same value.  On
> +@sc{gnu}/Linux native targets, this is accomplished by using
> +@code{ALL_LWPS} in @code{target_insert_watchpoint} and
> +@code{target_remove_watchpoint} and by using @code{linux_set_new_thread}
> +to register a handler for newly created threads.

Is this perhaps the opposite of ``process-wide watchpoints''?  If so,
it sounds like a contradiction: first you say we don't support
per-thread watchpoints, then you say we do (for some platforms).

What am I missing here?