Re: [PATCH v3 3/7] start/stop btrace with coresight etm and parse etm buffer. nat independant

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

On 3/30/21 11:52 PM, Zied Guermazi wrote:
> 
> This patch extend branch tracing by adding the functions needed
> to collect parameters for decoding ETM traces and decoding them.
> 
> gdb/ChangeLog
> 
> 	* arch/arm.h: Add defines for exception numbers as encoded
> 	in the ETM traces
> 	* btrace.c (ftrace_new_function): log new function
> 	(ftrace_remove_last_insn): New.
> 	(cs_etm_get_etmv3_config): New.
> 	(cs_etm_get_etmv4_config): New.
> 	(cs_etm_get_isa_flag): New.
> 	(cs_etm_get_instruction_class): New.
> 	(cs_etm_update_btrace_with_inst_range): New.
> 	(cs_etm_update_btrace_with_exception): New.
> 	(cs_etm_update_btrace_with_trace_on): New.
> 	(cs_etm_trace_element_callback): New.
> 	(cs_etm_free_decoder): New.
> 	(cs_etm_create_decoder): New.
> 	(btrace_etm_readmem_callback): New.
> 	(cs_etm_add_mem_access_callback): New.
> 	(cs_etm_process_data_block): New.
> 	(btrace_print_all): New.
> 	(btrace_compute_ftrace_etm): New.
> 	(btrace_compute_ftrace_1): add handling of CoreSight traces.
> 	(btrace_enable): add error message if ETM unavailable.
> 	(btrace_stitch_trace): add handling of CoreSight traces.
> 	(maint_info_btrace_cmd): add handling of CoreSight trace format.
> 	* btrace.h (btrace_insn_flag): add ARM ISA flags.
> 	* record-btrace.c (cs_etm_reconstruct_cpsr): New.
> 	(record_btrace_target::fetch_registers): fetch cpsr register
> 	from insn->flags when applicable
> 
> gdbsupport/ChangeLog
> 
> 	* btrace-common.h (cs_etmv3_trace_params): New
> 	(cs_etmv4_trace_params): New.
> 	(cs_etm_trace_params): New.
> 	(cs_etm_decoder_params): New
> 	(btrace_data_etm_config): New.
> 	(btrace_data_etm): New.
> 	(btrace_data): add a btrace_data_etm.
> 	* btrace-common.cc (btrace_data::fini): handle CoreSight traces.
> 	(btrace_data::empty): handle CoreSight traces.
> 	(btrace_data_append): handle CoreSight traces.
> 
> 
> ---
>   gdb/ChangeLog               |  27 +-
>   gdb/arch/arm.h              |  33 ++
>   gdb/btrace.c                | 606 ++++++++++++++++++++++++++++++++++++
>   gdb/btrace.h                |  16 +-
>   gdb/record-btrace.c         |  56 +++-
>   gdbsupport/ChangeLog        |  13 +
>   gdbsupport/btrace-common.cc |  40 +++
>   gdbsupport/btrace-common.h  |  93 ++++++
>   8 files changed, 876 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index a63fdfa1911..4453a3e8185 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,28 @@
> +2021-02-25  Zied Guermazi  <zied.guermazi@trande.de>
> +
> +	* btrace.c (ftrace_remove_last_insn): New.
> +	(cs_etm_get_etmv3_config): New.
> +	(cs_etm_get_etmv4_config): New.
> +	(cs_etm_update_btrace_with_inst_range): New.
> +	(cs_etm_update_btrace_with_exception): New.
> +	(cs_etm_update_btrace_with_trace_on): New.
> +	(cs_etm_trace_element_callback): New.
> +	(cs_etm_create_decoder): New.
> +	(cs_etm_free_decoder): New.
> +	(btrace_etm_readmem_callback): New.
> +	(cs_etm_add_mem_access_callback): New.
> +	(cs_etm_process_data_block): New.
> +	(btrace_print_all): New.
> +	(btrace_compute_ftrace_etm): New.
> +	(btrace_compute_ftrace_1): add handling of CoreSight traces.
> +	(btrace_enable): add error message if ETM unavailable.
> +	(btrace_stitch_trace): add handling of CoreSight traces.
> +	(maint_info_btrace_cmd): add handling of CoreSight trace format.
> +	* btrace.h (record_btrace_reg_entry): new.
> +	(btrace_insn): add a vector of record_btrace_reg_entry.
> +	* record-btrace.c (record_btrace_target::fetch_registers): fetch
> +	registers from insn->registers when available
> +
>   2021-02-25  Zied Guermazi  <zied.guermazi@trande.de>
>   
>   	* record-btrace.c (record_btrace_print_etm_conf): New.
> @@ -9,7 +34,7 @@
>   	(_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>
> +2021-02-25  Zied Guermazi  <zied.guermazi@trande.de>
>   
>   	* Makefile.in LIBOPENCSD_C_API: set the flag.
>   	* config.in LIBOPENCSD_C_API: undefine it if
> diff --git a/gdb/arch/arm.h b/gdb/arch/arm.h
> index fa589fd0582..93fd8b7d163 100644
> --- a/gdb/arch/arm.h
> +++ b/gdb/arch/arm.h
> @@ -150,6 +150,39 @@ enum arm_m_profile_type {
>   #define BranchDest(addr,instr) \
>     ((CORE_ADDR) (((unsigned long) (addr)) + 8 + (sbits (instr, 0, 23) << 2)))
>   
> +/* Exception numbers as encoded in ETMv3.4 for ARMv7-M processors.
> +   see IHI0014Q_etm_architecture_spec table 7.11  */

see -> See

> +#define CS_ETMV3_4_CORTEX_M_EXCEPTION_USAGE_FAULT    		0x009
> +#define CS_ETMV3_4_CORTEX_M_EXCEPTION_NMI     			0x00a
> +#define CS_ETMV3_4_CORTEX_M_EXCEPTION_SVC     			0x00b
> +#define CS_ETMV3_4_CORTEX_M_EXCEPTION_DEBUG_MONITOR  		0x00c
> +#define CS_ETMV3_4_CORTEX_M_EXCEPTION_MEM_USAGE      		0x00d
> +#define CS_ETMV3_4_CORTEX_M_EXCEPTION_PEND_SV 			0x00e
> +#define CS_ETMV3_4_CORTEX_M_EXCEPTION_SYS_TICK			0x00f
> +#define CS_ETMV3_4_CORTEX_M_EXCEPTION_PROCESSOR_RESET		0x011
> +#define CS_ETMV3_4_CORTEX_M_EXCEPTION_HARD_FAULT     		0x013
> +#define CS_ETMV3_4_CORTEX_M_EXCEPTION_BUS_FAULT      		0x015
> +#define CS_ETMV3_4_CORTEX_M_EXCEPTION_IRQ(n) \
> +  (n == 0 ? 0x008 : n < 8 ? n : n + 0x010)
> +#define CS_ETMV3_4_CORTEX_M_EXCEPTION_IS_IRQ(n) \
> +  ((n > 0x000 && n < 0x009)||(n > 0x017 && n < 0x200))
> +
> +/* Exception numbers as encoded in ETMv3.4 for non ARMv7-M processors.
> +   see IHI0014Q_etm_architecture_spec table 7.12  */

see -> See

> +#define CS_ETMV3_4_CORTEX_A_R_EXCEPTION_HALTING_DEBUG		0x01
> +#define CS_ETMV3_4_CORTEX_A_R_EXCEPTION_SECURE_MONITOR_CALL  	0x02
> +#define CS_ETMV3_4_CORTEX_A_R_EXCEPTION_ENTRY_TO_HYP_MODE    	0x03
> +#define CS_ETMV3_4_CORTEX_A_R_EXCEPTION_ASYNCHRONOUS_DATA_ABORT      0x04
> +#define CS_ETMV3_4_CORTEX_A_R_EXCEPTION_JAZELLE_THUMBEE_CHECK	0x05
> +#define CS_ETMV3_4_CORTEX_A_R_EXCEPTION_PROCESSOR_RESET      	0x08
> +#define CS_ETMV3_4_CORTEX_A_R_EXCEPTION_UNDEFINED_INSTRUCTION	0x09
> +#define CS_ETMV3_4_CORTEX_A_R_EXCEPTION_SUPERVISOR_CALL      	0x0a
> +#define CS_ETMV3_4_CORTEX_A_R_EXCEPTION_PREFETCH_ABORT_SW_BKPT	0x0b
> +#define CS_ETMV3_4_CORTEX_A_R_EXCEPTION_SYNCHRONOUS_DATA_ABORT_SW_WP 0x0c
> +#define CS_ETMV3_4_CORTEX_A_R_EXCEPTION_GENERIC      		0x0d
> +#define CS_ETMV3_4_CORTEX_A_R_EXCEPTION_IRQ   			0x0e
> +#define CS_ETMV3_4_CORTEX_A_R_EXCEPTION_FIQ   			0x0f
> +
>   /* Forward declaration.  */
>   struct regcache;
>   
> diff --git a/gdb/btrace.c b/gdb/btrace.c
> index c697f37f46c..1ab5a74f0b6 100644
> --- a/gdb/btrace.c
> +++ b/gdb/btrace.c
> @@ -42,6 +42,7 @@
>   #include <inttypes.h>
>   #include <ctype.h>
>   #include <algorithm>
> +#include "arch/arm.h"
>   
>   /* Command lists for btrace maintenance commands.  */
>   static struct cmd_list_element *maint_btrace_cmdlist;
> @@ -257,6 +258,8 @@ ftrace_new_function (struct btrace_thread_info *btinfo,
>       }
>   
>     btinfo->functions.emplace_back (mfun, fun, number, insn_offset, level);
> +  ftrace_debug (&btinfo->functions.back (), "new function");
> +
>     return &btinfo->functions.back ();
>   }
>   
> @@ -671,6 +674,36 @@ ftrace_update_insns (struct btrace_function *bfun, const btrace_insn &insn)
>       ftrace_debug (bfun, "update insn");
>   }
>   
> +#if defined (HAVE_LIBOPENCSD_C_API)
> +  /* Remove last instruction from BFUN's list.
> +
> +     This function is not generic and is granted to work properly
> +     only if the same removed instruction will be added later.  */
> +

The indentation of the comment block seems wrong. The comment itself 
reads a bit confusing, but maybe that's me not understanding the tracing 
mechanisms.

Could it be clarified?

> +static void
> +ftrace_remove_last_insn (struct btrace_thread_info *btinfo)
> +{
> +  /* If we didn't have a function, we return.  */
> +  if (btinfo->functions.empty ())
> +    return;
> +
> +  struct btrace_function  *bfun;

spurious space before *bfun.

> +  bfun = &btinfo->functions.back ();
> +  /* If we had a gap before, we return.  */
> +  if (bfun->errcode != 0)
> +    return;
> +
> +  if (!bfun->insn.empty ())
> +    bfun->insn.pop_back ();
> +  else
> +    {
> +      /* A valid function must have at least one insn.  */

insn -> instruction, given it is a comment. I think we should spell it 
out completely.

> +      internal_error (__FILE__, __LINE__,
> +	    _("Attempt to remove last instruction from an empty function"));

Indentation is wrong for the error message.

> +    }
> +}
> +#endif /* defined (HAVE_LIBOPENCSD_C_API) */
> +
>   /* Classify the instruction at PC.  */
>   
>   static enum btrace_insn_class
> @@ -1502,6 +1535,562 @@ btrace_compute_ftrace_pt (struct thread_info *tp,
>   
>   #endif /* defined (HAVE_LIBIPT)  */
>   
> +#if defined (HAVE_LIBOPENCSD_C_API)
> +
> +/* This structure holds the status and the context for decoding ETM traces.
> +It is also used in the ETM trace element callback to get the context.  */

Indentation of the second line is off.

> +struct cs_etm_decoder
> +{
> +  /* The tree representing CoreSight architecture in the SoC.  */
> +  dcd_tree_handle_t dcd_tree;
> +
> +  /* Callback function to allow the decoder to access program memory.  */
> +  Fn_MemAcc_CB mem_access;

The GNU Coding Standards says variables should be lower case.

> +
> +  /* thread_info of traced thread.  */
> +  struct thread_info *t_info;
> +
> +  /* returned value of previously processed block.  */

The GNU Coding Standards says sentences should start with a capital 
letter unless they are identifiers, like the thread_info case. Please 
fix all further cases of this throughout the series.

> +  ocsd_datapath_resp_t prev_return;
> +
> +  /* ARM architecture version of associated core.  */
> +  ocsd_arch_version_t arch_version;
> +
> +  /* list of gaps in the execution record.  */
> +  std::vector<unsigned int> &gaps;
> +};
> +
> +/* fills a ocsd_etmv3_cfg from a cs_etm_trace_params.  */
> +
> +static void
> +cs_etm_get_etmv3_config (const struct cs_etm_trace_params *params,
> +			  ocsd_etmv3_cfg *config)
> +{
> +  config->reg_idr = params->etmv3.reg_idr;
> +  config->reg_ctrl = params->etmv3.reg_ctrl;
> +  config->reg_ccer = params->etmv3.reg_ccer;
> +  config->reg_trc_id = params->etmv3.reg_trc_id;
> +  config->arch_ver = (ocsd_arch_version_t)params->arch_ver;
> +  config->core_prof = (ocsd_core_profile_t)params->core_profile;
> +}
> +
> +/* fills a ocsd_etmv4_cfg from a cs_etm_trace_params.  */
> +
> +static void
> +cs_etm_get_etmv4_config (const struct cs_etm_trace_params *params,
> +			  ocsd_etmv4_cfg *config)
> +{
> +  config->reg_configr = params->etmv4.reg_configr;
> +  config->reg_traceidr = params->etmv4.reg_traceidr;
> +  config->reg_idr0 = params->etmv4.reg_idr0;
> +  config->reg_idr1 = params->etmv4.reg_idr1;
> +  config->reg_idr2 = params->etmv4.reg_idr2;
> +  config->reg_idr8 = params->etmv4.reg_idr8;
> +  config->reg_idr9 = 0;
> +  config->reg_idr10 = 0;
> +  config->reg_idr11 = 0;
> +  config->reg_idr12 = 0;
> +  config->reg_idr13 = 0;
> +  config->arch_ver = (ocsd_arch_version_t)params->arch_ver;
> +  config->core_prof = (ocsd_core_profile_t)params->core_profile;
> +}
> +
> +/* Set the insn flag to track ISA mode of ARM processor.  */
> +

insn -> instruction

> +static btrace_insn_flags
> +cs_etm_get_isa_flag (const ocsd_generic_trace_elem *elem)
> +{
> +  switch (elem->isa)
> +    {
> +    case ocsd_isa_arm:
> +      return BTRACE_INSN_FLAG_ISA_ARM;
> +
> +    case ocsd_isa_thumb2:
> +      return BTRACE_INSN_FLAG_ISA_THUMB2;
> +
> +    case ocsd_isa_aarch64:
> +      return BTRACE_INSN_FLAG_ISA_AARCH64;
> +
> +    case ocsd_isa_tee:
> +      return BTRACE_INSN_FLAG_ISA_TEE;
> +
> +    case ocsd_isa_jazelle:
> +      return BTRACE_INSN_FLAG_ISA_JAZELLE;
> +
> +    case ocsd_isa_custom:
> +      return BTRACE_INSN_FLAG_ISA_CUSTOM;
> +
> +    case ocsd_isa_unknown:
> +      return BTRACE_INSN_FLAG_ISA_UNKNOWN;
> +
> +    default:
> +      return 0;

Does "0" map to any meaningful btrace_insn_flags value? It doesn't look 
like it. We should either definete it, or error/assert here.

> +    }
> +}
> +
> +static enum btrace_insn_class
> +cs_etm_get_instruction_class (const ocsd_generic_trace_elem *elem)
> +{
> +  switch (elem->last_i_type)
> +    {
> +    case OCSD_INSTR_BR:
> +    case OCSD_INSTR_BR_INDIRECT:
> +      switch (elem->last_i_subtype)
> +	{
> +	case OCSD_S_INSTR_V8_RET:
> +	case OCSD_S_INSTR_V8_ERET:
> +	case OCSD_S_INSTR_V7_IMPLIED_RET:
> +	  return BTRACE_INSN_RETURN;
> +
> +	case OCSD_S_INSTR_BR_LINK:
> +	  return BTRACE_INSN_CALL;
> +
> +	case OCSD_S_INSTR_NONE:
> +	  return BTRACE_INSN_JUMP;
> +	}
> +      return BTRACE_INSN_OTHER;
> +
> +    case OCSD_INSTR_ISB:
> +    case OCSD_INSTR_DSB_DMB:
> +    case OCSD_INSTR_WFI_WFE:
> +    case OCSD_INSTR_OTHER:
> +      return BTRACE_INSN_OTHER;
> +
> +    default:
> +      return BTRACE_INSN_OTHER;
> +
> +    }
> +}
> +
> +/* Update btrace in the case of an instruction range.  */
> +
> +static void
> +cs_etm_update_btrace_with_inst_range (const void *context,
> +				       const ocsd_generic_trace_elem *elem)
> +{
> +  gdb_assert (elem->elem_type == OCSD_GEN_TRC_ELEM_INSTR_RANGE);
> +
> +  struct cs_etm_decoder *etm_decoder;
> +  etm_decoder = (struct cs_etm_decoder *) context;
> +  if (etm_decoder->t_info == nullptr)
> +    return;
> +
> +  struct thread_info *tp;
> +  tp = etm_decoder->t_info;
> +
> +  struct btrace_thread_info *btinfo;
> +  btinfo = &tp->btrace;
> +
> +  struct gdbarch *gdbarch;
> +  gdbarch = target_gdbarch ();

It is better to have it like so:

struct gdbarch *gdbarch = target_gdbarch ();

> +
> +  struct btrace_insn insn;
> +  CORE_ADDR pc;
> +  int size;
> +  pc = elem->st_addr;

Make it shorter like so:

CORE_ADDR pc = elem->st_addr;

> +  for (int i = 0; i< elem->num_instr_range; i++)
> +    {
> +      insn.pc = pc;
> +      try
> +	{
> +	  size = gdb_insn_length (gdbarch, pc);
> +	}
> +      catch (const gdb_exception_error &err)
> +	{
> +	  error (_("Failed to get the size of the instruction."));
> +	}
> +
> +      struct btrace_function *bfun;
> +      bfun = ftrace_update_function (btinfo, pc);
> +      insn.iclass = BTRACE_INSN_OTHER;
> +      insn.size = size;
> +      if (etm_decoder->arch_version == ARCH_V7)
> +	insn.flags = cs_etm_get_isa_flag (elem);
> +
> +      if (i == elem->num_instr_range -1)
> +	insn.iclass = cs_etm_get_instruction_class (elem);
> +
> +      ftrace_update_insns (bfun, insn);
> +      pc = pc + size;
> +    }
> +}
> +
> +/* Update btrace in the case of an exception.  */
> +
> +static void
> +cs_etm_update_btrace_with_exception (const void *context,
> +				      const ocsd_generic_trace_elem *elem)
> +{
> +  gdb_assert (elem->elem_type == OCSD_GEN_TRC_ELEM_EXCEPTION);
> +
> +  struct cs_etm_decoder *etm_decoder;
> +  etm_decoder = (struct cs_etm_decoder *) context;
> +  if (etm_decoder->t_info == nullptr)
> +    return;
> +
> +  struct thread_info *tp;
> +  tp = etm_decoder->t_info;
> +
> +  struct btrace_thread_info *btinfo;
> +  btinfo = &tp->btrace;
> +  /* Handle the implementation of breakpoints in gdb for arm (v7) architecture
> +     using undefined instructions.  */
> +
> +  if (etm_decoder->arch_version == ARCH_V7)
> +    {
> +      if (elem->exception_number
> +	   == CS_ETMV3_4_CORTEX_A_R_EXCEPTION_UNDEFINED_INSTRUCTION)
> +	{
> +	  DEBUG ("handle breakpoints implementation in gdb for ARMv7");
> +	  ftrace_remove_last_insn (btinfo);
> +	}
> +    }
> +}
> +
> +/* Update btrace in the case of a trace on.  */
> +
> +static void
> +cs_etm_update_btrace_with_trace_on (const void *context,
> +				     const ocsd_generic_trace_elem *elem)
> +{
> +  gdb_assert (elem->elem_type == OCSD_GEN_TRC_ELEM_TRACE_ON);
> +
> +  struct cs_etm_decoder *etm_decoder;
> +  etm_decoder = (struct cs_etm_decoder *)context;

struct cs_etm_decoder *etm_decoder = (struct cs_etm_decoder *) context;

> +  if (etm_decoder->t_info == nullptr)
> +    return;
> +
> +  struct thread_info *tp;
> +  tp = etm_decoder->t_info;

struct thread_info *tp = etm_decoder->t_info;
> +
> +  struct btrace_thread_info *btinfo;
> +  btinfo = &tp->btrace;

struct btrace_thread_info *btinfo = &tp->btrace;

> +
> +  if (elem->trace_on_reason != TRACE_ON_NORMAL)
> +    ftrace_new_gap (btinfo, elem->trace_on_reason, etm_decoder->gaps);
> +

Spurious new line.

> +}
> +
> +/*  Callback function when a ocsd_generic_trace_elem is emitted.  */
> +
> +static ocsd_datapath_resp_t
> +cs_etm_trace_element_callback (const void *context,
> +				const ocsd_trc_index_t indx,

indx -> index?

> +				const uint8_t trace_chan_id,
> +				const ocsd_generic_trace_elem *elem)
> +{
> +  char str_buffer[128];

How is this buffer size determined? Is it safe for it to be hardcoded 
like this? Or should we use a std:string instead?

> +  if (ocsd_gen_elem_str (elem, str_buffer, 128) == OCSD_OK)
> +    DEBUG ("ETM trace_element: index= %d, channel= 0x%x, %s",
> +	   indx, trace_chan_id, str_buffer);

Given GDB can be built as a cross debugger, we shouldn't expect sizes to 
be the same for all architectures. From what I've read, ocsd_trc_index_t 
can be either 32-bit or 64-bit. I think we should print it as %s using 
pulongest instead.

We can also print %x values using %s and phex/phex_nz if you want.
> +
> +  switch (elem->elem_type)
> +    {
> +    case OCSD_GEN_TRC_ELEM_TRACE_ON:
> +      cs_etm_update_btrace_with_trace_on (context, elem);
> +      break;
> +
> +    case OCSD_GEN_TRC_ELEM_INSTR_RANGE:
> +      cs_etm_update_btrace_with_inst_range (context, elem);
> +      break;
> +
> +    case OCSD_GEN_TRC_ELEM_EXCEPTION:
> +      cs_etm_update_btrace_with_exception (context, elem);
> +      break;
> +
> +    case OCSD_GEN_TRC_ELEM_UNKNOWN:
> +    case OCSD_GEN_TRC_ELEM_EO_TRACE:
> +    case OCSD_GEN_TRC_ELEM_NO_SYNC:
> +    case OCSD_GEN_TRC_ELEM_EXCEPTION_RET:
> +    case OCSD_GEN_TRC_ELEM_TIMESTAMP:
> +    case OCSD_GEN_TRC_ELEM_PE_CONTEXT:
> +    case OCSD_GEN_TRC_ELEM_ADDR_NACC:
> +    case OCSD_GEN_TRC_ELEM_CYCLE_COUNT:
> +    case OCSD_GEN_TRC_ELEM_ADDR_UNKNOWN:
> +    case OCSD_GEN_TRC_ELEM_EVENT:
> +    case OCSD_GEN_TRC_ELEM_SWTRACE:
> +    case OCSD_GEN_TRC_ELEM_CUSTOM:
> +    default:
> +      break;

Why have the above block if we're just falling through and breaking? I'd 
say just remove these and let it return OCSD_RESP_CONT.

> +
> +    }
> +  return OCSD_RESP_CONT;
> +}
> +
> +/* Create a cs_etm_decoder and initialize it.  */
> +
> +static bool
> +cs_etm_create_decoder (struct cs_etm_trace_params *t_params,
> +			struct cs_etm_decoder *decoder)
> +{
> +  const char *decoder_name;
> +  ocsd_etmv3_cfg config_etmv3;
> +  ocsd_etmv4_cfg trace_config_etmv4;
> +  void *trace_config;
> +  switch (t_params->protocol)
> +    {
> +    case OCSD_PROTOCOL_ETMV3:
> +    case OCSD_PROTOCOL_PTM:
> +      cs_etm_get_etmv3_config (t_params, &config_etmv3);
> +      decoder_name = (t_params->protocol == OCSD_PROTOCOL_ETMV3)
> +		      ? OCSD_BUILTIN_DCD_ETMV3 : OCSD_BUILTIN_DCD_PTM;
> +      trace_config = &config_etmv3;
> +      decoder->arch_version = ARCH_V7;
> +      break;
> +
> +    case OCSD_PROTOCOL_ETMV4I:
> +      cs_etm_get_etmv4_config (t_params, &trace_config_etmv4);
> +      decoder_name = OCSD_BUILTIN_DCD_ETMV4I;
> +      trace_config = &trace_config_etmv4;
> +      decoder->arch_version = ARCH_V8;
> +      break;
> +
> +    default:
> +      decoder->arch_version = ARCH_UNKNOWN;
> +      DEBUG ("cs_etm_create_decoder: Unknown architecture version");
> +      return false;
> +
> +  }
> +  uint8_t csid;
> +  if (ocsd_dt_create_decoder (decoder->dcd_tree, decoder_name,
> +			      OCSD_CREATE_FLG_FULL_DECODER,
> +			      trace_config, &csid))
> +    {
> +      DEBUG ("ocsd_dt_create_decoder failed");
> +      return false;
> +    }
> +
> +  if (ocsd_dt_set_gen_elem_outfn (decoder->dcd_tree,
> +				  cs_etm_trace_element_callback,
> +				  decoder))
> +    {
> +      DEBUG ("ocsd_dt_set_gen_elem_outfn failed");
> +      return false;
> +    }
> +
> +  decoder->prev_return = OCSD_RESP_CONT;
> +  return true;
> +}
> +
> +/* Free a cs_etm_decoder.  */
> +
> +static void
> +cs_etm_free_decoder (struct cs_etm_decoder *decoder)
> +{
> +  if (decoder == nullptr)
> +    return;
> +
> +  ocsd_destroy_dcd_tree (decoder->dcd_tree);
> +  decoder->dcd_tree = nullptr;
> +  decoder->t_info = nullptr;
> +  free (decoder);

malloc/free are not supposed to be called directly. Use xmalloc/xfree.

> +}
> +
> +/* Allocate a cs_etm_decoder and initialize it.  */
> +
> +static struct cs_etm_decoder *
> +cs_etm_alloc_decoder (struct thread_info *tp, int num_cpu,
> +		      struct cs_etm_decoder_params d_params,
> +		      std::vector<cs_etm_trace_params> * t_params)
> +{
> +  ocsd_dcd_tree_src_t src_type = OCSD_TRC_SRC_SINGLE;
> +  uint32_t deformatterCfgFlags = 0;
> +
> +  if (d_params.formatted)
> +    src_type = OCSD_TRC_SRC_FRAME_FORMATTED;
> +  if (d_params.frame_aligned)
> +    deformatterCfgFlags |= OCSD_DFRMTR_FRAME_MEM_ALIGN;
> +  if (d_params.fsyncs)
> +    deformatterCfgFlags |= OCSD_DFRMTR_HAS_FSYNCS;
> +  if (d_params.hsyncs)
> +    deformatterCfgFlags |= OCSD_DFRMTR_HAS_HSYNCS;
> +  if (d_params.reset_on_4x_sync)
> +    deformatterCfgFlags |= OCSD_DFRMTR_RESET_ON_4X_FSYNC;
> +
> +  dcd_tree_handle_t dcdtree_handle;
> +  dcdtree_handle = ocsd_create_dcd_tree (src_type, deformatterCfgFlags);
> +
> +  if (dcdtree_handle == C_API_INVALID_TREE_HANDLE)
> +    {
> +      DEBUG ("ocsd_create_dcd_tree failed");
> +      return nullptr;
> +    }
> +  struct cs_etm_decoder *decoder;
> +
> +  decoder = (struct cs_etm_decoder*) xmalloc (sizeof (struct cs_etm_decoder));
> +  decoder->dcd_tree = dcdtree_handle;
> +
> +  bool ret;
> +  for (int i = 0; i < num_cpu; i++)
> +    {
> +      ret = cs_etm_create_decoder (&(t_params->at (i)), decoder);
> +      if (ret == false)

You can use ret/!ret for bools.

> +	{
> +	  DEBUG ("cs_etm_create_decoder failed");
> +	  cs_etm_free_decoder (decoder);
> +	  return nullptr;
> +	}
> +
> +    }
> +  decoder->t_info = tp;
> +  return decoder;
> +}
> +
> +/* A callback function to allow the trace decoder to read the inferior's
> +   memory.  */
> +
> +static uint32_t
> +btrace_etm_readmem_callback (const void *p_context, const ocsd_vaddr_t address,
> +			      const ocsd_mem_space_acc_t mem_space,
> +			      const uint32_t reqBytes, uint8_t *byteBuffer)
> +{
> +  int result, errcode;
> +
> +  result = (int) reqBytes;
> +  try
> +  {
> +      errcode = target_read_code ((CORE_ADDR) address, byteBuffer, reqBytes);
> +      if (errcode != 0)
> +	result = 0;
> +  }
> +  catch (const gdb_exception_error &error)
> +  {
> +      result = 0;
> +  }
> +
> +  return result;
> +}
> +
> +/* Add memory access callback to the decoder.  */
> +
> +static ocsd_err_t
> +cs_etm_add_mem_access_callback (struct cs_etm_decoder *decoder,
> +				 CORE_ADDR start, CORE_ADDR end,
> +				 Fn_MemAcc_CB p_cb_func)
> +{
> +  ocsd_err_t error;
> +  error = ocsd_dt_add_callback_mem_acc (decoder->dcd_tree,
> +					 (ocsd_vaddr_t) start,
> +					 (ocsd_vaddr_t) end,
> +					 OCSD_MEM_SPACE_ANY,
> +					 p_cb_func, decoder);
> +  if (error == OCSD_OK)
> +    decoder->mem_access = p_cb_func;
> +
> +  return error;
> +}
> +
> +/* Process an etm traces data block.  */
> +
> +static int
> +cs_etm_process_data_block (struct cs_etm_decoder *decoder,
> +			   uint64_t index, const uint8_t *buf,
> +			   size_t len, size_t *consumed)
> +{
> +  int ret = 0;
> +  ocsd_datapath_resp_t cur = OCSD_RESP_CONT;
> +  ocsd_datapath_resp_t prev_return = decoder->prev_return;
> +  size_t processed = 0;
> +  uint32_t count;
> +
> +  while (processed < len)
> +    {
> +      if (OCSD_DATA_RESP_IS_WAIT (prev_return))
> +	{
> +	  cur = ocsd_dt_process_data (decoder->dcd_tree, OCSD_OP_FLUSH,
> +				       0, 0,  nullptr,  nullptr);
> +	} else if (OCSD_DATA_RESP_IS_CONT (prev_return))
> +	{
> +	  cur = ocsd_dt_process_data (decoder->dcd_tree,
> +				       OCSD_OP_DATA,
> +				       index + processed, len - processed,
> +				       &buf[processed], &count);
> +	  processed += count;
> +	} else
> +	{
> +	  DEBUG_FTRACE ("ocsd_dt_process_data returned with %d.\n", cur);

Same thing about GDB being built as a cross debugger. If 
ocsd_datapath_resp_t can be a 32-bit or 64-bit value, then it is safer 
to use the %s modifier and pulongest.

> +	  ret = -EINVAL;
> +	  break;
> +	}
> +
> +      /* Return to the input code if the packet buffer is full.
> +	 Flushing will get done once the packet buffer has been
> +	 processed.  */
> +      if (OCSD_DATA_RESP_IS_WAIT (cur))
> +	break;
> +
> +      prev_return = cur;
> +    }
> +
> +  decoder->prev_return = cur;
> +  *consumed = processed;
> +
> +  return ret;

Given this only returns 0 and -EINVAL, should we return a bool instead 
for success/failure?

> +}
> +
> +/* Print all function in a btrace.  */

function -> functions

> +
> +static void
> +btrace_print_all (struct btrace_thread_info *btinfo)
> +{
> +  for (const btrace_function &function : btinfo->functions)
> +    ftrace_debug (&function, "");
> +}
> +
> +static void
> +btrace_compute_ftrace_etm (struct thread_info *tp,
> +			   const struct btrace_data_etm *btrace,
> +			   std::vector<unsigned int> &gaps)
> +{
> +  DEBUG_FTRACE ("btrace->size is 0x%x for thread %s",
> +		(unsigned int)(btrace->size), print_thread_id (tp));
> +  if (btrace->size == 0)
> +    return;
> +
> +  struct btrace_thread_info *btinfo;
> +  btinfo = &tp->btrace;

struct btrace_thread_info *btinfo = &tp->btrace;

> +  if (btinfo->functions.empty ())
> +    btinfo->level = 0;
> +
> +  struct cs_etm_decoder *decoder;
> +  decoder = cs_etm_alloc_decoder (tp,btrace->config.num_cpu,
> +				  btrace->config.etm_decoder_params,
> +				  btrace->config.etm_trace_params);

struct cs_etm_decoder *decoder
   = cs_etm_alloc_decoder (tp,btrace->config.num_cpu,
                           btrace->config.etm_decoder_params,
                           btrace->config.etm_trace_params);

More cases of this below...

> +  if (decoder == nullptr)
> +    error (_("Failed to allocate ARM CoreSight ETM Trace decoder."));
> +
> +  ocsd_err_t ocsd_error;
> +  ocsd_error = cs_etm_add_mem_access_callback (decoder,
> +					       (CORE_ADDR)0x0L, (CORE_ADDR) -1L,
> +					       btrace_etm_readmem_callback);

... here...

> +  if (ocsd_error!= OCSD_OK)
> +    error (_("Failed to add CoreSight Trace decoder memory access callback."));
> +
> +  int errcode;
> +  size_t consumed;
> +  errcode = cs_etm_process_data_block (decoder,
> +				       0, btrace->data,
> +				       btrace->size, &consumed);

and here.

> +  if (errcode!=0)
> +    error (_("Failed to decode ARM CoreSight ETM Trace."));
> +  ftrace_compute_global_level_offset (btinfo);
> +  btrace_add_pc (tp);
> +  btrace_print_all (btinfo);
> +  cs_etm_free_decoder (decoder);
> +
> +}
> +#else /*    defined (HAVE_LIBOPENCSD_C_API)    */
> +
> +static void
> +btrace_compute_ftrace_etm (struct thread_info *tp,
> +			   const struct btrace_data_etm *btrace,
> +			   std::vector<unsigned int> &gaps)
> +{
> +

Spurious newline.

> +  internal_error (__FILE__, __LINE__, _("Unexpected branch trace format."));
> +}
> +#endif /*    defined (HAVE_LIBOPENCSD_C_API)    */
> +
>   /* Compute the function branch trace from a block branch trace BTRACE for
>      a thread given by BTINFO.  If CPU is not NULL, overwrite the cpu in the
>      branch trace configuration.  This is currently only used for the PT
> @@ -1531,6 +2120,10 @@ btrace_compute_ftrace_1 (struct thread_info *tp,
>   
>         btrace_compute_ftrace_pt (tp, &btrace->variant.pt, gaps);
>         return;
> +
> +    case BTRACE_FORMAT_ETM:
> +      btrace_compute_ftrace_etm (tp, &btrace->variant.etm, gaps);
> +      return;
>       }
>   
>     internal_error (__FILE__, __LINE__, _("Unknown branch trace format."));
> @@ -1599,6 +2192,10 @@ btrace_enable (struct thread_info *tp, const struct btrace_config *conf)
>     if (conf->format == BTRACE_FORMAT_PT)
>       error (_("Intel Processor Trace support was disabled at compile time."));
>   #endif /* !defined (HAVE_LIBIPT) */
> +#if !defined (HAVE_LIBOPENCSD_C_API)
> +  if (conf->format == BTRACE_FORMAT_ETM)
> +    error (_("ARM CoreSight Trace support was disabled at compile time."));

Maybe rephrase it as the following?

error (_("ARM CoreSight Trace support disabled at compile time."));


> +#endif /* !defined (HAVE_LIBOPENCSD_C_API) */
>   
>     DEBUG ("enable thread %s (%s)", print_thread_id (tp),
>   	 target_pid_to_str (tp->ptid).c_str ());
> @@ -1791,6 +2388,10 @@ btrace_stitch_trace (struct btrace_data *btrace, struct thread_info *tp)
>       case BTRACE_FORMAT_PT:
>         /* Delta reads are not supported.  */
>         return -1;
> +
> +    case BTRACE_FORMAT_ETM:
> +      /* Delta reads are not supported.  */
> +      return -1;

Given the outcome is the same, we should move this case up together with 
BTRACE_FORMAT_PT.

>       }
>   
>     internal_error (__FILE__, __LINE__, _("Unknown branch trace format."));
> @@ -3413,6 +4014,11 @@ maint_info_btrace_cmd (const char *args, int from_tty)
>         }
>         break;
>   #endif /* defined (HAVE_LIBIPT)  */
> +#if defined (HAVE_LIBOPENCSD_C_API)
> +    case BTRACE_FORMAT_ETM:
> +      printf_unfiltered (_("Version: %s.\n"), ocsd_get_version_str ());
> +      break;
> +#endif /* defined (HAVE_LIBOPENCSD_C_API) */
>       }
>   }
>   
> diff --git a/gdb/btrace.h b/gdb/btrace.h
> index 8f6ce32103d..92e14b6aadf 100644
> --- a/gdb/btrace.h
> +++ b/gdb/btrace.h
> @@ -34,6 +34,12 @@
>   #  include <intel-pt.h>
>   #endif
>   
> +#if defined (HAVE_LIBOPENCSD_C_API)
> +#  include <opencsd/c_api/opencsd_c_api.h>
> +#  include <opencsd/etmv4/trc_pkt_types_etmv4.h>
> +#  include <opencsd/ocsd_if_types.h>
> +#endif
> +
>   #include <vector>
>   
>   struct thread_info;
> @@ -59,7 +65,15 @@ enum btrace_insn_class
>   enum btrace_insn_flag
>   {
>     /* The instruction has been executed speculatively.  */
> -  BTRACE_INSN_FLAG_SPECULATIVE = (1 << 0)
> +  BTRACE_INSN_FLAG_SPECULATIVE =	(1 << 0),
> +  BTRACE_INSN_FLAG_ISA_ARM =		(1 << 24),
> +  BTRACE_INSN_FLAG_ISA_THUMB2 =	(2 << 24),
> +  BTRACE_INSN_FLAG_ISA_AARCH64 =	(3 << 24),
> +  BTRACE_INSN_FLAG_ISA_TEE =		(4 << 24),
> +  BTRACE_INSN_FLAG_ISA_JAZELLE =	(5 << 24),
> +  BTRACE_INSN_FLAG_ISA_CUSTOM =	(6 << 24),
> +  BTRACE_INSN_FLAG_ISA_UNKNOWN =	(7 << 24),
> +  BTRACE_INSN_FLAG_ISA_MASK =		(15 << 24)
>   };
>   DEF_ENUM_FLAGS_TYPE (enum btrace_insn_flag, btrace_insn_flags);
>   
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index 0c9ab7c2529..6002526c3cd 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -44,6 +44,7 @@
>   #include "cli/cli-style.h"
>   #include "async-event.h"
>   #include <forward_list>
> +#include "arch/arm.h"
>   
>   static const target_info record_btrace_target_info = {
>     "record-btrace",
> @@ -1556,6 +1557,30 @@ record_btrace_target::remove_breakpoint (struct gdbarch *gdbarch,
>     return ret;
>   }
>   
> +/* Reconstruct the content of CPSR register according to insn->flags.  */

content of CPSR -> contents of the CPSR

"insn->flags" -> instruction flags?

> +
> +static unsigned int
> +cs_etm_reconstruct_cpsr (const struct btrace_insn *insn)
> +{
> +  switch (insn->flags & BTRACE_INSN_FLAG_ISA_MASK)
> +    {
> +    case BTRACE_INSN_FLAG_ISA_ARM:
> +      return 0;
> +
> +    case BTRACE_INSN_FLAG_ISA_THUMB2:
> +      return 0x20;
> +
> +    case BTRACE_INSN_FLAG_ISA_TEE:
> +      return 0x1000020;
> +
> +    case BTRACE_INSN_FLAG_ISA_JAZELLE:
> +      return 0x1000000;
> +

For better readability, we should make all of these magic numbers 
constants with a meaningful name.

> +    default:
> +      return 0;

What does 0 mean in this context? Should we error/assert if this isn't 
valid?

> +    }
> +}
> +
>   /* The fetch_registers method of target record-btrace.  */
>   
>   void
> @@ -1576,19 +1601,38 @@ record_btrace_target::fetch_registers (struct regcache *regcache, int regno)
>         struct gdbarch *gdbarch;
>         int pcreg;
>   
> +      insn = btrace_insn_get (replay);
> +      gdb_assert (insn != nullptr);
> +
>         gdbarch = regcache->arch ();
>         pcreg = gdbarch_pc_regnum (gdbarch);
>         if (pcreg < 0)
>   	return;
> -
> -      /* We can only provide the PC register.  */
> -      if (regno >= 0 && regno != pcreg)
> +      /* We can only provide the PC or cpsr registers here.  */

For consistency...

cpsr -> CPSR

> +      if (regno >= 0 && !(regno == pcreg || regno == ARM_PS_REGNUM))
>   	return;
>   	
> -      insn = btrace_insn_get (replay);
> -      gdb_assert (insn != NULL);
> -
> +      if (regno == pcreg)
> +	{
>   	  regcache->raw_supply (regno, &insn->pc);
> +	  return;
> +	}
> +      if (regno == ARM_PS_REGNUM)
> +	{
> +	  /*  Provide cpsr register in the case of an armv7 target.  */

cpsr -> CPSR

> +	  const struct target_desc *tdesc;
> +	  tdesc = gdbarch_target_desc (gdbarch);

const struct target_desc *tdesc = gdbarch_target_desc (gdbarch);

> +
> +	  const char *tdesc_name;
> +	  tdesc_name = tdesc_architecture_name (tdesc);
> +	  if (strcmp (tdesc_name, "arm") == 0)
> +	    {
> +	      int cpsr;
> +	      cpsr = cs_etm_reconstruct_cpsr (insn);
> +	      regcache->raw_supply (regno, &cpsr);
> +	    }
> +	  return;
> +	}
>       }
>     else
>       this->beneath ()->fetch_registers (regcache, regno);
> diff --git a/gdbsupport/ChangeLog b/gdbsupport/ChangeLog
> index ed4708b7028..38dc5a79277 100644
> --- a/gdbsupport/ChangeLog
> +++ b/gdbsupport/ChangeLog
> @@ -1,3 +1,16 @@
> +2021-02-25  Zied Guermazi  <zied.guermazi@trande.de>
> +
> +	* btrace-common.h (cs_etmv3_trace_params): New
> +	(cs_etmv4_trace_params): New.
> +	(cs_etm_trace_params): New.
> +	(cs_etm_decoder_params): New
> +	(btrace_data_etm_config): New.
> +	(btrace_data_etm): New.
> +	(btrace_data): add a btrace_data_etm.
> +	* btrace-common.cc (btrace_data::fini): handle CoreSight traces.
> +	(btrace_data::empty): handle CoreSight traces.
> +	(btrace_data_append): handle CoreSight traces.
> +
>   2021-02-25  Zied Guermazi  <zied.guermazi@trande.de>
>   
>   	* btrace-common.h (btrace_format): add BTRACE_FORMAT_ETM
> diff --git a/gdbsupport/btrace-common.cc b/gdbsupport/btrace-common.cc
> index 82701942aa9..0ddc140d0c9 100644
> --- a/gdbsupport/btrace-common.cc
> +++ b/gdbsupport/btrace-common.cc
> @@ -86,6 +86,10 @@ btrace_data::fini ()
>       case BTRACE_FORMAT_PT:
>         xfree (variant.pt.data);
>         return;
> +
> +    case BTRACE_FORMAT_ETM:
> +      xfree (variant.etm.data);
> +      return;
>       }
>   
>     internal_error (__FILE__, __LINE__, _("Unkown branch trace format."));
> @@ -106,6 +110,9 @@ btrace_data::empty () const
>   
>       case BTRACE_FORMAT_PT:
>         return (variant.pt.size == 0);
> +
> +    case BTRACE_FORMAT_ETM:
> +      return (variant.etm.size == 0);
>       }
>   
>     internal_error (__FILE__, __LINE__, _("Unkown branch trace format."));
> @@ -191,6 +198,39 @@ btrace_data_append (struct btrace_data *dst,
>   	  }
>   	}
>         return 0;
> +
> +    case BTRACE_FORMAT_ETM:
> +      switch (dst->format)
> +	{
> +	default:
> +	  return -1;

I know this was copied from the current code, but I think this default 
case should go to the last position in the switch block. Though valid, 
it looks really odd like this (at least for me).

> +
> +	case BTRACE_FORMAT_NONE:
> +	  dst->format = BTRACE_FORMAT_ETM;
> +	  dst->variant.etm.data = nullptr;
> +	  dst->variant.etm.size = 0;
> +
> +	/* fall-through.  */
> +	case BTRACE_FORMAT_ETM:
> +	  {
> +	    gdb_byte *data;
> +	    size_t size;
> +
> +	    size = src->variant.etm.size + dst->variant.etm.size;
> +	    data = (gdb_byte *) xmalloc (size);

size_t size = src->variant.etm.size + dst->variant.etm.size;
gdb_byte *data = (gdb_byte *) xmalloc (size);

> +	
> +	    if (dst->variant.etm.size > 0)
> +	      memcpy (data, dst->variant.etm.data, dst->variant.etm.size);
> +	    memcpy (data + dst->variant.etm.size, src->variant.etm.data,
> +		    src->variant.etm.size);
> +	
> +	    xfree (dst->variant.etm.data);
> +
> +	    dst->variant.etm.data = data;
> +	    dst->variant.etm.size = size;
> +	  }
> +	}
> +      return 0;
>       }
>   
>     internal_error (__FILE__, __LINE__, _("Unkown branch trace format."));
> diff --git a/gdbsupport/btrace-common.h b/gdbsupport/btrace-common.h
> index 153b977723a..c04e9954377 100644
> --- a/gdbsupport/btrace-common.h
> +++ b/gdbsupport/btrace-common.h
> @@ -187,6 +187,96 @@ struct btrace_data_pt
>     size_t size;
>   };
>   
> +/* Parameters needed for decoding ETMv3 traces.
> +   See open source coresight trace decoder library (opencsd)
> +   for further details.  */
> +struct cs_etmv3_trace_params
> +{
> +  /* ETMv3 Main Control Register.  */
> +  uint32_t reg_ctrl;
> +
> +  /* ETMv3 Trace ID Register.  */
> +  uint32_t reg_trc_id;
> +
> +  /* ETMv3 Configuration Code Extension Register.  */
> +  uint32_t reg_ccer;
> +
> +  /* ETMv3 ID Register.  */
> +  uint32_t reg_idr;
> +};
> +
> +/* Parameters needed for decoding ETMv4 traces.
> +   See open source coresight trace decoder library (opencsd)
> +   for further details.  */
> +struct cs_etmv4_trace_params
> +{
> +  /* ETMv4 ID Register 0.  */
> +  uint32_t reg_idr0;
> +
> +  /* ETMv4 ID Register 1.  */
> +  uint32_t reg_idr1;
> +
> +  /* ETMv4 ID Register 2.  */
> +  uint32_t reg_idr2;
> +
> +  /* ETMv4 ID Register 8.  */
> +  uint32_t reg_idr8;
> +
> +  /* ETMv4 Config Register.  */
> +  uint32_t reg_configr;
> +
> +  /* ETMv4 Trace ID Register.  */
> +  uint32_t reg_traceidr;
> +};
> +
> +/* Parameters of trace source.  */
> +struct cs_etm_trace_params
> +{
> +  int arch_ver;
> +  int core_profile;
> +  int protocol;
> +  union {
> +    struct cs_etmv3_trace_params etmv3;
> +    struct cs_etmv4_trace_params etmv4;
> +  };
> +};
> +
> +/* Parameters of trace sink/decoder.  */
> +struct cs_etm_decoder_params
> +{
> +  uint8_t formatted:1,     /* Formatted input, or single source input.  */
> +  fsyncs	   :1,     /* Formatted data has fsyncs.  */
> +  hsyncs	   :1,     /* Formatted data has hsyncs.  */
> +  frame_aligned    :1,     /* Formatted frames are memory aligned.  */
> +  reset_on_4x_sync :1,     /* Reset decoders at 4x consecutive fsyncs.  */
> +  __res	    :3;     /* Reserved, not used.  */
> +};
> +
> +/* Configuration information to go with the etm trace data.  */
> +struct btrace_data_etm_config
> +{
> +  /* The number of cpu (trace sources).  */

I got slightly confused with this thinking it was the number of CPU's, 
but it is the CPU/Core number.

How about rephrasing this as the following?

"The CPU number (the trace source)"

> +  int    num_cpu;

And maybe rename this variable to "cpu" or "cpu_number". Just a suggestion.

> +  std::vector<struct cs_etm_trace_params> *etm_trace_params;
> +  struct cs_etm_decoder_params etm_decoder_params;
> +};
> +
> +/* Branch trace in arm Processor Trace format.  */

arm -> ARM

> +struct btrace_data_etm
> +{
> +  /* Some configuration information to go with the data.  */
> +  struct btrace_data_etm_config config;
> +
> +  /* The trace data.  */
> +  gdb_byte *data;
> +
> +  /* The size of DATA in bytes.  */
> +  size_t size;
> +
> +  /* Trace id for this thread. set to 0xFF to ignore it during parsing.  */

Two spaces after period.
> +  int trace_id;
> +};
> +
>   /* The branch trace data.  */
>   struct btrace_data
>   {
> @@ -224,6 +314,9 @@ struct btrace_data
>   
>       /* Format == BTRACE_FORMAT_PT.  */
>       struct btrace_data_pt pt;
> +
> +    /* Format == BTRACE_FORMAT_ETM.  */
> +    struct btrace_data_etm etm;
>     } variant;
>   
>   private:
>