Re: [PATCH] powerpc/boot: Delete unneeded .globl _zimage_start
Fangrui Song writes: > .globl sets the symbol binding to STB_GLOBAL while .weak sets the > binding to STB_WEAK. They should not be used together. It is accidetal > rather then intentional that GNU as let .weak override .globl while > clang integrated assembler let the last win. > 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 So it seems the patch is OK but people don't agree with the explanation in the changelog. Please send a v2 that captures the responses from Segher and Alan. cheers > 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] powerpc/boot: Delete unneeded .globl _zimage_start
On Wed, Mar 25, 2020 at 05:22:31AM +, Joel Stanley wrote: > On Wed, 25 Mar 2020 at 05:19, Fangrui Song wrote: > > > > .globl sets the symbol binding to STB_GLOBAL while .weak sets the > > binding to STB_WEAK. They should not be used together. It is accidetal > > rather then intentional that GNU as let .weak override .globl while > > clang integrated assembler let the last win. No, it isn't accidental. gas deliberately lets .weak override .globl. Since 1996-07-26, git commit 5ca547dc239 I'm fine with the patch so far as it is true that there is no need for both .globl and .weak (and it looks silly to have both), but the explanation isn't true. The patch is needed because the clang assembler is incompatible with gas in this detail. > > 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 > > Cc: Joel Stanley > > Cc: Michael Ellerman > > Cc: Nick Desaulniers > > Cc: clang-built-li...@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: > > Your explanation makes sense to me. I've added Alan to cc for his review. > > Reviewed-by: Joel Stanley > > Thanks for the patch. > > Cheers, > > Joel > > > .globl _zimage_start_lib > > -- > > 2.25.1.696.g5e7596f4ac-goog -- Alan Modra Australia Development Lab, IBM
Re: [PATCH] powerpc/boot: Delete unneeded .globl _zimage_start
On Tue, Mar 24, 2020 at 10:18:20PM -0700, Fangrui Song wrote: > .globl sets the symbol binding to STB_GLOBAL while .weak sets the > binding to STB_WEAK. They should not be used together. It is accidetal > rather then intentional that GNU as let .weak override .globl while > clang integrated assembler let the last win. Nothing is "overridden". The as manual says (.weak): This directive sets the weak attribute on the comma separated list of symbol 'names'. If the symbols do not already exist, they will be created. so this behaviour is obviously as intended (or was later documented in any case), so LLVM has a bug to fix (whether you like this (much saner) behaviour or not). Segher
Re: [PATCH] powerpc/boot: Delete unneeded .globl _zimage_start
On Wed, 25 Mar 2020 at 05:19, Fangrui Song wrote: > > .globl sets the symbol binding to STB_GLOBAL while .weak sets the > binding to STB_WEAK. They should not be used together. It is accidetal > rather then intentional that GNU as let .weak override .globl while > clang integrated assembler let the last win. > > 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 > Cc: Joel Stanley > Cc: Michael Ellerman > Cc: Nick Desaulniers > Cc: clang-built-li...@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: Your explanation makes sense to me. I've added Alan to cc for his review. Reviewed-by: Joel Stanley Thanks for the patch. Cheers, Joel > .globl _zimage_start_lib > -- > 2.25.1.696.g5e7596f4ac-goog >
