[PATCH 1/7] Add conditional oopsing with annotation

[PATCH 1/7] Add conditional oopsing with annotation

[David Howells]

Add the ability to create an annotated oops report.  This is useful for
displaying the output of assertion failures where direct display of the values
being checked is of greater value than the register dump.

This could technically be done simply by issuing one or more printk() calls
followed by a BUG() but in practice this has a serious disadvantage in that
people reporting a bug usually seem to take the "cut here" line literally and
discard everything prior to it when making a report - thus eliminating the most
important bit of information after the file/line number.

There are number of possible solutions to this.  I've used the last in this
list:

 (1) Emit the "cut here" line early, suppressing the one produced by the BUG()
     handler.  This would allow the annotation to be formed of multiple
     printk() calls.

 (2) Get rid of the "cut here" line entirely.

 (3) Pass the annotation through to the exception handler.  For practical
     reasons, this limits the number of annotations to a single format string
     and parameters.  This means that a va_list has to be passed through and
     thence to vprintk() - which should be okay.  It also requires arch support
     to retrieve the annotation data.


This facility can be made use of by one of:

	ANNOTATED_BUG(const char *fmt, ...);
	ANNOTATED_BUG_ON(bool condition, const char *fmt, ...);

This prints a report that looks like:

	------------[ cut here ]------------
	kernel BUG at fs/dcache.c:863!
	invalid opcode: 0000 [#1] SMP
	...

if fmt is NULL and:

	------------[ cut here ]------------
	kernel BUG at fs/dcache.c:863!
	Dentry 0xffff880032675ed8{i=242,n=Documents} still in use (1) [unmount of nfs 12:01]
	invalid opcode: 0000 [#1] SMP
	...

otherwise.

For this to work the arch code must provide two things:

	#define arch_annotated_bug(struct annotated_bug *desc)

to perform the oops and:

	#define is_arch_annotated_bug(struct pt_regs *regs)

for report_bug() to find whether or not an annotated bug occurred and, if so,
return a pointer to its description as passed to arch_annotated_bug().

If arch_annotated_bug() is not defined, then the code will fall back to doing
a printk() and a BUG().

Signed-off-by: David Howells <dhowells@redhat.com>
---

 arch/x86/include/asm/bug.h |   14 ++++++++++++++
 include/asm-generic/bug.h  |   32 ++++++++++++++++++++++++++++++++
 include/linux/bug.h        |   15 +++++++++++++++
 kernel/panic.c             |   35 +++++++++++++++++++++++++++++++++++
 lib/bug.c                  |   14 ++++++++++++++
 5 files changed, 110 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
index f654d1b..d6c109d 100644
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -33,6 +33,20 @@ do {								\
 } while (0)
 #endif
 
+extern const void __arch_annotated_bug;
+#define arch_annotated_bug(desc)				\
+	do {							\
+		asm volatile(".globl __arch_annotated_bug\n"	\
+			     "__arch_annotated_bug:\n"		\
+			     "	ud2\n"				\
+			     : : "d" (desc));			\
+		unreachable();					\
+	} while (0)
+
+#define is_arch_annotated_bug(regs)					\
+	({ ((const void *)regs->ip == &__arch_annotated_bug) ?		\
+			(struct annotated_bug *)regs->dx : NULL; })
+
 #endif /* !CONFIG_BUG */
 
 #include <asm-generic/bug.h>
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 84458b0..05db3fe 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -2,6 +2,7 @@
 #define _ASM_GENERIC_BUG_H
 
 #include <linux/compiler.h>
+#include <stdarg.h>
 
 #ifdef CONFIG_BUG
 
@@ -202,4 +203,35 @@ extern void warn_slowpath_null(const char *file, const int line);
 # define WARN_ON_SMP(x)			({0;})
 #endif
 
+/**
+ * ANNOTATED_BUG - Give annotated oops
+ * FMT: The annotation format (as printk)
+ */
+#ifdef CONFIG_BUG
+
+#define ANNOTATED_BUG(FMT, ...)						\
+	do {								\
+		annotated_bug(__FILE__, __LINE__, FMT, ## __VA_ARGS__); \
+	} while (0)
+
+#else
+
+#define ANNOTATED_BUG(FMT, ...)		\
+	do {					\
+		no_printk(FMT, ## __VA_ARGS__);	\
+	} while (0)
+
 #endif
+
+/**
+ * ANNOTATED_BUG_ON - Give annotated oops if the given expression is not true
+ * X: The expression to check
+ * FMT: The annotation format (as printk)
+ */
+#define ANNOTATED_BUG_ON(X, FMT, ...)				\
+	do {							\
+		if (unlikely(!(X)))				\
+			ANNOTATED_BUG(FMT, ## __VA_ARGS__);	\
+	} while (0)
+
+#endif /* _ASM_GENERIC_BUG_H */
diff --git a/include/linux/bug.h b/include/linux/bug.h
index d276b55..6498861 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -35,4 +35,19 @@ static inline enum bug_trap_type report_bug(unsigned long bug_addr,
 }
 
 #endif	/* CONFIG_GENERIC_BUG */
+
+
+/*
+ * Data for annotated bug (assertion failure) report.
+ */
+struct annotated_bug {
+	const char	*file;
+	const char	*fmt;
+	va_list		args;
+	int		line;
+};
+
+extern void annotated_bug(const char *file, int line, const char *fmt, ...)
+	__attribute__ ((format (printf, 3, 4)));
+
 #endif	/* _LINUX_BUG_H */
diff --git a/kernel/panic.c b/kernel/panic.c
index b2659360..91e22c5 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -23,6 +23,7 @@
 #include <linux/init.h>
 #include <linux/nmi.h>
 #include <linux/dmi.h>
+#include <linux/bug.h>
 
 #define PANIC_TIMER_STEP 100
 #define PANIC_BLINK_SPD 18
@@ -447,3 +448,37 @@ static int __init oops_setup(char *s)
 	return 0;
 }
 early_param("oops", oops_setup);
+
+/*
+ * Default annotated bug handler.
+ */
+#ifndef arch_annotated_bug
+static void arch_annotated_bug(struct annotated_bug *bug)
+{
+	printk(KERN_EMERG "------------[ cut here ]------------\n");
+	printk(KERN_CRIT "kernel BUG at %s:%u!\n", bug->file, bug->line);
+	if (bug->fmt)
+		vprintk(bug->fmt, bug->args);
+
+	/* We don't really want to call BUG() here as it'll emit another "cut
+	 * here" line, so the arches really need to override this.
+	 */
+	BUG();
+}
+#endif
+
+/*
+ * Display assertion failure and exit with SIGSEGV.
+ */
+void annotated_bug(const char *file, int line, const char *fmt, ...)
+{
+	struct annotated_bug bug = {
+		.file	= file,
+		.line	= line,
+		.fmt	= fmt,
+	};
+
+	va_start(bug.args, fmt);
+	arch_annotated_bug(&bug);
+}
+EXPORT_SYMBOL(annotated_bug);
diff --git a/lib/bug.c b/lib/bug.c
index 1955209..f551fe5 100644
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -126,12 +126,26 @@ const struct bug_entry *find_bug(unsigned long bugaddr)
 enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs)
 {
 	const struct bug_entry *bug;
+	struct annotated_bug *anno;
 	const char *file;
 	unsigned line, warning;
 
 	if (!is_valid_bugaddr(bugaddr))
 		return BUG_TRAP_TYPE_NONE;
 
+#ifdef arch_annotated_bug
+	anno = is_arch_annotated_bug(regs);
+	if (anno) {
+		printk(KERN_EMERG "------------[ cut here ]------------\n");
+
+		printk(KERN_CRIT "kernel BUG at %s:%u!\n", anno->file, anno->line);
+		if (anno->fmt)
+			vprintk(anno->fmt, anno->args);
+
+		return BUG_TRAP_TYPE_BUG;
+	}
+#endif
+
 	bug = find_bug(bugaddr);
 
 	file = NULL;

[PATCH 2/7] Make shrink_dcache_for_umount_subtree() use ANNOTATED_BUG()

[David Howells]

Make shrink_dcache_for_umount_subtree() use ANNOTATED_BUG() rather than doing
printk() then BUG() so that the annotation appears _after_ the "cut here" line
instead of before it.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/dcache.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 89509b5..4083c50 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -37,6 +37,7 @@
 #include <linux/rculist_bl.h>
 #include <linux/prefetch.h>
 #include <linux/ratelimit.h>
+#include <linux/bug.h>
 #include "internal.h"
 
 /*
@@ -889,9 +890,9 @@ static void shrink_dcache_for_umount_subtree(struct dentry *dentry)
 			dentry_lru_prune(dentry);
 			__d_shrink(dentry);
 
-			if (dentry->d_count != 0) {
-				printk(KERN_ERR
-				       "BUG: Dentry %p{i=%lx,n=%s}"
+			if (unlikely(dentry->d_count != 0))
+				ANNOTATED_BUG(
+				       "Dentry %p{i=%lx,n=%s}"
 				       " still in use (%d)"
 				       " [unmount of %s %s]\n",
 				       dentry,
@@ -901,8 +902,6 @@ static void shrink_dcache_for_umount_subtree(struct dentry *dentry)
 				       dentry->d_count,
 				       dentry->d_sb->s_type->name,
 				       dentry->d_sb->s_id);
-				BUG();
-			}
 
 			if (IS_ROOT(dentry)) {
 				parent = NULL;

[PATCH 3/7] Add assertion checking macros

[David Howells]

Add a range of ASSERT* macros to linux/assert.h for performing runtime
assertions.  These will use ANNOTATED_BUG() to create an annotated oops if the
check fails.

All but the first macro will display the values and condition(s) involved in
the failed check as these are very useful for working out the cause of the
problem and may not necessarily be easily determinable from the register and
stack dumps.

The checks are only enabled under two circumstances:

 (1) CONFIG_DEBUG_ENABLE_ASSERTIONS=y

 (2) ENABLE_ASSERTIONS is defined prior to the #inclusion of <linux/assert.h>

There are five macros provided:

 (a) ASSERT(X)

     Issue an assertion failure error if X is false.  In other words, require
     the expression X to be true.  For example:

	ASSERT(val != 0);

     There is no need to display val here in the case the expression fails
     since it can only be 0.  If this fails, it produces an error like the
     following:

	------------[ cut here ]------------
	kernel BUG at fs/fscache/main.c:109!
	ASSERTION FAILED
	invalid opcode: 0000 [#1] SMP

 (b) ASSERTCMP(X, OP, Y)

     Issue an assertion failure error if the expression X OP Y is false.  For
     example:

	ASSERTCMP(x, >, 12)

     If an oops is produced, then the values of X and Y will be displayed in
     hex, along with OP:

	------------[ cut here ]------------
	kernel BUG at fs/fscache/main.c:109!
	ASSERTION FAILED: 2 > c is false
	invalid opcode: 0000 [#1] SMP

 (c) ASSERTRANGE(X, OP, Y, OP2, Z)

     Issue an assertion failure error if the expression X OP Y or if the
     expression Y OP2 Z is false.  Typically OP and OP2 would be < or <=,
     looking something like:

	ASSERTRANGE(11, <, x, <=, 13);

     and giving the following error:

	------------[ cut here ]------------
	kernel BUG at fs/fscache/main.c:109!
	ASSERTION FAILED: b < 2 <= d is false
	invalid opcode: 0000 [#1] SMP

and for compactness, where an assertion should only apply under certain
circumstances:

 (d) ASSERTIF(C, X)

     If condition C is true, issue an assertion failure error if X is false.
     For example:

	ASSERTIF(test_bit(FSCACHE_OP_EXCLUSIVE, &op->flags),
		 object->n_exclusive != 0);

 (e) ASSERTIFCMP(C, X, OP, Y)

     This is a combination of ASSERTIF and ASSERTCMP.  For example:

	ASSERTIFCMP(test_bit(FSCACHE_OP_EXCLUSIVE, &op->flags),
		    object->n_exclusive, >, 0);

Signed-off-by: David Howells <dhowells@redhat.com>
---

 include/linux/assert.h |  111 ++++++++++++++++++++++++++++++++++++++++++++++++
 lib/Kconfig.debug      |    8 +++
 2 files changed, 119 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/assert.h

diff --git a/include/linux/assert.h b/include/linux/assert.h
new file mode 100644
index 0000000..e1e9264
--- /dev/null
+++ b/include/linux/assert.h
@@ -0,0 +1,111 @@
+/* Assertion checking
+ *
+ * Copyright (C) 2011 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public Licence
+ * as published by the Free Software Foundation; either version
+ * 2 of the Licence, or (at your option) any later version.
+ */
+
+#ifndef _LINUX_ASSERT_H
+#define _LINUX_ASSERT_H
+
+#include <linux/bug.h>
+
+/*
+ * ENABLE_ASSERTIONS can be set by an individual module to override the global
+ * setting and turn assertions on for just that module.
+ */
+#if defined(CONFIG_DEBUG_ENABLE_ASSERTIONS) || defined(ENABLE_ASSERTIONS)
+
+#define cond_assertion_failed(FMT, ...)					\
+	do {								\
+		ANNOTATED_BUG("ASSERTION FAILED" FMT, ## __VA_ARGS__); \
+	} while (0)
+
+#else
+
+#define cond_assertion_failed(FMT, ...)		\
+	do {					\
+		no_printk("ASSERTION FAILED" FMT, ## __VA_ARGS__);	\
+	} while (0)
+
+#endif
+
+/**
+ * ASSERT - Oops if the given expression is not true
+ * X: The expression to check
+ */
+#define ASSERT(X)							\
+do {									\
+	if (unlikely(!(X)))						\
+		cond_assertion_failed("");				\
+} while (0)
+
+/**
+ * ASSERTCMP - Oops if the specified check fails
+ * X: The value to check
+ * OP: The operator to use for comparison
+ * Y: The value to compare against
+ *
+ * The two values are displayed in the oops report if the assertion fails.
+ */
+#define ASSERTCMP(X, OP, Y)						\
+do {									\
+	if (unlikely(!((X) OP (Y))))					\
+		cond_assertion_failed(": %lx " #OP " %lx is false\n", \
+				      (unsigned long)(X),		\
+				      (unsigned long)(Y));		\
+} while (0)
+
+/**
+ * ASSERTIF - If condition is true, oops if the given expression is not true
+ * C: The condition under which to perform the check
+ * X: The expression to check
+ */
+#define ASSERTIF(C, X)							\
+do {									\
+	if (unlikely((C) && !(X)))					\
+		cond_assertion_failed("");				\
+} while (0)
+
+/**
+ * ASSERTIFCMP - If condition is true, oops if the specified check fails
+ * C: The condition under which to perform the check
+ * X: The value to check
+ * OP: The operator to use for comparison
+ * Y: The value to compare against
+ *
+ * The two values are displayed in the oops report if the assertion fails.
+ */
+#define ASSERTIFCMP(C, X, OP, Y)					\
+do {									\
+	if (unlikely((C) && !((X) OP (Y))))				\
+		cond_assertion_failed(": %lx " #OP " %lx is false\n",	\
+				      (unsigned long)(X),		\
+				      (unsigned long)(Y));		\
+} while (0)
+
+/**
+ * ASSERTCMP - Oops if the value is outside of the specified range
+ * X: The lower bound
+ * OP: The operator to use to check against the lower bound (< or <=)
+ * Y: The value to check
+ * OP2: The operator to use to check against the upper bound (< or <=)
+ * Z: The upper bound
+ *
+ * The three values are displayed in the oops report if the assertion fails.
+ */
+#define ASSERTRANGE(X, OP, Y, OP2, Z)					\
+do {									\
+	if (unlikely(!((X) OP (Y)) || !((Y) OP2 (Z))))			\
+		cond_assertion_failed(": %lx " #OP " %lx " #OP2		\
+				      " %lx is false\n",		\
+				      (unsigned long)(X),		\
+				      (unsigned long)(Y),		\
+				      (unsigned long)(Z));		\
+} while(0)
+
+#endif /* _LINUX_ASSERT_H */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 82928f5..b14acf9 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -149,6 +149,14 @@ config DEBUG_KERNEL
 	  Say Y here if you are developing drivers or trying to debug and
 	  identify kernel problems.
 
+config DEBUG_ENABLE_ASSERTIONS
+	bool "Enable assertion checks"
+	depends on BUG
+	help
+	  Say Y here to globally enable checks made by the ASSERT*() macros.
+	  If such a check fails, BUG() processing will be invoked and an
+	  annotated oops will be emitted.
+
 config DEBUG_SHIRQ
 	bool "Debug shared IRQ handlers"
 	depends on DEBUG_KERNEL && GENERIC_HARDIRQS

[PATCH 4/7] FS-Cache: Use new core assertion macros

[David Howells]

Make FS-Cache use the new core assertion macros in place of its own.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/fscache/debug.h    |   82 ++++++++++++++++++++++++++++++++
 fs/fscache/internal.h |  126 +------------------------------------------------
 2 files changed, 84 insertions(+), 124 deletions(-)
 create mode 100644 fs/fscache/debug.h

diff --git a/fs/fscache/debug.h b/fs/fscache/debug.h
new file mode 100644
index 0000000..fcdbbab
--- /dev/null
+++ b/fs/fscache/debug.h
@@ -0,0 +1,82 @@
+/* Debug tracing
+ *
+ * Copyright (C) 2004-2011 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public Licence
+ * as published by the Free Software Foundation; either version
+ * 2 of the Licence, or (at your option) any later version.
+ */
+
+#include <linux/printk.h>
+#define ENABLE_ASSERTIONS
+#include <linux/assert.h>
+
+/*
+ * debug tracing
+ */
+#define dbgprintk(FMT, ...) \
+	printk(KERN_DEBUG "[%-6.6s] "FMT"\n", current->comm, ##__VA_ARGS__)
+
+#define kenter(FMT, ...) dbgprintk("==> %s("FMT")", __func__, ##__VA_ARGS__)
+#define kleave(FMT, ...) dbgprintk("<== %s()"FMT"", __func__, ##__VA_ARGS__)
+#define kdebug(FMT, ...) dbgprintk(FMT, ##__VA_ARGS__)
+
+#define kjournal(FMT, ...) no_printk(FMT, ##__VA_ARGS__)
+
+#ifdef __KDEBUG
+#define _enter(FMT, ...) kenter(FMT, ##__VA_ARGS__)
+#define _leave(FMT, ...) kleave(FMT, ##__VA_ARGS__)
+#define _debug(FMT, ...) kdebug(FMT, ##__VA_ARGS__)
+
+#elif defined(CONFIG_FSCACHE_DEBUG)
+#define _enter(FMT, ...)			\
+do {						\
+	if (__do_kdebug(ENTER))			\
+		kenter(FMT, ##__VA_ARGS__);	\
+} while (0)
+
+#define _leave(FMT, ...)			\
+do {						\
+	if (__do_kdebug(LEAVE))			\
+		kleave(FMT, ##__VA_ARGS__);	\
+} while (0)
+
+#define _debug(FMT, ...)			\
+do {						\
+	if (__do_kdebug(DEBUG))			\
+		kdebug(FMT, ##__VA_ARGS__);	\
+} while (0)
+
+#else
+#define _enter(FMT, ...) no_printk("==> %s("FMT")", __func__, ##__VA_ARGS__)
+#define _leave(FMT, ...) no_printk("<== %s()"FMT"", __func__, ##__VA_ARGS__)
+#define _debug(FMT, ...) no_printk(FMT, ##__VA_ARGS__)
+#endif
+
+/*
+ * determine whether a particular optional debugging point should be logged
+ * - we need to go through three steps to persuade cpp to correctly join the
+ *   shorthand in FSCACHE_DEBUG_LEVEL with its prefix
+ */
+#define ____do_kdebug(LEVEL, POINT) \
+	unlikely((fscache_debug & \
+		  (FSCACHE_POINT_##POINT << (FSCACHE_DEBUG_ ## LEVEL * 3))))
+#define ___do_kdebug(LEVEL, POINT) \
+	____do_kdebug(LEVEL, POINT)
+#define __do_kdebug(POINT) \
+	___do_kdebug(FSCACHE_DEBUG_LEVEL, POINT)
+
+#define FSCACHE_DEBUG_CACHE	0
+#define FSCACHE_DEBUG_COOKIE	1
+#define FSCACHE_DEBUG_PAGE	2
+#define FSCACHE_DEBUG_OPERATION	3
+
+#define FSCACHE_POINT_ENTER	1
+#define FSCACHE_POINT_LEAVE	2
+#define FSCACHE_POINT_DEBUG	4
+
+#ifndef FSCACHE_DEBUG_LEVEL
+#define FSCACHE_DEBUG_LEVEL CACHE
+#endif
diff --git a/fs/fscache/internal.h b/fs/fscache/internal.h
index f6aad48..4be65c9 100644
--- a/fs/fscache/internal.h
+++ b/fs/fscache/internal.h
@@ -24,6 +24,7 @@
 
 #include <linux/fscache-cache.h>
 #include <linux/sched.h>
+#include "debug.h"
 
 #define FSCACHE_MIN_THREADS	4
 #define FSCACHE_MAX_THREADS	32
@@ -288,7 +289,7 @@ static inline void fscache_raise_event(struct fscache_object *object,
  */
 static inline void fscache_cookie_put(struct fscache_cookie *cookie)
 {
-	BUG_ON(atomic_read(&cookie->usage) <= 0);
+	ASSERTCMP(atomic_read(&cookie->usage), >, 0);
 	if (atomic_dec_and_test(&cookie->usage))
 		__fscache_cookie_put(cookie);
 }
@@ -313,126 +314,3 @@ void fscache_put_context(struct fscache_cookie *cookie, void *context)
 	if (cookie->def->put_context)
 		cookie->def->put_context(cookie->netfs_data, context);
 }
-
-/*****************************************************************************/
-/*
- * debug tracing
- */
-#define dbgprintk(FMT, ...) \
-	printk(KERN_DEBUG "[%-6.6s] "FMT"\n", current->comm, ##__VA_ARGS__)
-
-#define kenter(FMT, ...) dbgprintk("==> %s("FMT")", __func__, ##__VA_ARGS__)
-#define kleave(FMT, ...) dbgprintk("<== %s()"FMT"", __func__, ##__VA_ARGS__)
-#define kdebug(FMT, ...) dbgprintk(FMT, ##__VA_ARGS__)
-
-#define kjournal(FMT, ...) no_printk(FMT, ##__VA_ARGS__)
-
-#ifdef __KDEBUG
-#define _enter(FMT, ...) kenter(FMT, ##__VA_ARGS__)
-#define _leave(FMT, ...) kleave(FMT, ##__VA_ARGS__)
-#define _debug(FMT, ...) kdebug(FMT, ##__VA_ARGS__)
-
-#elif defined(CONFIG_FSCACHE_DEBUG)
-#define _enter(FMT, ...)			\
-do {						\
-	if (__do_kdebug(ENTER))			\
-		kenter(FMT, ##__VA_ARGS__);	\
-} while (0)
-
-#define _leave(FMT, ...)			\
-do {						\
-	if (__do_kdebug(LEAVE))			\
-		kleave(FMT, ##__VA_ARGS__);	\
-} while (0)
-
-#define _debug(FMT, ...)			\
-do {						\
-	if (__do_kdebug(DEBUG))			\
-		kdebug(FMT, ##__VA_ARGS__);	\
-} while (0)
-
-#else
-#define _enter(FMT, ...) no_printk("==> %s("FMT")", __func__, ##__VA_ARGS__)
-#define _leave(FMT, ...) no_printk("<== %s()"FMT"", __func__, ##__VA_ARGS__)
-#define _debug(FMT, ...) no_printk(FMT, ##__VA_ARGS__)
-#endif
-
-/*
- * determine whether a particular optional debugging point should be logged
- * - we need to go through three steps to persuade cpp to correctly join the
- *   shorthand in FSCACHE_DEBUG_LEVEL with its prefix
- */
-#define ____do_kdebug(LEVEL, POINT) \
-	unlikely((fscache_debug & \
-		  (FSCACHE_POINT_##POINT << (FSCACHE_DEBUG_ ## LEVEL * 3))))
-#define ___do_kdebug(LEVEL, POINT) \
-	____do_kdebug(LEVEL, POINT)
-#define __do_kdebug(POINT) \
-	___do_kdebug(FSCACHE_DEBUG_LEVEL, POINT)
-
-#define FSCACHE_DEBUG_CACHE	0
-#define FSCACHE_DEBUG_COOKIE	1
-#define FSCACHE_DEBUG_PAGE	2
-#define FSCACHE_DEBUG_OPERATION	3
-
-#define FSCACHE_POINT_ENTER	1
-#define FSCACHE_POINT_LEAVE	2
-#define FSCACHE_POINT_DEBUG	4
-
-#ifndef FSCACHE_DEBUG_LEVEL
-#define FSCACHE_DEBUG_LEVEL CACHE
-#endif
-
-/*
- * assertions
- */
-#if 1 /* defined(__KDEBUGALL) */
-
-#define ASSERT(X)							\
-do {									\
-	if (unlikely(!(X))) {						\
-		printk(KERN_ERR "\n");					\
-		printk(KERN_ERR "FS-Cache: Assertion failed\n");	\
-		BUG();							\
-	}								\
-} while (0)
-
-#define ASSERTCMP(X, OP, Y)						\
-do {									\
-	if (unlikely(!((X) OP (Y)))) {					\
-		printk(KERN_ERR "\n");					\
-		printk(KERN_ERR "FS-Cache: Assertion failed\n");	\
-		printk(KERN_ERR "%lx " #OP " %lx is false\n",		\
-		       (unsigned long)(X), (unsigned long)(Y));		\
-		BUG();							\
-	}								\
-} while (0)
-
-#define ASSERTIF(C, X)							\
-do {									\
-	if (unlikely((C) && !(X))) {					\
-		printk(KERN_ERR "\n");					\
-		printk(KERN_ERR "FS-Cache: Assertion failed\n");	\
-		BUG();							\
-	}								\
-} while (0)
-
-#define ASSERTIFCMP(C, X, OP, Y)					\
-do {									\
-	if (unlikely((C) && !((X) OP (Y)))) {				\
-		printk(KERN_ERR "\n");					\
-		printk(KERN_ERR "FS-Cache: Assertion failed\n");	\
-		printk(KERN_ERR "%lx " #OP " %lx is false\n",		\
-		       (unsigned long)(X), (unsigned long)(Y));		\
-		BUG();							\
-	}								\
-} while (0)
-
-#else
-
-#define ASSERT(X)			do {} while (0)
-#define ASSERTCMP(X, OP, Y)		do {} while (0)
-#define ASSERTIF(C, X)			do {} while (0)
-#define ASSERTIFCMP(C, X, OP, Y)	do {} while (0)
-
-#endif /* assert or not */

[PATCH 5/7] CacheFiles: Use new core assertion macros

[David Howells]

Make CacheFiles use the new core assertion macros in place of its own.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/cachefiles/internal.h |   53 ++--------------------------------------------
 1 files changed, 2 insertions(+), 51 deletions(-)

diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
index bd6bc1b..fd88085 100644
--- a/fs/cachefiles/internal.h
+++ b/fs/cachefiles/internal.h
@@ -14,6 +14,8 @@
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 #include <linux/security.h>
+#define ENABLE_ASSERTIONS
+#include <linux/assert.h>
 
 struct cachefiles_cache;
 struct cachefiles_object;
@@ -301,54 +303,3 @@ do {							\
 #define _leave(FMT, ...) no_printk("<== %s()"FMT"", __func__, ##__VA_ARGS__)
 #define _debug(FMT, ...) no_printk(FMT, ##__VA_ARGS__)
 #endif
-
-#if 1 /* defined(__KDEBUGALL) */
-
-#define ASSERT(X)							\
-do {									\
-	if (unlikely(!(X))) {						\
-		printk(KERN_ERR "\n");					\
-		printk(KERN_ERR "CacheFiles: Assertion failed\n");	\
-		BUG();							\
-	}								\
-} while (0)
-
-#define ASSERTCMP(X, OP, Y)						\
-do {									\
-	if (unlikely(!((X) OP (Y)))) {					\
-		printk(KERN_ERR "\n");					\
-		printk(KERN_ERR "CacheFiles: Assertion failed\n");	\
-		printk(KERN_ERR "%lx " #OP " %lx is false\n",		\
-		       (unsigned long)(X), (unsigned long)(Y));		\
-		BUG();							\
-	}								\
-} while (0)
-
-#define ASSERTIF(C, X)							\
-do {									\
-	if (unlikely((C) && !(X))) {					\
-		printk(KERN_ERR "\n");					\
-		printk(KERN_ERR "CacheFiles: Assertion failed\n");	\
-		BUG();							\
-	}								\
-} while (0)
-
-#define ASSERTIFCMP(C, X, OP, Y)					\
-do {									\
-	if (unlikely((C) && !((X) OP (Y)))) {				\
-		printk(KERN_ERR "\n");					\
-		printk(KERN_ERR "CacheFiles: Assertion failed\n");	\
-		printk(KERN_ERR "%lx " #OP " %lx is false\n",		\
-		       (unsigned long)(X), (unsigned long)(Y));		\
-		BUG();							\
-	}								\
-} while (0)
-
-#else
-
-#define ASSERT(X)			do {} while (0)
-#define ASSERTCMP(X, OP, Y)		do {} while (0)
-#define ASSERTIF(C, X)			do {} while (0)
-#define ASSERTIFCMP(C, X, OP, Y)	do {} while (0)
-
-#endif

[PATCH 6/7] AFS: Use new core assertion macros

[David Howells]

Make AFS use the new core assertion macros in place of its own.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/afs/internal.h |   90 +----------------------------------------------------
 1 files changed, 2 insertions(+), 88 deletions(-)

diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index d2b0888..36989c0 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -20,6 +20,8 @@
 #include <linux/sched.h>
 #include <linux/fscache.h>
 #include <linux/backing-dev.h>
+#define ENABLE_ASSERTIONS
+#include <linux/assert.h>
 
 #include "afs.h"
 #include "afs_vl.h"
@@ -800,91 +802,3 @@ do {							\
 #define _leave(FMT,...)	no_printk("<== %s()"FMT"",__func__ ,##__VA_ARGS__)
 #define _debug(FMT,...)	no_printk("    "FMT ,##__VA_ARGS__)
 #endif
-
-/*
- * debug assertion checking
- */
-#if 1 // defined(__KDEBUGALL)
-
-#define ASSERT(X)						\
-do {								\
-	if (unlikely(!(X))) {					\
-		printk(KERN_ERR "\n");				\
-		printk(KERN_ERR "AFS: Assertion failed\n");	\
-		BUG();						\
-	}							\
-} while(0)
-
-#define ASSERTCMP(X, OP, Y)						\
-do {									\
-	if (unlikely(!((X) OP (Y)))) {					\
-		printk(KERN_ERR "\n");					\
-		printk(KERN_ERR "AFS: Assertion failed\n");		\
-		printk(KERN_ERR "%lu " #OP " %lu is false\n",		\
-		       (unsigned long)(X), (unsigned long)(Y));		\
-		printk(KERN_ERR "0x%lx " #OP " 0x%lx is false\n",	\
-		       (unsigned long)(X), (unsigned long)(Y));		\
-		BUG();							\
-	}								\
-} while(0)
-
-#define ASSERTRANGE(L, OP1, N, OP2, H)					\
-do {									\
-	if (unlikely(!((L) OP1 (N)) || !((N) OP2 (H)))) {		\
-		printk(KERN_ERR "\n");					\
-		printk(KERN_ERR "AFS: Assertion failed\n");		\
-		printk(KERN_ERR "%lu "#OP1" %lu "#OP2" %lu is false\n",	\
-		       (unsigned long)(L), (unsigned long)(N),		\
-		       (unsigned long)(H));				\
-		printk(KERN_ERR "0x%lx "#OP1" 0x%lx "#OP2" 0x%lx is false\n", \
-		       (unsigned long)(L), (unsigned long)(N),		\
-		       (unsigned long)(H));				\
-		BUG();							\
-	}								\
-} while(0)
-
-#define ASSERTIF(C, X)						\
-do {								\
-	if (unlikely((C) && !(X))) {				\
-		printk(KERN_ERR "\n");				\
-		printk(KERN_ERR "AFS: Assertion failed\n");	\
-		BUG();						\
-	}							\
-} while(0)
-
-#define ASSERTIFCMP(C, X, OP, Y)					\
-do {									\
-	if (unlikely((C) && !((X) OP (Y)))) {				\
-		printk(KERN_ERR "\n");					\
-		printk(KERN_ERR "AFS: Assertion failed\n");		\
-		printk(KERN_ERR "%lu " #OP " %lu is false\n",		\
-		       (unsigned long)(X), (unsigned long)(Y));		\
-		printk(KERN_ERR "0x%lx " #OP " 0x%lx is false\n",	\
-		       (unsigned long)(X), (unsigned long)(Y));		\
-		BUG();							\
-	}								\
-} while(0)
-
-#else
-
-#define ASSERT(X)				\
-do {						\
-} while(0)
-
-#define ASSERTCMP(X, OP, Y)			\
-do {						\
-} while(0)
-
-#define ASSERTRANGE(L, OP1, N, OP2, H)		\
-do {						\
-} while(0)
-
-#define ASSERTIF(C, X)				\
-do {						\
-} while(0)
-
-#define ASSERTIFCMP(C, X, OP, Y)		\
-do {						\
-} while(0)
-
-#endif /* __KDEBUGALL */

[PATCH 7/7] RxRPC: Use new core assertion macros

[David Howells]

Make RxRPC use the new core assertion macros in place of its own.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 net/rxrpc/ar-internal.h |   71 +----------------------------------------------
 1 files changed, 2 insertions(+), 69 deletions(-)

diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index 8e22bd3..b86fe78 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -10,6 +10,8 @@
  */
 
 #include <rxrpc/packet.h>
+#define ENABLE_ASSERTIONS
+#include <linux/assert.h>
 
 #if 0
 #define CHECK_SLAB_OKAY(X)				     \
@@ -657,75 +659,6 @@ do {							\
 #endif
 
 /*
- * debug assertion checking
- */
-#if 1 // defined(__KDEBUGALL)
-
-#define ASSERT(X)						\
-do {								\
-	if (unlikely(!(X))) {					\
-		printk(KERN_ERR "\n");				\
-		printk(KERN_ERR "RxRPC: Assertion failed\n");	\
-		BUG();						\
-	}							\
-} while(0)
-
-#define ASSERTCMP(X, OP, Y)						\
-do {									\
-	if (unlikely(!((X) OP (Y)))) {					\
-		printk(KERN_ERR "\n");					\
-		printk(KERN_ERR "RxRPC: Assertion failed\n");		\
-		printk(KERN_ERR "%lu " #OP " %lu is false\n",		\
-		       (unsigned long)(X), (unsigned long)(Y));		\
-		printk(KERN_ERR "0x%lx " #OP " 0x%lx is false\n",	\
-		       (unsigned long)(X), (unsigned long)(Y));		\
-		BUG();							\
-	}								\
-} while(0)
-
-#define ASSERTIF(C, X)						\
-do {								\
-	if (unlikely((C) && !(X))) {				\
-		printk(KERN_ERR "\n");				\
-		printk(KERN_ERR "RxRPC: Assertion failed\n");	\
-		BUG();						\
-	}							\
-} while(0)
-
-#define ASSERTIFCMP(C, X, OP, Y)					\
-do {									\
-	if (unlikely((C) && !((X) OP (Y)))) {				\
-		printk(KERN_ERR "\n");					\
-		printk(KERN_ERR "RxRPC: Assertion failed\n");		\
-		printk(KERN_ERR "%lu " #OP " %lu is false\n",		\
-		       (unsigned long)(X), (unsigned long)(Y));		\
-		printk(KERN_ERR "0x%lx " #OP " 0x%lx is false\n",	\
-		       (unsigned long)(X), (unsigned long)(Y));		\
-		BUG();							\
-	}								\
-} while(0)
-
-#else
-
-#define ASSERT(X)				\
-do {						\
-} while(0)
-
-#define ASSERTCMP(X, OP, Y)			\
-do {						\
-} while(0)
-
-#define ASSERTIF(C, X)				\
-do {						\
-} while(0)
-
-#define ASSERTIFCMP(C, X, OP, Y)		\
-do {						\
-} while(0)
-
-#endif /* __KDEBUGALL */
-
-/*
  * socket buffer accounting / leak finding
  */
 static inline void __rxrpc_new_skb(struct sk_buff *skb, const char *fn)

Re: [PATCH 3/7] Add assertion checking macros

[Andi Kleen]

David Howells <dhowells@redhat.com> writes:
>
>      There is no need to display val here in the case the expression fails
>      since it can only be 0.  If this fails, it produces an error like the
>      following:
>
> 	------------[ cut here ]------------
> 	kernel BUG at fs/fscache/main.c:109!
> 	ASSERTION FAILED

It would be nice to display the expression here like an user space
assert. While it can be looked up in the source it would
make quick eyeballing easier. 

Probably wouldn't cost too much additional text size?

>
>  (e) ASSERTIFCMP(C, X, OP, Y)
>
>      This is a combination of ASSERTIF and ASSERTCMP.  For example:
>
> 	ASSERTIFCMP(test_bit(FSCACHE_OP_EXCLUSIVE, &op->flags),
> 		    object->n_exclusive, >, 0);

You probably want a checkpatch rule with those to only allow a few
selected functions like test_bit. Otherwise people will put real side
effects in there.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH 1/7] Add conditional oopsing with annotation

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1477 bytes --]

On Friday 16 December 2011 09:13:57 David Howells wrote:
> --- a/arch/x86/include/asm/bug.h
> +++ b/arch/x86/include/asm/bug.h
> 
> +extern const void __arch_annotated_bug;
> +#define arch_annotated_bug(desc)				\
> +	do {							\
> +		asm volatile(".globl __arch_annotated_bug\n"	\
> +			     "__arch_annotated_bug:\n"		\
> +			     "	ud2\n"				\
> +			     : : "d" (desc));			\
> +		unreachable();					\
> +	} while (0)
> +
> +#define is_arch_annotated_bug(regs)					\
> +	({ ((const void *)regs->ip == &__arch_annotated_bug) ?		\
> +			(struct annotated_bug *)regs->dx : NULL; })

seems like this is making arches duplicate a bit of logic they shouldn't have 
to ... you could have asm-generic/bug.h do:

#ifdef arch_annotated_bug_fail

extern const void __arch_annotated_bug;
# define ARCH_ANNOTATED_SYM_STR CONFIG_SYMBOL_PREFIX"__arch_annotated_bug"

# define arch_annotated_bug(desc) \
	do { \
		asm volatile(".globl " ARCH_ANNOTATED_SYM ";\n" \
			ARCH_ANNOTATED_SYM_STR":\n"); \
		arch_annotated_bug_fail(desc) ; \
		unreachable(); \
	} while (0)

# define is_arch_annotated_bug(regs) \
	({ (instruction_pointer(regs) == &__arch_annotated_bug) ? \
		(struct annotated_bug *)arch_annotated_bug_struct(regs) : \
		NULL; })

#endif

so now arches have to define two small bits.  in the x86 case:
#define arch_annotated_bug_fail(desc) asm volatile("ud2;" : : "d" (desc))
#define arch_annotated_bug_struct(regs) (regs)->dx
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/7] Add conditional oopsing with annotation

[Ingo Molnar]

* David Howells <dhowells@redhat.com> wrote:

> This facility can be made use of by one of:
> 
> 	ANNOTATED_BUG(const char *fmt, ...);
> 	ANNOTATED_BUG_ON(bool condition, const char *fmt, ...);

Hm, what about WARN_ON()s?

WARN_ON() and WARN_ON_ONCE() covers like 99% of the *actual* 
bugreports we get: we are actively getting rid of BUG()s that do 
trigger and are asking all new patches to come with WARN_ON()s.

BUG()s are generally a poor way of reporting bugs.

Another, much bigger issue is the actual syntax:

> -       BUG_ON(atomic_read(&cookie->usage) <= 0);
> +       ASSERTCMP(atomic_read(&cookie->usage), >, 0);

NAK on that concept on two grounds!

1) BUG_ON() is a well-known pattern. Changing it to the inverted
   assert() braindamage is going to cause confusion years down
   the road. Years ago we've settled on using BUG*() and WARN*()
   assertions to include conditions that check the 'bad'
   condition'. assert() covers the negated 'good' condition -
   which works but the two are truly awful when mixed.

2) The '>,0' syntax is ugly.

Why don't we simply extend the *existing* primitives with a 
'verbose' variant that saves the text string of the macro using 
the '#param' syntax, intead of modifying the usage sites with a 
pointless splitting along logical ops?

Doing that would also remove the rather pointess ANNOTATED_() 
prefix, which is like totally uninteresting in the actual usage 
sites. WARN()s want to be as short, obvious and low-profile as 
possible.

So the whole schizophrenic split between BUG_ON()/WARN_ON() and 
the assert() world is nonsensical and confusing - we should 
settle on *one* logical variant to make code reading easier. And 
i thought in the kernel we already settled on one of these 
variants and are using it almost exclusively ...

So this series does not look good enough to me yet.

Thanks,

	Ingo

Re: [PATCH 1/7] Add conditional oopsing with annotation

[David Howells]

Ingo Molnar <mingo@elte.hu> wrote:

> Hm, what about WARN_ON()s?

Good point.

> BUG()s are generally a poor way of reporting bugs.

Sometimes you can't continue, and there's no way you can know how to clean up
- after all, if you see values you don't expect, how can you handle them?

> 1) BUG_ON() is a well-known pattern. Changing it to the inverted
>    assert() braindamage is going to cause confusion years down
>    the road.

In my opinion, BUG_ON() is usually inverted from what it should be.
Frequently, it seems to be bugging on an inverted condition.

assert() is generally well known from userspace.  You lay out your
expectations with asserts.  An assert says "I expect this value to comply with
this condition" - such as a value being in a specific range.  Generally I
would turn them on whilst debugging stuff, and suppressing them at other
times, but I'd leave BUG[_ON]() and WARN[_ON]() enabled.

> 2) The '>,0' syntax is ugly.

And works rather well.  I've been using it for a while now in the kernel quite
successfully.

> Why don't we simply extend the *existing* primitives with a 
> 'verbose' variant that saves the text string of the macro using 
> the '#param' syntax, intead of modifying the usage sites with a 
> pointless splitting along logical ops?

You've completely missed the point.  Go back and re-read the description.

> Doing that would also remove the rather pointess ANNOTATED_() 
> prefix, which is like totally uninteresting in the actual usage 
> sites. WARN()s want to be as short, obvious and low-profile as 
> possible.

You need to distinguish annotated errors/warnings from non-annotated ones...
unless you're planning on completely converting all the BUG/WARN calls in one
patch?

You could call it BUG_ON_VERBOSE() - but verbose BUG_ON already exists as a
concept within the kernel, so I chose a different name.


Note that my preferred mechanism would be to permit the cut-here line to be
emitted early, and have the one that's emitted by the BUG handler then
suppressed, but Linus doesn't like that.  I think it would be preferable to
trying to pass a va_list argument through the exception handler.

David

Re: [PATCH 3/7] Add assertion checking macros

[David Howells]

Andi Kleen <andi@firstfloor.org> wrote:

> David Howells <dhowells@redhat.com> writes:
> >
> >      There is no need to display val here in the case the expression fails
> >      since it can only be 0.  If this fails, it produces an error like the
> >      following:
> >
> > 	------------[ cut here ]------------
> > 	kernel BUG at fs/fscache/main.c:109!
> > 	ASSERTION FAILED
> 
> It would be nice to display the expression here like an user space
> assert. While it can be looked up in the source it would
> make quick eyeballing easier. 
> 
> Probably wouldn't cost too much additional text size?

It can easily be made selectable.  I don't think text size should be much of
an issue - but it will also add a bunch of strings (preprocessed expressions)
to the rodata, and I'm not sure how big those can get - particularly if
they've got nested macros within (test_bit() for example).

David