This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2 2/4] tracepoint multithread and multiprocess support (gdbserver)
- From: Pedro Alves <palves at redhat dot com>
- To: Hui Zhu <teawater at gmail dot com>
- Cc: gdb-patches ml <gdb-patches at sourceware dot org>
- Date: Fri, 20 Dec 2013 19:06:04 +0000
- Subject: Re: [PATCH v2 2/4] tracepoint multithread and multiprocess support (gdbserver)
- Authentication-results: sourceware.org; auth=none
- References: <52B26826 dot 4090806 at gmail 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