From: Pedro Alves <palves@redhat.com>
To: Hui Zhu <teawater@gmail.com>
Cc: gdb-patches ml <gdb-patches@sourceware.org>
Subject: Re: [PATCH v2 2/4] tracepoint multithread and multiprocess support (gdbserver)
Date: Fri, 20 Dec 2013 19:06:00 -0000 [thread overview]
Message-ID: <52B4951C.3010401@redhat.com> (raw)
In-Reply-To: <52B26826.4090806@gmail.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
prev parent reply other threads:[~2013-12-20 19:06 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-12-19 3:30 Hui Zhu
2013-12-20 19:06 ` Pedro Alves [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52B4951C.3010401@redhat.com \
--to=palves@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=teawater@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).