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 8A8BD3959E5F for ; Wed, 16 Sep 2020 19:59:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8A8BD3959E5F 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 [172.16.0.95] (192-222-181-218.qc.cable.ebox.net [192.222.181.218]) (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 042AA1E96F; Wed, 16 Sep 2020 15:59:42 -0400 (EDT) Subject: Re: [PATCH] gdbserver: Add GNU/Linux support for ARC To: Shahab Vahedi , gdb-patches@sourceware.org Cc: Shahab Vahedi , Anton Kolesov , Francois Bedard References: <20200826165647.5221-1-shahab.vahedi@gmail.com> From: Simon Marchi Message-ID: <3f460c81-2cb7-08f8-21b2-5a333723c24b@simark.ca> Date: Wed, 16 Sep 2020 15:59:42 -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: <20200826165647.5221-1-shahab.vahedi@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: tl Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, 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: Wed, 16 Sep 2020 19:59:45 -0000 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 > > 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 > > * 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 ® : 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