mailing list of musl libc
 help / color / mirror / code / Atom feed
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

  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).