[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20220322125933.GB7074@brightrain.aerifal.cx>
Date: Tue, 22 Mar 2022 08:59:36 -0400
From: Rich Felker <dalias@...c.org>
To: 王洪亮 <wanghongliang@...ngson.cn>
Cc: musl@...ts.openwall.com, liuxue@...ngson.cn, lvjianmin@...ngson.cn
Subject: Re: add loongarch64 port
On Tue, Mar 22, 2022 at 11:52:35AM +0800, 王洪亮 wrote:
> Hi,
>
> Thank you for give us some useful suggestions on the first submit
>
> from 翟小娟 <zhaixiaojuan@...ngson.cn>.
>
> we review code and make changes on code specification, code style
>
> and code errors.
>
>
> The new patch has been published in
>
> https://github.com/loongson/musl/tree/loongarch-v1.0.
>
>
> The linux kernel has been published in
>
> https://github.com/loongson/linux/tree/loongarch-next.
>
>
> we alse published gcc, glibc in https://github.com/loongson
>
> and submitting to the community.
>
>
> we can supply a physical machine remote login for test musl.
>
> It has been compiled and run the libc-test successfully.
>
>
> Please code review, thanks!
>
>
> Hongliang Wang
>
> >From f5df725607675b370daec09a14ad130c792fa0d2 Mon Sep 17 00:00:00 2001
> From: wanghongliang <wanghongliang@...ngson.cn>
> Date: Mon, 21 Mar 2022 06:13:20 +0800
> Subject: [PATCH] add loongarch64 port.
>
> Author: xiaojuanZhai <zhaixiaojuan@...ngson.cn>
> Author: meidanLi <limeidan@...ngson.cn>
> Author: guoqiChen <chenguoqi@...ngson.cn>
> Author: xiaolinZhao <zhaoxiaolin@...ngson.cn>
> Author: Fanpeng <fanpeng@...ngson.cn>
> Author: jiantaoShan <shanjiantao@...ngson.cn>
> Author: xuhuiQiang <qiangxuhui@...ngson.cn>
> Author: jingyunHua <huajingyun@...ngson.cn>
> Author: liuxue <liuxue@...ngson.cn>
> Author: wanghongliang <wanghongliang@...ngson.cn>
>
> Signed-off-by: wanghongliang <wanghongliang@...ngson.cn>
> ---
> arch/loongarch64/atomic_arch.h | 53 ++++
> arch/loongarch64/bits/alltypes.h.in | 20 ++
> arch/loongarch64/bits/fcntl.h | 40 +++
> arch/loongarch64/bits/fenv.h | 20 ++
> arch/loongarch64/bits/float.h | 16 ++
> arch/loongarch64/bits/hwcap.h | 14 +
> arch/loongarch64/bits/posix.h | 2 +
> arch/loongarch64/bits/ptrace.h | 4 +
> arch/loongarch64/bits/reg.h | 66 +++++
> arch/loongarch64/bits/setjmp.h | 1 +
> arch/loongarch64/bits/signal.h | 79 ++++++
> arch/loongarch64/bits/stat.h | 20 ++
> arch/loongarch64/bits/stdint.h | 20 ++
> arch/loongarch64/bits/syscall.h.in | 307 +++++++++++++++++++++
> arch/loongarch64/bits/user.h | 5 +
> arch/loongarch64/crt_arch.h | 14 +
> arch/loongarch64/kstat.h | 21 ++
> arch/loongarch64/pthread_arch.h | 13 +
> arch/loongarch64/reloc.h | 35 +++
> arch/loongarch64/syscall_arch.h | 137 +++++++++
> configure | 1 +
> crt/loongarch64/crti.s | 15 +
> crt/loongarch64/crtn.s | 12 +
> include/elf.h | 66 +++++
> src/fenv/loongarch64/fenv.S | 72 +++++
> src/ldso/loongarch64/dlsym.s | 12 +
> src/setjmp/loongarch64/longjmp.S | 37 +++
> src/setjmp/loongarch64/setjmp.S | 34 +++
> src/signal/loongarch64/restore.s | 10 +
> src/signal/loongarch64/sigsetjmp.s | 29 ++
> src/thread/loongarch64/__set_thread_area.s | 7 +
> src/thread/loongarch64/__unmapself.s | 7 +
> src/thread/loongarch64/clone.s | 28 ++
> src/thread/loongarch64/syscall_cp.s | 29 ++
> 34 files changed, 1246 insertions(+)
> create mode 100644 arch/loongarch64/atomic_arch.h
> create mode 100644 arch/loongarch64/bits/alltypes.h.in
> create mode 100644 arch/loongarch64/bits/fcntl.h
> create mode 100644 arch/loongarch64/bits/fenv.h
> create mode 100644 arch/loongarch64/bits/float.h
> create mode 100644 arch/loongarch64/bits/hwcap.h
> create mode 100644 arch/loongarch64/bits/posix.h
> create mode 100644 arch/loongarch64/bits/ptrace.h
> create mode 100644 arch/loongarch64/bits/reg.h
> create mode 100644 arch/loongarch64/bits/setjmp.h
> create mode 100644 arch/loongarch64/bits/signal.h
> create mode 100644 arch/loongarch64/bits/stat.h
> create mode 100644 arch/loongarch64/bits/stdint.h
> create mode 100644 arch/loongarch64/bits/syscall.h.in
> create mode 100644 arch/loongarch64/bits/user.h
> create mode 100644 arch/loongarch64/crt_arch.h
> create mode 100644 arch/loongarch64/kstat.h
> create mode 100644 arch/loongarch64/pthread_arch.h
> create mode 100644 arch/loongarch64/reloc.h
> create mode 100644 arch/loongarch64/syscall_arch.h
> create mode 100644 crt/loongarch64/crti.s
> create mode 100644 crt/loongarch64/crtn.s
> create mode 100644 src/fenv/loongarch64/fenv.S
> create mode 100644 src/ldso/loongarch64/dlsym.s
> create mode 100644 src/setjmp/loongarch64/longjmp.S
> create mode 100644 src/setjmp/loongarch64/setjmp.S
> create mode 100644 src/signal/loongarch64/restore.s
> create mode 100644 src/signal/loongarch64/sigsetjmp.s
> create mode 100644 src/thread/loongarch64/__set_thread_area.s
> create mode 100644 src/thread/loongarch64/__unmapself.s
> create mode 100644 src/thread/loongarch64/clone.s
> create mode 100644 src/thread/loongarch64/syscall_cp.s
>
> diff --git a/arch/loongarch64/atomic_arch.h b/arch/loongarch64/atomic_arch.h
> new file mode 100644
> index 00000000..bf4805c9
> --- /dev/null
> +++ b/arch/loongarch64/atomic_arch.h
> @@ -0,0 +1,53 @@
> +#define a_ll a_ll
> +static inline int a_ll(volatile int *p)
> +{
> + int v;
> + __asm__ __volatile__ (
> + "ll.w %0, %1"
> + : "=r"(v)
> + : "ZC"(*p));
> + return v;
> +}
> +
> +#define a_sc a_sc
> +static inline int a_sc(volatile int *p, int v)
> +{
> + int r;
> + __asm__ __volatile__ (
> + "sc.w %0, %1"
> + : "=r"(r), "=ZC"(*p)
> + : "0"(v) : "memory");
> + return r;
> +}
> +
> +#define a_ll_p a_ll_p
> +static inline void *a_ll_p(volatile void *p)
> +{
> + void *v;
> + __asm__ __volatile__ (
> + "ll.d %0, %1"
> + : "=r"(v)
> + : "ZC"(*(void *volatile *)p));
> + return v;
> +}
> +
> +#define a_sc_p a_sc_p
> +static inline int a_sc_p(volatile void *p, void *v)
> +{
> + long r;
> + __asm__ __volatile__ (
> + "sc.d %0, %1"
> + : "=r"(r), "=ZC"(*(void *volatile *)p)
> + : "0"(v)
> + : "memory");
> + return r;
> +}
> +
> +#define a_barrier a_barrier
> +static inline void a_barrier()
> +{
> + __asm__ __volatile__ ("dbar 0" : : : "memory");
> +}
> +
> +#define a_pre_llsc a_barrier
> +#define a_post_llsc a_barrier
I don't see anything wrong here standing out, but haven't reviewed the
ISA's requirements on the use of ll/sc to be sure. If anyone else has
knowledge to review this part that would be great.
> diff --git a/arch/loongarch64/bits/alltypes.h.in b/arch/loongarch64/bits/alltypes.h.in
> new file mode 100644
> index 00000000..38871b5f
> --- /dev/null
> +++ b/arch/loongarch64/bits/alltypes.h.in
> @@ -0,0 +1,20 @@
> +#define _Addr long
> +#define _Int64 long
> +#define _Reg long
> +
> +#define __BYTE_ORDER 1234
> +#define __LONG_MAX 0x7fffffffffffffffL
> +
> +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 unsigned nlink_t;
va_list and __isoc_va_list are no longer defined by the arch's
alltypes.h.in, so they can be removed. I think the rest of that is ok.
> diff --git a/arch/loongarch64/bits/fcntl.h b/arch/loongarch64/bits/fcntl.h
> new file mode 100644
> index 00000000..9bcbb7ff
> --- /dev/null
> +++ b/arch/loongarch64/bits/fcntl.h
> @@ -0,0 +1,40 @@
> +#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 020000000
> +#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_SETOWN 8
> +#define F_GETOWN 9
> +#define F_SETSIG 10
> +#define F_GETSIG 11
> +
> +#define F_GETLK 5
> +#define F_SETLK 6
> +#define F_SETLKW 7
> +
> +#define F_SETOWN_EX 15
> +#define F_GETOWN_EX 16
> +
> +#define F_GETOWNER_UIDS 17
AFAICT this file is identical to the generic one (with 32/64 bit #if
evaluated already) so it can and should be dropped.
> +++ b/arch/loongarch64/bits/signal.h
> @@ -0,0 +1,79 @@
> +#if defined(_POSIX_SOURCE) || defined(_POSIX_C_SOURCE) \
> + || defined(_XOPEN_SOURCE) || defined(_GNU_SOURCE) || defined(_BSD_SOURCE)
> +
> +#if defined(_XOPEN_SOURCE) || defined(_GNU_SOURCE) || defined(_BSD_SOURCE)
> +#define MINSIGSTKSZ 4096
> +#define SIGSTKSZ 16384
> +#endif
> +
> +typedef unsigned long greg_t, gregset_t[32];
> +
> +typedef struct sigcontext {
> + unsigned long pc;
> + gregset_t gregs;
> + unsigned int flags;
> + unsigned long extcontext[0] __attribute__((__aligned__(16)));
> +}mcontext_t;
[0] is not valid, and having a flexible array member here is possibly
not even useful since I don't think it would be valid to access it via
uc->uc_mcontext.extcontext[] since the instance of mcontext_t inside
the ucontext struct does not have FAM space belonging to it, even if
additional space was allocated past the end of the ucontext_t. In
other words, I think compilers would be justified in treating attempts
to access it this way as UB and optimizing them out.
> diff --git a/arch/loongarch64/bits/stat.h b/arch/loongarch64/bits/stat.h
> new file mode 100644
> index 00000000..b620e142
> --- /dev/null
> +++ b/arch/loongarch64/bits/stat.h
> @@ -0,0 +1,20 @@
> +struct stat {
> + dev_t st_dev;
> + int __pad1[3];
> + ino_t st_ino;
> + mode_t st_mode;
> + nlink_t st_nlink;
> + uid_t st_uid;
> + gid_t st_gid;
> + dev_t st_rdev;
> + unsigned int __pad2[2];
> + off_t st_size;
> + int __pad3;
> + struct timespec st_atim;
> + struct timespec st_mtim;
> + struct timespec st_ctim;
> + blksize_t st_blksize;
> + unsigned int __pad4;
> + blkcnt_t st_blocks;
> + int __pad5[14];
> +};
__pad1[3] looks wrong -- st_ino will be aligned on a 64-bit boundary
so there can't be an odd number of 32-bit ints before it. Naturally,
it will be aligned with additional padding, but the intent here was
probably to make all the padding explicit. Moreover, this isn't
reflecting a kernel structure, just a userspace type that musl gets to
define, so I'm not sure why it's being defined for a new arch with
lots of gratuitous padding rather than just using a generic
definition. I guess it was just copied from the mips64 one (which also
has these problems).
If you're already using this in production and don't want to break
ABI, I don't see any real need to change it though. It doesn't really
matter. Making it __pad1[4] would still be a good idea (and we should
fix that on mips64) -- that doesn't break ABI.
> diff --git a/arch/loongarch64/pthread_arch.h b/arch/loongarch64/pthread_arch.h
> new file mode 100644
> index 00000000..27f50e4c
> --- /dev/null
> +++ b/arch/loongarch64/pthread_arch.h
> @@ -0,0 +1,13 @@
> +static inline uintptr_t __get_tp()
> +{
> + uintptr_t tp;
> + __asm__ ("or %0, $tp, $zero" : "=r" (tp) );
> + return tp;
> +}
> +
> +#define TLS_ABOVE_TP
> +#define GAP_ABOVE_TP 0
> +
> +#define DTP_OFFSET 0
> +
> +#define MC_PC pc
This can be done as something like:
static inline uintptr_t __get_tp()
{
register uintptr_t tp __asm__("tp");
__asm__ ("" : "=r" (tp) );
return tp;
}
if the compiler doesn't botch it. Historically clang had trouble with
that, so if that's the case, just stick with what you had -- it's not
too costly doing a gratuitous move.
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.
