Hi
On Fri, Oct 28, 2022 at 6:40 PM Pali Rohár <p...@kernel.org> wrote:
>
> Hello! This is really much better solution! Few comments are below.
>
> On Friday 28 October 2022 18:18:49 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.
> >
> > Drop trying to read the initial env with elf tools from the compiler
> > specific object file in favour of adding and using a host tool with
> > the only functionality of printing the initial env to stdout.
> >
> > See also:
> > https://lore.kernel.org/all/927b122e-1f62-e790-f5ca-30bae4332...@foss.st.com/
> >
> > Signed-off-by: Max Krummenacher <max.krummenac...@toradex.com>
> >
> > ---
> >
> > Changes in v2:
> > - reworked to build a host tool which prints the configured
> > environment as proposed by Pali Rohár
> >
> > https://lore.kernel.org/u-boot/20221018174827.1393211-1-max.oss...@gmail.com/
> > - renamed patch, v1 used "Makefile: fix u-boot-initial-env target if lto is
> > enabled"
> >
> > Makefile | 7 ++++---
> > scripts/.gitignore | 1 +
> > scripts/Makefile | 5 +++++
> > scripts/printinitialenv.c | 44 +++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 54 insertions(+), 3 deletions(-)
> > create mode 100644 scripts/printinitialenv.c
> >
> > diff --git a/Makefile b/Makefile
> > index 0f1174718f7..2f97d63c398 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2442,9 +2442,10 @@ endif
> > $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modpost
> >
> > quiet_cmd_genenv = GENENV $@
> > -cmd_genenv = $(OBJCOPY) --dump-section .rodata.default_environment=$@
> > env/common.o; \
> > - sed --in-place -e 's/\x00/\x0A/g' $@; sed --in-place -e '/^\s*$$/d'
> > $@; \
> > - sort --field-separator== -k1,1 --stable $@ -o $@
> > +cmd_genenv = \
> > + scripts/printinitialenv | \
> > + sed -e 's/\x00/\x0A/g' -e '/^\s*$$/d' | \
>
> I think that you do not need this sed anymore as you print newline in
> host tool.
Missed that one, will change in a V3.
>
> > + sort --field-separator== -k1,1 --stable -o $@
> >
> > u-boot-initial-env: u-boot.bin
>
> It is needed to update dependencies for u-boot-initial-env target. Now
> it does not depend on u-boot.bin but rather on printinitialenv tool.
I'm unsure if that is the best way forward. The initial solution would
also not need to depend on u-boot.bin but rather on env/common.o.
I guess that the intention was that the U-Boot binary and the
u-boot-initial-env file should not be out of sync.
Opinions?
>
> > $(call if_changed,genenv)
> > diff --git a/scripts/.gitignore b/scripts/.gitignore
> > index 08cc824bac3..6068724a0d4 100644
> > --- a/scripts/.gitignore
> > +++ b/scripts/.gitignore
> > @@ -2,3 +2,4 @@
> > # Generated files
> > #
> > bin2c
> > +printinitialenv
> > diff --git a/scripts/Makefile b/scripts/Makefile
> > index 8731e6cecd7..ba993820571 100644
> > --- a/scripts/Makefile
> > +++ b/scripts/Makefile
> > @@ -5,8 +5,13 @@
> > #
> > ---------------------------------------------------------------------------
> >
> > hostprogs-$(CONFIG_BUILD_BIN2C) += bin2c
> > +hostprogs-y += printinitialenv
>
> I'm not sure if the scripts/ is the correct directory for the this tool.
> Maybe it should be in tools? Lets wait what maintainers or Tom say.
According to tools/Makefile tools should be used for tools which
are not dependent on any boards. This here is a helper during
the build of a particular U-Boot configuration. That is why I put it
into scripts/.
Max
>
> >
> > always := $(hostprogs-y)
> >
> > +HOSTCFLAGS_printinitialenv.o += \
> > + $(patsubst -I%,-idirafter%, $(filter -I%, $(UBOOTINCLUDE))) \
> > + -DUSE_HOSTCC
> > +
> > # Let clean descend into subdirs
> > subdir- += basic kconfig dtc
> > diff --git a/scripts/printinitialenv.c b/scripts/printinitialenv.c
> > new file mode 100644
> > index 00000000000..c58b234d679
> > --- /dev/null
> > +++ b/scripts/printinitialenv.c
> > @@ -0,0 +1,44 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * (C) Copyright 2022
> > + * Max Krummenacher, Toradex
> > + *
> > + * Snippets taken from tools/env/fw_env.c
> > + *
> > + * This prints the list of default environment variables as currently
> > + * configured.
> > + *
> > + */
> > +
> > +#include <stdio.h>
> > +
> > +/* Pull in the current config to define the default environment */
> > +#include <linux/kconfig.h>
> > +
> > +#ifndef __ASSEMBLY__
> > +#define __ASSEMBLY__ /* get only #defines from config.h */
> > +#include <config.h>
> > +#undef __ASSEMBLY__
> > +#else
> > +#include <config.h>
> > +#endif
> > +
> > +#define DEFAULT_ENV_INSTANCE_STATIC
> > +#include <generated/environment.h>
> > +#include <env_default.h>
> > +
> > +int main(void)
> > +{
> > + char *env, *nxt;
> > +
> > + for (env = default_environment; *env; env = nxt + 1) {
> > + for (nxt = env; *nxt; ++nxt) {
> > + if (nxt >=
> > &default_environment[sizeof(default_environment)]) {
> > + fprintf(stderr, "## Error: environment not
> > terminated\n");
> > + return -1;
> > + }
> > + }
> > + printf("%s\n", env);
> > + }
> > + return 0;
> > +}
> > --
> > 2.35.3
> >
