| Message ID | 20200325164257.170229-1-maskray@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 968339fad422a58312f67718691b717dac45c399 |
| Headers | show |
| Series | [v2] powerpc/boot: Delete unneeded .globl _zimage_start | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (a87b93bdf800a4d7a42d95683624a4516e516b4f) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | warning | total: 1 errors, 0 warnings, 0 checks, 9 lines checked |
| snowpatch_ozlabs/needsstable | warning | Please consider tagging this patch for stable! |
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
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.
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
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
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
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
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
.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(-)