[PATCH] gdbserver: Add GNU/Linux support for ARC

Shahab Vahedi shahab.vahedi@gmail.com
Tue Oct 6 16:21:13 GMT 2020


Hi Simon,

On Wed, Sep 16, 2020 at 03:59:42PM -0400, Simon Marchi wrote:
> On 2020-08-26 12:56 p.m., Shahab Vahedi via Gdb-patches wrote:
> > +static const struct target_desc *
> > +arc_linux_read_description (void)
> > +{
> > +#ifdef __ARC700__
> > +  arc_gdbarch_features features (4, ARC_ISA_ARCV1);
> > +#else
> > +  arc_gdbarch_features features (4, ARC_ISA_ARCV2);
> > +#endif
> 
> I should have mentionned this in an earlier review, but
> "arc_gdbarch_features" is a bit mis-named.  "gdbarch" specifically
> refers to the type/interface in GDB, but arc_gdbarch_features is not
> related to that.  "arc_arch_features" would be a bit better, if you want
> to change it in an obvious patch.

I renamed it in the next version.  Therefore, v2 is a patch series and
not a single patch anymore.

> > +/* The breakpoint instruction is TRAP_S 1, network function ntohs can be
> > +   used to convert its little-endian form (0x3e78) to the host
> > +   representation, which may be little-endian or big-endian (network
> > +   representation is defined to be little-endian).  */
> 
> Isn't network byte order big endian?

Got rid of it.

> > +static void
> > +arc_fill_gregset (struct regcache *regcache, void *buf)
> > +{
> > +  struct user_regs_struct *regbuf = (struct user_regs_struct *) buf;
> > +
> > +  /* Core registers.  */
> > +  collect_register_by_name (regcache, "r0", &(regbuf->scratch.r0));
> >    ...
> > +  /* PC should be written to "return address", instead of stop address.  */
> > +  collect_register_by_name (regcache, "pc", &(regbuf->scratch.ret));
> 
> Can you explain why the PC is written to "ret", whereas when going the
> other way we take the value from the "stop_pc" field?

I've added the following comments in the code in v2.  I hope it is
helpful.

  /* The current "pc" value must be written to "eret" (exception return
     address) register, because that is the address that the kernel code
     will jump back to after a breakpoint exception has been raised.
     The "pc_stop" value is ignored by the genregs_set() in
     linux/arch/arc/kernel/ptrace.c.  */
  collect_register_by_name (regcache, "pc", &(regbuf->scratch.ret));

  /* The genregs_get() in linux/arch/arc/kernel/ptrace.c populates the
     pseudo register "stop_pc" with the "efa" (exception fault address)
     register.  This was deemed necessary, because the breakpoint
     instruction, "trap_s 1", is a committing one; i.e. the "eret"
     (excpetion return address) register will be pointing to the next
     instruction, while "efa" points to the address that raised the
     breakpoint.  */
  supply_register_by_name (regcache, "pc", &(regbuf->stop_pc));

> > +
> > +const gdb_byte *
> > +arc_target::sw_breakpoint_from_kind (int kind, int *size)
> > +{
> > +  static bool initialized = false;
> > +  static gdb_byte arc_linux_trap_s[TRAP_S_1_SIZE] = { 0, };
> > +
> > +  if (!initialized)
> > +    {
> > +      uint16_t breakpoint = ntohs (TRAP_S_1_OPCODE);
> > +      *((uint16_t *) arc_linux_trap_s) = breakpoint;
> > +      initialized = true;
> > +    }
> > +
> > +  gdb_assert (kind == TRAP_S_1_SIZE);
> 
> I always find it a little odd that we choose the breakpoint size as the
> kind, instead of just an arbitrary number (first one 0, second one 1,
> etc).   What if we want to use another breakpoint instruction that is
> two bytes long?  Anyway, not really a problem, just something I wonder
> about.

I agree.  I rather use an enum that acts like a key in a map that returns
the size.  I see this pattern in "gdb" code as well.  I plan to tackle
this in a separate patch.  So I can focus on sharing the code base as
much as possible.


Cheers,
Shahab


More information about the Gdb-patches mailing list