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=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 31838 invoked from network); 16 May 2022 14:27:21 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 16 May 2022 14:27:21 -0000 Received: (qmail 17967 invoked by uid 550); 16 May 2022 14:27:18 -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 17932 invoked from network); 16 May 2022 14:27:17 -0000 Date: Mon, 16 May 2022 10:27:04 -0400 From: Rich Felker To: =?utf-8?B?546L5rSq5Lqu?= Cc: musl@lists.openwall.com Message-ID: <20220516142704.GR7074@brightrain.aerifal.cx> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] 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 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