[PATCH] env: another attempt at fixing SPL build failures

[PATCH] env: another attempt at fixing SPL build failures

[Rasmus Villemoes]

I'm also seeing the build failure that commit

7d4776545b env: solve compilation error in SPL

tried to fix, namely that the reference to env_flags_validate from
env_htab cannot be satisfied when flags.o is not built in. However,
that commit got reverted by

d90fc9c3de Revert "env: solve compilation error in SPL"

Necessary, but not sufficient conditions to see this are

CONFIG_SPL=y (obviously)
CONFIG_SPL_ENV_SUPPORT=n (so flags.o does not get compiled)
CONFIG_SPL_LIBCOMMON_SUPPORT=y (so env/built-in.o is part of the SPL link)

Now, these are satisfied for e.g. imx6q_logic_defconfig. But that
builds just fine, and spl/u-boot-spl.map lists .data.env_htab among
the discarded (garbage collected) sections. Yet, on our
mpc8309-derived board, we do see the build failure, so perhaps the
linker works a bit differently on ppc than on ARM, or there's yet some
other configuration option needed to observe the break.

This is another attempt at solving it, which also cleans up
env/Makefile a bit: Introduce a def_bool y symbol CONFIG_ENV_SUPPORT
which complements CONFIG_(SPL/TPL)_SUPPORT. Then use
CONFIG_$(SPL_TPL_)ENV_SUPPORT to decide whether to include the five
basic env/*.o files. For attr.o, flags.o and callback.o, this
shouldn't change anything. Also, common.o and env.o still get
unconditionally built for U-boot proper. But for TPL/SPL, those two
are only included if CONFIG_(SPL/TPL)_SUPPORT is set.

Having that symbol should also allow simplifying conditionals such as

#if !defined(CONFIG_SPL_BUILD) || CONFIG_IS_ENABLED(ENV_SUPPORT)

found in drivers/reset/reset-socfpga.c to just
CONFIG_IS_ENABLED(ENV_SUPPORT).

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 env/Kconfig  |  3 +++
 env/Makefile | 13 +++++--------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/env/Kconfig b/env/Kconfig
index ed12609f6a..4661082f0e 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -1,5 +1,8 @@
 menu "Environment"
 
+config ENV_SUPPORT
+	def_bool y
+
 config ENV_IS_NOWHERE
 	bool "Environment is not stored"
 	default y if !ENV_IS_IN_EEPROM && !ENV_IS_IN_EXT4 && \
diff --git a/env/Makefile b/env/Makefile
index 90144d6caf..e2a165b8f1 100644
--- a/env/Makefile
+++ b/env/Makefile
@@ -3,12 +3,13 @@
 # (C) Copyright 2004-2006
 # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
 
-obj-y += common.o env.o
+obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += common.o
+obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += env.o
+obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += attr.o
+obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += flags.o
+obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += callback.o
 
 ifndef CONFIG_SPL_BUILD
-obj-y += attr.o
-obj-y += callback.o
-obj-y += flags.o
 obj-$(CONFIG_ENV_IS_IN_EEPROM) += eeprom.o
 extra-$(CONFIG_ENV_IS_EMBEDDED) += embedded.o
 obj-$(CONFIG_ENV_IS_IN_EEPROM) += embedded.o
@@ -19,10 +20,6 @@ obj-$(CONFIG_ENV_IS_IN_ONENAND) += onenand.o
 obj-$(CONFIG_ENV_IS_IN_SATA) += sata.o
 obj-$(CONFIG_ENV_IS_IN_REMOTE) += remote.o
 obj-$(CONFIG_ENV_IS_IN_UBI) += ubi.o
-else
-obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += attr.o
-obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += flags.o
-obj-$(CONFIG_$(SPL_TPL_)ENV_SUPPORT) += callback.o
 endif
 
 obj-$(CONFIG_$(SPL_TPL_)ENV_IS_NOWHERE) += nowhere.o
-- 
2.23.0

[PATCH] env: another attempt at fixing SPL build failures

[Rasmus Villemoes]

On 15/12/2019 23.29, Rasmus Villemoes wrote:
> I'm also seeing the build failure that commit
> 
> 7d4776545b env: solve compilation error in SPL
> 
> tried to fix, namely that the reference to env_flags_validate from
> env_htab cannot be satisfied when flags.o is not built in. However,
> that commit got reverted by
> 
> d90fc9c3de Revert "env: solve compilation error in SPL"
> 
> Necessary, but not sufficient conditions to see this are
> 
> CONFIG_SPL=y (obviously)
> CONFIG_SPL_ENV_SUPPORT=n (so flags.o does not get compiled)
> CONFIG_SPL_LIBCOMMON_SUPPORT=y (so env/built-in.o is part of the SPL link)
> 
> Now, these are satisfied for e.g. imx6q_logic_defconfig. But that
> builds just fine, and spl/u-boot-spl.map lists .data.env_htab among
> the discarded (garbage collected) sections. Yet, on our
> mpc8309-derived board, we do see the build failure, so perhaps the
> linker works a bit differently on ppc than on ARM, or there's yet some
> other configuration option needed to observe the break.

Yeah, I think this is a difference in how the linker works on ppc vs
arm. Doing

git grep --files-with-matches SPL=y -- configs/ | xargs grep
--files-with-matches SPL_LIBCOMMON_SUPPORT=y | xargs grep
--files-without-match SPL_ENV_SUPPORT | xargs head -n1

shows that all the in-tree defconfigs with the above combination are ARM
boards (except for microblaze_defconfig, but I don't have such a
toolchain), and they most likely all build just fine. But taking some
random PPC config (say T2080QDS_NAND_defconfig) with the first and third
point, and then manually disabling SPL_ENV_SUPPORT, immediately shows
the break.

For reference, I have

$ ${CROSS_COMPILE}ld --version
GNU ld (GNU Binutils for Ubuntu) 2.30

> This is another attempt at solving it, which also cleans up
> env/Makefile a bit: Introduce a def_bool y symbol CONFIG_ENV_SUPPORT
> which complements CONFIG_(SPL/TPL)_SUPPORT. Then use
> CONFIG_$(SPL_TPL_)ENV_SUPPORT to decide whether to include the five
> basic env/*.o files. For attr.o, flags.o and callback.o, this
> shouldn't change anything. Also, common.o and env.o still get
> unconditionally built for U-boot proper. But for TPL/SPL, those two
> are only included if CONFIG_(SPL/TPL)_SUPPORT is set.

Any comments on this approach, apart from the spellos (I'm missing ENV_
in front of SUPPORT in two places)?

Rasmus

[PATCH] env: another attempt at fixing SPL build failures

[Tom Rini]

On Fri, Jan 10, 2020 at 02:28:54PM +0000, Rasmus Villemoes wrote:
> On 15/12/2019 23.29, Rasmus Villemoes wrote:
> > I'm also seeing the build failure that commit
> > 
> > 7d4776545b env: solve compilation error in SPL
> > 
> > tried to fix, namely that the reference to env_flags_validate from
> > env_htab cannot be satisfied when flags.o is not built in. However,
> > that commit got reverted by
> > 
> > d90fc9c3de Revert "env: solve compilation error in SPL"
> > 
> > Necessary, but not sufficient conditions to see this are
> > 
> > CONFIG_SPL=y (obviously)
> > CONFIG_SPL_ENV_SUPPORT=n (so flags.o does not get compiled)
> > CONFIG_SPL_LIBCOMMON_SUPPORT=y (so env/built-in.o is part of the SPL link)
> > 
> > Now, these are satisfied for e.g. imx6q_logic_defconfig. But that
> > builds just fine, and spl/u-boot-spl.map lists .data.env_htab among
> > the discarded (garbage collected) sections. Yet, on our
> > mpc8309-derived board, we do see the build failure, so perhaps the
> > linker works a bit differently on ppc than on ARM, or there's yet some
> > other configuration option needed to observe the break.
> 
> Yeah, I think this is a difference in how the linker works on ppc vs
> arm. Doing
> 
> git grep --files-with-matches SPL=y -- configs/ | xargs grep
> --files-with-matches SPL_LIBCOMMON_SUPPORT=y | xargs grep
> --files-without-match SPL_ENV_SUPPORT | xargs head -n1
> 
> shows that all the in-tree defconfigs with the above combination are ARM
> boards (except for microblaze_defconfig, but I don't have such a
> toolchain), and they most likely all build just fine. But taking some
> random PPC config (say T2080QDS_NAND_defconfig) with the first and third
> point, and then manually disabling SPL_ENV_SUPPORT, immediately shows
> the break.
> 
> For reference, I have
> 
> $ ${CROSS_COMPILE}ld --version
> GNU ld (GNU Binutils for Ubuntu) 2.30

Which SPL are you using on PowerPC?  There's the one based
CONFIG_SPL_FRAMEWORK and the one that's not.  I suspect it's a framework
vs not problem here rather than linker exactly.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200110/bb480667/attachment.sig>

[PATCH] env: another attempt at fixing SPL build failures

[Rasmus Villemoes]

On 10/01/2020 15.34, Tom Rini wrote:
> On Fri, Jan 10, 2020 at 02:28:54PM +0000, Rasmus Villemoes wrote:
>> On 15/12/2019 23.29, Rasmus Villemoes wrote:
>>> I'm also seeing the build failure that commit
>>>
>>> 7d4776545b env: solve compilation error in SPL
>>>
>>
>> Yeah, I think this is a difference in how the linker works on ppc vs
>> arm. Doing
>>
>>
>> For reference, I have
>>
>> $ ${CROSS_COMPILE}ld --version
>> GNU ld (GNU Binutils for Ubuntu) 2.30
> 
> Which SPL are you using on PowerPC?  There's the one based
> CONFIG_SPL_FRAMEWORK and the one that's not.  I suspect it's a framework
> vs not problem here rather than linker exactly.
> 

No, it's nothing to do with SPL per se. Try something completely silly
such as

diff --git a/common/console.c b/common/console.c
index 168ba60d0d..bbff516154 100644
--- a/common/console.c
+++ b/common/console.c
@@ -22,6 +22,21 @@

 DECLARE_GLOBAL_DATA_PTR;

+struct foo {
+       int (*f)(int, int);
+       long a[100];
+};
+struct bar {
+       int a; int b; int c;
+};
+extern int func9999(int a, int b);
+struct foo foo9999 = { .f = func9999, };
+struct bar bar9999 = { .a = 0xa, .b = 0xb, .c = 0xc };
+
+#if 0
+int func9999(int a, int b) { return a + b; }
+#endif
+

For an arm target, this builds just fine, and u-boot.map (and
spl/u-boot-spl.map if an SPL is built) shows that foo9999 and bar9999
gets discarded just fine.

On powerpc, this fails with "undefined reference to `func9999'".

Now, changing to #if 1 instead, arm still completely eliminates both
data items as well as func9999. On powerpc, the link succeeds, and
bar9999 has been discarded, but both foo9999 and func9999 are present in
the final image.

So it seems that the ppc linker somehow fails to eliminate unreferenced
.data sections with relocation entries (it fails the same way if one
changes the pointer to an "int *").

Rasmus

[PATCH] env: another attempt at fixing SPL build failures

[Rasmus Villemoes]

On 11/01/2020 23.44, Rasmus Villemoes wrote:
> On 10/01/2020 15.34, Tom Rini wrote:
>> On Fri, Jan 10, 2020 at 02:28:54PM +0000, Rasmus Villemoes wrote:
>>> On 15/12/2019 23.29, Rasmus Villemoes wrote:
>>>> I'm also seeing the build failure that commit
>>>>
>>>> 7d4776545b env: solve compilation error in SPL
>>>>
>>>
>>> Yeah, I think this is a difference in how the linker works on ppc vs
>>> arm. Doing
>>>
>>>
>>> For reference, I have
>>>
>>> $ ${CROSS_COMPILE}ld --version
>>> GNU ld (GNU Binutils for Ubuntu) 2.30
>>
>> Which SPL are you using on PowerPC?  There's the one based
>> CONFIG_SPL_FRAMEWORK and the one that's not.  I suspect it's a framework
>> vs not problem here rather than linker exactly.
>>
> 
> No, it's nothing to do with SPL per se. 

OK, I think I found it. On powerpc, the .fixup section contains a
reference to .data.rel.env_htab, and all of .fixup is kept (because of
KEEP(*(.fixup))), so env_htab cannot get garbage collected. Hence
anything that section refers to must also exist. So it's not a defect in
ppc-ld vs arm-ld, it's just a consequence of ppc having that .fixup section.

So, the only way to fix that is by either making sure env_flags_validate
exists in the link, or avoiding having env_htab being part of the link
in the first place. My patch does the latter.

Rasmus

[PATCH] env: another attempt at fixing SPL build failures

[Tom Rini]

On Sun, Jan 12, 2020 at 08:23:02PM +0000, Rasmus Villemoes wrote:
> On 11/01/2020 23.44, Rasmus Villemoes wrote:
> > On 10/01/2020 15.34, Tom Rini wrote:
> >> On Fri, Jan 10, 2020 at 02:28:54PM +0000, Rasmus Villemoes wrote:
> >>> On 15/12/2019 23.29, Rasmus Villemoes wrote:
> >>>> I'm also seeing the build failure that commit
> >>>>
> >>>> 7d4776545b env: solve compilation error in SPL
> >>>>
> >>>
> >>> Yeah, I think this is a difference in how the linker works on ppc vs
> >>> arm. Doing
> >>>
> >>>
> >>> For reference, I have
> >>>
> >>> $ ${CROSS_COMPILE}ld --version
> >>> GNU ld (GNU Binutils for Ubuntu) 2.30
> >>
> >> Which SPL are you using on PowerPC?  There's the one based
> >> CONFIG_SPL_FRAMEWORK and the one that's not.  I suspect it's a framework
> >> vs not problem here rather than linker exactly.
> >>
> > 
> > No, it's nothing to do with SPL per se. 
> 
> OK, I think I found it. On powerpc, the .fixup section contains a
> reference to .data.rel.env_htab, and all of .fixup is kept (because of
> KEEP(*(.fixup))), so env_htab cannot get garbage collected. Hence
> anything that section refers to must also exist. So it's not a defect in
> ppc-ld vs arm-ld, it's just a consequence of ppc having that .fixup section.
> 
> So, the only way to fix that is by either making sure env_flags_validate
> exists in the link, or avoiding having env_htab being part of the link
> in the first place. My patch does the latter.

Thanks for digging in to the root cause on this.  I'll take a harder
look at your patch, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200122/3cc86029/attachment.sig>

[PATCH] env: another attempt at fixing SPL build failures

[Tom Rini]

On Sun, Dec 15, 2019 at 10:29:39PM +0000, Rasmus Villemoes wrote:

> I'm also seeing the build failure that commit
> 
> 7d4776545b env: solve compilation error in SPL
> 
> tried to fix, namely that the reference to env_flags_validate from
> env_htab cannot be satisfied when flags.o is not built in. However,
> that commit got reverted by
> 
> d90fc9c3de Revert "env: solve compilation error in SPL"
> 
> Necessary, but not sufficient conditions to see this are
> 
> CONFIG_SPL=y (obviously)
> CONFIG_SPL_ENV_SUPPORT=n (so flags.o does not get compiled)
> CONFIG_SPL_LIBCOMMON_SUPPORT=y (so env/built-in.o is part of the SPL link)
> 
> Now, these are satisfied for e.g. imx6q_logic_defconfig. But that
> builds just fine, and spl/u-boot-spl.map lists .data.env_htab among
> the discarded (garbage collected) sections. Yet, on our
> mpc8309-derived board, we do see the build failure, so perhaps the
> linker works a bit differently on ppc than on ARM, or there's yet some
> other configuration option needed to observe the break.
> 
> This is another attempt at solving it, which also cleans up
> env/Makefile a bit: Introduce a def_bool y symbol CONFIG_ENV_SUPPORT
> which complements CONFIG_(SPL/TPL)_SUPPORT. Then use
> CONFIG_$(SPL_TPL_)ENV_SUPPORT to decide whether to include the five
> basic env/*.o files. For attr.o, flags.o and callback.o, this
> shouldn't change anything. Also, common.o and env.o still get
> unconditionally built for U-boot proper. But for TPL/SPL, those two
> are only included if CONFIG_(SPL/TPL)_SUPPORT is set.
> 
> Having that symbol should also allow simplifying conditionals such as
> 
> #if !defined(CONFIG_SPL_BUILD) || CONFIG_IS_ENABLED(ENV_SUPPORT)
> 
> found in drivers/reset/reset-socfpga.c to just
> CONFIG_IS_ENABLED(ENV_SUPPORT).
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200123/dcca0aa8/attachment.sig>