From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18938 invoked by alias); 15 Nov 2010 22:23:23 -0000 Received: (qmail 18924 invoked by uid 22791); 15 Nov 2010 22:23:21 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 15 Nov 2010 22:23:16 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id BEC212BAC26; Mon, 15 Nov 2010 17:23:14 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id iRFMKyKVfj41; Mon, 15 Nov 2010 17:23:14 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 7FD5C2BAC15; Mon, 15 Nov 2010 17:23:14 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id CC1E7145C0D; Mon, 15 Nov 2010 14:23:10 -0800 (PST) Date: Mon, 15 Nov 2010 22:23:00 -0000 From: Joel Brobecker To: Thiago Jung Bauermann Cc: gdb-patches@sourceware.org Subject: Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops Message-ID: <20101115222310.GB4434@adacore.com> References: <1282074071.2606.702.camel@hactar> <201010161843.43062.pedro@codesourcery.com> <1287534691.2686.17.camel@hactar> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1287534691.2686.17.camel@hactar> User-Agent: Mutt/1.5.20 (2009-06-14) 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/msg00180.txt.bz2 > 2010-10-19 Thiago Jung Bauermann > > Convert hardware watchpoints to use breakpoint_ops. > > gdb/ > * breakpoint.h (breakpoint_ops) : Return int instead of void. > Accept pointer to struct bp_location instead of pointer to > struct breakpoint. Adapt all implementations. > (breakpoint_ops) : 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 ": " 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