[PATCH v4] ARM: Use generic BUG() handler

[PATCH v4] ARM: Use generic BUG() handler

[Simon Glass]

From: Simon Glass <sjg@google.com>

ARM uses its own BUG() handler which makes its output slightly different
from other archtectures.

One of the problems is that the ARM implementation doesn't report the function
with the BUG() in it, but always reports the PC being in __bug(). The generic
implementation doesn't have this problem.

Currently we get something like:

kernel BUG at fs/proc/breakme.c:35!
Unable to handle kernel NULL pointer dereference at virtual address 00000000
...
PC is at __bug+0x20/0x2c

With this patch it displays:

kernel BUG at fs/proc/breakme.c:35!
Internal error: Oops - undefined instruction: 0 [#1] PREEMPT SMP
...
PC is at write_breakme+0xd0/0x1b4

This implementation uses an undefined instruction to implement BUG, and sets up
a bug table containing the relevant information. Many versions of gcc do not
support %c properly for ARM (inserting a # when they shouldn't) so we work
around this using distasteful macro magic.

v1: Initial version to replace existing ARM BUG() implementation with something
more similar to other architectures.

v2: Add Thumb support, remove backtrace whitespace output changes. Change to
use macros instead of requiring the asm %d flag to work (thanks to
Dave Martin <dave.martin@linaro.org>)

v3: Remove old BUG() implementation in favor of this one.
Remove the Backtrace: message (will submit this separately).
Use ARM_EXIT_KEEP() so that some architectures can dump exit text at link time
thanks to Stephen Boyd <sboyd@codeaurora.org> (although since we always
define GENERIC_BUG this might be academic.)
Rebase to linux-2.6.git master.

v4: Allow BUGS in modules (these were not reported correctly in v3)
(thanks to Stephen Boyd <sboyd@codeaurora.org> for suggesting that.)
Remove __bug() as this is no longer needed.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/Kconfig              |    4 +++
 arch/arm/include/asm/bug.h    |   55 ++++++++++++++++++++++++++++++++++------
 arch/arm/kernel/traps.c       |   31 +++++++++++++++-------
 arch/arm/kernel/vmlinux.lds.S |    3 +-
 4 files changed, 73 insertions(+), 20 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 5b9f78b..7d7df40 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -207,6 +207,10 @@ config ARM_PATCH_PHYS_VIRT_16BIT
 	def_bool y
 	depends on ARM_PATCH_PHYS_VIRT && ARCH_MSM
 
+config GENERIC_BUG
+	def_bool y
+	depends on BUG
+
 source "init/Kconfig"
 
 source "kernel/Kconfig.freezer"
diff --git a/arch/arm/include/asm/bug.h b/arch/arm/include/asm/bug.h
index 4d88425..2914724 100644
--- a/arch/arm/include/asm/bug.h
+++ b/arch/arm/include/asm/bug.h
@@ -3,21 +3,58 @@
 
 
 #ifdef CONFIG_BUG
-#ifdef CONFIG_DEBUG_BUGVERBOSE
-extern void __bug(const char *file, int line) __attribute__((noreturn));
-
-/* give file/line information */
-#define BUG()		__bug(__FILE__, __LINE__)
 
+/*
+ * Use a suitable undefined instruction to use for ARM/Thumb2 bug handling.
+ * We need to be careful not to conflict with those used by other modules and
+ * the register_undef_hook() system.
+ */
+#ifdef CONFIG_THUMB2_KERNEL
+#define BUG_INSTR_VALUE 0xde02
+#define BUG_INSTR_TYPE ".hword "
 #else
+#define BUG_INSTR_VALUE 0xe7f001f2
+#define BUG_INSTR_TYPE ".word "
+#endif
 
-/* this just causes an oops */
-#define BUG()		do { *(int *)0 = 0; } while (1)
 
-#endif
+#define BUG() _BUG(__FILE__, __LINE__, BUG_INSTR_VALUE)
+#define _BUG(file, line, value) __BUG(file, line, value)
+
+#ifdef CONFIG_DEBUG_BUGVERBOSE
+
+/*
+ * The extra indirection is to ensure that the __FILE__ string comes through
+ * OK. Many version of gcc do not support the asm %c parameter which would be
+ * preferable to this unpleasantness. We use mergeable string sections to
+ * avoid multiple copies of the string appearing in the kernel image.
+ */
+
+#define __BUG(__file, __line, __value)				\
+do {								\
+	BUILD_BUG_ON(sizeof(struct bug_entry) != 12);		\
+	asm volatile("1:\t" BUG_INSTR_TYPE #__value "\n"	\
+		".pushsection .rodata.str, \"aMS\", 1\n"	\
+		"2:\t.asciz " #__file "\n" 			\
+		".popsection\n" 				\
+		".pushsection __bug_table,\"a\"\n"		\
+		"3:\t.word 1b, 2b\n"				\
+		"\t.hword " #__line ", 0\n"			\
+		".popsection");					\
+	unreachable();						\
+} while (0)
+
+#else  /* not CONFIG_DEBUG_BUGVERBOSE */
+
+#define __BUG(__file, __line, __value)				\
+do {								\
+	asm volatile(BUG_INSTR_TYPE #__value);			\
+	unreachable();						\
+} while (0)
+#endif  /* CONFIG_DEBUG_BUGVERBOSE */
 
 #define HAVE_ARCH_BUG
-#endif
+#endif  /* CONFIG_BUG */
 
 #include <asm-generic/bug.h>
 
diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index f0000e1..400df50 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -21,6 +21,7 @@
 #include <linux/kdebug.h>
 #include <linux/module.h>
 #include <linux/kexec.h>
+#include <linux/bug.h>
 #include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/sched.h>
@@ -271,6 +272,8 @@ void die(const char *str, struct pt_regs *regs, int err)
 	spin_lock_irq(&die_lock);
 	console_verbose();
 	bust_spinlocks(1);
+	if (!user_mode(regs))
+		report_bug(regs->ARM_pc, regs);
 	ret = __die(str, err, thread, regs);
 
 	if (regs && kexec_should_crash(thread->task))
@@ -302,6 +305,24 @@ void arm_notify_die(const char *str, struct pt_regs *regs,
 	}
 }
 
+#ifdef CONFIG_GENERIC_BUG
+
+int is_valid_bugaddr(unsigned long pc)
+{
+#ifdef CONFIG_THUMB2_KERNEL
+	unsigned short bkpt;
+#else
+	unsigned long bkpt;
+#endif
+
+	if (probe_kernel_address((unsigned *)pc, bkpt))
+		return 0;
+
+	return bkpt == BUG_INSTR_VALUE;
+}
+
+#endif
+
 static LIST_HEAD(undef_hook);
 static DEFINE_SPINLOCK(undef_lock);
 
@@ -693,16 +714,6 @@ baddataabort(int code, unsigned long instr, struct pt_regs *regs)
 	arm_notify_die("unknown data abort code", regs, &info, instr, 0);
 }
 
-void __attribute__((noreturn)) __bug(const char *file, int line)
-{
-	printk(KERN_CRIT"kernel BUG at %s:%d!\n", file, line);
-	*(int *)0 = 0;
-
-	/* Avoid "noreturn function does return" */
-	for (;;);
-}
-EXPORT_SYMBOL(__bug);
-
 void __readwrite_bug(const char *fn)
 {
 	printk("%s called, but not implemented\n", fn);
diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
index b4348e6..81d4efe 100644
--- a/arch/arm/kernel/vmlinux.lds.S
+++ b/arch/arm/kernel/vmlinux.lds.S
@@ -21,7 +21,8 @@
 #define ARM_CPU_KEEP(x)
 #endif
 
-#if defined(CONFIG_SMP_ON_UP) && !defined(CONFIG_DEBUG_SPINLOCK)
+#if (defined(CONFIG_SMP_ON_UP) && !defined(CONFIG_DEBUG_SPINLOCK)) || \
+	defined(CONFIG_GENERIC_BUG)
 #define ARM_EXIT_KEEP(x)	x
 #else
 #define ARM_EXIT_KEEP(x)
-- 
1.7.3.1

[PATCH v4] ARM: Use generic BUG() handler

[Stephen Boyd]

On 04/14/2011 04:00 PM, Simon Glass wrote:
> From: Simon Glass <sjg@google.com>
>
> ARM uses its own BUG() handler which makes its output slightly different
> from other archtectures.
>
> One of the problems is that the ARM implementation doesn't report the function
> with the BUG() in it, but always reports the PC being in __bug(). The generic
> implementation doesn't have this problem.
>
> Currently we get something like:
>
> kernel BUG at fs/proc/breakme.c:35!
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
> ...
> PC is at __bug+0x20/0x2c
>
> With this patch it displays:
>
> kernel BUG at fs/proc/breakme.c:35!
> Internal error: Oops - undefined instruction: 0 [#1] PREEMPT SMP
> ...
> PC is at write_breakme+0xd0/0x1b4
>
> This implementation uses an undefined instruction to implement BUG, and sets up
> a bug table containing the relevant information. Many versions of gcc do not
> support %c properly for ARM (inserting a # when they shouldn't) so we work
> around this using distasteful macro magic.
>
> v1: Initial version to replace existing ARM BUG() implementation with something
> more similar to other architectures.
>
> v2: Add Thumb support, remove backtrace whitespace output changes. Change to
> use macros instead of requiring the asm %d flag to work (thanks to
> Dave Martin <dave.martin@linaro.org>)
>
> v3: Remove old BUG() implementation in favor of this one.
> Remove the Backtrace: message (will submit this separately).
> Use ARM_EXIT_KEEP() so that some architectures can dump exit text at link time
> thanks to Stephen Boyd <sboyd@codeaurora.org> (although since we always
> define GENERIC_BUG this might be academic.)
> Rebase to linux-2.6.git master.
>
> v4: Allow BUGS in modules (these were not reported correctly in v3)
> (thanks to Stephen Boyd <sboyd@codeaurora.org> for suggesting that.)
> Remove __bug() as this is no longer needed.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH v4] ARM: Use generic BUG() handler

[Simon Glass]

On Thu, Apr 14, 2011 at 7:10 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 04/14/2011 04:00 PM, Simon Glass wrote:
>> From: Simon Glass <sjg@google.com>
>>
>> ARM uses its own BUG() handler which makes its output slightly different
>> from other archtectures.


>> Signed-off-by: Simon Glass <sjg@chromium.org>
>
> Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

Thanks Stephen. Since there were no other comments I have submitted
this to RMK's patch tracking system.

Regards,
Simon

>
> --
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>
>

[PATCH v4] ARM: Use generic BUG() handler

[anish singh]

On Wed, Apr 20, 2011 at 8:10 AM, Simon Glass <sjg@chromium.org> wrote:

> On Thu, Apr 14, 2011 at 7:10 PM, Stephen Boyd <sboyd@codeaurora.org>
> wrote:
> > On 04/14/2011 04:00 PM, Simon Glass wrote:
> >> From: Simon Glass <sjg@google.com>
> >>
> >> ARM uses its own BUG() handler which makes its output slightly different
> >> from other archtectures.
>
>
> >> Signed-off-by: Simon Glass <sjg@chromium.org>
> >
> > Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
>
> Thanks Stephen. Since there were no other comments I have submitted
> this to RMK's patch tracking system.
>

Will this be part of linux kernel as i am not aware of patch tracking
system? I want to back port this feature on my
board which has Linux 2.6.35.7 kernel version(android gingerbread).

>
> Regards,
> Simon
>
> >
> > --
> > Sent by an employee of the Qualcomm Innovation Center, Inc.
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum.
> >
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110420/aef1f9ca/attachment-0001.html>

[PATCH v4] ARM: Use generic BUG() handler

[Stephen Boyd]

> On Wed, Apr 20, 2011 at 8:10 AM, Simon Glass <sjg@chromium.org> wrote:
>
>> On Thu, Apr 14, 2011 at 7:10 PM, Stephen Boyd <sboyd@codeaurora.org>
>> wrote:
>> > On 04/14/2011 04:00 PM, Simon Glass wrote:
>> >> From: Simon Glass <sjg@google.com>
>> >>
>> >> ARM uses its own BUG() handler which makes its output slightly
>> different
>> >> from other archtectures.
>>
>>
>> >> Signed-off-by: Simon Glass <sjg@chromium.org>
>> >
>> > Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
>>
>> Thanks Stephen. Since there were no other comments I have submitted
>> this to RMK's patch tracking system.
>>
>
> Will this be part of linux kernel as i am not aware of patch tracking
> system? I want to back port this feature on my
> board which has Linux 2.6.35.7 kernel version(android gingerbread).
>

It's here:
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6885/1

I imagine it will be added when/if Russell picks it up. Sadly it
increases the LOC in ARM so Simon might need to really
sell it to get it merged.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH v4] ARM: Use generic BUG() handler

[Ramirez Luna, Omar]

Hi

On Wed, Apr 20, 2011 at 1:01 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> It's here:
> http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6885/1
>
> I imagine it will be added when/if Russell picks it up. Sadly it
> increases the LOC in ARM so Simon might need to really
> sell it to get it merged.

I guess saying that it indirectly fixes wrong code generation on some
cases[1] might give an extra push.

Regards,

Omar

---
[1] http://lkml.org/lkml/2011/3/31/490

[PATCH v4] ARM: Use generic BUG() handler

[Olof Johansson]

On Wed, Apr 20, 2011 at 11:01:59AM -0700, Stephen Boyd wrote:

> It's here:
> http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6885/1
> 
> I imagine it will be added when/if Russell picks it up. Sadly it
> increases the LOC in ARM so Simon might need to really
> sell it to get it merged.

It's core code though, and the concern today is not with ARM core code
as much as with the various platforms that are growing seemingly without
upper bounds.


-Olof

[PATCH v4] ARM: Use generic BUG() handler

[Simon Glass]

On Mon, Apr 25, 2011 at 6:47 PM, Olof Johansson <olof@lixom.net> wrote:
> On Wed, Apr 20, 2011 at 11:01:59AM -0700, Stephen Boyd wrote:
>
>> It's here:
>> http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6885/1
>>
>> I imagine it will be added when/if Russell picks it up. Sadly it
>> increases the LOC in ARM so Simon might need to really
>> sell it to get it merged.
>
> It's core code though, and the concern today is not with ARM core code
> as much as with the various platforms that are growing seemingly without
> upper bounds.

Hi,

Can anyone advise please what happens next with this patch? So far I
have posted it to the list, it has been reviewed on the list and I
have put it into the patch system. What is the next step please to get
this into the kernel?

Regards,
Simon

>
>
> -Olof
>

[PATCH v4] ARM: Use generic BUG() handler

[Russell King - ARM Linux]

On Thu, May 19, 2011 at 10:24:31PM -0700, Simon Glass wrote:
> On Mon, Apr 25, 2011 at 6:47 PM, Olof Johansson <olof@lixom.net> wrote:
> > On Wed, Apr 20, 2011 at 11:01:59AM -0700, Stephen Boyd wrote:
> >
> >> It's here:
> >> http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6885/1
> >>
> >> I imagine it will be added when/if Russell picks it up. Sadly it
> >> increases the LOC in ARM so Simon might need to really
> >> sell it to get it merged.
> >
> > It's core code though, and the concern today is not with ARM core code
> > as much as with the various platforms that are growing seemingly without
> > upper bounds.
> 
> Hi,
> 
> Can anyone advise please what happens next with this patch? So far I
> have posted it to the list, it has been reviewed on the list and I
> have put it into the patch system. What is the next step please to get
> this into the kernel?

Sorry, I've finally got back to looking at this.

+               ".pushsection .rodata.str, \"aMS\", 1\n"        \

According to my gas manual:

| 7.88 `.pushsection NAME [, SUBSECTION] [, "FLAGS"[, @TYPE[,ARGUMENTS]]]'
| ...
|    This directive pushes the current section (and subsection) onto the
| top of the section stack, and then replaces the current section and
| subsection with `name' and `subsection'. The optional `flags', `type'
| and `arguments' are treated the same as in the `.section' (*note
| Section::) directive.
| 
| 7.94 `.section NAME'
| ...
| ELF Version
| ...
|    Note on targets where the `@' character is the start of a comment (eg
| ARM) then another character is used instead.  For example the ARM port
| uses the `%' character.
| 
|    If FLAGS contains the `M' symbol then the TYPE argument must be
| specified as well as an extra argument--ENTSIZE--like this:
| 
|      .section NAME , "FLAGS"M, @TYPE, ENTSIZE
| 
|    Sections with the `M' flag but not `S' flag must contain fixed size
| constants, each ENTSIZE octets long. Sections with both `M' and `S'
| must contain zero terminated strings where each character is ENTSIZE
| bytes long. The linker may remove duplicates within sections with the
| same name, same entity size and same flags.  ENTSIZE must be an
| absolute expression.

It appears that the TYPE argument is missing.  As the GAS manual says
its required, then I think it really ought to be there.  Any comment?

[PATCH v4] ARM: Use generic BUG() handler

[Dave Martin]

On Wed, Jul 06, 2011 at 09:06:07PM +0100, Russell King - ARM Linux wrote:
> On Thu, May 19, 2011 at 10:24:31PM -0700, Simon Glass wrote:
> > On Mon, Apr 25, 2011 at 6:47 PM, Olof Johansson <olof@lixom.net> wrote:
> > > On Wed, Apr 20, 2011 at 11:01:59AM -0700, Stephen Boyd wrote:
> > >
> > >> It's here:
> > >> http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6885/1
> > >>
> > >> I imagine it will be added when/if Russell picks it up. Sadly it
> > >> increases the LOC in ARM so Simon might need to really
> > >> sell it to get it merged.
> > >
> > > It's core code though, and the concern today is not with ARM core code
> > > as much as with the various platforms that are growing seemingly without
> > > upper bounds.
> > 
> > Hi,
> > 
> > Can anyone advise please what happens next with this patch? So far I
> > have posted it to the list, it has been reviewed on the list and I
> > have put it into the patch system. What is the next step please to get
> > this into the kernel?
> 
> Sorry, I've finally got back to looking at this.
> 
> +               ".pushsection .rodata.str, \"aMS\", 1\n"        \
> 
> According to my gas manual:
> 
> | 7.88 `.pushsection NAME [, SUBSECTION] [, "FLAGS"[, @TYPE[,ARGUMENTS]]]'
> | ...
> |    This directive pushes the current section (and subsection) onto the
> | top of the section stack, and then replaces the current section and
> | subsection with `name' and `subsection'. The optional `flags', `type'
> | and `arguments' are treated the same as in the `.section' (*note
> | Section::) directive.
> | 
> | 7.94 `.section NAME'
> | ...
> | ELF Version
> | ...
> |    Note on targets where the `@' character is the start of a comment (eg
> | ARM) then another character is used instead.  For example the ARM port
> | uses the `%' character.
> | 
> |    If FLAGS contains the `M' symbol then the TYPE argument must be
> | specified as well as an extra argument--ENTSIZE--like this:
> | 
> |      .section NAME , "FLAGS"M, @TYPE, ENTSIZE
> | 
> |    Sections with the `M' flag but not `S' flag must contain fixed size
> | constants, each ENTSIZE octets long. Sections with both `M' and `S'
> | must contain zero terminated strings where each character is ENTSIZE
> | bytes long. The linker may remove duplicates within sections with the
> | same name, same entity size and same flags.  ENTSIZE must be an
> | absolute expression.
> 
> It appears that the TYPE argument is missing.  As the GAS manual says
> its required, then I think it really ought to be there.  Any comment?

I guess type should be %progbits.  That's what the compiler generates for
the assembler input in similar situations.

Interestingly, gas does not require this argument and seems to default to
progbits anyway ... but it seems best to do what the manual says.

Cheers
---Dave

[PATCH v4] ARM: Use generic BUG() handler

[Simon Glass]

[Resent, without html, sorry]

Hi Russell & Dave,

On Thu, Jul 7, 2011 at 2:28 AM, Dave Martin <dave.martin@linaro.org> wrote:
> On Wed, Jul 06, 2011 at 09:06:07PM +0100, Russell King - ARM Linux wrote:
>>
>> Sorry, I've finally got back to looking at this.
>>
>> + ? ? ? ? ? ? ? ".pushsection .rodata.str, \"aMS\", 1\n" ? ? ? ?\
>>
>> According to my gas manual:
>>
>> | 7.88 `.pushsection NAME [, SUBSECTION] [, "FLAGS"[, @TYPE[,ARGUMENTS]]]'
>> | ...
[snip]
>> It appears that the TYPE argument is missing. ?As the GAS manual says
>> its required, then I think it really ought to be there. ?Any comment?
>
> I guess type should be %progbits. ?That's what the compiler generates for
> the assembler input in similar situations.
>
> Interestingly, gas does not require this argument and seems to default to
> progbits anyway ... but it seems best to do what the manual says.
>

Thanks for this. I will update the patch and resend to the list.

Regards,
Simon

> Cheers
> ---Dave
>