Re: [PATCH v4 3/3] arc: Add GNU/Linux support for ARC

[Simon Marchi <simark@simark.ca>]

This is OK, with the nits mentioned below fixed.

On 2020-07-23 3:35 p.m., Shahab Vahedi via Gdb-patches wrote:
> From: Anton Kolesov <Anton.Kolesov@synopsys.com>
> 
> ARC Linux targets differences from baremetal:
> 
> - No support for hardware single instruction stepping.
> - Different access rules to registers.
> - Use of another instruction for breakpoints.
> 
> v2: Changes after Tom's remarks [1]
>  arc-linux-tdep.c
>   - Use true/false instead of TRUE/FALSE.
>   - arc_linux_sw_breakpoint_from_kind (): Break long lines into two.
>   - arc_linux_sw_breakpoint_from_kind (): Remove starting blank line.
>   - Use explicit number evaluation, e.g: if (a & b) -> if ((a & b) != 0)
>  arc-tdep.c
>   - Use explicit number evaluation, e.g: if (a & b) -> if ((a & b) != 0)
>  gdb/configure.tgt
>   - arc*-*-linux*): Remove "build_gdbserver=yes".
> 
> v3: Changes after Simon's remarks [2]
>   arc-linux-tdep.c
>   - Use "return trap_size" instead of cryptic "return 2".
>   - Removed unnecessary curly braces.
>   - Removed "void" from "_initialize_arc_linux_tdep (void)".
> 
> [1] Tom's remarks
> https://sourceware.org/pipermail/gdb-patches/2020-April/167887.html
> 
> [2] Simon's remarks
> https://sourceware.org/pipermail/gdb-patches/2020-May/168513.html
> 
> 2020-07-22  Anton Kolesov  <anton.kolesov@synopsys.com>
> 
> 	* configure.tgt: ARC support for GNU/Linux.
> 	* Makefile.in (ALL_TARGET_OBJS): Likewise.
> 	* arc-linux-tdep.c: New file.
> 	* arc-tdep.h (ARC_STATUS32_L_MASK, ARC_STATUS32_DE_MASK): Declare.
> 	* arc-tdep.c (arc_write_pc): Use it.
> ---
>  gdb/Makefile.in      |   1 +
>  gdb/arc-linux-tdep.c | 283 +++++++++++++++++++++++++++++++++++++++++++
>  gdb/arc-tdep.c       |   3 +-
>  gdb/arc-tdep.h       |   5 +
>  gdb/configure.tgt    |   5 +
>  5 files changed, 295 insertions(+), 2 deletions(-)
>  create mode 100644 gdb/arc-linux-tdep.c
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 9d484457397..85304b9f549 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -697,6 +697,7 @@ ALL_64_TARGET_OBS = \
>  # All other target-dependent objects files (used with --enable-targets=all).
>  ALL_TARGET_OBS = \
>  	aarch32-tdep.o \
> +	arc-linux-tdep.o \
>  	arc-tdep.o \
>  	arch/aarch32.o \
>  	arch/arc.o \
> diff --git a/gdb/arc-linux-tdep.c b/gdb/arc-linux-tdep.c
> new file mode 100644
> index 00000000000..be24a072881
> --- /dev/null
> +++ b/gdb/arc-linux-tdep.c
> @@ -0,0 +1,283 @@
> +/* Target dependent code for GNU/Linux ARC.
> +
> +   Copyright 2020 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +/* GDB header files.  */
> +#include "defs.h"
> +#include "linux-tdep.h"
> +#include "objfiles.h"
> +#include "opcode/arc.h"
> +#include "osabi.h"
> +#include "solib-svr4.h"
> +
> +/* ARC header files.  */
> +#include "opcodes/arc-dis.h"
> +#include "arc-tdep.h"
> +
> +/* Implement the "cannot_fetch_register" gdbarch method.  */
> +
> +static int
> +arc_linux_cannot_fetch_register (struct gdbarch *gdbarch, int regnum)
> +{
> +  /* Assume that register is readable if it is unknown.  */
> +  switch (regnum)
> +    {
> +    case ARC_ILINK_REGNUM:
> +    case ARC_RESERVED_REGNUM:
> +    case ARC_LIMM_REGNUM:
> +      return true;
> +    case ARC_R30_REGNUM:
> +    case ARC_R58_REGNUM:
> +    case ARC_R59_REGNUM:
> +      return !arc_mach_is_arcv2 (gdbarch);
> +    }
> +  if (regnum > ARC_BLINK_REGNUM && regnum < ARC_LP_COUNT_REGNUM)
> +    return true;
> +  return false;

Not very important, but it would be more idiomatic to write:

  return regnum > ARC_BLINK_REGNUM && regnum < ARC_LP_COUNT_REGNUM;

> +}
> +
> +/* Implement the "cannot_store_register" gdbarch method.  */
> +
> +static int
> +arc_linux_cannot_store_register (struct gdbarch *gdbarch, int regnum)
> +{
> +  /* Assume that register is writable if it is unknown.  */
> +  switch (regnum)
> +    {
> +    case ARC_ILINK_REGNUM:
> +    case ARC_RESERVED_REGNUM:
> +    case ARC_LIMM_REGNUM:
> +    case ARC_PCL_REGNUM:
> +      return true;
> +    case ARC_R30_REGNUM:
> +    case ARC_R58_REGNUM:
> +    case ARC_R59_REGNUM:
> +      return !arc_mach_is_arcv2 (gdbarch);
> +    }
> +  if (regnum > ARC_BLINK_REGNUM && regnum < ARC_LP_COUNT_REGNUM)
> +    return true;
> +  return false;

Same here.

> +}
> +
> +/* For ARC Linux, breakpoint uses the 16-bit TRAP_S 1 instruction, which

It would read better if "breakpoint" was plural, so "breakpoints use the...".

> +   is 0x3e78 (little endian) or 0x783e (big endian).  */
> +
> +static const gdb_byte arc_linux_trap_s_be[] = { 0x78, 0x3e };
> +static const gdb_byte arc_linux_trap_s_le[] = { 0x3e, 0x78 };
> +static const int trap_size = 2;   /* Number of bytes to insert "trap".  */
> +
> +/* Implement the "breakpoint_kind_from_pc" gdbarch method.  */
> +
> +static int
> +arc_linux_breakpoint_kind_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr)
> +{
> +  return trap_size;
> +}
> +
> +/* Implement the "sw_breakpoint_from_kind" gdbarch method.  */
> +
> +static const gdb_byte *
> +arc_linux_sw_breakpoint_from_kind (struct gdbarch *gdbarch,
> +				   int kind, int *size)
> +{
> +  *size = kind;
> +  return ((gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
> +	  ? arc_linux_trap_s_be
> +	  : arc_linux_trap_s_le);
> +}
> +
> +/* Implement the "software_single_step" gdbarch method.  */
> +
> +static std::vector<CORE_ADDR>
> +arc_linux_software_single_step (struct regcache *regcache)
> +{
> +  struct gdbarch *gdbarch = regcache->arch ();
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +  struct disassemble_info di = arc_disassemble_info (gdbarch);
> +
> +  /* Read current instruction.  */
> +  struct arc_instruction curr_insn;
> +  arc_insn_decode (regcache_read_pc (regcache), &di, arc_delayed_print_insn,
> +		   &curr_insn);
> +  CORE_ADDR next_pc = arc_insn_get_linear_next_pc (curr_insn);
> +
> +  std::vector<CORE_ADDR> next_pcs;
> +
> +  /* For instructions with delay slots, the fall thru is not the
> +     instruction immediately after the current instruction, but the one
> +     after that.  */
> +  if (curr_insn.has_delay_slot)
> +    {
> +      struct arc_instruction next_insn;
> +      arc_insn_decode (next_pc, &di, arc_delayed_print_insn, &next_insn);
> +      next_pcs.push_back (arc_insn_get_linear_next_pc (next_insn));
> +    }
> +  else
> +    next_pcs.push_back (next_pc);
> +
> +  ULONGEST status32;
> +  regcache_cooked_read_unsigned (regcache, gdbarch_ps_regnum (gdbarch),
> +				 &status32);
> +
> +  if (curr_insn.is_control_flow)
> +    {
> +      CORE_ADDR branch_pc = arc_insn_get_branch_target (curr_insn);
> +      if (branch_pc != next_pc)
> +	next_pcs.push_back (branch_pc);
> +    }
> +  /* Is current instruction the last in a loop body?  */
> +  else if (tdep->has_hw_loops)
> +    {
> +      /* If STATUS32.L is 1, then ZD-loops are disabled.  */
> +      if ((status32 & ARC_STATUS32_L_MASK) == 0)
> +	{
> +	  ULONGEST lp_end, lp_start, lp_count;
> +	  regcache_cooked_read_unsigned (regcache, ARC_LP_START_REGNUM,
> +					 &lp_start);
> +	  regcache_cooked_read_unsigned (regcache, ARC_LP_END_REGNUM, &lp_end);
> +	  regcache_cooked_read_unsigned (regcache, ARC_LP_COUNT_REGNUM,
> +					 &lp_count);
> +
> +	  if (arc_debug)
> +	    {
> +	      debug_printf ("arc-linux: lp_start = %s, lp_end = %s, "
> +			    "lp_count = %s, next_pc = %s\n",
> +			    paddress (gdbarch, lp_start),
> +			    paddress (gdbarch, lp_end),
> +			    pulongest (lp_count),
> +			    paddress (gdbarch, next_pc));
> +	    }
> +
> +	  if (next_pc == lp_end && lp_count > 1)
> +	    {
> +	      /* The instruction is in effect a jump back to the start of
> +		 the loop.  */
> +	      next_pcs.push_back (lp_start);
> +	    }
> +
> +	}

Remove empty line above.

> +    }
> +
> +  /* Is this a delay slot?  Then next PC is in BTA register.  */
> +  if ((status32 & ARC_STATUS32_DE_MASK) != 0)
> +    {
> +      ULONGEST bta;
> +      regcache_cooked_read_unsigned (regcache, ARC_BTA_REGNUM, &bta);
> +      next_pcs.push_back (bta);
> +    }
> +
> +  return next_pcs;
> +}
> +
> +/* Implement the "skip_solib_resolver" gdbarch method.
> +
> +   See glibc_skip_solib_resolver for details.  */
> +
> +static CORE_ADDR
> +arc_linux_skip_solib_resolver (struct gdbarch *gdbarch, CORE_ADDR pc)
> +{
> +  /* For uClibc 0.9.26+.
> +
> +     An unresolved PLT entry points to "__dl_linux_resolve", which calls
> +     "_dl_linux_resolver" to do the resolving and then eventually jumps to
> +     the function.
> +
> +     So we look for the symbol `_dl_linux_resolver', and if we are there,
> +     gdb sets a breakpoint at the return address, and continues.  */
> +  struct bound_minimal_symbol resolver =
> +    lookup_minimal_symbol ("_dl_linux_resolver", NULL, NULL);

Put the equal sign on the next line:

  struct bound_minimal_symbol resolver
    = lookup_minimal_symbol ("_dl_linux_resolver", NULL, NULL);

Simon