* [PATCH v2] powerpc/boot: Delete unneeded .globl _zimage_start @ 2020-03-25 16:42 Fangrui Song 2020-03-26 22:16 ` Segher Boessenkool 2020-04-01 12:53 ` Michael Ellerman 0 siblings, 2 replies; 7+ messages in thread From: Fangrui Song @ 2020-03-25 16:42 UTC (permalink / raw) To: linuxppc-dev Cc: Fangrui Song, Alan Modra, Nick Desaulniers, clang-built-linux, Joel Stanley .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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc/boot: Delete unneeded .globl _zimage_start 2020-03-25 16:42 [PATCH v2] powerpc/boot: Delete unneeded .globl _zimage_start Fangrui Song @ 2020-03-26 22:16 ` Segher Boessenkool 2020-03-26 22:26 ` Fangrui Song 2020-04-01 12:53 ` Michael Ellerman 1 sibling, 1 reply; 7+ messages in thread From: Segher Boessenkool @ 2020-03-26 22:16 UTC (permalink / raw) To: Fangrui Song Cc: Alan Modra, Nick Desaulniers, clang-built-linux, Joel Stanley, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc/boot: Delete unneeded .globl _zimage_start 2020-03-26 22:16 ` Segher Boessenkool @ 2020-03-26 22:26 ` Fangrui Song 2020-03-27 15:24 ` Segher Boessenkool 0 siblings, 1 reply; 7+ messages in thread From: Fangrui Song @ 2020-03-26 22:26 UTC (permalink / raw) To: Segher Boessenkool Cc: Alan Modra, Nick Desaulniers, clang-built-linux, Joel Stanley, linuxppc-dev 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc/boot: Delete unneeded .globl _zimage_start 2020-03-26 22:26 ` Fangrui Song @ 2020-03-27 15:24 ` Segher Boessenkool 2020-03-27 16:50 ` Fangrui Song 0 siblings, 1 reply; 7+ messages in thread From: Segher Boessenkool @ 2020-03-27 15:24 UTC (permalink / raw) To: Fangrui Song Cc: Alan Modra, Nick Desaulniers, clang-built-linux, Joel Stanley, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc/boot: Delete unneeded .globl _zimage_start 2020-03-27 15:24 ` Segher Boessenkool @ 2020-03-27 16:50 ` Fangrui Song 2020-03-27 18:27 ` Segher Boessenkool 0 siblings, 1 reply; 7+ messages in thread From: Fangrui Song @ 2020-03-27 16:50 UTC (permalink / raw) To: Segher Boessenkool Cc: Alan Modra, Nick Desaulniers, clang-built-linux, Joel Stanley, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc/boot: Delete unneeded .globl _zimage_start 2020-03-27 16:50 ` Fangrui Song @ 2020-03-27 18:27 ` Segher Boessenkool 0 siblings, 0 replies; 7+ messages in thread From: Segher Boessenkool @ 2020-03-27 18:27 UTC (permalink / raw) To: Fangrui Song Cc: Alan Modra, Nick Desaulniers, clang-built-linux, Joel Stanley, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc/boot: Delete unneeded .globl _zimage_start 2020-03-25 16:42 [PATCH v2] powerpc/boot: Delete unneeded .globl _zimage_start Fangrui Song 2020-03-26 22:16 ` Segher Boessenkool @ 2020-04-01 12:53 ` Michael Ellerman 1 sibling, 0 replies; 7+ messages in thread From: Michael Ellerman @ 2020-04-01 12:53 UTC (permalink / raw) To: Fangrui Song, linuxppc-dev Cc: clang-built-linux, Nick Desaulniers, Joel Stanley, Fangrui Song, Alan Modra 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-04-01 13:44 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-25 16:42 [PATCH v2] powerpc/boot: Delete unneeded .globl _zimage_start Fangrui Song 2020-03-26 22:16 ` Segher Boessenkool 2020-03-26 22:26 ` Fangrui Song 2020-03-27 15:24 ` Segher Boessenkool 2020-03-27 16:50 ` Fangrui Song 2020-03-27 18:27 ` Segher Boessenkool 2020-04-01 12:53 ` Michael Ellerman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).