| Message ID | 1308776689-28115-1-git-send-email-sjg@chromium.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Wednesday, June 22, 2011 17:04:49 Simon Glass wrote: > +/* > + * An assertion is run-time check done in debug mode only. If DEBUG is not > + * defined then it is skipped. It does not BUG or halt U-Boot, but tries > to + * continue execution in any case. 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 + * BUG-type reset (which may just do the same > assertion again). > + */ > +#define assert(x) \ > + ({ if (!(x)) printf("Assertion failure '%s' %s line %d\n", \ > + #x, __FILE__, __LINE__); }) > #else > #define debug(fmt,args...) > #define debugX(level,fmt,args...) > +#define assert(x) > #endif /* DEBUG */ the trouble with ifdef magic like this is that errors/warnings can be introduced when DEBUG isnt defined, and then only noticed when DEBUG is defined. so how about: #ifdef DEBUG # define _DEBUG 1 #else # define _DEBUG 2 #endif #define assert(x) \ do { \ if ((x) && _DEBUG) \ printf("%s:%s():%i: assertion failure: %s\n", \ __FILE__, __func__, __LINE__, #x); \ } while (0) (and yes, i know debug() and debugX() are also broken this way) -mike
Dear Mike Frysinger, In message <201106221723.27679.vapier@gentoo.org> you wrote: > > the trouble with ifdef magic like this is that errors/warnings can be=20 > introduced when DEBUG isnt defined, and then only noticed when DEBUG is=20 > defined. so how about: > > #ifdef DEBUG > # define _DEBUG 1 > #else > # define _DEBUG 2 1 and 2? You don't happen to mean 1 and 0 ? > #define assert(x) \ > do { \ > if ((x) && _DEBUG) \ > printf("%s:%s():%i: assertion failure: %s\n", \ > __FILE__, __func__, __LINE__, #x); \ > } while (0) This way the code will _always_ be compiled in. Best regards, Wolfgang Denk
On Wednesday, June 22, 2011 17:56:49 Wolfgang Denk wrote: > Mike Frysinger wrote: > > the trouble with ifdef magic like this is that errors/warnings can be=20 > > introduced when DEBUG isnt defined, and then only noticed when DEBUG > > is=20 defined. so how about: > > > > #ifdef DEBUG > > # define _DEBUG 1 > > #else > > # define _DEBUG 2 > > 1 and 2? You don't happen to mean 1 and 0 ? err, of course. i had something semi-related on my mind as i banged that out. -mike
On Thu, Jun 23, 2011 at 2:53 AM, Mike Frysinger <vapier@gentoo.org> wrote: > On Wednesday, June 22, 2011 17:04:49 Simon Glass wrote: >> +/* >> + * An assertion is run-time check done in debug mode only. If DEBUG is not >> + * defined then it is skipped. It does not BUG or halt U-Boot, but tries >> to + * continue execution in any case. 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 + * BUG-type reset (which may just do the same >> assertion again). >> + */ >> +#define assert(x) \ >> + ({ if (!(x)) printf("Assertion failure '%s' %s line %d\n", \ >> + #x, __FILE__, __LINE__); }) >> #else >> #define debug(fmt,args...) >> #define debugX(level,fmt,args...) >> +#define assert(x) >> #endif /* DEBUG */ > > the trouble with ifdef magic like this is that errors/warnings can be > introduced when DEBUG isnt defined, and then only noticed when DEBUG is > defined. so how about: Symbian OS, that had an array of defensive programming features, had two ASSERT macros: Something like ASSERT_DEBUG(x) and ASSERT_ALWAYS(x). ASSERT_DEBUG(x) could be used more liberally because it is compiled out in production image and ASSERT_ALWAYS(x) could be used in more critical run-time errors. My rule of thumb for using these two was this: 1. ASSERT_DEBUG(x) was used for invariant checking, that's for catching errors that could arise out of programming errors. This was used more liberally in the code. 2. ASSERT_ALWAYS(x) was used to catch erros due to invalid run-time parameters that one may not be able to catch during testing. best regards, Aneesh
On Wed, Jun 22, 2011 at 3:08 PM, Mike Frysinger <vapier@gentoo.org> wrote: > On Wednesday, June 22, 2011 17:56:49 Wolfgang Denk wrote: >> Mike Frysinger wrote: >> > the trouble with ifdef magic like this is that errors/warnings can be=20 >> > introduced when DEBUG isnt defined, and then only noticed when DEBUG >> > is=20 defined. so how about: >> > >> > #ifdef DEBUG >> > # define _DEBUG 1 >> > #else >> > # define _DEBUG 2 >> >> 1 and 2? You don't happen to mean 1 and 0 ? > > err, of course. i had something semi-related on my mind as i banged that out. Hi Mike & Wolfgang, That sounds better thanks, I will send a new patch. It would be good to have it compile the same code in and out of DEBUG mode. Regards, Simon > -mike > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot > >
On Wed, Jun 22, 2011 at 10:49 PM, V, Aneesh <aneesh@ti.com> wrote: > On Thu, Jun 23, 2011 at 2:53 AM, Mike Frysinger <vapier@gentoo.org> wrote: >> On Wednesday, June 22, 2011 17:04:49 Simon Glass wrote: >>> +/* >>> + * An assertion is run-time check done in debug mode only. If DEBUG is not >>> + * defined then it is skipped. It does not BUG or halt U-Boot, but tries >>> to + * continue execution in any case. 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 + * BUG-type reset (which may just do the same >>> assertion again). >>> + */ >>> +#define assert(x) \ >>> + ({ if (!(x)) printf("Assertion failure '%s' %s line %d\n", \ >>> + #x, __FILE__, __LINE__); }) >>> #else >>> #define debug(fmt,args...) >>> #define debugX(level,fmt,args...) >>> +#define assert(x) >>> #endif /* DEBUG */ >> >> the trouble with ifdef magic like this is that errors/warnings can be >> introduced when DEBUG isnt defined, and then only noticed when DEBUG is >> defined. so how about: Hi Aneesh, > > Symbian OS, that had an array of defensive programming features, had two > ASSERT macros: > > Something like ASSERT_DEBUG(x) and ASSERT_ALWAYS(x). Symbian OS can live on in U-Boot! > > ASSERT_DEBUG(x) could be used more liberally because it is compiled out in > production image and ASSERT_ALWAYS(x) could be used in more critical run-time > errors. > > My rule of thumb for using these two was this: > > 1. ASSERT_DEBUG(x) was used for invariant checking, that's for catching errors > that could arise out of programming errors. This was used more liberally in the > code. > 2. ASSERT_ALWAYS(x) was used to catch erros due to invalid run-time parameters > that one may not be able to catch during testing. With this patch we have: - assert: compiled out for production code, used for debug, like your ASSERT_DEBUG I think - BUG_ON: halt/reset even in production code, used for production code and critical faults where continued execution is certainly pointless or counterproductive. Like your ASSERT_ALWAYS I think. Regards, Simon > > best regards, > Aneesh > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
On Thursday, June 23, 2011 17:39:12 Simon Glass wrote: > - BUG_ON: halt/reset even in production code, used for production code > and critical faults where continued execution is certainly pointless > or counterproductive. Like your ASSERT_ALWAYS I think. if we're going to go this route, then we should just take the API linux already has. that means WARN/WARN_ON/BUG/BUG_ON. we've got more in common with linux than we'll ever have with symbian. -mike
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..f2f1326 100644 --- a/include/common.h +++ b/include/common.h @@ -119,9 +119,22 @@ typedef volatile unsigned char vu_char; #ifdef DEBUG #define debug(fmt,args...) printf (fmt ,##args) #define debugX(level,fmt,args...) if (DEBUG>=level) printf(fmt,##args); + +/* + * An assertion is run-time check done in debug mode only. If DEBUG is not + * defined then it is skipped. It does not BUG or halt U-Boot, but tries to + * continue execution in any case. 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 + * BUG-type reset (which may just do the same assertion again). + */ +#define assert(x) \ + ({ if (!(x)) printf("Assertion failure '%s' %s line %d\n", \ + #x, __FILE__, __LINE__); }) #else #define debug(fmt,args...) #define debugX(level,fmt,args...) +#define assert(x) #endif /* DEBUG */ #define error(fmt, args...) do { \ 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,
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> --- common/dlmalloc.c | 7 ------- include/common.h | 13 +++++++++++++ include/malloc.h | 8 -------- lib/qsort.c | 5 ----- 4 files changed, 13 insertions(+), 20 deletions(-)