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 <[email protected]>
--
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
On Wed, 27 Sep 2006 23:00:03 -0700
Jeremy Fitzhardinge <[email protected]> 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 <[email protected]>
>
> --
> 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.
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
On Wed, 27 Sep 2006 23:49:49 -0700
Jeremy Fitzhardinge <[email protected]> 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.
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
> 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
On 28 Sep 2006 09:17:31 +0200
Andi Kleen <[email protected]> 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.
On Thu, Sep 28, 2006 at 12:26:10AM -0700, Andrew Morton wrote:
> On 28 Sep 2006 09:17:31 +0200
> Andi Kleen <[email protected]> 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
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
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
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
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
On Thu, 28 Sep 2006 03:27:12 -0700
Jeremy Fitzhardinge <[email protected]> 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.
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
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