From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3021 invoked by alias); 8 Feb 2012 19:33:34 -0000 Received: (qmail 3008 invoked by uid 22791); 8 Feb 2012 19:33:32 -0000 X-SWARE-Spam-Status: No, hits=-6.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 08 Feb 2012 19:33:11 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q18JX8WK018533 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 8 Feb 2012 14:33:08 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q18JX69T010500; Wed, 8 Feb 2012 14:33:07 -0500 Message-ID: <4F32CDF2.8090906@redhat.com> Date: Wed, 08 Feb 2012 19:33:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20120131 Thunderbird/10.0 MIME-Version: 1.0 To: "Gustavo, Luis" CC: Tom Tromey , gdb-patches@sourceware.org Subject: Re: [rfc target-side break conditions 3/5 v2] GDB-side changes References: <4F230A13.9060400@mentor.com> <4F3107C2.2060400@mentor.com> In-Reply-To: <4F3107C2.2060400@mentor.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: 2012-02/txt/msg00109.txt.bz2 On 02/07/2012 11:15 AM, Luis Gustavo wrote: > + /* Does this target support evaluation breakpoint conditions on its end? */ > + int (*to_supports_breakpoint_conditions) (void); s/evaluation/evaluating/ or s/evaluation/evaluation of/ ? On 02/07/2012 11:15 AM, Luis Gustavo wrote: > +#define ALL_BP_LOCATIONS_AT_ADDR(BP_LOCP_TMP, BP_LOCP_START, ADDRESS) \ > + if (!BP_LOCP_START) \ > + BP_LOCP_START = get_first_locp_gte_addr (ADDRESS); \ > + for (BP_LOCP_TMP = BP_LOCP_START; \ > + (BP_LOCP_TMP < bp_location + bp_location_count \ > + && (*BP_LOCP_TMP)->address == ADDRESS); \ > + BP_LOCP_TMP++) > + This will break if ever someone does if (foo) ALL_BP_LOCATIONS_AT_ADDR(...); You can move the initialization of BP_LOCP_START to the for initializer instead, like so: #define ALL_BP_LOCATIONS_AT_ADDR(BP_LOCP_TMP, BP_LOCP_START, ADDRESS) \ for (BP_LOCP_START = BP_LOCP_START == NULL ? get_first_locp_gte_addr (ADDRESS) : NULL, \ BP_LOCP_TMP = BP_LOCP_START; \ (BP_LOCP_TMP < bp_location + bp_location_count \ && (*BP_LOCP_TMP)->address == ADDRESS); \ BP_LOCP_TMP++) On 02/07/2012 11:15 AM, Luis Gustavo wrote: > + when we go through update_global_location_list (...). */ Please drop the "()" whenever you refer to a function by name. */ > /* Delete a breakpoint and clean up all traces of it in the data > - structures. */ > + structures. If UPDATE is true, proceed to update the list of > + locations, otherwise don't update it. */ > > -void > -delete_breakpoint (struct breakpoint *bpt) > +static void > +delete_breakpoint_1 (struct breakpoint *bpt, int update) > { > struct breakpoint *b; > > @@ -11955,8 +12451,12 @@ delete_breakpoint (struct breakpoint *bp > belong to this breakpoint. Do this before freeing the breakpoint > itself, since remove_breakpoint looks at location's owner. It > might be better design to have location completely > - self-contained, but it's not the case now. */ > - update_global_location_list (0); > + self-contained, but it's not the case now. > + > + If we have conditions being evaluated on the target, we will also > + synchronize the list of conditions due to a deleted duplicate > + breakpoint. */ > + update_global_location_list (update); > > bpt->ops->dtor (bpt); > /* On the chance that someone will soon try again to delete this > @@ -11965,6 +12465,24 @@ delete_breakpoint (struct breakpoint *bp > xfree (bpt); > } > > +/* Delete a breakpoint but don't update the list of locations. */ > + > +void > +delete_breakpoint (struct breakpoint *bpt) > +{ > + delete_breakpoint_1 (bpt, 0); > +} > + > +/* Delete a breakpoint and update the list of locations. We use > + this one to synchronize the list of conditions with the target > + in case the target is responsible for evaluating them. */ > + > +void > +delete_breakpoint_with_update (struct breakpoint *bpt) > +{ > + delete_breakpoint_1 (bpt, 1); > +} > + > static void > do_delete_breakpoint_cleanup (void *b) > { > @@ -12017,7 +12535,10 @@ iterate_over_related_breakpoints (struct > static void > do_delete_breakpoint (struct breakpoint *b, void *ignore) > { > - delete_breakpoint (b); > + if (is_breakpoint (b)) > + delete_breakpoint_with_update (b); > + else > + delete_breakpoint (b); > } > > /* A callback for map_breakpoint_numbers that calls > @@ -12056,7 +12577,7 @@ delete_command (char *arg, int from_tty) > { > ALL_BREAKPOINTS_SAFE (b, b_tmp) > if (user_breakpoint_p (b)) > - delete_breakpoint (b); > + do_delete_breakpoint (b, NULL); > } > } > else > @@ -12840,6 +13361,9 @@ disable_breakpoint (struct breakpoint *b > > bpt->enable_state = bp_disabled; > > + /* Mark breakpoint locations modified. */ > + mark_breakpoint_modified (bpt); > + > if (target_supports_enable_disable_tracepoint () > && current_trace_status ()->running && is_tracepoint (bpt)) > { > @@ -12849,7 +13373,10 @@ disable_breakpoint (struct breakpoint *b > target_disable_tracepoint (location); > } > > - update_global_location_list (0); > + if (is_breakpoint (bpt)) > + update_global_location_list (1); > + else > + update_global_location_list (0); I'm confused. How does this (and all similar places) address the issued I pointed out before? If you're passing one to update_global_location_list when deleting a breakpoint, you're pretty much defeating the whole purpose of the update_global_location_list's argument in the first place. -- Pedro Alves