Re: riscv port for review

[Michael Forney <mforney@mforney.org>]

On 2018-10-18, Michael Forney <mforney@mforney.org> wrote:
> On 2018-10-11, Michael Forney <mforney@mforney.org> wrote:
>> On 2018-09-27, Rich Felker <dalias@libc.org> wrote:
>>> Pulled from here:
>>> https://github.com/riscv/riscv-musl/commit/6a4f4a9c774608add4b02f95322518bd2f5f51ee
>>>
>>> Attached for review.
>>
>> I noticed that some fcntl.h constants are incorrect (O_DIRECTORY,
>> O_NOFOLLOW, O_DIRECT, O_LARGEFILE, and O_TMPFILE). Linux doesn't seem
>> to have a riscv-specific fcntl.h, so I think they just come from
>> asm-generic.
>
> While playing around with a riscv32-linux-musl toolchain and tinyemu,
> I found a few more issues:
>
> - riscv linux has no renameat syscall. I think __NR_renameat needs to
> be removed from arch/riscv*/bits/syscall.h.in, and musl's rename and
> renameat need to fallback to SYS_renameat2 if SYS_renameat is not
> defined.
>
> - arch/riscv32/bits/syscall.h.in defines syscall names as if it were
> 64-bit. I think the following changes are necessary
>     __NR_fcntl -> __NR_fcntl64
>     __NR_statfs -> __NR_statfs64
>     __NR_fstatfs -> __NR_fstatfs64
>     __NR_truncate -> __NR_truncate64
>     __NR_ftruncate -> __NR_ftruncate64
>     __NR_lseek -> __NR__llseek (and __NR_llseek?)
>     __NR_fstatat -> __NR_fstatat64
>     __NR_fstat -> __NR_fstat64
>     __NR_mmap -> __NR_mmap2
>     __NR_fadvise64 -> __NR_fadvise64_64
>
> - Since riscv32 uses fcntl64, and musl's struct flock corresponds to
> struct flock64 on 32-bit, F_GETLK, F_SETLK, and F_SETLKW should be
> defined to the corresponding *64 values (12, 13, 14). This matches
> arch/generic/bits/fcntl.h, so I think arch/riscv32/bits/fcntl.h should
> just be completely removed as Rich suggested.
> arch/riscv64/bits/fcntl.h needs to stay to define the non *64 values
> (5, 6, 7).
>
> - For detecting soft float, configure compares $ARCH to riscv and
> riscv64, but ARCH is set to riscv32 or riscv64 above. Also ARCH is set
> to riscv32 when $target is riscv* but not riscv64*. Should this be
> riscv32*?
>
> - There are several instances of preprocessor checks
> __riscv_soft_float, but this does not seem to be defined by gcc.
> Perhaps this is superseded by __riscv_flen or __riscv_float_abi_soft?
>
> - The functions in src/math/riscv* don't fall back to the C
> implementation for soft float.

Forgot to mention one other thing I found: __SYSCALL_LL_O on riscv32
is defined as if registers pairs need to be aligned for syscalls, but
that does not appear to be the case.

My proposed fixes are available here:
https://github.com/michaelforney/musl/compare/6a4f4a9...riscv

Zach van Rijn has applied those patches in his toolchains at
https://musl.cc, and Fabrice Bellard is using them in his buildroot
port at https://bellard.org/tinyemu/buildroot.html. It'd be nice to
get them applied/squashed into https://github.com/riscv/riscv-musl.

I haven't done any actual review of the port (and I don't actually
know much about riscv). I just ran into these issues while trying to
get some programs to run under linux in tinyemu.