[U-Boot] [PATCH v3] Add assert() for debug assertions

assert() is like BUG_ON() but compiles to nothing unless DEBUG is defined. This is useful when a condition is an error but a board reset is unlikely to fix it, so it is better to soldier on in hope. Assertion failures should be caught during development/test.
It turns out that assert() is defined separately in a few places in U-Boot with various meanings. This patch cleans up some of these.
Build errors exposed by this change (and defining DEBUG) are also fixed in this patch.
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v2: - Changed macros so that all code is compiled even if DEBUG is disabled
Changes in v3: - Use panic() instead of printf() - Use separate __assert_fail() function to reduce memory footprint - Changed output format to match assert(3)
common/dlmalloc.c | 7 ------- include/common.h | 21 +++++++++++++++++++++ include/malloc.h | 8 -------- lib/qsort.c | 5 ----- lib/vsprintf.c | 8 ++++++++ 5 files changed, 29 insertions(+), 20 deletions(-)
diff --git a/common/dlmalloc.c b/common/dlmalloc.c index e9bab09..f2080c6 100644 --- a/common/dlmalloc.c +++ b/common/dlmalloc.c @@ -286,13 +286,6 @@ extern "C" {
*/
-#ifdef DEBUG -#include <assert.h> -#else -#define assert(x) ((void)0) -#endif - - /* INTERNAL_SIZE_T is the word-size used for internal bookkeeping of chunk sizes. On a 64-bit machine, you can reduce malloc diff --git a/include/common.h b/include/common.h index 1e21b7a..0956b93 100644 --- a/include/common.h +++ b/include/common.h @@ -124,6 +124,27 @@ typedef volatile unsigned char vu_char; #define debugX(level,fmt,args...) #endif /* DEBUG */
+#ifdef DEBUG +# define _DEBUG 1 +#else +# define _DEBUG 0 +#endif + +/* + * An assertion is run-time check done in debug mode only. If DEBUG is not + * defined then it is skipped. If DEBUG is defined and the assertion fails, + * then it calls panic*( which may or may not reset/halt U-Boot (see + * CONFIG_PANIC_HANG), It is hoped that all failing assertions are found + * before release, and after release it is hoped that they don't matter. But + * 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); +#define assert(x) \ + ({ if (!(x) && _DEBUG) \ + __assert_fail(#x, __FILE__, __LINE__, __func__); }) + #define error(fmt, args...) do { \ printf("ERROR: " fmt "\nat %s:%d/%s()\n", \ ##args, __FILE__, __LINE__, __func__); \ diff --git a/include/malloc.h b/include/malloc.h index 3e145ad..ecf3c67 100644 --- a/include/malloc.h +++ b/include/malloc.h @@ -285,14 +285,6 @@ extern "C" {
*/
-#ifdef DEBUG -/* #include <assert.h> */ -#define assert(x) ((void)0) -#else -#define assert(x) ((void)0) -#endif - - /* INTERNAL_SIZE_T is the word-size used for internal bookkeeping of chunk sizes. On a 64-bit machine, you can reduce malloc diff --git a/lib/qsort.c b/lib/qsort.c index 1cc0d31..86c392c 100644 --- a/lib/qsort.c +++ b/lib/qsort.c @@ -17,11 +17,6 @@
#include <linux/types.h> #include <exports.h> -#if 0 -#include <assert.h> -#else -#define assert(arg) -#endif
void qsort(void *base, size_t nel, diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 3b924ec..1f01b70 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -728,3 +728,11 @@ void panic(const char *fmt, ...) do_reset (NULL, 0, 0, NULL); #endif } + +void __assert_fail(const char *assertion, const char *file, unsigned line, + const char *function) +{ + /* This will not return */ + panic("%s:%u: %s: Assertion `%s' failed.", file, line, function, + assertion); +}

Dear Mike Frysinger,
In message 201106291643.32477.vapier@gentoo.org you wrote:
do we really care about that when people are using #define DEBUG ? i'd say this is an unnecessary indirection ...
Yes, we do care about the memory footprint - no matter whether with or without DEBUG.
Best regards,
Wolfgang Denk

On Wednesday, June 29, 2011 17:01:23 Wolfgang Denk wrote:
Mike Frysinger wrote:
do we really care about that when people are using #define DEBUG ? i'd say this is an unnecessary indirection ...
Yes, we do care about the memory footprint - no matter whether with or without DEBUG.
there's caring about it, and there's nitpicking over ~10 bytes per call site. -mike

Did this go anywhere? I will resend... - Simon
On Wed, Jun 29, 2011 at 2:54 PM, Mike Frysinger vapier@gentoo.org wrote:
On Wednesday, June 29, 2011 17:01:23 Wolfgang Denk wrote:
Mike Frysinger wrote:
do we really care about that when people are using #define DEBUG ? i'd say this is an unnecessary indirection ...
Yes, we do care about the memory footprint - no matter whether with or without DEBUG.
there's caring about it, and there's nitpicking over ~10 bytes per call site. -mike

On Wednesday, September 07, 2011 18:29:16 Simon Glass wrote:
On Wed, Jun 29, 2011 at 2:54 PM, Mike Frysinger vapier@gentoo.org wrote:
On Wednesday, June 29, 2011 17:01:23 Wolfgang Denk wrote:
Mike Frysinger wrote:
do we really care about that when people are using #define DEBUG ? i'd say this is an unnecessary indirection ...
Yes, we do care about the memory footprint - no matter whether with or without DEBUG.
there's caring about it, and there's nitpicking over ~10 bytes per call site.
Did this go anywhere? I will resend...
i dont care enough to fight wolfgang over it -mike

Hi Mike / Wolfgang,
On Thu, Sep 8, 2011 at 5:34 PM, Mike Frysinger vapier@gentoo.org wrote:
On Wednesday, September 07, 2011 18:29:16 Simon Glass wrote:
On Wed, Jun 29, 2011 at 2:54 PM, Mike Frysinger vapier@gentoo.org wrote:
On Wednesday, June 29, 2011 17:01:23 Wolfgang Denk wrote:
Mike Frysinger wrote:
do we really care about that when people are using #define DEBUG ? i'd say this is an unnecessary indirection ...
Yes, we do care about the memory footprint - no matter whether with or without DEBUG.
there's caring about it, and there's nitpicking over ~10 bytes per call site.
Did this go anywhere? I will resend...
i dont care enough to fight wolfgang over it
OK. Any other comments on this Wolfgang or is this finished?
Regards, Simon
-mike

Dear Simon Glass,
In message 1309376974-12943-1-git-send-email-sjg@chromium.org you wrote:
assert() is like BUG_ON() but compiles to nothing unless DEBUG is defined. This is useful when a condition is an error but a board reset is unlikely to fix it, so it is better to soldier on in hope. Assertion failures should be caught during development/test.
It turns out that assert() is defined separately in a few places in U-Boot with various meanings. This patch cleans up some of these.
Build errors exposed by this change (and defining DEBUG) are also fixed in this patch.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Changed macros so that all code is compiled even if DEBUG is disabled
Changes in v3:
- Use panic() instead of printf()
- Use separate __assert_fail() function to reduce memory footprint
- Changed output format to match assert(3)
common/dlmalloc.c | 7 ------- include/common.h | 21 +++++++++++++++++++++ include/malloc.h | 8 -------- lib/qsort.c | 5 ----- lib/vsprintf.c | 8 ++++++++ 5 files changed, 29 insertions(+), 20 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk

Commit 21726a7 "Add assert() for debug assertions" caused build warnings for many systems:
In file included from bedbug.c:6: /home/wd/git/u-boot/work/include/bedbug/bedbug.h:24:1: warning: "assert" redefined In file included from bedbug.c:3: /home/wd/git/u-boot/work/include/common.h:144:1: warning: this is the location of the previous definition In file included from cmd_bedbug.c:10: /home/wd/git/u-boot/work/include/bedbug/bedbug.h:24:1: warning: "assert" redefined In file included from cmd_bedbug.c:5: /home/wd/git/u-boot/work/include/common.h:144:1: warning: this is the location of the previous definition
Signed-off-by: Wolfgang Denk wd@denx.de Cc: Simon Glass sjg@chromium.org --- include/bedbug/bedbug.h | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/include/bedbug/bedbug.h b/include/bedbug/bedbug.h index 471215e..0c5d687 100644 --- a/include/bedbug/bedbug.h +++ b/include/bedbug/bedbug.h @@ -21,8 +21,6 @@ #endif #endif
-#define assert( condition ) if( (condition) ) _exit(0) - #endif /* _BEDBUG_H */

Hi Wolfgang,
On Fri, Sep 9, 2011 at 3:38 PM, Wolfgang Denk wd@denx.de wrote:
Commit 21726a7 "Add assert() for debug assertions" caused build warnings for many systems:
In file included from bedbug.c:6: /home/wd/git/u-boot/work/include/bedbug/bedbug.h:24:1: warning: "assert" redefined In file included from bedbug.c:3: /home/wd/git/u-boot/work/include/common.h:144:1: warning: this is the location of the previous definition In file included from cmd_bedbug.c:10: /home/wd/git/u-boot/work/include/bedbug/bedbug.h:24:1: warning: "assert" redefined In file included from cmd_bedbug.c:5: /home/wd/git/u-boot/work/include/common.h:144:1: warning: this is the location of the previous definition
I was wondering about that one :-) Thank you. I also ignored ubifs since it seemed to have its own system, and one in yaffs which was commented out. If it helps:
Acked-by: Simon Glass sjg@chromium.org
Regards, Simon
Signed-off-by: Wolfgang Denk wd@denx.de Cc: Simon Glass sjg@chromium.org
include/bedbug/bedbug.h | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/include/bedbug/bedbug.h b/include/bedbug/bedbug.h index 471215e..0c5d687 100644 --- a/include/bedbug/bedbug.h +++ b/include/bedbug/bedbug.h @@ -21,8 +21,6 @@ #endif #endif
-#define assert( condition ) if( (condition) ) _exit(0)
#endif /* _BEDBUG_H */
-- 1.7.6

Dear Wolfgang Denk,
In message 1315607922-7285-1-git-send-email-wd@denx.de you wrote:
Commit 21726a7 "Add assert() for debug assertions" caused build warnings for many systems:
In file included from bedbug.c:6: /home/wd/git/u-boot/work/include/bedbug/bedbug.h:24:1: warning: "assert" redefined In file included from bedbug.c:3: /home/wd/git/u-boot/work/include/common.h:144:1: warning: this is the location of the previous definition In file included from cmd_bedbug.c:10: /home/wd/git/u-boot/work/include/bedbug/bedbug.h:24:1: warning: "assert" redefined In file included from cmd_bedbug.c:5: /home/wd/git/u-boot/work/include/common.h:144:1: warning: this is the location of the previous definition
Signed-off-by: Wolfgang Denk wd@denx.de Cc: Simon Glass sjg@chromium.org
include/bedbug/bedbug.h | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk

Commit 21726a7 "Add assert() for debug assertions" broke building the utx8245 board:
dlmalloc.c: In function 'do_check_chunk': dlmalloc.c:1660: error: 'sz' undeclared (first use in this function) dlmalloc.c:1660: error: (Each undeclared identifier is reported only once dlmalloc.c:1660: error: for each function it appears in.) dlmalloc.c: In function 'do_check_free_chunk': dlmalloc.c:1689: error: 'next' undeclared (first use in this function) dlmalloc.c: In function 'do_check_malloced_chunk': dlmalloc.c:1748: error: 'sz' undeclared (first use in this function) dlmalloc.c:1750: error: 'room' undeclared (first use in this function)
Signed-off-by: Wolfgang Denk wd@denx.de Cc: Simon Glass sjg@chromium.org --- common/dlmalloc.c | 6 ------ 1 files changed, 0 insertions(+), 6 deletions(-)
diff --git a/common/dlmalloc.c b/common/dlmalloc.c index f2080c6..c645d73 100644 --- a/common/dlmalloc.c +++ b/common/dlmalloc.c @@ -1647,9 +1647,7 @@ static void do_check_chunk(mchunkptr p) static void do_check_chunk(p) mchunkptr p; #endif { -#if 0 /* causes warnings because assert() is off */ INTERNAL_SIZE_T sz = p->size & ~PREV_INUSE; -#endif /* 0 */
/* No checkable chunk is mmapped */ assert(!chunk_is_mmapped(p)); @@ -1671,9 +1669,7 @@ static void do_check_free_chunk(p) mchunkptr p; #endif { INTERNAL_SIZE_T sz = p->size & ~PREV_INUSE; -#if 0 /* causes warnings because assert() is off */ mchunkptr next = chunk_at_offset(p, sz); -#endif /* 0 */
do_check_chunk(p);
@@ -1737,10 +1733,8 @@ static void do_check_malloced_chunk(mchunkptr p, INTERNAL_SIZE_T s) static void do_check_malloced_chunk(p, s) mchunkptr p; INTERNAL_SIZE_T s; #endif { -#if 0 /* causes warnings because assert() is off */ INTERNAL_SIZE_T sz = p->size & ~PREV_INUSE; long room = sz - s; -#endif /* 0 */
do_check_inuse_chunk(p);

Dear Wolfgang Denk,
In message 1315663665-24926-1-git-send-email-wd@denx.de you wrote:
Commit 21726a7 "Add assert() for debug assertions" broke building the utx8245 board:
dlmalloc.c: In function 'do_check_chunk': dlmalloc.c:1660: error: 'sz' undeclared (first use in this function) dlmalloc.c:1660: error: (Each undeclared identifier is reported only once dlmalloc.c:1660: error: for each function it appears in.) dlmalloc.c: In function 'do_check_free_chunk': dlmalloc.c:1689: error: 'next' undeclared (first use in this function) dlmalloc.c: In function 'do_check_malloced_chunk': dlmalloc.c:1748: error: 'sz' undeclared (first use in this function) dlmalloc.c:1750: error: 'room' undeclared (first use in this function)
Signed-off-by: Wolfgang Denk wd@denx.de Cc: Simon Glass sjg@chromium.org
common/dlmalloc.c | 6 ------ 1 files changed, 0 insertions(+), 6 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk

Commit 21726a7 "Add assert() for debug assertions" caused build warnings for all tegra2 based boards:
clock.c:36:1: warning: "assert" redefined In file included from clock.c:29: include/common.h:144:1: warning: this is the location of the previous definition
Signed-off-by: Wolfgang Denk wd@denx.de Cc: Simon Glass sjg@chromium.org --- arch/arm/cpu/armv7/tegra2/clock.c | 8 -------- 1 files changed, 0 insertions(+), 8 deletions(-)
diff --git a/arch/arm/cpu/armv7/tegra2/clock.c b/arch/arm/cpu/armv7/tegra2/clock.c index 67eed14..0aaed7d 100644 --- a/arch/arm/cpu/armv7/tegra2/clock.c +++ b/arch/arm/cpu/armv7/tegra2/clock.c @@ -28,14 +28,6 @@ #include <asm/arch/tegra2.h> #include <common.h>
-#ifdef DEBUG -#define assert(x) \ - ({ if (!(x)) printf("Assertion failure '%s' %s line %d\n", \ - #x, __FILE__, __LINE__); }) -#else -#define assert(x) -#endif - /* * Get the oscillator frequency, from the corresponding hardware configuration * field.

Dear Wolfgang Denk,
In message 1315664363-3979-1-git-send-email-wd@denx.de you wrote:
Commit 21726a7 "Add assert() for debug assertions" caused build warnings for all tegra2 based boards:
clock.c:36:1: warning: "assert" redefined In file included from clock.c:29: include/common.h:144:1: warning: this is the location of the previous definition
Signed-off-by: Wolfgang Denk wd@denx.de Cc: Simon Glass sjg@chromium.org
arch/arm/cpu/armv7/tegra2/clock.c | 8 -------- 1 files changed, 0 insertions(+), 8 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk

Hi Wolfgang,
On Sat, Sep 10, 2011 at 1:44 PM, Wolfgang Denk wd@denx.de wrote:
Dear Wolfgang Denk,
In message 1315664363-3979-1-git-send-email-wd@denx.de you wrote:
Commit 21726a7 "Add assert() for debug assertions" caused build warnings for all tegra2 based boards:
clock.c:36:1: warning: "assert" redefined In file included from clock.c:29: include/common.h:144:1: warning: this is the location of the previous definition
Thanks again. The patch set I just sent out modifies the assert() in this file, but I will remove it in V2.
Regards, Simon
Signed-off-by: Wolfgang Denk wd@denx.de Cc: Simon Glass sjg@chromium.org
arch/arm/cpu/armv7/tegra2/clock.c | 8 -------- 1 files changed, 0 insertions(+), 8 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de "More software projects have gone awry for lack of calendar time than for all other causes combined." - Fred Brooks, Jr., _The Mythical Man Month_
participants (3)
-
Mike Frysinger
-
Simon Glass
-
Wolfgang Denk