[PATCH 2/6] arm: clean up v7 and v8 linker scripts for bss_start/end
Ilias Apalodimas
ilias.apalodimas at linaro.org
Wed Mar 6 10:08:54 CET 2024
Hi Sam,
First of all, thanks for taking the time to review this.
On Wed, 6 Mar 2024 at 09:32, Sam Edwards <cfsworks at 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 at linaro.org>
> > Tested-by: Caleb Connolly <caleb.connolly at 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
More information about the U-Boot
mailing list