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

[Thiago Jung Bauermann <bauerman at br dot ibm dot com>]

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