Re: [PATCH 4/6] Notify breakpoint-modified when uploaded tracepoints are merged.

[Pedro Alves <palves@redhat.com>]

On 12/04/2012 04:44 AM, Yao Qi wrote:
> When any location of a tracepoint is marked as installed/inserted,
> notify 'breakpoint-modified' observer.
> 
> gdb:
> 
> 2012-12-03  Yao Qi  <yao@codesourcery.com>
> 
> 	* tracepoint.c (merge_uploaded_tracepoints): Record all modified
> 	tracepoints and notify 'breakpoint-modified' observer for them.

This looks good to me.  Some comment suggestions below.

I ran out of time for now, so I'll review the rest of the series later or tomorrow.

> ---
>  gdb/tracepoint.c |   28 ++++++++++++++++++++++++++++
>  1 files changed, 28 insertions(+), 0 deletions(-)
> 
> diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
> index c467b9c..0c023bd 100644
> --- a/gdb/tracepoint.c
> +++ b/gdb/tracepoint.c
> @@ -3475,6 +3475,10 @@ void
>  merge_uploaded_tracepoints (struct uploaded_tp **uploaded_tps)
>  {
>    struct uploaded_tp *utp;
> +  /* A set of tracepoints which are modified.  */
> +  VEC(breakpoint_p) *modified_tp = NULL;
> +  int ix;
> +  struct breakpoint *b;
>  
>    /* Look for GDB tracepoints that match up with our uploaded versions.  */
>    for (utp = *uploaded_tps; utp; utp = utp->next)
> @@ -3485,6 +3489,8 @@ merge_uploaded_tracepoints (struct uploaded_tp **uploaded_tps)
>        loc = find_matching_tracepoint_location (utp);
>        if (loc)
>  	{
> +	  int found = 0;
> +
>  	  /* Mark this location as already inserted.  */
>  	  loc->inserted = 1;
>  	  t = (struct tracepoint *) loc->owner;
> @@ -3492,6 +3498,22 @@ merge_uploaded_tracepoints (struct uploaded_tp **uploaded_tps)
>  			     "as target's tracepoint %d at %s.\n"),
>  			   loc->owner->number, utp->number,
>  			   paddress (loc->gdbarch, utp->addr));
> +
> +	  /* One location LOC of tracepoint is modified ('inserted' is
> +	     set to 1), save LOC->owner in MODIFIED_TP if LOC->owner
> +	     is not added to MODIFIED_TP before.  The observer
> +	     'breakpoint-modified' will be notified later with every
> +	     tracepoint saved in MODIFIED_TP.  */

I suggest:

	  /* The tracepoint was modified (a location was marked as inserted
	     in the target).  Save it in MODIFIED_TP if not there yet.  The
	     'breakpoint-modified' observers will be notified later once for each
	     tracepoint saved in MODIFIED_TP.  */


> +	  for (ix = 0;
> +	       VEC_iterate (breakpoint_p, modified_tp, ix, b);
> +	       ix++)
> +	    if (b == loc->owner)
> +	      {
> +		found = 1;
> +		break;
> +	      }
> +	  if (!found)
> +	    VEC_safe_push (breakpoint_p, modified_tp, loc->owner);
>  	}
>        else
>  	{
> @@ -3514,6 +3536,12 @@ merge_uploaded_tracepoints (struct uploaded_tp **uploaded_tps)
>  	t->number_on_target = utp->number;
>      }
>  
> +  /* Notify 'breakpoint-modified' observer that at least one of B's
> +     locations is changed.  */

  /* Notify 'breakpoint-modified' observers that at least one of B's
     locations was changed.  */

> +  for (ix = 0; VEC_iterate (breakpoint_p, modified_tp, ix, b); ix++)
> +    observer_notify_breakpoint_modified (b);
> +
> +  VEC_free (breakpoint_p, modified_tp);
>    free_uploaded_tps (uploaded_tps);
>  }


-- 
Pedro Alves