Re: RISCV NFPREG?

["Maciej W. Rozycki" <macro at wdc dot com>]

On Wed, 9 Oct 2019, Jim Wilson wrote:

> >  We have this:
> > #define ELF_NFPREG      NFPREG
> > in sysdeps/unix/sysv/linux/riscv/bits/procfs.h, however NFPREG is nowhere
> > defined.  This has come from your initial commit 7f33b09c65e3 ("RISC-V:
> > Linux ABI").  Do you happen to remember what the intent was here if any?
> 
> I don't know the history here, but looking at an old pre-upstream copy
> of the glibc port, I see in ucontext.h
> 
> /* Number of general registers.  */
> #define NGREG   32
> #define NFPREG  32
> 
> and
> 
> /* Container for all general registers.  */
> typedef greg_t gregset_t[NGREG];
> 
> /* Container for all FPU registers.  */
> typedef fpreg_t fpregset_t[NFPREG];

 Right, this isn't exactly right.

> So it was probably dropped when fpregset was expanded to include fcsr.
> That poses a problem, because we have 32 FP regs which may be 32-bit,
> 64-bit, or 128-bit, and 1 fcsr reg which is always 32-bit, so how
> exactly do we count the number of FP regs when we have two different
> types of FP registers that may be of two different sizes?  It is
> simpler to say that we don't have a fixed number of FP registers and
> not define NFPREG.  That forces people to parse the fpregset
> structure, instead of trying to index into it as an array of registers
> which doesn't work.  When NFPREG was dropped, they probably missed the
> use in procfs.h.  I had to deal with some of these problems when I
> added the FP reg ptrace support to the linux kernel and gdb.  Anyways,
> I'd suggest just removing the useless NFPREG stuff in profcs.h.

 These are all good points.

 Please observe however that for ELF we actually have 33 uniform FPR slots 
in the NT_PRFPREG core file note, because this is how Linux regsets work, 
even though the FCSR slot is only partially occupied; consequently this 
also applies to PTRACE_GETREGSET/PTRACE_SETREGSET ptrace(2) requests.

 Here's how the kernel definition of the regset looks like:

	[REGSET_F] = {
		.core_note_type = NT_PRFPREG,
		.n = ELF_NFPREG,
		.size = sizeof(elf_fpreg_t),
		.align = sizeof(elf_fpreg_t),
		.get = &riscv_fpr_get,
		.set = &riscv_fpr_set,
	},

and then arch/riscv/include/uapi/asm/elf.h has this:

#define ELF_NFPREG (sizeof(struct __riscv_d_ext_state) / sizeof(elf_fpreg_t))

(which amounts to 33).  And given that this is a Linux's user API header 
technically we have this macro as a part of the API already.

> I'm puzzled by your gdb comment though, because as far as I know the
> riscv gdb port isn't using NFPREG, and shouldn't need it.  Though
> maybe you are working on the missing gdbserver support for RISC-V?

 Correct; I've been cleaning up some corresponding native GDB/Linux stuff 
too, which can ultimately be shared (I'll do that as a part of the patch 
series).

>  If
> you do readd NFPREG or __NFPREG it is probably worth adding a comment
> explaining that you can't just iterate through the fpregset as an
> array because of the different sized fcsr register.

 Correct, but PTRACE_GETREGSET/PTRACE_SETREGSET requests need to have the 
FPR regset size specified as `33 * flen'.

 So I think we should follow Linux and either include <asm/elf.h> in 
<bits/procfs.h> for the kernel-provided definition or fix our own.

  Maciej