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 12838 invoked from network); 22 Sep 2023 01:37:13 -0000 Received: from second.openwall.net (193.110.157.125) by inbox.vuxu.org with ESMTPUTF8; 22 Sep 2023 01:37:13 -0000 Received: (qmail 29743 invoked by uid 550); 22 Sep 2023 01:37:07 -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 29705 invoked from network); 22 Sep 2023 01:37:06 -0000 To: Jianmin Lv , musl@lists.openwall.com References: <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> <65857ef7-0ef8-d3c8-d6d8-ea577b99e793@loongson.cn> <20230920131619.GA1427497@port70.net> From: Hongliang Wang Message-ID: <9dd23cf9-9795-0704-3a83-085ad9e6054a@loongson.cn> Date: Fri, 22 Sep 2023 09:37:27 +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: <20230920131619.GA1427497@port70.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-CM-TRANSID:AQAAf8DxjNyr7wxlAa8NAA--.27928S3 X-CM-SenderInfo: pzdqwxxrqjzxhdqjqz5rrqw2lrqou0/ X-Coremail-Antispam: 1Uk129KBj93XoW3JF15Jry7CryDCw13GF1Dtwc_yoW3Zw1xpF WFka1UKrWDJr1fJ34xK3WrXFyayrW8J3WUXr1fXFyUZr90v3s2qr1Iqr4Y9rnrur48Gw1j vr4Utw1furn8AagCm3ZEXasCq-sJn29KB7ZKAUJUUUUU529EdanIXcx71UUUUU7KY7ZEXa sCq-sGcSsGvfJ3Ic02F40EFcxC0VAKzVAqx4xG6I80ebIjqfuFe4nvWSU5nxnvy29KBjDU 0xBIdaVrnRJUUUvYb4IE77IF4wAFF20E14v26r1j6r4UM7CY07I20VC2zVCF04k26cxKx2 IYs7xG6rWj6s0DM7CIcVAFz4kK6r1j6r18M28lY4IEw2IIxxk0rwA2F7IY1VAKz4vEj48v e4kI8wA2z4x0Y4vE2Ix0cI8IcVAFwI0_Jr0_JF4l84ACjcxK6xIIjxv20xvEc7CjxVAFwI 0_Jr0_Gr1l84ACjcxK6I8E87Iv67AKxVWxJVW8Jr1l84ACjcxK6I8E87Iv6xkF7I0E14v2 6r4j6r4UJwAS0I0E0xvYzxvE52x082IY62kv0487Mc804VCY07AIYIkI8VC2zVCFFI0UMc 02F40EFcxC0VAKzVAqx4xG6I80ewAv7VC0I7IYx2IY67AKxVWUJVWUGwAv7VC2z280aVAF wI0_Jr0_Gr1lOx8S6xCaFVCjc4AY6r1j6r4UM4x0Y48IcVAKI48JMxk0xIA0c2IEe2xFo4 CEbIxvr21l42xK82IYc2Ij64vIr41l4I8I3I0E4IkC6x0Yz7v_Jr0_Gr1lx2IqxVAqx4xG 67AKxVWUJVWUGwC20s026x8GjcxK67AKxVWUGVWUWwC2zVAF1VAY17CE14v26r1Y6r17MI IYrxkI7VAKI48JMIIF0xvE2Ix0cI8IcVAFwI0_Jr0_JF4lIxAIcVC0I7IYx2IY6xkF7I0E 14v26r1j6r4UMIIF0xvE42xK8VAvwI8IcIk0rVWUJVWUCwCI42IY6I8E87Iv67AKxVWUJV W8JwCI42IY6I8E87Iv6xkF7I0E14v26r1j6r4UYxBIdaVFxhVjvjDU0xZFpf9x07UE-erU UUUU= Subject: Re: [musl] add loongarch64 port v7. Hi, Thank you for your review, I will modify it according to the review suggestions and post v8 patch later. Hongliang Wang 在 2023/9/20 下午9:16, Szabolcs Nagy 写道: > * Jianmin Lv [2023-09-20 15:45:39 +0800]: >> 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. > > i think you should post a v8 patch to move > this forward. (not on github, but here) > >> 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: >>>>> -+#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. > > please use 1234. > >>>>> -+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. > > please use plain unsigned, not unsigned int. > >>>>> -+      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 > > this looks ok to me. > > (original code looks sketchy to me.) > >>>>>   +#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. > > i would change it back to the asm. > > generic version should work, but maybe we don't > want to trust the compiler here (there may be > ways to compile the generic code that is not > compatible with incompletely relocated libc.so) > the asm is safer. > >>>> 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. > > this should be ok. > >>>> 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. > > please add back the padding field. > >>>> 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. > > please fix it. > >>>> 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. > > it sounds like the correct asm is $fcsr0, so that's > what should be used on all assemblers, not just on > clang as. > > only broken old gnu as should use different syntax. > for this a configure test is needed that adds a > CFLAG like -DBROKEN_LOONGARCH_FCSR_ASM when fails. > and use that for the ifdef. > >>> >>> 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? > > all issues look minor. > > if you post a v8 it likely gets into a release faster. >