From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-9.7 required=5.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,URIBL_BLACK,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 24053 invoked from network); 9 Sep 2020 21:33:19 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 9 Sep 2020 21:33:19 -0000 Received: (qmail 16067 invoked by uid 550); 9 Sep 2020 21:33:17 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 13692 invoked from network); 9 Sep 2020 21:29:08 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:subject:in-reply-to:cc:from:to:message-id:mime-version :content-transfer-encoding; bh=mnjIVpScEcp6dK/UsqEI01Zy7ZZiTV7IM5XTmPoqz1k=; b=gFdrlob+T9KKCJror6CvMqoi/uuiw/dKmM29WK6brfd7gRirHQyanJaoMWCuBsqPlF uhNF/0oaUP61CF+H1gSbF0CK8GKKwnj5JFZtS/7AQsRnHpq7YCSjBX3w9/lzdWHW6e4v 9rLoo1CSVaKdgDtULGTgNDXoLWo2mMXo1aCmiR2RfUV2XqhIciIlnn5EV8Z1XCF6GN+1 anlrDipPrW1WuJVbIsIdxvYDmfw8udg5APC/Vl2MzKjOIJVZv/btHT6Jq4LfgL9mZSFA 2o8w3m+85/sliTGM7F8Mb8v3eL58CJPbw9FPo9dhwFsk/sjPYUhdqCshC7G/bH1CSOP6 ceoQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:subject:in-reply-to:cc:from:to:message-id :mime-version:content-transfer-encoding; bh=mnjIVpScEcp6dK/UsqEI01Zy7ZZiTV7IM5XTmPoqz1k=; b=TqS9BOMVgnVRJInqq0uZJZhyu1HLmyTU4ZCipXeDSq+8hmCd2K72j+l2eB4URbicec CoqH+7fqDL5dIiVBz93C7EXE9P/8a/+JDXcglYC0qgpJTxPI3xFsq/+GE9j40M0kfT/j FMwQGrpaZU8xovQgepp9CbTYsCvLi6NmpMzkrnsqz2aPCTJ8WWUpiRWefM9EMBHboznc qzgGhsxKi5lM1ArsBacXLJb0LPwkY8yKf4OJtThc2WQbc1rK8fkSpbMOua+KaM8Lzn1y 7jxafc3KMP4GEGIIS1wMtpLfFOE8Rq10dOcEz9rBQmZqjSgMe02OTPkheWUkw5jRpyo3 tpXA== X-Gm-Message-State: AOAM5314m40KSQ6QUULXeowygdTmsJolhsFV66qrg3xtguXWbQX/zE9/ AT6gAKBBGFCTamQqcF7ojyk6SzodcDz47zMU X-Google-Smtp-Source: ABdhPJzywnkh2RFJYHmpfB7YLvp81QkK0SqoR8wlvU5o4IE83gP7yFaF5NyGkv4MkziJnZXYWV+Jxg== X-Received: by 2002:a63:1644:: with SMTP id 4mr2004379pgw.232.1599686935699; Wed, 09 Sep 2020 14:28:55 -0700 (PDT) Date: Wed, 09 Sep 2020 14:28:55 -0700 (PDT) X-Google-Original-Date: Wed, 09 Sep 2020 14:28:50 PDT (-0700) In-Reply-To: <20200909202824.GZ3265@brightrain.aerifal.cx> CC: musl@lists.openwall.com From: Palmer Dabbelt To: dalias@libc.org Message-ID: Mime-Version: 1.0 (MHng) Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [musl] riscv32 v2 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 >> 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 >> 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 >> 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 >> 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 >> 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 >> 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 >> +#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 >> 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 >> 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 >> 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