Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops

Joel Brobecker
2010-11-15T22:23:23Z

> 2010-10-19 Thiago Jung Bauermann <bauerman@br.ibm.com>
>
> Convert hardware watchpoints to use breakpoint_ops.
>
> gdb/
> * breakpoint.h (breakpoint_ops) <insert>: Return int instead of void.
> Accept pointer to struct bp_location instead of pointer to
> struct breakpoint. Adapt all implementations.
> (breakpoint_ops) <remove>: Accept pointer to struct bp_location instead
> of pointer to struct breakpoint. Adapt all implementations.
> * breakpoint.c (insert_catchpoint): Delete function.
> (insert_bp_location): Call the watchpoint or catchpoint's
> breakpoint_ops.insert method.
> (remove_breakpoint_1): Call the watchpoint or catchpoint's
> breakpoint_ops.remove method.
> (insert_watchpoint, remove_watchpoint): New functions.
> (watchpoint_breakpoint_ops): New structure.
> (watch_command_1): Initialize the OPS field.
> * inf-child.c (inf_child_insert_fork_catchpoint)
> (inf_child_remove_fork_catchpoint, inf_child_insert_vfork_catchpoint)
> (inf_child_remove_vfork_catchpoint, inf_child_insert_exec_catchpoint)
> (inf_child_remove_exec_catchpoint, inf_child_set_syscall_catchpoint):
> Delete functions.
> (inf_child_target): Remove initialization of to_insert_fork_catchpoint,
> to_remove_fork_catchpoint, to_insert_vfork_catchpoint,
> to_remove_vfork_catchpoint, to_insert_exec_catchpoint,
> to_remove_exec_catchpoint and to_set_syscall_catchpoint.
> * target.c (update_current_target): Change default implementation of
> to_insert_fork_catchpoint, to_remove_fork_catchpoint,
> to_insert_vfork_catchpoint, to_remove_vfork_catchpoint,
> to_insert_exec_catchpoint, to_remove_exec_catchpoint and
> to_set_syscall_catchpoint to return_one.
> (debug_to_insert_fork_catchpoint, debug_to_insert_vfork_catchpoint)
> (debug_to_insert_exec_catchpoint): Report return value.
> * target.h (to_insert_fork_catchpoint, to_insert_vfork_catchpoint)
> (to_insert_fork_catchpoint): Change declaration to return int instead
> of void.
>
> gdb/testsuite/
> * gdb.base/foll-exec.exp: Adapt to new error string when the catchpoint
> type is not supported.
> * gdb.base/foll-fork.exp: Likewise.
> * gdb.base/foll-vfork.exp: Likewise.

I'm OK with this patch. Just a possible suggestion below...
Can you wait a couple more days to give anyone one last chance
at making comments on this patch? After that, please go ahead
and commit.

> + if (val == 1)
> + warning (_("\
> +Inserting catchpoint %d: Your system does not support this type of catchpoint."),
> + bpt->owner->number);
> + else
> + warning (_("Error inserting catchpoint %d."), bpt->owner->number);

Just curious: Why not say "Error inserting catchpoint %d" in both cases
(we would still keep the ": <not supported>" part in the first case)?

> - void (*to_insert_fork_catchpoint) (int);
> + int (*to_insert_fork_catchpoint) (int);
> int (*to_remove_fork_catchpoint) (int);
> - void (*to_insert_vfork_catchpoint) (int);
> + int (*to_insert_vfork_catchpoint) (int);
> int (*to_remove_vfork_catchpoint) (int);
> int (*to_follow_fork) (struct target_ops *, int);
> - void (*to_insert_exec_catchpoint) (int);
> + int (*to_insert_exec_catchpoint) (int);
> int (*to_remove_exec_catchpoint) (int);
> int (*to_set_syscall_catchpoint) (int, int, int, int, int *);

I think we really should be documenting at least the return value.
Apparently, at least some of these "method" are documented though
their associated "target_<...>" macro/function. I'd rather see
that documentation next to the method rather than the macro, but
that's for another discussion.

--
Joel