From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 909 invoked by alias); 16 Nov 2010 19:03:57 -0000 Received: (qmail 898 invoked by uid 22791); 16 Nov 2010 19:03:56 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from e24smtp02.br.ibm.com (HELO e24smtp02.br.ibm.com) (32.104.18.86) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 16 Nov 2010 19:03:45 +0000 Received: from mailhub1.br.ibm.com (mailhub1.br.ibm.com [9.18.232.109]) by e24smtp02.br.ibm.com (8.14.4/8.13.1) with ESMTP id oAGJGRaU025875 for ; Tue, 16 Nov 2010 17:16:27 -0200 Received: from d24av04.br.ibm.com (d24av04.br.ibm.com [9.8.31.97]) by mailhub1.br.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id oAGJAS7s774502 for ; Tue, 16 Nov 2010 17:10:28 -0200 Received: from d24av04.br.ibm.com (loopback [127.0.0.1]) by d24av04.br.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id oAGJ3eGI008092 for ; Tue, 16 Nov 2010 17:03:41 -0200 Received: from [9.8.0.71] ([9.8.0.71]) by d24av04.br.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with ESMTP id oAGJ3eiX007055; Tue, 16 Nov 2010 17:03:40 -0200 Subject: Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops From: Thiago Jung Bauermann To: Joel Brobecker Cc: gdb-patches@sourceware.org In-Reply-To: <20101115222310.GB4434@adacore.com> References: <1282074071.2606.702.camel@hactar> <201010161843.43062.pedro@codesourcery.com> <1287534691.2686.17.camel@hactar> <20101115222310.GB4434@adacore.com> Content-Type: text/plain; charset="UTF-8" Date: Tue, 16 Nov 2010 19:03:00 -0000 Message-ID: <1289933508.3202.13.camel@hactar> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-11/txt/msg00209.txt.bz2 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 ": " 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