[PATCH v2 1/1] Makefile: rework u-boot-initial-env target

[Max Krummenacher]

Hi

On Fri, Oct 28, 2022 at 6:40 PM Pali Rohár <pali at 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.krummenacher at 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-30bae4332c77@foss.st.com/
> >
> > Signed-off-by: Max Krummenacher <max.krummenacher at 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.09@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
> >