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

Thiago Jung Bauermann
2011-07-23T02:40:04Z

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