From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7152 invoked by alias); 25 Feb 2013 20:33:02 -0000 Received: (qmail 7133 invoked by uid 22791); 25 Feb 2013 20:33:00 -0000 X-SWARE-Spam-Status: No, hits=-7.9 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_CP,TW_XS 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; Mon, 25 Feb 2013 20:32:54 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r1PKWmPF022575 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 25 Feb 2013 15:32:48 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r1PKWje6026339; Mon, 25 Feb 2013 15:32:46 -0500 Message-ID: <512BCA6D.6050909@redhat.com> Date: Mon, 25 Feb 2013 20:33:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130110 Thunderbird/17.0.2 MIME-Version: 1.0 To: markus.t.metzger@intel.com CC: jan.kratochvil@redhat.com, gdb-patches@sourceware.org, markus.t.metzger@gmail.com Subject: Re: [rfc] remote, btrace: add branch tracing protocol to Qbtrace packet References: <1361546562-26827-1-git-send-email-markus.t.metzger@intel.com> In-Reply-To: <1361546562-26827-1-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: 2013-02/txt/msg00635.txt.bz2 On 02/22/2013 03:22 PM, markus.t.metzger@intel.com wrote: > From: Markus Metzger > > We might want to add support for LBR branch tracing in the future. > > To prepare for this, I would specify the branch trace recording method > in the Qbtrace packet when requesting branch tracing. > > The supported branch trace recording methods will further be listed > in response to a QSupported packet. > > The patch works but I'm not sure whether it is in the spirit of > the remote serial protocol. I'm not sure I have a definite answer. I'll just shoot some answers/questions, and let's see if we can converse on something. Hmm: add_packet_config_cmd (&remote_protocol_packets[PACKET_Qbtrace], - "Qbtrace", "enable-btrace", 0); + "Qbtrace:bts", "enable-btrace", 0); How would you change this when another method in addition to bts is added? What if a remote stub sends in "Qbtrace:foo", but no mention of bts? The remote understands "Qbtrace:off", but this does not express that. > > > This patch does not enable GDB to support different recording methods. > GDB still only supports BTS-based branch trace recording. > > But I hope this will avoid having to change the remote serial protocol > later on when we want to add the new recording method. > > > 2013-02-22 Markus Metzger Double space after date too. > --- > gdb/doc/gdb.texinfo | 10 +++++++--- > gdb/gdbserver/server.c | 6 +++--- > gdb/remote.c | 4 ++-- > 3 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo > index d6842f1..ec934a5 100644 > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -37570,7 +37570,10 @@ rather than reporting the hit to @value{GDBN}. > The remote stub understands the @samp{qbtrace} packet. > > @item Qbtrace > -The remote stub understands the @samp{Qbtrace} packet. > +The remote stub understands the @samp{Qbtrace} packet. The branch > +trace recording method will be specified after the packet name > +separated by a single colon (@code{:}). There will be one entry for > +each supported branch trace recording method. How are the multiple supported methods separated? I think s/will be/is/ is better in the manual. Hmm, I think the user reading this is left wondering what exactly are the supported methods. qSupported already has a way of passing arguments: @table @samp @item @var{name}=@var{value} The remote protocol feature @var{name} is supported, and associated with the specified @var{value}. The format of @var{value} depends on the feature, but it must not include a semicolon. @item @var{name}+ So, that'd be, e.g.: Qbtrace=bts,foo,bar An existing feature that supports multiple values is: @item xmlRegisters This feature indicates that @value{GDBN} supports the XML target description. If the stub sees @samp{xmlRegisters=} with target specific strings separated by a comma, it will report register description. > > @end table > > @@ -37951,8 +37954,9 @@ No new branch trace data is available. > A badly formed request or an error was encountered. > @end table > > -@item Qbtrace:on > -Enable branch tracing for the current thread. > +@item Qbtrace:@var{method} > +Enable branch tracing for the current thread using the @var{method} > +branch trace recording method. I think a mention / cross reference to the corresponding qSupported packet description would be nice. > > Reply: > @table @samp > diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c > index c335af7..debef5d 100644 > --- a/gdb/gdbserver/server.c > +++ b/gdb/gdbserver/server.c > @@ -463,12 +463,12 @@ handle_btrace_general_set (char *own_buf) > > err = NULL; > > - if (strcmp (op, "on") == 0) > + if (strcmp (op, "bts") == 0) > err = handle_btrace_enable (thread); > else if (strcmp (op, "off") == 0) > err = handle_btrace_disable (thread); > else > - err = "E.Bad Qbtrace operation. Use on or off."; > + err = "E.Bad Qbtrace operation. Use bts or off."; > > if (err != 0) > strcpy (own_buf, err); > @@ -1828,7 +1828,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p) > if (target_supports_btrace ()) > { > strcat (own_buf, ";qbtrace+"); > - strcat (own_buf, ";Qbtrace+"); > + strcat (own_buf, ";Qbtrace:bts+"); > strcat (own_buf, ";qXfer:btrace:read+"); > } > > diff --git a/gdb/remote.c b/gdb/remote.c > index 6f95125..2322ece 100755 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -11164,7 +11164,7 @@ send_Qbtrace (ptid_t ptid, int enable) > set_general_thread (ptid); > > buf += xsnprintf (buf, endbuf - buf, "%s:", packet->name); > - buf += xsnprintf (buf, endbuf - buf, enable ? "on" : "off"); > + buf += xsnprintf (buf, endbuf - buf, enable ? "bts" : "off"); > putpkt (rs->buf); > getpkt (&rs->buf, &rs->buf_size, 0); > > @@ -11928,7 +11928,7 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL, > "qbtrace", "query-btrace", 0); > > add_packet_config_cmd (&remote_protocol_packets[PACKET_Qbtrace], > - "Qbtrace", "enable-btrace", 0); > + "Qbtrace:bts", "enable-btrace", 0); > > add_packet_config_cmd (&remote_protocol_packets[PACKET_qXfer_btrace], > "qXfer:btrace", "read-btrace", 0); > -- Pedro Alves