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

Pedro Alves
2011-07-26T19:46:39Z

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