* [PATCH] powerpc/boot: Delete unneeded .globl _zimage_start
@ 2020-03-25 5:18 Fangrui Song
2020-03-25 5:22 ` Joel Stanley
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Fangrui Song @ 2020-03-25 5:18 UTC (permalink / raw)
To: linuxppc-dev
Cc: Nick Desaulniers, Joel Stanley, Fangrui Song, clang-built-linux
.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 <maskray@google.com>
Cc: Joel Stanley <joel@jms.id.au>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nick Desaulniers <ndesaulniers@google.com>
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/boot: Delete unneeded .globl _zimage_start
2020-03-25 5:18 [PATCH] powerpc/boot: Delete unneeded .globl _zimage_start Fangrui Song
@ 2020-03-25 5:22 ` Joel Stanley
2020-03-25 8:37 ` Alan Modra
2020-03-25 6:23 ` Segher Boessenkool
2020-03-25 10:42 ` Michael Ellerman
2 siblings, 1 reply; 6+ messages in thread
From: Joel Stanley @ 2020-03-25 5:22 UTC (permalink / raw)
To: Fangrui Song, Alan Modra
Cc: Nick Desaulniers, linuxppc-dev, clang-built-linux
On Wed, 25 Mar 2020 at 05:19, Fangrui Song <maskray@google.com> 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 <maskray@google.com>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> 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:
Your explanation makes sense to me. I've added Alan to cc for his review.
Reviewed-by: Joel Stanley <joel@jms.id.au>
Thanks for the patch.
Cheers,
Joel
> .globl _zimage_start_lib
> --
> 2.25.1.696.g5e7596f4ac-goog
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/boot: Delete unneeded .globl _zimage_start
2020-03-25 5:18 [PATCH] powerpc/boot: Delete unneeded .globl _zimage_start Fangrui Song
2020-03-25 5:22 ` Joel Stanley
@ 2020-03-25 6:23 ` Segher Boessenkool
2020-03-25 10:42 ` Michael Ellerman
2 siblings, 0 replies; 6+ messages in thread
From: Segher Boessenkool @ 2020-03-25 6:23 UTC (permalink / raw)
To: Fangrui Song
Cc: Nick Desaulniers, linuxppc-dev, Joel Stanley, clang-built-linux
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/boot: Delete unneeded .globl _zimage_start
2020-03-25 5:22 ` Joel Stanley
@ 2020-03-25 8:37 ` Alan Modra
2020-03-25 16:34 ` Fangrui Song
0 siblings, 1 reply; 6+ messages in thread
From: Alan Modra @ 2020-03-25 8:37 UTC (permalink / raw)
To: Joel Stanley
Cc: Nick Desaulniers, linuxppc-dev, Fangrui Song, clang-built-linux
On Wed, Mar 25, 2020 at 05:22:31AM +0000, Joel Stanley wrote:
> On Wed, 25 Mar 2020 at 05:19, Fangrui Song <maskray@google.com> 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 <maskray@google.com>
> > Cc: Joel Stanley <joel@jms.id.au>
> > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > 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:
>
> Your explanation makes sense to me. I've added Alan to cc for his review.
>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
>
> Thanks for the patch.
>
> Cheers,
>
> Joel
>
> > .globl _zimage_start_lib
> > --
> > 2.25.1.696.g5e7596f4ac-goog
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/boot: Delete unneeded .globl _zimage_start
2020-03-25 5:18 [PATCH] powerpc/boot: Delete unneeded .globl _zimage_start Fangrui Song
2020-03-25 5:22 ` Joel Stanley
2020-03-25 6:23 ` Segher Boessenkool
@ 2020-03-25 10:42 ` Michael Ellerman
2 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2020-03-25 10:42 UTC (permalink / raw)
To: Fangrui Song, linuxppc-dev
Cc: clang-built-linux, Nick Desaulniers, Joel Stanley, Fangrui Song
Fangrui Song <maskray@google.com> 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 <maskray@google.com>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/boot: Delete unneeded .globl _zimage_start
2020-03-25 8:37 ` Alan Modra
@ 2020-03-25 16:34 ` Fangrui Song
0 siblings, 0 replies; 6+ messages in thread
From: Fangrui Song @ 2020-03-25 16:34 UTC (permalink / raw)
To: Alan Modra
Cc: Nick Desaulniers, clang-built-linux, Joel Stanley, linuxppc-dev
On 2020-03-25, Alan Modra wrote:
>On Wed, Mar 25, 2020 at 05:22:31AM +0000, Joel Stanley wrote:
>> On Wed, 25 Mar 2020 at 05:19, Fangrui Song <maskray@google.com> 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
Fair. I am sadded by this commit.
% sed -n 'N;/.globl.*\n.*.weak/p;D' binutils-gdb/gas/testsuite/**/*(.)
I checked all occurrences and none is a real test excercising the behavior (.weak override .globl).
All seem accidental.
It is unclear that clang integrated assembler should copy this
behavior, though. For the record, I asked on binutils@sourceware.org
whether the assembler should error when .weak/.local can override a
previous binding directive. It was rejected
https://sourceware.org/pipermail/binutils/2020-March/110376.html
On the clang integrated assembler side, we may try building things with
an error or leave the overriding behavior as is.
>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.
Since using one of .weak|.globl is nearly well-known, I'll send PATCH v2 with
the description updated.
On 2020-03-25, Segher Boessenkool wrote:
>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).
I will probably not call this a bug. I have recently discovered other
discrepancy for which I think copying gas behaviors can just clutter up the
code. We may need our own assembler documentation at some point.
>> > 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: Joel Stanley <joel@jms.id.au>
>> > Cc: Michael Ellerman <mpe@ellerman.id.au>
>> > Cc: Nick Desaulniers <ndesaulniers@google.com>
>> > 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:
>>
>> Your explanation makes sense to me. I've added Alan to cc for his review.
>>
>> Reviewed-by: Joel Stanley <joel@jms.id.au>
>>
>> Thanks for the patch.
>>
>> Cheers,
>>
>> Joel
>>
>> > .globl _zimage_start_lib
>> > --
>> > 2.25.1.696.g5e7596f4ac-goog
>
>--
>Alan Modra
>Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-25 16:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-25 5:18 [PATCH] powerpc/boot: Delete unneeded .globl _zimage_start Fangrui Song
2020-03-25 5:22 ` Joel Stanley
2020-03-25 8:37 ` Alan Modra
2020-03-25 16:34 ` Fangrui Song
2020-03-25 6:23 ` Segher Boessenkool
2020-03-25 10:42 ` Michael Ellerman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.