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

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

[Simon Glass]

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 bd10f31..6268460 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 c029fbb..79dead3 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -730,3 +730,11 @@ void panic(const char *fmt, ...)
 	while (1)
 		;
 }
+
+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);
+}
-- 
1.7.3.1

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

[Wolfgang Denk]

Dear Simon Glass,

In message <1315434680-21641-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>
...
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -730,3 +730,11 @@ void panic(const char *fmt, ...)
>  	while (1)
>  		;
>  }
> +
> +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);
> +}

Can you please #ifdef it so it doesn't get added for the non-debug
case, too?

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 at denx.de
The human mind  ordinarily  operates  at  only  ten  percent  of  its
capacity. The rest is overhead for the operating system.

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

[Simon Glass]

Hi Wolfgang,

On Fri, Oct 21, 2011 at 3:20 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <1315434680-21641-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>
> ...
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -730,3 +730,11 @@ void panic(const char *fmt, ...)
>> ? ? ? while (1)
>> ? ? ? ? ? ? ? ;
>> ?}
>> +
>> +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);
>> +}
>
> Can you please #ifdef it so it doesn't get added for the non-debug
> case, too?

Well I could, but then if someone defines DEBUG in a file they will
get a link error I think. Or are you thinking of adding a new CONFIG
which *permits* people to use DEBUG?

Regards,
Simon

>
> 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 at denx.de
> The human mind ?ordinarily ?operates ?at ?only ?ten ?percent ?of ?its
> capacity. The rest is overhead for the operating system.
>

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

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ1OYiqTT0Y+mWnkdh7ZZWruXy5n0atB6e4CSjuQkQcvKw@mail.gmail.com> you wrote:
>
> > Can you please #ifdef it so it doesn't get added for the non-debug
> > case, too?
> 
> Well I could, but then if someone defines DEBUG in a file they will
> get a link error I think. Or are you thinking of adding a new CONFIG
> which *permits* people to use DEBUG?

I don't understand what you mean.

__assert_fail() only ever gets used (and thus needs to be compiled in)
if DEBUG is defined, right?  SO why cannot you wrap the code in a
"#ifdef DEBUG" ?

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 at denx.de
"There's only one kind of woman ..." "Or man, for  that  matter.  You
either believe in yourself or you don't."
	-- Kirk and Harry Mudd, "Mudd's Women", stardate 1330.1

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

[Mike Frysinger]

On Sun, Oct 23, 2011 at 04:35, Wolfgang Denk wrote:
> Simon Glass wrote:
>> > Can you please #ifdef it so it doesn't get added for the non-debug
>> > case, too?
>>
>> Well I could, but then if someone defines DEBUG in a file they will
>> get a link error I think. Or are you thinking of adding a new CONFIG
>> which *permits* people to use DEBUG?
>
> I don't understand what you mean.
>
> __assert_fail() only ever gets used (and thus needs to be compiled in)
> if DEBUG is defined, right? ?SO why cannot you wrap the code in a
> "#ifdef DEBUG" ?

people often enable DEBUG on a per file basis.  so if i'm debugging
drivers/spi/bfin_spi.c, i put "#define DEBUG" at the top of it.
-mike

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

[Wolfgang Denk]

Dear Mike Frysinger,

In message <CAJaTeTq_=k+1KwM3SZCoJ-zdm0_KRh9r5arp8Bbej+OJJ7BYag@mail.gmail.com> you wrote:
>
> > __assert_fail() only ever gets used (and thus needs to be compiled in)
> > if DEBUG is defined, right? =A0SO why cannot you wrap the code in a
> > "#ifdef DEBUG" ?
> 
> people often enable DEBUG on a per file basis.  so if i'm debugging
> drivers/spi/bfin_spi.c, i put "#define DEBUG" at the top of it.

Right you are. I missed that.

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 at denx.de
Landing: a controlled mid-air collision with a planet.

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

[Mike Frysinger]

On Fri, Oct 21, 2011 at 18:20, Wolfgang Denk wrote:
> Simon Glass wrote:
>> +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);
>> +}
>
> Can you please #ifdef it so it doesn't get added for the non-debug
> case, too?

most arches are building with --gc-sections now which means there is
no overhead added for them.  should we look at fixing the few holdouts
rather than adding #ifdefs ?
-mike

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

[Wolfgang Denk]

Dear Mike Frysinger,

In message <CAJaTeTrUJH+p1jTU88d5KDv4EU57DiU+=_ibp9DZHXU78U1HdQ@mail.gmail.com> you wrote:
>
> >> +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);
> >> +}
> >
> > Can you please #ifdef it so it doesn't get added for the non-debug
> > case, too?
>
> most arches are building with --gc-sections now which means there is
> no overhead added for them.  should we look at fixing the few holdouts
> rather than adding #ifdefs ?

I think Power does use --gc-sections, still I see code size growing.

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 at denx.de
All your people must learn before you can reach for the stars.
	-- Kirk, "The Gamesters of Triskelion", stardate 3259.2