RE: [PATCH 3/8] start/stop btrace with coresight etm and parse etm buffer. nat independant

["Metzger, Markus T" <markus.t.metzger@intel.com>]

Hello Zied,

> I reworked the patch for C coding style. To my knowledge there is not coding styles issues left.

Thanks a lot.  That should make it easier to review.

 
> For the point concerning the use of a vector of registers in btrace_insn:
> a vector of registers is added to the btrace_insn structure. This adds an overhead of 3 pointers when the vector is empty. It will be the overhead when not used (intel pt). I think this should be acceptable. 

That's doubling the current size; tripling it if we declared ICLASS 8b (as it
should be).  I don't think that's acceptable.


> on ARMv7 this is need for all instructions, to allow step and next to set the breakpoints properly in the code.

Is this to distinguish ARM and Thumb modes?  How would the information
be used?

> on ARMv8, this is currently not used.
> This implementation paves the way to handling ETM traces with data, so that gdb can have access in addition, to the registers altered during an instruction. This is very useful in execution record and replay since we can then reconstruct (some of) the variables.

I see how this can be very useful.  We should definitely support that, but
we need to find a better way of storing the information.


> +                case ocsd_isa_thumb2:
> +                  cpsr = 0x20;
> +                  break;
> +                case ocsd_isa_tee:
> +                  cpsr = 0x1000020;
> +                  break;
> +                case ocsd_isa_jazelle:
> +                  cpsr = 0x1000000;
> +                  break;
> +                default:
> +                  cpsr = 0;
> +              }
> It might make sense to turn this if into a small helper function.
> [Zied] it can be done, I am keeping it as is since there is no reuse.

It reduces the size of the function.  Even if there is no reuse it improves
readability.

 
> +                              ARM_PS_REGNUM, (gdb_byte *)&cpsr);
> +              insn.registers.push_back (reg);
> +            }
> +          if (i == elem->num_instr_range -1)
> +            {
> +              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:
> +                      insn.iclass=BTRACE_INSN_RETURN;
> +                      break;
> +                    case OCSD_S_INSTR_BR_LINK:
> +                      insn.iclass=BTRACE_INSN_CALL;
> +                      break;
> +                    case OCSD_S_INSTR_NONE:
> +                      insn.iclass=BTRACE_INSN_JUMP;
> +                  }
> +                  break;
> +                    case OCSD_INSTR_ISB:
> +                    case OCSD_INSTR_DSB_DMB:
> +                    case OCSD_INSTR_WFI_WFE:
> +                    case OCSD_INSTR_OTHER:
> +                      insn.iclass=BTRACE_INSN_OTHER;
> +                      break;
> +                    default:
> +                      break;
> +              }
> This switch could be a small helper function, too.
> [Zied] it can be done, I am keeping it as is since there is no reuse. 

Same here.  All this text could be just one line.

 
> +            }
> +          ftrace_update_insns (bfun, insn);
> +          pc = pc + size;
> +        }
> +    }
> +}
> +#undef ARM_PS_REGNUM
> +
> +#define ARM_EXCEPTION_UNDEFINED_INSTRUCTION 9
> Should this go into arch/arm.h?
> [Zied] hardware exceptions are not listed in this header file.

If we need to enumerate them somewhere, I think it should be in this file.

 
> +          if (elem->exception_number ==
> ARM_EXCEPTION_UNDEFINED_INSTRUCTION)
> +            {
> +              DEBUG ("handle breakpoints implementation in gdb for armv7");
> +              ftrace_remove_last_insn(btinfo);
> Could we detect this before we added the instruction?
> [Zied] Unfortunately not. On armv7 the processor attempts first to execute (and logs) the instruction, then it raises the exception.

I meant before we added the instruction to the trace.

 
> +    default:
> +      decoder->arch_version = ARCH_UNKNOWN;
> +      return false;
> Wouldn't it make more sense to error () or return an error code?
> There are different things that can go wrong, it seems.
> [Zied] yes, we can add some error code and add some logs according to the error code. Nevertheless, I prefer rather adding logs in the place where it can go wrong, and raise a go/no-go (it is not possible to recover from some errors or handle errors differently) .

I'm fine with issuing warnings right where we detect non-fatal errors and with
error()ing out on fatal errors.

There's not point in a bool return type, though; void would do just fine.

 
> +  decoder->dcd_tree = dcdtree_handle;
> +
> +  for (i = 0; i < num_cpu; i++) {
> +      ret = cs_etm_create_decoder(&(t_params->at(i)),
> +                                  decoder);
> Is this overwriting the same decoder object?
> [Zied] yes and No :). It updates the dcd_tree with a new node. It overwrites arch_version. This is safe, since there is no Linux kernel running on an ARM multi core MPU with cores belonging to different architectures.

But there's nothing preventing that, either, as long as the kernel sticks to some
common base ISA.

It seems cleaner to not partially overwrite things.  And this doesn't seem to be
performance critical.


> +      if (ret == false)
> +        {
> +          ocsd_destroy_dcd_tree(decoder->dcd_tree);
> +          free(decoder);
> Do we need to undo anything for the other cpus from 0 to i-1?
> [Zied] no, decoder->dcd_tree holds the nodes of all previous cpus
> 
> 
> There's a cs_etm_free_decoder function defined below.  Should
> this be called to free the decoder?
> [Zied] yes it can be called instead of this peace of code. this is not really needed.

Let's do that.

  
>    DEBUG ("enable thread %s (%s)", print_thread_id (tp),
>           target_pid_to_str (tp->ptid).c_str ());
> @@ -1791,6 +2370,9 @@ 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:
> Please add an empty line before a new case.
> [Zied] this was  a bad conflict handling  in an rebase, it is corrected in the next commit. I do not know how to handle it

You'd simply fixup that commit.

 
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index 39ac9aafc28..107abd7e653 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -3020,7 +3029,6 @@ cmd_record_btrace_start (const char *args, int from_tty)
>      }
>  }
> 
> -
> Hmmm, I do not see two empty lines at that location.
> [Zied] yes, it is. it was an error during a merge in a rebase. it is now hidden in the history and I do not find a way to fix it. when you squash the patches, the code will look fine.

We're not going to squash the patches.  You could rebase and edit the parent commit
to not introduce those empty lines.  The change will propagate to this patch and the
hunk will be removed.

 
> +/* The following enum must be aligned with the
> +   open source coresight trace decoder library (opencsd).  */
> Could we get them from a header file?
> [Zied] this was intentionally done so, since btrace-common.h is included in many files. This is to allow compiling gdb if opencsd library is not present.

We could conditionally include the header and all code that references anything
declared in it.  Would that be feasible?

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