From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 114964 invoked by alias); 13 Apr 2017 15:48:56 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 114950 invoked by uid 89); 13 Apr 2017 15:48:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.4 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 13 Apr 2017 15:48:53 +0000 Received: from svr-orw-mbx-03.mgc.mentorg.com ([147.34.90.203]) by relay1.mentorg.com with esmtp id 1cygzN-0003g2-NY from Luis_Gustavo@mentor.com ; Thu, 13 Apr 2017 08:48:53 -0700 Received: from [172.30.1.10] (147.34.91.1) by svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) with Microsoft SMTP Server (TLS) id 15.0.1210.3; Thu, 13 Apr 2017 08:48:50 -0700 Subject: Re: [PATCH 1/4] Cleanups to FreeBSD/mips native register operations. References: <20170412183727.22483-1-jhb@FreeBSD.org> <20170412183727.22483-2-jhb@FreeBSD.org> To: John Baldwin , Reply-To: Luis Machado From: Luis Machado Message-ID: <8989db11-ec9e-33f8-ae97-fbd4518bdaef@codesourcery.com> Date: Thu, 13 Apr 2017 15:48:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170412183727.22483-2-jhb@FreeBSD.org> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-ClientProxiedBy: svr-orw-mbx-02.mgc.mentorg.com (147.34.90.202) To svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) X-IsSubscribed: yes X-SW-Source: 2017-04/txt/msg00439.txt.bz2 On 04/12/2017 01:37 PM, John Baldwin wrote: > Compare against the "raw" PC register number instead of the cooked > register number when determining if a register was handled by > PT_GETREGS. Previously the register fetch/store operations only tried > PT_GETREGS to fetch any individual register. The result was that > fetching or storing an individual register not covered by PT_GETREGS > (such as floating point registers) did not work. > > While here, remove an early exit to simplify the code flow from the > PT_GETREGS / PT_SETREGS case, and add a getfpregs_supplies similar to > getregs_supplies to describe the registers supplied by PT_GETFPREGS > and PT_SETFPREGS. > > gdb/ChangeLog: > > * mips-fbsd-nat.c (getregs_supplies): Fix upper bound comparison. > (getpfpregs_supplies): New function. > (mips_fbsd_fetch_inferior_registers): Remove early exit and use > getfpregs_supplies. > (mips_fbsd_store_inferior_registers): Likewise. > --- > gdb/ChangeLog | 8 ++++++++ > gdb/mips-fbsd-nat.c | 26 ++++++++++++++------------ > 2 files changed, 22 insertions(+), 12 deletions(-) Only a few nits. > diff --git a/gdb/mips-fbsd-nat.c b/gdb/mips-fbsd-nat.c > index 078df52db6..e2ed63e829 100644 > --- a/gdb/mips-fbsd-nat.c > +++ b/gdb/mips-fbsd-nat.c > @@ -37,7 +37,16 @@ static bool > getregs_supplies (struct gdbarch *gdbarch, int regnum) > { > return (regnum >= MIPS_ZERO_REGNUM > - && regnum <= gdbarch_pc_regnum (gdbarch)); > + && regnum <= mips_regnum (gdbarch)->pc); > +} Can the BSD backend override the pc value in gdbarch_pc_regnum (...) so it fits what is expected? Or is this a case where the cooked pc register number is still useful and we need to handle things differently for the raw pc register number? > + > +/* Determine if PT_GETFPREGS fetches this register. */ Pedantically, "... fetches REGNUM". > @@ -47,9 +56,9 @@ static void > mips_fbsd_fetch_inferior_registers (struct target_ops *ops, > struct regcache *regcache, int regnum) > { > + struct gdbarch *gdbarch = get_regcache_arch (regcache); > pid_t pid = get_ptrace_pid (regcache_get_ptid (regcache)); > > - struct gdbarch *gdbarch = get_regcache_arch (regcache); > if (regnum == -1 || getregs_supplies (gdbarch, regnum)) With C++ we can leave the declaration closer to its use. Same in the other case below. > @@ -58,12 +67,9 @@ mips_fbsd_fetch_inferior_registers (struct target_ops *ops, > perror_with_name (_("Couldn't get registers")); > > mips_fbsd_supply_gregs (regcache, regnum, ®s, sizeof (register_t)); > - if (regnum != -1) > - return; > } > > - if (regnum == -1 > - || regnum >= gdbarch_fp0_regnum (get_regcache_arch (regcache))) > + if (regnum == -1 || getfpregs_supplies (gdbarch, regnum)) Does MIPS on fsbd handle vector registers? I ask this because regnum >= "fp0 regnum" may mean anything other than general purpose registers. If there are vector (or higher-numbered registers), the new conditional block means something different compared to the old one. If not, then the change looks sane. > @@ -82,9 +88,9 @@ static void > mips_fbsd_store_inferior_registers (struct target_ops *ops, > struct regcache *regcache, int regnum) > { > + struct gdbarch *gdbarch = get_regcache_arch (regcache); > pid_t pid = get_ptrace_pid (regcache_get_ptid (regcache)); > > - struct gdbarch *gdbarch = get_regcache_arch (regcache); > if (regnum == -1 || getregs_supplies (gdbarch, regnum)) > { > struct reg regs; Same as above about declaring something closer to where it is used. > @@ -97,13 +103,9 @@ mips_fbsd_store_inferior_registers (struct target_ops *ops, > > if (ptrace (PT_SETREGS, pid, (PTRACE_TYPE_ARG3) ®s, 0) == -1) > perror_with_name (_("Couldn't write registers")); > - > - if (regnum != -1) > - return; > } > > - if (regnum == -1 > - || regnum >= gdbarch_fp0_regnum (get_regcache_arch (regcache))) > + if (regnum == -1 || getfpregs_supplies (gdbarch, regnum)) Same thing as above, about higher-numbered registers. Otherwise i have no further comments.