[PATCH v2] arm32: Avoid using solaris syntax for .section directive

[PATCH v2] arm32: Avoid using solaris syntax for .section directive

[Khem Raj]

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(-)

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
-- 
2.41.0

Re: [PATCH v2] arm32: Avoid using solaris syntax for .section directive

[Julien Grall]

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,

-- 
Julien Grall

Re: [PATCH v2] arm32: Avoid using solaris syntax for .section directive

[Khem Raj]

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

Re: [PATCH v2] arm32: Avoid using solaris syntax for .section directive

[Michal Orzel]

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

Re: [PATCH v2] arm32: Avoid using solaris syntax for .section directive

[Jan Beulich]

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,
> 

Re: [PATCH v2] arm32: Avoid using solaris syntax for .section directive

[Julien Grall]

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,

-- 
Julien Grall

Re: [PATCH v2] arm32: Avoid using solaris syntax for .section directive

[Jan Beulich]

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

Re: [PATCH v2] arm32: Avoid using solaris syntax for .section directive

[Julien Grall]

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,

-- 
Julien Grall

Re: [PATCH v2] arm32: Avoid using solaris syntax for .section directive

[Julien Grall]

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,

-- 
Julien Grall