[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20220516142704.GR7074@brightrain.aerifal.cx>
Date: Mon, 16 May 2022 10:27:04 -0400
From: Rich Felker <dalias@...c.org>
To: 王洪亮 <wanghongliang@...ngson.cn>
Cc: musl@...ts.openwall.com
Subject: Re: add loongarch64 port v3
On Thu, May 05, 2022 at 11:21:01AM +0800, 王洪亮 wrote:
> Hi,
>
> I published loongarch64 port v3 in
> https://github.com/loongson/musl/tree/loongarch-v2.0.
>
> fixed two problem:
>
> 1.use __NR_clone3 in __clone().
>
> 2.remove __NR_fstat and __NR_newfstatat.
>
> please code review if there are any other problems ? thanks.
>
>
> Hongliang Wang.
OK, review follows:
> diff --git a/arch/loongarch64/bits/signal.h b/arch/loongarch64/bits/signal.h
> new file mode 100644
> index 00000000..e6613516
> --- /dev/null
> +++ 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;
Again, [0] is not valid C. If the extension field is going to be
declared at all it needs to be declared in a way it can be accessed
without invoking UB, e.g. as a FAM. I'm also not clear on how
specifying the alignment here helps since any object created in a way
that the alignment would affect cannot have the FAM present.
Style nit: missing space between } and mcontext_t.
> diff --git a/arch/loongarch64/bits/stat.h b/arch/loongarch64/bits/stat.h
> new file mode 100644
> index 00000000..b7f4221b
> --- /dev/null
> +++ b/arch/loongarch64/bits/stat.h
> @@ -0,0 +1,18 @@
> +struct stat {
> + dev_t st_dev;
> + 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 long __pad;
> + off_t st_size;
> + blksize_t st_blksize;
> + int __pad2;
> + blkcnt_t st_blocks;
> + struct timespec st_atim;
> + struct timespec st_mtim;
> + struct timespec st_ctim;
> + unsigned __unused[2];
> +};
Looks like this matches the 'generic' definition now, which is good.
We haven't yet put a generic vesion in arch/generic/bits/stat.h, but
at some point that should happen and if/when it does this file can be
dropped. It's fine for now though.
> diff --git a/arch/loongarch64/kstat.h b/arch/loongarch64/kstat.h
> new file mode 100644
> index 00000000..900f119d
> --- /dev/null
> +++ b/arch/loongarch64/kstat.h
> @@ -0,0 +1,21 @@
> +struct kstat {
> + dev_t st_dev;
> + 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 long __pad;
> + off_t st_size;
> + blksize_t st_blksize;
> + int __pad2;
> + blkcnt_t st_blocks;
> + long st_atime_sec;
> + unsigned long st_atime_nsec;
> + long st_mtime_sec;
> + unsigned long st_mtime_nsec;
> + long st_ctime_sec;
> + unsigned long st_ctime_nsec;
> + unsigned __unused[2];
> +};
I think this file should be removed, right? -- since there is no
legacy stat sycall with a kstat structure.
> diff --git a/arch/loongarch64/reloc.h b/arch/loongarch64/reloc.h
> new file mode 100644
> index 00000000..249cb40c
> --- /dev/null
> +++ b/arch/loongarch64/reloc.h
> @@ -0,0 +1,35 @@
> +#ifndef __RELOC_H__
> +#define __RELOC_H__
> +
> +#define _GNU_SOURCE
> +#include <endian.h>
You cannot define FTMs in the middle of a header file that might be
included somewhere other than the very top of a source file. But there
does not seem to be any reason it's defined at all.
> diff --git a/src/ldso/loongarch64/dlsym.s b/src/ldso/loongarch64/dlsym.s
> new file mode 100644
> index 00000000..10ac5c7f
> --- /dev/null
> +++ b/src/ldso/loongarch64/dlsym.s
> @@ -0,0 +1,12 @@
> +.global dlsym
> +.hidden __dlsym
> +.type dlsym,@function
> +dlsym:
> + move $a2, $ra
> + la.global $r16, __dlsym
> + addi.d $sp, $sp, -8
> + st.d $ra, $sp, 0
> + jirl $ra, $r16, 0
> + ld.d $ra, $sp, 0
> + addi.d $sp, $sp, 8
> + jirl $r0, $ra, 0
Is there a reason this can't be a tail call? Maybe the ABI requires
that the caller reserve space for the callee to spill arg registers
that are used?
> diff --git a/src/thread/loongarch64/clone.s b/src/thread/loongarch64/clone.s
> new file mode 100644
> index 00000000..ec920c7b
> --- /dev/null
> +++ b/src/thread/loongarch64/clone.s
> @@ -0,0 +1,47 @@
> +#__clone(func, stack, flags, arg, ptid, tls, ctid)
> +# a0, a1, a2, a3, a4, a5, a6
> +# sys_clone3(struct clone_args *cl_args, size_t size)
> +# a0 a1
> +
> +.global __clone
> +.hidden __clone
> +.type __clone,@function
> +__clone:
> + # Save function pointer and argument pointer on new thread stack
> + addi.d $a1, $a1, -16
> + st.d $a0, $a1, 0 # save function pointer
> + st.d $a3, $a1, 8 # save argument pointer
> +
> + li.d $t0, ~0x004000ff # mask CSIGNAL and CLONE_DETACHED
> + and $t1, $a2, $t0 # cl_args.flags
> + li.d $t0, 0x000000ff # CSIGNAL
> + and $t2, $a2, $t0 # cl_args.exit_signal
> +
> + bstrins.d $sp, $zero, 2, 0 # align stack to 8 bytes
> + addi.d $sp, $sp, -88 # struct clone_args
> + st.d $t1, $sp, 0 # flags
> + st.d $a4, $sp, 8 # pidfd
> + st.d $a6, $sp, 16 # child_tid
> + st.d $a4, $sp, 24 # parent_tid
> + st.d $t2, $sp, 32 # exit_signal
> + st.d $a1, $sp, 40 # stack
> + st.d $zero, $sp, 48 # stack_size
> + st.d $a5, $sp, 56 # tls
> + st.d $zero, $sp, 64 # set_tid
> + st.d $zero, $sp, 72 # set_tid_size
> + st.d $zero, $sp, 80 # cgroup
> +
> + move $a0, $sp
> + li.d $a1, 88
> + li.d $a7, 435 # __NR_clone3
> + syscall 0 # call clone3
> +
> + beqz $a0, 1f # whether child process
> + addi.d $sp, $sp, 88
> + jirl $zero, $ra, 0 # parent process return
> +1:
> + ld.d $t8, $sp, 0 # function pointer
> + ld.d $a0, $sp, 8 # argument pointer
> + jirl $ra, $t8, 0 # call the user's function
> + li.d $a7, 93
> + syscall 0 # child process exit
Based on the outcome of the kernel discussiong about __NR_clone3 vs
__NR_clone, this likely needs to be reverted to use __NR_clone again.
I'm waiting to see how that comes out. Sorry for the back and forth
while we try to get this right on the kernel side.
If anyone else wants to take a further look at this, the things I have
not reviewed in detail are the arch-specific asm, particularly atomics
and fenv stuff that require knowledge of how the arch works. As best I
can tell these don't affect ABI and are fixable after merge if
something doesn't work right, so I'm not too concerned about them.
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.
