[RFC] replace assorted ASSERT()s by something officially sanctioned

[RFC] replace assorted ASSERT()s by something officially sanctioned

[Dean Nelson]

It doesn't appear that an officially 'sanctioned' version of ASSERT() or
an ASSERT()-like macro exists.

And by the proliferation of its use in the linux 2.6 kernel (I saw over
3000 references to it), it would seem that BUG_ON() does not satisfy all
of the requirements of the community.

One problem with BUG_ON() is that it is always enabled. And even though
the compiler does a good job of minimizing the impact of the conditional
expression, there are times when the conditional check requires the
accessing of a cacheline that would not get accessed had the BUG_ON() not
been enabled. And if that cacheline were one that is hotly contended for,
one's performance can be adversely affected.

For debugging purposes it would be nice to have a version of BUG_ON() that
was only enabled if DEBUG was set. This is what appears to be behind the use
of the ASSERT()-like macros.

As an example of what I have in mind, I've included the following quilt
patch.

Thanks,
Dean


Index: linux/include/asm-i386/bug.h
===================================================================
--- linux.orig/include/asm-i386/bug.h
+++ linux/include/asm-i386/bug.h
@@ -21,6 +21,12 @@
 
 #define BUG_ON(condition) do { if (unlikely((condition)!=0)) BUG(); } while(0)
 
+#ifdef DEBUG
+#define DBUG_ON(condition)	BUG_ON(condition)
+#else
+#define DBUG_ON(condition)
+#endif
+
 #define PAGE_BUG(page) do { \
 	BUG(); \
 } while (0)

Re: [RFC] replace assorted ASSERT()s by something officially sanctioned

[Michael Buesch]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> +#ifdef DEBUG
> +#define DBUG_ON(condition)	BUG_ON(condition)
> +#else
> +#define DBUG_ON(condition)
> +#endif

As condition is lost when DEBUG is not defined, what about that:

#else
# define DBUG_ON(condition)	do { if (condition) { /* nothing */ } } while (0)
#endif

letting the compiler optimize away all the stuff and removing
the risk of loosing an expression in ( ).

- --
Regards Michael Buesch  [ http://www.tuxsoft.de.vu ]


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFA2dPAFGK1OIvVOP4RAuPrAJ9juu+dZLSt1QjsMQeko82n9OgLqgCeN0TQ
Fec6PgrBWBRwLxl6U65QVmw=
=l7+o
-----END PGP SIGNATURE-----

Re: [RFC] replace assorted ASSERT()s by something officially sanctioned

[Jeff Garzik]

Dean Nelson wrote:
> It doesn't appear that an officially 'sanctioned' version of ASSERT() or
> an ASSERT()-like macro exists.
> 
> And by the proliferation of its use in the linux 2.6 kernel (I saw over
> 3000 references to it), it would seem that BUG_ON() does not satisfy all
> of the requirements of the community.
> 
> One problem with BUG_ON() is that it is always enabled. And even though
> the compiler does a good job of minimizing the impact of the conditional
> expression, there are times when the conditional check requires the
> accessing of a cacheline that would not get accessed had the BUG_ON() not
> been enabled. And if that cacheline were one that is hotly contended for,
> one's performance can be adversely affected.
> 
> For debugging purposes it would be nice to have a version of BUG_ON() that
> was only enabled if DEBUG was set. This is what appears to be behind the use
> of the ASSERT()-like macros.
> 
> As an example of what I have in mind, I've included the following quilt
> patch.
> 
> Thanks,
> Dean
> 
> 
> Index: linux/include/asm-i386/bug.h
> ===================================================================
> --- linux.orig/include/asm-i386/bug.h
> +++ linux/include/asm-i386/bug.h
> @@ -21,6 +21,12 @@
>  
>  #define BUG_ON(condition) do { if (unlikely((condition)!=0)) BUG(); } while(0)
>  
> +#ifdef DEBUG
> +#define DBUG_ON(condition)	BUG_ON(condition)
> +#else
> +#define DBUG_ON(condition)
> +#endif

This won't work as it often needs to be driver-granular.  Also, 
WARN_ON() is often the closer implementation of assert(), than BUG_ON()

	Jeff

Re: [RFC] replace assorted ASSERT()s by something officially sanctioned

[Dean Nelson]

On Wed, Jun 23, 2004 at 03:46:36PM -0400, Jeff Garzik wrote:
> Dean Nelson wrote:
> >--- linux.orig/include/asm-i386/bug.h
> >+++ linux/include/asm-i386/bug.h
> >@@ -21,6 +21,12 @@
> >
> > #define BUG_ON(condition) do { if (unlikely((condition)!=0)) BUG(); }
> > while(0)
> >
> >+#ifdef DEBUG
> >+#define DBUG_ON(condition)	BUG_ON(condition)
> >+#else
> >+#define DBUG_ON(condition)
> >+#endif
>
> This won't work as it often needs to be driver-granular.  Also,

I agree that most often the granularity of debugging needs to be at the driver
level. But I was just taking my lead from dev_dbg(), which uses '#ifdef DEBUG'
in the same way I've proposed. I would argue that the enabling/disabling of
dev_dbg() is also something one would want to control at the driver level. I'm
assuming people do this by adding a '#define DEBUG' prior to the driver's
#include of <linux/kernel.h> which has a #include of <asm/bug.h>.
 
The dev_printk() family of macros is a good example of what I'm interested in.
They have a set of macros (dev_err(), dev_warn(), dev_info()) that are always
enabled. Then there is dev_dbg() which is only enabled if DEBUG is defined.
(I'm not defending the choice of DEBUG as the enabling switch. I'm merely
interested in having the ability to enable/disable BUG_ON() when the driver
is built.)


> WARN_ON() is often the closer implementation of assert(), than BUG_ON()

I would agree that there are two basic flavors of ASSERT()-like macros; one
that printks some info and then Oops the system, and the other that simply
printks some info. So I would suggest providing both options, something like:
 
    #ifdef DEBUG
    #define DBUG_ON(condition)  BUG_ON(condition)
    #define DWARN_ON(condition) WARN_ON(condition)
    #else
    #define DBUG_ON(condition)  do { if (condition) { /* nothing */ } } while (0)
    #define DWARN_ON(condition) do { if (condition) { /* nothing */ } } while (0)
    #endif
 
(I really don't care what the names of these macros end up being. Again, I'm
just interested in a BUG_ON()/WARN_ON()-like mechanism that is conditionally
compiled in (enabled) by some type of #ifdef switch (like DEBUG) and is
'sanctioned' by the community for use by drivers.)
 
Thanks,
Dean