This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Thiago Jung Bauermann <bauerman at br dot ibm dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Mon, 15 Nov 2010 14:23:10 -0800
- Subject: Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops
- References: <1282074071.2606.702.camel@hactar> <201010161843.43062.pedro@codesourcery.com> <1287534691.2686.17.camel@hactar>
> 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