Re: [musl] add loongarch64 port v5

[Rich Felker <dalias@libc.org>]

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