[rfc] remote, btrace: add branch tracing protocol to Qbtrace packet

[rfc] remote, btrace: add branch tracing protocol to Qbtrace packet

[markus.t.metzger]

From: Markus Metzger <markus.t.metzger@intel.com>

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.


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  <markus.t.metzger@intel.com>
---
 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.
 
 @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.
 
 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);
-- 
1.7.1

Re: [rfc] remote, btrace: add branch tracing protocol to Qbtrace packet

[Eli Zaretskii]

> From: markus.t.metzger@intel.com
> Cc: gdb-patches@sourceware.org, markus.t.metzger@gmail.com,        Markus Metzger <markus.t.metzger@intel.com>
> Date: Fri, 22 Feb 2013 16:22:42 +0100
> 
>  gdb/doc/gdb.texinfo    |   10 +++++++---
>  gdb/gdbserver/server.c |    6 +++---
>  gdb/remote.c           |    4 ++--
>  3 files changed, 12 insertions(+), 8 deletions(-)

The documentation part is OK, with one comment:

>> +@item Qbtrace:@var{method}
> +Enable branch tracing for the current thread using the @var{method}
> +branch trace recording method.

I would rephrase slightly:

 Enable branch tracing for the current thread using the specified
 @var{method} for branch trace recording.

RE: [rfc] remote, btrace: add branch tracing protocol to Qbtrace packet

[Metzger, Markus T]

> -----Original Message-----
> From: Eli Zaretskii [mailto:eliz@gnu.org]
> Sent: Friday, February 22, 2013 4:52 PM

> >  gdb/doc/gdb.texinfo    |   10 +++++++---
> >  gdb/gdbserver/server.c |    6 +++---
> >  gdb/remote.c           |    4 ++--
> >  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> The documentation part is OK, with one comment:
> 
> >> +@item Qbtrace:@var{method}
> > +Enable branch tracing for the current thread using the @var{method}
> > +branch trace recording method.
> 
> I would rephrase slightly:
> 
>  Enable branch tracing for the current thread using the specified
>  @var{method} for branch trace recording.

Thanks. I changed it.

Do you know who would be a good reviewer for the remote serial
protocol changes?

Thanks,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [rfc] remote, btrace: add branch tracing protocol to Qbtrace packet

[Jan Kratochvil]

On Mon, 25 Feb 2013 09:50:23 +0100, Metzger, Markus T wrote:
> Do you know who would be a good reviewer for the remote serial
> protocol changes?

Definitely Pedro.


Jan

FW: [rfc] remote, btrace: add branch tracing protocol to Qbtrace packet

[Metzger, Markus T]

Hi Pedro,

Jan recommended that you review this patch about changes to the remote
serial protocol.

Thanks,
Markus.

-----Original Message-----
From: Metzger, Markus T 
Sent: Friday, February 22, 2013 4:23 PM
To: jan.kratochvil@redhat.com
Cc: gdb-patches@sourceware.org; markus.t.metzger@gmail.com; Metzger, Markus T
Subject: [rfc] remote, btrace: add branch tracing protocol to Qbtrace packet

From: Markus Metzger <markus.t.metzger@intel.com>

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.


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  <markus.t.metzger@intel.com>
---
 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.
 
 @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.
 
 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);
-- 
1.7.1

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [rfc] remote, btrace: add branch tracing protocol to Qbtrace packet

[Pedro Alves]

On 02/22/2013 03:22 PM, markus.t.metzger@intel.com wrote:
> From: Markus Metzger <markus.t.metzger@intel.com>
> 
> 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  <markus.t.metzger@intel.com>

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

RE: [rfc] remote, btrace: add branch tracing protocol to Qbtrace packet

[Metzger, Markus T]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Monday, February 25, 2013 9:33 PM

Thanks for your feedback.


> > 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?

I would add another line, e.g.

add_packet_config_cmd (&remote_protocol_packets[PACKET_Qbtrace],
    "Qbtrace:lbr", "enable-btrace-lbr", 0);


> 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.

At the moment, GDB and GDBserver equate btrace with btrace-bts.

Eventually, I want GDB to collect all the supported btrace methods and allow
the user to specify one of the supported methods, i.e.

set record btrace method bts
set record btrace method lbr

I would create the respective sub-commands dynamically after querying the
target.  Unfortunately, we cannot remove the commands again on disconnect.

As default, GDB will pick one of the supported methods, say, the first in the
qSupported response.

If some GDBserver only supports the foo branch trace recording method and
neither bts nor lbr, GDB will pick foo as default and only allow foo to be configured
as recording method.

Any GDBserver that supports Qbtrace will need to support Qbtrace:off.
If necessary, I can add this to the qSupported packets or we could define
"off" as the empty recording method.


> >  @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?

There will be separate entries for each supported method in the qSupported
response, i.e.

...:Qbtrace:bts+:Qbtrace:lbr+:...


> 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

That would be OK, as well.

How would I handle this on GDB side?
When GDB wants to enable branch tracing with method foo, what would
it send? "Qbtrace=foo"?


> 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.

Looks like xmlRegisters=... is sent by GDB.
I would need this to be sent by GDBserver and parsed by GDB.

I've also only seen this as parameter to qSupported. Is this a packet
on its own, as well?

[...]

What would be the minimal change required to fix the protocol?


Thanks,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [rfc] remote, btrace: add branch tracing protocol to Qbtrace packet

[Jan Kratochvil]

On Tue, 26 Feb 2013 10:24:32 +0100, Metzger, Markus T wrote:
> > 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.
> 
> At the moment, GDB and GDBserver equate btrace with btrace-bts.
> 
> Eventually, I want GDB to collect all the supported btrace methods and allow
> the user to specify one of the supported methods, i.e.
> 
> set record btrace method bts
> set record btrace method lbr

From the user interface point of view I expected rather:
	record full
	record btrace
	record lbr

It is already one level of "configuration", two levels of configuration make
it a bit too complicated for the user IMO.  Sure the code inside GDB would be
shared between "record-btrace" and "record-lbr" targets but that is hidden
from the user.


> I would create the respective sub-commands dynamically after querying the
> target.  Unfortunately, we cannot remove the commands again on disconnect.

I do not think there should be dynamically created commands, it is not
established in GDB.


> As default, GDB will pick one of the supported methods, say, the first in the
> qSupported response.

Do you expect more "btrace" variants than "bts" and "lbr"?

If "lbr" exists it should be enabled by default without asking the user.

For "bts" one has to type "record btrace" as it is a noticeable slowdown.

Or do I miss some plans?


> There will be separate entries for each supported method in the qSupported
> response, i.e.
> 
> ...:Qbtrace:bts+:Qbtrace:lbr+:...

colons vs. semicolons mistaken:

  ...;Qbtrace:bts+;Qbtrace:lbr+;...


No preference for the qSupported details, I am OK with your proposal myself,
"qXfer" gdbserver->gdb feature is designed that way already.


Thanks,
Jan

RE: [rfc] remote, btrace: add branch tracing protocol to Qbtrace packet

[Metzger, Markus T]

> -----Original Message-----
> From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com]
> Sent: Wednesday, February 27, 2013 5:48 PM


> > Eventually, I want GDB to collect all the supported btrace methods and allow
> > the user to specify one of the supported methods, i.e.
> >
> > set record btrace method bts
> > set record btrace method lbr
> 
> From the user interface point of view I expected rather:
> 	record full
> 	record btrace
> 	record lbr
> 
> It is already one level of "configuration", two levels of configuration make
> it a bit too complicated for the user IMO.  Sure the code inside GDB would be
> shared between "record-btrace" and "record-lbr" targets but that is hidden
> from the user.

That should be possible, as well.


> > I would create the respective sub-commands dynamically after querying the
> > target.  Unfortunately, we cannot remove the commands again on disconnect.
> 
> I do not think there should be dynamically created commands, it is not
> established in GDB.

I just thought it's more convenient for the user to only be offered commands
if the feature is available.  I'm OK to add those commands unconditionally.

Btw, I think target sub-commands are created dynamically when you call add_target.


> > As default, GDB will pick one of the supported methods, say, the first in the
> > qSupported response.
> 
> Do you expect more "btrace" variants than "bts" and "lbr"?

I would expect other processors to offer similar features.


> If "lbr" exists it should be enabled by default without asking the user.

We could do that until the user requests BTS tracing or full tracing. This would
mean that there would be a record target pushed implicitly. With the current
logic, this would prevent displaced stepping and would require an explicit
record stop before another record target can be pushed.

I think we should discuss the details once we start sending patches to enable
LBR tracing.


> For "bts" one has to type "record btrace" as it is a noticeable slowdown.

Agreed.


> > There will be separate entries for each supported method in the qSupported
> > response, i.e.
> >
> > ...:Qbtrace:bts+:Qbtrace:lbr+:...
> 
> colons vs. semicolons mistaken:
> 
>   ...;Qbtrace:bts+;Qbtrace:lbr+;...
> 
> 
> No preference for the qSupported details, I am OK with your proposal myself,
> "qXfer" gdbserver->gdb feature is designed that way already.

Pedro objected and rather wanted a comma-separated list, i.e.

....;Qbtrace=bts,lbr;...

I'm fine either way. For the way I proposed and what seems to be your preference,
as well, I already found code in GDB to handle it. For Pedro's preferred way, I have
not found anything that I could reuse.

I would like to get the minimal changes in with the btrace series so we don't need
to rework the protocol later on and are forced to maintain backwards compatibility.
I do not intend to implement full support for different branch trace recording
methods right now, since we currently only have one. We will add that support
when there's a need.

Pedro did not respond to my reply. I don't know how to proceed, here.

Regards,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [rfc] remote, btrace: add branch tracing protocol to Qbtrace packet

[Jan Kratochvil]

On Thu, 28 Feb 2013 09:45:23 +0100, Metzger, Markus T wrote:
> Btw, I think target sub-commands are created dynamically when you call add_target.

Yes; but add_target is called unconditionally during GDB startup so from the
user point of view the commands are not created dynamically.


> > If "lbr" exists it should be enabled by default without asking the user.
> 
> We could do that until the user requests BTS tracing or full tracing. This would
> mean that there would be a record target pushed implicitly. With the current
> logic, this would prevent displaced stepping and would require an explicit
> record stop before another record target can be pushed.
> 
> I think we should discuss the details once we start sending patches to enable
> LBR tracing.

Yes; I understand it may not be trivial but LBR by default would be great.


> > No preference for the qSupported details, I am OK with your proposal myself,
> > "qXfer" gdbserver->gdb feature is designed that way already.
> 
> Pedro objected and rather wanted a comma-separated list, i.e.
> 
> ....;Qbtrace=bts,lbr;...
> 
> I'm fine either way. For the way I proposed and what seems to be your preference,
> as well, I already found code in GDB to handle it. For Pedro's preferred way, I have
> not found anything that I could reuse.
> 
> I would like to get the minimal changes in with the btrace series so we don't need
> to rework the protocol later on and are forced to maintain backwards compatibility.
> I do not intend to implement full support for different branch trace recording
> methods right now, since we currently only have one. We will add that support
> when there's a need.
> 
> Pedro did not respond to my reply. I don't know how to proceed, here.

OK; hopefully Pedro will.  For example qXfer:* sets a precedent for it IMO
although delegating all gdbserver decisions to Pedro:

PacketSize=3fff;QPassSignals+;qXfer:libraries-svr4:read+;qXfer:auxv:read+;qXfer:spu:read+;qXfer:spu:write+;qXfer:siginfo:read+;qXfer:siginfo:write+;qXfer:features:read+;QStartNoAckMode+;qXfer:osdata:read+;multiprocess+;QNonStop+;QDisableRandomization+;qXfer:threads:read+;ConditionalTracepoints+;TraceStateVariables+;TracepointSource+;DisconnectedTracing+;FastTracepoints+;StaticTracepoints+;InstallInTrace+;qXfer:statictrace:read+;qXfer:traceframe-info:read+;EnableDisableTracepoints+;tracenz+


Thanks,
Jan