diff mbox series

[v9,07/10] btrace, gdbserver: Add ptwrite to btrace_config_pt.

Message ID 20230704123600.5944-8-felix.willgerodt@intel.com
State New
Headers
Series Extensions for PTWRITE |

Commit Message

Willgerodt, Felix July 4, 2023, 12:35 p.m. UTC 
  This enables gdb and gdbserver to communicate about ptwrite support.  If
ptwrite support would be enabled unconditionally, GDBs with older libipt
versions would break.
---
 gdb/btrace.c                 |  8 +++++++-
 gdb/doc/gdb.texinfo          | 21 +++++++++++++++++++++
 gdb/features/btrace-conf.dtd |  1 +
 gdb/remote.c                 | 33 +++++++++++++++++++++++++++++++++
 gdbserver/linux-low.cc       |  3 +++
 gdbserver/server.cc          | 18 ++++++++++++++++++
 gdbsupport/btrace-common.h   |  6 ++++++
 7 files changed, 89 insertions(+), 1 deletion(-)

Comments

Eli Zaretskii July 4, 2023, 12:49 p.m. UTC | #1 
> Cc: Felix Willgerodt <felix.willgerodt@intel.com>
> Date: Tue,  4 Jul 2023 14:35:57 +0200
> From: Felix Willgerodt via Gdb-patches <gdb-patches@sourceware.org>
> 
> This enables gdb and gdbserver to communicate about ptwrite support.  If
> ptwrite support would be enabled unconditionally, GDBs with older libipt
> versions would break.
> ---
>  gdb/btrace.c                 |  8 +++++++-
>  gdb/doc/gdb.texinfo          | 21 +++++++++++++++++++++
>  gdb/features/btrace-conf.dtd |  1 +
>  gdb/remote.c                 | 33 +++++++++++++++++++++++++++++++++
>  gdbserver/linux-low.cc       |  3 +++
>  gdbserver/server.cc          | 18 ++++++++++++++++++
>  gdbsupport/btrace-common.h   |  6 ++++++
>  7 files changed, 89 insertions(+), 1 deletion(-)

Thanks.

> +@item Qbtrace-conf:pt:ptwrite=@var{(yes|no)}
> +Indicate support for @code{PTWRITE} packets.  This allows for backwards
> +compatibility.

is it intentional that PTWRITE is in caps here?  In other places you
used lower-case.

> +Reply:
> +@table @samp
> +@item OK
> +The ptwrite config parameter has been set.

"Config parameter"?  Isn't this the response to the ptwrite packet?

Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Terekhov, Mikhail via Gdb-patches July 5, 2023, 10:04 a.m. UTC | #2 
> -----Original Message-----
> From: Eli Zaretskii <eliz@gnu.org>
> Sent: Dienstag, 4. Juli 2023 14:50
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org; Metzger, Markus T
> <markus.t.metzger@intel.com>; simark@simark.ca
> Subject: Re: [PATCH v9 07/10] btrace, gdbserver: Add ptwrite to
> btrace_config_pt.
> 
> > Cc: Felix Willgerodt <felix.willgerodt@intel.com>
> > Date: Tue,  4 Jul 2023 14:35:57 +0200
> > From: Felix Willgerodt via Gdb-patches <gdb-patches@sourceware.org>
> >
> > This enables gdb and gdbserver to communicate about ptwrite support.  If
> > ptwrite support would be enabled unconditionally, GDBs with older libipt
> > versions would break.
> > ---
> >  gdb/btrace.c                 |  8 +++++++-
> >  gdb/doc/gdb.texinfo          | 21 +++++++++++++++++++++
> >  gdb/features/btrace-conf.dtd |  1 +
> >  gdb/remote.c                 | 33 +++++++++++++++++++++++++++++++++
> >  gdbserver/linux-low.cc       |  3 +++
> >  gdbserver/server.cc          | 18 ++++++++++++++++++
> >  gdbsupport/btrace-common.h   |  6 ++++++
> >  7 files changed, 89 insertions(+), 1 deletion(-)
> 
> Thanks.
> 
> > +@item Qbtrace-conf:pt:ptwrite=@var{(yes|no)}
> > +Indicate support for @code{PTWRITE} packets.  This allows for backwards
> > +compatibility.
> 
> is it intentional that PTWRITE is in caps here?  In other places you
> used lower-case.

In this series, ptwrite has a couple of different meanings really.
First, it is an x86 instruction, these are usually written in caps, I actually
added the "@code" based on your previous feedback.

Then, ptwrite is also kind of the feature name. That is why different
implementation details, like the gdb.ptwrite python module, or the
ptwrite config parameter are named the same.

In the sentence that you commented on here, it is meant to mean
the ptwrite libipt paket. In that case, I would think ptwrite is again more
the instruction name, hence I wrote it in all caps.

I can only see ptwrite in lower letters in one place really:

+The ptwrite config parameter has been set.

I assume this is the occasion you meant? As it is specifically the RSP config
parameter mentioned here, and that is lower-case, I also put it lower case.

> > +Reply:
> > +@table @samp
> > +@item OK
> > +The ptwrite config parameter has been set.
> 
> "Config parameter"?  Isn't this the response to the ptwrite packet?
> 
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Thanks,
Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
diff mbox series

Patch

diff --git a/gdb/btrace.c b/gdb/btrace.c
index 2c20763b570..6cbb53b01f1 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -2273,7 +2273,7 @@  parse_xml_btrace_conf_pt (struct gdb_xml_parser *parser,
 			  std::vector<gdb_xml_value> &attributes)
 {
   struct btrace_config *conf;
-  struct gdb_xml_value *size;
+  struct gdb_xml_value *size, *ptwrite;
 
   conf = (struct btrace_config *) user_data;
   conf->format = BTRACE_FORMAT_PT;
@@ -2282,10 +2282,16 @@  parse_xml_btrace_conf_pt (struct gdb_xml_parser *parser,
   size = xml_find_attribute (attributes, "size");
   if (size != NULL)
     conf->pt.size = (unsigned int) *(ULONGEST *) size->value.get ();
+
+  ptwrite = xml_find_attribute (attributes, "ptwrite");
+  if (ptwrite != nullptr)
+    conf->pt.ptwrite = (bool) *(ULONGEST *) ptwrite->value.get ();
 }
 
 static const struct gdb_xml_attribute btrace_conf_pt_attributes[] = {
   { "size", GDB_XML_AF_OPTIONAL, gdb_xml_parse_attr_ulongest, NULL },
+  { "ptwrite", GDB_XML_AF_OPTIONAL, gdb_xml_parse_attr_enum,
+    gdb_xml_enums_boolean },
   { NULL, GDB_XML_AF_NONE, NULL, NULL }
 };
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 55a51449c7a..cf06eb4bcd7 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -43933,6 +43933,11 @@  These are the currently defined stub features and their properties:
 @tab @samp{-}
 @tab Yes
 
+@item @samp{Qbtrace-conf:pt:ptwrite}
+@tab Yes
+@tab @samp{-}
+@tab Yes
+
 @item @samp{QNonStop}
 @tab No
 @tab @samp{-}
@@ -44244,6 +44249,9 @@  The remote stub understands the @samp{Qbtrace-conf:bts:size} packet.
 @item Qbtrace-conf:pt:size
 The remote stub understands the @samp{Qbtrace-conf:pt:size} packet.
 
+@item Qbtrace-conf:pt:ptwrite
+The remote stub understands the @samp{Qbtrace-conf:pt:ptwrite} packet.
+
 @item swbreak
 The remote stub reports the @samp{swbreak} stop reason for memory
 breakpoints.
@@ -44751,6 +44759,18 @@  The ring buffer size has been set.
 A badly formed request or an error was encountered.
 @end table
 
+@item Qbtrace-conf:pt:ptwrite=@var{(yes|no)}
+Indicate support for @code{PTWRITE} packets.  This allows for backwards
+compatibility.
+
+Reply:
+@table @samp
+@item OK
+The ptwrite config parameter has been set.
+@item E.errtext
+A badly formed request or an error was encountered.
+@end table
+
 @end table
 
 @node Architecture-Specific Protocol Details
@@ -47399,6 +47419,7 @@  The formal DTD for the branch trace configuration format is given below:
 
 <!ELEMENT pt	EMPTY>
 <!ATTLIST pt	size	CDATA	#IMPLIED>
+<!ATTLIST pt	ptwrite	(yes | no)	#IMPLIED>
 @end smallexample
 
 @include agentexpr.texi
diff --git a/gdb/features/btrace-conf.dtd b/gdb/features/btrace-conf.dtd
index 3fbe3a4dc32..7aeaa4695e1 100644
--- a/gdb/features/btrace-conf.dtd
+++ b/gdb/features/btrace-conf.dtd
@@ -12,3 +12,4 @@ 
 
 <!ELEMENT pt	EMPTY>
 <!ATTLIST pt	size	CDATA	#IMPLIED>
+<!ATTLIST pt	ptwrite	(yes | no) #IMPLIED>
diff --git a/gdb/remote.c b/gdb/remote.c
index 7e3d6adfe4f..095aa630236 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -286,6 +286,9 @@  enum {
   /* Support for the Qbtrace-conf:pt:size packet.  */
   PACKET_Qbtrace_conf_pt_size,
 
+  /* Support for the Qbtrace-conf:pt:ptwrite packet.  */
+  PACKET_Qbtrace_conf_pt_ptwrite,
+
   /* Support for exec events.  */
   PACKET_exec_event_feature,
 
@@ -5597,6 +5600,8 @@  static const struct protocol_feature remote_protocol_features[] = {
     PACKET_exec_event_feature },
   { "Qbtrace-conf:pt:size", PACKET_DISABLE, remote_supported_packet,
     PACKET_Qbtrace_conf_pt_size },
+  { "Qbtrace-conf:pt:ptwrite", PACKET_DISABLE, remote_supported_packet,
+    PACKET_Qbtrace_conf_pt_ptwrite },
   { "vContSupported", PACKET_DISABLE, remote_supported_packet, PACKET_vContSupported },
   { "QThreadEvents", PACKET_DISABLE, remote_supported_packet, PACKET_QThreadEvents },
   { "no-resumed", PACKET_DISABLE, remote_supported_packet, PACKET_no_resumed },
@@ -14211,6 +14216,31 @@  remote_target::btrace_sync_conf (const btrace_config *conf)
 
       rs->btrace_config.pt.size = conf->pt.size;
     }
+
+  if ((m_features.packet_support (PACKET_Qbtrace_conf_pt_ptwrite)
+       == PACKET_ENABLE)
+	&& conf->pt.ptwrite != rs->btrace_config.pt.ptwrite)
+    {
+      pos = buf;
+      const char *ptw = conf->pt.ptwrite ? "yes" : "no";
+      const char *name
+	= packets_descriptions[PACKET_Qbtrace_conf_pt_ptwrite].name;
+      pos += xsnprintf (pos, endbuf - pos, "%s=\"%s\"", name, ptw);
+
+      putpkt (buf);
+      getpkt (&rs->buf, 0);
+
+      if (m_features.packet_ok (buf, PACKET_Qbtrace_conf_pt_ptwrite)
+	  == PACKET_ERROR)
+	{
+	  if (buf[0] == 'E' && buf[1] == '.')
+	    error (_("Failed to sync ptwrite config: %s"), buf + 2);
+	  else
+	    error (_("Failed to sync ptwrite config."));
+	}
+
+      rs->btrace_config.pt.ptwrite = conf->pt.ptwrite;
+    }
 }
 
 /* Read TP's btrace configuration from the target and store it into CONF.  */
@@ -15390,6 +15420,9 @@  Show the maximum size of the address (in bits) in a memory packet."), NULL,
   add_packet_config_cmd (PACKET_Qbtrace_conf_pt_size, "Qbtrace-conf:pt:size",
 			 "btrace-conf-pt-size", 0);
 
+  add_packet_config_cmd (PACKET_Qbtrace_conf_pt_ptwrite, "Qbtrace-conf:pt:ptwrite",
+			 "btrace-conf-pt-ptwrite", 0);
+
   add_packet_config_cmd (PACKET_vContSupported, "vContSupported",
 			 "verbose-resume-supported", 0);
 
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 8ab16698632..69471b9998e 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -6876,6 +6876,9 @@  linux_process_target::read_btrace_conf (const btrace_target_info *tinfo,
 	  string_xml_appendf (*buffer, "<pt");
 	  string_xml_appendf (*buffer, " size=\"0x%x\"", conf->pt.size);
 	  string_xml_appendf (*buffer, "/>\n");
+	  string_xml_appendf (*buffer, " ptwrite=\"%s\"",
+			     conf->pt.ptwrite ? "yes" : "no");
+	  string_xml_appendf (*buffer, "/>\n");
 	  break;
 	}
     }
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index c57270175b4..f54095b9a37 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -547,6 +547,23 @@  handle_btrace_conf_general_set (char *own_buf)
 
       current_btrace_conf.pt.size = (unsigned int) size;
     }
+  else if (strncmp (op, "pt:ptwrite=", strlen ("pt:ptwrite=")) == 0)
+    {
+      bool ptwrite;
+
+      op += strlen ("pt:ptwrite=");
+      if (strncmp (op, "\"yes\"", strlen ("\"yes\"")) == 0)
+	ptwrite = true;
+      else if (strncmp (op, "\"no\"", strlen ("\"no\"")) == 0)
+	ptwrite = false;
+      else
+	{
+	  strcpy (own_buf, "E.Bad ptwrite value.");
+	  return -1;
+	}
+
+      current_btrace_conf.pt.ptwrite = ptwrite;
+    }
   else
     {
       strcpy (own_buf, "E.Bad Qbtrace configuration option.");
@@ -2146,6 +2163,7 @@  supported_btrace_packets (char *buf)
   strcat (buf, ";Qbtrace-conf:bts:size+");
   strcat (buf, ";Qbtrace:pt+");
   strcat (buf, ";Qbtrace-conf:pt:size+");
+  strcat (buf, ";Qbtrace-conf:pt:ptwrite+");
   strcat (buf, ";Qbtrace:off+");
   strcat (buf, ";qXfer:btrace:read+");
   strcat (buf, ";qXfer:btrace-conf:read+");
diff --git a/gdbsupport/btrace-common.h b/gdbsupport/btrace-common.h
index e287c93a6c1..03cc60648e9 100644
--- a/gdbsupport/btrace-common.h
+++ b/gdbsupport/btrace-common.h
@@ -117,6 +117,12 @@  struct btrace_config_pt
      This is unsigned int and not size_t since it is registered as
      control variable for "set record btrace pt buffer-size".  */
   unsigned int size;
+
+  /* Configuration bit for ptwrite packets.
+
+     If both gdb and gdbserver support this, gdb will try to enable ptwrite
+     packets when tracing is started.  */
+  bool ptwrite;
 };
 
 /* A branch tracing configuration.