RISCV NFPREG?

RISCV NFPREG?

[Maciej W. Rozycki]

Palmer,

 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?

 We also have:

#define ELF_NGREG	NGREG

(in the same place) and then:

# define NGREG	32

in sysdeps/unix/sysv/linux/riscv/sys/ucontext.h, however under __USE_MISC 
only.

 We don't use any of these macros and the way they have been defined means 
they're mostly useless to user programs.  All the other architectures 
define them unconditionally and then do use them one way or another in 
public headers.

 Unfortunately I'll have to work it around now in GDB for backwards 
compatibility, but has there been any plan here that was interrupted and 
never completed?

 If not, I'll make some patches to fix it up, by defining __NGREG and 
__NFPREG unconditionally as some targets do and use these to fix up the 
mess.

  Maciej

Re: RISCV NFPREG?

[Jim Wilson]

On Tue, Oct 8, 2019 at 10:42 PM Maciej W. Rozycki <macro@wdc.com> 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];

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.

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?  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.

Jim

Re: RISCV NFPREG?

[Maciej W. Rozycki]

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

Re: RISCV NFPREG?

[Jim Wilson]

On Thu, Oct 10, 2019 at 7:41 AM Maciej W. Rozycki <macro@wdc.com> wrote:
>  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.

I wrote this linux kernel ptrace support.  I did it this way because I
didn't see a better way to do it.  But if you look at riscv_fpr_get
and riscv_fpr_set, you will see that I'm using two copyout/copyin
calls to copy the two different types of registers.  I'm not making
any assumptions about the number of registers there.  We probably do
have to accept that we have 33 FP registers, but we need to be careful
about what exactly this means, which is why I suggested adding
comments to document that this is really 32 FP regs and 1 FCSR which
is not the same thing as 33 FP regs.  I'm not an expert with the linux
kernel/glibc interface, so I don't understand all the implications
here.  I'm not objecting to your changes.  I'm just trying to provide
a little history.

Jim

Re: RISCV NFPREG?

[Palmer Dabbelt]

On Tue, 08 Oct 2019 22:41:48 PDT (-0700), macro@wdc.com wrote:
> Palmer,
>
>  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?
>
>  We also have:
>
> #define ELF_NGREG	NGREG
>
> (in the same place) and then:
>
> # define NGREG	32
>
> in sysdeps/unix/sysv/linux/riscv/sys/ucontext.h, however under __USE_MISC
> only.
>
>  We don't use any of these macros and the way they have been defined means
> they're mostly useless to user programs.  All the other architectures
> define them unconditionally and then do use them one way or another in
> public headers.
>
>  Unfortunately I'll have to work it around now in GDB for backwards
> compatibility, but has there been any plan here that was interrupted and
> never completed?
>
>  If not, I'll make some patches to fix it up, by defining __NGREG and
> __NFPREG unconditionally as some targets do and use these to fix up the
> mess.

That seems fine.  This looks like cruft that's left over from before we 
properly sorted out the multilib stuff.

On Thu, 10 Oct 2019 15:12:31 PDT (-0700), Jim Wilson wrote:
> On Thu, Oct 10, 2019 at 7:41 AM Maciej W. Rozycki <macro@wdc.com> wrote:
>>  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.
>
> I wrote this linux kernel ptrace support.  I did it this way because I
> didn't see a better way to do it.  But if you look at riscv_fpr_get
> and riscv_fpr_set, you will see that I'm using two copyout/copyin
> calls to copy the two different types of registers.  I'm not making
> any assumptions about the number of registers there.  We probably do
> have to accept that we have 33 FP registers, but we need to be careful
> about what exactly this means, which is why I suggested adding
> comments to document that this is really 32 FP regs and 1 FCSR which
> is not the same thing as 33 FP regs.  I'm not an expert with the linux
> kernel/glibc interface, so I don't understand all the implications
> here.  I'm not objecting to your changes.  I'm just trying to provide
> a little history.

That's the right way to do it.  The ptrace stuff matches mcontext, and none of 
that has anything to do with these #defines any more -- IIRC, before we sorted 
out multilib we had different mcontexts for the different ABIs, but we've since 
fixed that as it doesn't work.