[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20221012182038.GI29905@brightrain.aerifal.cx>
Date: Wed, 12 Oct 2022 14:20:39 -0400
From: Rich Felker <dalias@...c.org>
To: 王洪亮 <wanghongliang@...ngson.cn>
Cc: musl@...ts.openwall.com
Subject: Re: add loongarch64 port v5
On Wed, Aug 03, 2022 at 04:15:57PM +0800, 王洪亮 wrote:
> Hi,
>
> I published loongarch64 port v5 in
> https://github.com/loongson/musl/tree/loongarch-v2.0.
>
> fixed the following problems:
> 1.Removed arch/loongarch64/bits/hwcap.h,which is not used.
> 2.Removed the unused macro definitions in arch/loongarch64/bits/reg.h.
> 3.#define SA_RESTORER 0x0,which means invalid in LoongArch.
> 4.Modified the position of "loongarch64*) ARCH=loongarch64 ;;" by
> Alphabetic order in configure.
> 5.Modified the macro definition of VDSO_CGT_VER to "LINUX_5.10"
> 6.Modified the macro definition of EF_LARCH_ABI and
> EF_LARCH_ABI_LP64D in elf.h
> 7.Modified some inappropriate comment.
> 8.Some assembly instructions optimization.
>
> please check again,I'm Looking forward to your reply,thanks.
OK, I've finally collected my notes from the past reviews and gone
thru the current version to check it against them. As far as I can
tell, all the requested changes were made, and I don't see anything
major wrong. A few minor details...
> diff --git a/include/elf.h b/include/elf.h
> index 86e2f0bb..1b0e9e71 100644
> --- a/include/elf.h
> +++ b/include/elf.h
> @@ -697,6 +697,11 @@ typedef struct {
> #define NT_MIPS_FP_MODE 0x801
> #define NT_MIPS_MSA 0x802
> #define NT_VERSION 1
> +#define NT_LOONGARCH_CPUCFG 0xa00
> +#define NT_LOONGARCH_CSR 0xa01
> +#define NT_LOONGARCH_LSX 0xa02
> +#define NT_LOONGARCH_LASX 0xa03
> +#define NT_LOONGARCH_LBT 0xa04
>
>
>
> @@ -3288,6 +3293,66 @@ enum
> #define R_RISCV_SET32 56
> #define R_RISCV_32_PCREL 57
>
> +/* LoongArch ELF Flags */
> +#define EM_LOONGARCH 258
> +
> +#define EF_LARCH_ABI 0x07
> +#define EF_LARCH_ABI_LP64D 0x03
> +
> +/* LoongArch specific dynamic relocations. */
> +#define R_LARCH_NONE 0
These are all the relocations, not just dynamic ones like the comment
says. But more to the point, elf.h and the public headers in general
don't have existing comments like this, and should not add them for a
new arch.
> diff --git a/src/fenv/loongarch64/fenv.S b/src/fenv/loongarch64/fenv.S
> new file mode 100644
> index 00000000..aa012c97
> --- /dev/null
> +++ b/src/fenv/loongarch64/fenv.S
> @@ -0,0 +1,72 @@
> +#ifndef __loongarch_soft_float
> +
> +.global feclearexcept
> +.type feclearexcept,@function
> +feclearexcept:
> + li.w $a1, 0x1f0000
> + and $a0, $a0, $a1
> + movfcsr2gr $a1, $r0
> + or $a1, $a1, $a0
> + xor $a1, $a1, $a0
> + movgr2fcsr $r0, $a1
> + li.w $a0, 0
> + jr $ra
> +
> +.global feraiseexcept
> +.type feraiseexcept,@function
> +feraiseexcept:
> + li.w $a1, 0x1f0000
> + and $a0, $a0, $a1
> + movfcsr2gr $a1, $r0
> + or $a1, $a1, $a0
> + movgr2fcsr $r0, $a1
> + li.w $a0, 0
> + jr $ra
> [...]
Here especially there is inconsistent indentation using spaces mixed
in with tabs in some places. Please fix this to use all tabs for
indentation. The general style rule in musl is that tabs are used for
indention levels and spaces are used to align text, with the principle
(a useful method to check) that nothing should look wrong if the
reader is using a non-default tab width.
There seem to be related issues throughout the asm source files; see
below:
> diff --git a/src/thread/loongarch64/__set_thread_area.s b/src/thread/loongarch64/__set_thread_area.s
> new file mode 100644
> index 00000000..6fd09a92
> --- /dev/null
> +++ b/src/thread/loongarch64/__set_thread_area.s
> @@ -0,0 +1,7 @@
> +.global __set_thread_area
> +.hidden __set_thread_area
> +.type __set_thread_area,@function
Here for example there are tabs between the directive and the name on
some lines but not others. It's not clear if this was copied from
somewhere upstream we have a mess now, or just introduced in the new
port. But it should probably be spaces everywhere, either a single
space or spaces to align at a common column.
> diff --git a/src/thread/loongarch64/clone.s b/src/thread/loongarch64/clone.s
> new file mode 100644
> index 00000000..86e69cfa
> --- /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, 3, 0 # align stack to 16 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
> + jr $ra # 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
And here some of the column alignment is done with tabs while other
parts are done with spaces. Should be all-spaces for alignment.
> diff --git a/src/thread/loongarch64/syscall_cp.s b/src/thread/loongarch64/syscall_cp.s
> new file mode 100644
> index 00000000..9f57d254
> --- /dev/null
> +++ b/src/thread/loongarch64/syscall_cp.s
> @@ -0,0 +1,29 @@
> +.global __cp_begin
> +.hidden __cp_begin
> +.global __cp_end
> +.hidden __cp_end
> +.global __cp_cancel
> +.hidden __cp_cancel
> +.hidden __cancel
> +.global __syscall_cp_asm
> +.hidden __syscall_cp_asm
> +.type __syscall_cp_asm,@function
> +
> +__syscall_cp_asm:
> +__cp_begin:
> + ld.w $a0, $a0, 0
> + bnez $a0, __cp_cancel
> + move $t8, $a1 # reserve system call number
> + move $a0, $a2
> + move $a1, $a3
> + move $a2, $a4
> + move $a3, $a5
> + move $a4, $a6
> + move $a5, $a7
> + move $a7, $t8
> + syscall 0
> +__cp_end:
> + jr $ra
> +__cp_cancel:
> + la.local $t8, __cancel
> + jr $t8
> --
> 2.36.0
More here too.
I might have missed some other instances of this. I'll try to run it
through some regex's to get a more complete list.
At this point I think this is all stylistic stuff, no real content
changes needed. I don't have a setup to test any of this, so there's
an assumption on my part baked in that this is basically working and
smoke-tested and whatnot. Can you point us at directions to get a
working toolchain and test execution environment so that myself or
others can run some basic checks?
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.
