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

[Zied Guermazi <zied.guermazi@trande.de>]

hi Luis

thanks for your review comments. Here is the status of the updates. The 
changes will be published in next version of the patch set.


On 31.03.21 20:30, Luis Machado wrote:
> On 3/30/21 11:52 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/ChangeLog               |  11 ++++
>>   gdb/NEWS                    |  16 ++++++
>>   gdb/doc/gdb.texinfo         |  51 +++++++++++++++-
>>   gdb/record-btrace.c         | 112 +++++++++++++++++++++++++++++++++++-
>>   gdb/record.c                |   2 +
>>   gdbsupport/ChangeLog        |  10 ++++
>>   gdbsupport/btrace-common.cc |   6 ++
>>   gdbsupport/btrace-common.h  |  22 ++++++-
>>   8 files changed, 226 insertions(+), 4 deletions(-)
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index 7178c30affb..a63fdfa1911 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,5 +1,16 @@
>>   2021-02-25  Zied Guermazi  <zied.guermazi@trande.de>
>>   +    * 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.c (record_start): add starting ETM traces.
>> +
>> +2021-02-02  Zied Guermazi  <zied.guermazi@trande.de>
>> +
>>       * Makefile.in LIBOPENCSD_C_API: set the flag.
>>       * config.in LIBOPENCSD_C_API: undefine it if
>>       libopenocsd library is available.
>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index 7f5a745d0c0..0dbd89eb360 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.
>>     * Building GDB now requires GMP (The GNU Multiple Precision 
>> Arithmetic
>>     Library).
>> @@ -35,6 +36,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 80ccf74a049..bd5b2c91869 100644
>> --- a/gdb/doc/gdb.texinfo
>> +++ b/gdb/doc/gdb.texinfo
>> @@ -7371,15 +7371,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
>> @@ -7393,7 +7397,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
>> @@ -7427,6 +7431,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.
>> @@ -7574,6 +7585,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.
>> @@ -7637,6 +7654,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.
>> @@ -7688,6 +7708,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
>> @@ -7739,6 +7782,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 d9cc7a3b6d8..0c9ab7c2529 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;
>>   @@ -525,6 +529,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
>> @@ -545,6 +565,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."));
>> @@ -2675,7 +2699,7 @@ record_btrace_target::stop (ptid_t ptid)
>>         tp->btrace.flags |= BTHR_STOP;
>>       }
>>       }
>> - }
>> +}
>
> Spurious change?
[Zied] this is fixing a wrong indentation. previously a space was 
erroneously put before the closing braces. I fixed it while I was 
visiting this function. shall I keep it or remove it?
>
>>     /* The can_execute_reverse method of target record-btrace.  */
>>   @@ -2936,6 +2960,27 @@ cmd_record_btrace_pt_start (const char 
>> *args, int from_tty)
>>       }
>>   }
>>   +/* Start recording in arm CoreSight ETM Trace format.  */
>
> arm -> ARM (and in other similar places).
[Zied] fixed here. I will look for similar occurrences.
>
>> +
>> +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
>> @@ -2950,10 +2995,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;
>> +
>
> The indentation of the curly braces seems off.

[Zied] in the code the indentation is fine. In the patch, the + at the 
beginning of the line change the way some editors displays it. here is 
the source code in gedit with spaces and tabs displayed.

>
>>         try
>>           {
>>             execute_command ("target record-btrace", from_tty);
>> @@ -2964,6 +3017,7 @@ cmd_record_btrace_start (const char *args, int 
>> from_tty)
>>             throw;
>>           }
>>       }
>> +    }
>>   }
>>     /* The "show record btrace replay-memory-access" command. */
>> @@ -3102,6 +3156,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);
>
> Indentation problem.

[Zied] the same issue mentioned above on the impact of the + on the way 
some editors align tabs. here is the code as displayed in gedit showing 
tabs and spaces

>
>> +}
>> +
>>   /* Initialize btrace commands.  */
>>     void _initialize_record_btrace ();
>> @@ -3165,6 +3230,13 @@ When set to \"none\", errata workarounds are 
>> disabled."),
>>             "set record btrace cpu ", 1,
>>             &set_record_btrace_cmdlist);
>>   +  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", "btrace etm", 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);
>> @@ -3226,6 +3298,41 @@ 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,
>> +            "set record btrace etm ", 0, &set_record_btrace_cmdlist);
>> +
>> +  add_show_prefix_cmd ("etm", class_support,
>> +               _("Show record btrace etm options."),
>> +               &show_record_btrace_etm_cmdlist,
>> +               "show record btrace etm ", 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,
>> +          &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 SoC.\n\
>
> Just a suggestion. Should we go with "device" or "System On a Chip"?
[Zied] in the terminology I am using, a System on Chip is a set of 
processors/cores and a set of devices (can also be designated as 
controllers, peripherals). is there a standard terminology in GDB to 
designate them? In the case of ETM sink, devices can be for example an 
ETB, a TMC or a TPIU.
>
>> +The list of available sinks on linux targets correspond to the files 
>> in \
>
> correspond -> corresponds
[Zied] fixed
>
>> +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,
>> @@ -3233,4 +3340,5 @@ 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;
>> +  record_btrace_conf.etm.size = 8 * 1024;
>
> Suggestion. It would be nice to have a more meaningful constant name 
> here, if possible.

[Zied] changed to

#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 cd541b56f43..c9de7b77370 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/ChangeLog b/gdbsupport/ChangeLog
>> index f27b57c2fa9..ed4708b7028 100644
>> --- a/gdbsupport/ChangeLog
>> +++ b/gdbsupport/ChangeLog
>> @@ -1,3 +1,13 @@
>> +2021-02-25  Zied Guermazi  <zied.guermazi@trande.de>
>> +
>> +    * btrace-common.h (btrace_format): add BTRACE_FORMAT_ETM
>> +    to the enum.
>> +    (btrace_cpu_vendor): add CV_ARM 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.
>> +
>>   2021-02-25  Zied Guermazi  <zied.guermazi@trande.de>
>>         * gdbsupport/common.m4: check --with-arm-cs configuration
>> diff --git a/gdbsupport/btrace-common.cc b/gdbsupport/btrace-common.cc
>> index 4f9ef855e74..82701942aa9 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.  */
>>

Kind Regards
Zied Guermazi