From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id ACE9B3858D35 for ; Sat, 1 Aug 2020 15:01:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org ACE9B3858D35 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.11] (173-246-6-90.qc.cable.ebox.net [173.246.6.90]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 0AAF31E792; Sat, 1 Aug 2020 11:01:41 -0400 (EDT) Subject: Re: [PATCH v4 3/3] arc: Add GNU/Linux support for ARC To: Shahab Vahedi , gdb-patches@sourceware.org Cc: Anton Kolesov , Francois Bedard , Shahab Vahedi , Tom Tromey References: <20200713154527.13430-1-shahab.vahedi@gmail.com> <20200723193532.25812-1-shahab.vahedi@gmail.com> <20200723193532.25812-4-shahab.vahedi@gmail.com> From: Simon Marchi Message-ID: <91b8d059-adf3-9761-8565-8b2b11e03b77@simark.ca> Date: Sat, 1 Aug 2020 11:01:35 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20200723193532.25812-4-shahab.vahedi@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-9.2 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_SHORT, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 01 Aug 2020 15:01:44 -0000 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 > > 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 > > * 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 . */ > + > +/* 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 > +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 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