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

["Willgerodt, Felix" <felix.willgerodt@intel.com>]

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Donnerstag, 12. August 2021 13:08
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; Simon Marchi
> <simon.marchi@polymtl.ca>; Pedro Alves <pedro@palves.net>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v3 07/12] btrace, gdbserver: Add ptwrite to
> btrace_config_pt.
> 
> 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.

I also looked at git and didn't bump it because of those. I don't know what
impact the version numbers have. I was hoping for some feedback on
the mailing list in case it is needed.

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

I don't see how we could guard here.
If I use master for GDB and master + ptwrite for gdbserver,
recording and the history commands work without error message.
That might just mean that there is no length check on GDB side.
But that won't be added to old GDBs anyway.

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

Done.

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