From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 128849 invoked by alias); 14 Apr 2017 18:02:58 -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 128776 invoked by uid 89); 14 Apr 2017 18:02:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-22.6 required=5.0 tests=AWL,BAYES_00,DATE_IN_PAST_24_48,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=ham version=3.3.2 spammy=bank, Thursday, thursday X-HELO: mail.baldwin.cx Received: from bigwig.baldwin.cx (HELO mail.baldwin.cx) (96.47.65.170) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 14 Apr 2017 18:02:56 +0000 Received: from ralph.baldwin.cx (c-73-231-226-104.hsd1.ca.comcast.net [73.231.226.104]) by mail.baldwin.cx (Postfix) with ESMTPSA id C493210A8BB; Fri, 14 Apr 2017 14:02:55 -0400 (EDT) From: John Baldwin To: Luis Machado Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/4] Cleanups to FreeBSD/mips native register operations. Date: Fri, 14 Apr 2017 18:02:00 -0000 Message-ID: <1671472.AujrONiZJ6@ralph.baldwin.cx> User-Agent: KMail/4.14.10 (FreeBSD/11.0-STABLE; KDE/4.14.10; amd64; ; ) In-Reply-To: <8989db11-ec9e-33f8-ae97-fbd4518bdaef@codesourcery.com> References: <20170412183727.22483-1-jhb@FreeBSD.org> <20170412183727.22483-2-jhb@FreeBSD.org> <8989db11-ec9e-33f8-ae97-fbd4518bdaef@codesourcery.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-IsSubscribed: yes X-SW-Source: 2017-04/txt/msg00468.txt.bz2 On Thursday, April 13, 2017 10:48:48 AM Luis Machado wrote: > 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? The cooked value is too large. In particular, the existing range right now goes from "0" to "cooked PC" which includes "raw GP + raw FP + cooked GP". This means that a request for a raw FP register will see 'getregs_supplies() return true and will try to satisfy it via PT_GETREGS (which doesn't work). The end result is that the register just isn't found. > > + > > +/* 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. Yes, though in this case, gdbarch is actually used sooner than 'pid'. I can revert these though. > > @@ -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. It does not, but I'm working on a research CPU that is an extension to MIPS (CHERI) and it uses a third bank of registers that live above 'fp' with a separate ptrace interface, etc. I don't know that the CHERI bits will ever be upstreamed since it is a research design, but this newer check seems strictly more correct as it only uses PT_GETFPREGS to fetch registers that are actually supported (e.g. it won't try to use it to fetch fir which FreeBSD doesn't export via PT_GETFPREGS). > If there are vector (or higher-numbered registers), the new conditional > block means something different compared to the old one. I think that the new conditional is more correct in the case of higher numbered registers as it means we don't try to use PT_GETFPREGS to fetch a register it doesn't support. > If not, then the change looks sane. -- John Baldwin