Re: [PATCH v2 2/4] tracepoint multithread and multiprocess support (gdbserver)

[Pedro Alves <palves at redhat dot com>]

On 12/19/2013 03:29 AM, Hui Zhu wrote:
> This version doesn't have big change, just update follow GDB trunk
> and update Changelog.
> 
> This patch is for the gdbserver.
> It will send ";MultiProcessTracepoint+" back to GDB if this gdbserver
> support tracepoint.
> 
> When cmd_qtdp got a "QTDP" packets that have P@var{thread-id}.
> 1.  Get ptid from thread-id.
> 2.  If this ptid's pid is not same with current process, send exx packets
>      back to GDB.

Hmm.  Did you sync with Luis?  ISTR he had some multi-process
tracing patches too.  IIRC, he just made it so that tracepoints
apply to the current process, and then GDB made sure to set the
general thread/process (Hg) to the current process before setting
each tracepoint.  So this error seems to go in that direction,
and the main desire here is to make the tracepoints thread-specific,
not really process-specific.  Right?

> 3.  If this ptid's lwp and tip is 0, set it to minus_one_ptid because
>      this ptid just has process info, gdbserver has done with process check.
> 3.  Save this ptid to tracepoint.
> 
> Before GDB trigger a tracepoint, it will check if its ptid is same with
> minus_one_ptid or tinfo->entry.id.  If not, doesn't trigger it.
> 
> Please help me review it.
> 
> Thanks,
> Hui
> 
> 2013-12-19  Hui Zhu  <teawater@gmail.com>
> 
> 	* server.c (handle_query): Send ";MultiProcessTracepoint+".
> 	* tracepoint.c (tracepoint): Add ptid.

 	* tracepoint.c (tracepoint) <ptid>: New field.


> 	(add_tracepoint): Initialize ptid.

 	(add_tracepoint): Initialize ptid field.

> 	(cmd_qtdp): Handle 'P'.
> 	(tracepoint_was_hit): Add check if tpoint->ptid is same with
> 	minus_one_ptid or tinfo->entry.id.

 	(tracepoint_was_hit): Only collect if tpoint->ptid is
	minus_one_ptid or tinfo->entry.id.

> 
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -1804,6 +1804,7 @@ handle_query (char *own_buf, int packet_
>   	  strcat (own_buf, ";EnableDisableTracepoints+");
>   	  strcat (own_buf, ";QTBuffer:size+");
>   	  strcat (own_buf, ";tracenz+");
> +	  strcat (own_buf, ";MultiProcessTracepoint+");
>   	}
>   
>         /* Support target-side breakpoint conditions and commands.  */
> --- a/gdb/gdbserver/tracepoint.c
> +++ b/gdb/gdbserver/tracepoint.c
> @@ -766,6 +766,8 @@ struct tracepoint
>   
>     CORE_ADDR compiled_cond;
>   
> +  ptid_t ptid;

This needs a comment.

> +
>     /* Link to the next tracepoint in the list.  */
>     struct tracepoint *next;
>   
> @@ -1822,6 +1824,7 @@ add_tracepoint (int num, CORE_ADDR addr)
>     tpoint->source_strings = NULL;
>     tpoint->compiled_cond = 0;
>     tpoint->handle = NULL;
> +  tpoint->ptid = minus_one_ptid;
>     tpoint->next = NULL;
>   
>     /* Find a place to insert this tracepoint into list in order to keep
> @@ -2551,6 +2554,29 @@ cmd_qtdp (char *own_buf)
>   	      tpoint->cond = gdb_parse_agent_expr (&actparm);
>   	      packet = actparm;
>   	    }
> +	  else if (*packet == 'P')
> +	    {
> +	      ++packet;
> +	      tpoint->ptid = read_ptid (packet, &packet);
> +
> +	      /* Check if this tracepoint is for current process.  */
> + 	      if (ptid_get_pid (current_ptid)
> +		    != ptid_get_pid (tpoint->ptid))
> +	        {
> +	          trace_debug ("\
> +Tracepoint error: tracepoint %d is not for current process", (int) num);

The number alone is not sufficient to identify the tracepoint.


> +		  write_enn (own_buf);
> +		  return;
> +		}
> +	      if (ptid_get_lwp (tpoint->ptid) == 0
> +		  && ptid_get_tid (tpoint->ptid) == 0)
> +	        {
> +		  /* This tracepoint is OK for all the thread of current
> +		     process, set its ptid to minus_one_ptid to make its
> +		     ptid is not checked before trigger this tracepoint.  */
> +		  tpoint->ptid = minus_one_ptid;

I don't think this is right.  GDBserver might be debugging multiple
processes, with only one of the processes tracing.  And ...

> +		}
> +	    }
>   	  else if (*packet == '-')
>   	    break;
>   	  else if (*packet == '\0')
> @@ -4562,10 +4588,14 @@ tracepoint_was_hit (struct thread_info *
>   		       target_pid_to_str (tinfo->entry.id),
>   		       tpoint->number, paddress (tpoint->address));
>   
> -	  /* Test the condition if present, and collect if true.  */
> -	  if (!tpoint->cond
> -	      || (condition_true_at_tracepoint
> -		  ((struct tracepoint_hit_ctx *) &ctx, tpoint)))
> +	  /* Check if tpoint->ptid is same with minus_one_ptid or
> +	     tinfo->entry.id, test the condition if present,
> +	     and collect if all true.  */
> +	  if ((ptid_equal (minus_one_ptid, tpoint->ptid)
> +	       || ptid_equal (tinfo->entry.id, tpoint->ptid))

Then if some other process happens to trigger an unrelated
breakpoint at exactly the same address a tracepoint is
set, then this will think a tracepoint was triggered.
So I think you should leave the tracepoint's ptid as
GDB sent, and then use ptid_match here.

> +	      && (!tpoint->cond
> +		  || (condition_true_at_tracepoint
> +		      ((struct tracepoint_hit_ctx *) &ctx, tpoint))))
>   	    collect_data_at_tracepoint ((struct tracepoint_hit_ctx *) &ctx,
>   					stop_pc, tpoint);
>   

-- 
Pedro Alves