Re: [PATCH v6 2/7] add btrace coresight related commands

[Luis Machado <luis.machado@linaro.org>]

On 5/31/21 6:33 PM, Zied Guermazi wrote:
> This patch extends the commands needed for using branch tracing
> with ARM CoreSight traces.
> Those commands are:
> set record btrace etm sink
> set record btrace etm buffer-size
> record btrace etm
> 
> gdb/ChangeLog
> 
> 	* NEWS: list new commands for extending btrace
> 	to support using ARM CoreSight Traces.
> 	* record-btrace.c (record_btrace_print_etm_conf): New.
> 	(record_btrace_print_conf): handle BTRACE_FORMAT_ETM.
> 	(cmd_record_btrace_etm_start): New.
> 	(cmd_record_btrace_start): handle starting ETM tracing.
> 	(cmd_show_record_btrace_cpu): extend for ARM cpus.
> 	(show_record_etm_buffer_size_value): New.
> 	(_initialize_record_btrace): add commands for ETM traces.
> 	(record_start): add starting ETM traces.
> 
> gdb/doc/ChangeLog
> 
> 	* gdb.texinfo (Process Record and Replay): Document extending
> 	GDB btrace commands to support using ARM CoreSight traces.
> 
> gdbsupport/ChangeLog
> 
> 	* btrace-common.h (btrace_format): add BTRACE_FORMAT_ETM
> 	to the enum.
> 	(btrace_config_etm): new struct.
> 	(btrace_config): add btrace_config_etm etm.
> 	* btrace-common.cc (btrace_format_string): add BTRACE_FORMAT_ETM.
> 	(btrace_format_short_string): add BTRACE_FORMAT_ETM.
> ---
>   gdb/NEWS                    |  16 +++++
>   gdb/doc/gdb.texinfo         |  51 +++++++++++++++-
>   gdb/record-btrace.c         | 117 +++++++++++++++++++++++++++++++++++-
>   gdb/record.c                |   2 +
>   gdbsupport/btrace-common.cc |   6 ++
>   gdbsupport/btrace-common.h  |  22 ++++++-
>   6 files changed, 210 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index ab678acec8b..02035901f6e 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -2,6 +2,7 @@
>   	     (Organized release by release)
>   
>   *** Changes since GDB 10
> +* Record btrace now  supports using ARM CoreSight ETM traces.

Spurious white-space above.

>   
>   * GDB now supports general memory tagging functionality if the underlying
>     architecture supports the proper primitives and hooks.  Currently this is
> @@ -77,6 +78,21 @@
>   
>   * New commands
>   
> +record btrace etm
> +record etm
> +  Start branch trace recording using ARM CoreSight trace format (ETM).
> +
> +set|show record btrace etm buffer-size
> +  Set and show the size of the ring buffer used for branch tracing in
> +  ETM format.
> +  The obtained size may differ from the requested size.  Use "info
> +  record" to see the obtained buffer size.
> +
> +set|show record btrace etm sink
> +  Set and show the trace sink used for branch tracing in
> +  ETM format.
> +  Use "default" to reset it to default sink.
> +
>   set debug event-loop
>   show debug event-loop
>     Control the display of debug output about GDB's event loop.
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 90d827a50e7..b9b09ff97f2 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -7479,15 +7479,19 @@ For architecture environments that support process record and replay,
>   @kindex record btrace
>   @kindex record btrace bts
>   @kindex record btrace pt
> +@kindex record btrace etm
>   @kindex record bts
>   @kindex record pt
> +@kindex record etm
>   @kindex rec
>   @kindex rec full
>   @kindex rec btrace
>   @kindex rec btrace bts
>   @kindex rec btrace pt
> +@kindex rec btrace etm
>   @kindex rec bts
>   @kindex rec pt
> +@kindex rec etm
>   @item record @var{method}
>   This command starts the process record and replay target.  The
>   recording method can be specified as parameter.  Without a parameter
> @@ -7501,7 +7505,7 @@ replay implementation.  This method allows replaying and reverse
>   execution.
>   
>   @item btrace @var{format}
> -Hardware-supported instruction recording, supported on Intel
> +Hardware-supported instruction recording, supported on Intel and ARM
>   processors.  This method does not record data.  Further, the data is
>   collected in a ring buffer so old data will be overwritten when the
>   buffer is full.  It allows limited reverse execution.  Variables and
> @@ -7535,6 +7539,13 @@ Decoding the recorded execution trace, on the other hand, is more
>   expensive than decoding @acronym{BTS} trace.  This is mostly due to the
>   increased number of instructions to process.  You should increase the
>   buffer-size with care.
> +
> +@item etm
> +@cindex ARM CoreSight Trace
> +Use the @dfn{ARM CoreSight Trace} recording format.  In this
> +format, @acronym{ETM, Extended Trace Macrocell} stores the processor
> +execution trace in a compressed form that is afterwards
> +decoded by @value{GDBN}.
>   @end table
>   
>   Not all recording formats may be available on all processors.
> @@ -7682,6 +7693,12 @@ and to read-write memory.  Beware that the accessed memory corresponds
>   to the live target and not necessarily to the current replay
>   position.
>   
> +@item set record btrace etm sink @var{sink}
> +Set ARM CoreSight ETM sink to collect traces.
> +On @sc{gnu}/Linux systems, possible values for @var{sink} are the name of the
> +files in the directory @file{/sys/bus/event_source/devices/cs_etm/sinks/}.
> +Use the value @kbd{default} to reset it to default sink.
> +
>   @item set record btrace cpu @var{identifier}
>   Set the processor to be used for enabling workarounds for processor
>   errata when decoding the trace.
> @@ -7745,6 +7762,9 @@ Recorded 84872 instructions in 3189 functions (0 gaps) for thread 1 (...).
>   @item show record btrace replay-memory-access
>   Show the current setting of @code{replay-memory-access}.
>   
> +@item show record btrace etm sink
> +Show ARM CoreSight ETM sink.
> +
>   @item show record btrace cpu
>   Show the processor to be used for enabling trace decode errata
>   workarounds.
> @@ -7796,6 +7816,29 @@ also need longer to process the branch trace data before it can be used.
>   Show the current setting of the requested ring buffer size for branch
>   tracing in Intel Processor Trace format.
>   
> +@kindex set record btrace etm
> +@item set record btrace etm buffer-size @var{size}
> +@itemx set record btrace etm buffer-size unlimited
> +Set the requested ring buffer size for branch tracing in ARM
> +CoreSight ETM Trace format.  Default is 16KB.
> +
> +If @var{size} is a positive number, then @value{GDBN} will try to
> +allocate a buffer of at least @var{size} bytes for each new thread
> +that uses the btrace recording method and the ARM CoreSight ETM
> +format.  The actually obtained buffer size may differ from the
> +requested @var{size}.  Use the @code{info record} command to see the
> +actual buffer size for each thread.
> +
> +If @var{limit} is @code{unlimited} or zero, @value{GDBN} will try to
> +allocate a buffer of 4MB.
> +
> +Bigger buffers mean longer traces.  On the other hand, @value{GDBN} will
> +also need longer to process the branch trace data before it can be used.
> +
> +@item show record btrace etm buffer-size @var{size}
> +Show the current setting of the requested ring buffer size for branch
> +tracing in ARM CoreSight ETM Trace format.
> +
>   @kindex info record
>   @item info record
>   Show various statistics about the recording depending on the recording
> @@ -7847,6 +7890,12 @@ For the @code{pt} recording format, it also shows:
>   @item
>   Size of the perf ring buffer.
>   @end itemize
> +
> +For the @code{etm} recording format, it also shows:
> +@itemize @bullet
> +@item
> +Size of the perf ring buffer.
> +@end itemize
>   @end table
>   
>   @kindex record delete
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index 00affb85d22..16ffb76272b 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -201,6 +201,10 @@ static struct cmd_list_element *show_record_btrace_bts_cmdlist;
>   static struct cmd_list_element *set_record_btrace_pt_cmdlist;
>   static struct cmd_list_element *show_record_btrace_pt_cmdlist;
>   
> +/* Command lists for "set/show record btrace etm".  */
> +static struct cmd_list_element *set_record_btrace_etm_cmdlist;
> +static struct cmd_list_element *show_record_btrace_etm_cmdlist;
> +
>   /* Command list for "set record btrace cpu".  */
>   static struct cmd_list_element *set_record_btrace_cpu_cmdlist;
>   
> @@ -526,6 +530,22 @@ record_btrace_print_pt_conf (const struct btrace_config_pt *conf)
>       }
>   }
>   
> +/* Print an ARM Processor Trace configuration.  */
> +
> +static void
> +record_btrace_print_etm_conf (const struct btrace_config_etm *conf)
> +{
> +  unsigned int size;
> +
> +  size = conf->size;
> +  if (size > 0)
> +    {
> +      const char *suffix;
> +      suffix = record_btrace_adjust_size (&size);
> +      printf_unfiltered (_("Buffer size: %u%s.\n"), size, suffix);
> +    }
> +}
> +
>   /* Print a branch tracing configuration.  */
>   
>   static void
> @@ -546,6 +566,10 @@ record_btrace_print_conf (const struct btrace_config *conf)
>       case BTRACE_FORMAT_PT:
>         record_btrace_print_pt_conf (&conf->pt);
>         return;
> +
> +    case BTRACE_FORMAT_ETM:
> +      record_btrace_print_etm_conf (&conf->etm);
> +      return;
>       }
>   
>     internal_error (__FILE__, __LINE__, _("Unknown branch trace format."));
> @@ -2676,7 +2700,7 @@ record_btrace_target::stop (ptid_t ptid)
>   	  tp->btrace.flags |= BTHR_STOP;
>   	}
>       }
> - }
> +}
>   

I keep seeing the above change. It looks spurious.

>   /* The can_execute_reverse method of target record-btrace.  */
>   
> @@ -2937,6 +2961,27 @@ cmd_record_btrace_pt_start (const char *args, int from_tty)
>       }
>   }
>   
> +/* Start recording in ARM CoreSight ETM Trace format.  */
> +
> +static void
> +cmd_record_btrace_etm_start (const char *args, int from_tty)
> +{
> +  if (args != nullptr && *args != 0)
> +    error (_("Invalid argument."));
> +
> +  record_btrace_conf.format = BTRACE_FORMAT_ETM;
> +
> +  try
> +    {
> +      execute_command ("target record-btrace", from_tty);
> +    }
> +  catch (const gdb_exception &exception)
> +    {
> +      record_btrace_conf.format = BTRACE_FORMAT_NONE;
> +      throw;
> +    }
> +}
> +
>   /* Alias for "target record".  */
>   
>   static void
> @@ -2951,10 +2996,18 @@ cmd_record_btrace_start (const char *args, int from_tty)
>       {
>         execute_command ("target record-btrace", from_tty);
>       }
> -  catch (const gdb_exception &exception)
> +  catch (const gdb_exception &exception_pt)
>       {
>         record_btrace_conf.format = BTRACE_FORMAT_BTS;
>   
> +      try
> +	{
> +	  execute_command ("target record-btrace", from_tty);
> +	}
> +      catch (const gdb_exception &exception_bts)
> +	{
> +	  record_btrace_conf.format = BTRACE_FORMAT_ETM;
> +
>   	  try
>   	    {
>   	      execute_command ("target record-btrace", from_tty);
> @@ -2965,6 +3018,7 @@ cmd_record_btrace_start (const char *args, int from_tty)
>   	      throw;
>   	    }
>   	}
> +    }
>   }
>   
>   /* The "show record btrace replay-memory-access" command.  */
> @@ -3103,6 +3157,17 @@ show_record_pt_buffer_size_value (struct ui_file *file, int from_tty,
>   		    value);
>   }
>   
> +/* The "record etm buffer-size" show value function.  */
> +
> +static void
> +show_record_etm_buffer_size_value (struct ui_file *file, int from_tty,
> +				   struct cmd_list_element *c,
> +				   const char *value)
> +{
> +  fprintf_filtered (file, _("The record/replay etm buffer size is %s.\n"),
> +		    value);
> +}
> +
>   /* Initialize btrace commands.  */
>   
>   void _initialize_record_btrace ();
> @@ -3170,6 +3235,15 @@ When set to \"none\", errata workarounds are disabled."),
>   		  1,
>   		  &set_record_btrace_cmdlist);
>   
> +
> +  cmd_list_element *record_btrace_etm_cmd
> +    = add_cmd ("etm", class_obscure, cmd_record_btrace_etm_start,
> +	       _("\
> +Start branch trace recording in ARM CoreSight ETM Trace format.\n\n\
> +This format may not be available on all processors."),
> +	     &record_btrace_cmdlist);
> +  add_alias_cmd ("etm", record_btrace_etm_cmd, class_obscure, 1, &record_cmdlist);
> +
>     add_cmd ("auto", class_support, cmd_set_record_btrace_cpu_auto, _("\
>   Automatically determine the cpu to be used for trace decode."),
>   	   &set_record_btrace_cpu_cmdlist);
> @@ -3231,6 +3305,43 @@ to see the actual buffer size."), NULL, show_record_pt_buffer_size_value,
>   			    &set_record_btrace_pt_cmdlist,
>   			    &show_record_btrace_pt_cmdlist);
>   
> +  add_basic_prefix_cmd ("etm", class_support,
> +			_("Set record btrace etm options."),
> +			&set_record_btrace_etm_cmdlist,
> +			0,
> +			&set_record_btrace_cmdlist);
> +
> +  add_show_prefix_cmd ("etm", class_support,
> +		       _("Show record btrace etm options."),
> +		       &show_record_btrace_etm_cmdlist,
> +		       0,
> +		       &show_record_btrace_cmdlist);
> +
> +  add_setshow_uinteger_cmd ("buffer-size", no_class,
> +			    &record_btrace_conf.etm.size,
> +			    _("Set the record/replay etm buffer size."),
> +			    _("Show the record/replay etm buffer size."), _("\
> +Bigger buffers allow longer recording but also take more time to process \
> +the recorded execution.\n\
> +The actual buffer size may differ from the requested size.  Use \"info record\"\
> + to see the actual buffer size."), NULL, show_record_etm_buffer_size_value,

This is inconsistent with what the documentation says:

"Bigger buffers mean longer traces.  On the other hand, @value{GDBN} 
will also need longer to process the branch trace data before it can be 
used."

How about using the same text so things are consistent across the codebase?


> +		  &set_record_btrace_etm_cmdlist,
> +		  &show_record_btrace_etm_cmdlist);
> +
> +  add_setshow_string_cmd ("sink", no_class,
> +			  &record_btrace_conf.etm.sink,
> +			  _("Set the record/replay ETM sink device."),
> +			  _("Show the record/replay ETM sink device."),
> +			  _("\
> +Sink device is the device that intercepts ETM traces and collects or routes \
> +them out of the System on Chip.\n\
> +The list of available sinks on linux targets corresponds to the files in \
> +the directory \"/sys/bus/event_source/devices/cs_etm/sinks/\".\n\
> +value \"default\" reset it to default sink"),
> +			  NULL, NULL,
> +			  &set_record_btrace_etm_cmdlist,
> +			  &show_record_btrace_etm_cmdlist);
> +
>     add_target (record_btrace_target_info, record_btrace_target_open);
>   
>     bfcache = htab_create_alloc (50, bfcache_hash, bfcache_eq, NULL,
> @@ -3238,4 +3349,6 @@ to see the actual buffer size."), NULL, show_record_pt_buffer_size_value,
>   
>     record_btrace_conf.bts.size = 64 * 1024;
>     record_btrace_conf.pt.size = 16 * 1024;
> +#define DEFAULT_ETM_BUFFER_SIZE (8 * 1024)
> +  record_btrace_conf.etm.size = DEFAULT_ETM_BUFFER_SIZE;
>   }
> diff --git a/gdb/record.c b/gdb/record.c
> index 6968c30d930..32492fb53c4 100644
> --- a/gdb/record.c
> +++ b/gdb/record.c
> @@ -118,6 +118,8 @@ record_start (const char *method, const char *format, int from_tty)
>   	execute_command_to_string ("record btrace bts", from_tty, false);
>         else if (strcmp (format, "pt") == 0)
>   	execute_command_to_string ("record btrace pt", from_tty, false);
> +      else if (strcmp (format, "etm") == 0)
> +	execute_command_to_string ("record btrace etm", from_tty, false);
>         else
>   	error (_("Invalid format."));
>       }
> diff --git a/gdbsupport/btrace-common.cc b/gdbsupport/btrace-common.cc
> index 4f9ef855e74..79ff21de44b 100644
> --- a/gdbsupport/btrace-common.cc
> +++ b/gdbsupport/btrace-common.cc
> @@ -36,6 +36,9 @@ btrace_format_string (enum btrace_format format)
>   
>       case BTRACE_FORMAT_PT:
>         return _("Intel Processor Trace");
> +
> +    case BTRACE_FORMAT_ETM:
> +      return _("ARM Processor CoreSight ETM Trace");
>       }
>   
>     internal_error (__FILE__, __LINE__, _("Unknown branch trace format"));
> @@ -56,6 +59,9 @@ btrace_format_short_string (enum btrace_format format)
>   
>       case BTRACE_FORMAT_PT:
>         return "pt";
> +
> +    case BTRACE_FORMAT_ETM:
> +      return "etm";
>       }
>   
>     internal_error (__FILE__, __LINE__, _("Unknown branch trace format"));
> diff --git a/gdbsupport/btrace-common.h b/gdbsupport/btrace-common.h
> index 26d26ec957f..153b977723a 100644
> --- a/gdbsupport/btrace-common.h
> +++ b/gdbsupport/btrace-common.h
> @@ -63,7 +63,10 @@ enum btrace_format
>     BTRACE_FORMAT_BTS,
>   
>     /* Branch trace is in Intel Processor Trace format.  */
> -  BTRACE_FORMAT_PT
> +  BTRACE_FORMAT_PT,
> +
> +  /* Branch trace is ARM CoreSight ETM format.  */
> +  BTRACE_FORMAT_ETM
>   };
>   
>   /* An enumeration of cpu vendors.  */
> @@ -119,6 +122,20 @@ struct btrace_config_pt
>     unsigned int size;
>   };
>   
> +/* An ARM CoreSight ETM Trace configuration.  */
> +
> +struct btrace_config_etm
> +{
> +  /* The size of the branch trace buffer in bytes.
> +
> +     This is unsigned int and not size_t since it is registered as
> +     control variable for "set record btrace etm buffer-size".  */
> +  unsigned int size;
> +
> +  /* The sink used to collect the traces.  */
> +  char *sink;
> +};
> +
>   /* A branch tracing configuration.
>   
>      This describes the requested configuration as well as the actually
> @@ -136,6 +153,9 @@ struct btrace_config
>   
>     /* The Intel Processor Trace format configuration.  */
>     struct btrace_config_pt pt;
> +
> +  /* The ARM CoreSight ETM Trace configuration.  */
> +  struct btrace_config_etm etm;
>   };
>   
>   /* Branch trace in BTS format.  */
> 

Other than those nits, I have no further comments on this.