[U-Boot] __FILE__ usage and and SPL limits for SRAM
Masahiro Yamada
yamada.masahiro at socionext.com
Sat Apr 22 05:30:09 UTC 2017
2017-04-10 4:27 GMT+09:00 Simon Glass <sjg at chromium.org>:
> 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
I can do this, but
I'd like to move forward synced with Linux.
Yesterday, I took some time to write patches
and sent them to Linux ML.
Plan A:
https://lkml.org/lkml/2017/4/21/623
https://patchwork.kernel.org/patch/9693559/
https://patchwork.kernel.org/patch/9693563/
Plan B:
https://patchwork.kernel.org/patch/9693623/
Let's see how it goes.
--
Best Regards
Masahiro Yamada
More information about the U-Boot
mailing list