Hi all Thanks for the feedback
On Tue, Oct 18, 2022 at 8:23 PM Tom Rini <tr...@konsulko.com> wrote: > > On Tue, Oct 18, 2022 at 08:19:23PM +0200, Pali Rohár wrote: > > On Tuesday 18 October 2022 14:17:23 Tom Rini wrote: > > > On Tue, Oct 18, 2022 at 08:06:27PM +0200, Pali Rohár wrote: > > > > On Tuesday 18 October 2022 14:04:46 Tom Rini wrote: > > > > > On Tue, Oct 18, 2022 at 08:03:31PM +0200, Pali Rohár wrote: > > > > > > On Tuesday 18 October 2022 19:48:27 Max Krummenacher wrote: > > > > > > > From: Max Krummenacher <max.krummenac...@toradex.com> > > > > > > > > > > > > > > With LTO enabled the U-Boot initial environment is no longer > > > > > > > stored > > > > > > > in an easy accessible section in env/common.o. I.e. the section > > > > > > > name > > > > > > > changes from build to build, its content maybe compressed and it > > > > > > > is > > > > > > > annotated with additional data. > > > > > > > > > > > > > > For GCC adding the option '-ffat-lto-objects' when compiling > > > > > > > common.o > > > > > > > adds additionally the traditional sections in the object file and > > > > > > > 'make u-boot-initial-env' would work also for the LTO enabled > > > > > > > case. > > > > > > > However clang doesn't have that option. > > > > > > > > > > > > > > Fix this by recompiling common.o into a object file only used for > > > > > > > the creation of u-boot-initial-env if LTO is enabled. > > > > > > > > > > > > > > See also: > > > > > > > https://lore.kernel.org/all/927b122e-1f62-e790-f5ca-30bae4332...@foss.st.com/ > > > > > > > > > > > > > > Signed-off-by: Max Krummenacher <max.krummenac...@toradex.com> > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > Makefile | 8 ++++++++ > > > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > > > > > diff --git a/Makefile b/Makefile > > > > > > > index 3866cc62f9a..cd45c720d23 100644 > > > > > > > --- a/Makefile > > > > > > > +++ b/Makefile > > > > > > > @@ -2451,9 +2451,17 @@ endif > > > > > > > $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modpost > > > > > > > > > > > > > > quiet_cmd_genenv = GENENV $@ > > > > > > > +ifeq ($(LTO_ENABLE),y) > > > > > > > +cmd_genenv = $(CC) $(filter-out $(LTO_CFLAGS),$(c_flags)) -c -o > > > > > > > env/initial_env.o env/common.c; \ > > > > > > > + $(OBJCOPY) --dump-section .rodata.default_environment=$@ > > > > > > > env/initial_env.o; \ > > > > > > > + sed --in-place -e 's/\x00/\x0A/g' $@; sed --in-place -e > > > > > > > '/^\s*$$/d' $@; \ > > > > > > > + sort --field-separator== -k1,1 --stable $@ -o $@; \ > > > > > > > + rm -f env/initial_env.o env/initial_env.su > > > > > > > +else > > > > > > > cmd_genenv = $(OBJCOPY) --dump-section > > > > > > > .rodata.default_environment=$@ env/common.o; \ > > > > > > > > > > > > This code is still broken because in some cases section name is not > > > > > > RO. > > > > > > > > > > Wait, when does that happen? > > > > > > > > E.g. for mvebu_espressobin-88f3720_defconfig > > > > > > Erm, ugh. I see 44be835d25ba ("arm: mvebu: Espressobin: Set default > > > value for $ethNaddr env variable") and c4df0f6f315c ("arm: mvebu: > > > Espressobin: Set default value for $fdtfile env variable") I guess we > > > couldn't solve this any other way? The platforms aren't unique in > > > needing / wanting to set MAC or fdtfile variables. > > > > Yes, we can solve it. Marek was working on solution for setting default > > variables at runtime but seems it is not finished yet. > > OK, then lets not assume DEFAULT_ENV_IS_RW is something we need to worry > about long term. The patch was intended to fix u-boot-initial-env make target for cases where it worked without 'CONFIG_LTO' to also work if that config gets enabled. Any use case where it already fails without 'CONFIG_LTO' set will not benefit. I agree that a variant of tools/env/fw_printenv compiled for the build host may solve additional issues present. However I will not be able to work on such a solution in the near future. Max > > -- > Tom