This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [rfc target-side break conditions 3/5 v2] GDB-side changes
- From: Pedro Alves <palves at redhat dot com>
- To: "Gustavo, Luis" <luis_gustavo at mentor dot com>
- Cc: Tom Tromey <tromey at redhat dot com>, gdb-patches at sourceware dot org
- Date: Wed, 08 Feb 2012 19:33:06 +0000
- Subject: Re: [rfc target-side break conditions 3/5 v2] GDB-side changes
- References: <4F230A13.9060400@mentor.com> <m3vcnjn1xa.fsf@fleche.redhat.com> <4F3107C2.2060400@mentor.com>
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