[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 15 Aug 2023 17:24:30 +0800
From: Hongliang Wang <wanghongliang@...ngson.cn>
To: musl@...ts.openwall.com
Subject: Re: add loongarch64 port v7.
在 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@...0n.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@...0n.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@...0n.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@...0n.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@...0n.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@...0n.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@...0n.name>
Signed-off-by: Huacai Chen <chenhuacai@...ngson.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
>
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.
