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

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

/Zied


On 31.03.21 22:23, Luis Machado wrote:
> 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
>
[Zied] done.
>
>> +#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
[Zied] Done.
>
>
>> +#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?
>
[Zied] indentation fixed.

this function is used to workaround the fact that GDB on ARMv7 
implements the breakpoint as an undefined instruction. in this case the 
first attempt to execute the instruction is traced. then the exception 
is traced.

when the undefined instruction exception is raised, we remove the last 
traced instruction. Some side effects of the last instruction (chaining 
of functions) are hard to clean up. so we just remove the instruction 
form the function it belongs to. This is safe because GDB patches again 
the code with the "real" instruction and executes it.

in conclusion this function is guaranteed to work properly if the 
instruction executed after the exception have the same side effects than 
the one that was removed.

the comment is changed to:

/* Remove last instruction from BFUN's list.
    This function is not generic and does not undo functions chaining.
    When adding an instruction after using it, the user must ensure
    that the instruction produces the same chaining.
    An example of good case, is when the same removed instruction
    is added later.  */


>
>> +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.
>
[Zied] fixed
>
>> +  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.
>
[Zied] done.
>
>> +      internal_error (__FILE__, __LINE__,
>> +        _("Attempt to remove last instruction from an empty 
>> function"));
>
> Indentation is wrong for the error message.
>
[Zied] done
>
>> +    }
>> +}
>> +#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.
>
[Zied] done.
>
>> +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.
>
[Zied] Fn_MemAcc_CB is the type of the variable. it is defined in the 
ETM decoder library. member variable is mem_access which is already 
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.
>
[Zied] done.
>
>> +  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.
>
[Zied]  default case shall not occur in ARMv7. Handled as internal_error.
>
>> +    }
>> +}
>> +
>> +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 ();
>
[Zied] done
>
>> +
>> +  struct btrace_insn insn;
>> +  CORE_ADDR pc;
>> +  int size;
>> +  pc = elem->st_addr;
>
> Make it shorter like so:
>
> CORE_ADDR pc = elem->st_addr;
>
[Zied] done.
>
>> +  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;
>
[Zied] done.
>
>> +  if (etm_decoder->t_info == nullptr)
>> +    return;
>> +
>> +  struct thread_info *tp;
>> +  tp = etm_decoder->t_info;
>
> struct thread_info *tp = etm_decoder->t_info;
>
[Zied] done.
>> +
>> +  struct btrace_thread_info *btinfo;
>> +  btinfo = &tp->btrace;
>
> struct btrace_thread_info *btinfo = &tp->btrace;
>
[Zied] done
>
>> +
>> +  if (elem->trace_on_reason != TRACE_ON_NORMAL)
>> +    ftrace_new_gap (btinfo, elem->trace_on_reason, etm_decoder->gaps);
>> +
>
> Spurious new line.
>
[Zied] removed
>
>> +}
>> +
>> +/*  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?
>
[Zied] Fixed.
>
>> +                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?
>
[Zied] it is big enough to hold known messages. In the worst case, 
ocsd_gen_elem_str is truncating the string to buffer_size. here is the 
implementation of this function.

OCSD_C_API ocsd_err_t ocsd_gen_elem_str(const ocsd_generic_trace_elem 
*p_pkt, char *buffer, const int buffer_size)
{
     ocsd_err_t err = OCSD_OK;
     if((buffer == NULL) || (buffer_size < 2))
         return OCSD_ERR_INVALID_PARAM_VAL;
     std::string str;
trcPrintElemToString<OcsdTraceElement,ocsd_generic_trace_elem>(p_pkt,str);
     if(str.size() > 0)
     {
         strncpy(buffer,str.c_str(),buffer_size -1);
         buffer[buffer_size-1] = 0;
     }
     return err;
}

considering this I recommend keeping it as it is.

>> +  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.
>
[Zied] yes, there is now a compilation flag ENABLE_LARGE_TRACE_SOURCES 
in opencsd. when defined, ocsd_trc_index_t will be 64 bits. change done.
>
> 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.
>
[Zied] these are all possible types of trace elements. current 
implementation for linux does not handle all of them, but they will be 
implemented in the future when for example cycle accurate is added 
(CYCLE_COUNT), timestamp is defined (TIMESTAMP) or support for 
arm-none-eabi (EXCEPTION_RET) is added.

I recommend keeping it as it is.

>
>> +
>> +    }
>> +  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.
>
[Zied] done.
>
>> +}
>> +
>> +/* 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.
>
[Zied] done
>
>> +    {
>> +      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.
>
[Zied] opencsd library and GDB are build and running on the same machine 
and for the same architecture. This is safe even if we are debugging 
remotely.
>
>> +      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
>
[Zied] done
>
>> +
>> +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;
>
[Zied] done
>
>> +  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);
>
>
[Zied] done
> 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...
>
[Zied] done
>
>> +  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.
>
[Zied] done
>
>> +  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.
>
[Zied] fixed
>
>> +  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."));
>
>
[Zied] I am simply using the same phrasing, few lines above for intel pt

error (_("Intel Processor Trace support was disabled at compile time."));

I recommend keeping the same wording

>
>> +#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?
>
[Zied] done
>> +
>> +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.

[Zied] I commented and documented them as following

/* Reconstruct the instruction set state bits of CPSR register
    according to instruction flags.
    See Table A2-1 in DDI0406B_arm_architecture_reference_manual
    for more details.  */

static unsigned int
cs_etm_reconstruct_cpsr_iset_state (const struct btrace_insn *insn)
{
   switch (insn->flags & BTRACE_INSN_FLAG_ISA_MASK)
     {
     case BTRACE_INSN_FLAG_ISA_ARM:
       /* ARM state: J and T bits are not set.  */
       return 0;

     case BTRACE_INSN_FLAG_ISA_THUMB2:
       /* THUMB state: J bit is not set, T bit is set.  */
       return 0x20;

     case BTRACE_INSN_FLAG_ISA_TEE:
       /* THUMB EE state: J and T bits are set.  */
       return 0x1000020;

     case BTRACE_INSN_FLAG_ISA_JAZELLE:
       /* JAZELLE state: J bit is set, T bit is not set.  */
       return 0x1000000;

     default:
       /* Default is ARM mode.  */
       return 0;
     }
}


>
>> +    default:
>> +      return 0;
>
> What does 0 mean in this context? Should we error/assert if this isn't 
> valid?
>
[Zied] falling back to ARM ISA
>
>> +    }
>> +}
>> +
>>   /* 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
>
[Zied] done.
>
>> +      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
[Zied] done
>
>
>> +      const struct target_desc *tdesc;
>> +      tdesc = gdbarch_target_desc (gdbarch);
>
> const struct target_desc *tdesc = gdbarch_target_desc (gdbarch);
>
[Zied] done
>
>> +
>> +      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);
>
[Zied] done
>
>> +
>> +        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)" 

[Zied] member renamed to cpu_count

comment changed to  /* Count of the CPUs (trace sources).  */

>
>
>> +  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
>
[Zied] done.
>
>> +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.
>
[Zied] I was not able to spot it.
>> +  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:
>>
>