Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 24 Mar 2022 10:22:42 +0800
From: 王洪亮 <wanghongliang@...ngson.cn>
To: musl@...ts.openwall.com
Subject: Re: add loongarch64 port

Hi,

I fixed these problems pointed out.

1. delete va_list and __isoc_va_list in arch's alltypes.h.in,
add blksize_t for struct stat and struct kstat.

diff --git a/arch/loongarch64/bits/alltypes.h.in 
b/arch/loongarch64/bits/alltypes.h.in
index 38871b5f..7c02c63c 100644
--- a/arch/loongarch64/bits/alltypes.h.in
+++ b/arch/loongarch64/bits/alltypes.h.in
@@ -5,9 +5,6 @@
  #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
@@ -16,5 +13,5 @@ TYPEDEF float float_t;
  TYPEDEF double double_t;

  TYPEDEF struct { long long __ll; long double __ld; } max_align_t;
-
  TYPEDEF unsigned nlink_t;
+TYPEDEF int blksize_t;

2. remove arch/loongarch64/bits/fcntl.h

3. adjust struct stat and kstat, keep consistent with kernel in
include/uapi/asm-generic/stat.h

diff --git a/arch/loongarch64/bits/stat.h b/arch/loongarch64/bits/stat.h
index b620e142..b7f4221b 100644
--- a/arch/loongarch64/bits/stat.h
+++ b/arch/loongarch64/bits/stat.h
@@ -1,20 +1,18 @@
  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];
+       unsigned long __pad;
         off_t st_size;
-       int __pad3;
+       blksize_t st_blksize;
+       int __pad2;
+       blkcnt_t st_blocks;
         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];
+       unsigned __unused[2];
  };

diff --git a/arch/loongarch64/kstat.h b/arch/loongarch64/kstat.h
index 4e39ffac..900f119d 100644
--- a/arch/loongarch64/kstat.h
+++ b/arch/loongarch64/kstat.h
@@ -6,9 +6,9 @@ struct kstat {
         uid_t st_uid;
         gid_t st_gid;
         dev_t st_rdev;
-       unsigned long __pad1;
+       unsigned long __pad;
         off_t st_size;
-       unsigned st_blksize;
+       blksize_t st_blksize;
         int __pad2;
         blkcnt_t st_blocks;
         long st_atime_sec;

4. optimize the __get_tp() implement. I already confirmed with
gcc and clang team, this optimize is ok.

diff --git a/arch/loongarch64/pthread_arch.h 
b/arch/loongarch64/pthread_arch.h
index 27f50e4c..95ee4c7a 100644
--- a/arch/loongarch64/pthread_arch.h
+++ b/arch/loongarch64/pthread_arch.h
@@ -1,7 +1,7 @@
  static inline uintptr_t __get_tp()
  {
-       uintptr_t tp;
-       __asm__ ("or %0, $tp, $zero" : "=r" (tp) );
+       register uintptr_t tp __asm__("tp");
+       __asm__ ("" : "=r" (tp) );
         return tp;
  }


Hongliang Wang


在 2022/3/22 下午8:59, Rich Felker 写道:
> 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.