From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-4.8 required=5.0 tests=MAILING_LIST_MULTI, NICE_REPLY_A,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 29981 invoked from network); 20 Sep 2023 07:46:35 -0000 Received: from second.openwall.net (193.110.157.125) by inbox.vuxu.org with ESMTPUTF8; 20 Sep 2023 07:46:35 -0000 Received: (qmail 28524 invoked by uid 550); 20 Sep 2023 07:46:29 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 28486 invoked from network); 20 Sep 2023 07:46:27 -0000 To: musl@lists.openwall.com References: <20230418093844.GP3630668@port70.net> <0fd19586-376c-14ec-a50b-8c561f9f82f2@loongson.cn> <72a1a5d1-6f57-9a12-7775-84e1ff2c1df2@loongson.cn> <64955faf-c213-34bf-c7e2-691064da51f9@loongson.cn> <968652d9-3780-0843-908e-f21bc93b6ee1@loongson.cn> <320c8444.bb9a.189c45903f8.Coremail.zhaixiaojuan@loongson.cn> <20230805154307.GS4163@brightrain.aerifal.cx> <20230813014109.GU4163@brightrain.aerifal.cx> <99d954ca-faee-2cac-97af-7fc2ecdb9a89@loongson.cn> Cc: Hongliang Wang From: Jianmin Lv Message-ID: <65857ef7-0ef8-d3c8-d6d8-ea577b99e793@loongson.cn> Date: Wed, 20 Sep 2023 15:45:39 +0800 User-Agent: Mozilla/5.0 (X11; Linux loongarch64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <99d954ca-faee-2cac-97af-7fc2ecdb9a89@loongson.cn> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-CM-TRANSID:AQAAf8Cxzt4jowplaWMMAA--.27313S3 X-CM-SenderInfo: 5oymxthqpl0qxorr0wxvrqhubq/ X-Coremail-Antispam: 1Uk129KBj93XoW3JF1rJFykKFy3ur1rCw48AFc_yoW3XFy5pF WFka1UKrWDJrn3J34xK3WrXFyak3y8Jw1UJryfXFyUZrW5Za4Iqr1Igr4Y9wnrur48Cw1j vF4Utw1rZrs8AagCm3ZEXasCq-sJn29KB7ZKAUJUUUUU529EdanIXcx71UUUUU7KY7ZEXa sCq-sGcSsGvfJ3Ic02F40EFcxC0VAKzVAqx4xG6I80ebIjqfuFe4nvWSU5nxnvy29KBjDU 0xBIdaVrnRJUUUv2b4IE77IF4wAFF20E14v26r1j6r4UM7CY07I20VC2zVCF04k26cxKx2 IYs7xG6rWj6s0DM7CIcVAFz4kK6r1j6r18M28lY4IEw2IIxxk0rwA2F7IY1VAKz4vEj48v e4kI8wA2z4x0Y4vE2Ix0cI8IcVAFwI0_Gr0_Xr1l84ACjcxK6xIIjxv20xvEc7CjxVAFwI 0_Gr0_Cr1l84ACjcxK6I8E87Iv67AKxVW8Jr0_Cr1UM28EF7xvwVC2z280aVCY1x0267AK xVW8Jr0_Cr1UM2AIxVAIcxkEcVAq07x20xvEncxIr21l57IF6xkI12xvs2x26I8E6xACxx 1l5I8CrVACY4xI64kE6c02F40Ex7xfMcIj6xIIjxv20xvE14v26r1j6r18McIj6I8E87Iv 67AKxVWUJVW8JwAm72CE4IkC6x0Yz7v_Jr0_Gr1lF7xvr2IY64vIr41lc7I2V7IY0VAS07 AlzVAYIcxG8wCF04k20xvY0x0EwIxGrwCFx2IqxVCFs4IE7xkEbVWUJVW8JwC20s026c02 F40E14v26r1j6r18MI8I3I0E7480Y4vE14v26r106r1rMI8E67AF67kF1VAFwI0_Jrv_JF 1lIxkGc2Ij64vIr41lIxAIcVC0I7IYx2IY67AKxVWUJVWUCwCI42IY6xIIjxv20xvEc7Cj xVAFwI0_Jr0_Gr1lIxAIcVCF04k26cxKx2IYs7xG6r1j6r1xMIIF0xvEx4A2jsIE14v26r 1j6r4UMIIF0xvEx4A2jsIEc7CjxVAFwI0_Jr0_GrUvcSsGvfC2KfnxnUUI43ZEXa7IU1CP fJUUUUU== Subject: Re: [musl] add loongarch64 port v7. Hi, Rich Sorry to bother you, but I just want to know if there is any progress on this, because Alpine is blocking by this patch for a long time. As Hongliang has explained questions you mentioned one by one, if any question need to be discussed, please point them out, so that the patch can be handled further. Thanks very much! Jianmin On 2023/8/15 下午4:17, Hongliang Wang wrote: > > 在 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 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 > 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 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 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 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 > 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 >     Signed-off-by: Huacai Chen > >> 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 >>