Re: [rfc target-side break conditions 3/5 v2] GDB-side changes

[Pedro Alves <palves@redhat.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