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=-7.5 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 25277 invoked from network); 14 Nov 2023 13:17:11 -0000 Received: from second.openwall.net (193.110.157.125) by inbox.vuxu.org with ESMTPUTF8; 14 Nov 2023 13:17:11 -0000 Received: (qmail 3216 invoked by uid 550); 14 Nov 2023 13:17:06 -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 3164 invoked from network); 14 Nov 2023 13:17:05 -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> <1f1d2528-ae86-46ea-64b1-c5b3ddb1709b@loongson.cn> From: Jingyun Hua Message-ID: <7b59ddc0-67ab-4ffa-e083-3e5086dd5de3@loongson.cn> Date: Tue, 14 Nov 2023 21:16:34 +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: <1f1d2528-ae86-46ea-64b1-c5b3ddb1709b@loongson.cn> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-CM-TRANSID:AQAAf8Axji8yc1NlcPVBAA--.13529S3 X-CM-SenderInfo: xkxdyxpqj130o6or00hjvr0hdfq/1tbiAQAPDWVS2SlAKQAAst X-Coremail-Antispam: 1Uk129KBj9fXoW3ZrWxZr4kCF13Jr17Zr48Zrc_yoW8GrWDWo WfGFs7Jw1rJr1UGr1UAw1DXry3Aw18Jr1DJryUGr13JF15ta4UJ34UGryUXayUtry8Gr4U Ja4UJw1UAFyUJrn5l-sFpf9Il3svdjkaLaAFLSUrUUUUUb8apTn2vfkv8UJUUUU8wcxFpf 9Il3svdxBIdaVrn0xqx4xG64xvF2IEw4CE5I8CrVC2j2Jv73VFW2AGmfu7bjvjm3AaLaJ3 UjIYCTnIWjp_UUUYx7kC6x804xWl14x267AKxVWUJVW8JwAFc2x0x2IEx4CE42xK8VAvwI 8IcIk0rVWrJVCq3wAFIxvE14AKwVWUJVWUGwA2ocxC64kIII0Yj41l84x0c7CEw4AK67xG Y2AK021l84ACjcxK6xIIjxv20xvE14v26r1I6r4UM28EF7xvwVC0I7IYx2IY6xkF7I0E14 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, I have a suggestion regarding the musl dynamic linker name for the LoongArch architecture: The basic ABI types are specified in the ABI document of the LoongArch, which can be seen: https://loongson.github.io/LoongArch-Documentation/LoongArch-ELF-ABI-EN.html and the dynamic linker name of glibc distinguishes lp64d, lp64s and lp64f with abi types.The v8 patch only defines the value of SUBARCH as "-sf" for “__loongarch_soft_float”, which means that only “ld-musl-loongarch64.so.1” or “ld-musl-loongarch64-sf.so.1” can be generated when compiled with the v8 patch. So I think the musl dynamic linker name for the LoongArch architecture needs to be modified, maybe defined like glibc? Regards, Jingyun Hua On 11/9/23 9:15 PM, Jingyun Hua wrote: > 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