Re: [musl] add loongarch64 port v7.

[Hongliang Wang <wanghongliang@loongson.cn>]

在 2023/8/13 上午9:41, Rich Felker 写道:
> On Sat, Aug 05, 2023 at 11:43:08AM -0400, Rich Felker wrote:
>> On Sat, Aug 05, 2023 at 02:18:35PM +0800, 翟小娟 wrote:
>>> We are currently working on bringing loongarch support to the alpine
>>> community. Alpine depends on musl, and Alpine and its users are
>>> waiting for musl to support loongarch. This patch seems to be a long
>>> time ago, we hope to know the review status of this patch.
>>
>> Last time I set out to look at it, I hit that the exact patch I had
>> previously reviewed and tentatively okayed wasn't stored anywhere
>> permanent. I realize this has taken quite a while, and now there is a
>> patch on the list, so this coming week I will go back and review the
>> diff of that against the previous posted version and mailing list
>> comments. As long as everything matches up, I expect to commit it.
>>
>> Thanks for checking back in on this, and sorry it's taken so long.
> 
> OK, I'm looking at this now as the diffs between v5 and v6 (which I
> theoretically reviewed before, but the exact v6 I looked at did not
> exist in permanent form) and between v6 and v7.
> 
> v5->v6:
> 
>> -+#define __BYTE_ORDER 1234
>> ++#define __BYTE_ORDER  __LITTLE_ENDIAN
> 
> This is gratuitous, mismatches what is done on other archs, and is
> less safe.
>
The modification is based on the following review suggestion(
WANG Xuerui <i@xen0n.name> reviewed in
https://www.openwall.com/lists/musl/2022/10/12/1):

`#define __BYTE_ORDER __LITTLE_ENDIAN` could be more consistent
with other arches.

>> -+TYPEDEF unsigned nlink_t;
>> ++TYPEDEF unsigned int nlink_t;
> 
> Gratuitous and in opposite direction of coding style used elsewhere in
> musl. There are a few other instances of this too.
> 
Based on the following review question(WANG Xuerui <i@xen0n.name>
reviewed in https://www.openwall.com/lists/musl/2022/10/12/1):

`unsigned int`? Same for other bare `unsigned` usages.

I fixed it to a explicit definition.

>> -+      register uintptr_t tp __asm__("tp");
>> -+      __asm__ ("" : "=r" (tp) );
>> ++      uintptr_t tp;
>> ++      __asm__ __volatile__("move %0, $tp" : "=r"(tp));
>>   +      return tp;
> 
> Not clear what the motivation is here. Is it working around a compiler
> bug? The original form was more optimal.
> 
The modification is based on the following review suggestion(
WANG Xuerui <i@xen0n.name> reviewed in
https://www.openwall.com/lists/musl/2022/10/12/1):

While the current approach works, it's a bit fragile [1], and
the simple and plain riscv version works too:

uintptr_t tp;
__asm__ ("move %0, $tp" : "=r"(tp));

[1]:https://gcc.gnu.org/onlinedocs/gcc/Local-Register-Variables.html#Local-Register-Variables

>>   +#define CRTJMP(pc,sp) __asm__ __volatile__( \
>> -+      "move $sp,%1 ; jr %0" : : "r"(pc), "r"(sp) : "memory" )
>> -+
>> -+#define GETFUNCSYM(fp, sym, got) __asm__ ( \
>> -+      ".hidden " #sym "\n" \
>> -+      ".align 8 \n" \
>> -+      "       la.local $t1, "#sym" \n" \
>> -+      "       move %0, $t1 \n" \
>> -+      : "=r"(*(fp)) : : "memory" )
>> ++      "move $sp, %1 ; jr %0" : : "r"(pc), "r"(sp) : "memory" )
> 
> Not clear why this was changed. It was never discussed afaik. It looks
> somewhat dubious removing GETFUNCSYM and using the maybe-unreliable
> default definition.
> 
Based on the following review question(
WANG Xuerui <i@xen0n.name> reviewed
in https://www.openwall.com/lists/musl/2022/10/12/1):

Does the generic version residing in ldso/dlstart.c not work?

I found the code logic is consistent with the generic version, So
I removed the definition here and replaced it with generic version.

> It also looks like v5->v6 rewrote sigsetjmp.
> 
Based on the following review question(
WANG Xuerui <i@xen0n.name> reviewed
in https://www.openwall.com/lists/musl/2022/10/12/1):

This is crazy complicated compared to the riscv port, why is the juggling
between a0/a1 and t5/t6 necessary?

I optimized the code implementations.

> v6->v7:
> 
> sigcontext/mcontext_t change mostly makes sense, but the
> namespace-safe and full mcontext_t differ in their use of the aligned
> attribute and it's not clear that the attribute is needed (since the
> offset is naturally aligned and any instance of the object is produced
> by the kernel, not by userspace).
> 
>> -+      long               __uc_pad;
> 
> This change to ucontext_t actually makes the struct ABI-incompatible
> in the namespace-safe version without the alignment attribute. IIRC I
> explicitly requested the explicit padding field to avoid this kind of
> footgun. Removing it does not seem like a good idea.
> 
Initially, we add __uc_pad to ensure uc_mcontext is 16 byte alignment.
Now, we added __attribute__((__aligned__(16))) to 
uc_mcontext.__extcontext[],
this can ensure uc_mcontext is also 16 byte alignment. so __uc_pad is 
not used.

Remove __uc_pad, from the point of struct layout, musl and kernel are
consistent. otherwise, I think it may bring a sense of inconsistency
between kernel and musl. Due to Loongarch is not merged into musl now,
Remove it no compatibility issues.

> In stat.h:
> 
>> -+      unsigned long __pad;
>> ++      unsigned long __pad1;
> 
> This is gratuitous and makes the definition gratuitously mismatch what
> will become the "generic" version of bits/stat.h. There is no contract
> for applications to be able to access these padding fields by name, so
> no motivation to make their names match glibc's names or the kernel's.
> 
OK.

> In fenv.S:
> 
>> ++#ifdef __clang__
>> ++#define FCSR $fcsr0
>> ++#else
>> ++#define FCSR $r0
>> ++#endif
> 
> It's not clear to me what's going on here. Is there a clang
> incompatibility in the assembler language you're working around? Or
> what? If so that seems like a tooling bug that should be fixed.
> 
The GNU assembler cannot correctly recognize $fcsr0, but the
LLVM IAS does not have this issue, so make a distinction.
This issue has been fixed in GNU assembler 2.41. but for compatible
with GNU assembler 2.40 and below, $r0 need reserved.

The linux kernel also has a similar distinction:

commit 38bb46f94544c5385bc35aa2bfc776dcf53a7b5d
Author: WANG Xuerui <git@xen0n.name>
Date:   Thu Jun 29 20:58:43 2023 +0800

     LoongArch: Prepare for assemblers with proper FCSR class support

     The GNU assembler (as of 2.40) mis-treats FCSR operands as GPRs, but
     the LLVM IAS does not. Probe for this and refer to FCSRs as"$fcsrNN"
     if support is present.

     Signed-off-by: WANG Xuerui <git@xen0n.name>
     Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>

> Otherwise, everything looks as expected, I think. I'm okay with making
> any fixups for merging rather than throwing this back on your team for
> more revisions, but can you please follow up and clarify the above?
> 
> Rich
>