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

["Philippe Waroquiers" <philippe.waroquiers@skynet.be>]

[-- 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
      {