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

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

hi Markus,

thanks for your review, here is the status of the rework as it will be 
published in next version of the patchset.


On 27.04.21 19:31, Metzger, Markus T wrote:
> Hello Zied,
>
>> This patch extend branch tracing by adding the functions needed
>> to collect parameters for decoding ETM traces and decoding them.
>> diff --git a/gdb/arch/arm.h b/gdb/arch/arm.h
>> index fa589fd0582..2b5d9f2c9dc 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.  */
>> +#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))
> Space between macro name and (.  Also spaces around ||.
[Zied] fixed
>
>
>> diff --git a/gdb/btrace.c b/gdb/btrace.c
>> index c697f37f46c..d14ccd765b2 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"
> GDB includes are in the first block.
[Zied] moved to the first block
>
>
>> @@ -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");
> There was no debug log on purpose as this function is typically called from
> other ftrace_new_* functions.  It is only called directly at the beginning and
> after a gap.
[Zied] removed
>
>
>> @@ -671,6 +674,39 @@ 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 does not undo functions chaining.
>> +   When adding an instruction after using it, the user must ensure
> Nit: should this be 'caller' instead of 'user'?
[Zied] changed
>
>> +   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;
>> +  bfun = &btinfo->functions.back ();
> Couldn't we simply declare BFUN as reference?
>
>
>> +  /* 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 instruction.  */
>> +      internal_error (__FILE__, __LINE__,
>> +		       _("Attempt to remove last instruction"
>> +			 "from an empty function"));
>> +    }
>> +}
>> +#endif /* defined (HAVE_LIBOPENCSD_C_API) */
>> @@ -1502,6 +1538,560 @@ btrace_compute_ftrace_pt (struct thread_info *tp,
>> +/* Set the instruction flag to track ISA mode of ARM processor.  */
>> +
>> +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:
>> +      internal_error (__FILE__, __LINE__,
>> +		       _("Undefined elem->isa value returned by OpenCsd."));
>> +    }
> Nit: if you put the internal_error outside of the switch, the compiler
> will complain if the enum gets extended.
>
>
>> +/* Returns the instruction class.  */
>> +
>> +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;
> Why not merge this with the default below?
[Zied] changed with a fall through to default.
>
>> +
>> +    default:
>> +      return BTRACE_INSN_OTHER;
>> +
> We don't need an empty line, here.
[Zied] empty lines removed.
>
>> +    }
>> +}
>> +
>> +/* 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 = (struct cs_etm_decoder *) context;
>> +  if (etm_decoder->t_info == nullptr)
>> +    return;
>> +
>> +  struct thread_info *tp = etm_decoder->t_info;
>> +
>> +  struct btrace_thread_info *btinfo = &tp->btrace;
>> +
>> +  struct gdbarch *gdbarch = target_gdbarch ();
>> +
>> +  struct btrace_insn insn;
>> +  CORE_ADDR pc = elem->st_addr;
>> +  int size;
> The distribution of empty lines between declarations is a bit unclear.
[Zied] empty lines removed.
>
>> +  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);
> You may initialize BFUN as part of the declaration.
[Zied] done.
>
>> +      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;
> There doesn't seem to be a need for a separate PC variable.
> We can work directly on INSN and initialize INSN.ICLASS outside
> of the loop as it won't change until the last iteration.  Same
> for SIZE.

[Zied] done

>
>
>> +  struct thread_info *tp = etm_decoder->t_info;
>> +
>> +  struct btrace_thread_info *btinfo = &tp->btrace;
>> +  /* Handle the implementation of breakpoints in gdb for arm (v7) architecture
>> +     using undefined instructions.  */
>> +
>> +  if (etm_decoder->arch_version == ARCH_V7)
> I'd add the empty line before the comment.  The comment refers to the if, correct?
[Zied] done.
>
>
>> +/* 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 = (struct cs_etm_decoder *)context;
>> +  if (etm_decoder->t_info == nullptr)
>> +    return;
>> +
>> +  struct thread_info *tp = etm_decoder->t_info;
>> +
>> +  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);
>> +}
> Same comment about empty lines between declarations.  Also, we don't really
> need TP or BTINFO until we decided to add a gap.
[Zied] declaration move within the if block.
>
> Would this guarantee that we resume from the same PC where we stopped
> previously if the reason is TRACE_ON_NORMAL?
[Zied] this is not guaranteed, we may land at a different PC. The 
landing address will be controlled by the filtering and triggering 
mechanism in ETM block. the user can trigger stopping the traces at a 
certain address and resuming it on another address.
>
>> +
>> +/* 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 index,
>> +				const uint8_t trace_chan_id,
>> +				const ocsd_generic_trace_elem *elem)
>> +{
>> +  if (record_debug != 0)
>> +    {
>> +      char str_buffer[128];
>> +      if (ocsd_gen_elem_str (elem, str_buffer, 128) == OCSD_OK)
>> +	DEBUG ("ETM trace_element: index= %s, channel= 0x%x, %s",
>> +	       pulongest (index), trace_chan_id, str_buffer);
>> +    }
>> +
>> +  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;
> It may make sense to cast CONTEXT here so we have proper types
> in all the other functions we're calling from here.
[Zied] yes, the cast and the check for the presence of t_info are moved 
here.
>
>
>> +
>> +    /* Not yet handled types, but may be supported in the future.  */
>> +    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;
>> +
>> +    }
> No need for an empty line here.  I also don't see a reason for listing all
> the unsupported types as long as we have a default.  If it's an enum, you
> might want to remove the default to get compiler warnings when the
> enum changes.
[Zied] many of them will get implemented either when supporting other 
variants of arm processors (cortex M)or adding other features (cycle 
accurate). I prefer keeping them visible at this point.
>
>
>> +  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;
> The naming seems inconsistent.  One is with TRACE_ prefix, the other isn't.
[Zied] aligned, trace prefix removed
>
>> +  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))
> Please add explicit comparisons against zero - unless this function is returning
> bool, of course.
[Zied] explicit comparison added
>
>> +    {
>> +      DEBUG ("ocsd_dt_create_decoder failed");
>> +      return false;
> Is there some error code/message that would help the user understand why we
> failed to create a decoder?
[Zied] The OpenCsd library is not offering such a helper function, so 
the error to string conversion is done in btrace.c . the issue is 
reported to opencsd developer.
>
>> +    }
>> +
>> +  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;
> Same here.
[Zied] done.
>
>> +    }
>> +
>> +  decoder->prev_return = OCSD_RESP_CONT;
>> +  return true;
>> +}
>
>> +/* Allocate a cs_etm_decoder and initialize it.  */
>> +
>> +static struct cs_etm_decoder *
>> +cs_etm_alloc_decoder (struct thread_info *tp, int cpu_count,
>> +		      struct cs_etm_decoder_params d_params,
> Do we really want to pass the struct by-value?
[Zied] no, it is not really needed. changed accordingly
>
>> +		      std::vector<cs_etm_trace_params> * t_params)
>> +{
>> +  ocsd_dcd_tree_src_t src_type = OCSD_TRC_SRC_SINGLE;
>> +  uint32_t deformatterCfgFlags = 0;
>
>> +  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;
> There is no need to declare this outside of the loop.  We're only using it inside.
> Is there a better name, maybe?  'if (!ret)' doesn't really describe what happens.

[Zied] moved inside the loop and renamed to errcode

cs_etm_create_decoder modified to return ocsd_err_t instead of boolean

>
>> +  for (int i = 0; i < cpu_count; i++)
>> +    {
>> +      ret = cs_etm_create_decoder (&(t_params->at (i)), decoder);
>> +      if (!ret)
>> +	{
>> +	  DEBUG ("cs_etm_create_decoder failed");
>> +	  cs_etm_free_decoder (decoder);
> We put all the error details in debug messages.  This would be relevant for the
> user, too.  And if we could get some error code/message from the library that
> would help, as well.
[Zied] cs_etm_create_decoder is reporting all encountered errors through 
debug messages
>
>> +	  return nullptr;
>> +	}
>> +
>> +    }
>> +  decoder->t_info = tp;
> I'd put the empty line after the }, not before.
[Zied] done.
>
>> +  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;
> Please declare on first use.
[Zied] done;
>
>> +
>> +  result = (int) reqBytes;
>> +  try
>> +  {
>
>
>> +/* Process an etm traces data block.  */
> Please describe the return value.
[Zied] done
>
>> +
>> +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;
> What does CUR mean?
[Zied] see below
>
> After reading the code is looks like it means the current decoder's return value.
> It also looks like we don't need two variables.  The only case where this makes
> a difference is if len == 0 and decoder->prev_return != OCSD_RESP_CONT.  And
> the error case.

[Zied] let me put few comments here to remove the confusion that can be 
brought by this implementation.

the function is intended to be called multiple times, so it stores its 
last status in decoder->prev_return. The advantage is not obvious when 
we consider only decoding a buffer of traces from memory. But when we 
consider processing saved traces (e.g. in a perf file, not yet 
implemented), we can have multiple chunk of traces ( and it is the case 
in a perf file), in this case we will need both variables.

I rewritten it to not handle the case where it is called multiple times. 
it will then look more comprehensive. I have also changed the signature 
to return a void, and to attempt to recover if errors are deteced while 
processing traces.

>
>> +  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))
> The else goes onto the next line.
[Zied] done
>
>> +	{
>> +	  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);
>> +	  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;
>> +}
>
>> +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 = &tp->btrace;
> I assume this won't change.  We might declare a reference, instead.
>
>> +  if (btinfo->functions.empty ())
>> +    btinfo->level = 0;
>> +
>> +  struct cs_etm_decoder *decoder
>> +   = cs_etm_alloc_decoder (tp,btrace->config.cpu_count,
> Space after ,
[Zied] done.
>
>> +			    btrace->config.etm_decoder_params,
>> +			    btrace->config.etm_trace_params);
>> +  if (decoder == nullptr)
>> +    error (_("Failed to allocate ARM CoreSight ETM Trace decoder."));
>> +
>> +  ocsd_err_t ocsd_error
>> +   = cs_etm_add_mem_access_callback (decoder,
>> +				      (CORE_ADDR)0x0L, (CORE_ADDR) -1L,
> Space after ) in the cast.
[Zied] done.
>
>> +				      btrace_etm_readmem_callback);
>> +  if (ocsd_error != OCSD_OK)
>> +    error (_("Failed to add CoreSight Trace decoder memory access callback."));
> I see we're just delaying the error () until here.  Is there a reason we're not
> throwing right away and instead return and then throw in the caller?
>
> Would it make sense to include the error code in the error message to give more
> information to the user?

[Zied] yes, I was delaying the error especially in 
cs_etm_create_decoder. The reason is to free the decoder, in the case of 
an error, at the same location where it was created. it is done so for 
readability and maintainability reasons.

Warnings are issued when the error gets detected, error is raised when 
the function cs_etm_alloc_decoder fails and returns with nullptr.

as mentioned above, error codes explanation is reported in the warnings. 
There is a shortage in the api for converting the errors to strings. I 
reported this to the developer of opencsd. in the mean time I added the 
description strings in the btrace.c files, this is to be removed once 
the api gets extended.

>
>> +
>> +  size_t consumed;
>> +  int errcode
>> +   = cs_etm_process_data_block (decoder, 0, btrace->data, btrace->size,
>> +				 &consumed);
>> +  if (errcode != 0)
>> +    {
>> +      warning (_("Error 0x%" PRIx32 " in decoding ARM CoreSight ETM Traces"
>> +      		" at offset %zu."), errcode, consumed);
>> +      if (errcode == OCSD_RESP_FATAL_INVALID_DATA)
>> +	ftrace_new_gap (btinfo, errcode, decoder->gaps);
> A gap is used to indicate that the trace it not contiguous.  How about the other
> error codes?

[Zied] OpenCsd offers an interface to register a call back for reporting 
error logs during data processing. I added it.

There is a shortage in the api in reporting decoding errors 
programmatically, to handle it properly and possibly recovering from it. 
I reported this to the developer of opencsd.

this function was reworked to attempt to recover in case of failure.

>
>> +    }
>> +
>> +  ftrace_compute_global_level_offset (btinfo);
>> +  btrace_add_pc (tp);
>> +  btrace_print_all (btinfo);
>> +  cs_etm_free_decoder (decoder);
>> +
>> +}
> No need for an empty line before a }.
[Zied] done.
>
>
>> /* The fetch_registers method of target record-btrace.  */
>>
>> void
>> @@ -1580,15 +1613,32 @@ record_btrace_target::fetch_registers (struct
>> regcache *regcache, int regno)
>>        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.  */
>> +      if (regno >= 0 && !(regno == pcreg || regno == ARM_PS_REGNUM))
>> 	return;
> This is generic code.  ARM_PS_REGNUM may mean something different
> for other architectures.
>
> I see how we handle it below but it is still confusing at this point.  Do we
> even need these two levels of checks?  We're repeating the check again
> below since we need to do different things.
>
> It would be cleaner to first check the architecture but I see how this is
> more efficient.  Let's keep it that way until we need to handle more
> registers or more architectures.

[Zied] this piece of code is called often, so I optimized it to return 
as soon as possible. This will save checking the architecture each time 
(and doing a string comparison).

do you know a better way to check the target (intel or arm or aarch64) 
without using the string name (an enumeration for example)?

>
>> +      if (regno == pcreg)
> That doesn't handle the -1 case for fetching all registers.
[Zied] the -1 case (all registers) is handled in a dedicated if.
>
>> +	{
>> 	  regcache->raw_supply (regno, &insn->pc);
>> +	  return;
>> +	}
>> +      if (regno == ARM_PS_REGNUM)
> Same here.
>
>> +	{
>> +	  /*  Provide CPSR register in the case of an armv7 target.  */
>> +	  const struct target_desc *tdesc = gdbarch_target_desc (gdbarch);
>> +
>> +	  const char *tdesc_name = tdesc_architecture_name (tdesc);
>> +	  if (strcmp (tdesc_name, "arm") == 0)
> Is that sufficient?
[Zied] : tdesc_name can be arm, aarch64, etc ...at this point it should 
be enough. Do you have other limitations in mind?
>
>> +	    {
>> +	      int cpsr;
>> +	      cpsr = cs_etm_reconstruct_cpsr_iset_state (insn);
>> +	      regcache->raw_supply (regno, &cpsr);
>> +	    }
>> +	  return;
>> +	}
>>      }
>>    else
>>      this->beneath ()->fetch_registers (regcache, regno);
>
>> diff --git a/gdbsupport/btrace-common.h b/gdbsupport/btrace-common.h
>> index 153b977723a..d431a616a83 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
> Aren't those structures declared in the decoder's header file?
>
> We shouldn't repeat those declarations here.
[Zied] This is an intermediate form close to the parameters as collected 
for the sys file system or the remote protocol fields. They are 
converted later on to the parameters as required by opencsd library.
>
>> +/* Parameters of trace source.  */
>> +struct cs_etm_trace_params
>> +{
> Same here and more below.  If we do need to declare them
> in GDB, they need comments for each field.
>
>> +/* Configuration information to go with the etm trace data.  */
>> +struct btrace_data_etm_config
>> +{
>> +  /* Count of the CPUs (trace sources).  */
>> +  int    cpu_count;
>> +  std::vector<struct cs_etm_trace_params> *etm_trace_params;
>> +  struct cs_etm_decoder_params etm_decoder_params;
>> +};
> Each field needs a comment.  For etm_trace_params, for example,
> it would be interesting whether the vector is per-cpu.  Why would
> we use a pointer to a vector instead of declaring the vector here?
>
>> +
>> +/* Branch trace in ARM Processor Trace format.  */
>> +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.  */
>> +  int trace_id;
> Does that imply that trace_id's are in the range 0..0xff-1?  In that case, uint8_t
> would be more appropriate.
>
> Is that a realistic limit?  How would we handle more threads than we have id's for?

[Zied] on a linux system, trace_id is assigned per cpu. and the kernel 
copies the traces of each thread in a dedicated ring buffer. by this, 
the traces belonging to different threads are de-multiplexed. On an RTOS 
system, especially when routing the traces outside of the SoC, the OS 
has no other mean for de-multiplexing the traces than the trace_id. The 
hardware (ETM IP) reserves 7 bits for the trace_id. The system limit, in 
regards to tracing, is then 111 running threads at the same time (see 
DDI0314H paragraph 9.6 for the limitations),  but this is not a big 
issue, since on those small systems, usually few threads are running.

on linux system, where we would like to ignore the per trace id demux, 
we set the trace_id to a not valid value namely 0xFF.

>
>
> Regards,
> Markus.
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928
>