* ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)
@ 2011-05-21 22:20 Philippe Waroquiers
2011-05-26 19:02 ` Tom Tromey
2011-05-27 3:25 ` ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver) Yao Qi
0 siblings, 2 replies; 32+ messages in thread
From: Philippe Waroquiers @ 2011-05-21 22:20 UTC (permalink / raw)
To: gdb-patches; +Cc: 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}
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver) 2011-05-21 22:20 ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver) Philippe Waroquiers @ 2011-05-26 19:02 ` Tom Tromey 2011-05-29 13:01 ` Philippe Waroquiers 2011-05-27 3:25 ` ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver) Yao Qi 1 sibling, 1 reply; 32+ messages in thread From: Tom Tromey @ 2011-05-26 19:02 UTC (permalink / raw) To: Philippe Waroquiers; +Cc: gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver) 2011-05-26 19:02 ` Tom Tromey @ 2011-05-29 13:01 ` Philippe Waroquiers 2011-05-30 15:26 ` Joel Brobecker 2011-05-31 19:07 ` x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) Pedro Alves 0 siblings, 2 replies; 32+ messages in thread From: Philippe Waroquiers @ 2011-05-29 13:01 UTC (permalink / raw) To: gdb-patches; +Cc: yao [-- 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 { ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver) 2011-05-29 13:01 ` Philippe Waroquiers @ 2011-05-30 15:26 ` Joel Brobecker 2011-05-31 19:07 ` x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) Pedro Alves 1 sibling, 0 replies; 32+ messages in thread From: Joel Brobecker @ 2011-05-30 15:26 UTC (permalink / raw) To: Philippe Waroquiers; +Cc: gdb-patches, yao 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) 2011-05-29 13:01 ` Philippe Waroquiers 2011-05-30 15:26 ` Joel Brobecker @ 2011-05-31 19:07 ` Pedro Alves 2011-05-31 20:25 ` Philippe Waroquiers 2011-05-31 21:29 ` Pedro Alves 1 sibling, 2 replies; 32+ messages in thread From: Pedro Alves @ 2011-05-31 19:07 UTC (permalink / raw) To: gdb-patches; +Cc: Philippe Waroquiers, yao 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]; } ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) 2011-05-31 19:07 ` x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) Pedro Alves @ 2011-05-31 20:25 ` Philippe Waroquiers 2011-05-31 20:53 ` Pedro Alves 2011-05-31 21:29 ` Pedro Alves 1 sibling, 1 reply; 32+ messages in thread From: Philippe Waroquiers @ 2011-05-31 20:25 UTC (permalink / raw) To: Pedro Alves, gdb-patches; +Cc: yao > 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) 2011-05-31 20:25 ` Philippe Waroquiers @ 2011-05-31 20:53 ` Pedro Alves 0 siblings, 0 replies; 32+ messages in thread From: Pedro Alves @ 2011-05-31 20:53 UTC (permalink / raw) To: Philippe Waroquiers; +Cc: gdb-patches, yao 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) 2011-05-31 19:07 ` x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) Pedro Alves 2011-05-31 20:25 ` Philippe Waroquiers @ 2011-05-31 21:29 ` Pedro Alves 2011-05-31 22:15 ` Philippe Waroquiers 1 sibling, 1 reply; 32+ messages in thread From: Pedro Alves @ 2011-05-31 21:29 UTC (permalink / raw) To: gdb-patches; +Cc: Philippe Waroquiers, yao 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" ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) 2011-05-31 21:29 ` Pedro Alves @ 2011-05-31 22:15 ` Philippe Waroquiers 2011-05-31 23:04 ` Pedro Alves 0 siblings, 1 reply; 32+ messages in thread From: Philippe Waroquiers @ 2011-05-31 22:15 UTC (permalink / raw) To: Pedro Alves, gdb-patches; +Cc: yao > +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) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) 2011-05-31 22:15 ` Philippe Waroquiers @ 2011-05-31 23:04 ` Pedro Alves 2011-06-01 14:35 ` Pedro Alves 0 siblings, 1 reply; 32+ messages in thread From: Pedro Alves @ 2011-05-31 23:04 UTC (permalink / raw) To: Philippe Waroquiers; +Cc: gdb-patches, yao 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) 2011-05-31 23:04 ` Pedro Alves @ 2011-06-01 14:35 ` Pedro Alves 2011-06-08 22:55 ` Philippe Waroquiers 0 siblings, 1 reply; 32+ messages in thread From: Pedro Alves @ 2011-06-01 14:35 UTC (permalink / raw) To: gdb-patches; +Cc: Philippe Waroquiers, yao 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; } ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) 2011-06-01 14:35 ` Pedro Alves @ 2011-06-08 22:55 ` Philippe Waroquiers 2011-06-09 0:00 ` Pedro Alves 0 siblings, 1 reply; 32+ messages in thread From: Philippe Waroquiers @ 2011-06-08 22:55 UTC (permalink / raw) To: Pedro Alves, gdb-patches; +Cc: yao > 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) 2011-06-08 22:55 ` Philippe Waroquiers @ 2011-06-09 0:00 ` Pedro Alves 2011-06-09 22:16 ` Philippe Waroquiers 0 siblings, 1 reply; 32+ messages in thread From: Pedro Alves @ 2011-06-09 0:00 UTC (permalink / raw) To: Philippe Waroquiers; +Cc: gdb-patches, yao 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) 2011-06-09 0:00 ` Pedro Alves @ 2011-06-09 22:16 ` Philippe Waroquiers 2011-07-21 17:20 ` Pedro Alves 0 siblings, 1 reply; 32+ messages in thread From: Philippe Waroquiers @ 2011-06-09 22:16 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, yao 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) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) 2011-06-09 22:16 ` Philippe Waroquiers @ 2011-07-21 17:20 ` Pedro Alves 2011-07-22 16:40 ` Philippe Waroquiers 0 siblings, 1 reply; 32+ messages in thread From: Pedro Alves @ 2011-07-21 17:20 UTC (permalink / raw) To: gdb-patches; +Cc: Philippe Waroquiers, yao 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; } ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) 2011-07-21 17:20 ` Pedro Alves @ 2011-07-22 16:40 ` Philippe Waroquiers 2011-07-22 16:43 ` Pedro Alves 2011-07-22 17:19 ` x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) Pedro Alves 0 siblings, 2 replies; 32+ messages in thread From: Philippe Waroquiers @ 2011-07-22 16:40 UTC (permalink / raw) To: Pedro Alves, gdb-patches; +Cc: yao > 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) 2011-07-22 16:40 ` Philippe Waroquiers @ 2011-07-22 16:43 ` Pedro Alves 2011-07-23 16:28 ` Thiago Jung Bauermann 2011-07-22 17:19 ` x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) Pedro Alves 1 sibling, 1 reply; 32+ messages in thread From: Pedro Alves @ 2011-07-22 16:43 UTC (permalink / raw) To: gdb-patches, Thiago Jung Bauermann; +Cc: Philippe Waroquiers, yao 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) 2011-07-22 16:43 ` Pedro Alves @ 2011-07-23 16:28 ` Thiago Jung Bauermann 2011-07-26 20:02 ` software watchpoints bug (was: Re: x86 watchpoints bug) Pedro Alves 0 siblings, 1 reply; 32+ messages in thread From: Thiago Jung Bauermann @ 2011-07-23 16:28 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches ml, Philippe Waroquiers, yao 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* software watchpoints bug (was: Re: x86 watchpoints bug) 2011-07-23 16:28 ` Thiago Jung Bauermann @ 2011-07-26 20:02 ` Pedro Alves 2011-07-27 3:45 ` Thiago Jung Bauermann 0 siblings, 1 reply; 32+ messages in thread From: Pedro Alves @ 2011-07-26 20:02 UTC (permalink / raw) To: gdb-patches; +Cc: Thiago Jung Bauermann, Philippe Waroquiers, yao 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: software watchpoints bug (was: Re: x86 watchpoints bug) 2011-07-26 20:02 ` software watchpoints bug (was: Re: x86 watchpoints bug) Pedro Alves @ 2011-07-27 3:45 ` Thiago Jung Bauermann 0 siblings, 0 replies; 32+ messages in thread From: Thiago Jung Bauermann @ 2011-07-27 3:45 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Philippe Waroquiers, yao 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) 2011-07-22 16:40 ` Philippe Waroquiers 2011-07-22 16:43 ` Pedro Alves @ 2011-07-22 17:19 ` Pedro Alves 1 sibling, 0 replies; 32+ messages in thread From: Pedro Alves @ 2011-07-22 17:19 UTC (permalink / raw) To: gdb-patches; +Cc: Philippe Waroquiers, yao 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver) 2011-05-21 22:20 ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver) Philippe Waroquiers 2011-05-26 19:02 ` Tom Tromey @ 2011-05-27 3:25 ` Yao Qi 2011-05-27 17:53 ` Tom Tromey 1 sibling, 1 reply; 32+ messages in thread From: Yao Qi @ 2011-05-27 3:25 UTC (permalink / raw) To: gdb-patches 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 (é½å°§) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver) 2011-05-27 3:25 ` ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver) Yao Qi @ 2011-05-27 17:53 ` Tom Tromey 2011-05-27 17:59 ` Pedro Alves 0 siblings, 1 reply; 32+ messages in thread From: Tom Tromey @ 2011-05-27 17:53 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver) 2011-05-27 17:53 ` Tom Tromey @ 2011-05-27 17:59 ` Pedro Alves 2011-05-30 4:06 ` Yao Qi 0 siblings, 1 reply; 32+ messages in thread From: Pedro Alves @ 2011-05-27 17:59 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey, Yao Qi 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver) 2011-05-27 17:59 ` Pedro Alves @ 2011-05-30 4:06 ` Yao Qi 2011-05-30 5:34 ` Philippe Waroquiers 2011-05-31 17:31 ` Pedro Alves 0 siblings, 2 replies; 32+ messages in thread From: Yao Qi @ 2011-05-30 4:06 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Tom Tromey 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 (é½å°§) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver) 2011-05-30 4:06 ` Yao Qi @ 2011-05-30 5:34 ` Philippe Waroquiers 2011-05-30 5:48 ` Yao Qi 2011-05-31 17:31 ` Pedro Alves 1 sibling, 1 reply; 32+ messages in thread From: Philippe Waroquiers @ 2011-05-30 5:34 UTC (permalink / raw) To: Yao Qi, Pedro Alves; +Cc: gdb-patches, Tom Tromey > 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver) 2011-05-30 5:34 ` Philippe Waroquiers @ 2011-05-30 5:48 ` Yao Qi 2011-05-30 6:31 ` Philippe Waroquiers 0 siblings, 1 reply; 32+ messages in thread From: Yao Qi @ 2011-05-30 5:48 UTC (permalink / raw) To: gdb-patches 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 (é½å°§) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver) 2011-05-30 5:48 ` Yao Qi @ 2011-05-30 6:31 ` Philippe Waroquiers 0 siblings, 0 replies; 32+ messages in thread From: Philippe Waroquiers @ 2011-05-30 6:31 UTC (permalink / raw) To: Yao Qi, gdb-patches > 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver) 2011-05-30 4:06 ` Yao Qi 2011-05-30 5:34 ` Philippe Waroquiers @ 2011-05-31 17:31 ` Pedro Alves 2011-05-31 18:06 ` Philippe Waroquiers 1 sibling, 1 reply; 32+ messages in thread From: Pedro Alves @ 2011-05-31 17:31 UTC (permalink / raw) To: gdb-patches; +Cc: Yao Qi, Tom Tromey, Philippe Waroquiers 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver) 2011-05-31 17:31 ` Pedro Alves @ 2011-05-31 18:06 ` Philippe Waroquiers 2011-06-01 15:15 ` Pedro Alves 0 siblings, 1 reply; 32+ messages in thread From: Philippe Waroquiers @ 2011-05-31 18:06 UTC (permalink / raw) To: Pedro Alves, gdb-patches; +Cc: Yao Qi, Tom Tromey > 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver) 2011-05-31 18:06 ` Philippe Waroquiers @ 2011-06-01 15:15 ` Pedro Alves 2011-06-05 20:55 ` Philippe Waroquiers 0 siblings, 1 reply; 32+ messages in thread From: Pedro Alves @ 2011-06-01 15:15 UTC (permalink / raw) To: gdb-patches; +Cc: Philippe Waroquiers, Yao Qi, Tom Tromey 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver) 2011-06-01 15:15 ` Pedro Alves @ 2011-06-05 20:55 ` Philippe Waroquiers 0 siblings, 0 replies; 32+ messages in thread From: Philippe Waroquiers @ 2011-06-05 20:55 UTC (permalink / raw) To: Pedro Alves, gdb-patches; +Cc: Yao Qi, Tom Tromey >> 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2011-07-27 1:29 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-05-21 22:20 ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver) Philippe Waroquiers 2011-05-26 19:02 ` Tom Tromey 2011-05-29 13:01 ` Philippe Waroquiers 2011-05-30 15:26 ` Joel Brobecker 2011-05-31 19:07 ` x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) Pedro Alves 2011-05-31 20:25 ` Philippe Waroquiers 2011-05-31 20:53 ` Pedro Alves 2011-05-31 21:29 ` Pedro Alves 2011-05-31 22:15 ` Philippe Waroquiers 2011-05-31 23:04 ` Pedro Alves 2011-06-01 14:35 ` Pedro Alves 2011-06-08 22:55 ` Philippe Waroquiers 2011-06-09 0:00 ` Pedro Alves 2011-06-09 22:16 ` Philippe Waroquiers 2011-07-21 17:20 ` Pedro Alves 2011-07-22 16:40 ` Philippe Waroquiers 2011-07-22 16:43 ` Pedro Alves 2011-07-23 16:28 ` Thiago Jung Bauermann 2011-07-26 20:02 ` software watchpoints bug (was: Re: x86 watchpoints bug) Pedro Alves 2011-07-27 3:45 ` Thiago Jung Bauermann 2011-07-22 17:19 ` x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver)) Pedro Alves 2011-05-27 3:25 ` ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver) Yao Qi 2011-05-27 17:53 ` Tom Tromey 2011-05-27 17:59 ` Pedro Alves 2011-05-30 4:06 ` Yao Qi 2011-05-30 5:34 ` Philippe Waroquiers 2011-05-30 5:48 ` Yao Qi 2011-05-30 6:31 ` Philippe Waroquiers 2011-05-31 17:31 ` Pedro Alves 2011-05-31 18:06 ` Philippe Waroquiers 2011-06-01 15:15 ` Pedro Alves 2011-06-05 20:55 ` Philippe Waroquiers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox