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

Simon Marchi
2020-08-01T15:01:42Z

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