From: Palmer Dabbelt <palmerdabbelt@google.com>
To: dalias@libc.org
Cc: musl@lists.openwall.com
Subject: Re: [musl] riscv32 v2
Date: Wed, 09 Sep 2020 14:28:55 -0700 (PDT) [thread overview]
Message-ID: <mhng-c1942a99-70fd-472a-8123-c7a2d270b7bd@palmerdabbelt-glaptop1> (raw)
In-Reply-To: <20200909202824.GZ3265@brightrain.aerifal.cx>
On Wed, 09 Sep 2020 13:28:27 PDT (-0700), dalias@libc.org wrote:
> On Fri, Sep 04, 2020 at 01:48:19AM -0400, Stefan O'Rear wrote:
>> Changes since v1:
>>
>> Fixed ptrace support by passing through high bits of WSTOPSIG.
>> WEXITSTATUS is still masked (required by POSIX); WTERMSIG is also
>> masked because bits 8-15 have nowhere to go.
>>
>> Added SYS_futex as an alias of SYS_futex_time64.
>>
>> Changed conditionals in patch 2. __wait4 is significantly reorganized
>> and now uses a conditionally defined wrapper in src/internal/syscall.h.
>> Duplication reduced in statx-using patches.
>>
>> Arnd Bergmann's comment about identical fcntl.h files has NOT been
>> addressed.
>>
>> Rich Felker's suggestion (on IRC) to use a 0-instruction __get_tp was
>> NOT implemented after discovering that it generates dramatically worse
>> code on clang and cannot easily be conditionalized. Bug reports to come.
>>
>> Patches other than 2, 6, 7, 10 are unchanged.
>>
>> Testing:
>>
>> Smoke tested on riscv32, replacing the musl libc.so in an
>> OpenEmbedded-generated VM with a dynamically linked systemd and verified
>> boot. Smoke testing on i386 and x86_64 by replacing libc.so in an
>> Alpine chroot and running build tools.
>>
>> libc-test was run on all three architectures. The errors on riscv32
>> are as follows:
>>
>> 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.
>>
>> The test results are identical between master and my branch on x86_64.
>> On i386, I saw a utime.exe and utime-static.exe error but have not
>> managed to reproduce them.
>>
>> I was not able to run LTP on musl on any of the three architectures
>> following the instructions in its README.
>>
>> make autotools && ./configure && make all -j16
>> eventually results in:
>> confstr01.c:51:3: error: '_CS_XBS5_ILP32_OFF32_CFLAGS' undeclared here (not in a function)
>>
>> A cloneable repository with the present version is:
>> git clone https://github.com/sorear/riscv-musl -b rv32_submit_v2
>
>> From 020ccd0e2c77ded655bab68c2b3a0d3dc1151aab Mon Sep 17 00:00:00 2001
>> From: Stefan O'Rear <sorear@fastmail.com>
>> Date: Thu, 3 Sep 2020 03:17:45 -0400
>> Subject: [PATCH 01/14] Remove ARMSUBARCH relic from configure
>
> commit 0f814a4e57e80d2512934820b878211e9d71c93e removed its use.
>
>> From d3c237f0b0f7e5d1d2a53f5382e370ce3f0c493c Mon Sep 17 00:00:00 2001
>> From: Stefan O'Rear <sorear@fastmail.com>
>> Date: Thu, 3 Sep 2020 03:27:03 -0400
>> Subject: [PATCH 02/14] time64: Don't make aliases to nonexistent syscalls
>>
>> riscv32 and future architectures lack the _time32 variants entirely, so
>> don't try to use their numbers.
>
> commit 4bbd7baea7c8538b3fb8e30f7b022a1eee071450 was written with the
> intent that future time64-only archs, including riscv32, not need to
> explicitly define the unadorned syscall names; the logic in
> internal/syscall.h would automatically define them as the
> corresponding _time64 syscall numbers. however, subsequent commits
> beginning with 5a105f19b5aae79dd302899e634b6b18b3dcd0d6 broke this
> when they renamed legacy time32 syscalls externally and introduced
> preprocessor logic in internal/syscall.h to define the unadorned names
> in terms of the renamed _time32 ones.
>
> flip the preprocessor logic for the latter to be dependent on the
> _time32 names being defined. this has the added benefit of producing a
> diagnostic for redefinition if a conflicting definition ever arises.
>
>> From f8cec3f6ff1e0a3737f1b55321e826f2208f940c Mon Sep 17 00:00:00 2001
>> From: Stefan O'Rear <sorear@fastmail.com>
>> Date: Thu, 3 Sep 2020 03:31:05 -0400
>> Subject: [PATCH 03/14] time64: Only getrlimit/setrlimit if they exist
>>
>> riscv32 and future architectures only provide prlimit64.
>> ---
>> src/misc/getrlimit.c | 6 +++++-
>> src/misc/setrlimit.c | 6 +++++-
>> 2 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/misc/getrlimit.c b/src/misc/getrlimit.c
>> index 2ab2f0f4..bf676307 100644
>> --- a/src/misc/getrlimit.c
>> +++ b/src/misc/getrlimit.c
>> @@ -6,12 +6,13 @@
>>
>> int getrlimit(int resource, struct rlimit *rlim)
>> {
>> - unsigned long k_rlim[2];
>> int ret = syscall(SYS_prlimit64, 0, resource, 0, rlim);
>> if (!ret) {
>> FIX(rlim->rlim_cur);
>> FIX(rlim->rlim_max);
>> }
>> +#ifdef SYS_getrlimit
>> + unsigned long k_rlim[2];
>> if (!ret || errno != ENOSYS)
>> return ret;
>> if (syscall(SYS_getrlimit, resource, k_rlim) < 0)
>> @@ -21,6 +22,9 @@ int getrlimit(int resource, struct rlimit *rlim)
>> FIX(rlim->rlim_cur);
>> FIX(rlim->rlim_max);
>> return 0;
>> +#else
>> + return ret;
>> +#endif
>> }
>
> No action required, but this could be improved by moving to __syscall
> with return __syscall_ret(ret) at the end outside the #endif. That's
> an independent change we can make later.
>
>> From 9860fca6d45169b2c299f526243b12bff3f8180e Mon Sep 17 00:00:00 2001
>> From: Stefan O'Rear <sorear@fastmail.com>
>> Date: Thu, 3 Sep 2020 03:33:10 -0400
>> Subject: [PATCH 04/14] time64: Only gettimeofday/settimeofday if exist
>>
>> riscv64 and future architectures only provide the clock_ functions.
>
> Commit message mentions settimeofday but it does not appear in the
> diff. There's presently no fallback for settimeofday anywhere in musl,
> and commit 2c2c3605d3b3ff32902c406d17ac44e7544be4e2 noted that it's
> not needed (although perhaps it would be nice to have anyway?). In any
> case, only action needed now is fixing the commit message.
>
>> From daab92fbd69f7c8e3c0ff6faba142de827d007e6 Mon Sep 17 00:00:00 2001
>> From: Stefan O'Rear <sorear@fastmail.com>
>> Date: Thu, 3 Sep 2020 03:45:08 -0400
>> Subject: [PATCH 05/14] Add src/internal/statx.h
>>
>> We need to make internal syscalls to SYS_statx when SYS_fstatat is not
>> available without changing the musl API.
>
> This wording is confusing. Perhaps just "make struct statx available
> for internal use outside fstatat.c."
>
>> From 9ca6f23f7fcb6a387a394bc09a2aad1971b27857 Mon Sep 17 00:00:00 2001
>> From: Stefan O'Rear <sorear@fastmail.com>
>> Date: Thu, 3 Sep 2020 05:20:45 -0400
>> Subject: [PATCH 07/14] Emulate wait4 using waitid
>>
>> riscv32 and future architectures lack wait4.
>>
>> waitpid is required by POSIX to be a cancellation point. pclose is
>> specified as undefined if a cancellation occurs, so it would be
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> This is not the case. It's specified as an optional cancellation
> point, not UB, but the only possible actions on cancellation are
> incompatible with other requirements of POSIX and with consistency of
> the program state. These essentially impose a requirement that it not
> be a cancellation point, or at least that it can't act on
> cancellation after the close finishes.
>
>> permitted for it to call a cancellable wait function; however, as a
>> quality of implementation matter, pclose must close the pipe fd before
>> it can wait (consider popen("yes","r")) and if the wait could be
>> interrupted the pipe FILE would be left in an intermediate state that
>> portable software cannot recover from, so the only useful behavior is
>> for pclose to NOT be a cancellation point. We therefore support both at
>> a small cost in code size.
>>
>> wait4 is historically not a cancellation point in musl; we retain that
>> since we need the non-cancellable version of __wait4 anyway.
>
> With the above fixed, I don't object to keeping this kind of message,
> but I'd rather focus on (or at least also have) an explanation of why
> this is needed. Key points seem to be that Linux has dropped SYS_wait4
> for new archs, but it's nontrivial to get the semantics needed for
> functions that use waitpid in terms of waitid, and that the rusage
> logic is only needed for wait4() not other functions that use
> SYS_wait4, so a common place to do the conversion is required.
>
>> ---
>> src/internal/__wait4.c | 55 ++++++++++++++++++++++++++++++++++++++++++
>> src/internal/syscall.h | 12 +++++++++
>> src/linux/wait4.c | 2 +-
>> src/process/waitpid.c | 2 +-
>> src/stdio/pclose.c | 2 +-
>> src/unistd/faccessat.c | 6 ++++-
>> 6 files changed, 75 insertions(+), 4 deletions(-)
>> create mode 100644 src/internal/__wait4.c
>>
>> diff --git a/src/internal/__wait4.c b/src/internal/__wait4.c
>> new file mode 100644
>> index 00000000..04d7dc64
>> --- /dev/null
>> +++ b/src/internal/__wait4.c
>> @@ -0,0 +1,55 @@
>> +#include <sys/wait.h>
>> +#include "syscall.h"
>> +
>> +#ifndef SYS_wait4
>> +hidden pid_t __wait4(pid_t pid, int *status, int options, void *kru, int cp)
>
> As mentioned before, I'd like to rename this to __sys_wait4 and make
> macros to call it as __sys_wait4 or __sys_wait4_cp (hiding the last
> argument) via internal/syscall.h (matching __sys_open pattern).
>
> If SYS_wait4 is defined, the macros in internal/syscall.h can then
> directly expand to __syscall[_cp](SYS_wait4, ...). Then the source
> files don't need their own #ifdef's.
>
>> From 3e6bd3fd86883b448fc250d96cde9d37f9efa879 Mon Sep 17 00:00:00 2001
>> From: Stefan O'Rear <sorear@fastmail.com>
>> Date: Thu, 3 Sep 2020 05:23:40 -0400
>> Subject: [PATCH 08/14] riscv: Fall back to syscall __riscv_flush_icache
>>
>> Matches glibc behavior and fixes a case where we could fall off the
>> function without returning a value.
>
> I would highlight in the commit title (first line) that this is fixing
> an actual bug, the case where the vdso function isn't defined.
> Something like:
>
> fix __riscv_flush_icache when vdso function is not available
>
> previously execution fell off the end of the function without
> performing any fallback or returning any value when the vdso
> function was not available.
>
>> From 8aabc20dade2b2c6019f46a528857bb434a38167 Mon Sep 17 00:00:00 2001
>> From: Stefan O'Rear <sorear@fastmail.com>
>> Date: Thu, 3 Sep 2020 05:26:50 -0400
>> Subject: [PATCH 09/14] riscv32: Target and subtarget detection
>
> Having them split out has been ok for review, but I think this and the
> remaining commits can be squashed for upstreaming. They don't
> individually produce consistent states you could build or use.
>
>> From aae7aeed7378f10cba709b6643acbd46f0b36213 Mon Sep 17 00:00:00 2001
>> From: Stefan O'Rear <sorear@fastmail.com>
>> Date: Thu, 3 Sep 2020 05:40:29 -0400
>> Subject: [PATCH 10/14] riscv32: add arch headers
>
> This is the only commit where you had significant informative message
> content, and it should probably be kept but revised slightly to apply
> too the whole port.
>
>> These are mostly copied from riscv64. _Addr and _Reg had to become int
>> to avoid errors in libstdc++ when size_t and std::size_t mismatch.
>
> This is just the psABI, not a libstdc++ issue. Almost all 32-bit archs
> use int rather than long for wordsize types.
>
>> There is no kernel stat struct; the userspace stat matches glibc in the
>> sizes and offsets of all fields (including glibc's __dev_t __pad1). The
>> jump buffer is 12 words larger to account for 12 saved double-precision
>> floats; additionally it should be 64-bit aligned to save doubles.
>
> "Should be" is confusing here and suggests it's not. Maybe explain it
> as the jmp_buf using 64-bit slots so that it remains sufficiently
> aligned for doubles.
>
>> The syscall list was significantly revised by deleting all time32 and
>> pre-statx syscalls, and renaming several syscalls that have different
>> names depending on __BITS_PER_LONG, notably mmap2 and _llseek.
>>
>> futex was added as an alias to futex_time64 since it is widely used by
>> software which does not pass time arguments.
>
> OK.
>
>> diff --git a/arch/riscv32/bits/fcntl.h b/arch/riscv32/bits/fcntl.h
>> new file mode 100644
>> index 00000000..ecb4d18f
>> --- /dev/null
>> +++ b/arch/riscv32/bits/fcntl.h
>> @@ -0,0 +1,38 @@
>> +#define O_CREAT 0100
>> +#define O_EXCL 0200
>> +#define O_NOCTTY 0400
>> +#define O_TRUNC 01000
>> +#define O_APPEND 02000
>> +#define O_NONBLOCK 04000
>> +#define O_DSYNC 010000
>> +#define O_SYNC 04010000
>> +#define O_RSYNC 04010000
>> +#define O_DIRECTORY 0200000
>> +#define O_NOFOLLOW 0400000
>> +#define O_CLOEXEC 02000000
>> +
>> +#define O_ASYNC 020000
>> +#define O_DIRECT 040000
>> +#define O_LARGEFILE 0100000
>> +#define O_NOATIME 01000000
>> +#define O_PATH 010000000
>> +#define O_TMPFILE 020200000
>> +#define O_NDELAY O_NONBLOCK
>> +
>> +#define F_DUPFD 0
>> +#define F_GETFD 1
>> +#define F_SETFD 2
>> +#define F_GETFL 3
>> +#define F_SETFL 4
>> +#define F_GETLK 5
>> +#define F_SETLK 6
>> +#define F_SETLKW 7
>> +#define F_SETOWN 8
>> +#define F_GETOWN 9
>> +#define F_SETSIG 10
>> +#define F_GETSIG 11
>> +
>> +#define F_SETOWN_EX 15
>> +#define F_GETOWN_EX 16
>> +
>> +#define F_GETOWNER_UIDS 17
>
> I think this file can be removed; after fixes it's identical to the
> generic one.
>
>> diff --git a/arch/riscv32/syscall_arch.h b/arch/riscv32/syscall_arch.h
>> new file mode 100644
>> index 00000000..9e916c76
>> --- /dev/null
>> +++ b/arch/riscv32/syscall_arch.h
>> @@ -0,0 +1,78 @@
>> +#define __SYSCALL_LL_E(x) \
>> +((union { long long ll; long l[2]; }){ .ll = x }).l[0], \
>> +((union { long long ll; long l[2]; }){ .ll = x }).l[1]
>> +#define __SYSCALL_LL_O(x) __SYSCALL_LL_E((x))
>> +
>> +#define __asm_syscall(...) \
>> + __asm__ __volatile__ ("ecall\n\t" \
>> + : "=r"(a0) : __VA_ARGS__ : "memory"); \
>> + return a0; \
>> +
>> +static inline long __syscall0(long n)
>> +{
>> + register long a7 __asm__("a7") = n;
>> + register long a0 __asm__("a0");
>> + __asm_syscall("r"(a7))
>> +}
>> +
>> +static inline long __syscall1(long n, long a)
>> +{
>> + register long a7 __asm__("a7") = n;
>> + register long a0 __asm__("a0") = a;
>> + __asm_syscall("r"(a7), "0"(a0))
>> +}
>> +
>> +static inline long __syscall2(long n, long a, long b)
>> +{
>> + register long a7 __asm__("a7") = n;
>> + register long a0 __asm__("a0") = a;
>> + register long a1 __asm__("a1") = b;
>> + __asm_syscall("r"(a7), "0"(a0), "r"(a1))
>> +}
>> +
>> +static inline long __syscall3(long n, long a, long b, long c)
>> +{
>> + register long a7 __asm__("a7") = n;
>> + register long a0 __asm__("a0") = a;
>> + register long a1 __asm__("a1") = b;
>> + register long a2 __asm__("a2") = c;
>> + __asm_syscall("r"(a7), "0"(a0), "r"(a1), "r"(a2))
>> +}
>> +
>> +static inline long __syscall4(long n, long a, long b, long c, long d)
>> +{
>> + register long a7 __asm__("a7") = n;
>> + register long a0 __asm__("a0") = a;
>> + register long a1 __asm__("a1") = b;
>> + register long a2 __asm__("a2") = c;
>> + register long a3 __asm__("a3") = d;
>> + __asm_syscall("r"(a7), "0"(a0), "r"(a1), "r"(a2), "r"(a3))
>> +}
>> +
>> +static inline long __syscall5(long n, long a, long b, long c, long d, long e)
>> +{
>> + register long a7 __asm__("a7") = n;
>> + register long a0 __asm__("a0") = a;
>> + register long a1 __asm__("a1") = b;
>> + register long a2 __asm__("a2") = c;
>> + register long a3 __asm__("a3") = d;
>> + register long a4 __asm__("a4") = e;
>> + __asm_syscall("r"(a7), "0"(a0), "r"(a1), "r"(a2), "r"(a3), "r"(a4))
>> +}
>> +
>> +static inline long __syscall6(long n, long a, long b, long c, long d, long e, long f)
>> +{
>> + register long a7 __asm__("a7") = n;
>> + register long a0 __asm__("a0") = a;
>> + register long a1 __asm__("a1") = b;
>> + register long a2 __asm__("a2") = c;
>> + register long a3 __asm__("a3") = d;
>> + register long a4 __asm__("a4") = e;
>> + register long a5 __asm__("a5") = f;
>> + __asm_syscall("r"(a7), "0"(a0), "r"(a1), "r"(a2), "r"(a3), "r"(a4), "r"(a5))
>> +}
>> +
>> +#define VDSO_USEFUL
>> +/* We don't have a clock_gettime function.
>> +#define VDSO_CGT_SYM "__vdso_clock_gettime"
>> +#define VDSO_CGT_VER "LINUX_2.6" */
>> --
>> 2.25.4
>>
>
> Is this correct? I see the comment is just copied from riscv64, but it
> seems wrong there, and here too. Also, is the vdso function named
> "clock_gettime" or "clock_gettime64" for riscv32? Or is there none at
> all and this macro just wrong?
Looks like we don't have __vdso_clock_gettime on rv32 but we do have one on
rv64. glibc doesn't have the clock VDSO calls on rv32.
I'm not opposed to adding some sort of clock-related VDSO calls on rv32, but it
looks like doing so will require some thought. Maybe it's best to wait on that
so we don't hold up the initial port?
>
> Rich
next prev parent reply other threads:[~2020-09-09 21:33 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-04 5:48 Stefan O'Rear
2020-09-07 10:47 ` Stefan O'Rear
2020-09-07 18:06 ` Rich Felker
2020-09-07 21:35 ` Arnd Bergmann
2020-09-07 21:45 ` Rich Felker
2020-09-07 21:58 ` Arnd Bergmann
2020-09-07 22:11 ` Rich Felker
2020-09-07 22:30 ` Arnd Bergmann
2020-09-08 1:02 ` Rich Felker
2020-09-08 7:00 ` Arnd Bergmann
2020-09-07 11:27 ` Stefan O'Rear
2020-09-07 18:09 ` Rich Felker
2020-09-08 1:54 ` Rich Felker
2020-09-09 6:07 ` Rich Felker
2020-09-09 20:28 ` Rich Felker
2020-09-09 21:28 ` Palmer Dabbelt [this message]
2020-09-09 21:36 ` Rich Felker
2020-09-09 23:08 ` Palmer Dabbelt
2020-09-10 7:36 ` Arnd Bergmann
2020-09-10 10:01 ` Vincenzo Frascino
2020-09-11 0:08 ` Palmer Dabbelt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=mhng-c1942a99-70fd-472a-8123-c7a2d270b7bd@palmerdabbelt-glaptop1 \
--to=palmerdabbelt@google.com \
--cc=dalias@libc.org \
--cc=musl@lists.openwall.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/musl/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).