| Message ID | 20230801174930.2995947-1-raj.khem@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] arm32: Avoid using solaris syntax for .section directive | expand |
Hi, Title: This patch is not arm32 specific anymore. So I would replace 'arm32' with 'arm'. This can be done on commit. On 01/08/2023 18:49, Khem Raj wrote: > Assembler from binutils 2.41 rejects [1] this syntax > > .section "name"[, flags...] > > where flags could be #alloc, #write, #execinstr, #exclude, and #tls [2] > > It is almost like a regression compared to 2.40 or older release, The next word after ',' start with an uppercase. Did you intend to use '.' rather than ','? That said, the documentation has the following: For SPARC ELF targets, the assembler supports another type of .section directive for compatibility with the Solaris assembler:" This leads me to think this is not a regression and instead an intended behavior (even though it breaks older build) even it breaks build. I would suggest to reword the commit message to: " Assembler from binutiles 2.41 will rejects ([1], [2]) the following syntax .section "name", #alloc for any other any target other than ELF SPARC. This means we can't use it in the Arm code. So switch to the GNU syntax .section name [, "flags"[, @type]] [1] https://sourceware.org/bugzilla/show_bug.cgi?id=11601 [2] https://sourceware.org/binutils/docs-2.41/as.html#Section If you agree with the commit message, I can update it while committing. We should also consider to backport. Cheers,
On Tue, Aug 1, 2023 at 2:03 PM Julien Grall <julien@xen.org> wrote: > > Hi, > > Title: This patch is not arm32 specific anymore. So I would replace > 'arm32' with 'arm'. This can be done on commit. > > On 01/08/2023 18:49, Khem Raj wrote: > > Assembler from binutils 2.41 rejects [1] this syntax > > > > .section "name"[, flags...] > > > > where flags could be #alloc, #write, #execinstr, #exclude, and #tls [2] > > > > It is almost like a regression compared to 2.40 or older release, > > The next word after ',' start with an uppercase. Did you intend to use > '.' rather than ','? > > That said, the documentation has the following: > > For SPARC ELF targets, the assembler supports another type of .section > directive for compatibility with the Solaris assembler:" > > This leads me to think this is not a regression and instead an intended > behavior (even though it breaks older build) even it breaks build. > > I would suggest to reword the commit message to: > > " > Assembler from binutiles 2.41 will rejects ([1], [2]) the following syntax > > .section "name", #alloc > > for any other any target other than ELF SPARC. This means we can't use > it in the Arm code. > > So switch to the GNU syntax > > .section name [, "flags"[, @type]] > > [1] https://sourceware.org/bugzilla/show_bug.cgi?id=11601 > [2] https://sourceware.org/binutils/docs-2.41/as.html#Section > > If you agree with the commit message, I can update it while committing. LGTM, go ahead. > > We should also consider to backport. > > Cheers, > > -- > Julien Grall
Hi, On 01/08/2023 19:49, Khem Raj wrote: > > > Assembler from binutils 2.41 rejects [1] this syntax > > .section "name"[, flags...] > > where flags could be #alloc, #write, #execinstr, #exclude, and #tls [2] > > It is almost like a regression compared to 2.40 or older release, > It likely went unnoticed so far because Linux kernel changed > to GNU syntax already in 5.5, to allow building with Clang's > integrated assembler. > > Switch to using GNU syntax > > .section name[, "flags"[, @type]] > > [1] https://sourceware.org/bugzilla/show_bug.cgi?id=11601 > [2] https://sourceware.org/binutils/docs-2.41/as.html#Section > > Signed-off-by: Khem Raj <raj.khem@gmail.com> With the changes suggested by Julien: Reviewed-by: Michal Orzel <michal.orzel@amd.com> ~Michal
On 01.08.2023 23:02, Julien Grall wrote: > Hi, > > Title: This patch is not arm32 specific anymore. So I would replace > 'arm32' with 'arm'. This can be done on commit. > > On 01/08/2023 18:49, Khem Raj wrote: >> Assembler from binutils 2.41 rejects [1] this syntax >> >> .section "name"[, flags...] >> >> where flags could be #alloc, #write, #execinstr, #exclude, and #tls [2] >> >> It is almost like a regression compared to 2.40 or older release, > > The next word after ',' start with an uppercase. Did you intend to use > '.' rather than ','? > > That said, the documentation has the following: > > For SPARC ELF targets, the assembler supports another type of .section > directive for compatibility with the Solaris assembler:" But note that "SPARC" was added there only by the commit introducing the perceived regression. Jan > This leads me to think this is not a regression and instead an intended > behavior (even though it breaks older build) even it breaks build. > > I would suggest to reword the commit message to: > > " > Assembler from binutiles 2.41 will rejects ([1], [2]) the following syntax > > .section "name", #alloc > > for any other any target other than ELF SPARC. This means we can't use > it in the Arm code. > > So switch to the GNU syntax > > .section name [, "flags"[, @type]] > > [1] https://sourceware.org/bugzilla/show_bug.cgi?id=11601 > [2] https://sourceware.org/binutils/docs-2.41/as.html#Section > > If you agree with the commit message, I can update it while committing. > > We should also consider to backport. > > Cheers, >
Hi Jan, On 02/08/2023 08:22, Jan Beulich wrote: > On 01.08.2023 23:02, Julien Grall wrote: >> Hi, >> >> Title: This patch is not arm32 specific anymore. So I would replace >> 'arm32' with 'arm'. This can be done on commit. >> >> On 01/08/2023 18:49, Khem Raj wrote: >>> Assembler from binutils 2.41 rejects [1] this syntax >>> >>> .section "name"[, flags...] >>> >>> where flags could be #alloc, #write, #execinstr, #exclude, and #tls [2] >>> >>> It is almost like a regression compared to 2.40 or older release, >> >> The next word after ',' start with an uppercase. Did you intend to use >> '.' rather than ','? >> >> That said, the documentation has the following: >> >> For SPARC ELF targets, the assembler supports another type of .section >> directive for compatibility with the Solaris assembler:" > > But note that "SPARC" was added there only by the commit introducing the > perceived regression. Yes, I noticed it while replying yesterday. I still would not describe it as a regression mainly because I am not convinced binutils will revert the change and it feels like a good move. Also, regarding your point about older tree on the bug report. I don't think we guarantee that stable works all new toolchain without any change. Cheers,
On 02.08.2023 09:54, Julien Grall wrote: > On 02/08/2023 08:22, Jan Beulich wrote: >> On 01.08.2023 23:02, Julien Grall wrote: >>> Title: This patch is not arm32 specific anymore. So I would replace >>> 'arm32' with 'arm'. This can be done on commit. >>> >>> On 01/08/2023 18:49, Khem Raj wrote: >>>> Assembler from binutils 2.41 rejects [1] this syntax >>>> >>>> .section "name"[, flags...] >>>> >>>> where flags could be #alloc, #write, #execinstr, #exclude, and #tls [2] >>>> >>>> It is almost like a regression compared to 2.40 or older release, >>> >>> The next word after ',' start with an uppercase. Did you intend to use >>> '.' rather than ','? >>> >>> That said, the documentation has the following: >>> >>> For SPARC ELF targets, the assembler supports another type of .section >>> directive for compatibility with the Solaris assembler:" >> >> But note that "SPARC" was added there only by the commit introducing the >> perceived regression. > > Yes, I noticed it while replying yesterday. I still would not describe > it as a regression mainly because I am not convinced binutils will > revert the change and it feels like a good move. I agree as to it being unlikely that the change is going to be (partly) reverted, unless someone strongly advocated for it. It also wouldn't be of much use to us, unless a 2.41.1 was cut very soon. > Also, regarding your point about older tree on the bug report. I don't > think we guarantee that stable works all new toolchain without any change. We don't guarantee that, no, but I think it is in our own interest to keep things building with the newest tool chains. When build-testing stable tree commits before pushing, I don't want to always have to remember to force the build to use an older tool chain, when normally my build rune would default to a pretty up-to-date one. This is also why for this specific class of changes I typically prefer to see them also go onto the security-only stable trees. Jan
Hi Jan, On 02/08/2023 09:01, Jan Beulich wrote: > On 02.08.2023 09:54, Julien Grall wrote: >> On 02/08/2023 08:22, Jan Beulich wrote: >>> On 01.08.2023 23:02, Julien Grall wrote: >>>> Title: This patch is not arm32 specific anymore. So I would replace >>>> 'arm32' with 'arm'. This can be done on commit. >>>> >>>> On 01/08/2023 18:49, Khem Raj wrote: >>>>> Assembler from binutils 2.41 rejects [1] this syntax >>>>> >>>>> .section "name"[, flags...] >>>>> >>>>> where flags could be #alloc, #write, #execinstr, #exclude, and #tls [2] >>>>> >>>>> It is almost like a regression compared to 2.40 or older release, >>>> >>>> The next word after ',' start with an uppercase. Did you intend to use >>>> '.' rather than ','? >>>> >>>> That said, the documentation has the following: >>>> >>>> For SPARC ELF targets, the assembler supports another type of .section >>>> directive for compatibility with the Solaris assembler:" >>> >>> But note that "SPARC" was added there only by the commit introducing the >>> perceived regression. >> >> Yes, I noticed it while replying yesterday. I still would not describe >> it as a regression mainly because I am not convinced binutils will >> revert the change and it feels like a good move. > > I agree as to it being unlikely that the change is going to be (partly) > reverted, unless someone strongly advocated for it. It also wouldn't be > of much use to us, unless a 2.41.1 was cut very soon. > >> Also, regarding your point about older tree on the bug report. I don't >> think we guarantee that stable works all new toolchain without any change. > > We don't guarantee that, no, but I think it is in our own interest to > keep things building with the newest tool chains. When build-testing > stable tree commits before pushing, I don't want to always have to > remember to force the build to use an older tool chain, when normally > my build rune would default to a pretty up-to-date one. This is where gitlab would definitely help because we can keep containers around to build older Xen versions. > This is also > why for this specific class of changes I typically prefer to see them > also go onto the security-only stable trees. I am ok if this is backported to older tree. This is trivial enough. Cheers,
Hi, On 01/08/2023 22:49, Khem Raj wrote: > On Tue, Aug 1, 2023 at 2:03 PM Julien Grall <julien@xen.org> wrote: >> >> Hi, >> >> Title: This patch is not arm32 specific anymore. So I would replace >> 'arm32' with 'arm'. This can be done on commit. >> >> On 01/08/2023 18:49, Khem Raj wrote: >>> Assembler from binutils 2.41 rejects [1] this syntax >>> >>> .section "name"[, flags...] >>> >>> where flags could be #alloc, #write, #execinstr, #exclude, and #tls [2] >>> >>> It is almost like a regression compared to 2.40 or older release, >> >> The next word after ',' start with an uppercase. Did you intend to use >> '.' rather than ','? >> >> That said, the documentation has the following: >> >> For SPARC ELF targets, the assembler supports another type of .section >> directive for compatibility with the Solaris assembler:" >> >> This leads me to think this is not a regression and instead an intended >> behavior (even though it breaks older build) even it breaks build. >> >> I would suggest to reword the commit message to: >> >> " >> Assembler from binutiles 2.41 will rejects ([1], [2]) the following syntax >> >> .section "name", #alloc >> >> for any other any target other than ELF SPARC. This means we can't use >> it in the Arm code. >> >> So switch to the GNU syntax >> >> .section name [, "flags"[, @type]] >> >> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=11601 >> [2] https://sourceware.org/binutils/docs-2.41/as.html#Section >> >> If you agree with the commit message, I can update it while committing. > > LGTM, go ahead. I have committed with: Acked-by: Julien Grall <jgrall@amazon.com> Cheers,
diff --git a/xen/arch/arm/arm32/proc-v7.S b/xen/arch/arm/arm32/proc-v7.S index c90a31d80f..6d3d19b873 100644 --- a/xen/arch/arm/arm32/proc-v7.S +++ b/xen/arch/arm/arm32/proc-v7.S @@ -29,7 +29,7 @@ brahma15mp_init: mcr CP32(r0, ACTLR) mov pc, lr - .section ".proc.info", #alloc + .section .proc.info, "a" .type __v7_ca15mp_proc_info, #object __v7_ca15mp_proc_info: .long 0x410FC0F0 /* Cortex-A15 */ @@ -38,7 +38,7 @@ __v7_ca15mp_proc_info: .long caxx_processor .size __v7_ca15mp_proc_info, . - __v7_ca15mp_proc_info - .section ".proc.info", #alloc + .section .proc.info, "a" .type __v7_ca7mp_proc_info, #object __v7_ca7mp_proc_info: .long 0x410FC070 /* Cortex-A7 */ @@ -47,7 +47,7 @@ __v7_ca7mp_proc_info: .long caxx_processor .size __v7_ca7mp_proc_info, . - __v7_ca7mp_proc_info - .section ".proc.info", #alloc + .section .proc.info, "a" .type __v7_brahma15mp_proc_info, #object __v7_brahma15mp_proc_info: .long 0x420F00F0 /* Broadcom Brahma-B15 */ diff --git a/xen/arch/arm/dtb.S b/xen/arch/arm/dtb.S index c39f3a095c..386f83ba64 100644 --- a/xen/arch/arm/dtb.S +++ b/xen/arch/arm/dtb.S @@ -1,3 +1,3 @@ - .section .dtb,#alloc + .section .dtb, "a" GLOBAL(_sdtb) .incbin CONFIG_DTB_FILE
Assembler from binutils 2.41 rejects [1] this syntax .section "name"[, flags...] where flags could be #alloc, #write, #execinstr, #exclude, and #tls [2] It is almost like a regression compared to 2.40 or older release, It likely went unnoticed so far because Linux kernel changed to GNU syntax already in 5.5, to allow building with Clang's integrated assembler. Switch to using GNU syntax .section name[, "flags"[, @type]] [1] https://sourceware.org/bugzilla/show_bug.cgi?id=11601 [2] https://sourceware.org/binutils/docs-2.41/as.html#Section Signed-off-by: Khem Raj <raj.khem@gmail.com> --- v1 -> v2: - Improvise on commit message - Make similar change in xen/arch/arm/dtb.S xen/arch/arm/arm32/proc-v7.S | 6 +++--- xen/arch/arm/dtb.S | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-)