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

[Simon Marchi]

Hi Shahab,

I've noted a few comments below.  Otherwise, it looks good to me.  I
don't know all the gdbserver quirks by heart, but I've looked at other
implementations (which is probably what you did too :)) and it all
seemed reasonable.  I also don't know the specific details of the
various ARC variants, so I trust you for that.

Once the nits are fixed, you can merge this to master and the
gdb-10-branch.

On 2020-08-26 12:56 p.m., Shahab Vahedi via Gdb-patches wrote:
> From: Anton Kolesov <Anton.Kolesov@synopsys.com>
>
> This gdbserver implementation supports ARC ABI v3 and v4 (older ARC ABI
> versions are not supported by other modern GNU tools or Linux itself).
> Gdbserver supports inspection of ARC HS registers R30, R58 and R59 - feature
> that has been added to Linux 4.12.  Whether gdbserver build will actually
> support this feature depends on the version of Linux headers used to build
> the server.
>
> gdbserver/ChangeLog:
>
> 2020-08-26  Anton Kolesov  <anton.kolesov@synopsys.com>
>
> 	* configure.srv: Support ARC architecture.
> 	* Makefile.in: Add linux-arc-low.cc and arch-arc.o.
> 	* linux-arc-low.cc: New file.
> ---
>  gdbserver/Makefile.in      |   6 +
>  gdbserver/configure.srv    |  11 +
>  gdbserver/linux-arc-low.cc | 398 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 415 insertions(+)
>  create mode 100644 gdbserver/linux-arc-low.cc
>
> diff --git a/gdbserver/Makefile.in b/gdbserver/Makefile.in
> index 9d7687be534..df1b4d16da0 100644
> --- a/gdbserver/Makefile.in
> +++ b/gdbserver/Makefile.in
> @@ -179,6 +179,7 @@ SFILES = \
>  	$(srcdir)/i387-fp.cc \
>  	$(srcdir)/inferiors.cc \
>  	$(srcdir)/linux-aarch64-low.cc \
> +	$(srcdir)/linux-arc-low.cc \
>  	$(srcdir)/linux-arm-low.cc \
>  	$(srcdir)/linux-ia64-low.cc \
>  	$(srcdir)/linux-low.cc \
> @@ -206,6 +207,7 @@ SFILES = \
>  	$(srcdir)/win32-low.cc \
>  	$(srcdir)/x86-low.cc \
>  	$(srcdir)/../gdb/alloc.c \
> +	$(srcdir)/../gdb/arch/arc.c \
>  	$(srcdir)/../gdb/arch/arm.c \
>  	$(srcdir)/../gdb/arch/arm-get-next-pcs.c \
>  	$(srcdir)/../gdb/arch/arm-linux.c \
> @@ -490,6 +492,10 @@ ax.o: ax.cc
>  	$(COMPILE) $(WARN_CFLAGS_NO_FORMAT) $<
>  	$(POSTCOMPILE)
>
> +arch-arc.o: ../gdb/arch/arc.c
> +	$(COMPILE) $<
> +	$(POSTCOMPILE)
> +

I think that's not needed.  This file can use the

  arch/%.o: ../gdb/arch/%.c

pattern rule and get compiled to arch/arc.o.

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

> +  struct target_desc *tdesc = arc_create_target_description (features);
> +
> +  static const char *expedite_regs[] = { "sp", "status32", nullptr };
> +  init_target_desc (tdesc, expedite_regs);
> +
> +  return tdesc;
> +}
> +
> +void
> +arc_target::low_arch_setup ()
> +{
> +  current_process ()->tdesc = arc_linux_read_description ();
> +}
> +
> +bool
> +arc_target::low_cannot_fetch_register (int regno)
> +{
> +  return (regno >= current_process ()->tdesc->reg_defs.size());

Space before parenthesis.

> +}
> +
> +bool
> +arc_target::low_cannot_store_register (int regno)
> +{
> +  return (regno >= current_process ()->tdesc->reg_defs.size());

Space before parenthesis.

> +}
> +
> +/* 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?
> +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));
> +  collect_register_by_name (regcache, "r1", &(regbuf->scratch.r1));
> +  collect_register_by_name (regcache, "r2", &(regbuf->scratch.r2));
> +  collect_register_by_name (regcache, "r3", &(regbuf->scratch.r3));
> +  collect_register_by_name (regcache, "r4", &(regbuf->scratch.r4));
> +  collect_register_by_name (regcache, "r5", &(regbuf->scratch.r5));
> +  collect_register_by_name (regcache, "r6", &(regbuf->scratch.r6));
> +  collect_register_by_name (regcache, "r7", &(regbuf->scratch.r7));
> +  collect_register_by_name (regcache, "r8", &(regbuf->scratch.r8));
> +  collect_register_by_name (regcache, "r9", &(regbuf->scratch.r9));
> +  collect_register_by_name (regcache, "r10", &(regbuf->scratch.r10));
> +  collect_register_by_name (regcache, "r11", &(regbuf->scratch.r11));
> +  collect_register_by_name (regcache, "r12", &(regbuf->scratch.r12));
> +  collect_register_by_name (regcache, "r13", &(regbuf->callee.r13));
> +  collect_register_by_name (regcache, "r14", &(regbuf->callee.r14));
> +  collect_register_by_name (regcache, "r15", &(regbuf->callee.r15));
> +  collect_register_by_name (regcache, "r16", &(regbuf->callee.r16));
> +  collect_register_by_name (regcache, "r17", &(regbuf->callee.r17));
> +  collect_register_by_name (regcache, "r18", &(regbuf->callee.r18));
> +  collect_register_by_name (regcache, "r19", &(regbuf->callee.r19));
> +  collect_register_by_name (regcache, "r20", &(regbuf->callee.r20));
> +  collect_register_by_name (regcache, "r21", &(regbuf->callee.r21));
> +  collect_register_by_name (regcache, "r22", &(regbuf->callee.r22));
> +  collect_register_by_name (regcache, "r23", &(regbuf->callee.r23));
> +  collect_register_by_name (regcache, "r24", &(regbuf->callee.r24));
> +  collect_register_by_name (regcache, "r25", &(regbuf->callee.r25));
> +  collect_register_by_name (regcache, "gp", &(regbuf->scratch.gp));
> +  collect_register_by_name (regcache, "fp", &(regbuf->scratch.fp));
> +  collect_register_by_name (regcache, "sp", &(regbuf->scratch.sp));
> +  collect_register_by_name (regcache, "blink", &(regbuf->scratch.blink));
> +
> +  /* Loop registers.  */
> +  collect_register_by_name (regcache, "lp_count", &(regbuf->scratch.lp_count));
> +  collect_register_by_name (regcache, "lp_start", &(regbuf->scratch.lp_start));
> +  collect_register_by_name (regcache, "lp_end", &(regbuf->scratch.lp_end));
> +
> +  /* 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?

> +
> +  /* Currently ARC Linux ptrace doesn't allow writes to status32 because
> +     some of it's bits are kernel mode-only and shoudn't be writable from

"its bits"

> +/* Look through a regcache's TDESC for a register named NAME.
> +   If found, return true. False otherwise.  */
> +
> +static bool
> +is_reg_name_available_p (const struct target_desc *tdesc,
> +			 const char *name)
> +{
> +  for (int i = 0; i < tdesc->reg_defs.size (); ++i)
> +    if (strcmp (name, tdesc->reg_defs[i].name) == 0)

It looks like this could be a range based iteration:

  for (const gdb::reg &reg : tdesc->reg_defs)

> +const regs_info *
> +arc_target::get_regs_info ()
> +{
> +  return &arc_regs_info;
> +}
> +
> +/* One of the methods necessary for Z0 packet support.
> +   See issue #35 for further details.  */

"issue #35" isn't relevant in upstream code.

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

Simon