[PATCH] Makefile: fix u-boot-initial-env target if lto is enabled

[PATCH] Makefile: fix u-boot-initial-env target if lto is enabled

[Max Krummenacher]

From: Max Krummenacher <max.krummenacher@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-30bae4332c77@foss.st.com/

Signed-off-by: Max Krummenacher <max.krummenacher@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; \
 	sed --in-place -e 's/\x00/\x0A/g' $@; sed --in-place -e '/^\s*$$/d' $@; \
 	sort --field-separator== -k1,1 --stable $@ -o $@
+endif
 
 u-boot-initial-env: u-boot.bin
 	$(call if_changed,genenv)
-- 
2.35.3

Re: [PATCH] Makefile: fix u-boot-initial-env target if lto is enabled

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2184 bytes --]

On Tue, Oct 18, 2022 at 07:48:27PM +0200, Max Krummenacher wrote:
> From: Max Krummenacher <max.krummenacher@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-30bae4332c77@foss.st.com/
> 
> Signed-off-by: Max Krummenacher <max.krummenacher@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; \
>  	sed --in-place -e 's/\x00/\x0A/g' $@; sed --in-place -e '/^\s*$$/d' $@; \
>  	sort --field-separator== -k1,1 --stable $@ -o $@
> +endif
>  
>  u-boot-initial-env: u-boot.bin
>  	$(call if_changed,genenv)

Can we pipe the output instead of making a new object file? Maybe not,
in a portable way it seems. But, I'm not sure the above respects using
O= as well so that does need to be checked and fixed if so.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Makefile: fix u-boot-initial-env target if lto is enabled

[Pali Rohár]

On Tuesday 18 October 2022 19:48:27 Max Krummenacher wrote:
> From: Max Krummenacher <max.krummenacher@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-30bae4332c77@foss.st.com/
> 
> Signed-off-by: Max Krummenacher <max.krummenacher@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.

So instead of using section name, I would rather suggest to compile
simple host application which prints it.

>  	sed --in-place -e 's/\x00/\x0A/g' $@; sed --in-place -e '/^\s*$$/d' $@; \
>  	sort --field-separator== -k1,1 --stable $@ -o $@
> +endif
>  
>  u-boot-initial-env: u-boot.bin
>  	$(call if_changed,genenv)
> -- 
> 2.35.3
> 

Re: [PATCH] Makefile: fix u-boot-initial-env target if lto is enabled

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2013 bytes --]

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.krummenacher@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-30bae4332c77@foss.st.com/
> > 
> > Signed-off-by: Max Krummenacher <max.krummenacher@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?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Makefile: fix u-boot-initial-env target if lto is enabled

[Pali Rohár]

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.krummenacher@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-30bae4332c77@foss.st.com/
> > > 
> > > Signed-off-by: Max Krummenacher <max.krummenacher@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?
> 
> -- 
> Tom

E.g. for mvebu_espressobin-88f3720_defconfig

Re: [PATCH] Makefile: fix u-boot-initial-env target if lto is enabled

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2697 bytes --]

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.krummenacher@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-30bae4332c77@foss.st.com/
> > > > 
> > > > Signed-off-by: Max Krummenacher <max.krummenacher@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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Makefile: fix u-boot-initial-env target if lto is enabled

[Pali Rohár]

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.krummenacher@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-30bae4332c77@foss.st.com/
> > > > > 
> > > > > Signed-off-by: Max Krummenacher <max.krummenacher@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.
> 
> -- 
> Tom

Yes, we can solve it. Marek was working on solution for setting default
variables at runtime but seems it is not finished yet.

Re: [PATCH] Makefile: fix u-boot-initial-env target if lto is enabled

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3272 bytes --]

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.krummenacher@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-30bae4332c77@foss.st.com/
> > > > > > 
> > > > > > Signed-off-by: Max Krummenacher <max.krummenacher@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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Makefile: fix u-boot-initial-env target if lto is enabled

[Max Krummenacher]

Hi all

Thanks for the feedback

On Tue, Oct 18, 2022 at 8:23 PM Tom Rini <trini@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.krummenacher@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-30bae4332c77@foss.st.com/
> > > > > > >
> > > > > > > Signed-off-by: Max Krummenacher <max.krummenacher@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

Re: [PATCH] Makefile: fix u-boot-initial-env target if lto is enabled

[Max Krummenacher]

Hi Tom

On Tue, Oct 18, 2022 at 7:57 PM Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Oct 18, 2022 at 07:48:27PM +0200, Max Krummenacher wrote:
> > From: Max Krummenacher <max.krummenacher@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-30bae4332c77@foss.st.com/
> >
> > Signed-off-by: Max Krummenacher <max.krummenacher@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; \
> >       sed --in-place -e 's/\x00/\x0A/g' $@; sed --in-place -e '/^\s*$$/d' $@; \
> >       sort --field-separator== -k1,1 --stable $@ -o $@
> > +endif
> >
> >  u-boot-initial-env: u-boot.bin
> >       $(call if_changed,genenv)
>
> Can we pipe the output instead of making a new object file? Maybe not,
> in a portable way it seems. But, I'm not sure the above respects using
> O= as well so that does need to be checked and fixed if so.

While I didn't test it seems that objcopy doesn't allow to pipe data in and out
of it.

Max
>
> --
> Tom

Re: [PATCH] Makefile: fix u-boot-initial-env target if lto is enabled

[Pali Rohár]

On Wednesday 19 October 2022 14:59:49 Max Krummenacher wrote:
> Hi Tom
> 
> On Tue, Oct 18, 2022 at 7:57 PM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Oct 18, 2022 at 07:48:27PM +0200, Max Krummenacher wrote:
> > > From: Max Krummenacher <max.krummenacher@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-30bae4332c77@foss.st.com/
> > >
> > > Signed-off-by: Max Krummenacher <max.krummenacher@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; \
> > >       sed --in-place -e 's/\x00/\x0A/g' $@; sed --in-place -e '/^\s*$$/d' $@; \
> > >       sort --field-separator== -k1,1 --stable $@ -o $@
> > > +endif
> > >
> > >  u-boot-initial-env: u-boot.bin
> > >       $(call if_changed,genenv)
> >
> > Can we pipe the output instead of making a new object file? Maybe not,
> > in a portable way it seems. But, I'm not sure the above respects using
> > O= as well so that does need to be checked and fixed if so.
> 
> While I didn't test it seems that objcopy doesn't allow to pipe data in and out
> of it.

I think it should be easier to compile a new application which includes
header file with env data and prints it to stdout. There are already
host tools like mkimage which are run during u-boot build process, so
adding a new simple host tool should not be a problem -- instead of
trying to hack objcopy, or try to find a way how to play with its
input/output redirection and symbol names to achieve same thing.

> Max
> >
> > --
> > Tom