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.

Hi Tom,

On 4 April 2017 at 13:06, Tom Rini trini@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