Re: [musl] riscv32 v2

[Rich Felker <dalias@libc.org>]

On Mon, Sep 07, 2020 at 06:47:00AM -0400, Stefan O'Rear wrote:
> On Fri, Sep 4, 2020, at 1:48 AM, Stefan O'Rear wrote:
> > FAIL src/api/main.exe [status 1]
> > FAIL src/functional/fcntl-static.exe [status 1]
> > FAIL src/functional/fcntl.exe [status 1]
> > FAIL src/functional/ipc_msg-static.exe [status 1]
> > FAIL src/functional/ipc_msg.exe [status 1]
> > FAIL src/functional/ipc_sem-static.exe [status 1]
> > FAIL src/functional/ipc_sem.exe [status 1]
> > FAIL src/functional/ipc_shm-static.exe [status 1]
> > FAIL src/functional/ipc_shm.exe [status 1]
> > FAIL src/functional/strptime-static.exe [status 1]
> > FAIL src/functional/strptime.exe [status 1]
> > FAIL src/math/fma.exe [status 1]
> > FAIL src/math/fmaf.exe [status 1]
> > FAIL src/math/powf.exe [status 1]
> > FAIL src/regression/malloc-brk-fail-static.exe [status 1]
> > FAIL src/regression/malloc-brk-fail.exe [status 1]
> > FAIL src/regression/pthread_atfork-errno-clobber-static.exe [status 1]
> > FAIL src/regression/pthread_atfork-errno-clobber.exe [status 1]
> > 
> > The fcntl and sysvipc errors do not correspond to any error in x86_64
> > and potentially require investigation, although they could be kernel
> > configuration issues.  x86_64 has a different but overlapping set of
> > math errors; qemu is known to not give bit-exact results for RISC-V
> > floating point.  The malloc, pthread, and src/api/main.exe failures
> > match failures on x86_64.
> 
> Attached patch reaches test failure parity between riscv32 and riscv64
> and will be included in v3.
> 
> * gdb HEAD wants ELF_NFPREG, so I set it in bits/user.h to the value
>   gdb needs.  (glibc does #define ELF_NFPREG NFPREG and expects gdb
>   to define NFPREG. I don't get this.)

Ick. Indeed I think this is wrong/probably an oversight in glibc, and
the way you've done it sounds better.

> * Restore accidentally removed errno setting in waitpid, fixes a gdb
>   assertion failure.

OK. As an aside (I haven't gotten to sending review for this yet,
sorry) I think I'd prefer to name the function __sys_wait4 or similar
to make it more clear that it's analogous to __sys_open[23] etc. (a
function or macro emulating the syscall) and not a namespace-safe
version of wait4. (musl's having __wait be a futex wait makes this
even more confusing, btw)

Perhaps also leave the int cp argument to the function but make
separate __sys_wait4 and __sys_wait4_cp macros to call it via so that
there's not a mysterious boolean argument that doesn't correspond to
an actual syscall argument. (This would also be parallel with how
__sys_open[23] is done.)

> * Zero IPC_64 because the kernel only recognizes one set of IPC commands.

OK.

> * Copy the IPC_TIME64 bits from arch/arm/bits to trigger the musl code
>   for fixing time64 IPC_STAT results.  I'm not super happy with this,
>   maybe there should be a new mechanism in musl for fixing IPC_STAT for
>   unconditionally-time64 architectures.

If the riscv32 IPC syscalls don't actually provide in-place time64 but
require translation, I think it's fairly appropriate as-is.

From the definitions in your patch, it looks like all the time fields
are fixed-word-order (little endian) and possibly not aligned, so it
seems like they can't be used in-place. Is this correct?

> * riscv32 _does_ provide both F_GETLK and F_GETLK32; make sure we use
>   the right one.

IIRC someone already suggested using the generic bits/fcntl.h, which
would have solved this. I also have unpushed changed that let 64-bit
archs share the generic bits/fcntl.h too, via #if on __LONG_MAX.

Rich