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

Message ID 20210422140921.175221-4-zied.guermazi@trande.de
State New
Headers show
Series
  • extend branch tracing to use ARM CoreSight traces
Related show

Commit Message

Zied Guermazi April 22, 2021, 2:09 p.m.
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_iset_state): 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/arch/arm.h              |  33 ++
 gdb/btrace.c                | 607 ++++++++++++++++++++++++++++++++++++
 gdb/btrace.h                |  16 +-
 gdb/record-btrace.c         |  58 +++-
 gdbsupport/btrace-common.cc |  37 +++
 gdbsupport/btrace-common.h  |  93 ++++++
 6 files changed, 839 insertions(+), 5 deletions(-)

-- 
2.25.1

Comments

Ulrich Weigand via Gdb-patches April 27, 2021, 5:31 p.m. | #1
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 ||.


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


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


>@@ -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'?

>+   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?

>+

>+    default:

>+      return BTRACE_INSN_OTHER;

>+


We don't need an empty line, here.

>+    }

>+}

>+

>+/* 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.

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

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


>+  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?


>+/* 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.

Would this guarantee that we resume from the same PC where we stopped
previously if the reason is TRACE_ON_NORMAL?

>+

>+/* 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.


>+

>+    /* 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.


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

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

>+    {

>+      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?

>+    }

>+

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

>+    }

>+

>+  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?

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

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

>+	  return nullptr;

>+	}

>+

>+    }

>+  decoder->t_info = tp;


I'd put the empty line after the }, not before.

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

>+

>+  result = (int) reqBytes;

>+  try

>+  {




>+/* Process an etm traces data block.  */


Please describe the return value.

>+

>+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?

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.

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

>+	{

>+	  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 ,

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

>+				      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?

>+

>+  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?

>+    }

>+

>+  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 }.


> /* 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.

>+      if (regno == pcreg)


That doesn't handle the -1 case for fetching all registers.

>+	{

> 	  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?

>+	    {

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

>+/* 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?


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
Zied Guermazi April 30, 2021, 1:42 a.m. | #2
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

>
Ulrich Weigand via Gdb-patches April 30, 2021, 8:33 a.m. | #3
Hello Zied,

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


We should add a gap whenever the trace is not contiguous.


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


We just say that things don't work.  We don't say what the actual
error was.  Even if there is no string representation, having an error
number would already help, e.g. in web searches.  Or you could look
at the decoder sources.


> +  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


My point was that those errors should not be debug messages but instead
make it to the user with sufficient detail, even if it is just an integer error code.


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


We insert a gap for OCSD_RESP_FATAL_INVALID_DATA.  Should we
insert gaps for other error codes, too?  Or maybe check the PC and
insert a gap when the trace is not contiguous?


> -      /* 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)?


I see the point in first checking the register number for performance
reasons.  I was wondering why we want two levels of checks.

How about something like this?

If ((regno < 0) || (regno == pcreg))
  {
    <handle pcreg>
  }
If ((regno < 0) || (regno == other_reg))
  {
    <handle other_reg>
  }
...


> +	  /*  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?


That's what I meant.  Shouldn't we check for aarch64, too?
Or is this really only relevant for 'arm'?


> +/* 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.


I don't think we should declare things in GDB that are
declared already somewhere else and maintained in that
other location.  It's too easy to get out of sync.

If we convert them later, can we already use the final
structures, here?


> +  /* 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.


This should be documented, here.  It's a bit odd why a value
in the middle of the range is reserved as invalid.  But it's really
that only 128 values are actually valid.

We may also want to change the type.  Or, if we anticipate this
limit to grow, leave a bigger type and document allowed values.

If we're free to choose the 'invalid' value, I'd rather pick one at
the edge of the range.

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
Zied Guermazi May 7, 2021, 1:52 a.m. | #4
hi Markus,

thanks for your support. the patch was reworked with your imput. below 
is the status

On 30.04.21 10:33, Metzger, Markus T wrote:
> Hello Zied,

>

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

> We should add a gap whenever the trace is not contiguous.

>

>

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

> We just say that things don't work.  We don't say what the actual

> error was.  Even if there is no string representation, having an error

> number would already help, e.g. in web searches.  Or you could look

> at the decoder sources.

[Zied] error code is now reported in the message. e.g warning 
(_("ocsd_dt_create_decoder failed with error: %d"), errcode);
>> +  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

> My point was that those errors should not be debug messages but instead

> make it to the user with sufficient detail, even if it is just an integer error code.

[Zied] idem
>> +      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.

> We insert a gap for OCSD_RESP_FATAL_INVALID_DATA.  Should we

> insert gaps for other error codes, too?  Or maybe check the PC and

> insert a gap when the trace is not contiguous?


[Zied] this is the list of error codes of data path

typedef enum _ocsd_datapath_resp_t {
     OCSD_RESP_CONT,                /**< Continue processing */
     OCSD_RESP_WARN_CONT,           /**< Continue processing  : a 
component logged a warning. */
     OCSD_RESP_ERR_CONT,            /**< Continue processing  : a 
component logged an error.*/
     OCSD_RESP_WAIT,                /**< Pause processing */
     OCSD_RESP_WARN_WAIT,           /**< Pause processing : a component 
logged a warning. */
     OCSD_RESP_ERR_WAIT,            /**< Pause processing : a component 
logged an error. */
     OCSD_RESP_FATAL_NOT_INIT,      /**< Processing Fatal Error :  
component unintialised. */
     OCSD_RESP_FATAL_INVALID_OP,    /**< Processing Fatal Error :  
invalid data path operation. */
     OCSD_RESP_FATAL_INVALID_PARAM, /**< Processing Fatal Error :  
invalid parameter in datapath call. */
     OCSD_RESP_FATAL_INVALID_DATA,  /**< Processing Fatal Error :  
invalid trace data */
     OCSD_RESP_FATAL_SYS_ERR,       /**< Processing Fatal Error :  
internal system error. */
} ocsd_datapath_resp_t;

OCSD_RESP_FATAL_INVALID_DATA is the one related to invalid/corrupted 
data, I will check if  OCSD_RESP_FATAL_SYS_ERR can also point to a gap

>> -      /* 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)?

> I see the point in first checking the register number for performance

> reasons.  I was wondering why we want two levels of checks.

>

> How about something like this?

>

> If ((regno < 0) || (regno == pcreg))

>    {

>      <handle pcreg>

>    }

> If ((regno < 0) || (regno == other_reg))

>    {

>      <handle other_reg>

>    }

> ...

[Zied] Changed to use this pattern.
>> +	  /*  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?

> That's what I meant.  Shouldn't we check for aarch64, too?

> Or is this really only relevant for 'arm'?

[Zied]: it is only relevant for arm. the debug IP was improved in armv8 
(aarch64)
>> +/* 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.

> I don't think we should declare things in GDB that are

> declared already somewhere else and maintained in that

> other location.  It's too easy to get out of sync.

>

> If we convert them later, can we already use the final

> structures, here?

[Zied] those structure has more members than the one required by 
opencsd, designed for usage with an RTOS instead of linux.
>> +  /* 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.

> This should be documented, here.  It's a bit odd why a value

> in the middle of the range is reserved as invalid.  But it's really

> that only 128 values are actually valid.

>

> We may also want to change the type.  Or, if we anticipate this

> limit to grow, leave a bigger type and document allowed values.

>

> If we're free to choose the 'invalid' value, I'd rather pick one at

> the edge of the range.

[Zied] type changed to uint8_t instead of int.
> 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

-- 

*Zied Guermazi*
founder

Trande UG
Leuschnerstraße 2
69469 Weinheim/Germany

Mobile: +491722645127
mailto:zied.guermazi@trande.de <mailto:zied.guermazi@trande.de>

*Trande UG*
Leuschnerstraße 2, D-69469 Weinheim; Telefon: +491722645127
Sitz der Gesellschaft: Weinheim- Registergericht: AG Mannheim HRB 736209 
- Geschäftsführung: Zied Guermazi

*Confidentiality Note*
This message is intended only for the use of the named recipient(s) and 
may contain confidential and/or privileged information. If you are not 
the intended recipient, please contact the sender and delete the 
message. Any unauthorized use of the information contained in this 
message is prohibited.

Patch

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))
+
+/* Exception numbers as encoded in ETMv3.4 for non ARMv7-M processors.
+   See IHI0014Q_etm_architecture_spec table 7.12.  */
+#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..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"
 
 /* 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,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
+   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 ();
+  /* 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) */
+
 /* Classify the instruction at PC.  */
 
 static enum btrace_insn_class
@@ -1502,6 +1538,560 @@  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.  */
+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;
+
+  /* thread_info of traced thread.  */
+  struct thread_info *t_info;
+
+  /* Returned value of previously processed block.  */
+  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 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."));
+    }
+}
+
+/* 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;
+
+    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 = (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;
+  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 = (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;
+  /* 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 = (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);
+}
+
+/* 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;
+
+    /* 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;
+
+    }
+  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;
+  xfree (decoder);
+}
+
+/* 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,
+		      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 < 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);
+	  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);
+	  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;
+}
+
+/* Print all functions in a btrace.  */
+
+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 = &tp->btrace;
+  if (btinfo->functions.empty ())
+    btinfo->level = 0;
+
+  struct cs_etm_decoder *decoder
+   = cs_etm_alloc_decoder (tp,btrace->config.cpu_count,
+			    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,
+				      btrace_etm_readmem_callback);
+  if (ocsd_error != OCSD_OK)
+    error (_("Failed to add CoreSight Trace decoder memory access callback."));
+
+  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);
+    }
+
+  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)
+{
+  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 +2121,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 +2193,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."));
+#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 +2389,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;
     }
 
   internal_error (__FILE__, __LINE__, _("Unknown branch trace format."));
@@ -3413,6 +4015,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 83768694193..bd32a03fcfa 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,38 @@  record_btrace_target::remove_breakpoint (struct gdbarch *gdbarch,
   return ret;
 }
 
+/* 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;
+    }
+}
+
 /* 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;
 
       insn = btrace_insn_get (replay);
-      gdb_assert (insn != NULL);
+      gdb_assert (insn != nullptr);
 
+      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.  */
+	  const struct target_desc *tdesc = gdbarch_target_desc (gdbarch);
+
+	  const char *tdesc_name = tdesc_architecture_name (tdesc);
+	  if (strcmp (tdesc_name, "arm") == 0)
+	    {
+	      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.cc b/gdbsupport/btrace-common.cc
index 79ff21de44b..146b49b6732 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,36 @@  btrace_data_append (struct btrace_data *dst,
 	  }
 	}
       return 0;
+
+    case BTRACE_FORMAT_ETM:
+      switch (dst->format)
+	{
+	default:
+	  return -1;
+
+	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:
+	  {
+	    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..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
+{
+  /* 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
+{
+  /* 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;
+};
+
+/* 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;
+};
+
 /* 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: