Re: riscv port for review

[Michael Clark <michaeljclark@mac.com>]

[-- Attachment #1: Type: text/plain, Size: 4344 bytes --]

Hi Rich,

> On 28/09/2018, at 2:47 PM, Rich Felker <dalias@libc.org> wrote:
> 
>> On Thu, Sep 27, 2018 at 10:24:04PM -0400, Rich Felker wrote:
>> Pulled from here:
>> https://github.com/riscv/riscv-musl/commit/6a4f4a9c774608add4b02f95322518bd2f5f51ee
>> 
>> Attached for review.
> 
>> diff --git a/arch/riscv32/bits/alltypes.h.in b/arch/riscv32/bits/alltypes.h.in
>> new file mode 100644
>> index 0000000..66ca18a
>> --- /dev/null
>> +++ b/arch/riscv32/bits/alltypes.h.in
>> @@ -0,0 +1,26 @@
>> +#define _Addr int
>> +#define _Int64 long long
>> +#define _Reg int
>> +
>> +TYPEDEF __builtin_va_list va_list;
>> +TYPEDEF __builtin_va_list __isoc_va_list;
>> +
>> +#ifndef __cplusplus
>> +TYPEDEF int wchar_t;
>> +#endif
>> +
>> +TYPEDEF float float_t;
>> +TYPEDEF double double_t;
>> +
>> +TYPEDEF struct { long long __ll; long double __ld; } max_align_t;
>> +
>> +TYPEDEF long time_t;
> 
> Is riscv32 time_t really 32-bit? If so that's really disappointing,
> but presumably unfixable...

This definitely is fixable as the riscv32 Linux ABI is not final. This is the first time a Linux libc ABI expert has looked closely at the musl port, and while riscv32 is present, all of my own testing has been performed on riscv64 Linux.

The RISC-V Glibc port currently only contains riscv64. The riscv32 ABI is still in the process of being finalised. Palmer likely knows the exact status. SiFive doesn’t have any 32-bit cores with MMUs so it hasn’t been high on the list of priorities.

In any case, now is a very good time to do some cross-checking between musl riscv32, qemu-riscv32, the glibc riscv32 port and riscv32 linux-kernel. I think we are just getting the default asm-generic in QEMU as we have not done anything special... yet...

I believe there is still an alignment issue with the stat structure in qemu-riscv32. When we last looked at this issue earlier in the year we didn’t have linux-kernel support for riscv32, as the toolchain/ABI focus this year was on finalising riscv64 in Glibc and Linux.

Debian and Fedora only have riscv64 ports and afaik riscv64 is all that is present in Glibc. Of course linux-kernel is authoritative for the ABI so we need to run tests using riscv32 Linux in full system emulation in qemu-system-riscv32; qemu-riscv32 also needs to be audited. At the time we were submitting qemu upstream we could build riscv32 kernels. This has solved and we need to get regular build and test for riscv32 and riscv64 Linux and QEMU in CI...

There is an open bug on riscv-qemu stat which we can look at now that linux-kernel has initial riscv32 support. We should write a test that prints offsetof and sizeof for the stat structure members using musl, glibc and linux headers to find out what’s happening...

I think there is still some time to nail down any remaining issues with riscv32. We definitely want 64-bit time_t and any other types that should be 64-bit should be audited now. Given the indirection through multiple levels of typedefs it’s not immediately clear without a bit of analysis.

We may do the same thing as was done with glibc and split the port up, first submitting riscv64, which is locked down.

I also had a question regarding whether we needed to #define __ARCH_WANT_STAT64 in linux/arch/riscv/include/asm/unistd.h or whether this is a legacy flag. All of the other 32-bit ports define it, but I do not know if it is necessary for a legacy free 32-bit port:

https://github.com/torvalds/linux/blob/master/arch/riscv/include/asm/unistd.h

BTW Thanks very much for the review... I’ll go through the remainder of the patch review comments and work on another revision... __riscv_xlen contains either 32 or 64 at present and is how RISC-V code distinguishes between riscv32 and riscv64. We can remove references to it from the musl riscv ports due to them not sharing code. Also, there is no official RISC-V big endian so we can remove EB. Big endian may be considered at some point, and it may well have appeared in a previous draft of the ISA manual, but at present, it is not defined in the ISA manual or toolchain, so we’ll remove it.

We’ll do some 32-bit testing... now is a good time. SiFive’s Linux board, the HiFive Unleashed, is riscv64, which likely explains the situation with riscv32.

Thanks,
Michael

[-- Attachment #2: Type: text/html, Size: 7140 bytes --]