Re: [PATCH 1/7] Add assertion support with annotated oopsing

[Ingo Molnar <mingo@elte.hu>]

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

> 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 #including <linux/assert.h> and then
> calling:
> 
> 	void assertion_failed(const char *fmt, ...);
> 
> This prints a report that looks like:
> 
> 	------------[ cut here ]------------
> 	ASSERTION FAILED at fs/dcache.c:863!
> 	invalid opcode: 0000 [#1] SMP
> 	...
> 
> if fmt is NULL and:
> 
> 	------------[ cut here ]------------
> 	ASSERTION FAILED 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_assertion_failed(struct assertion_failure *desc)
> 
> to perform the oops and:
> 
> 	#define arch_assertion_failure(struct pt_regs *regs)
> 
> for report_bug() to find whether or not an assertion failure occurred and, if
> so, return a pointer to its description as passed to arch_assertion_failure().
> 
> If arch_assertion_failed() 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  |    1 +
>  include/linux/assert.h     |   36 ++++++++++++++++++++++++++++++++++++
>  kernel/panic.c             |   31 +++++++++++++++++++++++++++++++
>  lib/bug.c                  |   16 ++++++++++++++++
>  5 files changed, 98 insertions(+), 0 deletions(-)
>  create mode 100644 include/linux/assert.h

Looks useful, but i'd suggest to make this a variant of the standard 
BUG_ON()/WARN_ON() checks we already have, not an explicit assert().

BUG_ON_VERBOSE() or such.

I find assert()'s inversion confusing when mixed with 
WARN_ON()/BUG_ON().

Likewise, the message of:

       	ASSERTION FAILED at fs/dcache.c:863!

is rather confusing to me (i never know how the condition printed is 
to be interpreted) - why not use the usual 'BUG: ...' message 
convention?

Thanks,

	Ingo