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

[Thiago Jung Bauermann <bauerman@br.ibm.com>]

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