[PATCH 02/24] Add MIPS32 FPU64 GDB target descriptions
Maciej W. Rozycki
macro@imgtec.com
Wed Oct 12 12:42:00 GMT 2016
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 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2012-2013 Free Software Foundation, Inc.
You need to list the current year in copyright notices for newly
submitted files, i.e.:
<!-- Copyright (C) 2016 Free Software Foundation, Inc.
Likewise throughout.
> diff --git a/gdb/features/mips-fpu64.xml b/gdb/features/mips-fpu64.xml
> new file mode 100644
> index 0000000..88dc9d9
> --- /dev/null
> +++ b/gdb/features/mips-fpu64.xml
> @@ -0,0 +1,45 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2007-2013 Free Software Foundation, Inc.
> +
> + Copying and distribution of this file, with or without modification,
> + are permitted in any medium without royalty provided the copyright
> + notice and this notice are preserved. -->
> +
> +<!DOCTYPE feature SYSTEM "gdb-target.dtd">
> +<feature name="org.gnu.gdb.mips.fpu">
> + <reg name="f0" bitsize="64" type="ieee_double"/>
> + <reg name="f1" bitsize="64" type="ieee_double"/>
> + <reg name="f2" bitsize="64" type="ieee_double"/>
> + <reg name="f3" bitsize="64" type="ieee_double"/>
> + <reg name="f4" bitsize="64" type="ieee_double"/>
> + <reg name="f5" bitsize="64" type="ieee_double"/>
> + <reg name="f6" bitsize="64" type="ieee_double"/>
> + <reg name="f7" bitsize="64" type="ieee_double"/>
> + <reg name="f8" bitsize="64" type="ieee_double"/>
> + <reg name="f9" bitsize="64" type="ieee_double"/>
> + <reg name="f10" bitsize="64" type="ieee_double"/>
> + <reg name="f11" bitsize="64" type="ieee_double"/>
> + <reg name="f12" bitsize="64" type="ieee_double"/>
> + <reg name="f13" bitsize="64" type="ieee_double"/>
> + <reg name="f14" bitsize="64" type="ieee_double"/>
> + <reg name="f15" bitsize="64" type="ieee_double"/>
> + <reg name="f16" bitsize="64" type="ieee_double"/>
> + <reg name="f17" bitsize="64" type="ieee_double"/>
> + <reg name="f18" bitsize="64" type="ieee_double"/>
> + <reg name="f19" bitsize="64" type="ieee_double"/>
> + <reg name="f20" bitsize="64" type="ieee_double"/>
> + <reg name="f21" bitsize="64" type="ieee_double"/>
> + <reg name="f22" bitsize="64" type="ieee_double"/>
> + <reg name="f23" bitsize="64" type="ieee_double"/>
> + <reg name="f24" bitsize="64" type="ieee_double"/>
> + <reg name="f25" bitsize="64" type="ieee_double"/>
> + <reg name="f26" bitsize="64" type="ieee_double"/>
> + <reg name="f27" bitsize="64" type="ieee_double"/>
> + <reg name="f28" bitsize="64" type="ieee_double"/>
> + <reg name="f29" bitsize="64" type="ieee_double"/>
> + <reg name="f30" bitsize="64" type="ieee_double"/>
> + <reg name="f31" bitsize="64" type="ieee_double"/>
> +
> + <reg name="fcsr" bitsize="32" group="float"/>
> + <reg name="fir" bitsize="32" group="float"/>
> +</feature>
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
More information about the Gdb-patches
mailing list