[U-Boot] FILE usage and and SPL limits for SRAM

Sun Apr 9 19:27:24 UTC 2017

Hi Tom,

On 4 April 2017 at 13:06, Tom Rini <trini at konsulko.com> wrote:
> On Tue, Mar 28, 2017 at 11:37:45AM +0530, Lokesh Vutla wrote:
>> + more folks.
>>
>> On Tuesday 28 March 2017 03:14 AM, Nishanth Menon wrote:
>> > Hi,
>> >
>> > we've kind of run into an interesting situation recently, but might be
>> > of interest for various folks trying to reduce the image sizes.
>> >
>> > our AM335x device has a limited amount of sram.. and the SPL tries to
>> > fit into it (a bit tricky given the restricted space we have on it on
>> > certain class of devices).
>> >
>> > arch/arm/mach-omap2/am33xx/u-boot-spl.lds is a bit custom tailored
>> > around this.
>> >
>> > Key in this is:
>> > . = ALIGN(4);
>> > .rodata : { *(SORT_BY_ALIGNMENT(.rodata*)) } >.sram
>> >
>> > . = ALIGN(4);
>> > .data : { *(SORT_BY_ALIGNMENT(.data*)) } >.sram
>> >
>> >
>> > Now, our jenkins build system happens to use a varied build path and
>> > uses O= path. to simplify the details:
>> > mkdir
>> > /tmp/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb/cccccccccccccccccccccccccccccccccccccccccccccccccc
>> >
>> > mkdir
>> > /tmp/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb/cccccccccccccccccccccccccccccccccccccccccccccccccc/b
>> >
>> >
>> > git clone u-boot
>> > cd u-boot
>> >
>> > git clean -fdx
>> > make CROSS_COMPILE=arm-linux-gnueabihf- O=../b am335x_evm_defconfig
>> > make CROSS_COMPILE=arm-linux-gnueabihf- O=../b all
>> >
>> > depending on depth of the path, this would fail.. a little bit of
>> > headscratching later..
>> > when using O= build system uses absolute paths, which translates to
>> > __FILE__ being absolute paths as well..
>> >
>> > in u-boot, any printf("%s", __FILE__) makes u-boot allocate this file
>> > path in rodata.
>> >
>> > So, depending on how deep the path is rodata size varies and ends up
>> > pushing .data out of sram max range.
>> >
>> > we dont really care to put a print of complete absolute path anyways,
>> > and I am not really sure of a clean way to resolve this:
>> > a) override __FILE__ with something.. -Wbuiltin-macro-redefined kicks in
>> > b) replace usage of __FILE__ with something like __FILENAME__ as
>> > recommended by [1]
>> >
>> >
>> > What is the suggestion we do?
>> >
>> > [1] http://stackoverflow.com/questions/8487986/file-macro-shows-full-path
>>
>> Any suggestions would be really helpful.
>
> So here's what I've come up with, and it's slightly incomplete:
> diff --git a/include/common.h b/include/common.h
> index 2cbbd5a60cd3..cdc61ef5b144 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -145,20 +145,19 @@ typedef volatile unsigned char    vu_char;
>   * in any case these failing assertions cannot be fixed with a reset (which
>   * may just do the same assertion again).
>   */
> -void __assert_fail(const char *assertion, const char *file, unsigned line,
> -                  const char *function);
> +void __assert_fail(const char *assertion, unsigned line, const char *function);
>  #define assert(x) \
>         ({ if (!(x) && _DEBUG) \
> -               __assert_fail(#x, __FILE__, __LINE__, __func__); })
> +               __assert_fail(#x, __LINE__, __func__); })
>
>  #define error(fmt, args...) do {                                       \
> -               printf("ERROR: " pr_fmt(fmt) "\nat %s:%d/%s()\n",       \
> -                       ##args, __FILE__, __LINE__, __func__);          \
> +               printf("ERROR: " pr_fmt(fmt) "\nat %s():%d\n",  \
> +                       ##args, __func__, __LINE__);            \
>  } while (0)
>
>  #ifndef BUG
>  #define BUG() do { \
> -       printf("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __FUNCTION__); \
> +       printf("BUG: failure at %s():%d!\n", __FUNCTION__, __LINE__); \
>         panic("BUG!"); \
>  } while (0)
>  #define BUG_ON(condition) do { if (unlikely((condition)!=0)) BUG(); } while(0)
> diff --git a/include/image.h b/include/image.h
> index 237251896029..a52c5355376e 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -1218,12 +1218,12 @@ static inline int fit_image_check_target_arch(const void *fdt, int node)
>  #ifdef CONFIG_FIT_VERBOSE
>  #define fit_unsupported(msg)   printf("! %s:%d " \
>                                 "FIT images not supported for '%s'\n", \
> -                               __FILE__, __LINE__, (msg))
> +                               __func__, __LINE__, (msg))
>
>  #define fit_unsupported_reset(msg)     printf("! %s:%d " \
>                                 "FIT images not supported for '%s' " \
>                                 "- must reset board to recover!\n", \
> -                               __FILE__, __LINE__, (msg))
> +                               __func__, __LINE__, (msg))
>  #else
>  #define fit_unsupported(msg)
>  #define fit_unsupported_reset(msg)
> diff --git a/include/linux/compat.h b/include/linux/compat.h
> index a43e4d66983d..db9fcb6d47c2 100644
> --- a/include/linux/compat.h
> +++ b/include/linux/compat.h
> @@ -100,14 +100,14 @@ static inline void kmem_cache_destroy(struct kmem_cache *cachep)
>
>  #ifndef BUG
>  #define BUG() do { \
> -       printf("U-Boot BUG at %s:%d!\n", __FILE__, __LINE__); \
> +       printf("U-Boot BUG at %s:%d!\n", __func__, __LINE__); \
>  } while (0)
>
>  #define BUG_ON(condition) do { if (condition) BUG(); } while(0)
>  #endif /* BUG */
>
>  #define WARN_ON(x) if (x) {printf("WARNING in %s line %d\n" \
> -                                 , __FILE__, __LINE__); }
> +                                 , __func__, __LINE__); }
>
>  #define PAGE_SIZE      4096
>
> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
> index 6def8f98aa41..b620463174d7 100644
> --- a/lib/tiny-printf.c
> +++ b/lib/tiny-printf.c
> @@ -228,11 +228,9 @@ int snprintf(char *buf, size_t size, const char *fmt, ...)
>         return ret;
>  }
>
> -void __assert_fail(const char *assertion, const char *file, unsigned line,
> -                  const char *function)
> +void __assert_fail(const char *assertion, unsigned line, const char *function)
>  {
>         /* This will not return */
> -       printf("%s:%u: %s: Assertion `%s' failed.", file, line, function,
> -              assertion);
> +       printf("%s:%u: Assertion `%s' failed.", function, line, assertion);
>         hang();
>  }
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 874a2951f705..d14cd67b3817 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -722,12 +722,10 @@ int vprintf(const char *fmt, va_list args)
>  }
>
>
> -void __assert_fail(const char *assertion, const char *file, unsigned line,
> -                  const char *function)
> +void __assert_fail(const char *assertion, unsigned line, const char *function)
>  {
>         /* This will not return */
> -       panic("%s:%u: %s: Assertion `%s' failed.", file, line, function,
> -             assertion);
> +       panic("%s:%u: Assertion `%s' failed.", function, line, assertion);
>  }
>
>  char *simple_itoa(ulong i)
>
> Why?  I'm not sure that in most cases __FILE__ is providing any more
> useful infomration on top of what we have from __func__ and __LINE__.
> That said, converting __assert_fail is probably not a good idea as it'll
> lead to cases (probably) where we don't have enough context to know
> where the problem really was, but with everything else being a macro it
> should evaluate out as useful as before at least.

I agree with this - the function name is normally more useful - it
provides immediate context as to what the error relates to, assuming
function names are sensible and functions are not too long.

 If we were to use a filename, it would be great if it could be
relative to the U-Boot source tree somehow.

Regards,
Simon

[U-Boot] __FILE__ usage and and SPL limits for SRAM

[U-Boot] FILE usage and and SPL limits for SRAM