From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 101987 invoked by alias); 12 Oct 2016 12:42:43 -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 101969 invoked by uid 89); 12 Oct 2016 12:42:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=4.3 required=5.0 tests=BAYES_00,GARBLED_SUBJECT,RP_MATCHES_RCVD,SPF_PASS autolearn=no version=3.3.2 spammy=You'll, GIT, WHICH, paperwork X-HELO: mailapp01.imgtec.com Received: from mailapp02.imgtec.com (HELO mailapp01.imgtec.com) (217.156.133.132) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 12 Oct 2016 12:42:32 +0000 Received: from HHMAIL03.hh.imgtec.org (unknown [10.44.0.21]) by Forcepoint Email with ESMTPS id 3EABCBEDFC936 for ; Wed, 12 Oct 2016 13:42:25 +0100 (IST) Received: from HHMAIL01.hh.imgtec.org (10.100.10.19) by HHMAIL03.hh.imgtec.org (10.44.0.21) with Microsoft SMTP Server (TLS) id 14.3.294.0; Wed, 12 Oct 2016 13:42:28 +0100 Received: from [10.20.78.104] (10.20.78.104) by HHMAIL01.hh.imgtec.org (10.100.10.21) with Microsoft SMTP Server id 14.3.294.0; Wed, 12 Oct 2016 13:42:27 +0100 Date: Wed, 12 Oct 2016 12:42:00 -0000 From: "Maciej W. Rozycki" To: Bhushan Attarde CC: , Matthew Fortune , James Hogan , "Andrew Bennett" , Subject: Re: [PATCH 02/24] Add MIPS32 FPU64 GDB target descriptions In-Reply-To: <1467038991-6600-2-git-send-email-bhushan.attarde@imgtec.com> Message-ID: References: <1467038991-6600-1-git-send-email-bhushan.attarde@imgtec.com> <1467038991-6600-2-git-send-email-bhushan.attarde@imgtec.com> User-Agent: Alpine 2.00 (DEB 1167 2008-08-23) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" X-SW-Source: 2016-10/txt/msg00311.txt.bz2 On Mon, 27 Jun 2016, Bhushan Attarde wrote: > Add a couple of new target descriptions for MIPS32 targets with 64-bit > floating point registers, with and without DSP. > > These are identical to their 32-bit FPU counterparts except that the FP > registers are 64-bits long to allow for debugging of F64=1 MIPS32 targets > such as MIPS32r2 compatible cores, and they include the Config5 CP0 > register which has an FRE bit. Hmm, has Linux kernel support for CP0.Config5 accesses gone upstream already? Can you give me an upstream commit ID and/or reference to the discussion where it has been approved if so? More importantly, what do we need CP0.Config5 access for in the first place? It looks to me like this bit is irrelevant to GDB as it does not affect the native (raw) register format. So the only use would be to let the user running a debugging session switch between the FRE and NFRE modes without the need to poke at CP1C.FRE or CP1C.NFRE registers with a CTC1 instruction, which by itself makes sense to me, but needs a further consideration. Additionally exposing CP0.Config5 may have security implications, especially as parts of the register have not been defined yet in the architectures and we'd have to force architecture maintainers somehow to ask us every time they intend to add a bit to this register to check if this has security implications and has to be avoided and/or explicitly handled in software. So overall it looks to me like we should avoid it. What we ought to do instead is -- since we need to extend structures anyway -- I think adding all the remaining CP1C registers. This would cover the earlier CP1C.UFR and CP1C.UNFR registers as well as any future additions, which would be further qualified with CP1.FIR bits to remove unimplemented registers from the view. Given the unusual semantics of these registers I'd be tempted to map both pairs to the read register only and pretend it to be r/w with the LSB directly mapping to CP0.Status.FR and CP0.Config5.FRE as with CFC1. Access would be r/o if user access to the selected bit was not permitted. This would provide the necessary semantics and avoid any security implications as GDB would only be able to poke/peek at the same bits the debuggee itself could. This could be sorted out the same way in core files. Finally, do we need to have these bits recorded in a signal frame? > F64 targets have an FR bit in the status register to specify the effective > register size, and the FRE bit is used with FR=1 to enable a compatibility > mode which is similar to FR=0 but with usable odd doubles which don't > alias with any other FP registers. This seems to contradict to how I read the architecture specification, which IIUC states that CP0.Config5.FRE=1 merely causes all single FP operations to trap. All 32 64-bit registers are usable for doubles with CP0.Status.FR=1 regardless of the state of CP0.Config5.FRE. Have I missed anything here? Can you please elaborate and perhaps rephrase the sentence so that the implications for GDB are more clearly seen? Further comments as to the change itself follow. > gdb/ChangeLog: > > * features/Makefile (WHICH): Add mips-fpu64-linux and > mips-fpu64-dsp-linux and Sort microblaze out of the way Mid-sentence capitalisation issue here. > of mips/mips64 Indentation and missing full stop. > (mips-fpu64-expedite): Set. > (mips-fpu64-dsp-expedite): Set. > * features/mips-fpu64-dsp-linux.xml: New file. > * features/mips-fpu64-linux.xml: New file. > * features/mips-fpu64.xml: New file. > * features/mips-fpu64-dsp-linux.c: New generated file. > * features/mips-fpu64-linux.c: New generated file. > * regformats/mips-fpu64-dsp-linux.dat: New generated file. > * regformats/mips-fpu64-linux.dat: New generated file. > > gdb/gdbserver/ChangeLog: > > * Makefile.in (clean): Delete mips-fpu64-linux.c and > mips-fpu64-dsp-linux.c. I think "Delete" is confusing here: are you deleting from the rule or deleting files? Please rephrase to make it unambiguous. Maybe: "Add a deletion of..."? > diff --git a/gdb/features/Makefile b/gdb/features/Makefile > index 10173cf..b6baaf66 100644 > --- a/gdb/features/Makefile > +++ b/gdb/features/Makefile > @@ -57,8 +57,8 @@ WHICH = aarch64 \ > i386/x32 i386/x32-linux \ > i386/x32-avx i386/x32-avx-linux \ > i386/x32-avx512 i386/x32-avx512-linux \ > - mips-linux mips-dsp-linux \ > microblaze-with-stack-protect \ > + mips-linux mips-dsp-linux mips-fpu64-linux mips-fpu64-dsp-linux \ > mips64-linux mips64-dsp-linux \ > nios2-linux \ > rs6000/powerpc-32 \ > @@ -100,11 +100,13 @@ i386/x32-avx-expedite = rbp,rsp,rip > i386/x32-avx-linux-expedite = rbp,rsp,rip > i386/x32-avx512-expedite = rbp,rsp,rip > i386/x32-avx512-linux-expedite = rbp,rsp,rip > +microblaze-expedite = r1,rpc > mips-expedite = r29,pc > mips-dsp-expedite = r29,pc > +mips-fpu64-expedite = r29,pc > +mips-fpu64-dsp-expedite = r29,pc > mips64-expedite = r29,pc > mips64-dsp-expedite = r29,pc > -microblaze-expedite = r1,rpc > nios2-linux-expedite = sp,pc > powerpc-expedite = r1,pc > rs6000/powerpc-cell32l-expedite = r1,pc,r0,orig_r3,r4 Please split Microblaze changes off to a separate preparatory patch. They can probably go in right away as obviously correct, but as a newcomer please allow 48 hours for people to chime in before committing. Speaking of which -- I don't see you listed in gdb/MAINTAINERS, so can you please clarify your status WRT the FSF copyright assignment? Are you covered by a company assignment? If your paperwork has been cleared, then I can commit your change on your behalf. > diff --git a/gdb/features/mips-fpu64-dsp-linux.xml b/gdb/features/mips-fpu64-dsp-linux.xml > new file mode 100644 > index 0000000..f0de5ce > --- /dev/null > +++ b/gdb/features/mips-fpu64-dsp-linux.xml > @@ -0,0 +1,20 @@ > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + > + This is AFAICT the same as mips64-fpu.xml, except for the sizes of `fcsr' and `fir', wrongly set to 64 bits in the latter file. I think we ought to avoid the unnecessary duplication, and just use a single template for the FPU, which is the same in the 64-bit FP case regardless of the machine word width of the CPU. Can you therefore please just reuse mips64-fpu.xml here, which I think should be renamed at the same time to mips-fpu64.xml, as with your proposal (GIT will detect a rename and show it properly with diffs and other stats)? For historical reasons we need to keep $fcsr and $fir at 64 bits in XML descriptions, as explained in commit 78b86327b530 ("mips-tdep: Make FCRs always 32-bit"), made specifically to address the inconsistency discovered in the context of your submission; none of any further FCRs added will have any historical implications though, so they ought to be 32-bit as in hardware. You'll have to adjust the callers of `mips_collect_register_32bit' and `mips_supply_register_32bit' in `gdbserver' accordingly so that `use_64bit' is set according to the FPU rather than the CPU model then. I don't think you'll need to adjust anything in mips-linux-tdep.c, however please double-check. > diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in > index 1e874e3..9913d6a 100644 > --- a/gdb/gdbserver/Makefile.in > +++ b/gdb/gdbserver/Makefile.in > @@ -357,7 +357,8 @@ clean: > rm -f reg-tilegx.c reg-tilegx32.c > rm -f arm-with-iwmmxt.c > rm -f arm-with-vfpv2.c arm-with-vfpv3.c arm-with-neon.c > - rm -f mips-linux.c mips-dsp-linux.c > + rm -f mips-linux.c mips-dsp-linux.c mips-fpu64-linux.c > + rm -f mips-fpu64-dsp-linux.c > rm -f mips64-linux.c mips64-dsp-linux.c > rm -f nios2-linux.c > rm -f powerpc-32.c powerpc-32l.c powerpc-64l.c powerpc-e500l.c This will then have to reflect the removal of mips64-fpu.xml of course. > index a54b9e7..422fd90 100644 > --- a/gdb/gdbserver/configure.srv > +++ b/gdb/gdbserver/configure.srv > @@ -187,15 +187,20 @@ case "${target}" in > ;; > mips*-*-linux*) srv_regobj="mips-linux.o" > srv_regobj="${srv_regobj} mips-dsp-linux.o" > + srv_regobj="${srv_regobj} mips-fpu64-linux.o" > + srv_regobj="${srv_regobj} mips-fpu64-dsp-linux.o" > srv_regobj="${srv_regobj} mips64-linux.o" > srv_regobj="${srv_regobj} mips64-dsp-linux.o" > srv_tgtobj="$srv_linux_obj linux-mips-low.o" > srv_tgtobj="${srv_tgtobj} mips-linux-watch.o" > srv_xmlfiles="mips-linux.xml" > srv_xmlfiles="${srv_xmlfiles} mips-dsp-linux.xml" > + srv_xmlfiles="${srv_xmlfiles} mips-fpu64-linux.xml" > + srv_xmlfiles="${srv_xmlfiles} mips-fpu64-dsp-linux.xml" > srv_xmlfiles="${srv_xmlfiles} mips-cpu.xml" > srv_xmlfiles="${srv_xmlfiles} mips-cp0.xml" > srv_xmlfiles="${srv_xmlfiles} mips-fpu.xml" > + srv_xmlfiles="${srv_xmlfiles} mips-fpu64.xml" > srv_xmlfiles="${srv_xmlfiles} mips-dsp.xml" > srv_xmlfiles="${srv_xmlfiles} mips64-linux.xml" > srv_xmlfiles="${srv_xmlfiles} mips64-dsp-linux.xml" Likewise. Maciej