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

[Pedro Alves <pedro at codesourcery dot com>]

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