diff mbox series

[U-Boot,v2] Enable expression support for CONFIG_BOARD_SIZE_LIMIT

Message ID 20181207192751.30422-1-wd@denx.de
State Changes Requested
Delegated to: Stefano Babic
Headers show
Series [U-Boot,v2] Enable expression support for CONFIG_BOARD_SIZE_LIMIT | expand

Commit Message

Wolfgang Denk Dec. 7, 2018, 7:27 p.m. UTC 
So far, the use of CONFIG_BOARD_SIZE_LIMIT would only work with
plain numeric constants.  Extend it to allow for expressions, so one
can for example use

	#define CONFIG_BOARD_SIZE_LIMIT	(768 << 10)

in the board configuration.

Signed-off-by: Wolfgang Denk <wd@denx.de>
Tested-by: Fabio Estevam <festevam@gmail.com>

Cc: Fabio Estevam <festevam@gmail.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Vanessa Maegima <vanessa.maegima@nxp.com>
Cc: Otavio Salvador <otavio@ossystems.com.br>
Cc: John Weber <john.weber@technexion.com>
Cc: Stefan Roese <sr@denx.de>
---
v2: replace bashism for evaluating expressions in CONFIG_BOARD_SIZE_LIMIT
    by another call to awk.

Note 1: As gawk lacks an eval function and we don't want to rely on
 bash being used as shell, we use another call to awk to evaluate the
 expression. This has the disadvantage that we cannot use expressions
 like "<<" which awk does not understand. OK, one could replace awk
 by something better...

Note 2: This patch focusses on enabling this new feature.  It does
 not addres another issue that should be solved in a later
 commit: the duplication of the same code in Makefile and
 arch/arm/mach-imx/Makefile

---
 Makefile                   | 17 ++++++++---------
 arch/arm/mach-imx/Makefile | 17 ++++++++---------
 2 files changed, 16 insertions(+), 18 deletions(-)

Comments

Tom Rini Dec. 14, 2018, 7:16 p.m. UTC | #1 
On Fri, Dec 07, 2018 at 08:27:51PM +0100, Wolfgang Denk wrote:

> So far, the use of CONFIG_BOARD_SIZE_LIMIT would only work with
> plain numeric constants.  Extend it to allow for expressions, so one
> can for example use
> 
> 	#define CONFIG_BOARD_SIZE_LIMIT	(768 << 10)
> 
> in the board configuration.
> 
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Tested-by: Fabio Estevam <festevam@gmail.com>
> 
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Vanessa Maegima <vanessa.maegima@nxp.com>
> Cc: Otavio Salvador <otavio@ossystems.com.br>
> Cc: John Weber <john.weber@technexion.com>
> Cc: Stefan Roese <sr@denx.de>
> ---
> v2: replace bashism for evaluating expressions in CONFIG_BOARD_SIZE_LIMIT
>     by another call to awk.
> 
> Note 1: As gawk lacks an eval function and we don't want to rely on
>  bash being used as shell, we use another call to awk to evaluate the
>  expression. This has the disadvantage that we cannot use expressions
>  like "<<" which awk does not understand. OK, one could replace awk
>  by something better...
> 
> Note 2: This patch focusses on enabling this new feature.  It does
>  not addres another issue that should be solved in a later
>  commit: the duplication of the same code in Makefile and
>  arch/arm/mach-imx/Makefile
> 
> ---
>  Makefile                   | 17 ++++++++---------
>  arch/arm/mach-imx/Makefile | 17 ++++++++---------
>  2 files changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 0d11ff9797..87eb0fd2b1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -774,15 +774,14 @@ LDPPFLAGS += \
>  
>  ifneq ($(CONFIG_BOARD_SIZE_LIMIT),)
>  BOARD_SIZE_CHECK = \
> -	@actual=`wc -c $@ | awk '{print $$1}'`; \
> -	limit=`printf "%d" $(CONFIG_BOARD_SIZE_LIMIT)`; \
> -	if test $$actual -gt $$limit; then \
> -		echo "$@ exceeds file size limit:" >&2 ; \
> -		echo "  limit:  $$limit bytes" >&2 ; \
> -		echo "  actual: $$actual bytes" >&2 ; \
> -		echo "  excess: $$((actual - limit)) bytes" >&2; \
> -		exit 1; \
> -	fi
> +	@(awk "END{print $$(echo $(CONFIG_BOARD_SIZE_LIMIT))}" /dev/null; wc -c $@ ) | \

So this fails with awk being provided by mawk, not gawk:
$ mawk "END{print $(echo 0xE0000)}" /dev/null;
0
$ gawk "END{print $(echo 0xE0000)}" /dev/null;
917504

And then things fail as mawk doesn't like hex.

Now, at the end of the day, I'm now not sure I agree with this patch in
concept.  When CONFIG_BOARD_SIZE_LIMIT is moved to Kconfig it will be
taking I would assume a hex input and so we don't have to worry about <<
at all.
Simon Goldschmidt March 6, 2019, 8:54 p.m. UTC | #2 
Tom,

On Fri, Dec 14, 2018 at 8:16 PM Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Dec 07, 2018 at 08:27:51PM +0100, Wolfgang Denk wrote:
>
> > So far, the use of CONFIG_BOARD_SIZE_LIMIT would only work with
> > plain numeric constants.  Extend it to allow for expressions, so one
> > can for example use
> >
> >       #define CONFIG_BOARD_SIZE_LIMIT (768 << 10)
> >
> > in the board configuration.
> >
> > Signed-off-by: Wolfgang Denk <wd@denx.de>
> > Tested-by: Fabio Estevam <festevam@gmail.com>
> >
> > Cc: Fabio Estevam <festevam@gmail.com>
> > Cc: Stefano Babic <sbabic@denx.de>
> > Cc: Vanessa Maegima <vanessa.maegima@nxp.com>
> > Cc: Otavio Salvador <otavio@ossystems.com.br>
> > Cc: John Weber <john.weber@technexion.com>
> > Cc: Stefan Roese <sr@denx.de>
> > ---
> > v2: replace bashism for evaluating expressions in CONFIG_BOARD_SIZE_LIMIT
> >     by another call to awk.
> >
> > Note 1: As gawk lacks an eval function and we don't want to rely on
> >  bash being used as shell, we use another call to awk to evaluate the
> >  expression. This has the disadvantage that we cannot use expressions
> >  like "<<" which awk does not understand. OK, one could replace awk
> >  by something better...
> >
> > Note 2: This patch focusses on enabling this new feature.  It does
> >  not addres another issue that should be solved in a later
> >  commit: the duplication of the same code in Makefile and
> >  arch/arm/mach-imx/Makefile
> >
> > ---
> >  Makefile                   | 17 ++++++++---------
> >  arch/arm/mach-imx/Makefile | 17 ++++++++---------
> >  2 files changed, 16 insertions(+), 18 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 0d11ff9797..87eb0fd2b1 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -774,15 +774,14 @@ LDPPFLAGS += \
> >
> >  ifneq ($(CONFIG_BOARD_SIZE_LIMIT),)
> >  BOARD_SIZE_CHECK = \
> > -     @actual=`wc -c $@ | awk '{print $$1}'`; \
> > -     limit=`printf "%d" $(CONFIG_BOARD_SIZE_LIMIT)`; \
> > -     if test $$actual -gt $$limit; then \
> > -             echo "$@ exceeds file size limit:" >&2 ; \
> > -             echo "  limit:  $$limit bytes" >&2 ; \
> > -             echo "  actual: $$actual bytes" >&2 ; \
> > -             echo "  excess: $$((actual - limit)) bytes" >&2; \
> > -             exit 1; \
> > -     fi
> > +     @(awk "END{print $$(echo $(CONFIG_BOARD_SIZE_LIMIT))}" /dev/null; wc -c $@ ) | \
>
> So this fails with awk being provided by mawk, not gawk:
> $ mawk "END{print $(echo 0xE0000)}" /dev/null;
> 0
> $ gawk "END{print $(echo 0xE0000)}" /dev/null;
> 917504
>
> And then things fail as mawk doesn't like hex.
>
> Now, at the end of the day, I'm now not sure I agree with this patch in
> concept.  When CONFIG_BOARD_SIZE_LIMIT is moved to Kconfig it will be
> taking I would assume a hex input and so we don't have to worry about <<
> at all.

Sorry to warm up an old thread, but I have some slightly new input on this.

I can understand you disliking the concept of this patch regarding U-Boot
proper size check (if CONFIG_BOARD_SIZE_LIMIT moves to Kconfig).

However, I think for SPL this is different: SPL often starts with one single
small SRAM shared for code + data. In this case, the size available for the
SPL binary often can be calculated like this:

CONFIG_SPL_MAX_SIZE = SRAM_SIZE - SYS_MALLOC_F_LEN -
GENERATED_GBL_DATA_SIZE - STACKSIZE;

Being like that, it cannot just be moved to Kconfig and by definition is not
a hardcoded single hex number but changes depending on other options.

I see two ways out here:
a) continue the way of this patch until it works for all shells/awks or
b) implement SPL binary size check using 4 constants instead of 1 (see above)

I'm willing to code this through, as I am hitting this limit on socfpga, so I
could need a decision ;-)

Regards,
Simon
Tom Rini March 8, 2019, 5:17 p.m. UTC | #3 
On Wed, Mar 06, 2019 at 09:54:20PM +0100, Simon Goldschmidt wrote:
> Tom,
> 
> On Fri, Dec 14, 2018 at 8:16 PM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Dec 07, 2018 at 08:27:51PM +0100, Wolfgang Denk wrote:
> >
> > > So far, the use of CONFIG_BOARD_SIZE_LIMIT would only work with
> > > plain numeric constants.  Extend it to allow for expressions, so one
> > > can for example use
> > >
> > >       #define CONFIG_BOARD_SIZE_LIMIT (768 << 10)
> > >
> > > in the board configuration.
> > >
> > > Signed-off-by: Wolfgang Denk <wd@denx.de>
> > > Tested-by: Fabio Estevam <festevam@gmail.com>
> > >
> > > Cc: Fabio Estevam <festevam@gmail.com>
> > > Cc: Stefano Babic <sbabic@denx.de>
> > > Cc: Vanessa Maegima <vanessa.maegima@nxp.com>
> > > Cc: Otavio Salvador <otavio@ossystems.com.br>
> > > Cc: John Weber <john.weber@technexion.com>
> > > Cc: Stefan Roese <sr@denx.de>
> > > ---
> > > v2: replace bashism for evaluating expressions in CONFIG_BOARD_SIZE_LIMIT
> > >     by another call to awk.
> > >
> > > Note 1: As gawk lacks an eval function and we don't want to rely on
> > >  bash being used as shell, we use another call to awk to evaluate the
> > >  expression. This has the disadvantage that we cannot use expressions
> > >  like "<<" which awk does not understand. OK, one could replace awk
> > >  by something better...
> > >
> > > Note 2: This patch focusses on enabling this new feature.  It does
> > >  not addres another issue that should be solved in a later
> > >  commit: the duplication of the same code in Makefile and
> > >  arch/arm/mach-imx/Makefile
> > >
> > > ---
> > >  Makefile                   | 17 ++++++++---------
> > >  arch/arm/mach-imx/Makefile | 17 ++++++++---------
> > >  2 files changed, 16 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/Makefile b/Makefile
> > > index 0d11ff9797..87eb0fd2b1 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -774,15 +774,14 @@ LDPPFLAGS += \
> > >
> > >  ifneq ($(CONFIG_BOARD_SIZE_LIMIT),)
> > >  BOARD_SIZE_CHECK = \
> > > -     @actual=`wc -c $@ | awk '{print $$1}'`; \
> > > -     limit=`printf "%d" $(CONFIG_BOARD_SIZE_LIMIT)`; \
> > > -     if test $$actual -gt $$limit; then \
> > > -             echo "$@ exceeds file size limit:" >&2 ; \
> > > -             echo "  limit:  $$limit bytes" >&2 ; \
> > > -             echo "  actual: $$actual bytes" >&2 ; \
> > > -             echo "  excess: $$((actual - limit)) bytes" >&2; \
> > > -             exit 1; \
> > > -     fi
> > > +     @(awk "END{print $$(echo $(CONFIG_BOARD_SIZE_LIMIT))}" /dev/null; wc -c $@ ) | \
> >
> > So this fails with awk being provided by mawk, not gawk:
> > $ mawk "END{print $(echo 0xE0000)}" /dev/null;
> > 0
> > $ gawk "END{print $(echo 0xE0000)}" /dev/null;
> > 917504
> >
> > And then things fail as mawk doesn't like hex.
> >
> > Now, at the end of the day, I'm now not sure I agree with this patch in
> > concept.  When CONFIG_BOARD_SIZE_LIMIT is moved to Kconfig it will be
> > taking I would assume a hex input and so we don't have to worry about <<
> > at all.
> 
> Sorry to warm up an old thread, but I have some slightly new input on this.
> 
> I can understand you disliking the concept of this patch regarding U-Boot
> proper size check (if CONFIG_BOARD_SIZE_LIMIT moves to Kconfig).
> 
> However, I think for SPL this is different: SPL often starts with one single
> small SRAM shared for code + data. In this case, the size available for the
> SPL binary often can be calculated like this:
> 
> CONFIG_SPL_MAX_SIZE = SRAM_SIZE - SYS_MALLOC_F_LEN -
> GENERATED_GBL_DATA_SIZE - STACKSIZE;
> 
> Being like that, it cannot just be moved to Kconfig and by definition is not
> a hardcoded single hex number but changes depending on other options.
> 
> I see two ways out here:
> a) continue the way of this patch until it works for all shells/awks or
> b) implement SPL binary size check using 4 constants instead of 1 (see above)
> 
> I'm willing to code this through, as I am hitting this limit on socfpga, so I
> could need a decision ;-)

OK, so a few thoughts here.
- What's the portable way to do hex-based math?  If we really need it?
- Really, CONFIG_{SPL,TPL}_MAX_SIZE is not user configurable, it's the
  SoC/design imposed limit.  CONFIG_BOARD_MAX_SIZE is somewhat
  configurable as that tends to also include "don't grow past X or we
  overwrite Y and break things".

Part of the problem area is that we need to take N values, which are
#defined, and use that to see if our final result is too large.  Prior
to needing device tree stuff, OK, we can get the linker on our side to
enforce this (well enough that we can fudge the rest, ie artificially
lower SRAM size a little and comment on why).  With DTBs and such, we
need to take a look at the final result too, of each stage.

So yes, I think we need to figure out some portable way to deal with
checking all constants.  And we'll Kconfig what we can/should Kconfig,
and #define what we need to define and (ugh) calculate and use what must
be done that way.  Thanks!
Martin Husemann March 8, 2019, 5:28 p.m. UTC | #4 
On Fri, Mar 08, 2019 at 12:17:09PM -0500, Tom Rini wrote:
> OK, so a few thoughts here.
> - What's the portable way to do hex-based math?  If we really need it?

Use printf(3) to convert to/from hex, and standard shell arithmetic
with $(( )).

Looks horrible, but something like:

v=$(( $( printf "%d\n" 0xa0 ) + $( printf "%d\n" 0x10 ) ))
printf "v = %d (%x)\n" $v $v


... maybe arranged into some sh helper functions.

Martin
Philipp Tomsich March 8, 2019, 5:53 p.m. UTC | #5 
> On 08.03.2019, at 18:28, Martin Husemann <martin@NetBSD.org> wrote:
> 
> On Fri, Mar 08, 2019 at 12:17:09PM -0500, Tom Rini wrote:
>> OK, so a few thoughts here.
>> - What's the portable way to do hex-based math?  If we really need it?
> 
> Use printf(3) to convert to/from hex, and standard shell arithmetic
> with $(( )).
> 
> Looks horrible, but something like:
> 
> v=$(( $( printf "%d\n" 0xa0 ) + $( printf "%d\n" 0x10 ) ))
> printf "v = %d (%x)\n" $v $v

Can we just assume that awk is available?  After all, AWK is defined
in the top-level Makefile...

> 
> 
> ... maybe arranged into some sh helper functions.
> 
> Martin
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
Simon Goldschmidt March 8, 2019, 6:16 p.m. UTC | #6 
Philipp Tomsich <philipp.tomsich@theobroma-systems.com> schrieb am Fr., 8.
März 2019, 18:53:

>
>
> > On 08.03.2019, at 18:28, Martin Husemann <martin@NetBSD.org> wrote:
> >
> > On Fri, Mar 08, 2019 at 12:17:09PM -0500, Tom Rini wrote:
> >> OK, so a few thoughts here.
> >> - What's the portable way to do hex-based math?  If we really need it?
> >
> > Use printf(3) to convert to/from hex, and standard shell arithmetic
> > with $(( )).
> >
> > Looks horrible, but something like:
> >
> > v=$(( $( printf "%d\n" 0xa0 ) + $( printf "%d\n" 0x10 ) ))
> > printf "v = %d (%x)\n" $v $v
>
> Can we just assume that awk is available?  After all, AWK is defined
> in the top-level Makefile...
>

I guess we can, but see Toms mail from mid of December: not all awk
flavours seem to supper hey numbers. So I guess the next round on this
patch should try to test different shells as well as different awk's...

Regards,
Simon
Tom Rini March 8, 2019, 7:55 p.m. UTC | #7 
On Fri, Mar 08, 2019 at 06:53:28PM +0100, Philipp Tomsich wrote:
> 
> 
> > On 08.03.2019, at 18:28, Martin Husemann <martin@NetBSD.org> wrote:
> > 
> > On Fri, Mar 08, 2019 at 12:17:09PM -0500, Tom Rini wrote:
> >> OK, so a few thoughts here.
> >> - What's the portable way to do hex-based math?  If we really need it?
> > 
> > Use printf(3) to convert to/from hex, and standard shell arithmetic
> > with $(( )).
> > 
> > Looks horrible, but something like:
> > 
> > v=$(( $( printf "%d\n" 0xa0 ) + $( printf "%d\n" 0x10 ) ))
> > printf "v = %d (%x)\n" $v $v
> 
> Can we just assume that awk is available?  After all, AWK is defined
> in the top-level Makefile...

No, we can't exactly.  In sum, gawk and mawk behave differently and mawk
doesn't do hex and yes, we run into that in the wild.  Whatever we do
here needs to be POSIX shell happy (or something more strict than that?)
as it needs to work on macOS and Free/Net/OpenBSD and anything else that
we can otherwise be building on.
Ismael Luceno Cortes March 15, 2019, 10:13 a.m. UTC | #8 
On 08/Mar/2019 18:28, Martin Husemann wrote:
> On Fri, Mar 08, 2019 at 12:17:09PM -0500, Tom Rini wrote:
> > OK, so a few thoughts here.
> > - What's the portable way to do hex-based math?  If we really need it?
> 
> Use printf(3) to convert to/from hex, and standard shell arithmetic
> with $(( )).
> 
> Looks horrible, but something like:
> 
> v=$(( $( printf "%d\n" 0xa0 ) + $( printf "%d\n" 0x10 ) ))
> printf "v = %d (%x)\n" $v $v
> 
> 
> ... maybe arranged into some sh helper functions.

dash, bash, mksh, zsh, all ksh-compatible shells in fact, support hex
numbers on arithmetic expressions, no need for conversion.
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index 0d11ff9797..87eb0fd2b1 100644
--- a/Makefile
+++ b/Makefile
@@ -774,15 +774,14 @@  LDPPFLAGS += \
 
 ifneq ($(CONFIG_BOARD_SIZE_LIMIT),)
 BOARD_SIZE_CHECK = \
-	@actual=`wc -c $@ | awk '{print $$1}'`; \
-	limit=`printf "%d" $(CONFIG_BOARD_SIZE_LIMIT)`; \
-	if test $$actual -gt $$limit; then \
-		echo "$@ exceeds file size limit:" >&2 ; \
-		echo "  limit:  $$limit bytes" >&2 ; \
-		echo "  actual: $$actual bytes" >&2 ; \
-		echo "  excess: $$((actual - limit)) bytes" >&2; \
-		exit 1; \
-	fi
+	@(awk "END{print $$(echo $(CONFIG_BOARD_SIZE_LIMIT))}" /dev/null; wc -c $@ ) | \
+	awk 'BEGIN { getline limit } \
+	{ if ($$1 > limit) { \
+		printf "%s exceeds file size limit:\n", $$2; \
+		printf "  limit:  %d bytes\n", limit; \
+		printf "  actual: %d bytes\n", $$1; \
+		printf "  excess: %d bytes\n", $$1 - limit; \
+		exit 1; } }'
 else
 BOARD_SIZE_CHECK =
 endif
diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile
index 53d9e5f42b..36d1ecc732 100644
--- a/arch/arm/mach-imx/Makefile
+++ b/arch/arm/mach-imx/Makefile
@@ -60,15 +60,14 @@  endif
 
 ifneq ($(CONFIG_BOARD_SIZE_LIMIT),)
 BOARD_SIZE_CHECK = \
-        @actual=`wc -c $@ | awk '{print $$1}'`; \
-        limit=`printf "%d" $(CONFIG_BOARD_SIZE_LIMIT)`; \
-        if test $$actual -gt $$limit; then \
-                echo "$@ exceeds file size limit:" >&2 ; \
-                echo "  limit:  $$limit bytes" >&2 ; \
-                echo "  actual: $$actual bytes" >&2 ; \
-                echo "  excess: $$((actual - limit)) bytes" >&2; \
-                exit 1; \
-        fi
+	@(awk "END{print $$(echo $(CONFIG_BOARD_SIZE_LIMIT))}" /dev/null; wc -c $@ ) | \
+	awk 'BEGIN { getline limit } \
+	{ if ($$1 > limit) { \
+		printf "%s exceeds file size limit:\n", $$2; \
+		printf "  limit:  %d bytes\n", limit; \
+		printf "  actual: %d bytes\n", $$1; \
+		printf "  excess: %d bytes\n", $$1 - limit; \
+		exit 1; } }'
 else
 BOARD_SIZE_CHECK =
 endif