[PATCH v5 1/3] RISC-V/Linux/native: Determine FLEN dynamically
Andrew Burgess
andrew.burgess@embecosm.com
Sun Feb 2 21:11:00 GMT 2020
* Maciej W. Rozycki <macro@wdc.com> [2020-02-01 20:20:46 +0000]:
> Fix RISC-V native Linux support to handle a 64-bit FPU (FLEN == 64) with
> both RV32 and RV64 systems, which is a part of the current Linux ABI for
> hard-float systems, rather than assuming that (FLEN == XLEN) in target
> description determination and that (FLEN == 64) in register access.
>
> We can do better however and not rely on any particular value of FLEN
> and probe for it dynamically, by observing that the PTRACE_GETREGSET
> ptrace(2) call will only accept an exact regset size, and that will
> reflect FLEN. Therefore iterate over the call in target description
> determination with a geometrically increasing regset size until a match
> is marked by a successful ptrace(2) call completion or we run beyond the
> maximum size we can support.
>
> Update register accessors accordingly, using FLEN determined to size the
> buffer used for NT_PRSTATUS requests and then to exchange data with the
> regcache.
>
> Also handle a glibc bug where ELF_NFPREG is defined in terms of NFPREG,
> however NFPREG is nowhere defined.
>
> gdb/
> * riscv-linux-nat.c [!NFPREG] (NFPREG): New macro.
> (supply_fpregset_regnum, fill_fpregset): Handle regset buffer
> offsets according to FLEN determined.
> (riscv_linux_nat_target::read_description): Determine FLEN
> dynamically.
> (riscv_linux_nat_target::fetch_registers): Size regset buffer
> according to FLEN determined.
> (riscv_linux_nat_target::store_registers): Likewise.
Thanks for working on this.
I'm happy for this patch to go in, with a couple of minor adjustments
that I've noted inline below.
Before you merge I just wanted to double check; given this patch is
from a wdc.com email, do you have a copyright assignment in place from
wdc?
Thanks,
Andrew
> ---
> No changes from v4.
>
> No changes from v3.
>
> Changes from v2:
>
> - Avoid an ambiguous FGR reference in
> `riscv_linux_nat_target::read_description'.
>
> - Advance through the buffer accessed in `supply_fpregset_regnum' and
> `fill_fpregset' so as to avoid doing a variable multiplication in a
> loop; also precalculate the buffer offset for single register accesses
> to avoid excessive line wrapping and improve code readability.
>
> - Move the `i' variable declaration to the beginning of `fill_fpregset',
> for consistency with `supply_fpregset_regnum' (since the loop is being
> rewritten anyway).
>
> Changes from v1:
>
> - Also set the size of the regset buffer dynamically in
> `riscv_linux_nat_target::fetch_registers' and
> `riscv_linux_nat_target::store_registers', and update `fill_fpregset'
> and `supply_fpregset_regnum' accordingly.
> ---
> gdb/riscv-linux-nat.c | 110 ++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 84 insertions(+), 26 deletions(-)
>
> gdb-riscv-linux-nat-flen.diff
> Index: binutils-gdb/gdb/riscv-linux-nat.c
> ===================================================================
> --- binutils-gdb.orig/gdb/riscv-linux-nat.c
> +++ binutils-gdb/gdb/riscv-linux-nat.c
> @@ -28,6 +28,11 @@
>
> #include <sys/ptrace.h>
>
> +/* Work around glibc header breakage causing ELF_NFPREG not to be usable. */
> +#ifndef NFPREG
> +# define NFPREG 33
> +#endif
> +
> /* RISC-V Linux native additions to the default linux support. */
>
> class riscv_linux_nat_target final : public linux_nat_target
> @@ -88,21 +93,35 @@ static void
> supply_fpregset_regnum (struct regcache *regcache, const prfpregset_t *fpregs,
> int regnum)
> {
> + int flen = register_size (regcache->arch (), RISCV_FIRST_FP_REGNUM);
> + union
> + {
> + const prfpregset_t *fpregs;
> + const gdb_byte *buf;
> + }
> + fpbuf = { .fpregs = fpregs };
> int i;
>
> if (regnum == -1)
> {
> /* We only support the FP registers and FCSR here. */
> - for (i = RISCV_FIRST_FP_REGNUM; i <= RISCV_LAST_FP_REGNUM; i++)
> - regcache->raw_supply (i, &fpregs->__d.__f[i - RISCV_FIRST_FP_REGNUM]);
> + for (i = RISCV_FIRST_FP_REGNUM;
> + i <= RISCV_LAST_FP_REGNUM;
> + i++, fpbuf.buf += flen)
> + regcache->raw_supply (i, fpbuf.buf);
>
> - regcache->raw_supply (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr);
> + regcache->raw_supply (RISCV_CSR_FCSR_REGNUM, fpbuf.buf);
> }
> else if (regnum >= RISCV_FIRST_FP_REGNUM && regnum <= RISCV_LAST_FP_REGNUM)
> - regcache->raw_supply (regnum,
> - &fpregs->__d.__f[regnum - RISCV_FIRST_FP_REGNUM]);
> + {
> + fpbuf.buf += flen * (regnum - RISCV_FIRST_FP_REGNUM);
> + regcache->raw_supply (regnum, fpbuf.buf);
> + }
> else if (regnum == RISCV_CSR_FCSR_REGNUM)
> - regcache->raw_supply (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr);
> + {
> + fpbuf.buf += flen * (RISCV_LAST_FP_REGNUM - RISCV_FIRST_FP_REGNUM + 1);
> + regcache->raw_supply (RISCV_CSR_FCSR_REGNUM, fpbuf.buf);
> + }
> }
>
> /* Copy all floating point registers from regset FPREGS into REGCACHE. */
> @@ -145,19 +164,35 @@ void
> fill_fpregset (const struct regcache *regcache, prfpregset_t *fpregs,
> int regnum)
> {
> + int flen = register_size (regcache->arch (), RISCV_FIRST_FP_REGNUM);
> + union
> + {
> + prfpregset_t *fpregs;
> + gdb_byte *buf;
> + }
> + fpbuf = { .fpregs = fpregs };
> + int i;
> +
> if (regnum == -1)
> {
> /* We only support the FP registers and FCSR here. */
> - for (int i = RISCV_FIRST_FP_REGNUM; i <= RISCV_LAST_FP_REGNUM; i++)
> - regcache->raw_collect (i, &fpregs->__d.__f[i - RISCV_FIRST_FP_REGNUM]);
> + for (i = RISCV_FIRST_FP_REGNUM;
> + i <= RISCV_LAST_FP_REGNUM;
> + i++, fpbuf.buf += flen)
> + regcache->raw_collect (i, fpbuf.buf);
>
> - regcache->raw_collect (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr);
> + regcache->raw_collect (RISCV_CSR_FCSR_REGNUM, fpbuf.buf);
> }
> else if (regnum >= RISCV_FIRST_FP_REGNUM && regnum <= RISCV_LAST_FP_REGNUM)
> - regcache->raw_collect (regnum,
> - &fpregs->__d.__f[regnum - RISCV_FIRST_FP_REGNUM]);
> + {
> + fpbuf.buf += flen * (regnum - RISCV_FIRST_FP_REGNUM);
> + regcache->raw_collect (regnum, fpbuf.buf);
> + }
> else if (regnum == RISCV_CSR_FCSR_REGNUM)
> - regcache->raw_collect (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr);
> + {
> + fpbuf.buf += flen * (RISCV_LAST_FP_REGNUM - RISCV_FIRST_FP_REGNUM + 1);
> + regcache->raw_collect (RISCV_CSR_FCSR_REGNUM, fpbuf.buf);
> + }
> }
>
> /* Return a target description for the current target. */
> @@ -166,8 +201,8 @@ const struct target_desc *
> riscv_linux_nat_target::read_description ()
> {
> struct riscv_gdbarch_features features;
> - struct iovec iov;
> elf_fpregset_t regs;
> + int flen;
> int tid;
>
> /* Figuring out xlen is easy. */
> @@ -175,19 +210,40 @@ riscv_linux_nat_target::read_description
>
> tid = inferior_ptid.lwp ();
>
> - iov.iov_base = ®s;
> - iov.iov_len = sizeof (regs);
> + /* Start with no f-registers. */
> + features.flen = 0;
>
> - /* Can we fetch the f-registers? */
> - if (ptrace (PTRACE_GETREGSET, tid, NT_FPREGSET,
> - (PTRACE_TYPE_ARG3) &iov) == -1)
> - features.flen = 0; /* No f-registers. */
> - else
> + /* How much worth of f-registers can we fetch if any? */
> + for (flen = sizeof (regs.__f.__f[0]); ; flen *= 2)
> {
> - /* TODO: We need a way to figure out the actual length of the
> - f-registers. We could have 64-bit x-registers, with 32-bit
> - f-registers. For now, just assumed xlen and flen match. */
> - features.flen = features.xlen;
> + size_t regset_size;
> + struct iovec iov;
> +
> + /* Regsets have a uniform slot size, so we count FSCR like
> + an FP data register. */
> + regset_size = ELF_NFPREG * flen;
> + if (regset_size > sizeof (regs))
> + break;
> +
> + iov.iov_base = ®s;
> + iov.iov_len = regset_size;
> + if (ptrace (PTRACE_GETREGSET, tid, NT_FPREGSET,
> + (PTRACE_TYPE_ARG3) &iov) == -1)
> + {
> + switch (errno)
> + {
> + case EINVAL:
> + continue;
> + case EIO:
> + break;
> + default:
> + perror_with_name (_("Couldn't get registers"));
> + break;
> + }
> + }
> + else
> + features.flen = flen;
> + break;
> }
>
> return riscv_create_target_description (features);
> @@ -228,7 +284,8 @@ riscv_linux_nat_target::fetch_registers
> elf_fpregset_t regs;
>
> iov.iov_base = ®s;
> - iov.iov_len = sizeof (regs);
> + iov.iov_len = ELF_NFPREG * register_size (regcache->arch (),
> + RISCV_FIRST_FP_REGNUM);
I think that this would be made clearer, and more robust if we added
this assertion:
gdb_assert (iov.iov_len <= sizeof (regs));
>
> if (ptrace (PTRACE_GETREGSET, tid, NT_FPREGSET,
> (PTRACE_TYPE_ARG3) &iov) == -1)
> @@ -289,7 +346,8 @@ riscv_linux_nat_target::store_registers
> elf_fpregset_t regs;
>
> iov.iov_base = ®s;
> - iov.iov_len = sizeof (regs);
> + iov.iov_len = ELF_NFPREG * register_size (regcache->arch (),
> + RISCV_FIRST_FP_REGNUM);
And I think we should add the assertion here too.
With these assertions, and assuming you have a suitable copyright
assignment, then feel free to merge this.
Thanks,
Andrew
>
> if (ptrace (PTRACE_GETREGSET, tid, NT_FPREGSET,
> (PTRACE_TYPE_ARG3) &iov) == -1)
More information about the Gdb-patches
mailing list