[PATCH] BUG() disassembly tweak

[PATCH] BUG() disassembly tweak

[Hugh Dickins]

Could we change the i386 BUG() macro slightly again?  Sorry to return
to this stale subject, but I'm now trying to do something constructive
about the insane disassembly issue, before 2.4.19 finalizes, and have
found that if FILE pointer and LINE short are interchanged, then ud2
instruction can be well displayed as an "Ap" instruction, like ljmp.
Whereas I've not found an instruction format that fits its current
ordering: maybe that's just my ignorance, please enlighten me if so.
Patch below against 2.5.15, applies with offset warning to 2.4.19-pre8.

Hugh

diff -urN 2.5.15/arch/i386/kernel/traps.c linux/arch/i386/kernel/traps.c
--- 2.5.15/arch/i386/kernel/traps.c	Tue Apr 23 12:18:51 2002
+++ linux/arch/i386/kernel/traps.c	Fri May 10 21:56:49 2002
@@ -260,9 +260,9 @@
 		goto no_bug;
 	if (ud2 != 0x0b0f)
 		goto no_bug;
-	if (__get_user(line, (unsigned short *)(eip + 2)))
+	if (__get_user(line, (unsigned short *)(eip + 6)))
 		goto bug;
-	if (__get_user(file, (char **)(eip + 4)) ||
+	if (__get_user(file, (char **)(eip + 2)) ||
 		(unsigned long)file < PAGE_OFFSET || __get_user(c, file))
 		file = "<bad filename>";
 
diff -urN 2.5.15/include/asm-i386/page.h linux/include/asm-i386/page.h
--- 2.5.15/include/asm-i386/page.h	Wed May  8 20:42:40 2002
+++ linux/include/asm-i386/page.h	Fri May 10 21:55:44 2002
@@ -98,9 +98,9 @@
 #if 1	/* Set to zero for a slightly smaller kernel */
 #define BUG()				\
  __asm__ __volatile__(	"ud2\n"		\
-			"\t.word %c0\n"	\
-			"\t.long %c1\n"	\
-			 : : "i" (__LINE__), "i" (__FILE__))
+			"\t.long %c0\n"	\
+			"\t.word %c1\n"	\
+			 : : "i" (__FILE__), "i" (__LINE__))
 #else
 #define BUG() __asm__ __volatile__("ud2\n")
 #endif

Re: [PATCH] BUG() disassembly tweak

[Linus Torvalds]

On Fri, 10 May 2002, Hugh Dickins wrote:
>
> Could we change the i386 BUG() macro slightly again?

If it wants to be changed, I'd actually personally prefer it to be changed
to take an explicit string instead of using the filename/linenr at all.

The filename/linenr one has the size problem (those absolute file names
are _long_), and sucks when you have slight kernel version skew and 
suddenly the information isn't obviously unambiguous at all.

It also sucks for inline functions or other users of BUG that would
potentially want to have different output.

In short, I suspect it would be nicer with 

	kernel BUG: release_task(current)

instead of

	kernel BUG at /home/torvalds/v2.5/linux/exit.c:59

(the exact point where the BUG happens _is_ given by the EIP, so in that
sense file and linenr are not actually all that useful. A descriptive
string would be more readable, and equally useful at pinpointing at a
source level).

		Linus

Re: [PATCH] BUG() disassembly tweak

[Dave Jones]

On Fri, May 10, 2002 at 03:10:04PM -0700, Linus Torvalds wrote:
 > If it wants to be changed, I'd actually personally prefer it to be changed
 > to take an explicit string instead of using the filename/linenr at all.

rather than fixing up the gazillions of existing BUG()'s we have
littered through the tree, maybe we could default to current behaviour
if no argument is passed ?

Failing that, resurrecting the k_assert() idea someone proposed
(jgarzik?) a few months back.

One reason for doing this would be to make it less painful for people
writing drivers that compile on 2.4/2.6        

    Dave.

--   
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs

Re: [PATCH] BUG() disassembly tweak

[Ingo Oeser]

On Sat, May 11, 2002 at 12:51:15AM +0200, Dave Jones wrote:
> On Fri, May 10, 2002 at 03:10:04PM -0700, Linus Torvalds wrote:
>  > If it wants to be changed, I'd actually personally prefer it to be changed
>  > to take an explicit string instead of using the filename/linenr at all.
[...]
> Failing that, resurrecting the k_assert() idea someone proposed
> (jgarzik?) a few months back.

Oh? I remember many people having objections against assert.

Personally I like BUG_ON() as is and it helped me already
catching a lot of bugs in my own code and removed the need for
several conditions and cleanup code for situation, that happen only
with wrong arguments/corruption and are bugs instead of user
stupidity.

BUG() is usally used, when we need to print much more information
about the error (like dumping some variables, which have wrong
values). In these cases a BUG_PRINTK() would be much more
useful, since most users of BUG() use printk before and only want
to save cleanup code for "impossible" conditions.

Or at least have a variant of BUG(), which has only triggers the
code path aborting, without printing filename/line_no (because
our own debug statements did this already).

Regards

Ingo Oeser
-- 
Science is what we can tell a computer. Art is everything else. --- D.E.Knuth

Re: [PATCH] BUG() disassembly tweak

[Andrew Morton]

Linus Torvalds wrote:
> 
> On Fri, 10 May 2002, Hugh Dickins wrote:
> >
> > Could we change the i386 BUG() macro slightly again?

Hugh's patch looks just dandy to me, even though I
don't understand the problem ;)
 
> If it wants to be changed, I'd actually personally prefer it to be changed
> to take an explicit string instead of using the filename/linenr at all.

There seem to be two classes of bug report(er)s: the good ones
who do a bit of up-front analysis and the less good ones....

I believe that the BUG() macro should be tuned to the less-good
ones.  Generally, these are folk who take their kernels
straight from kernel.org and the line number is not a problem.
People who have applied other patches have the experience and
knowledge to go digging in the source tree and find which
line it was, and to include this in the report.

So..  dunno about others, but I find file-n-line works
not too badly.

> The filename/linenr one has the size problem (those absolute file names
> are _long_), and sucks when you have slight kernel version skew and
> suddenly the information isn't obviously unambiguous at all.

Well given that kbuild-2.5 has a single makefile running
out of $(TOPDIR), it _should_ be chopping the absolute
pathname out of the include path and the .c path:

	gcc -I include drivers/net/foo.c

There will be no starting-with-slash __FILE__s in the output of
this command.

> It also sucks for inline functions or other users of BUG that would
> potentially want to have different output.
> 
> In short, I suspect it would be nicer with
> 
>         kernel BUG: release_task(current)
> 
> instead of
> 
>         kernel BUG at /home/torvalds/v2.5/linux/exit.c:59
> 
> (the exact point where the BUG happens _is_ given by the EIP, so in that
> sense file and linenr are not actually all that useful. A descriptive
> string would be more readable, and equally useful at pinpointing at a
> source level).

Isn't this the traditional assert(), which includes
that-which-was-asserted in the message?

	kernel assertion of "!PageLocked(page)" failed at foo.c:42.

That's easy to do in the context of BUG_ON(), and greatly
increases our chances of pinpointing the failing line...

-

Re: [PATCH] BUG() disassembly tweak

[Keith Owens]

On Fri, 10 May 2002 15:58:16 -0700, 
Andrew Morton <akpm@zip.com.au> wrote:
>Well given that kbuild-2.5 has a single makefile running
>out of $(TOPDIR), it _should_ be chopping the absolute
>pathname out of the include path and the .c path:
>
>	gcc -I include drivers/net/foo.c
>
>There will be no starting-with-slash __FILE__s in the output of
>this command.

Separate source and object.  Multiple source trees to support
additional drivers, filesystems etc.  kbuild 2.5 runs in the object
directory, reading from the source directories.  Path names are
absolute.

Re: [PATCH] BUG() disassembly tweak

[Hugh Dickins]

On Sat, 11 May 2002, Keith Owens wrote:
> On Fri, 10 May 2002 15:58:16 -0700, 
> Andrew Morton <akpm@zip.com.au> wrote:
> >
> >There will be no starting-with-slash __FILE__s in the output of
> >this command.
> 
> Separate source and object.  Multiple source trees to support
> additional drivers, filesystems etc.  kbuild 2.5 runs in the object
> directory, reading from the source directories.  Path names are
> absolute.

Is there some escaped syntax whereby we can (usefully) put
KBUILD_BASENAME into the BUG() macro in place of __FILE__?

Hugh

Re: [PATCH] BUG() disassembly tweak

[Kai Germaschewski]

On Sat, 11 May 2002, Hugh Dickins wrote:

> Is there some escaped syntax whereby we can (usefully) put
> KBUILD_BASENAME into the BUG() macro in place of __FILE__?

It doesn't help. When compiling, KBUILD_BASENAME is the name of the main
source file, i.e. the one given on the command line (basically the same
thing as __BASEFILE__ or whatever that gcc preprocessor variable is
called).

You don't want to get "BUG at foo:23", when the BUG instruction is
actually at filesystem.h:23. Only the compiler knows which source file it
is currently dealing with.

--Kai

Re: [PATCH] BUG() disassembly tweak

[Keith Owens]

On Sat, 11 May 2002 02:58:32 +0100 (BST), 
Hugh Dickins <hugh@veritas.com> wrote:
>Is there some escaped syntax whereby we can (usefully) put
>KBUILD_BASENAME into the BUG() macro in place of __FILE__?

AFAICT without testing it, you should be able to directly replace
__FILE__ with KBUILD_BASENAME.  Both are just pre-processor variables.

On my bells and whistles list (things to add after kbuild 2.5 is in the
kernel) is KBUILD_UNIQUE_NAME.  That variable will contain enough of
the pathname to uniquely identify the source.  For globally unique
names it is the same as KBUILD_BASENAME with a .[cS] suffix.  For
repeated filenames like inode.c it is fs/inode.c for the top level,
ext2/inode.c for an individual filesystem.

Re: [PATCH] BUG() disassembly tweak

[Hugh Dickins]

On Fri, 10 May 2002, Linus Torvalds wrote:
> On Fri, 10 May 2002, Hugh Dickins wrote:
> >
> > Could we change the i386 BUG() macro slightly again?
> 
> If it wants to be changed, I'd actually personally prefer it to be changed
> to take an explicit string instead of using the filename/linenr at all.

Aaargh, rerun!  Last time I suggested a tiny mod to get BUG() working
right (not losing the registers in its message display), you had new
ideas of how it could work, saving kernel space; and Andrew implemented
that magnificently.

I thought I was the only one dissatisfied (that a disassembler cannot
make sense of this line number and filename pointer dumped into the
instruction stream after the ud2: laugh at the ingenious instructions
ksymoops shows after the ud2 these days).

> The filename/linenr one has the size problem (those absolute file names
> are _long_), and sucks when you have slight kernel version skew and 
> suddenly the information isn't obviously unambiguous at all.

Absolute filenames are long, yes, but (in 2.4 anyway) few remain:
the .c filenames never came out absolute, always just leafname,
and Andrew has dealt with the vast majority of the .h filenames
from inlines (e.g. by using out_of_line_bug for them).  Does the
2.5 build not work out like that?

It's really 2.4.19 that's worrying me, that a small tweak now
(exchanging line and file) can make the new style much more palatable
to disassemblers; once 2.4.19 is out, it'll be confusing to change
(disassemblers don't ususally need to know the version of what they
are disassembling: no problem for kdb, but a problem for objdump).

> It also sucks for inline functions or other users of BUG that would
> potentially want to have different output.
> 
> In short, I suspect it would be nicer with 
> 
> 	kernel BUG: release_task(current)

Sure there's a case for more info; but maybe that's something else
than the simple BUG() we're used to dropping in wherever; let's fix
up what we've got now, and muse at leisure on what else to provide.

> instead of
> 
> 	kernel BUG at /home/torvalds/v2.5/linux/exit.c:59

I don't see those - exit.c:59 would be all you see in 2.4.19-pre.
"strings vmlinux | grep /home" currently shows me just:

/home/hugh/1908H/include/linux/raid/md_k.h
/home/hugh/1908H/include/linux/nfs_page.h
/home/hugh/1908H/include/linux/nfs_page.h
/home/hugh/1908H/include/linux/nfs_page.h
/home/hugh/1908H/include/linux/nfs_page.h

> (the exact point where the BUG happens _is_ given by the EIP, so in that
> sense file and linenr are not actually all that useful. A descriptive
> string would be more readable, and equally useful at pinpointing at a
> source level).

Hackers have better things to concentrate upon than dreaming up
descriptive strings: the beauty of BUG() is that you can just drop
it in (oops, I was about to say "without thinking").  I don't deny
the case for assertions, but what Andrew provided last time around
is really pretty good, and slips down more easily with the line<->file.

Hugh

Re: [PATCH] BUG() disassembly tweak

[Keith Owens]

On Sat, 11 May 2002 02:04:02 +0100 (BST), 
Hugh Dickins <hugh@veritas.com> wrote:
>I thought I was the only one dissatisfied (that a disassembler cannot
>make sense of this line number and filename pointer dumped into the
>instruction stream after the ud2: laugh at the ingenious instructions
>ksymoops shows after the ud2 these days).

IMNSHO the instructions _after_ the oops are almost useless.  I would
be much happier to see all architectures display the code line like
this (from alpha):

Code: 44220001  f4200003  46520400 <a77d9c38> 6b9b4a40  a44803a8  42425401  42c10403  40603401

Showing instructions either side of the oops and marking the
instruction pointer with <>.  Even that version displays too much data
after the oops, for debugging you need more instructions leading up to
the failure and fewer instructions afterwards.

The variable length i386 instructions are a problem, finding a decent
start point is tricky.  ksymoops handles up to 64 bytes of code so
dumping EIP-56:EIP+8 would increase the chance of the disassembler
syncing to the correct instructions.  To be absolutely sure, dump two
code lines on i386.

Code: EIP-56 ... <EIP> ... EIP+8
Code: EIP ... EIP+8

The second line starts with EIP and does not have <> around any values,
it guarantees that we get a clean decode of the failing instruction.
ksymoops will print both code lines, which makes the decoded trace look
a little strange, but it is worth it to get better debugging.  For
architectures with fixed length instructions this is not a problem.

Code: c6 05 00 00 00 00 00 <eb> e7 8d 76 00 b8 00 e0 ff ff 21 e0 ff 

>>EIP; c0113f8c No symbols available   <=====

...

Code;  c0113f85 No symbols available
00000000 <_EIP>:
Code;  c0113f85 No symbols available
   0:   c6 05 00 00 00 00 00      movb   $0x0,0x0
Code;  c0113f8c No symbols available   <=====
   7:   eb e7                     jmp    fffffff0 <_EIP+0xfffffff0> c0113f75 No symbols available   <=====
Code;  c0113f8e No symbols available
   9:   8d 76 00                  lea    0x0(%esi),%esi
Code;  c0113f91 No symbols available
   c:   b8 00 e0 ff ff            mov    $0xffffe000,%eax
Code;  c0113f96 No symbols available
  11:   21 e0                     and    %esp,%eax
Code;  c0113f98 No symbols available
  13:   ff 00                     incl   (%eax)

Code: eb e7 8d 76 00 b8 00 e0 (second code line, no <>)


Code;  c0113f8c No symbols available
00000000 <_EIP>:
Code;  c0113f8c No symbols available
   0:   eb e7                     jmp    ffffffe9 <_EIP+0xffffffe9> c0113f75 No symbols available
Code;  c0113f8e No symbols available
   2:   8d 76 00                  lea    0x0(%esi),%esi
Code;  c0113f91 No symbols available
   5:   b8 00 e0 00 00            mov    $0xe000,%eax

Re: [PATCH] BUG() disassembly tweak

[Keith Owens]

On Sat, 11 May 2002 11:34:07 +1000, 
Keith Owens <kaos@ocs.com.au> wrote:
>The variable length i386 instructions are a problem, finding a decent
>start point is tricky.  ksymoops handles up to 64 bytes of code so
>dumping EIP-56:EIP+8 would increase the chance of the disassembler
>syncing to the correct instructions.

Duh, fencepost.  EIP-56:EIP+7, of course.

Re: [PATCH] BUG() disassembly tweak

[Hugh Dickins]

On Sat, 11 May 2002, Keith Owens wrote:
> 
> IMNSHO the instructions _after_ the oops are almost useless.

Oh, I agree: I'd be perfectly happy for ksymoops to abandon trying
to disassemble the instructions after the oops, and I don't think
it need try to disassemble the instructions before the oops either.

It is important that it show the bytes (preferably before and)
after the oops, so that an investigator can locate that sequence
of bytes in a built object; and it is important that we have some
tool (objdump) which can disassemble the built object despite ud2
and its accompanying data; but ksymoops does not need to do it.

Hugh

Re: [PATCH] BUG() disassembly tweak

[Keith Owens]

On Sat, 11 May 2002 03:07:51 +0100 (BST), 
Hugh Dickins <hugh@veritas.com> wrote:
>On Sat, 11 May 2002, Keith Owens wrote:
>> 
>> IMNSHO the instructions _after_ the oops are almost useless.
>
>Oh, I agree: I'd be perfectly happy for ksymoops to abandon trying
>to disassemble the instructions after the oops, and I don't think
>it need try to disassemble the instructions before the oops either.
>
>It is important that it show the bytes (preferably before and)
>after the oops, so that an investigator can locate that sequence
>of bytes in a built object; and it is important that we have some
>tool (objdump) which can disassemble the built object despite ud2
>and its accompanying data; but ksymoops does not need to do it.

ksymoops does more than decode the instructions.  It does all the work
required to build the dummy object ready for objdump to do the
decoding.  It also massages the objdump output to make it more readable
and does symbol lookup on the absolute addresses returned by objdump.

Re: [PATCH] BUG() disassembly tweak

[Hugh Dickins]

On Fri, 10 May 2002, Hugh Dickins wrote:
> Could we change the i386 BUG() macro slightly again?  Sorry to return
> to this stale subject, but I'm now trying to do something constructive
> about the insane disassembly issue, before 2.4.19 finalizes, and have
> found that if FILE pointer and LINE short are interchanged, then ud2
> instruction can be well displayed as an "Ap" instruction, like ljmp.

My enthusiasm for making even the slightest change (ud2 Ap) to the
various disassemblers has waned over the weekend.  Particularly a
change that would require objdump to know whether and what version
of kernel it was dealing with.  And it seems to be an issue which
worries only me.

So I retract my earlier patch, and won't attempt to persuade Marcelo
to include it (or the one below) in 2.4.19: it was good for minimizing
change to the disassemblers, but no good for avoiding change to them.
Personally, I'll use some patch like the one below: waste another
byte to encapsulate the file pointer and line number in an ljmp.

Hugh

--- 2.4.19-pre8/arch/i386/kernel/traps.c	Fri May  3 12:17:41 2002
+++ linux/arch/i386/kernel/traps.c	Mon May 13 15:27:09 2002
@@ -256,9 +256,9 @@
 		goto no_bug;
 	if (ud2 != 0x0b0f)
 		goto no_bug;
-	if (__get_user(line, (unsigned short *)(eip + 2)))
+	if (__get_user(line, (unsigned short *)(eip + 7)))
 		goto bug;
-	if (__get_user(file, (char **)(eip + 4)) ||
+	if (__get_user(file, (char **)(eip + 3)) ||
 		(unsigned long)file < PAGE_OFFSET || __get_user(c, file))
 		file = "<bad filename>";
 
--- 2.4.19-pre8/include/asm-i386/page.h	Wed May  8 20:22:42 2002
+++ linux/include/asm-i386/page.h	Mon May 13 15:24:50 2002
@@ -89,18 +89,19 @@
 #ifndef __ASSEMBLY__
 
 /*
- * Tell the user there is some problem. Beep too, so we can
- * see^H^H^Hhear bugs in early bootup as well!
+ * Tell the user there is some problem.
  * The offending file and line are encoded after the "officially
- * undefined" opcode for parsing in the trap handler.
+ * undefined" opcode for parsing in the trap handler, with an
+ * "ljmp" code inserted so as not to confuse disassemblers.
  */
 
 #if 1	/* Set to zero for a slightly smaller kernel */
 #define BUG()				\
  __asm__ __volatile__(	"ud2\n"		\
-			"\t.word %c0\n"	\
-			"\t.long %c1\n"	\
-			 : : "i" (__LINE__), "i" (__FILE__))
+			"\t.byte 0xea\n" \
+			"\t.long %c0\n"	\
+			"\t.word %c1\n"	\
+			 : : "i" (__FILE__), "i" (__LINE__))
 #else
 #define BUG() __asm__ __volatile__("ud2\n")
 #endif