From: "Benoît Thébaudeau" <benoit.thebaudeau@advansee.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH] ARM: Fix __bss_start and __bss_end in linker scripts
Date: Fri, 5 Apr 2013 15:56:46 +0200 (CEST) [thread overview]
Message-ID: <300263803.1207216.1365170206861.JavaMail.root@advansee.com> (raw)
In-Reply-To: <20130405080043.531e1b3d@lilith>
Hi Albert,
On Friday, April 5, 2013 8:00:43 AM, Albert ARIBAUD wrote:
> Hi Beno?t,
>
> CC:ing Stephen Warren who wrote commit 2b7818d4 which git blame tells
> me added the ASSERT() to arch/arm/cpu/u-boot.lds, and Tom Rini to help
> decide what to do.
>
> On Fri, 5 Apr 2013 05:44:55 +0200 (CEST), Beno?t Th?baudeau
> <benoit.thebaudeau@advansee.com> wrote:
>
> > Hi Albert,
> >
> > On Friday, April 5, 2013 1:54:38 AM, Albert ARIBAUD wrote:
> > > Hi Beno?t,
> > >
> > > On Fri, 5 Apr 2013 01:13:51 +0200 (CEST), Beno?t Th?baudeau
> > > <benoit.thebaudeau@advansee.com> wrote:
> > >
> > > > On Friday, April 5, 2013 1:05:53 AM, Beno?t Th?baudeau wrote:
> > > > > Hi Albert,
> > > > >
> > > > > On Friday, April 5, 2013 12:13:53 AM, Albert ARIBAUD wrote:
> > > > > > Subject: [PATCH] ARM: Fix __bss_start and __bss_end in linker
> > > > > > scripts
> > > > > >
> > > > > > Commit 3ebd1cbc introduced compiler-generated __bss_start
> > > > > > and __bss_end__ and commit c23561e7 rewrote all __bss_end__
> > > > > > as __bss_end. Their merge caused silent and harmless but
> > > > > > potentially bug-inducing clashes between compiler- and linker-
> > > > > > enerated __bss_end symbols.
> > > > > >
> > > > > > Make __bss_end and __bss_start compiler-only, and create
> > > > > > __bss_base and __bss_limit for linker-only use.
> > > > > >
> > > > > > Reported-by: Beno?t Th?baudeau <benoit.thebaudeau@advansee.com>
> > > > > > Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> > > > > > ---
> > > > > > arch/arm/cpu/ixp/u-boot.lds | 14 ++++++++++----
> > > > > > arch/arm/cpu/u-boot.lds | 14 ++++++++++----
> > > > > > board/actux1/u-boot.lds | 14 ++++++++++----
> > > > > > board/actux2/u-boot.lds | 14 ++++++++++----
> > > > > > board/actux3/u-boot.lds | 14 ++++++++++----
> > > > > > board/dvlhost/u-boot.lds | 14 ++++++++++----
> > > > > > board/freescale/mx31ads/u-boot.lds | 14 ++++++++++----
> > > > > > 7 files changed, 70 insertions(+), 28 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/arm/cpu/ixp/u-boot.lds
> > > > > > b/arch/arm/cpu/ixp/u-boot.lds
> > > > > > index 8345b55..c999829 100644
> > > > > > --- a/arch/arm/cpu/ixp/u-boot.lds
> > > > > > +++ b/arch/arm/cpu/ixp/u-boot.lds
> > > > > > @@ -67,17 +67,23 @@ SECTIONS
> > > > > >
> > > > > > _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));
> > > > > > + HIDDEN(__bss_base = .);
> > > > > > }
> > > > > >
> > > > > > - .bss __bss_start (OVERLAY) : {
> > > > > > + .bss __bss_base (OVERLAY) : {
> > > > > > *(.bss*)
> > > > > > . = ALIGN(4);
> > > > > > - __bss_end = .;
> > > > > > + HIDDEN(__bss_limit = .);
> > > > > > }
> > > > > > - .bss_end __bss_end (OVERLAY) : {
> > > > > > - KEEP(*(__bss_end));
> > > > > > + .bss_end __bss_limit (OVERLAY) : {
> > > > > > + KEEP(*(.__bss_end));
> > > > > > }
> > > > > >
> > > > > > /DISCARD/ : { *(.dynstr*) }
> > > > > > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> > > > > > index 3a1083d..0543b06 100644
> > > > > > --- a/arch/arm/cpu/u-boot.lds
> > > > > > +++ b/arch/arm/cpu/u-boot.lds
> > > > > > @@ -81,18 +81,24 @@ 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));
> > > > > > + HIDDEN(__bss_base = .);
> > > > > > }
> > > > > >
> > > > > > - .bss __bss_start (OVERLAY) : {
> > > > > > + .bss __bss_base (OVERLAY) : {
> > > > > > *(.bss*)
> > > > > > . = ALIGN(4);
> > > > > > - __bss_end = .;
> > > > > > + HIDDEN(__bss_limit = .);
> > > > > > }
> > > > > >
> > > > > > - .bss_end __bss_end (OVERLAY) : {
> > > > > > - KEEP(*(__bss_end));
> > > > > > + .bss_end __bss_limit (OVERLAY) : {
> > > > > > + KEEP(*(.__bss_end));
> > > > > > }
> > > > > >
> > > > > > /DISCARD/ : { *(.dynstr*) }
> > > > > > diff --git a/board/actux1/u-boot.lds b/board/actux1/u-boot.lds
> > > > > > index c76728a..7e5c4d8 100644
> > > > > > --- a/board/actux1/u-boot.lds
> > > > > > +++ b/board/actux1/u-boot.lds
> > > > > > @@ -74,17 +74,23 @@ SECTIONS
> > > > > >
> > > > > > _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));
> > > > > > + HIDDEN(__bss_base = .);
> > > > > > }
> > > > > >
> > > > > > - .bss __bss_start (OVERLAY) : {
> > > > > > + .bss __bss_base (OVERLAY) : {
> > > > > > *(.bss*)
> > > > > > . = ALIGN(4);
> > > > > > - __bss_end = .;
> > > > > > + HIDDEN(__bss_limit = .);
> > > > > > }
> > > > > > - .bss_end __bss_end (OVERLAY) : {
> > > > > > - KEEP(*(__bss_end));
> > > > > > + .bss_end __bss_limit (OVERLAY) : {
> > > > > > + KEEP(*(.__bss_end));
> > > > > > }
> > > > > >
> > > > > > /DISCARD/ : { *(.dynstr*) }
> > > > > > diff --git a/board/actux2/u-boot.lds b/board/actux2/u-boot.lds
> > > > > > index 984f70e..ce1b7c9 100644
> > > > > > --- a/board/actux2/u-boot.lds
> > > > > > +++ b/board/actux2/u-boot.lds
> > > > > > @@ -74,17 +74,23 @@ SECTIONS
> > > > > >
> > > > > > _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));
> > > > > > + HIDDEN(__bss_base = .);
> > > > > > }
> > > > > >
> > > > > > - .bss __bss_start (OVERLAY) : {
> > > > > > + .bss __bss_base (OVERLAY) : {
> > > > > > *(.bss*)
> > > > > > . = ALIGN(4);
> > > > > > - __bss_end = .;
> > > > > > + HIDDEN(__bss_limit = .);
> > > > > > }
> > > > > > - .bss_end __bss_end (OVERLAY) : {
> > > > > > - KEEP(*(__bss_end));
> > > > > > + .bss_end __bss_limit (OVERLAY) : {
> > > > > > + KEEP(*(.__bss_end));
> > > > > > }
> > > > > >
> > > > > > /DISCARD/ : { *(.dynstr*) }
> > > > > > diff --git a/board/actux3/u-boot.lds b/board/actux3/u-boot.lds
> > > > > > index fc48cf0..3e091dd 100644
> > > > > > --- a/board/actux3/u-boot.lds
> > > > > > +++ b/board/actux3/u-boot.lds
> > > > > > @@ -74,17 +74,23 @@ SECTIONS
> > > > > >
> > > > > > _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));
> > > > > > + HIDDEN(__bss_base = .);
> > > > > > }
> > > > > >
> > > > > > - .bss __bss_start (OVERLAY) : {
> > > > > > + .bss __bss_base (OVERLAY) : {
> > > > > > *(.bss*)
> > > > > > . = ALIGN(4);
> > > > > > - __bss_end = .;
> > > > > > + HIDDEN(__bss_limit = .);
> > > > > > }
> > > > > > - .bss_end __bss_end (OVERLAY) : {
> > > > > > - KEEP(*(__bss_end));
> > > > > > + .bss_end __bss_limit (OVERLAY) : {
> > > > > > + KEEP(*(.__bss_end));
> > > > > > }
> > > > > >
> > > > > > /DISCARD/ : { *(.dynstr*) }
> > > > > > diff --git a/board/dvlhost/u-boot.lds b/board/dvlhost/u-boot.lds
> > > > > > index b13d3e1..fe3c21b 100644
> > > > > > --- a/board/dvlhost/u-boot.lds
> > > > > > +++ b/board/dvlhost/u-boot.lds
> > > > > > @@ -74,17 +74,23 @@ SECTIONS
> > > > > >
> > > > > > _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));
> > > > > > + HIDDEN(__bss_base = .);
> > > > > > }
> > > > > >
> > > > > > - .bss __bss_start (OVERLAY) : {
> > > > > > + .bss __bss_base (OVERLAY) : {
> > > > > > *(.bss*)
> > > > > > . = ALIGN(4);
> > > > > > - __bss_end = .;
> > > > > > + HIDDEN(__bss_limit = .);
> > > > > > }
> > > > > > - .bss_end __bss_end (OVERLAY) : {
> > > > > > - KEEP(*(__bss_end));
> > > > > > + .bss_end __bss_limit (OVERLAY) : {
> > > > > > + KEEP(*(.__bss_end));
> > > > > > }
> > > > > >
> > > > > > /DISCARD/ : { *(.dynstr*) }
> > > > > > diff --git a/board/freescale/mx31ads/u-boot.lds
> > > > > > b/board/freescale/mx31ads/u-boot.lds
> > > > > > index 264c4e8..e6885a5 100644
> > > > > > --- a/board/freescale/mx31ads/u-boot.lds
> > > > > > +++ b/board/freescale/mx31ads/u-boot.lds
> > > > > > @@ -80,17 +80,23 @@ SECTIONS
> > > > > >
> > > > > > _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));
> > > > > > + HIDDEN(__bss_base = .);
> > > > > > }
> > > > > >
> > > > > > - .bss __bss_start (OVERLAY) : {
> > > > > > + .bss __bss_base (OVERLAY) : {
> > > > > > *(.bss*)
> > > > > > . = ALIGN(4);
> > > > > > - __bss_end = .;
> > > > > > + HIDDEN(__bss_limit = .);
> > > > > > }
> > > > > > - .bss_end __bss_end (OVERLAY) : {
> > > > > > - KEEP(*(__bss_end));
> > > > > > + .bss_end __bss_limit (OVERLAY) : {
> > > > > > + KEEP(*(.__bss_end));
> > > > > > }
> > > > > >
> > > > > > /DISCARD/ : { *(.bss*) }
> > > > > > --
> > > > > > 1.7.10.4
> > > > > >
> > > > > >
> > > > >
> > > > > Looks good, but what about the __bss_end in
> > > > > ASSERT(__bss_end < (CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE), "SPL
> > > > > image
> > > > > too big");
> > > > > ?
> > > > >
> > > > > Shouldn't it be replaced with __bss_limit, or even better, with
> > > > > __image_copy_end
> > > > > (why should BSS be taken into account for SPL image size?)?
> > > >
> > > > I meant __bss_base, not __image_copy_end.
> > >
> > > Part of SPL can use BSS, once board_init_f() has handed things over to
> > > board_init_r(),
> >
> > I agree with that.
> >
> > > and this test is meant to ensure that .text+.data+.bss
> > > fits in the RAM available to SPL. This ASSERT() compares the upper
> > > limit of this RAM to the upper limit of the SPL needs, i.e. __bss_end.
> > >
> > > IOW, this ASSERT is about how much memory SPL will use when it runs,
> >
> > This is where there might be an issue with the definition / usage of
> > CONFIG_SPL_MAX_SIZE.
> >
> > I had understood that this is the purpose of this assert, but this usage of
> > CONFIG_SPL_MAX_SIZE, while frequent on arm, conflicts with its definition
> > in
> > README, as well as with some arm and other usages, e.g. in
> > include/configs/am335x_evm.h or include/configs/p1_p2_rdb_pc.h.
It's not even that frequent on ARM. I've looked at a few other ARM linker
scripts (arm1136, omap), and they don't count bss in CONFIG_SPL_MAX_SIZE.
> > The README's definition is also how I understood this config when
> > discussing it
> > with Scott about my 16/30, which led to the #error test that was added at
> > some
> > point to this patch.
>
> Sorry, I'd missed that. Adding Stephen for any comment regarding the
> ASSERT() and CONFIG_SPL_MAX_SIZE semantics.
>
> > According to README, CONFIG_SPL_MAX_SIZE is for text + data + rodata (i.e.
> > for
> > the size of the binary image file to be programmed on the target device),
> > while
> > CONFIG_SPL_BSS_MAX_SIZE is dedicated to the BSS. Hence, in order to comply
> > with
> > this definition, that assert should either use __bss_base/__bss_start, or
> > compare against CONFIG_SPL_MAX_SIZE + CONFIG_SPL_BSS_MAX_SIZE, depending on
> > whether it's supposed to test the binary image size or the memory
> > footprint.
> > There could even be asserts testing both. This would anyway be a new patch
> > separate from this one.
>
> It would indeed be a different patch.
>
> IIUC, this future patch would increase the limit for SPL run-time size,
> as the constant against which the ASS tests __bss_end for would
> necessarily be greater than it is now. Correct? If so, this future
> patch should not break any target, as it would loosen the constraint,
> not tighten it.
Yes, it would either be the same or relaxed a bit, depending on the chosen
option:
- Define CONFIG_SPL_BSS_MAX_SIZE and test against CONFIG_SPL_MAX_SIZE +
CONFIG_SPL_BSS_MAX_SIZE, the sum remaining the same as or being larger than
currently, depending on the new values for CONFIG_SPL_MAX_SIZE and
CONFIG_SPL_BSS_MAX_SIZE.
- Define a new config meaning text + data + rodata + bss (e.g.
CONFIG_SPL_MAX_RAM_SIZE or CONFIG_SPL_MAX_MEM_FOOTPRINT), and just replace
CONFIG_SPL_MAX_SIZE with it for the users of arch/arm/cpu/u-boot*.lds, taking
care that this was the only meaning those users were giving to
CONFIG_SPL_MAX_SIZE.
The first option would probably be preferable, using the same value for
CONFIG_SPL_MAX_SIZE, and a non-zero value for CONFIG_SPL_BSS_MAX_SIZE.
> Tom, would such a patch, if posted today, be acceptable for 2013.04?
>
> > If you decided not to change that, CONFIG_SPL_MAX_SIZE would have different
> > meanings depending on the board, and CONFIG_SPL_PAD_TO could be smaller
> > than it
> > in some cases, so the #error test in my 16/30 could be removed, but no such
> > case
> > exists so far, making this 16/30 change not strictly necessary for now.
>
> IF a patch is posted, it should be done above your series, and take
> care of fixing 16/30 as necessary.
If this assert is fixed according to the README definition, nothing needs to be
changed in 16/30.
> Now, pending Tom's approval, the question is: who does this future
> patch?
>
> Beno?t, You obviously have most of the necessary inputs, but I might
> be more available to prepare a patch now. Your pick -- and in any case,
> you earn a second Reported-By: badge. :)
I let you handle this, and approved for my RB badge. :)
Best regards,
Beno?t
next prev parent reply other threads:[~2013-04-05 13:56 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-04-04 22:13 [U-Boot] [PATCH] ARM: Fix __bss_start and __bss_end in linker scripts Albert ARIBAUD
2013-04-04 23:05 ` Benoît Thébaudeau
2013-04-04 23:13 ` Benoît Thébaudeau
2013-04-04 23:54 ` Albert ARIBAUD
2013-04-05 3:44 ` Benoît Thébaudeau
2013-04-05 6:00 ` Albert ARIBAUD
2013-04-05 13:53 ` Tom Rini
2013-04-05 15:59 ` Stephen Warren
2013-04-11 17:52 ` Tom Warren
2013-04-11 17:59 ` Albert ARIBAUD
2013-04-11 18:13 ` Tom Warren
2013-04-05 13:56 ` Benoît Thébaudeau [this message]
2013-04-05 16:00 ` Tom Rini
2013-04-05 17:32 ` Benoît Thébaudeau
2013-04-05 17:55 ` Tom Rini
2013-04-05 19:17 ` Albert ARIBAUD
2013-04-05 19:28 ` Albert ARIBAUD
2013-04-05 19:44 ` Tom Rini
2013-04-05 20:04 ` Albert ARIBAUD
2013-04-05 20:23 ` Tom Rini
2013-04-08 19:03 ` Albert ARIBAUD
2013-04-08 19:56 ` Tom Rini
2013-04-08 20:05 ` Albert ARIBAUD
2013-04-04 23:12 ` Albert ARIBAUD
2013-04-11 15:30 ` Albert ARIBAUD
2013-04-11 15:43 ` [U-Boot] [PATCH V2] " Albert ARIBAUD
2013-04-12 16:34 ` Albert ARIBAUD
2013-04-13 21:16 ` Albert ARIBAUD
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=300263803.1207216.1365170206861.JavaMail.root@advansee.com \
--to=benoit.thebaudeau@advansee.com \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.