From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23292 invoked by alias); 28 Nov 2012 19:23:46 -0000 Received: (qmail 23268 invoked by uid 22791); 28 Nov 2012 19:23:44 -0000 X-SWARE-Spam-Status: No, hits=-7.8 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_QB,TW_XS,TW_XZ X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 28 Nov 2012 19:23:33 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qASJNTRW031100 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 28 Nov 2012 14:23:31 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id qASJNRdL021430; Wed, 28 Nov 2012 14:23:27 -0500 Message-ID: <50B664AE.1020006@redhat.com> Date: Wed, 28 Nov 2012 19:23:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Thunderbird/17.0 MIME-Version: 1.0 To: markus.t.metzger@intel.com CC: gdb-patches@sourceware.org, markus.t.metzger@gmail.com, jan.kratochvil@redhat.com, palves@redhat.com, tromey@redhat.com, kettenis@gnu.org Subject: Re: [patch v4 08/13] remote, btrace: add branch trace remote ops References: <1354013351-14791-1-git-send-email-markus.t.metzger@intel.com> <1354013351-14791-9-git-send-email-markus.t.metzger@intel.com> In-Reply-To: <1354013351-14791-9-git-send-email-markus.t.metzger@intel.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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 X-SW-Source: 2012-11/txt/msg00825.txt.bz2 New packets always need documentation. And NEWS entries too. The new "set remote XXX" commands need docs too. On 11/27/2012 10:49 AM, markus.t.metzger@intel.com wrote: > From: Markus Metzger > > Add the gdb remote target operations for branch tracing. We define the following > packets: > > qbtrace: query if new trace data is available for a thread > returns "yes" or "no" or "Enn" The code isn't handling "no". > > Qbtrace:on: enable branch tracing for one thread > returns "OK" or "Enn" > > Qbtrace:off: disable branch tracing for one thread > returns "OK" or "Enn" Nit, I'd put first: > Qbtrace::on enable branch tracing for one thread > returns "OK" or "Enn" > > Qbtrace::off disable branch tracing for one thread > returns "OK" or "Enn" How does "btrace enable all" for new threads work with remote targets? GDB isn't notified of new threads until they stop for some reason. > > qXfer:btrace:read: read the full branch trace data for one thread No need to pass the thread in the annex. qXfer packets use the general context set by remote_xfer_partial -> set_current_thread. > (_initialize_remote): Add btrace packets. Say: (_initialize_remote): Add packet configuration for branch tracing. > diff --git a/gdb/remote.c b/gdb/remote.c > index 929d4f5..c512b93 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -67,6 +67,7 @@ > #include "ax.h" > #include "ax-gdb.h" > #include "agent.h" > +#include "btrace.h" Should be mentioned on the change log too. > > /* Temp hacks for tracepoint encoding migration. */ > static char *target_buf; > @@ -1292,6 +1293,9 @@ enum { > PACKET_qXfer_fdpic, > PACKET_QDisableRandomization, > PACKET_QAgent, > + PACKET_qbtrace, > + PACKET_Qbtrace, > + PACKET_qXfer_btrace, > PACKET_MAX > }; > > @@ -3946,6 +3950,10 @@ static struct protocol_feature remote_protocol_features[] = { > { "QAgent", PACKET_DISABLE, remote_supported_packet, PACKET_QAgent}, > { "tracenz", PACKET_DISABLE, > remote_string_tracing_feature, -1 }, > + { "qbtrace", PACKET_DISABLE, remote_supported_packet, PACKET_qbtrace }, > + { "Qbtrace", PACKET_DISABLE, remote_supported_packet, PACKET_Qbtrace }, > + { "qXfer:btrace:read", PACKET_DISABLE, remote_supported_packet, > + PACKET_qXfer_btrace } > }; > > static char *remote_support_xml; > @@ -8682,6 +8690,10 @@ remote_xfer_partial (struct target_ops *ops, enum target_object object, > return remote_read_qxfer (ops, "uib", annex, readbuf, offset, len, > &remote_protocol_packets[PACKET_qXfer_uib]); > > + case TARGET_OBJECT_BTRACE: > + return remote_read_qxfer (ops, "btrace", annex, readbuf, offset, len, > + &remote_protocol_packets[PACKET_qXfer_btrace]); > + > default: > return -1; > } > @@ -11001,6 +11013,137 @@ remote_can_use_agent (void) > return (remote_protocol_packets[PACKET_QAgent].support != PACKET_DISABLE); > } > > +struct btrace_target_info > +{ > + /* The ptid of the traced thread. */ > + ptid_t ptid; > +}; Hmm, why would you need to store this? To get at a struct btrace_target_info, you need to start from a struct thread_info, so why not just pass that down, or pass the ptid down? > + > +/* Check whether the target supports branch tracing. */ > +static int > +remote_supports_btrace (void) > +{ > + if (remote_protocol_packets[PACKET_qbtrace].support != PACKET_ENABLE) > + return 0; > + if (remote_protocol_packets[PACKET_Qbtrace].support != PACKET_ENABLE) > + return 0; > + if (remote_protocol_packets[PACKET_qXfer_btrace].support != PACKET_ENABLE) > + return 0; > + > + return 1; > +} > + > +/* Enable branch tracing for @ptid. */ > +static struct btrace_target_info * > +remote_enable_btrace (ptid_t ptid) > +{ > + struct packet_config *packet = &remote_protocol_packets[PACKET_Qbtrace]; > + struct remote_state *rs = get_remote_state (); > + struct btrace_target_info *tinfo = NULL; > + char *buf = rs->buf; > + char *endbuf = rs->buf + get_remote_packet_size (); > + > + if (packet->support != PACKET_ENABLE) > + error (_("Target does not support branch tracing.")); > + > + buf += xsnprintf (buf, endbuf - buf, "%s", packet->name); > + buf += xsnprintf (buf, endbuf - buf, ":on:"); > + buf = write_ptid (buf, endbuf, ptid); > + putpkt (rs->buf); > + getpkt (&rs->buf, &rs->buf_size, 0); > + > + if (packet_ok (rs->buf, packet) != PACKET_OK) > + error (_("Could not enable branch tracing for %s."), > + target_pid_to_str (ptid)); > + > + tinfo = xzalloc (sizeof (*tinfo)); > + tinfo->ptid = ptid; > + > + return tinfo; > +} > + > +/* Disable branch tracing. */ > +static void > +remote_disable_btrace (struct btrace_target_info *tinfo) > +{ > + struct packet_config *packet = &remote_protocol_packets[PACKET_Qbtrace]; > + struct remote_state *rs = get_remote_state (); > + char *buf = rs->buf; > + char *endbuf = rs->buf + get_remote_packet_size (); > + > + if (packet->support != PACKET_ENABLE) > + error (_("Target does not support branch tracing.")); > + > + buf += xsnprintf (buf, endbuf - buf, "%s", packet->name); > + buf += xsnprintf (buf, endbuf - buf, ":off:"); > + buf = write_ptid (buf, endbuf, tinfo->ptid); > + putpkt (rs->buf); > + getpkt (&rs->buf, &rs->buf_size, 0); > + > + if (packet_ok (rs->buf, packet) != PACKET_OK) > + error (_("Could not disable branch tracing for %s."), > + target_pid_to_str (tinfo->ptid)); > + > + xfree (tinfo); > +} remote_enable_btrace/remote_disable_btrace are almost identical. You could factor this out to a function that takes a on/off boolean. -- Pedro Alves