| Message ID | 20240304090113.1410575-3-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Clean up arm linker scripts | expand |
On 3/4/24 02:01, Ilias Apalodimas wrote: > commit 3ebd1cbc49f0 ("arm: make __bss_start and __bss_end__ compiler-generated") > and > commit f84a7b8f54db ("ARM: Fix __bss_start and __bss_end in linker scripts") > were moving the bss_start/end on c generated variables that were > injected in their own sections. The reason was that we needed relative > relocations for position independent code and linker bugs back then > prevented us from doing so. > > However, the linker documentation pages states that symbols that are > defined within a section definition will create a relocatable > type with the value being a fixed offset from the base of a section. > This have been fixed since 2016 [1]. So let's start cleaning this up > starting with the bss_start and bss_end variables. Convert them into > symbols within the .bss section definition. > > [0] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13 > [1] commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for shared object") > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > Tested-by: Caleb Connolly <caleb.connolly@linaro.org> # Qualcomm sdm845 > --- > arch/arm/cpu/armv8/u-boot-spl.lds | 14 +++----------- > arch/arm/cpu/armv8/u-boot.lds | 15 +++------------ > arch/arm/cpu/u-boot.lds | 22 ++++------------------ > arch/arm/lib/sections.c | 2 -- > arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 14 +++----------- > arch/arm/mach-zynq/u-boot.lds | 21 +++------------------ > 6 files changed, 16 insertions(+), 72 deletions(-) > > diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds > index 7cb9d731246d..16fddb46e9cb 100644 > --- a/arch/arm/cpu/armv8/u-boot-spl.lds > +++ b/arch/arm/cpu/armv8/u-boot-spl.lds > @@ -63,18 +63,10 @@ SECTIONS > > _image_binary_end = .; > > - .bss_start (NOLOAD) : { > - . = ALIGN(8); > - KEEP(*(.__bss_start)); > - } >.sdram > - > - .bss (NOLOAD) : { > + .bss : { > + __bss_start = .; > *(.bss*) > - . = ALIGN(8); > - } >.sdram > - > - .bss_end (NOLOAD) : { > - KEEP(*(.__bss_end)); > + __bss_end = .; > } >.sdram > > /DISCARD/ : { *(.rela*) } > diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds > index fb6a30c922f7..c4ee10ebc3ff 100644 > --- a/arch/arm/cpu/armv8/u-boot.lds > +++ b/arch/arm/cpu/armv8/u-boot.lds > @@ -149,19 +149,10 @@ SECTIONS > > _end = .; > > - . = ALIGN(8); > - > - .bss_start : { > - KEEP(*(.__bss_start)); > - } > - > - .bss : { > + .bss ALIGN(8): { > + __bss_start = .; > *(.bss*) > - . = ALIGN(8); > - } > - > - .bss_end : { > - KEEP(*(.__bss_end)); > + __bss_end = .; > } > > /DISCARD/ : { *(.dynsym) } > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds > index 7724c9332c3b..90d329b1ebe0 100644 > --- a/arch/arm/cpu/u-boot.lds > +++ b/arch/arm/cpu/u-boot.lds > @@ -206,27 +206,13 @@ SECTIONS > *(.mmutable) > } > > -/* > - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c > - * __bss_base and __bss_limit are for linker only (overlay ordering) > - */ > - > - .bss_start __rel_dyn_start (OVERLAY) : { > - KEEP(*(.__bss_start)); > - __bss_base = .; > - } > - > - .bss __bss_base (OVERLAY) : { > + .bss ALIGN(4): { Hi Ilias, I have been reviewing this patchset by diffing output binaries and linker maps between successive patches. Most of the patches in this series result in no change to the output binary whatsoever (which also means, no regressions!) but this one does have a change: .bss is being moved after .mmutable. You should be able to see this yourself by comparing `u-boot.map` after a successful build, before and after applying this patch. Since the current u-boot.lds puts .bss right after __image_copy_end (which makes sense) and uses OVERLAY to overlap .rel.dyn (which... I guess makes sense, if U-Boot zero-initializes .bss after applying relocations), perhaps this patch should be moving the .bss section up there in the .lds, putting (OVERLAY) back, and adding a comment to the effect of "These sections occupy the same memory, but their lifetimes do not overlap: U-Boot initializes .bss only after applying relocations and therefore after it doesn't need .rel.dyn any more." We might also decide that the overlay memory-saving trick isn't actually all that important anymore and that .bss should have a new home. Someone far more seasoned than I should be the one to make that decision though. The rest of the patchset looks great! I'll add my tags to those in a bit. Cheers, Sam > + __bss_start = .; > *(.bss*) > - . = ALIGN(4); > - __bss_limit = .; > - } > - > - .bss_end __bss_limit (OVERLAY) : { > - KEEP(*(.__bss_end)); > + __bss_end = .; > } > > - .dynsym _image_binary_end : { *(.dynsym) } > + .dynsym : { *(.dynsym) } > .dynbss : { *(.dynbss) } > .dynstr : { *(.dynstr*) } > .dynamic : { *(.dynamic*) } > diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c > index 857879711c6a..8e8bd5797e16 100644 > --- a/arch/arm/lib/sections.c > +++ b/arch/arm/lib/sections.c > @@ -19,8 +19,6 @@ > * aliasing warnings. > */ > > -char __bss_start[0] __section(".__bss_start"); > -char __bss_end[0] __section(".__bss_end"); > char __image_copy_start[0] __section(".__image_copy_start"); > char __image_copy_end[0] __section(".__image_copy_end"); > char __rel_dyn_start[0] __section(".__rel_dyn_start"); > diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds > index 74618eba591b..b7887194026e 100644 > --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds > +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds > @@ -56,18 +56,10 @@ SECTIONS > > _image_binary_end = .; > > - .bss_start (NOLOAD) : { > - . = ALIGN(8); > - KEEP(*(.__bss_start)); > - } > - > - .bss (NOLOAD) : { > + .bss ALIGN(8) : { > + __bss_start = .; > *(.bss*) > - . = ALIGN(8); > - } > - > - .bss_end (NOLOAD) : { > - KEEP(*(.__bss_end)); > + __bss_end = .; > } > > /DISCARD/ : { *(.dynsym) } > diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds > index 3b7c9d515f8b..8d3259821719 100644 > --- a/arch/arm/mach-zynq/u-boot.lds > +++ b/arch/arm/mach-zynq/u-boot.lds > @@ -102,26 +102,11 @@ SECTIONS > > _image_binary_end = .; > > -/* > - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c > - * __bss_base and __bss_limit are for linker only (overlay ordering) > - */ > - > - .bss_start __rel_dyn_start (OVERLAY) : { > - KEEP(*(.__bss_start)); > - __bss_base = .; > - } > - > - .bss __bss_base (OVERLAY) : { > + .bss ALIGN(4): { > + __bss_start = .; > *(.bss*) > - . = ALIGN(8); > - __bss_limit = .; > - } > - > - .bss_end __bss_limit (OVERLAY) : { > - KEEP(*(.__bss_end)); > + __bss_end = .; > } > - > /* > * Zynq needs to discard these sections because the user > * is expected to pass this image on to tools for boot.bin
Hi Sam, First of all, thanks for taking the time to review this. On Wed, 6 Mar 2024 at 09:32, Sam Edwards <cfsworks@gmail.com> wrote: > > On 3/4/24 02:01, Ilias Apalodimas wrote: > > commit 3ebd1cbc49f0 ("arm: make __bss_start and __bss_end__ compiler-generated") > > and > > commit f84a7b8f54db ("ARM: Fix __bss_start and __bss_end in linker scripts") > > were moving the bss_start/end on c generated variables that were > > injected in their own sections. The reason was that we needed relative > > relocations for position independent code and linker bugs back then > > prevented us from doing so. > > > > However, the linker documentation pages states that symbols that are > > defined within a section definition will create a relocatable > > type with the value being a fixed offset from the base of a section. > > This have been fixed since 2016 [1]. So let's start cleaning this up > > starting with the bss_start and bss_end variables. Convert them into > > symbols within the .bss section definition. > > > > [0] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13 > > [1] commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for shared object") > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > Tested-by: Caleb Connolly <caleb.connolly@linaro.org> # Qualcomm sdm845 > > --- > > arch/arm/cpu/armv8/u-boot-spl.lds | 14 +++----------- > > arch/arm/cpu/armv8/u-boot.lds | 15 +++------------ > > arch/arm/cpu/u-boot.lds | 22 ++++------------------ > > arch/arm/lib/sections.c | 2 -- > > arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 14 +++----------- > > arch/arm/mach-zynq/u-boot.lds | 21 +++------------------ > > 6 files changed, 16 insertions(+), 72 deletions(-) > > > > diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds > > index 7cb9d731246d..16fddb46e9cb 100644 > > --- a/arch/arm/cpu/armv8/u-boot-spl.lds > > +++ b/arch/arm/cpu/armv8/u-boot-spl.lds > > @@ -63,18 +63,10 @@ SECTIONS > > > > _image_binary_end = .; > > > > - .bss_start (NOLOAD) : { > > - . = ALIGN(8); > > - KEEP(*(.__bss_start)); > > - } >.sdram > > - > > - .bss (NOLOAD) : { > > + .bss : { > > + __bss_start = .; > > *(.bss*) > > - . = ALIGN(8); > > - } >.sdram > > - > > - .bss_end (NOLOAD) : { > > - KEEP(*(.__bss_end)); > > + __bss_end = .; > > } >.sdram > > > > /DISCARD/ : { *(.rela*) } > > diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds > > index fb6a30c922f7..c4ee10ebc3ff 100644 > > --- a/arch/arm/cpu/armv8/u-boot.lds > > +++ b/arch/arm/cpu/armv8/u-boot.lds > > @@ -149,19 +149,10 @@ SECTIONS > > > > _end = .; > > > > - . = ALIGN(8); > > - > > - .bss_start : { > > - KEEP(*(.__bss_start)); > > - } > > - > > - .bss : { > > + .bss ALIGN(8): { > > + __bss_start = .; > > *(.bss*) > > - . = ALIGN(8); > > - } > > - > > - .bss_end : { > > - KEEP(*(.__bss_end)); > > + __bss_end = .; > > } > > > > /DISCARD/ : { *(.dynsym) } > > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds > > index 7724c9332c3b..90d329b1ebe0 100644 > > --- a/arch/arm/cpu/u-boot.lds > > +++ b/arch/arm/cpu/u-boot.lds > > @@ -206,27 +206,13 @@ SECTIONS > > *(.mmutable) > > } > > > > -/* > > - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c > > - * __bss_base and __bss_limit are for linker only (overlay ordering) > > - */ > > - > > - .bss_start __rel_dyn_start (OVERLAY) : { > > - KEEP(*(.__bss_start)); > > - __bss_base = .; > > - } > > - > > - .bss __bss_base (OVERLAY) : { > > + .bss ALIGN(4): { > > Hi Ilias, > > I have been reviewing this patchset by diffing output binaries and > linker maps between successive patches. Most of the patches in this > series result in no change to the output binary whatsoever (which also > means, no regressions!) but this one does have a change: .bss is being > moved after .mmutable. You should be able to see this yourself by > comparing `u-boot.map` after a successful build, before and after > applying this patch. Yes it does > > Since the current u-boot.lds puts .bss right after __image_copy_end > (which makes sense) and uses OVERLAY to overlap .rel.dyn (which... I > guess makes sense, if U-Boot zero-initializes .bss after applying > relocations), perhaps this patch should be moving the .bss section up > there in the .lds, putting (OVERLAY) back, and adding a comment to the > effect of "These sections occupy the same memory, but their lifetimes do > not overlap: U-Boot initializes .bss only after applying relocations and > therefore after it doesn't need .rel.dyn any more." We could do that, I honestly don't have a strong opinion on that. > > We might also decide that the overlay memory-saving trick isn't actually > all that important anymore and that .bss should have a new home. Someone > far more seasoned than I should be the one to make that decision though. I am not sure tbh. I imagine that the overlay saving trick between .rel_dyn and .bss would only matter in U-Boot SPL where memory is limited. The v8 version of SPL doesn't have the overlay but the v7 does and I left it untouched on purpose. On top of that the overlay on v7 for SPL is defined after the image_binary_end. Perhaps Tom remembers a bit more of the lds history than I do. Tom thoughts? Thanks! /Ilias > > The rest of the patchset looks great! I'll add my tags to those in a bit. > > Cheers, > Sam > > > + __bss_start = .; > > *(.bss*) > > - . = ALIGN(4); > > - __bss_limit = .; > > - } > > - > > - .bss_end __bss_limit (OVERLAY) : { > > - KEEP(*(.__bss_end)); > > + __bss_end = .; > > } > > > > - .dynsym _image_binary_end : { *(.dynsym) } > > + .dynsym : { *(.dynsym) } > > .dynbss : { *(.dynbss) } > > .dynstr : { *(.dynstr*) } > > .dynamic : { *(.dynamic*) } > > diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c > > index 857879711c6a..8e8bd5797e16 100644 > > --- a/arch/arm/lib/sections.c > > +++ b/arch/arm/lib/sections.c > > @@ -19,8 +19,6 @@ > > * aliasing warnings. > > */ > > > > -char __bss_start[0] __section(".__bss_start"); > > -char __bss_end[0] __section(".__bss_end"); > > char __image_copy_start[0] __section(".__image_copy_start"); > > char __image_copy_end[0] __section(".__image_copy_end"); > > char __rel_dyn_start[0] __section(".__rel_dyn_start"); > > diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds > > index 74618eba591b..b7887194026e 100644 > > --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds > > +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds > > @@ -56,18 +56,10 @@ SECTIONS > > > > _image_binary_end = .; > > > > - .bss_start (NOLOAD) : { > > - . = ALIGN(8); > > - KEEP(*(.__bss_start)); > > - } > > - > > - .bss (NOLOAD) : { > > + .bss ALIGN(8) : { > > + __bss_start = .; > > *(.bss*) > > - . = ALIGN(8); > > - } > > - > > - .bss_end (NOLOAD) : { > > - KEEP(*(.__bss_end)); > > + __bss_end = .; > > } > > > > /DISCARD/ : { *(.dynsym) } > > diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds > > index 3b7c9d515f8b..8d3259821719 100644 > > --- a/arch/arm/mach-zynq/u-boot.lds > > +++ b/arch/arm/mach-zynq/u-boot.lds > > @@ -102,26 +102,11 @@ SECTIONS > > > > _image_binary_end = .; > > > > -/* > > - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c > > - * __bss_base and __bss_limit are for linker only (overlay ordering) > > - */ > > - > > - .bss_start __rel_dyn_start (OVERLAY) : { > > - KEEP(*(.__bss_start)); > > - __bss_base = .; > > - } > > - > > - .bss __bss_base (OVERLAY) : { > > + .bss ALIGN(4): { > > + __bss_start = .; > > *(.bss*) > > - . = ALIGN(8); > > - __bss_limit = .; > > - } > > - > > - .bss_end __bss_limit (OVERLAY) : { > > - KEEP(*(.__bss_end)); > > + __bss_end = .; > > } > > - > > /* > > * Zynq needs to discard these sections because the user > > * is expected to pass this image on to tools for boot.bin
On Wed, 6 Mar 2024 at 11:08, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Sam, > > First of all, thanks for taking the time to review this. > > On Wed, 6 Mar 2024 at 09:32, Sam Edwards <cfsworks@gmail.com> wrote: > > > > On 3/4/24 02:01, Ilias Apalodimas wrote: > > > commit 3ebd1cbc49f0 ("arm: make __bss_start and __bss_end__ compiler-generated") > > > and > > > commit f84a7b8f54db ("ARM: Fix __bss_start and __bss_end in linker scripts") > > > were moving the bss_start/end on c generated variables that were > > > injected in their own sections. The reason was that we needed relative > > > relocations for position independent code and linker bugs back then > > > prevented us from doing so. > > > > > > However, the linker documentation pages states that symbols that are > > > defined within a section definition will create a relocatable > > > type with the value being a fixed offset from the base of a section. > > > This have been fixed since 2016 [1]. So let's start cleaning this up > > > starting with the bss_start and bss_end variables. Convert them into > > > symbols within the .bss section definition. > > > > > > [0] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13 > > > [1] commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for shared object") > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > Tested-by: Caleb Connolly <caleb.connolly@linaro.org> # Qualcomm sdm845 > > > --- > > > arch/arm/cpu/armv8/u-boot-spl.lds | 14 +++----------- > > > arch/arm/cpu/armv8/u-boot.lds | 15 +++------------ > > > arch/arm/cpu/u-boot.lds | 22 ++++------------------ > > > arch/arm/lib/sections.c | 2 -- > > > arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 14 +++----------- > > > arch/arm/mach-zynq/u-boot.lds | 21 +++------------------ > > > 6 files changed, 16 insertions(+), 72 deletions(-) > > > > > > diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds > > > index 7cb9d731246d..16fddb46e9cb 100644 > > > --- a/arch/arm/cpu/armv8/u-boot-spl.lds > > > +++ b/arch/arm/cpu/armv8/u-boot-spl.lds > > > @@ -63,18 +63,10 @@ SECTIONS > > > > > > _image_binary_end = .; > > > > > > - .bss_start (NOLOAD) : { > > > - . = ALIGN(8); > > > - KEEP(*(.__bss_start)); > > > - } >.sdram > > > - > > > - .bss (NOLOAD) : { > > > + .bss : { > > > + __bss_start = .; > > > *(.bss*) > > > - . = ALIGN(8); > > > - } >.sdram > > > - > > > - .bss_end (NOLOAD) : { > > > - KEEP(*(.__bss_end)); > > > + __bss_end = .; > > > } >.sdram > > > > > > /DISCARD/ : { *(.rela*) } > > > diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds > > > index fb6a30c922f7..c4ee10ebc3ff 100644 > > > --- a/arch/arm/cpu/armv8/u-boot.lds > > > +++ b/arch/arm/cpu/armv8/u-boot.lds > > > @@ -149,19 +149,10 @@ SECTIONS > > > > > > _end = .; > > > > > > - . = ALIGN(8); > > > - > > > - .bss_start : { > > > - KEEP(*(.__bss_start)); > > > - } > > > - > > > - .bss : { > > > + .bss ALIGN(8): { > > > + __bss_start = .; > > > *(.bss*) > > > - . = ALIGN(8); > > > - } > > > - > > > - .bss_end : { > > > - KEEP(*(.__bss_end)); > > > + __bss_end = .; > > > } > > > > > > /DISCARD/ : { *(.dynsym) } > > > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds > > > index 7724c9332c3b..90d329b1ebe0 100644 > > > --- a/arch/arm/cpu/u-boot.lds > > > +++ b/arch/arm/cpu/u-boot.lds > > > @@ -206,27 +206,13 @@ SECTIONS > > > *(.mmutable) > > > } > > > > > > -/* > > > - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c > > > - * __bss_base and __bss_limit are for linker only (overlay ordering) > > > - */ > > > - > > > - .bss_start __rel_dyn_start (OVERLAY) : { > > > - KEEP(*(.__bss_start)); > > > - __bss_base = .; > > > - } > > > - > > > - .bss __bss_base (OVERLAY) : { > > > + .bss ALIGN(4): { > > > > Hi Ilias, > > > > I have been reviewing this patchset by diffing output binaries and > > linker maps between successive patches. Most of the patches in this > > series result in no change to the output binary whatsoever (which also > > means, no regressions!) but this one does have a change: .bss is being > > moved after .mmutable. You should be able to see this yourself by > > comparing `u-boot.map` after a successful build, before and after > > applying this patch. > > Yes it does > > > > > Since the current u-boot.lds puts .bss right after __image_copy_end > > (which makes sense) and uses OVERLAY to overlap .rel.dyn (which... I > > guess makes sense, if U-Boot zero-initializes .bss after applying > > relocations), perhaps this patch should be moving the .bss section up > > there in the .lds, putting (OVERLAY) back, and adding a comment to the > > effect of "These sections occupy the same memory, but their lifetimes do > > not overlap: U-Boot initializes .bss only after applying relocations and > > therefore after it doesn't need .rel.dyn any more." > > We could do that, I honestly don't have a strong opinion on that. replying to myself here, but there was a relevant discussion here as well https://lore.kernel.org/u-boot/ZcJF2uGIMj-jxT3M@hera/ > > > > > We might also decide that the overlay memory-saving trick isn't actually > > all that important anymore and that .bss should have a new home. Someone > > far more seasoned than I should be the one to make that decision though. > > I am not sure tbh. I imagine that the overlay saving trick between > .rel_dyn and .bss would only matter in U-Boot SPL where memory is > limited. The v8 version of SPL doesn't have the overlay but the v7 > does and I left it untouched on purpose. On top of that the overlay on > v7 for SPL is defined after the image_binary_end. > Perhaps Tom remembers a bit more of the lds history than I do. Tom thoughts? > > Thanks! > /Ilias > > > > > The rest of the patchset looks great! I'll add my tags to those in a bit. > > > > Cheers, > > Sam > > > > > + __bss_start = .; > > > *(.bss*) > > > - . = ALIGN(4); > > > - __bss_limit = .; > > > - } > > > - > > > - .bss_end __bss_limit (OVERLAY) : { > > > - KEEP(*(.__bss_end)); > > > + __bss_end = .; > > > } > > > > > > - .dynsym _image_binary_end : { *(.dynsym) } > > > + .dynsym : { *(.dynsym) } > > > .dynbss : { *(.dynbss) } > > > .dynstr : { *(.dynstr*) } > > > .dynamic : { *(.dynamic*) } > > > diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c > > > index 857879711c6a..8e8bd5797e16 100644 > > > --- a/arch/arm/lib/sections.c > > > +++ b/arch/arm/lib/sections.c > > > @@ -19,8 +19,6 @@ > > > * aliasing warnings. > > > */ > > > > > > -char __bss_start[0] __section(".__bss_start"); > > > -char __bss_end[0] __section(".__bss_end"); > > > char __image_copy_start[0] __section(".__image_copy_start"); > > > char __image_copy_end[0] __section(".__image_copy_end"); > > > char __rel_dyn_start[0] __section(".__rel_dyn_start"); > > > diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds > > > index 74618eba591b..b7887194026e 100644 > > > --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds > > > +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds > > > @@ -56,18 +56,10 @@ SECTIONS > > > > > > _image_binary_end = .; > > > > > > - .bss_start (NOLOAD) : { > > > - . = ALIGN(8); > > > - KEEP(*(.__bss_start)); > > > - } > > > - > > > - .bss (NOLOAD) : { > > > + .bss ALIGN(8) : { > > > + __bss_start = .; > > > *(.bss*) > > > - . = ALIGN(8); > > > - } > > > - > > > - .bss_end (NOLOAD) : { > > > - KEEP(*(.__bss_end)); > > > + __bss_end = .; > > > } > > > > > > /DISCARD/ : { *(.dynsym) } > > > diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds > > > index 3b7c9d515f8b..8d3259821719 100644 > > > --- a/arch/arm/mach-zynq/u-boot.lds > > > +++ b/arch/arm/mach-zynq/u-boot.lds > > > @@ -102,26 +102,11 @@ SECTIONS > > > > > > _image_binary_end = .; > > > > > > -/* > > > - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c > > > - * __bss_base and __bss_limit are for linker only (overlay ordering) > > > - */ > > > - > > > - .bss_start __rel_dyn_start (OVERLAY) : { > > > - KEEP(*(.__bss_start)); > > > - __bss_base = .; > > > - } > > > - > > > - .bss __bss_base (OVERLAY) : { > > > + .bss ALIGN(4): { > > > + __bss_start = .; > > > *(.bss*) > > > - . = ALIGN(8); > > > - __bss_limit = .; > > > - } > > > - > > > - .bss_end __bss_limit (OVERLAY) : { > > > - KEEP(*(.__bss_end)); > > > + __bss_end = .; > > > } > > > - > > > /* > > > * Zynq needs to discard these sections because the user > > > * is expected to pass this image on to tools for boot.bin
diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds index 7cb9d731246d..16fddb46e9cb 100644 --- a/arch/arm/cpu/armv8/u-boot-spl.lds +++ b/arch/arm/cpu/armv8/u-boot-spl.lds @@ -63,18 +63,10 @@ SECTIONS _image_binary_end = .; - .bss_start (NOLOAD) : { - . = ALIGN(8); - KEEP(*(.__bss_start)); - } >.sdram - - .bss (NOLOAD) : { + .bss : { + __bss_start = .; *(.bss*) - . = ALIGN(8); - } >.sdram - - .bss_end (NOLOAD) : { - KEEP(*(.__bss_end)); + __bss_end = .; } >.sdram /DISCARD/ : { *(.rela*) } diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds index fb6a30c922f7..c4ee10ebc3ff 100644 --- a/arch/arm/cpu/armv8/u-boot.lds +++ b/arch/arm/cpu/armv8/u-boot.lds @@ -149,19 +149,10 @@ SECTIONS _end = .; - . = ALIGN(8); - - .bss_start : { - KEEP(*(.__bss_start)); - } - - .bss : { + .bss ALIGN(8): { + __bss_start = .; *(.bss*) - . = ALIGN(8); - } - - .bss_end : { - KEEP(*(.__bss_end)); + __bss_end = .; } /DISCARD/ : { *(.dynsym) } diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds index 7724c9332c3b..90d329b1ebe0 100644 --- a/arch/arm/cpu/u-boot.lds +++ b/arch/arm/cpu/u-boot.lds @@ -206,27 +206,13 @@ SECTIONS *(.mmutable) } -/* - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c - * __bss_base and __bss_limit are for linker only (overlay ordering) - */ - - .bss_start __rel_dyn_start (OVERLAY) : { - KEEP(*(.__bss_start)); - __bss_base = .; - } - - .bss __bss_base (OVERLAY) : { + .bss ALIGN(4): { + __bss_start = .; *(.bss*) - . = ALIGN(4); - __bss_limit = .; - } - - .bss_end __bss_limit (OVERLAY) : { - KEEP(*(.__bss_end)); + __bss_end = .; } - .dynsym _image_binary_end : { *(.dynsym) } + .dynsym : { *(.dynsym) } .dynbss : { *(.dynbss) } .dynstr : { *(.dynstr*) } .dynamic : { *(.dynamic*) } diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c index 857879711c6a..8e8bd5797e16 100644 --- a/arch/arm/lib/sections.c +++ b/arch/arm/lib/sections.c @@ -19,8 +19,6 @@ * aliasing warnings. */ -char __bss_start[0] __section(".__bss_start"); -char __bss_end[0] __section(".__bss_end"); char __image_copy_start[0] __section(".__image_copy_start"); char __image_copy_end[0] __section(".__image_copy_end"); char __rel_dyn_start[0] __section(".__rel_dyn_start"); diff --git a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds index 74618eba591b..b7887194026e 100644 --- a/arch/arm/mach-rockchip/u-boot-tpl-v8.lds +++ b/arch/arm/mach-rockchip/u-boot-tpl-v8.lds @@ -56,18 +56,10 @@ SECTIONS _image_binary_end = .; - .bss_start (NOLOAD) : { - . = ALIGN(8); - KEEP(*(.__bss_start)); - } - - .bss (NOLOAD) : { + .bss ALIGN(8) : { + __bss_start = .; *(.bss*) - . = ALIGN(8); - } - - .bss_end (NOLOAD) : { - KEEP(*(.__bss_end)); + __bss_end = .; } /DISCARD/ : { *(.dynsym) } diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds index 3b7c9d515f8b..8d3259821719 100644 --- a/arch/arm/mach-zynq/u-boot.lds +++ b/arch/arm/mach-zynq/u-boot.lds @@ -102,26 +102,11 @@ SECTIONS _image_binary_end = .; -/* - * Compiler-generated __bss_start and __bss_end, see arch/arm/lib/bss.c - * __bss_base and __bss_limit are for linker only (overlay ordering) - */ - - .bss_start __rel_dyn_start (OVERLAY) : { - KEEP(*(.__bss_start)); - __bss_base = .; - } - - .bss __bss_base (OVERLAY) : { + .bss ALIGN(4): { + __bss_start = .; *(.bss*) - . = ALIGN(8); - __bss_limit = .; - } - - .bss_end __bss_limit (OVERLAY) : { - KEEP(*(.__bss_end)); + __bss_end = .; } - /* * Zynq needs to discard these sections because the user * is expected to pass this image on to tools for boot.bin
commit 3ebd1cbc49f0 ("arm: make __bss_start and __bss_end__ compiler-generated") and commit f84a7b8f54db ("ARM: Fix __bss_start and __bss_end in linker scripts") were moving the bss_start/end on c generated variables that were injected in their own sections. The reason was that we needed relative relocations for position independent code and linker bugs back then prevented us from doing so. However, the linker documentation pages states that symbols that are defined within a section definition will create a relocatable type with the value being a fixed offset from the base of a section. This have been fixed since 2016 [1]. So let's start cleaning this up starting with the bss_start and bss_end variables. Convert them into symbols within the .bss section definition. [0] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html#SEC13 [1] commit 6b3b0ab89663 ("Make linker assigned symbol dynamic only for shared object") Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> Tested-by: Caleb Connolly <caleb.connolly@linaro.org> # Qualcomm sdm845 --- arch/arm/cpu/armv8/u-boot-spl.lds | 14 +++----------- arch/arm/cpu/armv8/u-boot.lds | 15 +++------------ arch/arm/cpu/u-boot.lds | 22 ++++------------------ arch/arm/lib/sections.c | 2 -- arch/arm/mach-rockchip/u-boot-tpl-v8.lds | 14 +++----------- arch/arm/mach-zynq/u-boot.lds | 21 +++------------------ 6 files changed, 16 insertions(+), 72 deletions(-)