RE: [PATCH v6 6/7] add support for coresight btrace via remote protocol

["Metzger, Markus T" <markus.t.metzger@intel.com>]

Hello Zied,

>+/* Parse a btrace etm "cpu-etm-config-etmv4_config" xml record.  */
>+
>+static void
>+parse_xml_btrace_etm_config_source_config_cpu_etmv4_config (

Isn't the element just called "etmv4-config"?


>+				struct gdb_xml_parser *parser,
>+				const struct gdb_xml_element *element,
>+				void *user_data,
>+				std::vector<gdb_xml_value> &attributes)
>+{
>+  struct btrace_data *btrace;
>+  cs_etm_trace_params *etm_trace_params;

No forward declarations; etm_trace_params can become a reference.

>+
>+  DEBUG ("parse_xml_btrace_etm_config_source_config_cpu_etmv4_config");
>+
>+  btrace = (struct btrace_data *) user_data;
>+  etm_trace_params = & (btrace->variant.etm.config.etm_trace_params->back
>());
>+
>+  struct gdb_xml_value *reg_idr0;
>+  reg_idr0
>+    = xml_find_attribute (attributes, "reg_idr0");

Please combine declaration and initialization.

>+
>+  struct gdb_xml_value *reg_idr1;
>+  reg_idr1
>+    = xml_find_attribute (attributes, "reg_idr1");
>+
>+  struct gdb_xml_value *reg_idr2;
>+  reg_idr2
>+    = xml_find_attribute (attributes, "reg_idr2");
>+
>+  struct gdb_xml_value *reg_idr8;
>+  reg_idr8
>+    = xml_find_attribute (attributes, "reg_idr8");
>+
>+  struct gdb_xml_value *reg_configr;
>+  reg_configr
>+    = xml_find_attribute (attributes, "reg_configr");
>+
>+  struct gdb_xml_value *reg_traceidr;
>+  reg_traceidr
>+    = xml_find_attribute (attributes, "reg_traceidr");
>+
>+  etm_trace_params->etmv4.reg_idr0
>+    = (unsigned int) *(ULONGEST *) reg_idr0->value.get ();

We should check those pointers before dereferencing.  If the XML
doesn't contain that attribute, the pointer would be nullptr.


>+/* Parse a btrace etm "cpu-etm-config" xml record.  */
>+
>+static void
>+parse_xml_btrace_etm_config_source_config_cpu_etm_config (

How would we arrive at that function name?  Wouldn't it simply be

parse_xml_btrace_cpu_etm_config?

More below.

>+  trace_id = xml_find_attribute (attributes, "trace_id");
>+  if (trace_id != NULL)
>+    btrace->variant.etm.trace_id
>+      = (uint8_t) *(ULONGEST *) trace_id->value.get ();

Shouldn't this be an error?


>+static const struct gdb_xml_attribute
>+btrace_etm_config_source_config_cpu_config_etmv3_config_attributes[] = {
>+  { "reg_ctrl", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },

NULL is spelled nullptr in new code.


>+  struct gdb_xml_value *sink;
>+  sink = xml_find_attribute (attributes, "sink");
>+  if (sink != nullptr)
>+    conf->etm.sink = (char*) sink->value.get ();

I don't think this memory is alive after parsing the XML.


>+static const struct gdb_xml_attribute btrace_conf_etm_attributes[] = {
>+  { "size", GDB_XML_AF_OPTIONAL, gdb_xml_parse_attr_ulongest, NULL },
>+  { "sink", GDB_XML_AF_OPTIONAL, NULL, NULL },
>+  { NULL, GDB_XML_AF_NONE, NULL, NULL }
>+};

Shouldn't there be entries for filter and config, too?


> These are the currently defined stub features, in more detail:
>@@ -42248,9 +42262,11 @@ The remote stub understands the
>@samp{Qbtrace:off} packet.
>
> @item Qbtrace:bts
> The remote stub understands the @samp{Qbtrace:bts} packet.
>+(@pxref{bts}).
>
> @item Qbtrace:pt
> The remote stub understands the @samp{Qbtrace:pt} packet.
>+(@pxref{pt}).
>
> @item Qbtrace-conf:bts:size
> The remote stub understands the @samp{Qbtrace-conf:bts:size} packet.
>@@ -42286,7 +42302,6 @@ The remote stub understands the
>@samp{QThreadEvents} packet.
> @item no-resumed
> The remote stub reports the @samp{N} stop reply.
>
>-
> @item memory-tagging
> The remote stub supports and implements the required memory tagging
> functionality and understands the @samp{qMemTags} (@pxref{qMemTags}) and
>@@ -42296,6 +42311,15 @@ For AArch64 GNU/Linux systems, this feature also
>requires access to the
> @file{/proc/@var{pid}/smaps} file so memory mapping page flags can be
>inspected.
> This is done via the @samp{vFile} requests.
>
>+@item Qbtrace:etm
>+The remote stub understands the @samp{Qbtrace:etm} packet.
>+(@pxref{etm}).

Shouldn't this command be documented together with Qbtrace:bts and
Qbtrace:pt above?


>+
>+@item Qbtrace-conf:etm:size
>+The remote stub understands the @samp{Qbtrace-conf:etm:size} packet.
>+
>+@item Qbtrace-conf:etm:sink
>+The remote stub understands the @samp{Qbtrace-conf:etm:sink} packet.
> @end table

Same here.  They would go to the Qbtrace-conf:... block.

This comment is for all new packets that are really extensions of existing
btrace packets for a new format.


>diff --git a/gdb/features/btrace-conf.dtd b/gdb/features/btrace-conf.dtd
>index 4b060bb408c..7334d035f34 100644
>--- a/gdb/features/btrace-conf.dtd
>+++ b/gdb/features/btrace-conf.dtd
>@@ -4,11 +4,17 @@
>      are permitted in any medium without royalty provided the copyright
>      notice and this notice are preserved.  -->
>
>-<!ELEMENT btrace-conf	(bts?, pt?)>
>-<!ATTLIST btrace-conf	version	CDATA	#FIXED "1.0">
>+<!ELEMENT btrace-conf	(bts?, pt?, etm?)>
>+<!ATTLIST btrace-conf	version	(1.0|1.1) #REQUIRED>

I don't know how this would be handled.  We'd need someone else to
review this part.


>diff --git a/gdb/remote.c b/gdb/remote.c
>index 9b465d77343..2defaa4503e 100644
>--- a/gdb/remote.c
>+++ b/gdb/remote.c
>@@ -2184,6 +2184,15 @@ enum {
>      packets and the tag violation stop replies.  */
>   PACKET_memory_tagging_feature,
>
>+  /* Support for the Qbtrace-etm packet.  */
>+  PACKET_Qbtrace_etm,
>+
>+  /* Support for the Qbtrace-conf:etm:size packet.  */
>+  PACKET_Qbtrace_conf_etm_size,
>+
>+  /* Support for the Qbtrace-conf:etm:sink packet.  */
>+  PACKET_Qbtrace_conf_etm_sink,

Packets for filter and config are missing.


>+static void
>+linux_low_encode_etm_config (struct buffer *buffer,
>+			    const struct btrace_data_etm_config *config)
>+{
>+  int architecture;
>+  buffer_grow_str (buffer, "<etm-config>\n");
>+  buffer_grow_str (buffer, "<source-config>\n");
>+  for (int i=0; i< config->cpu_count;i++)
>+  {
>+    if ((config->etm_trace_params->at (i).protocol == OCSD_PROTOCOL_ETMV3)
>+	||(config->etm_trace_params->at (i).protocol == OCSD_PROTOCOL_PTM))

space before (

>+      {
>+	architecture = ARCH_V7;
>+      }

No {} for a single statement.

>+    else if (config->etm_trace_params->at (i).protocol ==
>OCSD_PROTOCOL_ETMV4I)
>+      {
>+	architecture = ARCH_V8;
>+      }
>+    else
>+      {
>+	architecture = ARCH_UNKNOWN;
>+      }
>+
>+    buffer_xml_printf (buffer,"<cpu-etm-config arch_ver=\"0x%x\" "
>+	"core_prof=\"0x%x\" cpu_id=\"0x%x\" protocol=\"0x%x\">\n",

Indentation of the split string is off.  More below.

>+			  architecture, profile_CortexA,
>+			  i, config->etm_trace_params->at (i).protocol);
>+    if (architecture == ARCH_V7)
>+    {
>+      buffer_xml_printf (buffer,
>+	"<etmv3-config reg_idr=\"0x%x\" reg_ctrl=\"0x%x\" "
>+	"reg_ccer=\"0x%x\" reg_trc_id=\"0x%x\"/>\n",
>+			  config->etm_trace_params->at (i).etmv3.reg_idr,
>+			  config->etm_trace_params->at (i).etmv3.reg_ctrl,
>+			  config->etm_trace_params->at (i).etmv3.reg_ccer,
>+			  config->etm_trace_params->at (i).etmv3.reg_trc_id);

It might help to stick to the order used in the corresponding schema.


>@@ -7077,7 +7161,7 @@ linux_process_target::read_btrace_conf (const
>btrace_target_info *tinfo,
>   const struct btrace_config *conf;
>
>   buffer_grow_str (buffer, "<!DOCTYPE btrace-conf SYSTEM \"btrace-
>conf.dtd\">\n");
>-  buffer_grow_str (buffer, "<btrace-conf version=\"1.0\">\n");
>+  buffer_grow_str (buffer, "<btrace-conf version=\"1.1\">\n");

We only need 1.1 for the new features.

If we built a new gdbserver from this it wouldn't be able to use PT or BTS
with an old GDB, although we didn't change anything, there.


>   conf = linux_btrace_conf (tinfo);
>   if (conf != NULL)
>@@ -7098,6 +7182,20 @@ linux_process_target::read_btrace_conf (const
>btrace_target_info *tinfo,
> 	  buffer_xml_printf (buffer, " size=\"0x%x\"", conf->pt.size);
> 	  buffer_xml_printf (buffer, "/>\n");
> 	  break;
>+
>+	case BTRACE_FORMAT_ETM:
>+	  buffer_xml_printf (buffer, "<etm");
>+	  buffer_xml_printf (buffer, " size=\"0x%x\"", conf->etm.size);
>+	  if (conf->etm.sink !=NULL)

spaces around != and nullptr instead of NULL

>+	    {
>+	      buffer_xml_printf (buffer, " sink=\"%s\"", conf->etm.sink);
>+	    }
>+	  else
>+	    {
>+	      buffer_xml_printf (buffer, " sink=\"default\"");

We treat nullptr as default above, yet we pass "default", here.  The parser
just installs that string.

Where do we handle the "default" sink?


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