[PATCH] Put the BUG __FILE__ and __LINE__ info out of line

[PATCH] Put the BUG __FILE__ and __LINE__ info out of line

[Jeremy Fitzhardinge]

When CONFIG_DEBUG_BUGVERBOSE is enabled, the embedded file and line
information makes a disassembler very unhappy, because it tries to
parse them as instructions (it probably makes the CPU's instruction
decoder a little unhappy too).

This patch moves them out of line, and calls the ud2 from the code -
the call makes sure the original %eip is available on the top of the
stack.  The result is a happy disassembler, with no loss of debugging
information.

Signed-off-by: Jeremy Fitzhardinge <jeremy@goop.org>

--
 arch/i386/kernel/vmlinux.lds.S |    2 ++
 include/asm-i386/bug.h         |   13 ++++++++-----
 2 files changed, 10 insertions(+), 5 deletions(-)

diff -r 1d29394927f3 arch/i386/kernel/vmlinux.lds.S
--- a/arch/i386/kernel/vmlinux.lds.S	Tue Sep 26 01:20:38 2006 -0700
+++ b/arch/i386/kernel/vmlinux.lds.S	Wed Sep 27 22:18:23 2006 -0700
@@ -27,6 +27,8 @@ SECTIONS
   _text = .;			/* Text and read-only data */
   .text : AT(ADDR(.text) - LOAD_OFFSET) {
 	*(.text)
+	__bugs = .;
+	*(.text.bugs)
 	SCHED_TEXT
 	LOCK_TEXT
 	KPROBES_TEXT
diff -r 1d29394927f3 include/asm-i386/bug.h
--- a/include/asm-i386/bug.h	Tue Sep 26 01:20:38 2006 -0700
+++ b/include/asm-i386/bug.h	Wed Sep 27 18:59:41 2006 -0700
@@ -11,11 +11,14 @@
 #ifdef CONFIG_BUG
 #define HAVE_ARCH_BUG
 #ifdef CONFIG_DEBUG_BUGVERBOSE
-#define BUG()				\
- __asm__ __volatile__(	"ud2\n"		\
-			"\t.word %c0\n"	\
-			"\t.long %c1\n"	\
-			 : : "i" (__LINE__), "i" (__FILE__))
+#define BUG()								\
+	__asm__ __volatile__("call 1f\n"				\
+			     ".section .text.bugs\n"			\
+			     "1:\tud2\n"					\
+			     "\t.word %c0\n"				\
+			     "\t.long %c1\n"				\
+			     ".previous\n"				\
+			     : : "i" (__LINE__), "i" (__FILE__))
 #else
 #define BUG() __asm__ __volatile__("ud2\n")
 #endif

Re: [PATCH] Put the BUG __FILE__ and __LINE__ info out of line

[Andrew Morton]

On Wed, 27 Sep 2006 23:00:03 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> When CONFIG_DEBUG_BUGVERBOSE is enabled, the embedded file and line
> information makes a disassembler very unhappy, because it tries to
> parse them as instructions (it probably makes the CPU's instruction
> decoder a little unhappy too).
> 
> This patch moves them out of line, and calls the ud2 from the code -
> the call makes sure the original %eip is available on the top of the
> stack.  The result is a happy disassembler, with no loss of debugging
> information.
> 
> Signed-off-by: Jeremy Fitzhardinge <jeremy@goop.org>
> 
> --
>  arch/i386/kernel/vmlinux.lds.S |    2 ++
>  include/asm-i386/bug.h         |   13 ++++++++-----
>  2 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff -r 1d29394927f3 arch/i386/kernel/vmlinux.lds.S
> --- a/arch/i386/kernel/vmlinux.lds.S	Tue Sep 26 01:20:38 2006 -0700
> +++ b/arch/i386/kernel/vmlinux.lds.S	Wed Sep 27 22:18:23 2006 -0700
> @@ -27,6 +27,8 @@ SECTIONS
>    _text = .;			/* Text and read-only data */
>    .text : AT(ADDR(.text) - LOAD_OFFSET) {
>  	*(.text)
> +	__bugs = .;
> +	*(.text.bugs)
>  	SCHED_TEXT
>  	LOCK_TEXT
>  	KPROBES_TEXT
> diff -r 1d29394927f3 include/asm-i386/bug.h
> --- a/include/asm-i386/bug.h	Tue Sep 26 01:20:38 2006 -0700
> +++ b/include/asm-i386/bug.h	Wed Sep 27 18:59:41 2006 -0700
> @@ -11,11 +11,14 @@
>  #ifdef CONFIG_BUG
>  #define HAVE_ARCH_BUG
>  #ifdef CONFIG_DEBUG_BUGVERBOSE
> -#define BUG()				\
> - __asm__ __volatile__(	"ud2\n"		\
> -			"\t.word %c0\n"	\
> -			"\t.long %c1\n"	\
> -			 : : "i" (__LINE__), "i" (__FILE__))
> +#define BUG()								\
> +	__asm__ __volatile__("call 1f\n"				\
> +			     ".section .text.bugs\n"			\
> +			     "1:\tud2\n"					\
> +			     "\t.word %c0\n"				\
> +			     "\t.long %c1\n"				\
> +			     ".previous\n"				\
> +			     : : "i" (__LINE__), "i" (__FILE__))
>  #else
>  #define BUG() __asm__ __volatile__("ud2\n")
>  #endif

hm.  Bigger vmlinux, smaller .text.

It means that we'll hit handle_BUG with that extra EIP pushed on the stack.
 What does that do to the stack trace, and to the unwinder?

It'll also muck up the displayed EIP, not that that matters a lot (well, it
might matter a bit if the BUG is in an inlined function).

We could get the correct EIP by fishing it off the stack (and subtracting
five from it?)

Or we could assume that BUG doesn't return (it doesn't) and make that call
a jmp.  But then we'd really lose the EIP.

Re: [PATCH] Put the BUG __FILE__ and __LINE__ info out of line

[Jeremy Fitzhardinge]

Andrew Morton wrote:
> hm.  Bigger vmlinux, smaller .text.
>   

Yep.

> It means that we'll hit handle_BUG with that extra EIP pushed on the stack.
>  What does that do to the stack trace, and to the unwinder?
>   
Dunno.  I was hoping Andi would pop up with the appropriate CFI gunk, if 
necessary.  But the reason for making it a call was to make it as 
unwindable as possible.

> It'll also muck up the displayed EIP, not that that matters a lot (well, it
> might matter a bit if the BUG is in an inlined function).
>
> We could get the correct EIP by fishing it off the stack (and subtracting
> five from it?)
>   

Yes, that's possible.

> Or we could assume that BUG doesn't return (it doesn't) and make that call
> a jmp.  But then we'd really lose the EIP.

Right.  Or it could save the EIP along with the line and filename.

    J

Re: [PATCH] Put the BUG __FILE__ and __LINE__ info out of line

[Andrew Morton]

On Wed, 27 Sep 2006 23:49:49 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> Andrew Morton wrote:
> > hm.  Bigger vmlinux, smaller .text.
> >   
> 
> Yep.
> 
> > It means that we'll hit handle_BUG with that extra EIP pushed on the stack.
> >  What does that do to the stack trace, and to the unwinder?
> >   
> Dunno.  I was hoping Andi would pop up with the appropriate CFI gunk, if 
> necessary.  But the reason for making it a call was to make it as 
> unwindable as possible.
> 
> > It'll also muck up the displayed EIP, not that that matters a lot (well, it
> > might matter a bit if the BUG is in an inlined function).
> >
> > We could get the correct EIP by fishing it off the stack (and subtracting
> > five from it?)
> >   
> 
> Yes, that's possible.
> 
> > Or we could assume that BUG doesn't return (it doesn't) and make that call
> > a jmp.  But then we'd really lose the EIP.
> 
> Right.  Or it could save the EIP along with the line and filename.
> 

Plan #17 is to just put the BUG inline and then put the EIP+file*+line into
a separate section, then search that section at BUG time to find the record
whose EIP points back at this ud2a.

It's a bit messy for modules, but it minimises the .text impact and keeps
disassembly happy, no?

And if done right it can probably be used by other architectures.

Re: [PATCH] Put the BUG __FILE__ and __LINE__ info out of line

[Andi Kleen]

On Wed, Sep 27, 2006 at 11:00:03PM -0700, Jeremy Fitzhardinge wrote:
> When CONFIG_DEBUG_BUGVERBOSE is enabled, the embedded file and line
> information makes a disassembler very unhappy, because it tries to
> parse them as instructions (it probably makes the CPU's instruction
> decoder a little unhappy too).
> 
> This patch moves them out of line, and calls the ud2 from the code -
> the call makes sure the original %eip is available on the top of the
> stack.  The result is a happy disassembler, with no loss of debugging
> information.

x86-64 has a much better solution for this. Please copy that one.

-Andi

Re: [PATCH] Put the BUG __FILE__ and __LINE__ info out of line

[Andi Kleen]

> Plan #17 is to just put the BUG inline and then put the EIP+file*+line into
> a separate section, then search that section at BUG time to find the record
> whose EIP points back at this ud2a.
> 
> It's a bit messy for modules, but it minimises the .text impact and keeps
> disassembly happy, no?
> 
> And if done right it can probably be used by other architectures.


The way x86-64 solved it was to turn the inline code into valid
instructions. This can be done with two additional bytes.
IMHO that's the right solution for the problem on i386 too

-Andi

Re: [PATCH] Put the BUG __FILE__ and __LINE__ info out of line

[Andrew Morton]

On 28 Sep 2006 09:17:31 +0200
Andi Kleen <ak@muc.de> wrote:

> > Plan #17 is to just put the BUG inline and then put the EIP+file*+line into
> > a separate section, then search that section at BUG time to find the record
> > whose EIP points back at this ud2a.
> > 
> > It's a bit messy for modules, but it minimises the .text impact and keeps
> > disassembly happy, no?
> > 
> > And if done right it can probably be used by other architectures.
> 
> 
> The way x86-64 solved it was to turn the inline code into valid
> instructions. This can be done with two additional bytes.
> IMHO that's the right solution for the problem on i386 too
> 

That uses even more text.

Re: [PATCH] Put the BUG __FILE__ and __LINE__ info out of line

[Andi Kleen]

On Thu, Sep 28, 2006 at 12:26:10AM -0700, Andrew Morton wrote:
> On 28 Sep 2006 09:17:31 +0200
> Andi Kleen <ak@muc.de> wrote:
> 
> > > Plan #17 is to just put the BUG inline and then put the EIP+file*+line into
> > > a separate section, then search that section at BUG time to find the record
> > > whose EIP points back at this ud2a.
> > > 
> > > It's a bit messy for modules, but it minimises the .text impact and keeps
> > > disassembly happy, no?
> > > 
> > > And if done right it can probably be used by other architectures.
> > 
> > 
> > The way x86-64 solved it was to turn the inline code into valid
> > instructions. This can be done with two additional bytes.
> > IMHO that's the right solution for the problem on i386 too
> > 
> 
> That uses even more text.

But no out of line section. So overall it's smaller, although the cache footprint
is 2 bytes larger. But then is 2 bytes larger really an issue? We don't have
_that_ many BUGs anyways.

I'll port the x86-64 way over to i386

-Andi

Re: [PATCH] Put the BUG __FILE__ and __LINE__ info out of line

[Jeremy Fitzhardinge]

Andrew Morton wrote:
> Plan #17 is to just put the BUG inline and then put the EIP+file*+line into
> a separate section, then search that section at BUG time to find the record
> whose EIP points back at this ud2a.
>   

Sure, but it seems a bit complex for this; I think simpler is better 
when the kernel has got itself into an iffy state.

> It's a bit messy for modules, but it minimises the .text impact and keeps
> disassembly happy, no?
>   
I'm not quite sure I understand your concern.  You're worried about the 
size increase to vmlinux in the case where you specify 
CONFIG_DEBUG_BUGVERBOSE?  Seems to me that if you specify it, you're 
willing to give up some kernel space for it, and adding 5 bytes/BUG 
isn't a huge deal (that's about 10k extra on my kernel).

Especially since the file+line info is mostly redundant anyway (since 
the kernel can tell you what a function an EIP is in), and completely 
redunant if you have debug info.  I guess its mostly useful so you can 
interpret the the bug message without access to kernel image.

> And if done right it can probably be used by other architectures.
>   

DWARF seems like the better answer to me.


    J

Re: [PATCH] Put the BUG __FILE__ and __LINE__ info out of line

[Jeremy Fitzhardinge]

Andi Kleen wrote:
> But no out of line section. So overall it's smaller, although the cache footprint
> is 2 bytes larger. But then is 2 bytes larger really an issue? We don't have
> _that_ many BUGs anyways.
>   

I think the out of line section is a feature; no point in crufting up 
the icache with BUG gunk, especially since a number of them are on 
fairly hot paths.

> I'll port the x86-64 way over to i386

It's neat, and it would solve my immediate problem, but I think my way 
is actually better.

    J

Re: [PATCH] Put the BUG __FILE__ and __LINE__ info out of line

[Andi Kleen]

On Thu, Sep 28, 2006 at 03:30:12AM -0700, Jeremy Fitzhardinge wrote:
> Andi Kleen wrote:
> >But no out of line section. So overall it's smaller, although the cache 
> >footprint
> >is 2 bytes larger. But then is 2 bytes larger really an issue? We don't 
> >have
> >_that_ many BUGs anyways.
> >  
> 
> I think the out of line section is a feature; no point in crufting up 
> the icache with BUG gunk, especially since a number of them are on 
> fairly hot paths.

It's 10 bytes per BUG. 

> 
> >I'll port the x86-64 way over to i386
> 
> It's neat, and it would solve my immediate problem, but I think my way 
> is actually better.

Your way will be even bigger because we would need to add
dwarf2 annotation to the out of line code.

-Andi

Re: [PATCH] Put the BUG __FILE__ and __LINE__ info out of line

[Hugh Dickins]

On Thu, 28 Sep 2006, Andi Kleen wrote:
> On Thu, Sep 28, 2006 at 03:30:12AM -0700, Jeremy Fitzhardinge wrote:
> > Andi Kleen wrote:
> > >But no out of line section. So overall it's smaller, although the cache 
> > >footprint
> > >is 2 bytes larger. But then is 2 bytes larger really an issue? We don't 
> > >have
> > >_that_ many BUGs anyways.
> > >  
> > 
> > I think the out of line section is a feature; no point in crufting up 
> > the icache with BUG gunk, especially since a number of them are on 
> > fairly hot paths.
> 
> It's 10 bytes per BUG. 

Or 9 bytes per BUG: I protested about the disassembly problem back
when the minimized BUG() first went in, and have been using "ljmp"
in my i386 builds ever since:

--- 2.6.18-git9/arch/i386/kernel/traps.c	2006-09-28 12:03:47.000000000 +0100
+++ linux/arch/i386/kernel/traps.c	2006-09-28 16:23:56.000000000 +0100
@@ -422,10 +422,10 @@ static void handle_BUG(struct pt_regs *r
 		char *file;
 		char c;
 
-		if (probe_kernel_address((unsigned short __user *)(eip + 2),
+		if (probe_kernel_address((unsigned short __user *)(eip + 7),
 					line))
 			break;
-		if (__get_user(file, (char * __user *)(eip + 4)) ||
+		if (__get_user(file, (char * __user *)(eip + 3)) ||
 		    (unsigned long)file < PAGE_OFFSET || __get_user(c, file))
 			file = "<bad filename>";
 
--- 2.6.18-git9/include/asm-i386/bug.h	2006-09-20 04:42:06.000000000 +0100
+++ linux/include/asm-i386/bug.h	2006-09-28 16:23:56.000000000 +0100
@@ -13,9 +13,11 @@
 #ifdef CONFIG_DEBUG_BUGVERBOSE
 #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__))
+	/* "ljmp long short" so disassemblers can make sense of it */
 #else
 #define BUG() __asm__ __volatile__("ud2\n")
 #endif

Re: [PATCH] Put the BUG __FILE__ and __LINE__ info out of line

[Andrew Morton]

On Thu, 28 Sep 2006 03:27:12 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> Andrew Morton wrote:
> > Plan #17 is to just put the BUG inline and then put the EIP+file*+line into
> > a separate section, then search that section at BUG time to find the record
> > whose EIP points back at this ud2a.
> >   
> 
> Sure, but it seems a bit complex for this; I think simpler is better 
> when the kernel has got itself into an iffy state.

It's just a linear search.

> > It's a bit messy for modules, but it minimises the .text impact and keeps
> > disassembly happy, no?
> >   
> I'm not quite sure I understand your concern.  You're worried about the 
> size increase to vmlinux in the case where you specify 
> CONFIG_DEBUG_BUGVERBOSE?

- We're using ten bytes of instruction cache where we could use two bytes

- If this is done right, other architectures can use the look-it-up code,
  thus cleaning up the kernel codebase.

And looky, powerpc already does this, so it'd be a matter of librarifying
their code.

Re: [PATCH] Put the BUG __FILE__ and __LINE__ info out of line

[Jeremy Fitzhardinge]

Andrew Morton wrote:
> - We're using ten bytes of instruction cache where we could use two bytes
>   

My patch reduces it to 5, but 2 would be better.

> - If this is done right, other architectures can use the look-it-up code,
>   thus cleaning up the kernel codebase.
>
> And looky, powerpc already does this, so it'd be a matter of librarifying
> their code.
>   

I'll have a look.

    J

Re: [PATCH] Put the BUG __FILE__ and __LINE__ info out of line

[Andi Kleen]

On Thu, Sep 28, 2006 at 04:30:19PM +0100, Hugh Dickins wrote:
> On Thu, 28 Sep 2006, Andi Kleen wrote:
> > On Thu, Sep 28, 2006 at 03:30:12AM -0700, Jeremy Fitzhardinge wrote:
> > > Andi Kleen wrote:
> > > >But no out of line section. So overall it's smaller, although the cache 
> > > >footprint
> > > >is 2 bytes larger. But then is 2 bytes larger really an issue? We don't 
> > > >have
> > > >_that_ many BUGs anyways.
> > > >  
> > > 
> > > I think the out of line section is a feature; no point in crufting up 
> > > the icache with BUG gunk, especially since a number of them are on 
> > > fairly hot paths.
> > 
> > It's 10 bytes per BUG. 
> 
> Or 9 bytes per BUG: I protested about the disassembly problem back
> when the minimized BUG() first went in, and have been using "ljmp"
> in my i386 builds ever since:

Good point.

Need to check if that works on x86-64 too. 

-Andi