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=-6.4 required=5.0 tests=MAILING_LIST_MULTI, NICE_REPLY_A,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 22169 invoked from network); 9 Nov 2023 13:16:04 -0000 Received: from second.openwall.net (193.110.157.125) by inbox.vuxu.org with ESMTPUTF8; 9 Nov 2023 13:16:04 -0000 Received: (qmail 9938 invoked by uid 550); 9 Nov 2023 13:16:00 -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 9900 invoked from network); 9 Nov 2023 13:15:58 -0000 To: 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> <9dd23cf9-9795-0704-3a83-085ad9e6054a@loongson.cn> <3838b2d6-8330-33b5-fd87-8af3404a29dc@loongson.cn> From: Jingyun Hua Message-ID: <1f1d2528-ae86-46ea-64b1-c5b3ddb1709b@loongson.cn> Date: Thu, 9 Nov 2023 21:15:30 +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: <3838b2d6-8330-33b5-fd87-8af3404a29dc@loongson.cn> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-CM-TRANSID:AQAAf8CxP91y20xlLf48AA--.3833S3 X-CM-SenderInfo: xkxdyxpqj130o6or00hjvr0hdfq/1tbiAQAKDWVMQak6iQABs1 X-Coremail-Antispam: 1Uk129KBj9fXoW3uF4kAr1DGF1DCF1UJF4UKFX_yoW8JrykCo WfGFs7Jw4rJr1UWr1DAwnrXry3Zw18Jr1DJryUGr13JF15ta45J34UGryUXayUtry8Gr4U Ja4UJw1DAFWUJrn5l-sFpf9Il3svdjkaLaAFLSUrUUUUUb8apTn2vfkv8UJUUUU8wcxFpf 9Il3svdxBIdaVrn0xqx4xG64xvF2IEw4CE5I8CrVC2j2Jv73VFW2AGmfu7bjvjm3AaLaJ3 UjIYCTnIWjp_UUUYx7kC6x804xWl14x267AKxVWUJVW8JwAFc2x0x2IEx4CE42xK8VAvwI 8IcIk0rVWrJVCq3wAFIxvE14AKwVWUJVWUGwA2ocxC64kIII0Yj41l84x0c7CEw4AK67xG Y2AK021l84ACjcxK6xIIjxv20xvE14v26r1j6r1xM28EF7xvwVC0I7IYx2IY6xkF7I0E14 v26r1j6r4UM28EF7xvwVC2z280aVAFwI0_Gr1j6F4UJwA2z4x0Y4vEx4A2jsIEc7CjxVAF wI0_Gr1j6F4UJwAS0I0E0xvYzxvE52x082IY62kv0487Mc804VCY07AIYIkI8VC2zVCFFI 0UMc02F40EFcxC0VAKzVAqx4xG6I80ewAv7VC0I7IYx2IY67AKxVWUJVWUGwAv7VC2z280 aVAFwI0_Jr0_Gr1lOx8S6xCaFVCjc4AY6r1j6r4UM4x0Y48IcVAKI48JMxk0xIA0c2IEe2 xFo4CEbIxvr21l42xK82IYc2Ij64vIr41l4I8I3I0E4IkC6x0Yz7v_Jr0_Gr1lx2IqxVAq x4xG67AKxVWUJVWUGwC20s026x8GjcxK67AKxVWUGVWUWwC2zVAF1VAY17CE14v26r1j6r 15MIIYrxkI7VAKI48JMIIF0xvE2Ix0cI8IcVAFwI0_Jr0_JF4lIxAIcVC0I7IYx2IY6xkF 7I0E14v26r1j6r4UMIIF0xvE42xK8VAvwI8IcIk0rVWUJVWUCwCI42IY6I8E87Iv67AKxV WUJVW8JwCI42IY6I8E87Iv6xkF7I0E14v26r1j6r4UYxBIdaVFxhVjvjDU0xZFpf9x07UE -erUUUUU= Subject: Re: [musl] add loongarch64 port v8. Hi all, Thank you for taking the time to check this email. I'm very interested in musl and alpine, and I'd like to know the current status of this patch and what we can do for it next. After Hongliang updated this patch from v7 to v8, in order to verify it, I built the alpine linux from scratch base on this musl v8 patch on the LoongArch64 machine, and successfully compiled projects such as gcc, rust, llvm, go, etc.. Currently, more than 8,500 packages have been built and running normally in my local repository. The patch has indeed been on hold for a long time. Is it in the immediate plans? Is there anything that needs to be modified and improved? Looking forward to your reply, thanks! Regards, Jingyun Hua 在 2023/9/26 上午11:28, Hongliang Wang 写道: > Hi, > > I have fixed the issues listed below, and post > 0001-add-loongarch64-port-v8.patch,as shown in the attachment. > please review again, if there are anything that need to be modified, > please point out, thank you. > > Hongliang Wang > > 在 2023/9/22 上午9:37, Hongliang Wang 写道: >> 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. > > --- a/arch/loongarch64/bits/alltypes.h.in > +++ b/arch/loongarch64/bits/alltypes.h.in > @@ -2,7 +2,7 @@ >  #define _Int64 long >  #define _Reg   long > > -#define __BYTE_ORDER  __LITTLE_ENDIAN > +#define __BYTE_ORDER  1234 >  #define __LONG_MAX    0x7fffffffffffffffL > > >>> >>>>>>> -+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. > > --- a/arch/loongarch64/bits/alltypes.h.in > +++ b/arch/loongarch64/bits/alltypes.h.in > @@ -2,7 +2,7 @@ > > -TYPEDEF unsigned int nlink_t; > +TYPEDEF unsigned nlink_t; >  TYPEDEF int blksize_t; > >>> >>>>>>> -+      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. >>> > --- a/arch/loongarch64/reloc.h > +++ b/arch/loongarch64/reloc.h > @@ -18,3 +18,10 @@ > >  #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" ) > >>>>>> 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. > > --- a/arch/loongarch64/bits/signal.h > +++ b/arch/loongarch64/bits/signal.h > @@ -40,6 +40,7 @@ typedef struct __ucontext >         struct __ucontext  *uc_link; >         stack_t            uc_stack; >         sigset_t           uc_sigmask; > +       long               __uc_pad; >         mcontext_t         uc_mcontext; >  } ucontext_t; > >>> >>>>>> 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. > > --- a/arch/loongarch64/bits/stat.h > +++ b/arch/loongarch64/bits/stat.h > @@ -6,7 +6,7 @@ struct stat { >         uid_t st_uid; >         gid_t st_gid; >         dev_t st_rdev; > -       unsigned long __pad1; > +       unsigned long __pad; >         off_t st_size; >         blksize_t st_blksize; >         int __pad2; > >>> >>>>>> 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. >>> > --- a/configure > +++ b/configure > > if test "$ARCH" = "loongarch64" ; then > trycppif __loongarch_soft_float "$t" && SUBARCH=${SUBARCH}-sf > +printf "checking whether compiler support FCSRs... " > +echo "__asm__(\"movfcsr2gr \$t0,\$fcsr0\");" > "$tmpc" > +if $CC -c -o /dev/null "$tmpc" >/dev/null 2>&1 ; then > +printf "yes\n" > +else > +printf "no\n" > +CFLAGS_AUTO="$CFLAGS_AUTO -DBROKEN_LOONGARCH_FCSR_ASM" > +fi > fi > > --- a/src/fenv/loongarch64/fenv.S > +++ b/src/fenv/loongarch64/fenv.S > @@ -1,9 +1,9 @@ >  #ifndef __loongarch_soft_float > > -#ifdef __clang__ > -#define FCSR $fcsr0 > -#else > +#ifdef BROKEN_LOONGARCH_FCSR_ASM >  #define FCSR $r0 > +#else > +#define FCSR $fcsr0 >  #endif > >  .global        feclearexcept > >>>>> >>>>> 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. >>> -- Jingyun Hua