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

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

Hello Markus,

thanks for your feedback. Below are the answers to your queries.


On 22.06.21 16:59, 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.
> Looks good, overall.  Comments are mostly about simplifications.
> Shall we discuss them in this email thread before going to v7?
>
>
>> diff --git a/gdb/btrace.c b/gdb/btrace.c
>> index 5e689c11d4b..2676389b63e 100644
>> --- a/gdb/btrace.c
>> +++ b/gdb/btrace.c
>> @@ -671,6 +672,38 @@ 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 caller must ensure
>> +   that the instruction produces the same chaining.
>> +   An example of good case, is when the same removed instruction
>> +   is added later.  */
> This function is called in one place and the way I understood it, we remove
> an undefined instruction that was used as a breakpoint.  I assume the behavior
> is that this undefined instruction is counted as executed in the trace and we
> want to fix that up.
>
> We don't add that instruction back, though, do we?
>
> And if we wanted to support that remove-then-re-insert use-case,
> wouldn't we want to return the removed instruction?
>
> I'd rather we document exactly that one use-case in which we need
> this functionality.  This allows documenting exactly what we're doing
> and why this is necessary.

[Zied] yes, the code is patched with an undefined instruction that rises 
an exception to trigger the breakpoint.

the traces decoder gets the attempt to execute the instruction in the 
trace and the exception, but decodes it as the original instruction, 
since the code was 'unpatched' in between.

we do not add the instruction back manually. but when the execution 
continue, and depending on the user action, the same instruction may be 
re-executed. we do not re-insert it manually, the decoder will decode it 
from the traces. therefore we can ignore the removed instruction.

following comment is added in cs_etm_update_btrace_with_exception

"Handle the implementation of breakpoints in gdb for arm (v7) architecture
      using undefined instructions.
      In arm (v7), breakpoints are implemented by patching the code with an
      undefined instruction. When the breakpoint is hit the code is 
'un-patched'.
      In the traces we see the exception of the undefined instruction
      and we get the original instruction from the decoder.
      When we continue the execution the original instruction will be
      re-executed, and we will get it again in the traces. To solve
      getting the instruction twice, the first occurrence is removed,
      since it was actually not executed."


>
>> +
>> +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;
> Should this be an error?
>
>> +
>> +  struct btrace_function *bfun = &btinfo->functions.back ();
>> +  /* If we had a gap before, we return.  */
>> +  if (bfun->errcode != 0)
>> +    return;
> In which case can we have a gap?
[Zied] this is to make sure that following calls are safe to be executed.
>> +
>> +  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"));
> We just removed an instruction in the then statement, which
> could well result in BFUN->INSN to become empty().
>
> The statement about valid functions above may be a bit too
> generic.  Also, ftrace_new_* () may create empty function
> segments.
>
> Isn't it rather that in our use-case, the caller knows that the
> function segment cannot be empty?
[Zied] yes, we are sure that we have at least the last executed 
instruction that raised the breakpoint execption.
>
>> +    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."));
> This internal error kills GDB.  Should this be a normal error that just
> stops trace processing for this unknown ISA?

[Zied] since I listed all possible values that may get returned by the 
decoder, the default case means that gdb and the decoder are doing 
something wrong and this should be handled as a fatal error.

This code is to be revisited if in the future, ARM adds other ISAs, and 
we would like to support them.

>
>
>> +  insn.iclass = BTRACE_INSN_OTHER;
>> +  insn.pc = elem->st_addr;
>> +  for (int i = 0; i< elem->num_instr_range; i++)
>> +    {
>> +      try
>> +	{
>> +	  insn.size = gdb_insn_length (gdbarch, insn.pc);
>> +	}
>> +      catch (const gdb_exception_error &err)
>> +	{
>> +	  error (_("Failed to get the size of the instruction."));
> Isn't the original exception good enough?
>
>> +	}
>> +
>> +      struct btrace_function *bfun = ftrace_update_function (btinfo, insn.pc);
>> +      if (etm_decoder->arch_version == ARCH_V7)
>> +	insn.flags = cs_etm_get_isa_flag (elem);
> ELEM isn't changing so we could set this once outside of the loop.
[Zied] moved outside
>
>> +
>> +      if (i == elem->num_instr_range -1)
>> +	insn.iclass = cs_etm_get_instruction_class (elem);
>> +
>> +      ftrace_update_insns (bfun, insn);
>> +      insn.pc = insn.pc + insn.size;
>> +    }
>> +}
>> +
>> +/* Update btrace in the case of an exception.  */
>> +
>> +static void
>> +cs_etm_update_btrace_with_exception (const struct cs_etm_decoder
>> *etm_decoder,
>> +				      const ocsd_generic_trace_elem *elem)
>> +{
>> +  gdb_assert (elem->elem_type == OCSD_GEN_TRC_ELEM_EXCEPTION);
>> +
>> +  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);
> Here we just discard the 'breakpoint' instruction, correct?  We don't really plan
> to re-insert it again.
[Zied] we will not insert it manually. it will be inserted as part of 
the executed code when the user continues the execution
>
> Can we ensure that we actually inserted that instruction into the current BFUN?
> We wouldn't need the helper function and all those checks would become asserts.
>
> I like little helper functions but if we just removed the insn here it would be more
> obvious why we're doing it.
>
>> +	}
>> +    }
>> +}
>> +
>> +/* Update btrace in the case of a trace on.  */
>> +
>> +static void
>> +cs_etm_update_btrace_with_trace_on (const struct cs_etm_decoder
>> *etm_decoder,
>> +				     const ocsd_generic_trace_elem *elem)
>> +{
>> +  gdb_assert (elem->elem_type == OCSD_GEN_TRC_ELEM_TRACE_ON);
>> +
>> +  if (elem->trace_on_reason != TRACE_ON_NORMAL)
>> +    {
>> +      struct thread_info *tp = etm_decoder->t_info;
>> +      struct btrace_thread_info *btinfo = &tp->btrace;
>> +      ftrace_new_gap (btinfo, elem->trace_on_reason, etm_decoder->gaps);
>> +    }
> Even for normal trace off/on pairs, we'd want a gap if PC is moving.
[Zied] yes, but I do not have here a mean for checking if the PC has 
moved. The pc field is not valid in this kind of elements
>
>
>> +}
>> +
>> +/* 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)
> sizeof (str_buffer)
[Zied] done.
>
>> +/* Callback to print error log.  */
>> +
>> +static void cs_etm_print_error_log (const void *p_context, const char
>> *psz_msg_str, const int str_len)
>> +{
>> +    char string_buffer[128];
>> +    memcpy (string_buffer, psz_msg_str, std::min(str_len, 127));
>> +    if (str_len >127)
>> +      string_buffer[127] = 0;
>> +    else
>> +      string_buffer[str_len] = 0;
> We can simply adjust str_len before the memcpy.  There's no reason to declare
> it const; it is passed by-value.
>
>
>> +  uint8_t csid;
>> +  errcode = ocsd_dt_create_decoder (decoder->dcd_tree, decoder_name,
>> +			      OCSD_CREATE_FLG_FULL_DECODER,
>> +			      trace_config, &csid);
>> +  if (errcode != OCSD_OK)
>> +    {
>> +      warning (_("ocsd_dt_create_decoder failed with error: %d"), errcode);
>> +      return errcode;
>> +    }
>> +
>> +  errcode = ocsd_dt_set_gen_elem_outfn (decoder->dcd_tree,
>> +					 cs_etm_trace_element_callback,
>> +					 decoder);
>> +  if (errcode != OCSD_OK)
>> +    {
>> +      warning (_("ocsd_dt_set_gen_elem_outfn failed failed with error: %d"),
>> +		   errcode);
>> +      return errcode;
>> +    }
>> +
>> +  errcode = ocsd_def_errlog_init (OCSD_ERR_SEV_ERROR, 1);
>> +  if (errcode != OCSD_OK)
>> +    {
>> +      warning (_("ocsd_def_errlog_init failed failed with error: %d"),
>> +		   errcode);
>> +      return errcode;
>> +    }
>> +
>> +  /* Initialize error printer.  */
>> +  errcode = ocsd_def_errlog_config_output (C_API_MSGLOGOUT_FLG_NONE,
>> nullptr);
>> +  if (errcode != OCSD_OK)
>> +    {
>> +      warning (_("ocsd_def_errlog_init failed failed with error: %d"),
>> +		   errcode);
>> +      return errcode;
>> +    }
>> +
>> +  errcode = ocsd_def_errlog_set_strprint_cb (decoder->dcd_tree, nullptr,
>> +					      cs_etm_print_error_log);
>> +  if (errcode != OCSD_OK)
>> +    {
>> +      warning (_("ocsd_def_errlog_set_strprint_cb failed failed with error: %d"),
>> +		   errcode);
>> +      return errcode;
>> +    }
>> +
>> +  decoder->prev_return = OCSD_RESP_CONT;
> There are several error returns that leave this field uninitialized.  I assume that
> the decoder is not working in those cases and will not be used.  Don't we need to
> undo anything before returning with an error?

[Zied] all those functions are setting some fields in the decoder. If 
something goes wrong the caller frees the decoder.


>
> I see that we call cs_etm_free_decoder () below on errors, which probably takes
> care of cleaning up partially complete decoders.  It still feels odd to return an error
> and leave a decoder partially initialized.
>
> Let's at least note that in the comment that on error, the caller is expected to
> call cs_etm_free_decoder ().
[Zied] Documented in the comment.
>
>
>> +  struct cs_etm_decoder *decoder;
>> +
>> +  decoder = (struct cs_etm_decoder*) xmalloc (sizeof (struct cs_etm_decoder));
>> +  decoder->dcd_tree = dcdtree_handle;
>> +
>> +  for (int i = 0; i < cpu_count; i++)
>> +    {
>> +      ocsd_err_t errcode = cs_etm_create_decoder (&(t_params->at (i)), decoder);
>> +      if (errcode != OCSD_OK)
>> +	{
>> +	  cs_etm_free_decoder (decoder);
>> +	  return nullptr;
>> +	}
>> +    }
>
>> +/* Process an etm traces data block.
>> +   In case of an error it resets the decoder and tries to process further.  */
>> +
>> +static void
>> +cs_etm_process_data_block (struct btrace_thread_info *btinfo,
>> +			   struct cs_etm_decoder *decoder,
>> +			   uint64_t index, const uint8_t *buf,
>> +			   size_t len, size_t *consumed)
>> +{
>> +  ocsd_datapath_resp_t data_path_return = OCSD_RESP_CONT;
>> +  size_t processed = 0;
>> +  uint32_t count;
>> +
>> +  while (processed < len)
>> +    {
>> +      if (OCSD_DATA_RESP_IS_CONT (data_path_return))
>> +	{
>> +	  data_path_return = ocsd_dt_process_data (decoder->dcd_tree,
>> +					OCSD_OP_DATA,
>> +					index + processed, len - processed,
>> +					&buf[processed], &count);
>> +	  processed += count;
>> +
>> +	}
> There seems to be an extra empty line.
>
> Should we assert COUNT > 0 || ! OCSD_DATA_RESP_IS_CONT (data_path_return)
> to ensure forward progress?
>
>> +      else if (OCSD_DATA_RESP_IS_WAIT (data_path_return))
>> +	{
>> +	  data_path_return = ocsd_dt_process_data (decoder->dcd_tree,
>> +					OCSD_OP_FLUSH,
>> +					0, 0, nullptr, nullptr);
> Same here for ! OCSD_DATA_RESP_IS_WAIT (data_path_return), although we
> still wouldn't be able to detect switching back and forth between those two.
>
>> +	}
>> +      else
>> +	{
>> +	  warning (_("error %d in ocsd_dt_process_data after processing %zu"),
>> +		      data_path_return, processed);
>> +	  ftrace_new_gap (btinfo, data_path_return, decoder->gaps);
>> +	  data_path_return = ocsd_dt_process_data (decoder->dcd_tree,
>> +					OCSD_OP_RESET,
>> +					0, 0, nullptr, nullptr);
>> +	  //todo: shall we increase processed? by 1, or 4 do we need to manually
>> align to 16 bytes?
> Please use /* */ for comments.  Most people use FIXME instead of todo.
>
> We may end up in an infinite loop if trying to reset the decoder triggers another
> error indefinitely.
>
> We should still be able to interrupt GDB itself using ^C on the CLI if we ever actually
> run into an infinite decode loop.  So maybe this is all overkill.
>
[Zied] after discussion with OpenCSD engineer. the current 
implementation is fine and the comment can be removed
>
>> +  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."));
> Could we print ICSD_ERROR in the error message?  Even printing it as an int would help.
[Zied] done
>
>
>> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
>> index 16ffb76272b..57b9c8ec487 100644
>> --- a/gdb/record-btrace.c
>> +++ b/gdb/record-btrace.c
>> @@ -1557,6 +1558,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.  */
> So we do not need to store CPSR after all?  We can infer the relevant bits
> from ISA information we get from the trace?  Nice.
[Zied]  few bits  in the insn->flags are used for this purpose, no 
dedicated field for the cspr is used nymore.
>
>> +
>> +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;
> How can we run into this default case?  Should this be an error
> in case we add new ISA modes in the future?
[Zied] this should not happens. changed to  internal_error
>
>> +    }
>> +}
>> +
>> /* The fetch_registers method of target record-btrace.  */
>>
>> void
>> @@ -1581,16 +1614,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.  */
> Let's extend the comment to say that CPSR is ARM and that we're checking
> the architecture below to avoid redundant checks when we want to fetch
> other registers.

[Zied] done, comment changed to

"      /* We can only provide the PC or PS registers here.
          PS is provided in the case of an ARMv7 target and it maps to CSPR.
          A check for the architecture is done below before providing it */

"

>
>> +      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 < 0) || (regno == pcreg))
>> +	{
>> 	  regcache->raw_supply (regno, &insn->pc);
>> 	}
>> +      if ((regno < 0) || (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);
> Please initialize in the declaration.
[Zied] done
>
>> +	      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..ee05ecb8b10 100644
>> --- a/gdbsupport/btrace-common.h
>> +++ b/gdbsupport/btrace-common.h
>
>> +/* Parameters of trace source.  */
>> +struct cs_etm_trace_params
>> +{
>> +  /* Architecture version of trace source.  */
>> +  int arch_ver;
>> +  /* Core profile of the trace source.  */
>> +  int core_profile;
>> +  /* Traces protocol.  */
>> +  int protocol;
>> +  union {
>> +    struct cs_etmv3_trace_params etmv3;
>> +    struct cs_etmv4_trace_params etmv4;
>> +  };
>> +};
> Please add empty lines between members.
[Zied] done
>
>> +
>> +/* Configuration information to go with the etm trace data.  */
>> +struct btrace_data_etm_config
>> +{
>> +  /* Count of the CPUs (trace sources).  */
>> +  int    cpu_count;
>> +  /* List of traces sources parameters.  */
>> +  std::vector<struct cs_etm_trace_params> *etm_trace_params;
>> +  /* Trace sink parameters.  */
>> +  struct cs_etm_decoder_params etm_decoder_params;
>> +};
> Also here.
>
>> +
>> +/* Branch trace in ARM Processor Trace format.  */
>> +struct btrace_data_etm
> Is that the correct term?

[Zied] done. changed to

"/* Branch trace in ARM Processor Extended Trace Macrocell format.  */"

>
>
> 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
>
Kind Regards
Zied Guermazi
-- 

*Zied Guermazi*
founder

Trande GmbH
Leuschnerstraße 2
69469 Weinheim/Germany

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

*Trande GmbH*
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.