[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200909202824.GZ3265@brightrain.aerifal.cx>
Date: Wed, 9 Sep 2020 16:28:27 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: riscv32 v2
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@...tmail.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@...tmail.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@...tmail.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@...tmail.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@...tmail.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@...tmail.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@...tmail.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@...tmail.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@...tmail.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?
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
