[v3,07/12] btrace, gdbserver: Add ptwrite to btrace_config_pt.

Message ID 20210616074205.1129553-8-felix.willgerodt@intel.com
State New
Headers show
Series
  • Extensions for PTWRITE
Related show

Commit Message

Lancelot SIX via Gdb-patches June 16, 2021, 7:42 a.m.
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/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* btrace.c (parse_xml_btrace_conf_pt): Add ptwrite to pt.conf.
	(btrace_conf_pt_attributes): Add ptwrite.

gdb/doc/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* gdb.texinfo (General Query Packets): Document Qbtrace-conf:pt:ptwrite.
	(Branch Trace Configuration Format): Document ptwrite in btrace-conf.

gdbserver/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* linux-low.cc (linux_process_target::read_btrace_conf): Add ptwrite.
	* server.cc (handle_btrace_conf_general_set): Handle pt:ptwrite.
	(supported_btrace_packets): Add Qbtrace-conf:pt:ptwrite.

gdbsupport/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* btrace-common.h (btrace_config_pt): Add ptwrite.
---
 gdb/btrace.c                 |  7 ++++++-
 gdb/doc/gdb.texinfo          | 21 +++++++++++++++++++++
 gdb/features/btrace-conf.dtd |  1 +
 gdb/remote.c                 | 30 ++++++++++++++++++++++++++++++
 gdbserver/linux-low.cc       |  1 +
 gdbserver/server.cc          | 16 ++++++++++++++++
 gdbsupport/btrace-common.h   |  6 ++++++
 7 files changed, 81 insertions(+), 1 deletion(-)

-- 
2.25.4

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

Comments

Lancelot SIX via Gdb-patches Aug. 12, 2021, 11:07 a.m. | #1
Thanks, Felix,

This patch looks good to me but I have a question about XML schema and a small nit.

>diff --git a/gdb/features/btrace-conf.dtd b/gdb/features/btrace-conf.dtd

>index 4b060bb408c..339ce4a4966 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	CDATA	#IMPLIED>


I don't know if we can simply add new attributes.  Would we at least need to bump the version number?  Looking at git log, others have added new attributes in the past.  And they didn't bump the version.


>@@ -7096,6 +7096,7 @@ linux_process_target::read_btrace_conf (const

>btrace_target_info *tinfo,

> 	case BTRACE_FORMAT_PT:

> 	  buffer_xml_printf (buffer, "<pt");

> 	  buffer_xml_printf (buffer, " size=\"0x%x\"", conf->pt.size);

>+	  buffer_xml_printf (buffer, " ptwrite=\"%d\"", conf->pt.ptwrite);


Would we need to guard that with a check whether GDB supports this new attribute?
Have you tried mixing an old GDB and a new gdbserver?


>diff --git a/gdbserver/server.cc b/gdbserver/server.cc

>index 32dcc05924e..b899f50db35 100644

>--- a/gdbserver/server.cc

>+++ b/gdbserver/server.cc

>@@ -546,6 +546,21 @@ 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)

>+    {

>+      int ptwrite;


Please declare below where it is initialized.

>+      char *endp = NULL;

>+

>+      errno = 0;

>+      ptwrite = strtoul (op + strlen ("pt:ptwrite="), &endp, 16);



Regards,
Markus.
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

Patch

diff --git a/gdb/btrace.c b/gdb/btrace.c
index 9f117a5e90d..2c4f3f67c6b 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -2261,7 +2261,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;
@@ -2270,10 +2270,15 @@  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 != NULL)
+    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_ulongest, NULL },
   { NULL, GDB_XML_AF_NONE, NULL, NULL }
 };
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 5e4f8e73b7d..60c1340d31d 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -41906,6 +41906,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{-}
@@ -42217,6 +42222,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.
@@ -42717,6 +42725,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{value}
+Control recording of ptwrite packets.  This was added 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
@@ -45359,6 +45379,7 @@  The formal DTD for the branch trace configuration format is given below:
 
 <!ELEMENT pt	EMPTY>
 <!ATTLIST pt	size	CDATA	#IMPLIED>
+<!ATTLIST pt	ptwrite	CDATA	#IMPLIED>
 @end smallexample
 
 @include agentexpr.texi
diff --git a/gdb/features/btrace-conf.dtd b/gdb/features/btrace-conf.dtd
index 4b060bb408c..339ce4a4966 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	CDATA	#IMPLIED>
diff --git a/gdb/remote.c b/gdb/remote.c
index 22933eeaeec..554dbbf107b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -2168,6 +2168,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,
 
@@ -5328,6 +5331,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 },
@@ -14028,6 +14033,28 @@  remote_target::btrace_sync_conf (const btrace_config *conf)
 
       rs->btrace_config.pt.size = conf->pt.size;
     }
+
+  packet = &remote_protocol_packets[PACKET_Qbtrace_conf_pt_ptwrite];
+  if (packet_config_support (packet) == PACKET_ENABLE
+      && conf->pt.ptwrite != rs->btrace_config.pt.ptwrite)
+    {
+      pos = buf;
+      pos += xsnprintf (pos, endbuf - pos, "%s=%d", packet->name,
+                        conf->pt.ptwrite);
+
+      putpkt (buf);
+      getpkt (&rs->buf, 0);
+
+      if (packet_ok (buf, packet) == 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 the current thread's btrace configuration from the target and
@@ -15230,6 +15257,9 @@  Show the maximum size of the address (in bits) in a memory packet."), NULL,
   add_packet_config_cmd (&remote_protocol_packets[PACKET_Qbtrace_conf_pt_size],
        "Qbtrace-conf:pt:size", "btrace-conf-pt-size", 0);
 
+  add_packet_config_cmd (&remote_protocol_packets[PACKET_Qbtrace_conf_pt_ptwrite],
+       "Qbtrace-conf:pt:ptwrite", "btrace-conf-pt-ptwrite", 0);
+
   add_packet_config_cmd (&remote_protocol_packets[PACKET_vContSupported],
 			 "vContSupported", "verbose-resume-supported", 0);
 
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 5c6191d941c..6554e9766ab 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -7096,6 +7096,7 @@  linux_process_target::read_btrace_conf (const btrace_target_info *tinfo,
 	case BTRACE_FORMAT_PT:
 	  buffer_xml_printf (buffer, "<pt");
 	  buffer_xml_printf (buffer, " size=\"0x%x\"", conf->pt.size);
+	  buffer_xml_printf (buffer, " ptwrite=\"%d\"", conf->pt.ptwrite);
 	  buffer_xml_printf (buffer, "/>\n");
 	  break;
 	}
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 32dcc05924e..b899f50db35 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -546,6 +546,21 @@  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)
+    {
+      int ptwrite;
+      char *endp = NULL;
+
+      errno = 0;
+      ptwrite = strtoul (op + strlen ("pt:ptwrite="), &endp, 16);
+      if (endp == NULL || *endp != 0 || errno != 0 || ptwrite > 1)
+	{
+	  strcpy (own_buf, "E.Bad ptwrite value.");
+	  return -1;
+	}
+
+      current_btrace_conf.pt.ptwrite = (bool) ptwrite;
+    }
   else
     {
       strcpy (own_buf, "E.Bad Qbtrace configuration option.");
@@ -2189,6 +2204,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 26d26ec957f..ccdf2fc25af 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 this is set, gdb will try to enable ptwrite packets if the OS
+     supports this.  */
+  bool ptwrite;
 };
 
 /* A branch tracing configuration.