* LoongArch: Backport r14-4674 "LoongArch: Delete macro definition ASM_OUTPUT_ALIGN_WITH_NOP."? @ 2024-02-06 16:23 Xi Ruoyao 2024-02-08 16:02 ` chenglulu 0 siblings, 1 reply; 9+ messages in thread From: Xi Ruoyao @ 2024-02-06 16:23 UTC (permalink / raw) To: chenglulu; +Cc: gcc-patches, WANG Xuerui, Jiajie Chen, Chenghua Xu Hi Lulu, I'm proposing to backport r14-4674 "LoongArch: Delete macro definition ASM_OUTPUT_ALIGN_WITH_NOP." to releases/gcc-12 and releases/gcc-13. The reasons: 1. Strictly speaking, the old ASM_OUTPUT_ALIGN_WITH_NOP macro may cause a correctness issue. For example, a developer may use -falign- functions=16 and then use the low 4 bits of a function pointer to encode some metainfo. Then ASM_OUTPUT_ALIGN_WITH_NOP causes the functions not really aligned to a 16 bytes boundary, causing some breakage. 2. With Binutils-2.42, ASM_OUTPUT_ALIGN_WITH_NOP can cause illegal opcodes. For example: .globl _start _start: .balign 32 nop nop nop addi.d $a0, $r0, 1 .balign 16,54525952,4 addi.d $a0, $a0, 1 is assembled and linked to: 0000000000000220 <_start>: 220: 03400000 nop 224: 03400000 nop 228: 03400000 nop 22c: 02c00404 li.d $a0, 1 230: 00000000 .word 0x00000000 # <== OOPS! 234: 02c00484 addi.d $a0, $a0, 1 Arguably this is a bug in GAS (it should at least error out for the unsupported case where .balign 16,54525952,4 appears with -mrelax; I'd prefer it to support the 3-operand .align directive even -mrelax for reasons I've given in [1]). But we can at least work it around by removing ASM_OUTPUT_ALIGN_WITH_NOP to allow using GCC 13.3 with Binutils 2.42. 3. Without ASM_OUTPUT_ALIGN_WITH_NOP, GCC just outputs something like ".align 5" which works as expected since Binutils-2.38. 4. GCC < 14 does not have a default setting of -falign-*, so changing this won't affect anyone who do not specify -falign-* explicitly. [1]:https://github.com/loongson-community/discussions/issues/41#issuecomment-1925872603 Is it OK to backport r14-4674 into releases/gcc-12 and releases/gcc-13 then? -- Xi Ruoyao <xry111@xry111.site> School of Aerospace Science and Technology, Xidian University ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: LoongArch: Backport r14-4674 "LoongArch: Delete macro definition ASM_OUTPUT_ALIGN_WITH_NOP."? 2024-02-06 16:23 LoongArch: Backport r14-4674 "LoongArch: Delete macro definition ASM_OUTPUT_ALIGN_WITH_NOP."? Xi Ruoyao @ 2024-02-08 16:02 ` chenglulu 2024-02-09 8:08 ` Xi Ruoyao 0 siblings, 1 reply; 9+ messages in thread From: chenglulu @ 2024-02-08 16:02 UTC (permalink / raw) To: Xi Ruoyao; +Cc: gcc-patches, WANG Xuerui, Jiajie Chen, Chenghua Xu 在 2024/2/7 上午12:23, Xi Ruoyao 写道: > Hi Lulu, > > I'm proposing to backport r14-4674 "LoongArch: Delete macro definition > ASM_OUTPUT_ALIGN_WITH_NOP." to releases/gcc-12 and releases/gcc-13. The > reasons: > > 1. Strictly speaking, the old ASM_OUTPUT_ALIGN_WITH_NOP macro may cause > a correctness issue. For example, a developer may use -falign- > functions=16 and then use the low 4 bits of a function pointer to encode > some metainfo. Then ASM_OUTPUT_ALIGN_WITH_NOP causes the functions not > really aligned to a 16 bytes boundary, causing some breakage. > > 2. With Binutils-2.42, ASM_OUTPUT_ALIGN_WITH_NOP can cause illegal > opcodes. For example: > > .globl _start > _start: > .balign 32 > nop > nop > nop > addi.d $a0, $r0, 1 > .balign 16,54525952,4 > addi.d $a0, $a0, 1 > > is assembled and linked to: > > 0000000000000220 <_start>: > 220: 03400000 nop > 224: 03400000 nop > 228: 03400000 nop > 22c: 02c00404 li.d $a0, 1 > 230: 00000000 .word 0x00000000 # <== OOPS! > 234: 02c00484 addi.d $a0, $a0, 1 > > Arguably this is a bug in GAS (it should at least error out for the > unsupported case where .balign 16,54525952,4 appears with -mrelax; I'd > prefer it to support the 3-operand .align directive even -mrelax for > reasons I've given in [1]). But we can at least work it around by > removing ASM_OUTPUT_ALIGN_WITH_NOP to allow using GCC 13.3 with Binutils > 2.42. > > 3. Without ASM_OUTPUT_ALIGN_WITH_NOP, GCC just outputs something like > ".align 5" which works as expected since Binutils-2.38. > > 4. GCC < 14 does not have a default setting of -falign-*, so changing > this won't affect anyone who do not specify -falign-* explicitly. > > [1]:https://github.com/loongson-community/discussions/issues/41#issuecomment-1925872603 > > Is it OK to backport r14-4674 into releases/gcc-12 and releases/gcc-13 > then? > Ok, I agree with you. Thanks! ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: LoongArch: Backport r14-4674 "LoongArch: Delete macro definition ASM_OUTPUT_ALIGN_WITH_NOP."? 2024-02-08 16:02 ` chenglulu @ 2024-02-09 8:08 ` Xi Ruoyao 2024-02-20 2:07 ` chenglulu 0 siblings, 1 reply; 9+ messages in thread From: Xi Ruoyao @ 2024-02-09 8:08 UTC (permalink / raw) To: chenglulu; +Cc: gcc-patches, WANG Xuerui, Jiajie Chen, Chenghua Xu On Fri, 2024-02-09 at 00:02 +0800, chenglulu wrote: > > 在 2024/2/7 上午12:23, Xi Ruoyao 写道: > > Hi Lulu, > > > > I'm proposing to backport r14-4674 "LoongArch: Delete macro definition > > ASM_OUTPUT_ALIGN_WITH_NOP." to releases/gcc-12 and releases/gcc-13. The > > reasons: > > > > 1. Strictly speaking, the old ASM_OUTPUT_ALIGN_WITH_NOP macro may cause > > a correctness issue. For example, a developer may use -falign- > > functions=16 and then use the low 4 bits of a function pointer to encode > > some metainfo. Then ASM_OUTPUT_ALIGN_WITH_NOP causes the functions not > > really aligned to a 16 bytes boundary, causing some breakage. > > > > 2. With Binutils-2.42, ASM_OUTPUT_ALIGN_WITH_NOP can cause illegal > > opcodes. For example: > > > > .globl _start > > _start: > > .balign 32 > > nop > > nop > > nop > > addi.d $a0, $r0, 1 > > .balign 16,54525952,4 > > addi.d $a0, $a0, 1 > > > > is assembled and linked to: > > > > 0000000000000220 <_start>: > > 220: 03400000 nop > > 224: 03400000 nop > > 228: 03400000 nop > > 22c: 02c00404 li.d $a0, 1 > > 230: 00000000 .word 0x00000000 # <== OOPS! > > 234: 02c00484 addi.d $a0, $a0, 1 > > > > Arguably this is a bug in GAS (it should at least error out for the > > unsupported case where .balign 16,54525952,4 appears with -mrelax; I'd > > prefer it to support the 3-operand .align directive even -mrelax for > > reasons I've given in [1]). But we can at least work it around by > > removing ASM_OUTPUT_ALIGN_WITH_NOP to allow using GCC 13.3 with Binutils > > 2.42. > > > > 3. Without ASM_OUTPUT_ALIGN_WITH_NOP, GCC just outputs something like > > ".align 5" which works as expected since Binutils-2.38. > > > > 4. GCC < 14 does not have a default setting of -falign-*, so changing > > this won't affect anyone who do not specify -falign-* explicitly. > > > > [1]:https://github.com/loongson-community/discussions/issues/41#issuecomment-1925872603 > > > > Is it OK to backport r14-4674 into releases/gcc-12 and releases/gcc-13 > > then? > > > Ok, I agree with you. > > Thanks! Oops, with Binutils-2.41 GAS will fail to assemble some conditional branches if we do this :(. Not sure what to do (maybe backporting both this and a simplified version of PR112330 fix?) Let's reconsider after the holiday... -- Xi Ruoyao <xry111@xry111.site> School of Aerospace Science and Technology, Xidian University ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: LoongArch: Backport r14-4674 "LoongArch: Delete macro definition ASM_OUTPUT_ALIGN_WITH_NOP."? 2024-02-09 8:08 ` Xi Ruoyao @ 2024-02-20 2:07 ` chenglulu 2024-02-20 11:25 ` Xi Ruoyao 0 siblings, 1 reply; 9+ messages in thread From: chenglulu @ 2024-02-20 2:07 UTC (permalink / raw) To: Xi Ruoyao; +Cc: gcc-patches, WANG Xuerui, Jiajie Chen, Chenghua Xu 在 2024/2/9 下午4:08, Xi Ruoyao 写道: > On Fri, 2024-02-09 at 00:02 +0800, chenglulu wrote: >> 在 2024/2/7 上午12:23, Xi Ruoyao 写道: >>> Hi Lulu, >>> >>> I'm proposing to backport r14-4674 "LoongArch: Delete macro definition >>> ASM_OUTPUT_ALIGN_WITH_NOP." to releases/gcc-12 and releases/gcc-13. The >>> reasons: >>> >>> 1. Strictly speaking, the old ASM_OUTPUT_ALIGN_WITH_NOP macro may cause >>> a correctness issue. For example, a developer may use -falign- >>> functions=16 and then use the low 4 bits of a function pointer to encode >>> some metainfo. Then ASM_OUTPUT_ALIGN_WITH_NOP causes the functions not >>> really aligned to a 16 bytes boundary, causing some breakage. >>> >>> 2. With Binutils-2.42, ASM_OUTPUT_ALIGN_WITH_NOP can cause illegal >>> opcodes. For example: >>> >>> .globl _start >>> _start: >>> .balign 32 >>> nop >>> nop >>> nop >>> addi.d $a0, $r0, 1 >>> .balign 16,54525952,4 >>> addi.d $a0, $a0, 1 >>> >>> is assembled and linked to: >>> >>> 0000000000000220 <_start>: >>> 220: 03400000 nop >>> 224: 03400000 nop >>> 228: 03400000 nop >>> 22c: 02c00404 li.d $a0, 1 >>> 230: 00000000 .word 0x00000000 # <== OOPS! >>> 234: 02c00484 addi.d $a0, $a0, 1 >>> >>> Arguably this is a bug in GAS (it should at least error out for the >>> unsupported case where .balign 16,54525952,4 appears with -mrelax; I'd >>> prefer it to support the 3-operand .align directive even -mrelax for >>> reasons I've given in [1]). But we can at least work it around by >>> removing ASM_OUTPUT_ALIGN_WITH_NOP to allow using GCC 13.3 with Binutils >>> 2.42. >>> >>> 3. Without ASM_OUTPUT_ALIGN_WITH_NOP, GCC just outputs something like >>> ".align 5" which works as expected since Binutils-2.38. >>> >>> 4. GCC < 14 does not have a default setting of -falign-*, so changing >>> this won't affect anyone who do not specify -falign-* explicitly. >>> >>> [1]:https://github.com/loongson-community/discussions/issues/41#issuecomment-1925872603 >>> >>> Is it OK to backport r14-4674 into releases/gcc-12 and releases/gcc-13 >>> then? >>> >> Ok, I agree with you. >> >> Thanks! > Oops, with Binutils-2.41 GAS will fail to assemble some conditional > branches if we do this :(. > > Not sure what to do (maybe backporting both this and a simplified > version of PR112330 fix?) Let's reconsider after the holiday... > To solve this problem,based on r14-4674, r14-5434 also needs to be transplanted. But I took a look and r14-5434 modified relatively many files. So I think that without worrying about performance and ensuring that there is no problem with binutils, I think we can make the following modifications: -/* "nop" instruction 54525952 (andi $r0,$r0,0) is - used for padding. */ +/* ".align num,,4" will insert "nop"(andi $r0,$r0,0) into padding by + default. */ #define ASM_OUTPUT_ALIGN_WITH_NOP(STREAM, LOG) \ - fprintf (STREAM, "\t.align\t%d,54525952,4\n", (LOG)) + fprintf (STREAM, "\t.align\t%d,,4\n", (LOG)) What do you think of it? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: LoongArch: Backport r14-4674 "LoongArch: Delete macro definition ASM_OUTPUT_ALIGN_WITH_NOP."? 2024-02-20 2:07 ` chenglulu @ 2024-02-20 11:25 ` Xi Ruoyao 2024-02-20 11:31 ` Xi Ruoyao 0 siblings, 1 reply; 9+ messages in thread From: Xi Ruoyao @ 2024-02-20 11:25 UTC (permalink / raw) To: chenglulu; +Cc: gcc-patches, WANG Xuerui, Jiajie Chen, Chenghua Xu On Tue, 2024-02-20 at 10:07 +0800, chenglulu wrote: > So I think that without worrying about performance and ensuring that > there is no problem > > with binutils, I think we can make the following modifications: > > -/* "nop" instruction 54525952 (andi $r0,$r0,0) is > - used for padding. */ > +/* ".align num,,4" will insert "nop"(andi $r0,$r0,0) into padding by > + default. */ > #define ASM_OUTPUT_ALIGN_WITH_NOP(STREAM, LOG) \ > - fprintf (STREAM, "\t.align\t%d,54525952,4\n", (LOG)) > + fprintf (STREAM, "\t.align\t%d,,4\n", (LOG)) > > What do you think of it? Unfortunately it will cause warnings with GAS 2.41 or earlier like t1.s:1: Warning: expected fill pattern missing t1.s:5: Warning: expected fill pattern missing And AFAIK these things may cause many test failures due to "excessive errors" if running the GCC test suite with these earlier GAS versions. Maybe we'll have to add some autoconf-based probing for the linker anyway? -- Xi Ruoyao <xry111@xry111.site> School of Aerospace Science and Technology, Xidian University ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: LoongArch: Backport r14-4674 "LoongArch: Delete macro definition ASM_OUTPUT_ALIGN_WITH_NOP."? 2024-02-20 11:25 ` Xi Ruoyao @ 2024-02-20 11:31 ` Xi Ruoyao 2024-02-20 11:50 ` chenglulu 0 siblings, 1 reply; 9+ messages in thread From: Xi Ruoyao @ 2024-02-20 11:31 UTC (permalink / raw) To: chenglulu; +Cc: gcc-patches, WANG Xuerui, Jiajie Chen, Chenghua Xu On Tue, 2024-02-20 at 19:25 +0800, Xi Ruoyao wrote: > On Tue, 2024-02-20 at 10:07 +0800, chenglulu wrote: > > > So I think that without worrying about performance and ensuring that > > there is no problem > > > > with binutils, I think we can make the following modifications: > > > > -/* "nop" instruction 54525952 (andi $r0,$r0,0) is > > - used for padding. */ > > +/* ".align num,,4" will insert "nop"(andi $r0,$r0,0) into padding by > > + default. */ > > #define ASM_OUTPUT_ALIGN_WITH_NOP(STREAM, LOG) \ > > - fprintf (STREAM, "\t.align\t%d,54525952,4\n", (LOG)) > > + fprintf (STREAM, "\t.align\t%d,,4\n", (LOG)) > > > > What do you think of it? > > Unfortunately it will cause warnings with GAS 2.41 or earlier like > > t1.s:1: Warning: expected fill pattern missing > t1.s:5: Warning: expected fill pattern missing > > And AFAIK these things may cause many test failures due to "excessive > errors" if running the GCC test suite with these earlier GAS versions. > Maybe we'll have to add some autoconf-based probing for the linker > anyway? Or just silence the warning passing "--no-warn" to the assembler but I'm highly unsure if this is really a good idea :(. -- Xi Ruoyao <xry111@xry111.site> School of Aerospace Science and Technology, Xidian University ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: LoongArch: Backport r14-4674 "LoongArch: Delete macro definition ASM_OUTPUT_ALIGN_WITH_NOP."? 2024-02-20 11:31 ` Xi Ruoyao @ 2024-02-20 11:50 ` chenglulu 2024-02-20 11:54 ` Xi Ruoyao 0 siblings, 1 reply; 9+ messages in thread From: chenglulu @ 2024-02-20 11:50 UTC (permalink / raw) To: Xi Ruoyao; +Cc: gcc-patches, WANG Xuerui, Jiajie Chen, Chenghua Xu 在 2024/2/20 下午7:31, Xi Ruoyao 写道: > On Tue, 2024-02-20 at 19:25 +0800, Xi Ruoyao wrote: >> On Tue, 2024-02-20 at 10:07 +0800, chenglulu wrote: >> >>> So I think that without worrying about performance and ensuring that >>> there is no problem >>> >>> with binutils, I think we can make the following modifications: >>> >>> -/* "nop" instruction 54525952 (andi $r0,$r0,0) is >>> - used for padding. */ >>> +/* ".align num,,4" will insert "nop"(andi $r0,$r0,0) into padding by >>> + default. */ >>> #define ASM_OUTPUT_ALIGN_WITH_NOP(STREAM, LOG) \ >>> - fprintf (STREAM, "\t.align\t%d,54525952,4\n", (LOG)) >>> + fprintf (STREAM, "\t.align\t%d,,4\n", (LOG)) >>> >>> What do you think of it? >> Unfortunately it will cause warnings with GAS 2.41 or earlier like >> >> t1.s:1: Warning: expected fill pattern missing >> t1.s:5: Warning: expected fill pattern missing >> >> And AFAIK these things may cause many test failures due to "excessive >> errors" if running the GCC test suite with these earlier GAS versions. >> Maybe we'll have to add some autoconf-based probing for the linker >> anyway? > Or just silence the warning passing "--no-warn" to the assembler but I'm > highly unsure if this is really a good idea :(. > I am not opposed to adding detection code, but I looked at this problem today and I think this change is the smallest change. I asked Meng Qinggang and he said that the warning of GAS 2.41 can be removed. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: LoongArch: Backport r14-4674 "LoongArch: Delete macro definition ASM_OUTPUT_ALIGN_WITH_NOP."? 2024-02-20 11:50 ` chenglulu @ 2024-02-20 11:54 ` Xi Ruoyao 2024-02-20 12:03 ` chenglulu 0 siblings, 1 reply; 9+ messages in thread From: Xi Ruoyao @ 2024-02-20 11:54 UTC (permalink / raw) To: chenglulu; +Cc: gcc-patches, WANG Xuerui, Jiajie Chen, Chenghua Xu On Tue, 2024-02-20 at 19:50 +0800, chenglulu wrote: > > 在 2024/2/20 下午7:31, Xi Ruoyao 写道: > > On Tue, 2024-02-20 at 19:25 +0800, Xi Ruoyao wrote: > > > On Tue, 2024-02-20 at 10:07 +0800, chenglulu wrote: > > > > > > > So I think that without worrying about performance and ensuring that > > > > there is no problem > > > > > > > > with binutils, I think we can make the following modifications: > > > > > > > > -/* "nop" instruction 54525952 (andi $r0,$r0,0) is > > > > - used for padding. */ > > > > +/* ".align num,,4" will insert "nop"(andi $r0,$r0,0) into padding by > > > > + default. */ > > > > #define ASM_OUTPUT_ALIGN_WITH_NOP(STREAM, LOG) \ > > > > - fprintf (STREAM, "\t.align\t%d,54525952,4\n", (LOG)) > > > > + fprintf (STREAM, "\t.align\t%d,,4\n", (LOG)) > > > > > > > > What do you think of it? > > > Unfortunately it will cause warnings with GAS 2.41 or earlier like > > > > > > t1.s:1: Warning: expected fill pattern missing > > > t1.s:5: Warning: expected fill pattern missing > > > > > > And AFAIK these things may cause many test failures due to "excessive > > > errors" if running the GCC test suite with these earlier GAS versions. > > > Maybe we'll have to add some autoconf-based probing for the linker > > > anyway? > > Or just silence the warning passing "--no-warn" to the assembler but I'm > > highly unsure if this is really a good idea :(. > > > I am not opposed to adding detection code, but I looked at this problem > today > > and I think this change is the smallest change. I asked Meng Qinggang and he > > said that the warning of GAS 2.41 can be removed. Yes, but we cannot change a released binutils-2.41 tarball and Binutils folks don't make point releases like GCC. -- Xi Ruoyao <xry111@xry111.site> School of Aerospace Science and Technology, Xidian University ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: LoongArch: Backport r14-4674 "LoongArch: Delete macro definition ASM_OUTPUT_ALIGN_WITH_NOP."? 2024-02-20 11:54 ` Xi Ruoyao @ 2024-02-20 12:03 ` chenglulu 0 siblings, 0 replies; 9+ messages in thread From: chenglulu @ 2024-02-20 12:03 UTC (permalink / raw) To: Xi Ruoyao; +Cc: gcc-patches, WANG Xuerui, Jiajie Chen, Chenghua Xu 在 2024/2/20 下午7:54, Xi Ruoyao 写道: > On Tue, 2024-02-20 at 19:50 +0800, chenglulu wrote: >> 在 2024/2/20 下午7:31, Xi Ruoyao 写道: >>> On Tue, 2024-02-20 at 19:25 +0800, Xi Ruoyao wrote: >>>> On Tue, 2024-02-20 at 10:07 +0800, chenglulu wrote: >>>> >>>>> So I think that without worrying about performance and ensuring that >>>>> there is no problem >>>>> >>>>> with binutils, I think we can make the following modifications: >>>>> >>>>> -/* "nop" instruction 54525952 (andi $r0,$r0,0) is >>>>> - used for padding. */ >>>>> +/* ".align num,,4" will insert "nop"(andi $r0,$r0,0) into padding by >>>>> + default. */ >>>>> #define ASM_OUTPUT_ALIGN_WITH_NOP(STREAM, LOG) \ >>>>> - fprintf (STREAM, "\t.align\t%d,54525952,4\n", (LOG)) >>>>> + fprintf (STREAM, "\t.align\t%d,,4\n", (LOG)) >>>>> >>>>> What do you think of it? >>>> Unfortunately it will cause warnings with GAS 2.41 or earlier like >>>> >>>> t1.s:1: Warning: expected fill pattern missing >>>> t1.s:5: Warning: expected fill pattern missing >>>> >>>> And AFAIK these things may cause many test failures due to "excessive >>>> errors" if running the GCC test suite with these earlier GAS versions. >>>> Maybe we'll have to add some autoconf-based probing for the linker >>>> anyway? >>> Or just silence the warning passing "--no-warn" to the assembler but I'm >>> highly unsure if this is really a good idea :(. >>> >> I am not opposed to adding detection code, but I looked at this problem >> today >> >> and I think this change is the smallest change. I asked Meng Qinggang and he >> >> said that the warning of GAS 2.41 can be removed. > Yes, but we cannot change a released binutils-2.41 tarball and Binutils > folks don't make point releases like GCC. > OK, I agree with you. I will backpoint r14-4674 and r14-5434 to gcc12 and gcc13. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-02-20 12:03 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-02-06 16:23 LoongArch: Backport r14-4674 "LoongArch: Delete macro definition ASM_OUTPUT_ALIGN_WITH_NOP."? Xi Ruoyao 2024-02-08 16:02 ` chenglulu 2024-02-09 8:08 ` Xi Ruoyao 2024-02-20 2:07 ` chenglulu 2024-02-20 11:25 ` Xi Ruoyao 2024-02-20 11:31 ` Xi Ruoyao 2024-02-20 11:50 ` chenglulu 2024-02-20 11:54 ` Xi Ruoyao 2024-02-20 12:03 ` chenglulu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).