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

Thiago Jung Bauermann
2010-11-16T19:03:57Z

Hi Joel,

Thanks for the review!

On Mon, 2010-11-15 at 14:23 -0800, Joel Brobecker wrote:
> 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.

Ok. This patch doesn't do much on its own, so I'll commit it just when
the masked and ranged watchpoints patch is approved.

> > + 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)?

I kept the wording as similar as possible to the original. Your
suggestion is then to have the code below instead?

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

I think it's good.

> > - 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.

What about these additional comments (I'll send the updated patch after
understanding your suggestion above)?

diff --git a/gdb/target.h b/gdb/target.h
index 54a6747..f4395c0 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -476,6 +476,11 @@ struct target_ops
char *, char *, char **, int);
void (*to_post_startup_inferior) (ptid_t);
void (*to_acknowledge_created_inferior) (int);
+
+
+ /* The insert and remove catchpoint functions return 0 for success,
+ 1 if the watchpoint type is not supported and -1 for failure. */
+
int (*to_insert_fork_catchpoint) (int);
int (*to_remove_fork_catchpoint) (int);
int (*to_insert_vfork_catchpoint) (int);
@@ -484,6 +489,7 @@ struct target_ops
int (*to_insert_exec_catchpoint) (int);
int (*to_remove_exec_catchpoint) (int);
int (*to_set_syscall_catchpoint) (int, int, int, int, int *);
+
int (*to_has_exited) (int, int, int *);
void (*to_mourn_inferior) (struct target_ops *);
int (*to_can_run) (void);
@@ -1042,7 +1048,8 @@ void target_create_inferior (char *exec_file, char *args,

/* On some targets, we can catch an inferior fork or vfork event when
it occurs. These functions insert/remove an already-created
- catchpoint for such events. */
+ catchpoint for such events. They return 0 for success, 1 if the
+ catchpoint type is not supported and -1 for failure. */

#define target_insert_fork_catchpoint(pid) \
(*current_target.to_insert_fork_catchpoint) (pid)
@@ -1068,7 +1075,8 @@ int target_follow_fork (int follow_child);

/* On some targets, we can catch an inferior exec event when it
occurs. These functions insert/remove an already-created
- catchpoint for such events. */
+ catchpoint for such events. They return 0 for success, 1 if the
+ catchpoint type is not supported and -1 for failure. */

#define target_insert_exec_catchpoint(pid) \
(*current_target.to_insert_exec_catchpoint) (pid)

--
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center