From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7712 invoked by alias); 20 Dec 2013 19:06:10 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 7701 invoked by uid 89); 20 Dec 2013 19:06:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 20 Dec 2013 19:06:09 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rBKJ66NG031408 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 20 Dec 2013 14:06:06 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id rBKJ64Rc016129; Fri, 20 Dec 2013 14:06:05 -0500 Message-ID: <52B4951C.3010401@redhat.com> Date: Fri, 20 Dec 2013 19:06:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Hui Zhu CC: gdb-patches ml Subject: Re: [PATCH v2 2/4] tracepoint multithread and multiprocess support (gdbserver) References: <52B26826.4090806@gmail.com> In-Reply-To: <52B26826.4090806@gmail.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-12/txt/msg00853.txt.bz2 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 > > * server.c (handle_query): Send ";MultiProcessTracepoint+". > * tracepoint.c (tracepoint): Add ptid. * tracepoint.c (tracepoint) : 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