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

[Pedro Alves <pedro@codesourcery.com>]

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