[PATCH v2] powerpc/boot: Delete unneeded .globl _zimage_start

[PATCH v2] powerpc/boot: Delete unneeded .globl _zimage_start

[Fangrui Song]

.globl sets the symbol binding to STB_GLOBAL while .weak sets the
binding to STB_WEAK. GNU as let .weak override .globl since binutils-gdb
5ca547dc2399a0a5d9f20626d4bf5547c3ccfddd (1996). Clang integrated
assembler let the last win but it may error in the future.

Since it is a convention that only one binding directive is used, just
delete .globl.

Fixes: cd197ffcf10b "[POWERPC] zImage: Cleanup and improve zImage entry point"
Fixes: ee9d21b3b358 "powerpc/boot: Ensure _zimage_start is a weak symbol"
Link: https://github.com/ClangBuiltLinux/linux/issues/937
Signed-off-by: Fangrui Song <maskray@google.com>
Cc: Alan Modra <amodra@gmail.com>
Cc: Joel Stanley <joel@jms.id.au>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Segher Boessenkool <segher@kernel.crashing.org>
Cc: clang-built-linux@googlegroups.com
---
 arch/powerpc/boot/crt0.S | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/powerpc/boot/crt0.S b/arch/powerpc/boot/crt0.S
index 92608f34d312..1d83966f5ef6 100644
--- a/arch/powerpc/boot/crt0.S
+++ b/arch/powerpc/boot/crt0.S
@@ -44,9 +44,6 @@ p_end:		.long	_end
 p_pstack:	.long	_platform_stack_top
 #endif
 
-	.globl	_zimage_start
-	/* Clang appears to require the .weak directive to be after the symbol
-	 * is defined. See https://bugs.llvm.org/show_bug.cgi?id=38921  */
 	.weak	_zimage_start
 _zimage_start:
 	.globl	_zimage_start_lib
-- 
2.25.1.696.g5e7596f4ac-goog

Re: [PATCH v2] powerpc/boot: Delete unneeded .globl _zimage_start

[Segher Boessenkool]

On Wed, Mar 25, 2020 at 09:42:57AM -0700, Fangrui Song wrote:
> .globl sets the symbol binding to STB_GLOBAL while .weak sets the
> binding to STB_WEAK. GNU as let .weak override .globl since binutils-gdb
> 5ca547dc2399a0a5d9f20626d4bf5547c3ccfddd (1996). Clang integrated
> assembler let the last win but it may error in the future.

GNU AS works for more than just ELF.  The way the assembler language
is defined, it is not .weak overriding .globl -- instead, .weak sets a
symbol attribute.  On an existing symbol (but it creates on if there is
none yet).

Clang is buggy if it does not allow valid (and perfectly normal)
assembler code like this.


Segher

Re: [PATCH v2] powerpc/boot: Delete unneeded .globl _zimage_start

[Fangrui Song]

On 2020-03-26, Segher Boessenkool wrote:
>On Wed, Mar 25, 2020 at 09:42:57AM -0700, Fangrui Song wrote:
>> .globl sets the symbol binding to STB_GLOBAL while .weak sets the
>> binding to STB_WEAK. GNU as let .weak override .globl since binutils-gdb
>> 5ca547dc2399a0a5d9f20626d4bf5547c3ccfddd (1996). Clang integrated
>> assembler let the last win but it may error in the future.
>
>GNU AS works for more than just ELF.  The way the assembler language
>is defined, it is not .weak overriding .globl -- instead, .weak sets a
>symbol attribute.  On an existing symbol (but it creates on if there is
>none yet).
>
>Clang is buggy if it does not allow valid (and perfectly normal)
>assembler code like this.

https://sourceware.org/pipermail/binutils/2020-March/110399.html

Alan: "I think it is completely fine for you to make the llvm assembler
error on inconsistent binding, or the last directive win.  Either of
those behaviours is logical and good, but you quite possibly will run
into a need to fix more user assembly.

I am doing some experiments whether making clang integrated assembler
error is feasible.

Re: [PATCH v2] powerpc/boot: Delete unneeded .globl _zimage_start

[Segher Boessenkool]

On Thu, Mar 26, 2020 at 03:26:12PM -0700, Fangrui Song wrote:
> On 2020-03-26, Segher Boessenkool wrote:
> >On Wed, Mar 25, 2020 at 09:42:57AM -0700, Fangrui Song wrote:
> >>.globl sets the symbol binding to STB_GLOBAL while .weak sets the
> >>binding to STB_WEAK. GNU as let .weak override .globl since binutils-gdb
> >>5ca547dc2399a0a5d9f20626d4bf5547c3ccfddd (1996). Clang integrated
> >>assembler let the last win but it may error in the future.
> >
> >GNU AS works for more than just ELF.  The way the assembler language
> >is defined, it is not .weak overriding .globl -- instead, .weak sets a
> >symbol attribute.  On an existing symbol (but it creates on if there is
> >none yet).
> >
> >Clang is buggy if it does not allow valid (and perfectly normal)
> >assembler code like this.
> 
> https://sourceware.org/pipermail/binutils/2020-March/110399.html
> 
> Alan: "I think it is completely fine for you to make the llvm assembler
> error on inconsistent binding, or the last directive win.  Either of
> those behaviours is logical and good, but you quite possibly will run
> into a need to fix more user assembly.

This would be fine and consistent behaviour, of course.  But it is not
appropriate if you want to pretend to be compatible to GNU toolchains.

Which is exactly why you want this kernel patch at all.  And the kernel
can (in this case) accommodate your buggy assembler, sure, but are you
going to "fix" all other programs with this "problem" as well?


Segher

Re: [PATCH v2] powerpc/boot: Delete unneeded .globl _zimage_start

[Fangrui Song]

On 2020-03-27, Segher Boessenkool wrote:
>On Thu, Mar 26, 2020 at 03:26:12PM -0700, Fangrui Song wrote:
>> On 2020-03-26, Segher Boessenkool wrote:
>> >On Wed, Mar 25, 2020 at 09:42:57AM -0700, Fangrui Song wrote:
>> >>.globl sets the symbol binding to STB_GLOBAL while .weak sets the
>> >>binding to STB_WEAK. GNU as let .weak override .globl since binutils-gdb
>> >>5ca547dc2399a0a5d9f20626d4bf5547c3ccfddd (1996). Clang integrated
>> >>assembler let the last win but it may error in the future.
>> >
>> >GNU AS works for more than just ELF.  The way the assembler language
>> >is defined, it is not .weak overriding .globl -- instead, .weak sets a
>> >symbol attribute.  On an existing symbol (but it creates on if there is
>> >none yet).
>> >
>> >Clang is buggy if it does not allow valid (and perfectly normal)
>> >assembler code like this.
>>
>> https://sourceware.org/pipermail/binutils/2020-March/110399.html
>>
>> Alan: "I think it is completely fine for you to make the llvm assembler
>> error on inconsistent binding, or the last directive win.  Either of
>> those behaviours is logical and good, but you quite possibly will run
>> into a need to fix more user assembly.
>
>This would be fine and consistent behaviour, of course.  But it is not
>appropriate if you want to pretend to be compatible to GNU toolchains.

We aim for compatibility with GNU in many aspects to make it easier for
people to switch over. However, just because there is a subtle behavior
in GNU toolchain does not mean we need to emulate that behavior. With
all due respect, there are a large quantity of legacy behaviors we don't
want to support. Quite interestingly, many times such behaviors turn out
to be not well tested - they are documented by git blame/log.

Building kernel with another mature toolchain is a good way to shake out
code that relies on undefined/subtle behaviors. The efforts improve
health of the kernel.

It may be a bit more off-topic now. I am more confident on linker/binary
utilities side. Not emulating traditional behaviors turns out to be a
great success for lld (LLVM linker). We managed to create a linker with
23+k lines of code which is able to build a majority of software. In
FreeBSD ports, 32k pieces of software just work, 130+ packages are
marked as LLD_UNSAFE, but many should be safe (need developers' testing)
as of lld 9.

>Which is exactly why you want this kernel patch at all.  And the kernel
>can (in this case) accommodate your buggy assembler, sure, but are you
>going to "fix" all other programs with this "problem" as well?
>
>Segher

For this particularly case, A "blanked write privs" binutils maintainer
acknowledged clang integrated assembler's behavior. Another "blanked
write privs" (but inactive) binutils maintainer does not feel strong
about his decision made 24 years ago.  With respect, I should mention
that our design decisions do not need their approval.  That said, we
will be careful with the these decisions because the choices may affect
several companies and several larger code bases.

This is why I mentioned in my previous message that I want to
experiment. I will try out the error on some large code bases.  Nick may
be able to help on Android side. Additionally, we may get help from
FreeBSD folks.

If you subscribe to binutils@sourceware.org, you may find in recent
months I am quite active there. I am a very tiny contributor there but I
try to communicate clang/LLVM binary utilities/lld's discrepancy (in
terms of traditional behaviors) and report issues to binutils. Hope
clang's success can give incentive to improve binutils code health as
well.


Cheers,
Fangrui

Re: [PATCH v2] powerpc/boot: Delete unneeded .globl _zimage_start

[Segher Boessenkool]

On Fri, Mar 27, 2020 at 09:50:54AM -0700, Fangrui Song wrote:
> We aim for compatibility with GNU in many aspects to make it easier for
> people to switch over. However, just because there is a subtle behavior
> in GNU toolchain does not mean we need to emulate that behavior.

It isn't subtle.  It is the explicit documented behaviour.

> With
> all due respect, there are a large quantity of legacy behaviors we don't
> want to support.

And it isn't legacy at all.  It is simply the defined semantics.

It is semantics that real code relies on (and has for ages) as well.


Segher

Re: [PATCH v2] powerpc/boot: Delete unneeded .globl _zimage_start

[Michael Ellerman]

On Wed, 2020-03-25 at 16:42:57 UTC, Fangrui Song wrote:
> .globl sets the symbol binding to STB_GLOBAL while .weak sets the
> binding to STB_WEAK. GNU as let .weak override .globl since binutils-gdb
> 5ca547dc2399a0a5d9f20626d4bf5547c3ccfddd (1996). Clang integrated
> assembler let the last win but it may error in the future.
> 
> Since it is a convention that only one binding directive is used, just
> delete .globl.
> 
> Fixes: cd197ffcf10b "[POWERPC] zImage: Cleanup and improve zImage entry point"
> Fixes: ee9d21b3b358 "powerpc/boot: Ensure _zimage_start is a weak symbol"
> Link: https://github.com/ClangBuiltLinux/linux/issues/937
> Signed-off-by: Fangrui Song <maskray@google.com>
> Cc: Alan Modra <amodra@gmail.com>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Segher Boessenkool <segher@kernel.crashing.org>
> Cc: clang-built-linux@googlegroups.com

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/968339fad422a58312f67718691b717dac45c399

cheers