ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)

ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)

[Philippe Waroquiers]

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

 
>> These are fine, but please leave 2 spaces between sentences in the
>> NEWS entry.
> I assume there will be some other comments (maybe related to ssize_t).
> I will update NEWS together with handling the other comments.

There was no other comments, so attached a new version of the patch,
only difference is 2 spaces in sentences in NEWS entry.

Thanks

Philippe

[-- Attachment #2: set-length-limit-4.txt --]
[-- Type: text/plain, Size: 4824 bytes --]

Index: gdb/NEWS
===================================================================
RCS file: /cvs/src/src/gdb/NEWS,v
retrieving revision 1.441
diff -c -p -r1.441 NEWS
*** gdb/NEWS	13 May 2011 22:36:06 -0000	1.441
--- gdb/NEWS	21 May 2011 19:48:36 -0000
***************
*** 3,8 ****
--- 3,18 ----
  
  *** Changes since GDB 7.3
  
+ * GDB has two new commands: "set remote hardware-watchpoint-length-limit"
+   and "show remote hardware-watchpoint-length-limit".  These allows to
+   set or show the maximum length limit (in bytes) of a remote
+   target hardware watchpoint.
+ 
+   This allows e.g. to use "unlimited" hardware watchpoints with the
+   gdbserver integrated in Valgrind version >= 3.7.0.  Such Valgrind
+   watchpoints are slower than real hardware watchpoints but are
+   significantly faster than gdb software watchpoints.
+ 
  * libthread-db-search-path now supports two special values: $sdir and $pdir.
    $sdir specifies the default system locations of shared libraries.
    $pdir specifies the directory where the libpthread used by the application
Index: gdb/remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.446
diff -c -p -r1.446 remote.c
*** gdb/remote.c	12 May 2011 12:09:16 -0000	1.446
--- gdb/remote.c	21 May 2011 19:48:42 -0000
*************** remote_remove_watchpoint (CORE_ADDR addr
*** 7756,7764 ****
--- 7756,7778 ----
  
  
  int remote_hw_watchpoint_limit = -1;
+ int remote_hw_watchpoint_length_limit = -1;
  int remote_hw_breakpoint_limit = -1;
  
  static int
+ remote_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
+ {
+   if (remote_hw_watchpoint_length_limit == 0)
+     return 0;
+   else if (remote_hw_watchpoint_length_limit < 0)
+     return 1;
+   else if (len <= remote_hw_watchpoint_length_limit)
+     return 1;
+   else
+     return 0;
+ }
+ 
+ static int
  remote_check_watch_resources (int type, int cnt, int ot)
  {
    if (type == bp_hardware_breakpoint)
*************** Specify the serial device it is connecte
*** 10326,10331 ****
--- 10340,10347 ----
    remote_ops.to_can_use_hw_breakpoint = remote_check_watch_resources;
    remote_ops.to_insert_hw_breakpoint = remote_insert_hw_breakpoint;
    remote_ops.to_remove_hw_breakpoint = remote_remove_hw_breakpoint;
+   remote_ops.to_region_ok_for_hw_watchpoint
+      = remote_region_ok_for_hw_watchpoint;
    remote_ops.to_insert_watchpoint = remote_insert_watchpoint;
    remote_ops.to_remove_watchpoint = remote_remove_watchpoint;
    remote_ops.to_kill = remote_kill;
*************** Specify a negative limit for unlimited."
*** 10735,10740 ****
--- 10751,10765 ----
  					   number of target hardware
  					   watchpoints is %s.  */
  			    &remote_set_cmdlist, &remote_show_cmdlist);
+   add_setshow_zinteger_cmd ("hardware-watchpoint-length-limit", no_class,
+ 			    &remote_hw_watchpoint_length_limit, _("\
+ Set the maximum length (in bytes) of a target hardware watchpoint."), _("\
+ Show the maximum length (in bytes) of a target hardware watchpoint."), _("\
+ Specify a negative limit for unlimited."),
+ 			    NULL, NULL, /* FIXME: i18n: The maximum
+                                            length (in bytes) of a target
+                                            hardware watchpoint is %s.  */
+ 			    &remote_set_cmdlist, &remote_show_cmdlist);
    add_setshow_zinteger_cmd ("hardware-breakpoint-limit", no_class,
  			    &remote_hw_breakpoint_limit, _("\
  Set the maximum number of target hardware breakpoints."), _("\
Index: gdb/doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.838
diff -c -p -r1.838 gdb.texinfo
*** gdb/doc/gdb.texinfo	13 May 2011 22:36:07 -0000	1.838
--- gdb/doc/gdb.texinfo	21 May 2011 19:49:12 -0000
*************** responses.
*** 16578,16583 ****
--- 16578,16595 ----
  Restrict @value{GDBN} to using @var{limit} remote hardware breakpoint or
  watchpoints.  A limit of -1, the default, is treated as unlimited.
  
+ @cindex limit hardware watchpoints length
+ @cindex remote target, limit watchpoints length
+ @anchor{set remote hardware-watchpoint-length-limit}
+ @item set remote hardware-watchpoint-length-limit @var{limit}
+ Restrict @value{GDBN} to using @var{limit} bytes for the maximum length of
+ a remote hardware watchpoint.  A limit of -1, the default, is treated
+ as unlimited.
+ 
+ @item show remote hardware-watchpoint-length-limit
+ Show the current limit (in bytes) of the maximum length of
+ a remote hardware watchpoint.
+ 
  @item set remote exec-file @var{filename}
  @itemx show remote exec-file
  @anchor{set remote exec-file}

Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)

[Tom Tromey]

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> There was no other comments, so attached a new version of the patch,
Philippe> only difference is 2 spaces in sentences in NEWS entry.

Do you have copyright papers on file?  If not, we will need them for
this patch.  Send me email off-list and I will get you started.  (If we
already did this and I forgot, apologies, I don't always find out about
assignments.)

The patch itself seems reasonable to me.  I am not overly familiar with
remote.c, but the new setting seems in keeping with other settings there.

Tom

Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)

[Yao Qi]

On 05/22/2011 06:20 AM, Philippe Waroquiers wrote:

I am not the people to approve or reject this patch.

>  
>  *** Changes since GDB 7.3
>  
> + * GDB has two new commands: "set remote hardware-watchpoint-length-limit"
> +   and "show remote hardware-watchpoint-length-limit".  These allows to
> +   set or show the maximum length limit (in bytes) of a remote
> +   target hardware watchpoint.
> + +   This allows e.g. to use "unlimited" hardware watchpoints with the
    ^ Looks there is a surplus '+'.

> +   gdbserver integrated in Valgrind version >= 3.7.0.  Such Valgrind
> +   watchpoints are slower than real hardware watchpoints but are
> +   significantly faster than gdb software watchpoints.

If I understand you correctly, this commands are only useful in
gdbserver+valgrind.  If gdb is running in normal remote target, using
`set remote hardware-watchpoint-length-limit' will make GDB work
incorrectly.

Maybe, we need a new target `remote-valgrind' here, and move your stuff
there.

> +  * libthread-db-search-path now supports two special values: $sdir and
> $pdir.

This line of change above is not about your patch.

> + + static int
    ^ a surplus '+'.


> Index: gdb/doc/gdb.texinfo
> ===================================================================
> RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
> retrieving revision 1.838
> diff -c -p -r1.838 gdb.texinfo
> *** gdb/doc/gdb.texinfo    13 May 2011 22:36:07 -0000    1.838
> --- gdb/doc/gdb.texinfo    21 May 2011 19:49:12 -0000
> *************** responses.
> *** 16578,16583 ****
> --- 16578,16595 ----
>  Restrict @value{GDBN} to using @var{limit} remote hardware breakpoint or
>  watchpoints.  A limit of -1, the default, is treated as unlimited.
>  
> + @cindex limit hardware watchpoints length
> + @cindex remote target, limit watchpoints length
> + @anchor{set remote hardware-watchpoint-length-limit}
> + @item set remote hardware-watchpoint-length-limit @var{limit}
> + Restrict @value{GDBN} to using @var{limit} bytes for the maximum
> length of
> + a remote hardware watchpoint.  A limit of -1, the default, is treated
> + as unlimited.
> + + @item show remote hardware-watchpoint-length-limit
    ^ a surplus '+'.

-- 
Yao (齐尧)

Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> Maybe, we need a new target `remote-valgrind' here, and move your
Yao> stuff there.

We discussed this a bit (last year?), but Pedro was against adding a new
target.  I don't recall why; I would like to know though.

Tom

Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)

[Pedro Alves]

On Friday 27 May 2011 18:53:19, Tom Tromey wrote:
> >>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:
> 
> Yao> Maybe, we need a new target `remote-valgrind' here, and move your
> Yao> stuff there.
> 
> We discussed this a bit (last year?), but Pedro was against adding a new
> target.  I don't recall why; I would like to know though.

It was in a different context (some target where endianess matters
depending on whether you're reading code or something else), but the
reasons are the same.  There's no need for one, and it adds to
user confusion, and IDE complication.  If the remote target needs
to behave differently against some remote stub, that calls 
for the remote end giving gdb enough information for gdb to 
adjust itself automatically.

I can't say I understand why was that being proposed in this case?
What is the patch breaking?

-- 
Pedro Alves

Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)

[Philippe Waroquiers]

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

On Fri, 27 May 2011 11:24:31 +0800 Yao Qi  wrote

> If I understand you correctly, this commands are only useful in
> gdbserver+valgrind.  If gdb is running in normal remote target, using
> `set remote hardware-watchpoint-length-limit' will make GDB work
> incorrectly.

Interesting remark. Waiting for the legal papers to be signed, it made
me work a little bit more, and allowed me to find a bug in the current
gdbserver hw watchpoint handling on x86 :
    linux-x86-low.c:511: A problem internal to GDBserver has been detected.
   Assertion `DR_FIRSTADDR <= regnum && regnum < DR_LASTADDR' failed.
I am attaching a new version of the patch which fixes this problem
+ attaching the test program.

The patch is supposed to be useful also for 'non Valgrind' gdbserver.

I think the current behaviour is inconsistent (whatever the gdbserver)
and the patch is making it more consistent.

Currently: the user can configure the nr of remote hw watchpoints
(default unlimited). GDB tries to place the watchpoints, and if it
fails, the user can then decide what to do (e.g. delete some watchpoints,
switch to software watchpoints, etc).
For a too long watchpoint, GDB silently switches to software watchpoints, which is
I find surprising : it will then run (hundreds of times?) slower without warning.

With the patch, a too long watchpoint will be handled similarly to too many
watchpoints. The user can then decide what to do for too long watchpoints
e.g. switch to software watchpoints, disable or delete some watchpoints, ...

For what concerns the bug: the problem is that the code in i386-low.c
can partially place a watchpoint, but to the contrary of i386-nat.c,
such a partial watchpoint will not be rolled back by breakpoint.c
The patch fixes this (as this bug is much easier to trigger with long
breakpoints which are accepted by gdb, but have to be rolled back
by gdbserver). 
To reproduce the bug, compile the attached s.c, and do the following:
      gdbserver :1234 ./s
gdb ./s
tar rem :1234
set breakpoint always-inserted on
watch s1
watch s2
watch s4
watch s3
del
y
break s.c:24
c
p p = &s3
c
        linux-x86-low.c:511: A problem internal to GDBserver has been detected.
        Assertion `DR_FIRSTADDR <= regnum && regnum < DR_LASTADDR' failed.

If you enable remote hw points debugging, you will see that after having deleted all
watchpoints, some debug registers are still set.
The patch fixes this by adding a rollback.

Question: is there somewhere a 'how to test gdbserver for dummies' ?
I read the gdb internal doc, and could only find a reference to
setting GDBSERVER=... variable when doing make check, but that does
not run the gdbserver tests (I believe).
I took a look at dejagnu documentation, but that is not for a dummy like me :).
In the meantime, I have only done limited manual tests.

Philippe


[-- Attachment #2: s.c --]
[-- Type: application/octet-stream, Size: 516 bytes --]

char s1[1]; char sep1[5000];
char s2[2]; char sep2[5000];
char s3[3]; char sep3[5000];
char s4[4]; char sep4[5000];
char s5[5]; char sep5[5000];
char s6[6]; char sep6[5000];
char s7[7]; char sep7[5000];
char s8[8]; char sep8[5000];

char s16[16]; char sep16[5000];

char s32[32]; char sep32[5000];

char s64[64]; char sep64[5000];

char s128[128]; char sep128[5000];

char s1000[1000]; char spe1[5000];
main()
{
   int i;
   char * p = s1000;
   for (i = 0; i < 1000; i++)
      p[i] = 1;
}

[-- Attachment #3: set-lenght-limit-5.txt --]
[-- Type: text/plain, Size: 7342 bytes --]

Index: gdb/NEWS
===================================================================
RCS file: /cvs/src/src/gdb/NEWS,v
retrieving revision 1.441
diff -c -p -r1.441 NEWS
*** gdb/NEWS	13 May 2011 22:36:06 -0000	1.441
--- gdb/NEWS	29 May 2011 12:39:19 -0000
***************
*** 3,8 ****
--- 3,18 ----
  
  *** Changes since GDB 7.3
  
+ * GDB has two new commands: "set remote hardware-watchpoint-length-limit"
+   and "show remote hardware-watchpoint-length-limit".  These allows to
+   set or show the maximum length limit (in bytes) of a remote
+   target hardware watchpoint.
+ 
+   This allows e.g. to use "unlimited" hardware watchpoints with the
+   gdbserver integrated in Valgrind version >= 3.7.0.  Such Valgrind
+   watchpoints are slower than real hardware watchpoints but are
+   significantly faster than gdb software watchpoints.
+ 
  * libthread-db-search-path now supports two special values: $sdir and $pdir.
    $sdir specifies the default system locations of shared libraries.
    $pdir specifies the directory where the libpthread used by the application
Index: gdb/remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.446
diff -c -p -r1.446 remote.c
*** gdb/remote.c	12 May 2011 12:09:16 -0000	1.446
--- gdb/remote.c	29 May 2011 12:39:20 -0000
*************** remote_remove_watchpoint (CORE_ADDR addr
*** 7756,7764 ****
--- 7756,7778 ----
  
  
  int remote_hw_watchpoint_limit = -1;
+ int remote_hw_watchpoint_length_limit = -1;
  int remote_hw_breakpoint_limit = -1;
  
  static int
+ remote_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
+ {
+   if (remote_hw_watchpoint_length_limit == 0)
+     return 0;
+   else if (remote_hw_watchpoint_length_limit < 0)
+     return 1;
+   else if (len <= remote_hw_watchpoint_length_limit)
+     return 1;
+   else
+     return 0;
+ }
+ 
+ static int
  remote_check_watch_resources (int type, int cnt, int ot)
  {
    if (type == bp_hardware_breakpoint)
*************** Specify the serial device it is connecte
*** 10326,10331 ****
--- 10340,10347 ----
    remote_ops.to_can_use_hw_breakpoint = remote_check_watch_resources;
    remote_ops.to_insert_hw_breakpoint = remote_insert_hw_breakpoint;
    remote_ops.to_remove_hw_breakpoint = remote_remove_hw_breakpoint;
+   remote_ops.to_region_ok_for_hw_watchpoint
+      = remote_region_ok_for_hw_watchpoint;
    remote_ops.to_insert_watchpoint = remote_insert_watchpoint;
    remote_ops.to_remove_watchpoint = remote_remove_watchpoint;
    remote_ops.to_kill = remote_kill;
*************** Specify a negative limit for unlimited."
*** 10735,10740 ****
--- 10751,10765 ----
  					   number of target hardware
  					   watchpoints is %s.  */
  			    &remote_set_cmdlist, &remote_show_cmdlist);
+   add_setshow_zinteger_cmd ("hardware-watchpoint-length-limit", no_class,
+ 			    &remote_hw_watchpoint_length_limit, _("\
+ Set the maximum length (in bytes) of a target hardware watchpoint."), _("\
+ Show the maximum length (in bytes) of a target hardware watchpoint."), _("\
+ Specify a negative limit for unlimited."),
+ 			    NULL, NULL, /* FIXME: i18n: The maximum
+                                            length (in bytes) of a target
+                                            hardware watchpoint is %s.  */
+ 			    &remote_set_cmdlist, &remote_show_cmdlist);
    add_setshow_zinteger_cmd ("hardware-breakpoint-limit", no_class,
  			    &remote_hw_breakpoint_limit, _("\
  Set the maximum number of target hardware breakpoints."), _("\
Index: gdb/doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.840
diff -c -p -r1.840 gdb.texinfo
*** gdb/doc/gdb.texinfo	17 May 2011 13:29:38 -0000	1.840
--- gdb/doc/gdb.texinfo	29 May 2011 12:39:24 -0000
*************** responses.
*** 16578,16583 ****
--- 16578,16595 ----
  Restrict @value{GDBN} to using @var{limit} remote hardware breakpoint or
  watchpoints.  A limit of -1, the default, is treated as unlimited.
  
+ @cindex limit hardware watchpoints length
+ @cindex remote target, limit watchpoints length
+ @anchor{set remote hardware-watchpoint-length-limit}
+ @item set remote hardware-watchpoint-length-limit @var{limit}
+ Restrict @value{GDBN} to using @var{limit} bytes for the maximum length of
+ a remote hardware watchpoint.  A limit of -1, the default, is treated
+ as unlimited.
+ 
+ @item show remote hardware-watchpoint-length-limit
+ Show the current limit (in bytes) of the maximum length of
+ a remote hardware watchpoint.
+ 
  @item set remote exec-file @var{filename}
  @itemx show remote exec-file
  @anchor{set remote exec-file}
Index: gdb/gdbserver/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/ChangeLog,v
retrieving revision 1.483
diff -c -p -r1.483 ChangeLog
*** gdb/gdbserver/ChangeLog	26 May 2011 15:49:25 -0000	1.483
--- gdb/gdbserver/ChangeLog	29 May 2011 12:39:24 -0000
***************
*** 1,3 ****
--- 1,10 ----
+ 2011-05-29  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
+ 
+ 	* i386-low.c (i386_low_insert_watchpoint): rollback a partially
+ 	inserted hw watchpoint. In i386-nat.c, this rollback is ensured
+ 	by breakpoint.c In i386-low.c (copied from i386-nat.c), the
+ 	rollback must be done by gdbserver.
+ 
  2011-05-26  Yao Qi  <yao@codesourcery.com>
  
  	* Makefile.in (thread-db.o): Track dependence to
Index: gdb/gdbserver/i386-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/i386-low.c,v
retrieving revision 1.6
diff -c -p -r1.6 i386-low.c
*** gdb/gdbserver/i386-low.c	27 Feb 2011 21:33:10 -0000	1.6
--- gdb/gdbserver/i386-low.c	29 May 2011 12:39:24 -0000
*************** Invalid value %d of operation in i386_ha
*** 412,418 ****
  	     in which case we just increment the reference counts of
  	     occupied debug registers).  If we break out of the loop
  	     too early, we could cause those addresses watched by
! 	     other watchpoints to be disabled when breakpoint.c reacts
  	     to our failure to insert this watchpoint and tries to
  	     remove it.  */
  	  if (status)
--- 412,418 ----
  	     in which case we just increment the reference counts of
  	     occupied debug registers).  If we break out of the loop
  	     too early, we could cause those addresses watched by
! 	     other watchpoints to be disabled when our caller reacts
  	     to our failure to insert this watchpoint and tries to
  	     remove it.  */
  	  if (status)
*************** i386_low_insert_watchpoint (struct i386_
*** 468,473 ****
--- 468,481 ----
      {
        retval = i386_handle_nonaligned_watchpoint (state, WP_INSERT,
  						  addr, len, type);
+       if (retval)
+          {
+             if (debug_hw_points)
+                i386_show_dr (state, "rolling back FAILED insert_watchpoint",
+                              addr, len, type);
+             i386_handle_nonaligned_watchpoint (state, WP_REMOVE,
+                                                addr, len, type);
+          }
      }
    else
      {

Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)

[Yao Qi]

On 05/28/2011 01:58 AM, Pedro Alves wrote:
> On Friday 27 May 2011 18:53:19, Tom Tromey wrote:
>>>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:
>>
>> Yao> Maybe, we need a new target `remote-valgrind' here, and move your
>> Yao> stuff there.
>>
>> We discussed this a bit (last year?), but Pedro was against adding a new
>> target.  I don't recall why; I would like to know though.
> 
> It was in a different context (some target where endianess matters
> depending on whether you're reading code or something else), but the
> reasons are the same.  There's no need for one, and it adds to
> user confusion, and IDE complication.  If the remote target needs
> to behave differently against some remote stub, that calls 
> for the remote end giving gdb enough information for gdb to 
> adjust itself automatically.
> 

Yes, I agree.

> I can't say I understand why was that being proposed in this case?
> What is the patch breaking?
> 

One new command "set remote hardware-watchpoint-length-limit"
 is added in the patch, which is only useful to gdbserver+valgrind.
When gdb is talking with normal gdbserver, it may be wrong to set
hardware-watchpoint-length-limit in gdb side.  Users should be careful
when using this command.

The ideal solution, IMO, is remote side gives GDB the value of
hardware-watchpoint-length-limit, however, I don't know it is easy or
hard to do such thing.

-- 
Yao (齐尧)

Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)

[Philippe Waroquiers]

> One new command "set remote hardware-watchpoint-length-limit"
> is added in the patch, which is only useful to gdbserver+valgrind.
> When gdb is talking with normal gdbserver, it may be wrong to set
> hardware-watchpoint-length-limit in gdb side.  Users should be careful
> when using this command.
I think the command is also useful in other cases 
(cfr my other post of yesterday http://sourceware.org/ml/gdb-patches/2011-05/msg00664.html).

But if you believe this should be aimed only (or mainly) at Valgrind gdbserver,
I can update the patch (e.g. the documentation   + change length default value)
to make it more Valgrind gdbserver specific.

> The ideal solution, IMO, is remote side gives GDB the value of
> hardware-watchpoint-length-limit, however, I don't know it is easy or
> hard to do such thing.

An approach where the remote side would tell which hw watchpoints are ok is the best.
It was discussed in another thread, but that is not easy to do.
E.g. for the moment, which hw watchpoints are accepted depends
on the order in which gdb inserts the watchpoints.

Currently, depending of the order of watchpoint insertion, you will even
trigger a bug in x86/amd64 gdbserver (cfr http://sourceware.org/ml/gdb-patches/2011-05/msg00664.html).

Philippe

Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)

[Yao Qi]

On 05/30/2011 01:33 PM, Philippe Waroquiers wrote:
> I think the command is also useful in other cases (cfr my other post of
> yesterday http://sourceware.org/ml/gdb-patches/2011-05/msg00664.html).
> 
> But if you believe this should be aimed only (or mainly) at Valgrind
> gdbserver,
> I can update the patch (e.g. the documentation   + change length default
> value)
> to make it more Valgrind gdbserver specific.
> 

Philippe,
As maintainers pointed out, adding a new target `remote-valgrind' is not
the right way to go, and I am not strongly against your patch, please
ignore my last comment on adding a new target.

-- 
Yao (齐尧)

Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)

[Philippe Waroquiers]

> Philippe,
> As maintainers pointed out, adding a new target `remote-valgrind' is not
> the right way to go, and I am not strongly against your patch, please
> ignore my last comment on adding a new target.

Effectively, a new target is not the right way to go.

But there are still two points worth discussing:
  * the bug in the current x86 gdbserver
  * and what behaviour is desired for "too long" watchpoints that
     can't be placed as hw watchpoints.

Philippe

Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)

[Joel Brobecker]

Hello Philippe,

> Question: is there somewhere a 'how to test gdbserver for dummies' ?
> I read the gdb internal doc, and could only find a reference to
> setting GDBSERVER=... variable when doing make check, but that does
> not run the gdbserver tests (I believe).

We have a wiki page on how to run the testsuite, and there is a section
that explains how to run the testsuite with gdbserver.

        http://sourceware.org/gdb/wiki/TestingGDB

-- 
Joel

Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)

[Pedro Alves]

On Monday 30 May 2011 05:06:09, Yao Qi wrote:
> > I can't say I understand why was that being proposed in this case?
> > What is the patch breaking?
> > 
> 
> One new command "set remote hardware-watchpoint-length-limit"
>  is added in the patch, which is only useful to gdbserver+valgrind.
> When gdb is talking with normal gdbserver, it may be wrong to set
> hardware-watchpoint-length-limit in gdb side.  Users should be careful
> when using this command.

If that was the only problem, than it'd be okay --- the user just
shouldn't use the command then.  GDB will just do what the
user told it to.  But, it looks like the patch changes the
behavior _even_ if the user doesn't use the command.

> The ideal solution, IMO, is remote side gives GDB the value of
> hardware-watchpoint-length-limit, however, I don't know it is easy or
> hard to do such thing.

We've also discussed completely getting rid of watchpoint
resources accounting recently.

-- 
Pedro Alves

Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)

[Philippe Waroquiers]

> If that was the only problem, than it'd be okay --- the user just
> shouldn't use the command then.  GDB will just do what the
> user told it to.  But, it looks like the patch changes the
> behavior _even_ if the user doesn't use the command.

Effectively, the patch changes the behaviour (but I believe in a more
consistent way). But if that is considered as not good, I can change
the patch so as to keep by default the old behaviour.

Note that thanks to the pointer Joel gave me for the gdbserver testing,
I have run the regression test suite (on debian 5.0 amd64) with and
without the patch, and there is no regression.

Now that I understand better how to test gdbserver, I will try to 
add a test which reproduces the gdbserver crash.

> 
>> The ideal solution, IMO, is remote side gives GDB the value of
>> hardware-watchpoint-length-limit, however, I don't know it is easy or
>> hard to do such thing.
> 
> We've also discussed completely getting rid of watchpoint
> resources accounting recently.
For sure, if that would appear, that would be nice.

I guess that what we need is a packet such as:
"here is a list of hw watchpoint, is this list ok ?" packet.

Note that one other thing that I find confusing in the current behaviour
is that if you have a certain set of hw watchpoints that were accepted
and you add a new one, you might obtain an error back referencing
an "old" accepted watchpoint.

I think it would be better if the watchpoints would always be re-inserted
by gdb in the same order.

Philippe

x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver))

[Pedro Alves]

On Sunday 29 May 2011 14:01:11, Philippe Waroquiers wrote:
> For what concerns the bug: the problem is that the code in i386-low.c
> can partially place a watchpoint, but to the contrary of i386-nat.c,
> such a partial watchpoint will not be rolled back by breakpoint.c

Not sure I understand what is different between GDB and GDBserver
here.  A watchpoint, from breakpoint.c's perpective can be composed
of several low-level watchpoints.  E.g., if the expression the user
wants to watch requires trapping accesses to two disjoint memory
regions for changes, each of those memory regions will correspond
to one low-level hardware watchpoint.  In GDBserver's or i386-nat.c's
perpective, there will be two watchpoints.  If the second fails to
insert, then breakpoint.c in GDB rolls back the first.  This applies
to GDBserver as well.

> The patch fixes this (as this bug is much easier to trigger with long
> breakpoints which are accepted by gdb, but have to be rolled back
> by gdbserver). 
> To reproduce the bug, compile the attached s.c, and do the following:
>       gdbserver :1234 ./s
> gdb ./s
> tar rem :1234
> set breakpoint always-inserted on
> watch s1
> watch s2
> watch s4
> watch s3
> del
> y
> break s.c:24
> c
> p p = &s3
> c
>         linux-x86-low.c:511: A problem internal to GDBserver has been detected.
>         Assertion `DR_FIRSTADDR <= regnum && regnum < DR_LASTADDR' failed.

First things first.  This assertion is actually bogus ( and I'm to blame
for it :-) ).  Patch below.  We get here with regnum == 3, which is quite
valid.  This means gdbserver is asserting whenever a watchpoint on DR3
triggers.  Vis:

(gdb) watch s1
(gdb) watch s2
(gdb) watch s3

(gdb) b 24
(gdb) c
...

on gdbserver side we have:

stopped_data_addr:
        CONTROL (DR7): 51150155          STATUS (DR6): 00000000
        DR0: addr=0x603768, ref.count=1  DR1: addr=0x60376a, ref.count=1
        DR2: addr=0x609a08, ref.count=1  DR3: addr=0x60d8e8, ref.count=1
                                                   ^^^^^^^^

so:

(gdb) p p = 0x60d8e8
(gdb) c

... puff!

../../../src/gdb/gdbserver/linux-x86-low.c:511: A problem internal to GDBserver has been detected.
Assertion `DR_FIRSTADDR <= regnum && regnum < DR_LASTADDR' failed.

Program exited with code 01.
(gdb) 

Pedro Alves

2011-05-31  Pedro Alves  <pedro@codesourcery.com>

	gdb/gdbserver/
	* linux-x86-low.c (i386_dr_low_get_addr): Fix off by one in
	assertion.
	* win32-i386-low.c (i386_dr_low_get_addr): Ditto.

---
 gdb/gdbserver/linux-x86-low.c  |    2 +-
 gdb/gdbserver/win32-i386-low.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Index: src/gdb/gdbserver/linux-x86-low.c
===================================================================
--- src.orig/gdb/gdbserver/linux-x86-low.c	2011-04-28 17:17:30.000000000 +0100
+++ src/gdb/gdbserver/linux-x86-low.c	2011-05-31 19:55:00.924959503 +0100
@@ -508,7 +508,7 @@ i386_dr_low_get_addr (int regnum)
   ptid_t ptid = ptid_of (lwp);
 
   /* DR6 and DR7 are retrieved with some other way.  */
-  gdb_assert (DR_FIRSTADDR <= regnum && regnum < DR_LASTADDR);
+  gdb_assert (DR_FIRSTADDR <= regnum && regnum <= DR_LASTADDR);
 
   return x86_linux_dr_get (ptid, regnum);
 }
Index: src/gdb/gdbserver/win32-i386-low.c
===================================================================
--- src.orig/gdb/gdbserver/win32-i386-low.c	2011-01-13 15:07:54.000000000 +0000
+++ src/gdb/gdbserver/win32-i386-low.c	2011-05-31 19:56:14.414959478 +0100
@@ -61,7 +61,7 @@ i386_dr_low_set_addr (const struct i386_
 CORE_ADDR
 i386_dr_low_get_addr (int regnum)
 {
-  gdb_assert (DR_FIRSTADDR <= regnum && regnum < DR_LASTADDR);
+  gdb_assert (DR_FIRSTADDR <= regnum && regnum <= DR_LASTADDR);
 
   return debug_reg_state.dr_mirror[regnum];
 }

Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver))

[Philippe Waroquiers]

> Not sure I understand what is different between GDB and GDBserver
> here.  A watchpoint, from breakpoint.c's perpective can be composed
> of several low-level watchpoints.  E.g., if the expression the user
> wants to watch requires trapping accesses to two disjoint memory
> regions for changes, each of those memory regions will correspond
> to one low-level hardware watchpoint.  In GDBserver's or i386-nat.c's
> perpective, there will be two watchpoints.  If the second fails to
> insert, then breakpoint.c in GDB rolls back the first.  This applies
> to GDBserver as well.

> ../../../src/gdb/gdbserver/linux-x86-low.c:511: A problem internal to GDBserver has been detected.
> Assertion `DR_FIRSTADDR <= regnum && regnum < DR_LASTADDR' failed.

Sorry for the somewhat wrong analysis of the bug.

I have applied your patch in the assert, and tested again.
The GDBserver does not crash anymore (but it still keeps a DR register
busy for no reason).

So, there is for sure still a difference of behaviour (probably in breakpoint.c
placing a "local" watch and a "remote" watch).

Note that there is another similar (but I believe correct) assert in the code, but 
slightly different. I am not sure to understand why regnum validity is tested
differently in the below:
  if (! (regnum >= 0 && regnum <= DR_LASTADDR - DR_FIRSTADDR))
    fatal ("Invalid debug register %d", regnum);

Thanks for looking at all this
Philippe

Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver))

[Pedro Alves]

On Tuesday 31 May 2011 21:25:30, Philippe Waroquiers wrote:
> 
> > Not sure I understand what is different between GDB and GDBserver
> > here.  A watchpoint, from breakpoint.c's perpective can be composed
> > of several low-level watchpoints.  E.g., if the expression the user
> > wants to watch requires trapping accesses to two disjoint memory
> > regions for changes, each of those memory regions will correspond
> > to one low-level hardware watchpoint.  In GDBserver's or i386-nat.c's
> > perpective, there will be two watchpoints.  If the second fails to
> > insert, then breakpoint.c in GDB rolls back the first.  This applies
> > to GDBserver as well.
> 
> > ../../../src/gdb/gdbserver/linux-x86-low.c:511: A problem internal to GDBserver has been detected.
> > Assertion `DR_FIRSTADDR <= regnum && regnum < DR_LASTADDR' failed.
> 
> Sorry for the somewhat wrong analysis of the bug.

Not to worry.  Thanks for tripping on this. :-)

> 
> I have applied your patch in the assert, and tested again.
> The GDBserver does not crash anymore (but it still keeps a DR register
> busy for no reason).

Yes, that's a bug to fix too.  I'll need a bit to take a better
look at your patch.

> 
> So, there is for sure still a difference of behaviour (probably in breakpoint.c
> placing a "local" watch and a "remote" watch).

It looks to me the bug is equality present on native gdb:

$ ./gdb ~/s 
GNU gdb (GDB) 7.3.50.20110527-cvs
...
(gdb) start
Temporary breakpoint 1 at 0x4004b8: file s.c, line 22.
Starting program: /home/pedro/s 

Temporary breakpoint 1, main () at s.c:22
22         char * p = s1000;
(gdb) set breakpoint always-inserted on 
(gdb) maint set show-debug-regs on 
(gdb) watch s1
Hardware watchpoint 2: s1
During symbol reading, incomplete CFI data; unspecified registers (e.g., rax) at 0x4004e3.
insert_watchpoint (addr=609a08, len=1, type=data-write):
        CONTROL (DR7): 0000000000010101          STATUS (DR6): 0000000000004000
        DR0: addr=0x0000000000609a08, ref.count=1  DR1: addr=0x0000000000000000, ref.count=0
        DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0
(gdb) watch s2
Hardware watchpoint 3: s2
insert_watchpoint (addr=60d8e8, len=2, type=data-write):
        CONTROL (DR7): 0000000000510105          STATUS (DR6): 0000000000004000
        DR0: addr=0x0000000000609a08, ref.count=1  DR1: addr=0x000000000060d8e8, ref.count=1
        DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0
(gdb) watch s4
Hardware watchpoint 4: s4
insert_watchpoint (addr=607280, len=4, type=data-write):
        CONTROL (DR7): 000000000d510115          STATUS (DR6): 0000000000004000
        DR0: addr=0x0000000000609a08, ref.count=1  DR1: addr=0x000000000060d8e8, ref.count=1
        DR2: addr=0x0000000000607280, ref.count=1  DR3: addr=0x0000000000000000, ref.count=0
(gdb) watch s3
Hardware watchpoint 5: s3
insert_watchpoint (addr=603768, len=3, type=data-write):
        CONTROL (DR7): 000000005d510155          STATUS (DR6): 0000000000004000
        DR0: addr=0x0000000000609a08, ref.count=1  DR1: addr=0x000000000060d8e8, ref.count=1
        DR2: addr=0x0000000000607280, ref.count=1  DR3: addr=0x0000000000603768, ref.count=1
Warning:
Could not insert hardware watchpoint 5.
Could not insert hardware breakpoints:
You may have requested too many hardware breakpoints/watchpoints.

(gdb) del
Delete all breakpoints? (y or n) y
remove_watchpoint (addr=609a08, len=1, type=data-write):
        CONTROL (DR7): 000000005d510154          STATUS (DR6): 0000000000004000
        DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x000000000060d8e8, ref.count=1
        DR2: addr=0x0000000000607280, ref.count=1  DR3: addr=0x0000000000603768, ref.count=1
remove_watchpoint (addr=60d8e8, len=2, type=data-write):
        CONTROL (DR7): 000000005d510150          STATUS (DR6): 0000000000004000
        DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x0000000000000000, ref.count=0
        DR2: addr=0x0000000000607280, ref.count=1  DR3: addr=0x0000000000603768, ref.count=1
remove_watchpoint (addr=607280, len=4, type=data-write):
        CONTROL (DR7): 000000005d510140          STATUS (DR6): 0000000000004000
        DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x0000000000000000, ref.count=0
        DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000603768, ref.count=1
(gdb) si
stopped_data_addr:
        CONTROL (DR7): 000000005d510140          STATUS (DR6): 0000000000004000
        DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x0000000000000000, ref.count=0
        DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000603768, ref.count=1
                                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
23         for (i = 0; i < 1000; i++)
(gdb) 

You just didn't trip on the assert there, because there's no such
assertion on gdb (because I had never ported the
patch that had added the assertion to gdbserver, back to gdb.  I'll need to
remember to do that, in the name of keeping the codebases as much in
sync as possible...)

> 
> Note that there is another similar (but I believe correct) assert in the code, but 
> slightly different. I am not sure to understand why regnum validity is tested
> differently in the below:
>   if (! (regnum >= 0 && regnum <= DR_LASTADDR - DR_FIRSTADDR))
>     fatal ("Invalid debug register %d", regnum);

Yeah, just different spellings of the same thing.

-- 
Pedro Alves

Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver))

[Pedro Alves]

On Tuesday 31 May 2011 20:07:09, Pedro Alves wrote:
> First things first.  This assertion is actually bogus ( and I'm to blame
> for it :-) ).  Patch below.  We get here with regnum == 3, which is quite
> valid.  This means gdbserver is asserting whenever a watchpoint on DR3
> triggers.  Vis:
> 
> (gdb) watch s1
> (gdb) watch s2
> (gdb) watch s3
> 
> (gdb) b 24
> (gdb) c
> ...
> 
> on gdbserver side we have:
> 
> stopped_data_addr:
>         CONTROL (DR7): 51150155          STATUS (DR6): 00000000
>         DR0: addr=0x603768, ref.count=1  DR1: addr=0x60376a, ref.count=1
>         DR2: addr=0x609a08, ref.count=1  DR3: addr=0x60d8e8, ref.count=1
>                                                    ^^^^^^^^
> 
> so:
> 
> (gdb) p p = 0x60d8e8
> (gdb) c
> 
> ... puff!
> 
> ../../../src/gdb/gdbserver/linux-x86-low.c:511: A problem internal to GDBserver has been detected.
> Assertion `DR_FIRSTADDR <= regnum && regnum < DR_LASTADDR' failed.
> 
> Program exited with code 01.
> (gdb) 
> 

Now with new testcase.  Applied to both mainline and 7.3.

Thanks,
-- 
Pedro Alves

2011-05-31  Pedro Alves  <pedro@codesourcery.com>

	gdb/gdbserver/
	* linux-x86-low.c (i386_dr_low_get_addr): Fix off by one in
	assertion.
	* win32-i386-low.c (i386_dr_low_get_addr): Ditto.

	gdb/testsuite/
	* gdb.arch/i386-dr3-watch.c: New file.
	* gdb.arch/i386-dr3-watch.exp: New file.

---
 gdb/gdbserver/linux-x86-low.c             |    2 -
 gdb/gdbserver/win32-i386-low.c            |    2 -
 gdb/testsuite/gdb.arch/i386-dr3-watch.c   |   43 ++++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/i386-dr3-watch.exp |   49 ++++++++++++++++++++++++++++++
 4 files changed, 94 insertions(+), 2 deletions(-)

Index: src/gdb/gdbserver/linux-x86-low.c
===================================================================
--- src.orig/gdb/gdbserver/linux-x86-low.c	2011-05-31 22:04:53.000000000 +0100
+++ src/gdb/gdbserver/linux-x86-low.c	2011-05-31 22:05:24.224956802 +0100
@@ -508,7 +508,7 @@ i386_dr_low_get_addr (int regnum)
   ptid_t ptid = ptid_of (lwp);
 
   /* DR6 and DR7 are retrieved with some other way.  */
-  gdb_assert (DR_FIRSTADDR <= regnum && regnum < DR_LASTADDR);
+  gdb_assert (DR_FIRSTADDR <= regnum && regnum <= DR_LASTADDR);
 
   return x86_linux_dr_get (ptid, regnum);
 }
Index: src/gdb/gdbserver/win32-i386-low.c
===================================================================
--- src.orig/gdb/gdbserver/win32-i386-low.c	2011-05-31 22:04:53.000000000 +0100
+++ src/gdb/gdbserver/win32-i386-low.c	2011-05-31 22:05:24.224956802 +0100
@@ -61,7 +61,7 @@ i386_dr_low_set_addr (const struct i386_
 CORE_ADDR
 i386_dr_low_get_addr (int regnum)
 {
-  gdb_assert (DR_FIRSTADDR <= regnum && regnum < DR_LASTADDR);
+  gdb_assert (DR_FIRSTADDR <= regnum && regnum <= DR_LASTADDR);
 
   return debug_reg_state.dr_mirror[regnum];
 }
Index: src/gdb/testsuite/gdb.arch/i386-dr3-watch.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.arch/i386-dr3-watch.c	2011-05-31 21:41:33.764957296 +0100
@@ -0,0 +1,43 @@
+/* Copyright 2011 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int i1;
+char gap1[32];
+
+int i2;
+char gap2[32];
+
+int i3;
+char gap3[32];
+
+int i4;
+
+void
+trigger (void)
+{
+  i1 = 1;
+  i2 = 2;
+  i3 = 3;
+  i4 = 4;
+}
+
+int
+main ()
+{
+  trigger ();
+  return 0;
+}
Index: src/gdb/testsuite/gdb.arch/i386-dr3-watch.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.arch/i386-dr3-watch.exp	2011-05-31 22:07:16.714956763 +0100
@@ -0,0 +1,49 @@
+# Copyright 2011 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# x86 and amd64 gdbserver had a bug where a watchpoint triggered by
+# the DR3 debug register would trip on a bogus assertion.
+
+# This test relies on being able to set 4 hardware watchpoints.  Since
+# that is not a valid assumption across most targets, and we're
+# testing a x86 specific bug, skip everywhere else.
+if { ![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"] } then {
+    return 0
+}
+
+set testfile "i386-dr3-watch"
+set srcfile ${testfile}.c
+
+if [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} {debug $additional_flags}] {
+    untested "failed to compile ${testfile}"
+    return -1
+}
+
+if ![runto_main] then {
+    untested "could not run to main"
+    return -1
+}
+
+gdb_test_no_output "set breakpoint always-inserted on"
+
+gdb_test "watch i1" "Hardware watchpoint .*: i1"
+gdb_test "watch i2" "Hardware watchpoint .*: i2"
+gdb_test "watch i3" "Hardware watchpoint .*: i3"
+gdb_test "watch i4" "Hardware watchpoint .*: i4"
+
+gdb_test "c" "Hardware watchpoint.*: i1.*" "continue to i1 watchpoint"
+gdb_test "c" "Hardware watchpoint.*: i2.*" "continue to i2 watchpoint"
+gdb_test "c" "Hardware watchpoint.*: i3.*" "continue to i3 watchpoint"
+gdb_test "c" "Hardware watchpoint.*: i4.*" "continue to i4 watchpoint"

Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver))

[Philippe Waroquiers]

> +gdb_test_no_output "set breakpoint always-inserted on"

I confirm that the bug of the DR register kept busy is also present
on a native GDB debugging.

Note the bug is slightly more difficult to trigger without
the  "set breakpoint always-inserted on"
as it seems to depend on the order in which watchpoints are inserted.

If you just do:
  break s.c:24
  run
  watch s1
  watch s2
  watch s4
  watch s3
  c
you do not trigger the busy bug as gdb inserts them
in the order :  s3
                     s4
                     s1
                     s2
and then there is no busy register remaining.

I was able to trigger the bug with the following sequence: s1 s2 s4 s16 (that gdb inserts
in the order s4 s1 s2 s16. See below the full trace of the bug with this order.

Note that it would be less user confusing if gdb would always insert the watchpoints in the
order the user entered them. No idea if this is easy to do.
Of course, the confusion only happens with limited hw watchpoint (with Valgrind gdbserver,
there will be no confusion :).
E.g. this is what you can obtain on amd64 with the Valgrind gdbserver (and the patched gdb
allowing unlimited length):
(gdb) info watch
Num     Type           Disp Enb Address            What
1       hw watchpoint  keep y                      s1
2       hw watchpoint  keep y                      s2
3       hw watchpoint  keep y                      s3
4       hw watchpoint  keep y                      s4
5       hw watchpoint  keep y                      s5
6       hw watchpoint  keep y                      s6
7       hw watchpoint  keep y                      s7
8       hw watchpoint  keep y                      s8
9       hw watchpoint  keep y                      s16
10      hw watchpoint  keep y                      s32
11      hw watchpoint  keep y                      s64
12      hw watchpoint  keep y                      s128
13      hw watchpoint  keep y                      s1000
 breakpoint already hit 1 time
(gdb) 
(the above is an advertisement  for the Valgrind gdbserver + patched gdb to allow unlimited length :)

Philippe


########################## bug without always-inserted, using s1 s2 s4 s16
...
stopped_data_addr:
 CONTROL (DR7): 0000000000000000          STATUS (DR6): 00000000ffff4ff0
 DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x0000000000000000, ref.count=0
 DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0

Breakpoint 1, main () at s.c:24
24       p[i] = 1;
(gdb) watch s1
Hardware watchpoint 2: s1
(gdb) watch s2
Hardware watchpoint 3: s2
(gdb) watch s4
Hardware watchpoint 4: s4
(gdb) watch s16
Hardware watchpoint 5: s16
(gdb) c
Continuing.
stopped_data_addr:
 CONTROL (DR7): 0000000000000000          STATUS (DR6): 00000000ffff4ff0
 DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x0000000000000000, ref.count=0
 DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0
insert_watchpoint (addr=606ac0, len=4, type=data-write):
 CONTROL (DR7): 00000000000d0101          STATUS (DR6): 00000000ffff4ff0
 DR0: addr=0x0000000000606ac0, ref.count=1  DR1: addr=0x0000000000000000, ref.count=0
 DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0
insert_watchpoint (addr=609248, len=1, type=data-write):
 CONTROL (DR7): 00000000001d0105          STATUS (DR6): 00000000ffff4ff0
 DR0: addr=0x0000000000606ac0, ref.count=1  DR1: addr=0x0000000000609248, ref.count=1
 DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0
insert_watchpoint (addr=60d128, len=2, type=data-write):
 CONTROL (DR7): 00000000051d0115          STATUS (DR6): 00000000ffff4ff0
 DR0: addr=0x0000000000606ac0, ref.count=1  DR1: addr=0x0000000000609248, ref.count=1
 DR2: addr=0x000000000060d128, ref.count=1  DR3: addr=0x0000000000000000, ref.count=0
insert_watchpoint (addr=60d1c0, len=16, type=data-write):
 CONTROL (DR7): 00000000951d0155          STATUS (DR6): 00000000ffff4ff0
 DR0: addr=0x0000000000606ac0, ref.count=1  DR1: addr=0x0000000000609248, ref.count=1
 DR2: addr=0x000000000060d128, ref.count=1  DR3: addr=0x000000000060d1c0, ref.count=1
Warning:
Could not insert hardware watchpoint 5.
Could not insert hardware breakpoints:
You may have requested too many hardware breakpoints/watchpoints.

remove_watchpoint (addr=606ac0, len=4, type=data-write):
 CONTROL (DR7): 00000000951d0154          STATUS (DR6): 00000000ffff4ff0
 DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x0000000000609248, ref.count=1
 DR2: addr=0x000000000060d128, ref.count=1  DR3: addr=0x000000000060d1c0, ref.count=1
remove_watchpoint (addr=609248, len=1, type=data-write):
 CONTROL (DR7): 00000000951d0150          STATUS (DR6): 00000000ffff4ff0
 DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x0000000000000000, ref.count=0
 DR2: addr=0x000000000060d128, ref.count=1  DR3: addr=0x000000000060d1c0, ref.count=1
remove_watchpoint (addr=60d128, len=2, type=data-write):
 CONTROL (DR7): 00000000951d0140          STATUS (DR6): 00000000ffff4ff0
 DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x0000000000000000, ref.count=0
 DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x000000000060d1c0, ref.count=1
0x0000000000400494 in main () at s.c:24
24       p[i] = 1;
(gdb) 

Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver))

[Pedro Alves]

On Tuesday 31 May 2011 23:15:13, Philippe Waroquiers wrote:
> 
> > +gdb_test_no_output "set breakpoint always-inserted on"
> 
> I confirm that the bug of the DR register kept busy is also present
> on a native GDB debugging.

Thanks.  I'm trying a different fix, but I don't think
I'll be able to finish it tonight.

> 
> Note the bug is slightly more difficult to trigger without
> the  "set breakpoint always-inserted on"
> as it seems to depend on the order in which watchpoints are inserted.
> 
> If you just do:
>   break s.c:24
>   run
>   watch s1
>   watch s2
>   watch s4
>   watch s3
>   c
> you do not trigger the busy bug as gdb inserts them
> in the order :  s3
>                      s4
>                      s1
>                      s2
> and then there is no busy register remaining.
> 
> I was able to trigger the bug with the following sequence: s1 s2 s4 s16 (that gdb inserts
> in the order s4 s1 s2 s16. See below the full trace of the bug with this order.
> 
> Note that it would be less user confusing if gdb would always insert the watchpoints in the
> order the user entered them. No idea if this is easy to do.
> Of course, the confusion only happens with limited hw watchpoint (with Valgrind gdbserver,
> there will be no confusion :).
> E.g. this is what you can obtain on amd64 with the Valgrind gdbserver (and the patched gdb
> allowing unlimited length):
> (gdb) info watch
> Num     Type           Disp Enb Address            What
> 1       hw watchpoint  keep y                      s1
> 2       hw watchpoint  keep y                      s2
> 3       hw watchpoint  keep y                      s3
> 4       hw watchpoint  keep y                      s4
> 5       hw watchpoint  keep y                      s5
> 6       hw watchpoint  keep y                      s6
> 7       hw watchpoint  keep y                      s7
> 8       hw watchpoint  keep y                      s8
> 9       hw watchpoint  keep y                      s16
> 10      hw watchpoint  keep y                      s32
> 11      hw watchpoint  keep y                      s64
> 12      hw watchpoint  keep y                      s128
> 13      hw watchpoint  keep y                      s1000
>  breakpoint already hit 1 time
> (gdb) 
> (the above is an advertisement  for the Valgrind gdbserver + patched gdb to allow unlimited length :)
> 
> Philippe
> 
> 
> ########################## bug without always-inserted, using s1 s2 s4 s16
> ...
> stopped_data_addr:
>  CONTROL (DR7): 0000000000000000          STATUS (DR6): 00000000ffff4ff0
>  DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x0000000000000000, ref.count=0
>  DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0
> 
> Breakpoint 1, main () at s.c:24
> 24       p[i] = 1;
> (gdb) watch s1
> Hardware watchpoint 2: s1
> (gdb) watch s2
> Hardware watchpoint 3: s2
> (gdb) watch s4
> Hardware watchpoint 4: s4
> (gdb) watch s16
> Hardware watchpoint 5: s16
> (gdb) c
> Continuing.
> stopped_data_addr:
>  CONTROL (DR7): 0000000000000000          STATUS (DR6): 00000000ffff4ff0
>  DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x0000000000000000, ref.count=0
>  DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0
> insert_watchpoint (addr=606ac0, len=4, type=data-write):
>  CONTROL (DR7): 00000000000d0101          STATUS (DR6): 00000000ffff4ff0
>  DR0: addr=0x0000000000606ac0, ref.count=1  DR1: addr=0x0000000000000000, ref.count=0
>  DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0
> insert_watchpoint (addr=609248, len=1, type=data-write):
>  CONTROL (DR7): 00000000001d0105          STATUS (DR6): 00000000ffff4ff0
>  DR0: addr=0x0000000000606ac0, ref.count=1  DR1: addr=0x0000000000609248, ref.count=1
>  DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0
> insert_watchpoint (addr=60d128, len=2, type=data-write):
>  CONTROL (DR7): 00000000051d0115          STATUS (DR6): 00000000ffff4ff0
>  DR0: addr=0x0000000000606ac0, ref.count=1  DR1: addr=0x0000000000609248, ref.count=1
>  DR2: addr=0x000000000060d128, ref.count=1  DR3: addr=0x0000000000000000, ref.count=0
> insert_watchpoint (addr=60d1c0, len=16, type=data-write):
>  CONTROL (DR7): 00000000951d0155          STATUS (DR6): 00000000ffff4ff0
>  DR0: addr=0x0000000000606ac0, ref.count=1  DR1: addr=0x0000000000609248, ref.count=1
>  DR2: addr=0x000000000060d128, ref.count=1  DR3: addr=0x000000000060d1c0, ref.count=1
> Warning:
> Could not insert hardware watchpoint 5.
> Could not insert hardware breakpoints:
> You may have requested too many hardware breakpoints/watchpoints.
> 
> remove_watchpoint (addr=606ac0, len=4, type=data-write):
>  CONTROL (DR7): 00000000951d0154          STATUS (DR6): 00000000ffff4ff0
>  DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x0000000000609248, ref.count=1
>  DR2: addr=0x000000000060d128, ref.count=1  DR3: addr=0x000000000060d1c0, ref.count=1
> remove_watchpoint (addr=609248, len=1, type=data-write):
>  CONTROL (DR7): 00000000951d0150          STATUS (DR6): 00000000ffff4ff0
>  DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x0000000000000000, ref.count=0
>  DR2: addr=0x000000000060d128, ref.count=1  DR3: addr=0x000000000060d1c0, ref.count=1
> remove_watchpoint (addr=60d128, len=2, type=data-write):
>  CONTROL (DR7): 00000000951d0140          STATUS (DR6): 00000000ffff4ff0
>  DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x0000000000000000, ref.count=0
>  DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x000000000060d1c0, ref.count=1
> 0x0000000000400494 in main () at s.c:24
> 24       p[i] = 1;
> (gdb) 
> 
> 

-- 
Pedro Alves

Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver))

[Pedro Alves]

On Wednesday 01 June 2011 00:04:31, Pedro Alves wrote:
> On Tuesday 31 May 2011 23:15:13, Philippe Waroquiers wrote:
> > 
> > > +gdb_test_no_output "set breakpoint always-inserted on"
> > 
> > I confirm that the bug of the DR register kept busy is also present
> > on a native GDB debugging.
> 
> Thanks.  I'm trying a different fix, but I don't think
> I'll be able to finish it tonight.

Here it is.  New test, fixes for both gdbserver and gdb.

I didn't like the rollback approach for a couple of
reasons.  First, given the reference counting of
debug registers, failing to insert half a watchpoint,
and then removing all of it looks fishy, and raises
alarms whenever I read it, because it gives the appearence
we'll mess up the reference counting for the part of the
watchpoint that we hadn't managed to insert in the first
place.  The second reason is, we're working with a
debug registers mirror.  If the watchpoint we want
to insert needs, e.g., two debug registers, it's completely
unnecessary to insert one, fail the second, and roll back
the first, and while doing that, notify the inferior twice
that the debug registers changed.  In non-stop mode, in
gdbserver, this can trigger an all-thread-momentary-stop
for no good reason.  Instead, we can switch to
transaction-style update of the debug registers.  Just
work on a mirror of the mirror of the debug registers,
and only if we suceeded in fitting the whole watchpoint,
do we update the inferior (commit).  If something fails,
we'll just forget the mirror of the mirror.

This is trivial to implement on the gdbserver side,
as we already have a nice "struct i386_debug_reg_state"
structure there.  The gdb side is simple as well, except
we doesn't have that structure yet, so the patch looks a
bit more complicated, but in fact it's just doing the exact
same change, plus bringing in that structure from
gdbserver.  (this structure is needed for multi-process
watchpoints support.)

Any comments on this?

-- 
Pedro Alves

2011-06-01  Pedro Alves  <pedro@codesourcery.com>

	gdb/testsuite/
	* gdb.arch/i386-dr3-watch.exp: Test that the i386 watchpoints
	backend doesn't leave used debug registers behind.

	gdb/gdbserver/
	* i386-low.c (i386_insert_aligned_watchpoint): Don't pass the info
	to the inferior here.
	(i386_remove_aligned_watchpoint): Ditto.
	(i386_handle_nonaligned_watchpoint): Return immediate on fail to
	fit part of the watchpoint in the debug registers.
	(update_inferior): New.
	(i386_low_insert_watchpoint): Work on a local mirror of the debug
	registers, and only update the inferior on success.
	(i386_low_remove_watchpoint): Ditto.

	gdb/
	* i386-nat.c (I386_DR_VACANT, I386_DR_LOCAL_ENABLE)
	(I386_DR_GLOBAL_ENABLE, I386_DR_DISABLE, I386_DR_SET_RW_LEN)
	(I386_DR_GET_RW_LEN, I386_DR_WATCH_HIT): Add state parameter and
	adjust.
	(dr_mirror, dr_status_mirror, dr_control_mirror): Delete.
	(struct i386_debug_reg_state): New.
	(i386_init_dregs): New.
	(dr_mirror): New.
	(i386_cleanup_dregs): Use i386_init_dregs.
	(i386_show_dr): Add state parameter and adjust.
	(i386_insert_aligned_watchpoint): Ditto.  Don't pass the info to
	the inferior here.
	(i386_remove_aligned_watchpoint): Likewise.
	(i386_handle_nonaligned_watchpoint): Add state parameter and adjust.
	(update_inferior): New.
	(i386_insert_watchpoint): Work on a local mirror of the debug
	registers, and only update the inferior on success.
	(i386_remove_watchpoint): Ditto.
	(i386_region_ok_for_watchpoint): Adjust.
	(i386_stopped_data_address): Adjust.
	(i386_insert_hw_breakpoint): Adjust.
	(i386_remove_hw_breakpoint): Adjust.

---
 gdb/gdbserver/i386-low.c                  |   74 +++++---
 gdb/i386-nat.c                            |  275 +++++++++++++++++++-----------
 gdb/testsuite/gdb.arch/i386-dr3-watch.exp |   44 ++++
 3 files changed, 269 insertions(+), 124 deletions(-)

Index: src/gdb/testsuite/gdb.arch/i386-dr3-watch.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.arch/i386-dr3-watch.exp	2011-06-01 15:19:12.869036452 +0100
+++ src/gdb/testsuite/gdb.arch/i386-dr3-watch.exp	2011-06-01 15:19:13.989036451 +0100
@@ -38,6 +38,8 @@ if ![runto_main] then {
 
 gdb_test_no_output "set breakpoint always-inserted on"
 
+# Test that we handle watchpoints in all of DR0-DR3.
+
 gdb_test "watch i1" "Hardware watchpoint .*: i1"
 gdb_test "watch i2" "Hardware watchpoint .*: i2"
 gdb_test "watch i3" "Hardware watchpoint .*: i3"
@@ -47,3 +49,45 @@ gdb_test "c" "Hardware watchpoint.*: i1.
 gdb_test "c" "Hardware watchpoint.*: i2.*" "continue to i2 watchpoint"
 gdb_test "c" "Hardware watchpoint.*: i3.*" "continue to i3 watchpoint"
 gdb_test "c" "Hardware watchpoint.*: i4.*" "continue to i4 watchpoint"
+
+delete_breakpoints
+
+# Regression test for a bug where the i386 watchpoints support backend
+# would leave some debug registers occupied even if not enough debug
+# registers were available to cover a single (low level) watchpoint.
+
+gdb_test "watch i1" \
+    "Hardware watchpoint .*: i1" \
+    "set watchpoint occuping one debug register"
+
+# gap1 too long to fit the 3 left over debug registers (but would fit
+# 4 if all were available).
+set test "watchpoint on gap1 does not fit debug registers"
+gdb_test_multiple "watch gap1" "$test" {
+    -re "Hardware watchpoint .*: gap1.*Warning:.*Could not insert hardware watchpoint.*You may have requested too many.*" {
+	pass $test
+    }
+    -re "Hardware watchpoint .*: gap1\r\n$gdb_prompt $" {
+	pass "$test (target emulates hardware watchpoints)"
+	return
+    }
+    -re "Watchpoint .*: gap1\r\n$gdb_prompt $" {
+	pass "$test (gdb figured out itself the watchpoint does not fit)"
+	return
+    }
+}
+
+# Start over.
+gdb_test "delete" \
+    "" \
+    "delete all watchpoints" \
+    "Delete all breakpoints.*$" \
+    "y"
+
+# If debug registers were left occupied by mistake, we'll fail to set
+# some of these watchpoints.  Each watchpoint should fit in one of the
+# 4 debug registers available.
+gdb_test "watch i1" "Hardware watchpoint .*: i1" "watch i1 still fits"
+gdb_test "watch i2" "Hardware watchpoint .*: i2" "watch i2 still fits"
+gdb_test "watch i3" "Hardware watchpoint .*: i3" "watch i3 still fits"
+gdb_test "watch i4" "Hardware watchpoint .*: i4" "watch i4 still fits"
Index: src/gdb/gdbserver/i386-low.c
===================================================================
--- src.orig/gdb/gdbserver/i386-low.c	2011-06-01 15:19:12.869036452 +0100
+++ src/gdb/gdbserver/i386-low.c	2011-06-01 15:19:13.989036451 +0100
@@ -307,10 +307,6 @@ i386_insert_aligned_watchpoint (struct i
   state->dr_control_mirror |= DR_LOCAL_SLOWDOWN;
   state->dr_control_mirror &= I386_DR_CONTROL_MASK;
 
-  /* Finally, actually pass the info to the inferior.  */
-  i386_dr_low_set_addr (state, i);
-  i386_dr_low_set_control (state);
-
   return 0;
 }
 
@@ -337,9 +333,6 @@ i386_remove_aligned_watchpoint (struct i
 	      /* Reset our mirror.  */
 	      state->dr_mirror[i] = 0;
 	      I386_DR_DISABLE (state, i);
-	      /* Reset it in the inferior.  */
-	      i386_dr_low_set_control (state);
-	      i386_dr_low_set_addr (state, i);
 	    }
 	  retval = 0;
 	}
@@ -360,7 +353,7 @@ i386_handle_nonaligned_watchpoint (struc
 				   i386_wp_op_t what, CORE_ADDR addr, int len,
 				   enum target_hw_bp_type type)
 {
-  int retval = 0, status = 0;
+  int retval = 0;
   int max_wp_len = TARGET_HAS_DR_LEN_8 ? 8 : 4;
 
   static const int size_try_array[8][8] =
@@ -398,25 +391,16 @@ i386_handle_nonaligned_watchpoint (struc
 	  unsigned len_rw = i386_length_and_rw_bits (size, type);
 
 	  if (what == WP_INSERT)
-	    status = i386_insert_aligned_watchpoint (state, addr, len_rw);
+	    retval = i386_insert_aligned_watchpoint (state, addr, len_rw);
 	  else if (what == WP_REMOVE)
-	    status = i386_remove_aligned_watchpoint (state, addr, len_rw);
+	    retval = i386_remove_aligned_watchpoint (state, addr, len_rw);
 	  else
 	    fatal ("\
 Invalid value %d of operation in i386_handle_nonaligned_watchpoint.\n",
 		   (int) what);
 
-	  /* We keep the loop going even after a failure, because some
-	     of the other aligned watchpoints might still succeed
-	     (e.g. if they watch addresses that are already watched,
-	     in which case we just increment the reference counts of
-	     occupied debug registers).  If we break out of the loop
-	     too early, we could cause those addresses watched by
-	     other watchpoints to be disabled when breakpoint.c reacts
-	     to our failure to insert this watchpoint and tries to
-	     remove it.  */
-	  if (status)
-	    retval = status;
+	  if (retval)
+	    break;
 	}
 
       addr += size;
@@ -448,6 +432,34 @@ Z_packet_to_hw_type (char type)
     }
 }
 
+/* Update the inferior debug registers state, in INF_STATE, with the
+   new debug registers state, in NEW_STATE.  */
+
+static void
+update_inferior (struct i386_debug_reg_state *inf_state,
+		 struct i386_debug_reg_state *new_state)
+{
+  int i;
+
+  ALL_DEBUG_REGISTERS (i)
+    {
+      if (new_state->dr_mirror[i] != inf_state->dr_mirror[i]
+	  || (new_state->dr_ref_count[i] != 0
+	      && inf_state->dr_ref_count[i] == 0))
+	{
+	  inf_state->dr_mirror[i] = new_state->dr_mirror[i];
+	  inf_state->dr_ref_count[i] = new_state->dr_ref_count[i];
+	  i386_dr_low_set_addr (inf_state, i);
+	}
+    }
+
+  if (new_state->dr_control_mirror != inf_state->dr_control_mirror)
+    {
+      inf_state->dr_control_mirror = new_state->dr_control_mirror;
+      i386_dr_low_set_control (inf_state);
+    }
+}
+
 /* Insert a watchpoint to watch a memory region which starts at
    address ADDR and whose length is LEN bytes.  Watch memory accesses
    of the type TYPE_FROM_PACKET.  Return 0 on success, -1 on failure.  */
@@ -458,6 +470,9 @@ i386_low_insert_watchpoint (struct i386_
 {
   int retval;
   enum target_hw_bp_type type = Z_packet_to_hw_type (type_from_packet);
+  /* Work on a local copy of the debug registers, and on success,
+     commit the change back to the inferior.  */
+  struct i386_debug_reg_state local_state = *state;
 
   if (type == hw_read)
     return 1; /* unsupported */
@@ -466,16 +481,19 @@ i386_low_insert_watchpoint (struct i386_
        && !(TARGET_HAS_DR_LEN_8 && len == 8))
       || addr % len != 0)
     {
-      retval = i386_handle_nonaligned_watchpoint (state, WP_INSERT,
+      retval = i386_handle_nonaligned_watchpoint (&local_state, WP_INSERT,
 						  addr, len, type);
     }
   else
     {
       unsigned len_rw = i386_length_and_rw_bits (len, type);
 
-      retval = i386_insert_aligned_watchpoint (state, addr, len_rw);
+      retval = i386_insert_aligned_watchpoint (&local_state, addr, len_rw);
     }
 
+  if (retval == 0)
+    update_inferior (state, &local_state);
+
   if (debug_hw_points)
     i386_show_dr (state, "insert_watchpoint", addr, len, type);
 
@@ -492,21 +510,27 @@ i386_low_remove_watchpoint (struct i386_
 {
   int retval;
   enum target_hw_bp_type type = Z_packet_to_hw_type (type_from_packet);
+  /* Work on a local copy of the debug registers, and on success,
+     commit the change back to the inferior.  */
+  struct i386_debug_reg_state local_state = *state;
 
   if (((len != 1 && len != 2 && len != 4)
        && !(TARGET_HAS_DR_LEN_8 && len == 8))
       || addr % len != 0)
     {
-      retval = i386_handle_nonaligned_watchpoint (state, WP_REMOVE,
+      retval = i386_handle_nonaligned_watchpoint (&local_state, WP_REMOVE,
 						  addr, len, type);
     }
   else
     {
       unsigned len_rw = i386_length_and_rw_bits (len, type);
 
-      retval = i386_remove_aligned_watchpoint (state, addr, len_rw);
+      retval = i386_remove_aligned_watchpoint (&local_state, addr, len_rw);
     }
 
+  if (retval == 0)
+    update_inferior (state, &local_state);
+
   if (debug_hw_points)
     i386_show_dr (state, "remove_watchpoint", addr, len, type);
 
Index: src/gdb/i386-nat.c
===================================================================
--- src.orig/gdb/i386-nat.c	2011-06-01 15:19:12.869036452 +0100
+++ src/gdb/i386-nat.c	2011-06-01 15:19:13.989036451 +0100
@@ -111,45 +111,88 @@ struct i386_dr_low_type i386_dr_low;
 
 /* The I'th debug register is vacant if its Local and Global Enable
    bits are reset in the Debug Control register.  */
-#define I386_DR_VACANT(i) \
-  ((dr_control_mirror & (3 << (DR_ENABLE_SIZE * (i)))) == 0)
+#define I386_DR_VACANT(state, i)					\
+  (((state)->dr_control_mirror & (3 << (DR_ENABLE_SIZE * (i)))) == 0)
 
 /* Locally enable the break/watchpoint in the I'th debug register.  */
-#define I386_DR_LOCAL_ENABLE(i) \
-  dr_control_mirror |= (1 << (DR_LOCAL_ENABLE_SHIFT + DR_ENABLE_SIZE * (i)))
+#define I386_DR_LOCAL_ENABLE(state, i) \
+  do { \
+    (state)->dr_control_mirror |= \
+      (1 << (DR_LOCAL_ENABLE_SHIFT + DR_ENABLE_SIZE * (i))); \
+  } while (0)
 
 /* Globally enable the break/watchpoint in the I'th debug register.  */
-#define I386_DR_GLOBAL_ENABLE(i) \
-  dr_control_mirror |= (1 << (DR_GLOBAL_ENABLE_SHIFT + DR_ENABLE_SIZE * (i)))
+#define I386_DR_GLOBAL_ENABLE(state, i) \
+  do { \
+    (state)->dr_control_mirror |= \
+      (1 << (DR_GLOBAL_ENABLE_SHIFT + DR_ENABLE_SIZE * (i))); \
+  } while (0)
 
 /* Disable the break/watchpoint in the I'th debug register.  */
-#define I386_DR_DISABLE(i) \
-  dr_control_mirror &= ~(3 << (DR_ENABLE_SIZE * (i)))
+#define I386_DR_DISABLE(state, i) \
+  do { \
+    (state)->dr_control_mirror &= \
+      ~(3 << (DR_ENABLE_SIZE * (i))); \
+  } while (0)
 
 /* Set in DR7 the RW and LEN fields for the I'th debug register.  */
-#define I386_DR_SET_RW_LEN(i,rwlen) \
+#define I386_DR_SET_RW_LEN(state, i, rwlen) \
   do { \
-    dr_control_mirror &= ~(0x0f << (DR_CONTROL_SHIFT+DR_CONTROL_SIZE*(i)));   \
-    dr_control_mirror |= ((rwlen) << (DR_CONTROL_SHIFT+DR_CONTROL_SIZE*(i))); \
+    (state)->dr_control_mirror &= \
+      ~(0x0f << (DR_CONTROL_SHIFT + DR_CONTROL_SIZE * (i))); \
+    (state)->dr_control_mirror |= \
+      ((rwlen) << (DR_CONTROL_SHIFT + DR_CONTROL_SIZE * (i))); \
   } while (0)
 
 /* Get from DR7 the RW and LEN fields for the I'th debug register.  */
-#define I386_DR_GET_RW_LEN(i) \
-  ((dr_control_mirror >> (DR_CONTROL_SHIFT + DR_CONTROL_SIZE * (i))) & 0x0f)
+#define I386_DR_GET_RW_LEN(dr7, i) \
+  (((dr7) \
+    >> (DR_CONTROL_SHIFT + DR_CONTROL_SIZE * (i))) & 0x0f)
 
 /* Mask that this I'th watchpoint has triggered.  */
 #define I386_DR_WATCH_MASK(i)	(1 << (i))
 
 /* Did the watchpoint whose address is in the I'th register break?  */
-#define I386_DR_WATCH_HIT(i)	(dr_status_mirror & I386_DR_WATCH_MASK (i))
+#define I386_DR_WATCH_HIT(dr6, i) ((dr6) & (1 << (i)))
 
 /* A macro to loop over all debug registers.  */
 #define ALL_DEBUG_REGISTERS(i)	for (i = 0; i < DR_NADDR; i++)
 
-/* Mirror the inferior's DRi registers.  We keep the status and
-   control registers separated because they don't hold addresses.  */
-static CORE_ADDR dr_mirror[DR_NADDR];
-static unsigned long dr_status_mirror, dr_control_mirror;
+
+/* Global state needed to track h/w watchpoints.  */
+
+struct i386_debug_reg_state
+{
+  /* Mirror the inferior's DRi registers.  We keep the status and
+     control registers separated because they don't hold addresses.
+     Note that since we can change these mirrors while threads are
+     running, we never trust them to explain a cause of a trap.
+     For that, we need to peek directly in the inferior registers.  */
+  CORE_ADDR dr_mirror[DR_NADDR];
+  unsigned dr_status_mirror, dr_control_mirror;
+
+  /* Reference counts for each debug register.  */
+  int dr_ref_count[DR_NADDR];
+};
+
+/* Clear the reference counts and forget everything we knew about the
+   debug registers.  */
+
+void
+i386_low_init_dregs (struct i386_debug_reg_state *state)
+{
+  int i;
+
+  ALL_DEBUG_REGISTERS (i)
+    {
+      state->dr_mirror[i] = 0;
+      state->dr_ref_count[i] = 0;
+    }
+  state->dr_control_mirror = 0;
+  state->dr_status_mirror  = 0;
+}
+
+static struct i386_debug_reg_state dr_mirror;
 
 /* Reference counts for each debug register.  */
 static int dr_ref_count[DR_NADDR];
@@ -172,7 +215,8 @@ static unsigned i386_length_and_rw_bits
    value of the bit-field from DR7 which describes the length and
    access type of the region to be watched by this watchpoint.  Return
    0 on success, -1 on failure.  */
-static int i386_insert_aligned_watchpoint (CORE_ADDR addr,
+static int i386_insert_aligned_watchpoint (struct i386_debug_reg_state *state,
+					   CORE_ADDR addr,
 					   unsigned len_rw_bits);
 
 /* Remove a watchpoint at address ADDR, which is assumed to be aligned
@@ -180,7 +224,8 @@ static int i386_insert_aligned_watchpoin
    value of the bits from DR7 which describes the length and access
    type of the region watched by this watchpoint.  Return 0 on
    success, -1 on failure.  */
-static int i386_remove_aligned_watchpoint (CORE_ADDR addr,
+static int i386_remove_aligned_watchpoint (struct i386_debug_reg_state *state,
+					   CORE_ADDR addr,
 					   unsigned len_rw_bits);
 
 /* Insert or remove a (possibly non-aligned) watchpoint, or count the
@@ -189,7 +234,8 @@ static int i386_remove_aligned_watchpoin
    successful insertion or removal, a positive number when queried
    about the number of registers, or -1 on failure.  If WHAT is not a
    valid value, bombs through internal_error.  */
-static int i386_handle_nonaligned_watchpoint (i386_wp_op_t what,
+static int i386_handle_nonaligned_watchpoint (struct i386_debug_reg_state *state,
+					      i386_wp_op_t what,
 					      CORE_ADDR addr, int len,
 					      enum target_hw_bp_type type);
 
@@ -201,15 +247,7 @@ static int i386_handle_nonaligned_watchp
 void
 i386_cleanup_dregs (void)
 {
-  int i;
-
-  ALL_DEBUG_REGISTERS(i)
-    {
-      dr_mirror[i] = 0;
-      dr_ref_count[i] = 0;
-    }
-  dr_control_mirror = 0;
-  dr_status_mirror  = 0;
+  i386_low_init_dregs (&dr_mirror);
 }
 
 /* Print the values of the mirrored debug registers.  This is called
@@ -217,7 +255,8 @@ i386_cleanup_dregs (void)
    show-debug-regs" at GDB's prompt.  */
 
 static void
-i386_show_dr (const char *func, CORE_ADDR addr,
+i386_show_dr (struct i386_debug_reg_state *state,
+	      const char *func, CORE_ADDR addr,
 	      int len, enum target_hw_bp_type type)
 {
   int addr_size = gdbarch_addr_bit (target_gdbarch) / 8;
@@ -239,13 +278,16 @@ i386_show_dr (const char *func, CORE_ADD
 				: "??unknown??"))));
   puts_unfiltered (":\n");
   printf_unfiltered ("\tCONTROL (DR7): %s          STATUS (DR6): %s\n",
-		     phex (dr_control_mirror, 8), phex (dr_status_mirror, 8));
+		     phex (state->dr_control_mirror, 8),
+		     phex (state->dr_status_mirror, 8));
   ALL_DEBUG_REGISTERS(i)
     {
       printf_unfiltered ("\
 \tDR%d: addr=0x%s, ref.count=%d  DR%d: addr=0x%s, ref.count=%d\n",
-		 i, phex (dr_mirror[i], addr_size), dr_ref_count[i],
-		 i+1, phex (dr_mirror[i+1], addr_size), dr_ref_count[i+1]);
+			 i, phex (state->dr_mirror[i], addr_size),
+			 state->dr_ref_count[i],
+			 i + 1, phex (state->dr_mirror[i + 1], addr_size),
+			 state->dr_ref_count[i+1]);
       i++;
     }
 }
@@ -311,7 +353,8 @@ Invalid hardware breakpoint length %d in
    success, -1 on failure.  */
 
 static int
-i386_insert_aligned_watchpoint (CORE_ADDR addr, unsigned len_rw_bits)
+i386_insert_aligned_watchpoint (struct i386_debug_reg_state *state,
+				CORE_ADDR addr, unsigned len_rw_bits)
 {
   int i;
 
@@ -323,11 +366,11 @@ i386_insert_aligned_watchpoint (CORE_ADD
      reuse it for this watchpoint as well (and save a register).  */
   ALL_DEBUG_REGISTERS(i)
     {
-      if (!I386_DR_VACANT (i)
-	  && dr_mirror[i] == addr
-	  && I386_DR_GET_RW_LEN (i) == len_rw_bits)
+      if (!I386_DR_VACANT (state, i)
+	  && state->dr_mirror[i] == addr
+	  && I386_DR_GET_RW_LEN (state->dr_control_mirror, i) == len_rw_bits)
 	{
-	  dr_ref_count[i]++;
+	  state->dr_ref_count[i]++;
 	  return 0;
 	}
     }
@@ -335,7 +378,7 @@ i386_insert_aligned_watchpoint (CORE_ADD
   /* Next, look for a vacant debug register.  */
   ALL_DEBUG_REGISTERS(i)
     {
-      if (I386_DR_VACANT (i))
+      if (I386_DR_VACANT (state, i))
 	break;
     }
 
@@ -346,9 +389,9 @@ i386_insert_aligned_watchpoint (CORE_ADD
   /* Now set up the register I to watch our region.  */
 
   /* Record the info in our local mirrored array.  */
-  dr_mirror[i] = addr;
-  dr_ref_count[i] = 1;
-  I386_DR_SET_RW_LEN (i, len_rw_bits);
+  state->dr_mirror[i] = addr;
+  state->dr_ref_count[i] = 1;
+  I386_DR_SET_RW_LEN (state, i, len_rw_bits);
   /* Note: we only enable the watchpoint locally, i.e. in the current
      task.  Currently, no i386 target allows or supports global
      watchpoints; however, if any target would want that in the
@@ -356,17 +399,9 @@ i386_insert_aligned_watchpoint (CORE_ADD
      to enable watchpoints globally or locally, and the code below
      should use global or local enable and slow-down flags as
      appropriate.  */
-  I386_DR_LOCAL_ENABLE (i);
-  dr_control_mirror |= DR_LOCAL_SLOWDOWN;
-  dr_control_mirror &= I386_DR_CONTROL_MASK;
-
-  /* Finally, actually pass the info to the inferior.  */
-  i386_dr_low.set_addr (i, addr);
-  i386_dr_low.set_control (dr_control_mirror);
-
-  /* Only a sanity check for leftover bits (set possibly only by inferior).  */
-  if (i386_dr_low.unset_status)
-    i386_dr_low.unset_status (I386_DR_WATCH_MASK (i));
+  I386_DR_LOCAL_ENABLE (state, i);
+  state->dr_control_mirror |= DR_LOCAL_SLOWDOWN;
+  state->dr_control_mirror &= I386_DR_CONTROL_MASK;
 
   return 0;
 }
@@ -378,25 +413,22 @@ i386_insert_aligned_watchpoint (CORE_ADD
    success, -1 on failure.  */
 
 static int
-i386_remove_aligned_watchpoint (CORE_ADDR addr, unsigned len_rw_bits)
+i386_remove_aligned_watchpoint (struct i386_debug_reg_state *state,
+				CORE_ADDR addr, unsigned len_rw_bits)
 {
   int i, retval = -1;
 
   ALL_DEBUG_REGISTERS(i)
     {
-      if (!I386_DR_VACANT (i)
-	  && dr_mirror[i] == addr
-	  && I386_DR_GET_RW_LEN (i) == len_rw_bits)
+      if (!I386_DR_VACANT (state, i)
+	  && state->dr_mirror[i] == addr
+	  && I386_DR_GET_RW_LEN (state->dr_control_mirror, i) == len_rw_bits)
 	{
-	  if (--dr_ref_count[i] == 0) /* no longer in use?  */
+	  if (--state->dr_ref_count[i] == 0) /* no longer in use?  */
 	    {
 	      /* Reset our mirror.  */
-	      dr_mirror[i] = 0;
-	      I386_DR_DISABLE (i);
-	      /* Reset it in the inferior.  */
-	      i386_dr_low.set_control (dr_control_mirror);
-	      if (i386_dr_low.reset_addr)
-		i386_dr_low.reset_addr (i);
+	      state->dr_mirror[i] = 0;
+	      I386_DR_DISABLE (state, i);
 	    }
 	  retval = 0;
 	}
@@ -413,10 +445,11 @@ i386_remove_aligned_watchpoint (CORE_ADD
    valid value, bombs through internal_error.  */
 
 static int
-i386_handle_nonaligned_watchpoint (i386_wp_op_t what, CORE_ADDR addr, int len,
+i386_handle_nonaligned_watchpoint (struct i386_debug_reg_state *state,
+				   i386_wp_op_t what, CORE_ADDR addr, int len,
 				   enum target_hw_bp_type type)
 {
-  int retval = 0, status = 0;
+  int retval = 0;
   int max_wp_len = TARGET_HAS_DR_LEN_8 ? 8 : 4;
 
   static int size_try_array[8][8] =
@@ -454,24 +487,15 @@ i386_handle_nonaligned_watchpoint (i386_
 	  unsigned len_rw = i386_length_and_rw_bits (size, type);
 
 	  if (what == WP_INSERT)
-	    status = i386_insert_aligned_watchpoint (addr, len_rw);
+	    retval = i386_insert_aligned_watchpoint (state, addr, len_rw);
 	  else if (what == WP_REMOVE)
-	    status = i386_remove_aligned_watchpoint (addr, len_rw);
+	    retval = i386_remove_aligned_watchpoint (state, addr, len_rw);
 	  else
 	    internal_error (__FILE__, __LINE__, _("\
 Invalid value %d of operation in i386_handle_nonaligned_watchpoint.\n"),
 			    (int)what);
-	  /* We keep the loop going even after a failure, because some
-	     of the other aligned watchpoints might still succeed
-	     (e.g. if they watch addresses that are already watched,
-	     in which case we just increment the reference counts of
-	     occupied debug registers).  If we break out of the loop
-	     too early, we could cause those addresses watched by
-	     other watchpoints to be disabled when breakpoint.c reacts
-	     to our failure to insert this watchpoint and tries to
-	     remove it.  */
-	  if (status)
-	    retval = status;
+	  if (retval)
+	    break;
 	}
 
       addr += size;
@@ -481,6 +505,35 @@ Invalid value %d of operation in i386_ha
   return retval;
 }
 
+/* Update the inferior debug registers state, in INF_STATE, with the
+   new debug registers state, in NEW_STATE.  */
+
+static void
+update_inferior (struct i386_debug_reg_state *state)
+{
+  int i;
+
+  ALL_DEBUG_REGISTERS (i)
+    {
+      if (state->dr_mirror[i] != dr_mirror.dr_mirror[i]
+	  || (state->dr_ref_count[i] != 0
+	      && dr_mirror.dr_ref_count[i] == 0))
+	{
+	  i386_dr_low.set_addr (i, state->dr_mirror[i]);
+
+	  /* Only a sanity check for leftover bits (set possibly only
+	     by inferior).  */
+	  if (i386_dr_low.unset_status)
+	    i386_dr_low.unset_status (I386_DR_WATCH_MASK (i));
+	}
+    }
+
+  if (state->dr_control_mirror != dr_mirror.dr_control_mirror)
+    i386_dr_low.set_control (state->dr_control_mirror);
+
+  dr_mirror = *state;
+}
+
 /* Insert a watchpoint to watch a memory region which starts at
    address ADDR and whose length is LEN bytes.  Watch memory accesses
    of the type TYPE.  Return 0 on success, -1 on failure.  */
@@ -490,22 +543,30 @@ i386_insert_watchpoint (CORE_ADDR addr,
 			struct expression *cond)
 {
   int retval;
+  /* Work on a local copy of the debug registers, and on success,
+     commit the change back to the inferior.  */
+  struct i386_debug_reg_state local_state = dr_mirror;
 
   if (type == hw_read)
     return 1; /* unsupported */
 
   if (((len != 1 && len !=2 && len !=4) && !(TARGET_HAS_DR_LEN_8 && len == 8))
       || addr % len != 0)
-    retval = i386_handle_nonaligned_watchpoint (WP_INSERT, addr, len, type);
+    retval = i386_handle_nonaligned_watchpoint (&local_state,
+						WP_INSERT, addr, len, type);
   else
     {
       unsigned len_rw = i386_length_and_rw_bits (len, type);
 
-      retval = i386_insert_aligned_watchpoint (addr, len_rw);
+      retval = i386_insert_aligned_watchpoint (&local_state,
+					       addr, len_rw);
     }
 
+  if (retval == 0)
+    update_inferior (&local_state);
+
   if (maint_show_dr)
-    i386_show_dr ("insert_watchpoint", addr, len, type);
+    i386_show_dr (&dr_mirror, "insert_watchpoint", addr, len, type);
 
   return retval;
 }
@@ -518,19 +579,27 @@ i386_remove_watchpoint (CORE_ADDR addr,
 			struct expression *cond)
 {
   int retval;
+  /* Work on a local copy of the debug registers, and on success,
+     commit the change back to the inferior.  */
+  struct i386_debug_reg_state local_state = dr_mirror;
 
   if (((len != 1 && len !=2 && len !=4) && !(TARGET_HAS_DR_LEN_8 && len == 8))
       || addr % len != 0)
-    retval = i386_handle_nonaligned_watchpoint (WP_REMOVE, addr, len, type);
+    retval = i386_handle_nonaligned_watchpoint (&local_state,
+						WP_REMOVE, addr, len, type);
   else
     {
       unsigned len_rw = i386_length_and_rw_bits (len, type);
 
-      retval = i386_remove_aligned_watchpoint (addr, len_rw);
+      retval = i386_remove_aligned_watchpoint (&local_state,
+					       addr, len_rw);
     }
 
+  if (retval == 0)
+    update_inferior (&local_state);
+
   if (maint_show_dr)
-    i386_show_dr ("remove_watchpoint", addr, len, type);
+    i386_show_dr (&dr_mirror, "remove_watchpoint", addr, len, type);
 
   return retval;
 }
@@ -545,7 +614,8 @@ i386_region_ok_for_watchpoint (CORE_ADDR
 
   /* Compute how many aligned watchpoints we would need to cover this
      region.  */
-  nregs = i386_handle_nonaligned_watchpoint (WP_COUNT, addr, len, hw_write);
+  nregs = i386_handle_nonaligned_watchpoint (&dr_mirror,
+					     WP_COUNT, addr, len, hw_write);
   return nregs <= DR_NADDR ? 1 : 0;
 }
 
@@ -559,30 +629,35 @@ i386_stopped_data_address (struct target
   CORE_ADDR addr = 0;
   int i;
   int rc = 0;
-
-  dr_status_mirror = i386_dr_low.get_status ();
+  unsigned status;
+  unsigned control;
+  struct i386_debug_reg_state *state = &dr_mirror;
+
+  dr_mirror.dr_status_mirror = i386_dr_low.get_status ();
+  status = dr_mirror.dr_status_mirror;
+  control = dr_mirror.dr_control_mirror;
 
   ALL_DEBUG_REGISTERS(i)
     {
-      if (I386_DR_WATCH_HIT (i)
+      if (I386_DR_WATCH_HIT (status, i)
 	  /* This second condition makes sure DRi is set up for a data
 	     watchpoint, not a hardware breakpoint.  The reason is
 	     that GDB doesn't call the target_stopped_data_address
 	     method except for data watchpoints.  In other words, I'm
 	     being paranoiac.  */
-	  && I386_DR_GET_RW_LEN (i) != 0
+	  && I386_DR_GET_RW_LEN (control, i) != 0
 	  /* This third condition makes sure DRi is not vacant, this
 	     avoids false positives in windows-nat.c.  */
-	  && !I386_DR_VACANT (i))
+	  && !I386_DR_VACANT (state, i))
 	{
-	  addr = dr_mirror[i];
+	  addr = state->dr_mirror[i];
 	  rc = 1;
 	  if (maint_show_dr)
-	    i386_show_dr ("watchpoint_hit", addr, -1, hw_write);
+	    i386_show_dr (&dr_mirror, "watchpoint_hit", addr, -1, hw_write);
 	}
     }
   if (maint_show_dr && addr == 0)
-    i386_show_dr ("stopped_data_addr", 0, 0, hw_write);
+    i386_show_dr (&dr_mirror, "stopped_data_addr", 0, 0, hw_write);
 
   if (rc)
     *addr_p = addr;
@@ -604,10 +679,11 @@ i386_insert_hw_breakpoint (struct gdbarc
 {
   unsigned len_rw = i386_length_and_rw_bits (1, hw_execute);
   CORE_ADDR addr = bp_tgt->placed_address;
-  int retval = i386_insert_aligned_watchpoint (addr, len_rw) ? EBUSY : 0;
+  int retval = i386_insert_aligned_watchpoint (&dr_mirror,
+					       addr, len_rw) ? EBUSY : 0;
 
   if (maint_show_dr)
-    i386_show_dr ("insert_hwbp", addr, 1, hw_execute);
+    i386_show_dr (&dr_mirror, "insert_hwbp", addr, 1, hw_execute);
 
   return retval;
 }
@@ -621,10 +697,11 @@ i386_remove_hw_breakpoint (struct gdbarc
 {
   unsigned len_rw = i386_length_and_rw_bits (1, hw_execute);
   CORE_ADDR addr = bp_tgt->placed_address;
-  int retval = i386_remove_aligned_watchpoint (addr, len_rw);
+  int retval = i386_remove_aligned_watchpoint (&dr_mirror,
+					       addr, len_rw);
 
   if (maint_show_dr)
-    i386_show_dr ("remove_hwbp", addr, 1, hw_execute);
+    i386_show_dr (&dr_mirror, "remove_hwbp", addr, 1, hw_execute);
 
   return retval;
 }

Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)

[Pedro Alves]

On Tuesday 31 May 2011 19:06:17, Philippe Waroquiers wrote:
> 
> > If that was the only problem, than it'd be okay --- the user just
> > shouldn't use the command then.  GDB will just do what the
> > user told it to.  But, it looks like the patch changes the
> > behavior _even_ if the user doesn't use the command.
> 
> Effectively, the patch changes the behaviour (but I believe in a more
> consistent way). But if that is considered as not good, I can change
> the patch so as to keep by default the old behaviour.

Thinking more about this, I agree.  The current default is
making it so that e.g., a single watchpoint on

 char s16[16];

is a sofware watchpoint against x86 gdbserver, but
it's a hardware watchpoint against native x86 gdb.  gdbserver
knows how to make that a hardware watchpoint, but gdb
is not giving it a chance --- the current default assumes you
can only set a hardware  watchpoint on a single word, but that's
not true on x86 gdbserver, given that the target knows to use
more than one debug register for a single watchpoint
if necessary.

> Note that one other thing that I find confusing in the current behaviour
> is that if you have a certain set of hw watchpoints that were accepted
> and you add a new one, you might obtain an error back referencing
> an "old" accepted watchpoint.
> 
> I think it would be better if the watchpoints would always be re-inserted
> by gdb in the same order.

I suppose that if we made insert_breakpoint_locations
walk breakpoints by increasing number instead of walking by
breakpoint locations, it'd be good enough.

-- 
Pedro Alves

Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)

[Philippe Waroquiers]

>> Note that one other thing that I find confusing in the current behaviour
>> is that if you have a certain set of hw watchpoints that were accepted
>> and you add a new one, you might obtain an error back referencing
>> an "old" accepted watchpoint.
>> 
>> I think it would be better if the watchpoints would always be re-inserted
>> by gdb in the same order.
> 
> I suppose that if we made insert_breakpoint_locations
> walk breakpoints by increasing number instead of walking by
> breakpoint locations, it'd be good enough.

Yes, I think this is avoiding the strange behaviour of having
an error on a previously accepted watchpoint.
If breakpoint always-inserted is on, then I thinkl the error will appear immediately,
which is also ok.

Thanks for the patch to fix the 'busy' debug register on x86, I should be able
to evaluate it end of this week.

Philippe

Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver))

[Philippe Waroquiers]

> Here it is.  New test, fixes for both gdbserver and gdb.
....
> Any comments on this?
>
> -- 
> Pedro Alves
>
> 2011-06-01  Pedro Alves  <pedro@codesourcery.com>


I think there is still something wrong with the DR registers, at least
with gdbserver.

First it looks like addr 0 is special, and keeps the register
busy. See below the trace of first the gdb session,
followed by the gdbserver session.

Reading symbols from /home/philippe/set-length-limit/s...done.
(gdb) tar rem :1234
Remote debugging using :1234
Reading symbols from /lib64/ld-linux-x86-64.so.2...(no debugging symbols found)...done.
Loaded symbols for /lib64/ld-linux-x86-64.so.2
0x00007ffff7de2a60 in ?? () from /lib64/ld-linux-x86-64.so.2
(gdb) set breakpoint always-inserted on
(gdb)  mo set debug-hw-points 1
H/W point debugging output enabled.
(gdb) watch * (int *) 0
Hardware watchpoint 1: * (int *) 0
(gdb) watch * (long *) 0
Hardware watchpoint 2: * (long *) 0
(gdb) watch * (int *) 8
Hardware watchpoint 3: * (int *) 8
(gdb) watch * (long *) 8
Hardware watchpoint 4: * (long *) 8
(gdb) del
Delete all breakpoints? (y or n) y
(gdb)


philippe@gcc10:~/set-length-limit/cvs/dr_busy_fix$ ./install/bin/gdbserver :1234 /home/philippe/set-length-limit/s
Process /home/philippe/set-length-limit/s created; pid = 20075
Listening on port 1234
Remote debugging from host 127.0.0.1
insert_watchpoint (addr=0, len=4, type=data-write):
 CONTROL (DR7): 000d0101          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=1  DR1: addr=0x0, ref.count=0
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
insert_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 009d0105          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=1  DR1: addr=0x0, ref.count=1
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
insert_watchpoint (addr=8, len=4, type=data-write):
 CONTROL (DR7): 0d9d0115          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=1  DR1: addr=0x0, ref.count=1
 DR2: addr=0x8, ref.count=1  DR3: addr=0x0, ref.count=0
insert_watchpoint (addr=8, len=8, type=data-write):
 CONTROL (DR7): 9d9d0155          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=1  DR1: addr=0x0, ref.count=1
 DR2: addr=0x8, ref.count=1  DR3: addr=0x8, ref.count=1
remove_watchpoint (addr=0, len=4, type=data-write):
 CONTROL (DR7): 9d9d0154          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=1  DR1: addr=0x0, ref.count=1
 DR2: addr=0x8, ref.count=1  DR3: addr=0x8, ref.count=1
remove_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 9d9d0150          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=1  DR1: addr=0x0, ref.count=1
 DR2: addr=0x8, ref.count=1  DR3: addr=0x8, ref.count=1
remove_watchpoint (addr=8, len=4, type=data-write):
 CONTROL (DR7): 9d9d0140          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=1  DR1: addr=0x0, ref.count=1
 DR2: addr=0x0, ref.count=0  DR3: addr=0x8, ref.count=1
remove_watchpoint (addr=8, len=8, type=data-write):
 CONTROL (DR7): 9d9d0100          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=1  DR1: addr=0x0, ref.count=1
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0

I suspect the problem might be in the following piece of code:
static void
update_inferior (struct i386_debug_reg_state *inf_state,
   struct i386_debug_reg_state *new_state)
{
  int i;

  ALL_DEBUG_REGISTERS (i)
    {
      if (new_state->dr_mirror[i] != inf_state->dr_mirror[i]
   || (new_state->dr_ref_count[i] != 0
       && inf_state->dr_ref_count[i] == 0))
 {

The dr_mirror is the address being watched.
But if address being watched is 0x0, then a 'busy' register
watching 0x0 and a non-busy register will have equal dr_mirror.
Then the || condition is bizarre as the ref.count will be updated
only if the current inf_state ref.count is 0.
It looks to me it would be clearer (and correct?) to always also
enter in the block updating really the inferior
if the dr_ref_counts are != (similarly to the dr_mirror).


After that, I have done some tests combining the patch fixing
the DR busy and the patch that allows to set the length
of remote hw watchpoints. This allows to test gdbserver
when it has to share low level watchpoints.
It looks like in this case, gdbserver does not properly
maintain the ref.count:
(!!!! so the below is with a gdb which includes the two
patches, so that a watchpoint of 16 bytes is accepted
by gdb and transmitted to gdbserver).


philippe@gcc10:~/set-length-limit/cvs/dr_busy_fix_and_set_length_from_src$ ./install/bin/gdb ~/set-length-limit/s
GNU gdb (GDB) 7.3.50.20110608-cvs
Copyright (C) 2011 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-unknown-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /home/philippe/set-length-limit/s...done.
(gdb) tar rem :1234
Remote debugging using :1234
Reading symbols from /lib64/ld-linux-x86-64.so.2...(no debugging symbols found)...done.
Loaded symbols for /lib64/ld-linux-x86-64.so.2
0x00007ffff7de2a60 in ?? () from /lib64/ld-linux-x86-64.so.2
(gdb) set breakpoint always-inserted on
(gdb) mo set debug-hw-points 1
H/W point debugging output enabled.
(gdb)  watch * (long *) 0x0
Hardware watchpoint 1: * (long *) 0x0
(gdb)  watch * (char (*)[16]) 0x0
Hardware watchpoint 2: * (char (*)[16]) 0x0
(gdb) del
Delete all breakpoints? (y or n) y
(gdb)

philippe@gcc10:~/set-length-limit/cvs/dr_busy_fix_and_set_length_from_src$ ./install/bin/gdbserver :1234 
/home/philippe/set-length-limit/s
Process /home/philippe/set-length-limit/s created; pid = 7504
Listening on port 1234
Remote debugging from host 127.0.0.1
insert_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 00090101          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=1  DR1: addr=0x0, ref.count=0
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
insert_watchpoint (addr=0, len=16, type=data-write):
 CONTROL (DR7): 00990105          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=1  DR1: addr=0x8, ref.count=1
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
remove_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 00990104          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=1  DR1: addr=0x8, ref.count=1
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
remove_watchpoint (addr=0, len=16, type=data-write):
 CONTROL (DR7): 00990104          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=1  DR1: addr=0x8, ref.count=1
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0


I double checked that I was testing with the gdbserver
containing the fix by attaching to gdbserver with another
gdb, and putting a break in the function update_inferior:
(gdb) attach 7499
Attaching to process 7499
Reading symbols from /home/philippe/set-length-limit/cvs/dr_busy_fix_and_set_length_from_src/install/bin/gdbserver...done.
Reading symbols from /lib/libdl.so.2...(no debugging symbols found)...done.
Loaded symbols for /lib/libdl.so.2
Reading symbols from /lib/libc.so.6...(no debugging symbols found)...done.
Loaded symbols for /lib/libc.so.6
Reading symbols from /lib64/ld-linux-x86-64.so.2...(no debugging symbols found)...done.
Loaded symbols for /lib64/ld-linux-x86-64.so.2
0x00007ffff7953d03 in select () from /lib/libc.so.6
(gdb) break update_inferior
Breakpoint 1 at 0x422010: file i386-low.c, line 441.
(gdb)

We see that the gdbserver is properly re-using a low level watchpoint
for a 2nd high level watchpoint, but the ref.count is kept to 1,
while it should go to 2.

We have similar ref.count problem with an address != 0:

philippe@gcc10:~/set-length-limit/cvs/dr_busy_fix_and_set_length_from_src$ ./install/bin/gdb ~/set-length-limit/s
GNU gdb (GDB) 7.3.50.20110608-cvs
Copyright (C) 2011 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-unknown-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /home/philippe/set-length-limit/s...done.
(gdb) tar rem :1234
Remote debugging using :1234
Reading symbols from /lib64/ld-linux-x86-64.so.2...(no debugging symbols found)...done.
Loaded symbols for /lib64/ld-linux-x86-64.so.2
0x00007ffff7de2a60 in ?? () from /lib64/ld-linux-x86-64.so.2
(gdb) set breakpoint always-inserted on
(gdb) mo set debug-hw-points 1
H/W point debugging output enabled.
(gdb) watch * (long *) 0x8
Hardware watchpoint 1: * (long *) 0x8
(gdb) watch * (char (*)[16]) 0x8
Hardware watchpoint 2: * (char (*)[16]) 0x8
(gdb) del
Delete all breakpoints? (y or n) y
(gdb)


Process /home/philippe/set-length-limit/s created; pid = 9959
Listening on port 1234
Remote debugging from host 127.0.0.1
insert_watchpoint (addr=8, len=8, type=data-write):
 CONTROL (DR7): 00090101          STATUS (DR6): 00000000
 DR0: addr=0x8, ref.count=1  DR1: addr=0x0, ref.count=0
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
insert_watchpoint (addr=8, len=16, type=data-write):
 CONTROL (DR7): 00990105          STATUS (DR6): 00000000
 DR0: addr=0x8, ref.count=1  DR1: addr=0x10, ref.count=1
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
remove_watchpoint (addr=8, len=8, type=data-write):
 CONTROL (DR7): 00990104          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=0  DR1: addr=0x10, ref.count=1
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
remove_watchpoint (addr=8, len=16, type=data-write):
 CONTROL (DR7): 00990104          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=0  DR1: addr=0x10, ref.count=1
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0

Maybe it would be possible to add some asserts in the handling
of the DR ? E.g. if the ref.count > 0, then the register must be 'active'
and if ref.count == 0, then register must be 'inactive' ?

Finally, a question about the below comment from a new test.
If I understood correctly, the 'single (low level)' should rather be 'single (high level)' ?

+# Regression test for a bug where the i386 watchpoints support backend
+# would leave some debug registers occupied even if not enough debug
+# registers were available to cover a single (low level) watchpoint.


Philippe

Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver))

[Pedro Alves]

On Wednesday 08 June 2011 23:55:11, Philippe Waroquiers wrote:
> I think there is still something wrong with the DR registers, at least
> with gdbserver.

Yes.  Many thanks for noticing this.

> I suspect the problem might be in the following piece of code:
> static void
> update_inferior (struct i386_debug_reg_state *inf_state,
>    struct i386_debug_reg_state *new_state)
> {
>   int i;
> 
>   ALL_DEBUG_REGISTERS (i)
>     {
>       if (new_state->dr_mirror[i] != inf_state->dr_mirror[i]
>    || (new_state->dr_ref_count[i] != 0
>        && inf_state->dr_ref_count[i] == 0))
>  {
> 
> The dr_mirror is the address being watched.
> But if address being watched is 0x0, then a 'busy' register
> watching 0x0 and a non-busy register will have equal dr_mirror.
> Then the || condition is bizarre as the ref.count will be updated
> only if the current inf_state ref.count is 0.

Not the ref.count.  The address to watch, DR[0-3].

> It looks to me it would be clearer (and correct?) to always also
> enter in the block updating really the inferior
> if the dr_ref_counts are != (similarly to the dr_mirror).

The point was that we need to set the DR[0-3] register
when activating the watchpoint.  If going from refcount
1 to 2, say, then the address hasn't changed, and, we
already know it contains the correct address (set when
we went from 0 to 1).

The problem is actually that I forgot one important line of
code when porting the changes from gdb to gdbserver.

Index: src/gdb/gdbserver/i386-low.c
===================================================================
--- src.orig/gdb/gdbserver/i386-low.c   2011-06-01 15:19:13.000000000 +0100
+++ src/gdb/gdbserver/i386-low.c        2011-06-09 00:36:18.008128016 +0100
@@ -458,6 +458,8 @@ update_inferior (struct i386_debug_reg_s
       inf_state->dr_control_mirror = new_state->dr_control_mirror;
       i386_dr_low_set_control (inf_state);
     }
+
+  *inf_state = *new_state;
 }
 
 /* Insert a watchpoint to watch a memory region which starts at

That is, I missed the "commit" from the mirror of the mirror to
the regular process mirror.  It's present on the gdb side
of the changes as:

static void
update_inferior (struct i386_debug_reg_state *state)
{
...
  dr_mirror = *state;
}

Sorry about that. :-P

I'll take a better look at this tomorrow and check if I
missed anything else.  I'll try to add yet some other test
that catches this.

> Maybe it would be possible to add some asserts in the handling
> of the DR ? E.g. if the ref.count > 0, then the register must be 'active'
> and if ref.count == 0, then register must be 'inactive' ?

Sounds like a good idea.

> Finally, a question about the below comment from a new test.
> If I understood correctly, the 'single (low level)' should rather be 'single (high level)' ?
> 
> +# Regression test for a bug where the i386 watchpoints support backend
> +# would leave some debug registers occupied even if not enough debug
> +# registers were available to cover a single (low level) watchpoint.

In the context of that comment, a high level watchpoint would be
a watchpoint on the GDB side (e.g., watch $expression creates
one high level watchpoint, with possibly many low level watchpoints,
or watchpoint locations).  This expression may require watching more
than one memory region.  Each of those regions will end up as a watchpoint
location, and a "low level" watchpoint inserted on the target for each
of them (a Z2 packet for each).  The bug happens when not enough
debug registers are available to cover a single Z2 packet / low level
watchpoint.  So the comment was correct if you think of high and
low level watchpoints like I was thinking.  Maybe you were thinking
of a high level watchpoint as what the target sees?

-- 
Pedro Alves

Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver))

[Philippe Waroquiers]

Thanks for the feedback, behaviour looks to be better
when adding the missing assignment. But I suspect I found another
bug in the area of "high level" to "low level" to "hw level" watchpoints.

>> I suspect the problem might be in the following piece of code:
>> static void
>> update_inferior (struct i386_debug_reg_state *inf_state,
>>    struct i386_debug_reg_state *new_state)
>> {
>>   int i;
>> 
>>   ALL_DEBUG_REGISTERS (i)
>>     {
>>       if (new_state->dr_mirror[i] != inf_state->dr_mirror[i]
>>    || (new_state->dr_ref_count[i] != 0
>>        && inf_state->dr_ref_count[i] == 0))
>>  {
>> 
>> The dr_mirror is the address being watched.
>> But if address being watched is 0x0, then a 'busy' register
>> watching 0x0 and a non-busy register will have equal dr_mirror.
>> Then the || condition is bizarre as the ref.count will be updated
>> only if the current inf_state ref.count is 0.
> 
> Not the ref.count.  The address to watch, DR[0-3].

Without the  *inf_state = *new_state,
I had some difficulties to understand the above code.

From what I understand now, the idea of this piece of code
is (only) to change the real value of the hw register.
But if inf_state->dr_mirror properly mirrors the value of the hw
register, then the inequality of the dr_mirror[i] should
be good enough to detect the need to change the hw register.

And if setting the address is only to be done when activating
the watchpoint, then the inequality on the ref count should be
good enough (and the assert new_state->dr_ref_count[i] == 1
should hold when changing the hw addr value).

Well it seems I still have difficulty to understand the code :).

But even if I do not understand the code, after adding 
the *inf_state = *new_state, the behaviour looks better.

>> +# registers were available to cover a single (low level) watchpoint.
> watchpoint.  So the comment was correct if you think of high and
> low level watchpoints like I was thinking.  Maybe you were thinking
> of a high level watchpoint as what the target sees?

Yes, I was interpreting "high level" being a Z2 packet, and "low level"
being the "hw" watchpoint.
Now, I understand that we have:
   high level watchpoints = "user defined watchpoints in gdb" = watched expressions
   low level watchpoints = "memory region watchpoints needed to implement the high level" = Z2 packets
   hw level  watchpoints =  "hw watchpoint(s) needed to implement the low level watchpoint(s)"

Thanks for the clarification


Doing some additional checks, I found something else slightly strange, but it seems
to be wrong at the mapping between "high level watchpoints" and "low level watchpoints".

In the below, you see that 3 identical watchpoints results in 1 single Z2
packet, but disabling the 3 watchpoints gives 3 z2 packets (sent
when the last watchpoint is disabled).
The reason for this assymetry looks not very clear to me, but that might
just be an implementation detail.
I however suspect there is still a bug, as after, when sharing a debug register
between two non-identical watchpoints, we are losing a part of the to be watched
zone : we still have a user level watchpoint of 16 bytes at 0x0, but the hw registers
are only watching 8 bytes at 0x8.
(the below is done with a patched gdb/gdbserver containing the "dr busy" fix
+ the missing assignment + the "set length" patch to allow watching 16 bytes
with gdbserver).
Note that the bug seems also present in native debugging (see native gdb session at the end).



(gdb) tar rem :1234
Remote debugging using :1234
Reading symbols from /lib64/ld-linux-x86-64.so.2...(no debugging symbols found)...done.
Loaded symbols for /lib64/ld-linux-x86-64.so.2
0x00007ffff7de2a60 in ?? () from /lib64/ld-linux-x86-64.so.2
(gdb) set breakpoint always-inserted  on
(gdb) mo set debug-hw-points 1
H/W point debugging output enabled.
(gdb) watch *(long *) 0x0
Hardware watchpoint 1: *(long *) 0x0
(gdb) watch *(long *) 0x0
Hardware watchpoint 2: *(long *) 0x0
(gdb) watch *(long *) 0x0
Hardware watchpoint 3: *(long *) 0x0
(gdb) dis 1
(gdb) dis 2
(gdb) dis 3
(gdb) enabl
(gdb) watch * (char (*)[16]) 0x0
Hardware watchpoint 4: * (char (*)[16]) 0x0
(gdb) dis 1
(gdb) dis 2
(gdb) dis 3
(gdb) 


Listening on port 1234
Remote debugging from host 127.0.0.1
insert_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 00090101          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=1  DR1: addr=0x0, ref.count=0
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
remove_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 00090100          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=0  DR1: addr=0x0, ref.count=0
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
remove_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 00090100          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=0  DR1: addr=0x0, ref.count=0
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
remove_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 00090100          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=0  DR1: addr=0x0, ref.count=0
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
insert_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 00090101          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=1  DR1: addr=0x0, ref.count=0
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
insert_watchpoint (addr=0, len=16, type=data-write):
 CONTROL (DR7): 00990105          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=2  DR1: addr=0x8, ref.count=1
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
remove_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 00990105          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=1  DR1: addr=0x8, ref.count=1
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
remove_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 00990104          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=0  DR1: addr=0x8, ref.count=1
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
remove_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 00990104          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=0  DR1: addr=0x8, ref.count=1
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0



(gdb) watch * (char (*)[16]) 0x0
Hardware watchpoint 6: * (char (*)[16]) 0x0
insert_watchpoint (addr=0, len=16, type=data-write):
 CONTROL (DR7): 0000000000990105          STATUS (DR6): 00000000ffff4ff0
 DR0: addr=0x0000000000000000, ref.count=2  DR1: addr=0x0000000000000008, ref.count=1
 DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0
(gdb) info wat
Num     Type           Disp Enb Address            What
1       hw watchpoint  keep y                      *(long *) 0x0
3       hw watchpoint  keep y                      *(long *) 0x0
4       hw watchpoint  keep y                      *(long *) 0x0
6       hw watchpoint  keep y                      * (char (*)[16]) 0x0
(gdb) dis 1
(gdb) dis 3
(gdb) dis 4
remove_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 0000000000990105          STATUS (DR6): 00000000ffff4ff0
 DR0: addr=0x0000000000000000, ref.count=1  DR1: addr=0x0000000000000008, ref.count=1
 DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0
remove_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 0000000000990104          STATUS (DR6): 00000000ffff4ff0
 DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x0000000000000008, ref.count=1
 DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0
remove_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 0000000000990104          STATUS (DR6): 00000000ffff4ff0
 DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x0000000000000008, ref.count=1
 DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0
(gdb) info watch
Num     Type           Disp Enb Address            What
1       hw watchpoint  keep n                      *(long *) 0x0
3       hw watchpoint  keep n                      *(long *) 0x0
4       hw watchpoint  keep n                      *(long *) 0x0
6       hw watchpoint  keep y                      * (char (*)[16]) 0x0
(gdb) 

Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver))

[Pedro Alves]

Hi Philippe.

I'm very sorry in the time it is taking to get this over with...

On Thursday 09 June 2011 23:16:35, Philippe Waroquiers wrote:
> Thanks for the feedback, behaviour looks to be better
> when adding the missing assignment. But I suspect I found another
> bug in the area of "high level" to "low level" to "hw level" watchpoints.
> 
> >> I suspect the problem might be in the following piece of code:
> >> static void
> >> update_inferior (struct i386_debug_reg_state *inf_state,
> >>    struct i386_debug_reg_state *new_state)
> >> {
> >>   int i;
> >> 
> >>   ALL_DEBUG_REGISTERS (i)
> >>     {
> >>       if (new_state->dr_mirror[i] != inf_state->dr_mirror[i]
> >>    || (new_state->dr_ref_count[i] != 0
> >>        && inf_state->dr_ref_count[i] == 0))
> >>  {
> >> 
> >> The dr_mirror is the address being watched.
> >> But if address being watched is 0x0, then a 'busy' register
> >> watching 0x0 and a non-busy register will have equal dr_mirror.
> >> Then the || condition is bizarre as the ref.count will be updated
> >> only if the current inf_state ref.count is 0.
> > 
> > Not the ref.count.  The address to watch, DR[0-3].
> 
> Without the  *inf_state = *new_state,
> I had some difficulties to understand the above code.
> 
> From what I understand now, the idea of this piece of code
> is (only) to change the real value of the hw register.
> But if inf_state->dr_mirror properly mirrors the value of the hw
> register, then the inequality of the dr_mirror[i] should
> be good enough to detect the need to change the hw register.
> 
> And if setting the address is only to be done when activating
> the watchpoint, then the inequality on the ref count should be
> good enough (and the assert new_state->dr_ref_count[i] == 1
> should hold when changing the hw addr value).
> 
> Well it seems I still have difficulty to understand the code :).

You are right, that wasn't clear.  This version of the function
is modelled on what you wrote above, with a twist -- we just check
if the register is transitioning from used or not used, a bit
easier to read than the refcounts checks.

The gdb native version now also better mirrors what the unpatched
code is doing, so is a bit different (gdbserver doesn't have the
unset_status or reset_addr callbacks).

I've renamed the function i386_update_inferior_debug_regs, btw.

This centralization opens the possibility of replacing most of
the i386_dr_low interface callbacks for a single callback that takes
a i386_debug_reg_state pointer (or an argument for each debug register)
afterwards, given that we now inform the target about changes all
in a single place -- that may save a few ptrace calls per watchpoint.

> >> +# registers were available to cover a single (low level) watchpoint.
> > watchpoint.  So the comment was correct if you think of high and
> > low level watchpoints like I was thinking.  Maybe you were thinking
> > of a high level watchpoint as what the target sees?
> 
> Yes, I was interpreting "high level" being a Z2 packet, and "low level"
> being the "hw" watchpoint.
> Now, I understand that we have:
>    high level watchpoints = "user defined watchpoints in gdb" = watched expressions
>    low level watchpoints = "memory region watchpoints needed to implement the high level" = Z2 packets
>    hw level  watchpoints =  "hw watchpoint(s) needed to implement the low level watchpoint(s)"
> 
> Thanks for the clarification

Ah, now it's clear why we were talking past each other.  I think I might
as well stop avoiding GDB's internal terminology for "low level
watchpoints" -- it's a watchpoint location.  I've tweaked the comment
accordingly.

I think nothing else changed in the patch.

> Doing some additional checks, I found something else slightly strange, but it seems
> to be wrong at the mapping between "high level watchpoints" and "low level watchpoints".
> 
> In the below, you see that 3 identical watchpoints results in 1 single Z2
> packet, but disabling the 3 watchpoints gives 3 z2 packets (sent
> when the last watchpoint is disabled).
> The reason for this assymetry looks not very clear to me, but that might
> just be an implementation detail.
> I however suspect there is still a bug, as after, when sharing a debug register
> between two non-identical watchpoints, we are losing a part of the to be watched
> zone : we still have a user level watchpoint of 16 bytes at 0x0, but the hw registers
> are only watching 8 bytes at 0x8.
> (the below is done with a patched gdb/gdbserver containing the "dr busy" fix
> + the missing assignment + the "set length" patch to allow watching 16 bytes
> with gdbserver).
> Note that the bug seems also present in native debugging (see native gdb session at the end).

Very nice catch.  Yes, pushing out 3 z2 packets is very wrong, and
is most certainly what is messing up the 16 bytes watchpoint -- that
watchpoint should have got spread out across two debug registers (which
can only watch 8 byte long regions), those extra z2 packets messed
up the refcount of the first register.  

I've debugged this a little, and it all looks like the bug is
within breakpoint.c:update_global_location_list, which isn't
handling duplicate locations across "disabled" breakpoints
correctly.  This not watchpoint specific, and, triggers
on 7.2 as well.  E.g., on 7.2, with breakpoints always inserted
on, triggering the bug with regular breakpoints:

...
Sending packet: $z0,4004b8,1#95...Packet received: OK
(gdb) b main
Breakpoint 2 at 0x4004b8: file s.c, line 22.
Sending packet: $Z0,4004b8,1#75...Packet received: OK
(gdb) b main
Note: breakpoint 2 also set at pc 0x4004b8.
Breakpoint 3 at 0x4004b8: file s.c, line 22.
(gdb) disable 2
(gdb) disable 3
Sending packet: $z0,4004b8,1#95...Packet received: OK
Sending packet: $z0,4004b8,1#95...Packet received: E01
warning: Error removing breakpoint 3
(gdb) 

GDB failed to realize that the locations were duplicate,
and that only one should have been removed --- there should
have been one z0 only...  Bummer.

Given that's an unrelated issue, this patch stands on its
own.  Could you run your sharp eye through it one more
time, please?

-- 
Pedro Alves

2011-07-21  Pedro Alves  <pedro@codesourcery.com>

	gdb/testsuite/
	* gdb.arch/i386-dr3-watch.exp: Test that the i386 watchpoints
	backend doesn't leave used debug registers behind.

	gdb/gdbserver/
	* i386-low.c (i386_insert_aligned_watchpoint): Don't pass the info
	to the inferior here.
	(i386_remove_aligned_watchpoint): Ditto.
	(i386_handle_nonaligned_watchpoint): Return immediate on fail to
	fit part of the watchpoint in the debug registers.
	(i386_update_inferior_debug_regs): New.
	(i386_low_insert_watchpoint): Work on a local mirror of the debug
	registers, and only update the inferior on success.
	(i386_low_remove_watchpoint): Ditto.

	gdb/
	* i386-nat.c (I386_DR_VACANT, I386_DR_LOCAL_ENABLE)
	(I386_DR_GLOBAL_ENABLE, I386_DR_DISABLE, I386_DR_SET_RW_LEN)
	(I386_DR_GET_RW_LEN, I386_DR_WATCH_HIT): Add state parameter and
	adjust.
	(dr_mirror, dr_status_mirror, dr_control_mirror): Delete.
	(struct i386_debug_reg_state): New.
	(i386_init_dregs): New.
	(dr_mirror): New.
	(i386_cleanup_dregs): Use i386_init_dregs.
	(i386_show_dr): Add state parameter and adjust.
	(i386_insert_aligned_watchpoint): Ditto.  Don't pass the info to
	the inferior here.
	(i386_remove_aligned_watchpoint): Likewise.
	(i386_handle_nonaligned_watchpoint): Add state parameter and adjust.
	(i386_update_inferior_debug_regs): New.
	(i386_insert_watchpoint): Work on a local mirror of the debug
	registers, and only update the inferior on success.
	(i386_remove_watchpoint): Ditto.
	(i386_region_ok_for_watchpoint): Adjust.
	(i386_stopped_data_address): Adjust.
	(i386_insert_hw_breakpoint): Adjust.
	(i386_remove_hw_breakpoint): Adjust.

---
 gdb/gdbserver/i386-low.c                  |   69 ++++---
 gdb/i386-nat.c                            |  283 +++++++++++++++++++-----------
 gdb/testsuite/gdb.arch/i386-dr3-watch.exp |   44 ++++
 3 files changed, 272 insertions(+), 124 deletions(-)

Index: src/gdb/testsuite/gdb.arch/i386-dr3-watch.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.arch/i386-dr3-watch.exp	2011-07-21 16:21:25.000000000 +0100
+++ src/gdb/testsuite/gdb.arch/i386-dr3-watch.exp	2011-07-21 16:56:08.773673926 +0100
@@ -38,6 +38,8 @@ if ![runto_main] then {
 
 gdb_test_no_output "set breakpoint always-inserted on"
 
+# Test that we handle watchpoints in all of DR0-DR3.
+
 gdb_test "watch i1" "Hardware watchpoint .*: i1"
 gdb_test "watch i2" "Hardware watchpoint .*: i2"
 gdb_test "watch i3" "Hardware watchpoint .*: i3"
@@ -47,3 +49,45 @@ gdb_test "c" "Hardware watchpoint.*: i1.
 gdb_test "c" "Hardware watchpoint.*: i2.*" "continue to i2 watchpoint"
 gdb_test "c" "Hardware watchpoint.*: i3.*" "continue to i3 watchpoint"
 gdb_test "c" "Hardware watchpoint.*: i4.*" "continue to i4 watchpoint"
+
+delete_breakpoints
+
+# Regression test for a bug where the i386 watchpoints support backend
+# would leave some debug registers occupied even if not enough debug
+# registers were available to cover a single watchpoint location.
+
+gdb_test "watch i1" \
+    "Hardware watchpoint .*: i1" \
+    "set watchpoint occuping one debug register"
+
+# gap1 too long to fit the 3 left over debug registers (but would fit
+# 4 if all were available).
+set test "watchpoint on gap1 does not fit debug registers"
+gdb_test_multiple "watch gap1" "$test" {
+    -re "Hardware watchpoint .*: gap1.*Warning:.*Could not insert hardware watchpoint.*You may have requested too many.*" {
+	pass $test
+    }
+    -re "Hardware watchpoint .*: gap1\r\n$gdb_prompt $" {
+	pass "$test (target emulates hardware watchpoints)"
+	return
+    }
+    -re "Watchpoint .*: gap1\r\n$gdb_prompt $" {
+	pass "$test (gdb figured out itself the watchpoint does not fit)"
+	return
+    }
+}
+
+# Start over.
+gdb_test "delete" \
+    "" \
+    "delete all watchpoints" \
+    "Delete all breakpoints.*$" \
+    "y"
+
+# If debug registers were left occupied by mistake, we'll fail to set
+# some of these watchpoints.  Each watchpoint should fit in one of the
+# 4 debug registers available.
+gdb_test "watch i1" "Hardware watchpoint .*: i1" "watch i1 still fits"
+gdb_test "watch i2" "Hardware watchpoint .*: i2" "watch i2 still fits"
+gdb_test "watch i3" "Hardware watchpoint .*: i3" "watch i3 still fits"
+gdb_test "watch i4" "Hardware watchpoint .*: i4" "watch i4 still fits"
Index: src/gdb/gdbserver/i386-low.c
===================================================================
--- src.orig/gdb/gdbserver/i386-low.c	2011-07-21 16:21:25.000000000 +0100
+++ src/gdb/gdbserver/i386-low.c	2011-07-21 16:56:08.773673926 +0100
@@ -307,10 +307,6 @@ i386_insert_aligned_watchpoint (struct i
   state->dr_control_mirror |= DR_LOCAL_SLOWDOWN;
   state->dr_control_mirror &= I386_DR_CONTROL_MASK;
 
-  /* Finally, actually pass the info to the inferior.  */
-  i386_dr_low_set_addr (state, i);
-  i386_dr_low_set_control (state);
-
   return 0;
 }
 
@@ -337,9 +333,6 @@ i386_remove_aligned_watchpoint (struct i
 	      /* Reset our mirror.  */
 	      state->dr_mirror[i] = 0;
 	      I386_DR_DISABLE (state, i);
-	      /* Reset it in the inferior.  */
-	      i386_dr_low_set_control (state);
-	      i386_dr_low_set_addr (state, i);
 	    }
 	  retval = 0;
 	}
@@ -360,7 +353,7 @@ i386_handle_nonaligned_watchpoint (struc
 				   i386_wp_op_t what, CORE_ADDR addr, int len,
 				   enum target_hw_bp_type type)
 {
-  int retval = 0, status = 0;
+  int retval = 0;
   int max_wp_len = TARGET_HAS_DR_LEN_8 ? 8 : 4;
 
   static const int size_try_array[8][8] =
@@ -398,25 +391,16 @@ i386_handle_nonaligned_watchpoint (struc
 	  unsigned len_rw = i386_length_and_rw_bits (size, type);
 
 	  if (what == WP_INSERT)
-	    status = i386_insert_aligned_watchpoint (state, addr, len_rw);
+	    retval = i386_insert_aligned_watchpoint (state, addr, len_rw);
 	  else if (what == WP_REMOVE)
-	    status = i386_remove_aligned_watchpoint (state, addr, len_rw);
+	    retval = i386_remove_aligned_watchpoint (state, addr, len_rw);
 	  else
 	    fatal ("\
 Invalid value %d of operation in i386_handle_nonaligned_watchpoint.\n",
 		   (int) what);
 
-	  /* We keep the loop going even after a failure, because some
-	     of the other aligned watchpoints might still succeed
-	     (e.g. if they watch addresses that are already watched,
-	     in which case we just increment the reference counts of
-	     occupied debug registers).  If we break out of the loop
-	     too early, we could cause those addresses watched by
-	     other watchpoints to be disabled when breakpoint.c reacts
-	     to our failure to insert this watchpoint and tries to
-	     remove it.  */
-	  if (status)
-	    retval = status;
+	  if (retval)
+	    break;
 	}
 
       addr += size;
@@ -448,6 +432,29 @@ Z_packet_to_hw_type (char type)
     }
 }
 
+/* Update the inferior debug registers state, in INF_STATE, with the
+   new debug registers state, in NEW_STATE.  */
+
+static void
+i386_update_inferior_debug_regs (struct i386_debug_reg_state *inf_state,
+				 struct i386_debug_reg_state *new_state)
+{
+  int i;
+
+  ALL_DEBUG_REGISTERS (i)
+    {
+      if (I386_DR_VACANT (new_state, i) != I386_DR_VACANT (inf_state, i))
+	i386_dr_low_set_addr (new_state, i);
+      else
+	gdb_assert (new_state->dr_mirror[i] == inf_state->dr_mirror[i]);
+    }
+
+  if (new_state->dr_control_mirror != inf_state->dr_control_mirror)
+    i386_dr_low_set_control (new_state);
+
+  *inf_state = *new_state;
+}
+
 /* Insert a watchpoint to watch a memory region which starts at
    address ADDR and whose length is LEN bytes.  Watch memory accesses
    of the type TYPE_FROM_PACKET.  Return 0 on success, -1 on failure.  */
@@ -458,6 +465,9 @@ i386_low_insert_watchpoint (struct i386_
 {
   int retval;
   enum target_hw_bp_type type = Z_packet_to_hw_type (type_from_packet);
+  /* Work on a local copy of the debug registers, and on success,
+     commit the change back to the inferior.  */
+  struct i386_debug_reg_state local_state = *state;
 
   if (type == hw_read)
     return 1; /* unsupported */
@@ -466,16 +476,19 @@ i386_low_insert_watchpoint (struct i386_
        && !(TARGET_HAS_DR_LEN_8 && len == 8))
       || addr % len != 0)
     {
-      retval = i386_handle_nonaligned_watchpoint (state, WP_INSERT,
+      retval = i386_handle_nonaligned_watchpoint (&local_state, WP_INSERT,
 						  addr, len, type);
     }
   else
     {
       unsigned len_rw = i386_length_and_rw_bits (len, type);
 
-      retval = i386_insert_aligned_watchpoint (state, addr, len_rw);
+      retval = i386_insert_aligned_watchpoint (&local_state, addr, len_rw);
     }
 
+  if (retval == 0)
+    i386_update_inferior_debug_regs (state, &local_state);
+
   if (debug_hw_points)
     i386_show_dr (state, "insert_watchpoint", addr, len, type);
 
@@ -492,21 +505,27 @@ i386_low_remove_watchpoint (struct i386_
 {
   int retval;
   enum target_hw_bp_type type = Z_packet_to_hw_type (type_from_packet);
+  /* Work on a local copy of the debug registers, and on success,
+     commit the change back to the inferior.  */
+  struct i386_debug_reg_state local_state = *state;
 
   if (((len != 1 && len != 2 && len != 4)
        && !(TARGET_HAS_DR_LEN_8 && len == 8))
       || addr % len != 0)
     {
-      retval = i386_handle_nonaligned_watchpoint (state, WP_REMOVE,
+      retval = i386_handle_nonaligned_watchpoint (&local_state, WP_REMOVE,
 						  addr, len, type);
     }
   else
     {
       unsigned len_rw = i386_length_and_rw_bits (len, type);
 
-      retval = i386_remove_aligned_watchpoint (state, addr, len_rw);
+      retval = i386_remove_aligned_watchpoint (&local_state, addr, len_rw);
     }
 
+  if (retval == 0)
+    i386_update_inferior_debug_regs (state, &local_state);
+
   if (debug_hw_points)
     i386_show_dr (state, "remove_watchpoint", addr, len, type);
 
Index: src/gdb/i386-nat.c
===================================================================
--- src.orig/gdb/i386-nat.c	2011-07-21 16:21:25.000000000 +0100
+++ src/gdb/i386-nat.c	2011-07-21 16:56:08.773673926 +0100
@@ -111,45 +111,88 @@ struct i386_dr_low_type i386_dr_low;
 
 /* The I'th debug register is vacant if its Local and Global Enable
    bits are reset in the Debug Control register.  */
-#define I386_DR_VACANT(i) \
-  ((dr_control_mirror & (3 << (DR_ENABLE_SIZE * (i)))) == 0)
+#define I386_DR_VACANT(state, i)					\
+  (((state)->dr_control_mirror & (3 << (DR_ENABLE_SIZE * (i)))) == 0)
 
 /* Locally enable the break/watchpoint in the I'th debug register.  */
-#define I386_DR_LOCAL_ENABLE(i) \
-  dr_control_mirror |= (1 << (DR_LOCAL_ENABLE_SHIFT + DR_ENABLE_SIZE * (i)))
+#define I386_DR_LOCAL_ENABLE(state, i) \
+  do { \
+    (state)->dr_control_mirror |= \
+      (1 << (DR_LOCAL_ENABLE_SHIFT + DR_ENABLE_SIZE * (i))); \
+  } while (0)
 
 /* Globally enable the break/watchpoint in the I'th debug register.  */
-#define I386_DR_GLOBAL_ENABLE(i) \
-  dr_control_mirror |= (1 << (DR_GLOBAL_ENABLE_SHIFT + DR_ENABLE_SIZE * (i)))
+#define I386_DR_GLOBAL_ENABLE(state, i) \
+  do { \
+    (state)->dr_control_mirror |= \
+      (1 << (DR_GLOBAL_ENABLE_SHIFT + DR_ENABLE_SIZE * (i))); \
+  } while (0)
 
 /* Disable the break/watchpoint in the I'th debug register.  */
-#define I386_DR_DISABLE(i) \
-  dr_control_mirror &= ~(3 << (DR_ENABLE_SIZE * (i)))
+#define I386_DR_DISABLE(state, i) \
+  do { \
+    (state)->dr_control_mirror &= \
+      ~(3 << (DR_ENABLE_SIZE * (i))); \
+  } while (0)
 
 /* Set in DR7 the RW and LEN fields for the I'th debug register.  */
-#define I386_DR_SET_RW_LEN(i,rwlen) \
+#define I386_DR_SET_RW_LEN(state, i, rwlen) \
   do { \
-    dr_control_mirror &= ~(0x0f << (DR_CONTROL_SHIFT+DR_CONTROL_SIZE*(i)));   \
-    dr_control_mirror |= ((rwlen) << (DR_CONTROL_SHIFT+DR_CONTROL_SIZE*(i))); \
+    (state)->dr_control_mirror &= \
+      ~(0x0f << (DR_CONTROL_SHIFT + DR_CONTROL_SIZE * (i))); \
+    (state)->dr_control_mirror |= \
+      ((rwlen) << (DR_CONTROL_SHIFT + DR_CONTROL_SIZE * (i))); \
   } while (0)
 
 /* Get from DR7 the RW and LEN fields for the I'th debug register.  */
-#define I386_DR_GET_RW_LEN(i) \
-  ((dr_control_mirror >> (DR_CONTROL_SHIFT + DR_CONTROL_SIZE * (i))) & 0x0f)
+#define I386_DR_GET_RW_LEN(dr7, i) \
+  (((dr7) \
+    >> (DR_CONTROL_SHIFT + DR_CONTROL_SIZE * (i))) & 0x0f)
 
 /* Mask that this I'th watchpoint has triggered.  */
 #define I386_DR_WATCH_MASK(i)	(1 << (i))
 
 /* Did the watchpoint whose address is in the I'th register break?  */
-#define I386_DR_WATCH_HIT(i)	(dr_status_mirror & I386_DR_WATCH_MASK (i))
+#define I386_DR_WATCH_HIT(dr6, i) ((dr6) & (1 << (i)))
 
 /* A macro to loop over all debug registers.  */
 #define ALL_DEBUG_REGISTERS(i)	for (i = 0; i < DR_NADDR; i++)
 
-/* Mirror the inferior's DRi registers.  We keep the status and
-   control registers separated because they don't hold addresses.  */
-static CORE_ADDR dr_mirror[DR_NADDR];
-static unsigned long dr_status_mirror, dr_control_mirror;
+
+/* Global state needed to track h/w watchpoints.  */
+
+struct i386_debug_reg_state
+{
+  /* Mirror the inferior's DRi registers.  We keep the status and
+     control registers separated because they don't hold addresses.
+     Note that since we can change these mirrors while threads are
+     running, we never trust them to explain a cause of a trap.
+     For that, we need to peek directly in the inferior registers.  */
+  CORE_ADDR dr_mirror[DR_NADDR];
+  unsigned dr_status_mirror, dr_control_mirror;
+
+  /* Reference counts for each debug register.  */
+  int dr_ref_count[DR_NADDR];
+};
+
+/* Clear the reference counts and forget everything we knew about the
+   debug registers.  */
+
+static void
+i386_init_dregs (struct i386_debug_reg_state *state)
+{
+  int i;
+
+  ALL_DEBUG_REGISTERS (i)
+    {
+      state->dr_mirror[i] = 0;
+      state->dr_ref_count[i] = 0;
+    }
+  state->dr_control_mirror = 0;
+  state->dr_status_mirror  = 0;
+}
+
+static struct i386_debug_reg_state dr_mirror;
 
 /* Reference counts for each debug register.  */
 static int dr_ref_count[DR_NADDR];
@@ -172,7 +215,8 @@ static unsigned i386_length_and_rw_bits
    value of the bit-field from DR7 which describes the length and
    access type of the region to be watched by this watchpoint.  Return
    0 on success, -1 on failure.  */
-static int i386_insert_aligned_watchpoint (CORE_ADDR addr,
+static int i386_insert_aligned_watchpoint (struct i386_debug_reg_state *state,
+					   CORE_ADDR addr,
 					   unsigned len_rw_bits);
 
 /* Remove a watchpoint at address ADDR, which is assumed to be aligned
@@ -180,7 +224,8 @@ static int i386_insert_aligned_watchpoin
    value of the bits from DR7 which describes the length and access
    type of the region watched by this watchpoint.  Return 0 on
    success, -1 on failure.  */
-static int i386_remove_aligned_watchpoint (CORE_ADDR addr,
+static int i386_remove_aligned_watchpoint (struct i386_debug_reg_state *state,
+					   CORE_ADDR addr,
 					   unsigned len_rw_bits);
 
 /* Insert or remove a (possibly non-aligned) watchpoint, or count the
@@ -189,7 +234,8 @@ static int i386_remove_aligned_watchpoin
    successful insertion or removal, a positive number when queried
    about the number of registers, or -1 on failure.  If WHAT is not a
    valid value, bombs through internal_error.  */
-static int i386_handle_nonaligned_watchpoint (i386_wp_op_t what,
+static int i386_handle_nonaligned_watchpoint (struct i386_debug_reg_state *state,
+					      i386_wp_op_t what,
 					      CORE_ADDR addr, int len,
 					      enum target_hw_bp_type type);
 
@@ -201,15 +247,7 @@ static int i386_handle_nonaligned_watchp
 void
 i386_cleanup_dregs (void)
 {
-  int i;
-
-  ALL_DEBUG_REGISTERS(i)
-    {
-      dr_mirror[i] = 0;
-      dr_ref_count[i] = 0;
-    }
-  dr_control_mirror = 0;
-  dr_status_mirror  = 0;
+  i386_init_dregs (&dr_mirror);
 }
 
 /* Print the values of the mirrored debug registers.  This is called
@@ -217,7 +255,8 @@ i386_cleanup_dregs (void)
    show-debug-regs" at GDB's prompt.  */
 
 static void
-i386_show_dr (const char *func, CORE_ADDR addr,
+i386_show_dr (struct i386_debug_reg_state *state,
+	      const char *func, CORE_ADDR addr,
 	      int len, enum target_hw_bp_type type)
 {
   int addr_size = gdbarch_addr_bit (target_gdbarch) / 8;
@@ -239,13 +278,16 @@ i386_show_dr (const char *func, CORE_ADD
 				: "??unknown??"))));
   puts_unfiltered (":\n");
   printf_unfiltered ("\tCONTROL (DR7): %s          STATUS (DR6): %s\n",
-		     phex (dr_control_mirror, 8), phex (dr_status_mirror, 8));
+		     phex (state->dr_control_mirror, 8),
+		     phex (state->dr_status_mirror, 8));
   ALL_DEBUG_REGISTERS(i)
     {
       printf_unfiltered ("\
 \tDR%d: addr=0x%s, ref.count=%d  DR%d: addr=0x%s, ref.count=%d\n",
-		 i, phex (dr_mirror[i], addr_size), dr_ref_count[i],
-		 i+1, phex (dr_mirror[i+1], addr_size), dr_ref_count[i+1]);
+			 i, phex (state->dr_mirror[i], addr_size),
+			 state->dr_ref_count[i],
+			 i + 1, phex (state->dr_mirror[i + 1], addr_size),
+			 state->dr_ref_count[i+1]);
       i++;
     }
 }
@@ -311,7 +353,8 @@ Invalid hardware breakpoint length %d in
    success, -1 on failure.  */
 
 static int
-i386_insert_aligned_watchpoint (CORE_ADDR addr, unsigned len_rw_bits)
+i386_insert_aligned_watchpoint (struct i386_debug_reg_state *state,
+				CORE_ADDR addr, unsigned len_rw_bits)
 {
   int i;
 
@@ -323,11 +366,11 @@ i386_insert_aligned_watchpoint (CORE_ADD
      reuse it for this watchpoint as well (and save a register).  */
   ALL_DEBUG_REGISTERS(i)
     {
-      if (!I386_DR_VACANT (i)
-	  && dr_mirror[i] == addr
-	  && I386_DR_GET_RW_LEN (i) == len_rw_bits)
+      if (!I386_DR_VACANT (state, i)
+	  && state->dr_mirror[i] == addr
+	  && I386_DR_GET_RW_LEN (state->dr_control_mirror, i) == len_rw_bits)
 	{
-	  dr_ref_count[i]++;
+	  state->dr_ref_count[i]++;
 	  return 0;
 	}
     }
@@ -335,7 +378,7 @@ i386_insert_aligned_watchpoint (CORE_ADD
   /* Next, look for a vacant debug register.  */
   ALL_DEBUG_REGISTERS(i)
     {
-      if (I386_DR_VACANT (i))
+      if (I386_DR_VACANT (state, i))
 	break;
     }
 
@@ -346,9 +389,9 @@ i386_insert_aligned_watchpoint (CORE_ADD
   /* Now set up the register I to watch our region.  */
 
   /* Record the info in our local mirrored array.  */
-  dr_mirror[i] = addr;
-  dr_ref_count[i] = 1;
-  I386_DR_SET_RW_LEN (i, len_rw_bits);
+  state->dr_mirror[i] = addr;
+  state->dr_ref_count[i] = 1;
+  I386_DR_SET_RW_LEN (state, i, len_rw_bits);
   /* Note: we only enable the watchpoint locally, i.e. in the current
      task.  Currently, no i386 target allows or supports global
      watchpoints; however, if any target would want that in the
@@ -356,17 +399,9 @@ i386_insert_aligned_watchpoint (CORE_ADD
      to enable watchpoints globally or locally, and the code below
      should use global or local enable and slow-down flags as
      appropriate.  */
-  I386_DR_LOCAL_ENABLE (i);
-  dr_control_mirror |= DR_LOCAL_SLOWDOWN;
-  dr_control_mirror &= I386_DR_CONTROL_MASK;
-
-  /* Finally, actually pass the info to the inferior.  */
-  i386_dr_low.set_addr (i, addr);
-  i386_dr_low.set_control (dr_control_mirror);
-
-  /* Only a sanity check for leftover bits (set possibly only by inferior).  */
-  if (i386_dr_low.unset_status)
-    i386_dr_low.unset_status (I386_DR_WATCH_MASK (i));
+  I386_DR_LOCAL_ENABLE (state, i);
+  state->dr_control_mirror |= DR_LOCAL_SLOWDOWN;
+  state->dr_control_mirror &= I386_DR_CONTROL_MASK;
 
   return 0;
 }
@@ -378,25 +413,22 @@ i386_insert_aligned_watchpoint (CORE_ADD
    success, -1 on failure.  */
 
 static int
-i386_remove_aligned_watchpoint (CORE_ADDR addr, unsigned len_rw_bits)
+i386_remove_aligned_watchpoint (struct i386_debug_reg_state *state,
+				CORE_ADDR addr, unsigned len_rw_bits)
 {
   int i, retval = -1;
 
   ALL_DEBUG_REGISTERS(i)
     {
-      if (!I386_DR_VACANT (i)
-	  && dr_mirror[i] == addr
-	  && I386_DR_GET_RW_LEN (i) == len_rw_bits)
+      if (!I386_DR_VACANT (state, i)
+	  && state->dr_mirror[i] == addr
+	  && I386_DR_GET_RW_LEN (state->dr_control_mirror, i) == len_rw_bits)
 	{
-	  if (--dr_ref_count[i] == 0) /* no longer in use?  */
+	  if (--state->dr_ref_count[i] == 0) /* no longer in use?  */
 	    {
 	      /* Reset our mirror.  */
-	      dr_mirror[i] = 0;
-	      I386_DR_DISABLE (i);
-	      /* Reset it in the inferior.  */
-	      i386_dr_low.set_control (dr_control_mirror);
-	      if (i386_dr_low.reset_addr)
-		i386_dr_low.reset_addr (i);
+	      state->dr_mirror[i] = 0;
+	      I386_DR_DISABLE (state, i);
 	    }
 	  retval = 0;
 	}
@@ -413,10 +445,11 @@ i386_remove_aligned_watchpoint (CORE_ADD
    valid value, bombs through internal_error.  */
 
 static int
-i386_handle_nonaligned_watchpoint (i386_wp_op_t what, CORE_ADDR addr, int len,
+i386_handle_nonaligned_watchpoint (struct i386_debug_reg_state *state,
+				   i386_wp_op_t what, CORE_ADDR addr, int len,
 				   enum target_hw_bp_type type)
 {
-  int retval = 0, status = 0;
+  int retval = 0;
   int max_wp_len = TARGET_HAS_DR_LEN_8 ? 8 : 4;
 
   static int size_try_array[8][8] =
@@ -454,24 +487,15 @@ i386_handle_nonaligned_watchpoint (i386_
 	  unsigned len_rw = i386_length_and_rw_bits (size, type);
 
 	  if (what == WP_INSERT)
-	    status = i386_insert_aligned_watchpoint (addr, len_rw);
+	    retval = i386_insert_aligned_watchpoint (state, addr, len_rw);
 	  else if (what == WP_REMOVE)
-	    status = i386_remove_aligned_watchpoint (addr, len_rw);
+	    retval = i386_remove_aligned_watchpoint (state, addr, len_rw);
 	  else
 	    internal_error (__FILE__, __LINE__, _("\
 Invalid value %d of operation in i386_handle_nonaligned_watchpoint.\n"),
 			    (int)what);
-	  /* We keep the loop going even after a failure, because some
-	     of the other aligned watchpoints might still succeed
-	     (e.g. if they watch addresses that are already watched,
-	     in which case we just increment the reference counts of
-	     occupied debug registers).  If we break out of the loop
-	     too early, we could cause those addresses watched by
-	     other watchpoints to be disabled when breakpoint.c reacts
-	     to our failure to insert this watchpoint and tries to
-	     remove it.  */
-	  if (status)
-	    retval = status;
+	  if (retval)
+	    break;
 	}
 
       addr += size;
@@ -481,6 +505,43 @@ Invalid value %d of operation in i386_ha
   return retval;
 }
 
+/* Update the inferior debug registers state, in INF_STATE, with the
+   new debug registers state, in NEW_STATE.  */
+
+static void
+i386_update_inferior_debug_regs (struct i386_debug_reg_state *new_state)
+{
+  int i;
+
+  ALL_DEBUG_REGISTERS (i)
+    {
+      if (I386_DR_VACANT (new_state, i) != I386_DR_VACANT (&dr_mirror, i))
+	{
+	  if (!I386_DR_VACANT (new_state, i))
+	    {
+	      i386_dr_low.set_addr (i, new_state->dr_mirror[i]);
+
+	      /* Only a sanity check for leftover bits (set possibly only
+		 by inferior).  */
+	      if (i386_dr_low.unset_status)
+		i386_dr_low.unset_status (I386_DR_WATCH_MASK (i));
+	    }
+	  else
+	    {
+	      if (i386_dr_low.reset_addr)
+		i386_dr_low.reset_addr (i);
+	    }
+	}
+      else
+	gdb_assert (new_state->dr_mirror[i] == dr_mirror.dr_mirror[i]);
+    }
+
+  if (new_state->dr_control_mirror != dr_mirror.dr_control_mirror)
+    i386_dr_low.set_control (new_state->dr_control_mirror);
+
+  dr_mirror = *new_state;
+}
+
 /* Insert a watchpoint to watch a memory region which starts at
    address ADDR and whose length is LEN bytes.  Watch memory accesses
    of the type TYPE.  Return 0 on success, -1 on failure.  */
@@ -490,22 +551,30 @@ i386_insert_watchpoint (CORE_ADDR addr,
 			struct expression *cond)
 {
   int retval;
+  /* Work on a local copy of the debug registers, and on success,
+     commit the change back to the inferior.  */
+  struct i386_debug_reg_state local_state = dr_mirror;
 
   if (type == hw_read)
     return 1; /* unsupported */
 
   if (((len != 1 && len !=2 && len !=4) && !(TARGET_HAS_DR_LEN_8 && len == 8))
       || addr % len != 0)
-    retval = i386_handle_nonaligned_watchpoint (WP_INSERT, addr, len, type);
+    retval = i386_handle_nonaligned_watchpoint (&local_state,
+						WP_INSERT, addr, len, type);
   else
     {
       unsigned len_rw = i386_length_and_rw_bits (len, type);
 
-      retval = i386_insert_aligned_watchpoint (addr, len_rw);
+      retval = i386_insert_aligned_watchpoint (&local_state,
+					       addr, len_rw);
     }
 
+  if (retval == 0)
+    i386_update_inferior_debug_regs (&local_state);
+
   if (maint_show_dr)
-    i386_show_dr ("insert_watchpoint", addr, len, type);
+    i386_show_dr (&dr_mirror, "insert_watchpoint", addr, len, type);
 
   return retval;
 }
@@ -518,19 +587,27 @@ i386_remove_watchpoint (CORE_ADDR addr,
 			struct expression *cond)
 {
   int retval;
+  /* Work on a local copy of the debug registers, and on success,
+     commit the change back to the inferior.  */
+  struct i386_debug_reg_state local_state = dr_mirror;
 
   if (((len != 1 && len !=2 && len !=4) && !(TARGET_HAS_DR_LEN_8 && len == 8))
       || addr % len != 0)
-    retval = i386_handle_nonaligned_watchpoint (WP_REMOVE, addr, len, type);
+    retval = i386_handle_nonaligned_watchpoint (&local_state,
+						WP_REMOVE, addr, len, type);
   else
     {
       unsigned len_rw = i386_length_and_rw_bits (len, type);
 
-      retval = i386_remove_aligned_watchpoint (addr, len_rw);
+      retval = i386_remove_aligned_watchpoint (&local_state,
+					       addr, len_rw);
     }
 
+  if (retval == 0)
+    i386_update_inferior_debug_regs (&local_state);
+
   if (maint_show_dr)
-    i386_show_dr ("remove_watchpoint", addr, len, type);
+    i386_show_dr (&dr_mirror, "remove_watchpoint", addr, len, type);
 
   return retval;
 }
@@ -545,7 +622,8 @@ i386_region_ok_for_watchpoint (CORE_ADDR
 
   /* Compute how many aligned watchpoints we would need to cover this
      region.  */
-  nregs = i386_handle_nonaligned_watchpoint (WP_COUNT, addr, len, hw_write);
+  nregs = i386_handle_nonaligned_watchpoint (&dr_mirror,
+					     WP_COUNT, addr, len, hw_write);
   return nregs <= DR_NADDR ? 1 : 0;
 }
 
@@ -559,30 +637,35 @@ i386_stopped_data_address (struct target
   CORE_ADDR addr = 0;
   int i;
   int rc = 0;
-
-  dr_status_mirror = i386_dr_low.get_status ();
+  unsigned status;
+  unsigned control;
+  struct i386_debug_reg_state *state = &dr_mirror;
+
+  dr_mirror.dr_status_mirror = i386_dr_low.get_status ();
+  status = dr_mirror.dr_status_mirror;
+  control = dr_mirror.dr_control_mirror;
 
   ALL_DEBUG_REGISTERS(i)
     {
-      if (I386_DR_WATCH_HIT (i)
+      if (I386_DR_WATCH_HIT (status, i)
 	  /* This second condition makes sure DRi is set up for a data
 	     watchpoint, not a hardware breakpoint.  The reason is
 	     that GDB doesn't call the target_stopped_data_address
 	     method except for data watchpoints.  In other words, I'm
 	     being paranoiac.  */
-	  && I386_DR_GET_RW_LEN (i) != 0
+	  && I386_DR_GET_RW_LEN (control, i) != 0
 	  /* This third condition makes sure DRi is not vacant, this
 	     avoids false positives in windows-nat.c.  */
-	  && !I386_DR_VACANT (i))
+	  && !I386_DR_VACANT (state, i))
 	{
-	  addr = dr_mirror[i];
+	  addr = state->dr_mirror[i];
 	  rc = 1;
 	  if (maint_show_dr)
-	    i386_show_dr ("watchpoint_hit", addr, -1, hw_write);
+	    i386_show_dr (&dr_mirror, "watchpoint_hit", addr, -1, hw_write);
 	}
     }
   if (maint_show_dr && addr == 0)
-    i386_show_dr ("stopped_data_addr", 0, 0, hw_write);
+    i386_show_dr (&dr_mirror, "stopped_data_addr", 0, 0, hw_write);
 
   if (rc)
     *addr_p = addr;
@@ -604,10 +687,11 @@ i386_insert_hw_breakpoint (struct gdbarc
 {
   unsigned len_rw = i386_length_and_rw_bits (1, hw_execute);
   CORE_ADDR addr = bp_tgt->placed_address;
-  int retval = i386_insert_aligned_watchpoint (addr, len_rw) ? EBUSY : 0;
+  int retval = i386_insert_aligned_watchpoint (&dr_mirror,
+					       addr, len_rw) ? EBUSY : 0;
 
   if (maint_show_dr)
-    i386_show_dr ("insert_hwbp", addr, 1, hw_execute);
+    i386_show_dr (&dr_mirror, "insert_hwbp", addr, 1, hw_execute);
 
   return retval;
 }
@@ -621,10 +705,11 @@ i386_remove_hw_breakpoint (struct gdbarc
 {
   unsigned len_rw = i386_length_and_rw_bits (1, hw_execute);
   CORE_ADDR addr = bp_tgt->placed_address;
-  int retval = i386_remove_aligned_watchpoint (addr, len_rw);
+  int retval = i386_remove_aligned_watchpoint (&dr_mirror,
+					       addr, len_rw);
 
   if (maint_show_dr)
-    i386_show_dr ("remove_hwbp", addr, 1, hw_execute);
+    i386_show_dr (&dr_mirror, "remove_hwbp", addr, 1, hw_execute);
 
   return retval;
 }

Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver))

[Philippe Waroquiers]

> I think nothing else changed in the patch.

I looked at the new patch and re-tested on f12/x86 and debian5/amd64, using 7.3.
Behaviour looks ok to me regarding the handling of debug registers.
(note I tested with the patch allowing to change the remote hw watchpoint length,
which I believe could be committed soon : FSF papers ok, waiting for a user now).

During the testing, I however found something else slightly strange.
With reference to the previous s.c test program, watching a string length 1000
is ok at the start (handled as a sw breakpoint), but this watchpoint cannot be disabled
then re-enabled:
   (gdb) watch s1000
   Hardware watchpoint 1: s1000
   (gdb) start     <<<<<<<<<<<<<<<<<<<<<<<<<< this runs slowly as s1000 is sw-watched
   Temporary breakpoint 2 at 0x400480: file s.c, line 22.
   Starting program: /home/philippe/gdb/s 
   Error in re-setting breakpoint 1: Expression cannot be implemented with read/access watchpoint.
   Error in re-setting breakpoint 1: Expression cannot be implemented with read/access watchpoint.
   Error in re-setting breakpoint 1: Expression cannot be implemented with read/access watchpoint.

   Temporary breakpoint 2, main () at s.c:22
   22    char * p = s1000;
   (gdb) dis 1
   (gdb) ena 1
   Cannot enable watchpoint 1: Expression cannot be implemented with read/access watchpoint.
   (gdb) 
At this point, if the watchpoint is deleted then re-created, then the watchpoint is again 'sw-accepted'.
Note that this looks to be a regression in 7.3.50.20110722-cvs, as I do not see the same problem on 7.2.
This regression is not linked with the DR patch (occurs both with the patched/non patched 7.3.50).

So, in summary:
  * the patch for the i386 debug register fix looks ok.
The following strange behaviours/bugs have still to be fixed or looked at:
  * handling of duplicate locations across disabled breakpoints
     (resulting in wrongly duplicated z packets and/or missing active debug registers in native)
  * watch s1000 then run then disable then enable impossible

 There was also a 'nice to have' which could be looked at:
  * ensure that the insertion of watchpoint is done using the order of breakpoints
     (so as to not have a new watchpoint causing an error/rejection on a previously accepted
      watchpoint).

Thanks for all the work

Philippe

Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver))

[Pedro Alves]

On Friday 22 July 2011 17:02:42, Philippe Waroquiers wrote:

> During the testing, I however found something else slightly strange.
> With reference to the previous s.c test program, watching a string length 1000
> is ok at the start (handled as a sw breakpoint), but this watchpoint cannot be disabled
> then re-enabled:
>    (gdb) watch s1000
>    Hardware watchpoint 1: s1000
>    (gdb) start     <<<<<<<<<<<<<<<<<<<<<<<<<< this runs slowly as s1000 is sw-watched
>    Temporary breakpoint 2 at 0x400480: file s.c, line 22.
>    Starting program: /home/philippe/gdb/s 
>    Error in re-setting breakpoint 1: Expression cannot be implemented with read/access watchpoint.
>    Error in re-setting breakpoint 1: Expression cannot be implemented with read/access watchpoint.
>    Error in re-setting breakpoint 1: Expression cannot be implemented with read/access watchpoint.
> 
>    Temporary breakpoint 2, main () at s.c:22
>    22    char * p = s1000;
>    (gdb) dis 1
>    (gdb) ena 1
>    Cannot enable watchpoint 1: Expression cannot be implemented with read/access watchpoint.
>    (gdb) 
> At this point, if the watchpoint is deleted then re-created, then the watchpoint is again 'sw-accepted'.
> Note that this looks to be a regression in 7.3.50.20110722-cvs, as I do not see the same problem on 7.2.

Hmm, I can reproduce this.  

#0  error (string=...) at ../../src/gdb/utils.c:776
#1  0x000000000064733d in update_watchpoint (b=0x1f4fde0, reparse=1) at ../../src/gdb/breakpoint.c:1456
#2  0x000000000065a364 in enable_breakpoint_disp (bpt=0x1f4fde0, disposition=disp_donttouch) at ../../src/gdb/breakpoint.c:11917

1453                }
1454              else if (b->ops && b->ops->works_in_software_mode
1455                       && !b->ops->works_in_software_mode (b))
1456                error (_("Expression cannot be implemented with "
1457                         "read/access watchpoint."));
1458              else
1459                b->type = bp_watchpoint;

(top-gdb) info symbol b->ops
watchpoint_breakpoint_ops in section .data of /home/pedro/gdb/baseline/build/gdb/gdb

(top-gdb) info symbol b->ops->works_in_software_mode
works_in_software_mode_watchpoint in section .text of /home/pedro/gdb/baseline/build/gdb/gdb

int
works_in_software_mode_watchpoint (const struct breakpoint *b)
{
  return b->type == bp_hardware_watchpoint;
}

(top-gdb) p b->type 
$5 = bp_watchpoint

From the error string, looks like the check should be something like:

              else if (b->type == bp_read_watchpoint
                       || b->type == bp_access_watchpoint)
                error (_("Expression cannot be implemented with "
                         "read/access watchpoint."));

instead, as those watchpoints can't indeed be implemented
as software watchpoints.  Though the intention may have
been to catch something about masked watchpoints.
Maybe better would be to change works_in_software_mode_watchpoint to:

int
works_in_software_mode_watchpoint (const struct breakpoint *b)
{
-  return b->type == bp_hardware_watchpoint;
+  return (b->type == bp_watchpoint || b->type == bp_hardware_watchpoint);
}

The error string could also be enhanced to include the real
watchpoint type (so a user of masked watchpoints doesn't get
confused).

Thiago, WDYT?

-- 
Pedro Alves

Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver))

[Pedro Alves]

On Friday 22 July 2011 17:02:42, Philippe Waroquiers wrote:
> > I think nothing else changed in the patch.
> 
> I looked at the new patch and re-tested on f12/x86 and debian5/amd64, using 7.3.
> Behaviour looks ok to me regarding the handling of debug registers.

Awesome.  I've applied it now.

> So, in summary:

> The following strange behaviours/bugs have still to be fixed or looked at:
>   * handling of duplicate locations across disabled breakpoints
>      (resulting in wrongly duplicated z packets and/or missing active debug registers in native)
>   * watch s1000 then run then disable then enable impossible
> 
>  There was also a 'nice to have' which could be looked at:
>   * ensure that the insertion of watchpoint is done using the order of breakpoints
>      (so as to not have a new watchpoint causing an error/rejection on a previously accepted
>       watchpoint).

For avoidance of doubt, I'm not working on any of
those issues at the moment (and I don't know when/if I will).
If you don't plan to, it'll be a good idea to record them in bugzilla.
Actually, it'll be a good idea in any case.

> Thanks for all the work

Thank you!  

-- 
Pedro Alves

Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver))

[Thiago Jung Bauermann]

On Fri, 2011-07-22 at 17:40 +0100, Pedro Alves wrote:
> On Friday 22 July 2011 17:02:42, Philippe Waroquiers wrote:
> int
> works_in_software_mode_watchpoint (const struct breakpoint *b)
> {
>   return b->type == bp_hardware_watchpoint;
> }
> 
> (top-gdb) p b->type 
> $5 = bp_watchpoint
> 
> From the error string, looks like the check should be something like:
> 
>               else if (b->type == bp_read_watchpoint
>                        || b->type == bp_access_watchpoint)
>                 error (_("Expression cannot be implemented with "
>                          "read/access watchpoint."));
> 
> instead, as those watchpoints can't indeed be implemented
> as software watchpoints.  Though the intention may have
> been to catch something about masked watchpoints.

Yes, that was indeed the intention. And I agree that the error string is
wrong when it is shown for a masked watchpoint (which can happen if
can-use-hw-watchpoints is 0).

> Maybe better would be to change works_in_software_mode_watchpoint to:
> 
> int
> works_in_software_mode_watchpoint (const struct breakpoint *b)
> {
> -  return b->type == bp_hardware_watchpoint;
> +  return (b->type == bp_watchpoint || b->type == bp_hardware_watchpoint);
> }

Agreed. I would only comment that the parenthesis are not necessary. :-)

Theoretically resources_needed_watchpoint would have to be adapted for
software watchpoints too, but in practice that function is only called
in hw_watchpoint_used_count, which is never called with bp_watchpoint as
an argument.

FWIW, my local branch with my rework of debug registers accounting
doesn't have hw_watchpoint_used_count anymore.

> The error string could also be enhanced to include the real
> watchpoint type (so a user of masked watchpoints doesn't get
> confused).

I tried to keep that code agnostic to the type of watchpoint at hand
(hence the breakpoint_ops methods), so what about a more generic error
message, like "There is no hardware debug support for this watchpoint."
or "Expression cannot be implemented with hardware debug resources."?

Otherwise, we could use something like:

	  else if (b->type == bp_read_watchpoint
		   || b->type == bp_access_watchpoint)
	    error (_("Expression cannot be implemented with "
		     "read/access watchpoint."));
	  else if (is_masked_watchpoint (b))
	    error (_("Expression cannot be implemented with masked watchpoint."));
 	  else if (b->ops && b->ops->works_in_software_mode
		   && !b->ops->works_in_software_mode (b))
	    error (_("Expression cannot be implemented with this type of watchpoint."));
	  else
	    b->type = bp_watchpoint;

The last else if is currently dead code, since only regular watchpoints
and masked watchpoints implement the works_in_software_mode method. So
either it or the one above it could be dropped. Or the last one could
replace all the else ifs above it.

I don't have a strong opinion on this one. Pick what you think is more
reasonable.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

software watchpoints bug (was: Re: x86 watchpoints bug)

[Pedro Alves]

On Saturday 23 July 2011 03:38:55, Thiago Jung Bauermann wrote:
> On Fri, 2011-07-22 at 17:40 +0100, Pedro Alves wrote:

> > Maybe better would be to change works_in_software_mode_watchpoint to:
> > 
> > int
> > works_in_software_mode_watchpoint (const struct breakpoint *b)
> > {
> > -  return b->type == bp_hardware_watchpoint;
> > +  return (b->type == bp_watchpoint || b->type == bp_hardware_watchpoint);
> > }
> 
> Agreed. I would only comment that the parenthesis are not necessary. :-)

Alright! :-)  I've applied the patch below, which does that, and
adds a new test.

> 
> Theoretically resources_needed_watchpoint would have to be adapted for
> software watchpoints too, but in practice that function is only called
> in hw_watchpoint_used_count, which is never called with bp_watchpoint as
> an argument.
> 
> FWIW, my local branch with my rework of debug registers accounting
> doesn't have hw_watchpoint_used_count anymore.

Oooh!  Is that branch somewhere public?

> 
> > The error string could also be enhanced to include the real
> > watchpoint type (so a user of masked watchpoints doesn't get
> > confused).
> 
> I tried to keep that code agnostic to the type of watchpoint at hand
> (hence the breakpoint_ops methods), so what about a more generic error
> message, like "There is no hardware debug support for this watchpoint."
> or "Expression cannot be implemented with hardware debug resources."?

...
> 	    error (_("Expression cannot be implemented with this type of watchpoint."));

Yeah, I'd go the path of making the error string more generic.  Perhaps
s/implemented/watched/.

-- 
Pedro Alves

2011-07-26  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* breakpoint.c (works_in_software_mode_watchpoint): Also return
	true for software watchpoints.

	gdb/testsuite/
	* gdb.base/watchpoint.exp
	(test_disable_enable_software_watchpoint): New procedure.
	(top level): Run it.

---
 gdb/breakpoint.c                      |    3 ++-
 gdb/testsuite/gdb.base/watchpoint.exp |   19 +++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

Index: src/gdb/testsuite/gdb.base/watchpoint.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.base/watchpoint.exp	2011-07-26 20:34:05.271448492 +0100
+++ src/gdb/testsuite/gdb.base/watchpoint.exp	2011-07-26 20:38:49.661448542 +0100
@@ -630,6 +630,23 @@ proc test_constant_watchpoint {} {
     gdb_test_no_output "delete \$bpnum" "delete watchpoint `7 + count'"
 }
 
+proc test_disable_enable_software_watchpoint {} {
+    # This is regression test for a bug that caused `enable' to fail
+    # for software watchpoints.
+
+    # Watch something not memory to force a software watchpoint.
+    gdb_test {watch $pc} ".*atchpoint \[0-9\]+: .pc"
+
+    gdb_test_no_output "disable \$bpnum" "disable watchpoint `\$pc'"
+    gdb_test_no_output "enable \$bpnum" "reenable watchpoint `\$pc'"
+
+    gdb_test "info watchpoint \$bpnum" \
+	".*watchpoint\[ \t\]+keep\[ \t\]+y\[ \t\]+.pc.*" \
+	"watchpoint `\$pc' is enabled"
+
+    gdb_test_no_output "delete \$bpnum" "delete watchpoint `\$pc'"
+}
+
 proc test_watch_location {} {
     gdb_breakpoint [gdb_get_line_number "func5 breakpoint here"]
     gdb_continue_to_breakpoint "func5 breakpoint here"
@@ -903,6 +920,8 @@ if [initialize] then {
 
     test_constant_watchpoint
 
+    test_disable_enable_software_watchpoint
+
     test_watch_location
 }
 
Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2011-07-26 20:34:05.271448492 +0100
+++ src/gdb/breakpoint.c	2011-07-26 20:36:48.211448520 +0100
@@ -8637,7 +8637,8 @@ resources_needed_watchpoint (const struc
 static int
 works_in_software_mode_watchpoint (const struct breakpoint *b)
 {
-  return b->type == bp_hardware_watchpoint;
+  /* Read and access watchpoints only work with hardware support.  */
+  return b->type == bp_watchpoint || b->type == bp_hardware_watchpoint;
 }
 
 static enum print_stop_action

Re: software watchpoints bug (was: Re: x86 watchpoints bug)

[Thiago Jung Bauermann]

On Tue, 2011-07-26 at 20:46 +0100, Pedro Alves wrote:
> On Saturday 23 July 2011 03:38:55, Thiago Jung Bauermann wrote:
> > On Fri, 2011-07-22 at 17:40 +0100, Pedro Alves wrote:
> 
> > > Maybe better would be to change works_in_software_mode_watchpoint to:
> > > 
> > > int
> > > works_in_software_mode_watchpoint (const struct breakpoint *b)
> > > {
> > > -  return b->type == bp_hardware_watchpoint;
> > > +  return (b->type == bp_watchpoint || b->type == bp_hardware_watchpoint);
> > > }
> > 
> > Agreed. I would only comment that the parenthesis are not necessary. :-)
> 
> Alright! :-)  I've applied the patch below, which does that, and
> adds a new test.

Great, thanks for the patch.
 
> > Theoretically resources_needed_watchpoint would have to be adapted for
> > software watchpoints too, but in practice that function is only called
> > in hw_watchpoint_used_count, which is never called with bp_watchpoint as
> > an argument.
> > 
> > FWIW, my local branch with my rework of debug registers accounting
> > doesn't have hw_watchpoint_used_count anymore.
> 
> Oooh!  Is that branch somewhere public?

Not yet. I think I just finished ironing out the bugs from my changes
related to watchpoints. I'm now starting to work on breakpoints.

I was thinking of posting the watchpoints patch as an RFC. I'll rebase
the branch on top of current master to get your breakpoint_ops changes
(great work BTW) and post it.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center