I am looking for comments please on this patch.
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.
Also backtraces were reported slightly differently - so I have added a newline
after the backtrace message, a space before the address, and ensured that the
message appears even when CONFIG_ARM_UNWIND is defined. These are aimed at
consistency between architectures, and may or may not be acceptable for ARM.
In any case they may be best as a separate patch.
Change-Id: I515db9a04e98084e6bbb21c4a1d13579568a0904
Signed-off-by: Simon Glass <[email protected]>
---
arch/arm/Kconfig | 4 ++++
arch/arm/include/asm/bug.h | 40 +++++++++++++++++++++++++++++++++++++++-
arch/arm/kernel/traps.c | 21 +++++++++++++++++++--
arch/arm/kernel/unwind.c | 1 +
arch/arm/kernel/vmlinux.lds.S | 15 +++++++++++++--
5 files changed, 76 insertions(+), 5 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 26d45e5..d4fb0fb 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -191,6 +191,10 @@ config VECTORS_BASE
help
The base address of exception vectors.
+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..bbbebe4 100644
--- a/arch/arm/include/asm/bug.h
+++ b/arch/arm/include/asm/bug.h
@@ -3,6 +3,40 @@
#ifdef CONFIG_BUG
+
+#ifdef CONFIG_GENERIC_BUG
+
+
+/* A suitable undefined instruction to use for ARM bug handling */
+#define BUG_INSTR_ARM 0xec000000
+
+
+#ifdef CONFIG_DEBUG_BUGVERBOSE
+#define BUG() \
+do { \
+ asm volatile("1:\t.word %c3\n" \
+ ".pushsection __bug_table,\"a\"\n" \
+ "2:\t.word 1b, %c0\n" \
+ "\t.hword %c1, 0\n" \
+ "\t.org 2b+%c2\n" \
+ ".popsection" \
+ : \
+ : "i" (__FILE__), "i" (__LINE__), \
+ "i" (sizeof(struct bug_entry)), \
+ "i" (BUG_INSTR_ARM)); \
+ unreachable(); \
+} while (0)
+
+#else
+#define BUG() \
+do { \
+ asm volatile("ud2"); \
+ unreachable(); \
+} while (0)
+#endif
+
+#else /* not CONFIG_GENERIC_BUG */
+
#ifdef CONFIG_DEBUG_BUGVERBOSE
extern void __bug(const char *file, int line) __attribute__((noreturn));
@@ -16,8 +50,12 @@ extern void __bug(const char *file, int line) __attribute__((noreturn));
#endif
+#endif /* CONFIG_GENERIC_BUG */
+
#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 ee57640..d5e5df9 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>
@@ -55,7 +56,8 @@ static void dump_mem(const char *, const char *, unsigned long, unsigned long);
void dump_backtrace_entry(unsigned long where, unsigned long from, unsigned long frame)
{
#ifdef CONFIG_KALLSYMS
- printk("[<%08lx>] (%pS) from [<%08lx>] (%pS)\n", where, (void *)where, from, (void *)from);
+ printk(" [<%08lx>] (%pS) from [<%08lx>] (%pS)\n", where, (void *)where,
+ from, (void *)from);
#else
printk("Function entered at [<%08lx>] from [<%08lx>]\n", where, from);
#endif
@@ -171,7 +173,7 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
unsigned int fp, mode;
int ok = 1;
- printk("Backtrace: ");
+ printk("Backtrace:\n");
if (!tsk)
tsk = current;
@@ -271,6 +273,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 +306,19 @@ void arm_notify_die(const char *str, struct pt_regs *regs,
}
}
+int is_valid_bugaddr(unsigned long pc)
+{
+ unsigned bkpt;
+
+ if (pc < PAGE_OFFSET)
+ return 0;
+ if (probe_kernel_address((unsigned *)pc, bkpt))
+ return 0;
+
+ return bkpt == BUG_INSTR_ARM;
+}
+
+
static LIST_HEAD(undef_hook);
static DEFINE_SPINLOCK(undef_lock);
diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index d2cb0b3..3f065bd 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -355,6 +355,7 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk)
register unsigned long current_sp asm ("sp");
pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
+ printk("Backtrace:\n");
if (!tsk)
tsk = current;
diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
index 86b66f3..591ab50 100644
--- a/arch/arm/kernel/vmlinux.lds.S
+++ b/arch/arm/kernel/vmlinux.lds.S
@@ -72,6 +72,18 @@ SECTIONS
PERCPU(PAGE_SIZE)
+ /*
+ * .exit.text is discarded at runtime, not link time, to deal with
+ * references from bug_table
+ */
+ .exit.text : AT(ADDR(.exit.text)) {
+ EXIT_TEXT
+ }
+
+ .exit.data : AT(ADDR(.exit.data)) {
+ EXIT_DATA
+ }
+
#ifndef CONFIG_XIP_KERNEL
. = ALIGN(PAGE_SIZE);
__init_end = .;
@@ -246,7 +258,6 @@ SECTIONS
__tcm_end = .;
}
#endif
-
BSS_SECTION(0, 0, 0)
_end = .;
@@ -254,7 +265,7 @@ SECTIONS
.comment 0 : { *(.comment) }
/* Default discards */
- DISCARDS
+ /*DISCARDS*/
#ifndef CONFIG_SMP_ON_UP
/DISCARD/ : {
--
1.7.3.1
On Mon, Feb 28, 2011 at 04:27:43PM -0800, Simon Glass wrote:
> + asm volatile("1:\t.word %c3\n" \
> + ".pushsection __bug_table,\"a\"\n" \
> + "2:\t.word 1b, %c0\n" \
> + "\t.hword %c1, 0\n" \
> + "\t.org 2b+%c2\n" \
%c doesn't work on lots of versions of gcc, which is why we can't use
the generic bug support. There's no way to reliably generate constants
without many compiler versions spitting out a '#' before them.
On Tue, Mar 01, 2011 at 08:49:49AM +0000, Russell King - ARM Linux wrote:
> On Mon, Feb 28, 2011 at 04:27:43PM -0800, Simon Glass wrote:
> > + asm volatile("1:\t.word %c3\n" \
> > + ".pushsection __bug_table,\"a\"\n" \
> > + "2:\t.word 1b, %c0\n" \
> > + "\t.hword %c1, 0\n" \
> > + "\t.org 2b+%c2\n" \
>
> %c doesn't work on lots of versions of gcc, which is why we can't use
> the generic bug support. There's no way to reliably generate constants
> without many compiler versions spitting out a '#' before them.
gcc 4.3.2:
asm(".word %c0" : : "i" (0));
produces:
.word #0
which gas chokes on:
/tmp/cc2hGOHd.s:12: Error: bad expression
/tmp/cc2hGOHd.s:12: Error: junk at end of line, first unrecognized character is `0'
So what this means is that it's impossible to generate constants in
assembly with GCC targetting ARM without having them prefixed by '#',
which in turn makes it impossible to use the generic BUG support.
I reported this bug to gcc folk many years ago. I've no idea which
version it has been fixed in or if it's even been fixed.
Russell King - ARM Linux writes:
> On Tue, Mar 01, 2011 at 08:49:49AM +0000, Russell King - ARM Linux wrote:
> > On Mon, Feb 28, 2011 at 04:27:43PM -0800, Simon Glass wrote:
> > > + asm volatile("1:\t.word %c3\n" \
> > > + ".pushsection __bug_table,\"a\"\n" \
> > > + "2:\t.word 1b, %c0\n" \
> > > + "\t.hword %c1, 0\n" \
> > > + "\t.org 2b+%c2\n" \
> >
> > %c doesn't work on lots of versions of gcc, which is why we can't use
> > the generic bug support. There's no way to reliably generate constants
> > without many compiler versions spitting out a '#' before them.
>
> gcc 4.3.2:
>
> asm(".word %c0" : : "i" (0));
>
> produces:
>
> .word #0
>
> which gas chokes on:
>
> /tmp/cc2hGOHd.s:12: Error: bad expression
> /tmp/cc2hGOHd.s:12: Error: junk at end of line, first unrecognized character is `0'
>
> So what this means is that it's impossible to generate constants in
> assembly with GCC targetting ARM without having them prefixed by '#',
> which in turn makes it impossible to use the generic BUG support.
>
> I reported this bug to gcc folk many years ago. I've no idea which
> version it has been fixed in or if it's even been fixed.
What's the gcc bugzilla bug number?
On Tue, Mar 01, 2011 at 10:12:40AM +0100, Mikael Pettersson wrote:
> Russell King - ARM Linux writes:
> > On Tue, Mar 01, 2011 at 08:49:49AM +0000, Russell King - ARM Linux wrote:
> > > On Mon, Feb 28, 2011 at 04:27:43PM -0800, Simon Glass wrote:
> > > > + asm volatile("1:\t.word %c3\n" \
> > > > + ".pushsection __bug_table,\"a\"\n" \
> > > > + "2:\t.word 1b, %c0\n" \
> > > > + "\t.hword %c1, 0\n" \
> > > > + "\t.org 2b+%c2\n" \
> > >
> > > %c doesn't work on lots of versions of gcc, which is why we can't use
> > > the generic bug support. There's no way to reliably generate constants
> > > without many compiler versions spitting out a '#' before them.
> >
> > gcc 4.3.2:
> >
> > asm(".word %c0" : : "i" (0));
> >
> > produces:
> >
> > .word #0
> >
> > which gas chokes on:
> >
> > /tmp/cc2hGOHd.s:12: Error: bad expression
> > /tmp/cc2hGOHd.s:12: Error: junk at end of line, first unrecognized character is `0'
> >
> > So what this means is that it's impossible to generate constants in
> > assembly with GCC targetting ARM without having them prefixed by '#',
> > which in turn makes it impossible to use the generic BUG support.
> >
> > I reported this bug to gcc folk many years ago. I've no idea which
> > version it has been fixed in or if it's even been fixed.
>
> What's the gcc bugzilla bug number?
No idea off hand - I'll have to search for that.
On Tue, Mar 1, 2011 at 2:03 AM, Russell King - ARM Linux
<[email protected]> wrote:
> On Tue, Mar 01, 2011 at 10:12:40AM +0100, Mikael Pettersson wrote:
>> Russell King - ARM Linux writes:
>> ?> On Tue, Mar 01, 2011 at 08:49:49AM +0000, Russell King - ARM Linux wrote:
>> ?> > On Mon, Feb 28, 2011 at 04:27:43PM -0800, Simon Glass wrote:
>> ?> > > + ? ? ?asm volatile("1:\t.word %c3\n" ? ? ? ? ? ? ? ? ? ? ? ? ?\
>> ?> > > + ? ? ? ? ? ? ? ? ? ".pushsection __bug_table,\"a\"\n" ? ? ? ? \
>> ?> > > + ? ? ? ? ? ? ? ? ? "2:\t.word 1b, %c0\n" ? ? ? ? ? ? ? ? ? ? ?\
>> ?> > > + ? ? ? ? ? ? ? ? ? "\t.hword %c1, 0\n" ? ? ? ? ? ? ? ? ? ? ? ?\
>> ?> > > + ? ? ? ? ? ? ? ? ? "\t.org 2b+%c2\n" ? ? ? ? ? ? ? ? ? ? ? ? ?\
>> ?> >
>> ?> > %c doesn't work on lots of versions of gcc, which is why we can't use
>> ?> > the generic bug support. ?There's no way to reliably generate constants
>> ?> > without many compiler versions spitting out a '#' before them.
>> ?>
>> ?> gcc 4.3.2:
>> ?>
>> ?> asm(".word %c0" : : "i" (0));
>> ?>
>> ?> produces:
>> ?>
>> ?> ? ? ? ? .word #0
>> ?>
>> ?> which gas chokes on:
>> ?>
>> ?> /tmp/cc2hGOHd.s:12: Error: bad expression
>> ?> /tmp/cc2hGOHd.s:12: Error: junk at end of line, first unrecognized character is `0'
>> ?>
>> ?> So what this means is that it's impossible to generate constants in
>> ?> assembly with GCC targetting ARM without having them prefixed by '#',
>> ?> which in turn makes it impossible to use the generic BUG support.
>> ?>
>> ?> I reported this bug to gcc folk many years ago. ?I've no idea which
>> ?> version it has been fixed in or if it's even been fixed.
>>
>> What's the gcc bugzilla bug number?
>
> No idea off hand - I'll have to search for that.
>
It seems I am lucky with the gcc I am using. I would have thought this
would be a pretty fundamental feature, but yes I did notice that %c
wasn't used anywhere.
Would this kernel feature be acceptable as a selectable config option
on ARM then?
Thanks,
Simon
On Tue, Mar 1, 2011 at 12:27 AM, Simon Glass <[email protected]> wrote:
> I am looking for comments please on this patch.
>
> 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.
>
> Also backtraces were reported slightly differently - so I have added a newline
> after the backtrace message, a space before the address, and ensured that the
> message appears even when CONFIG_ARM_UNWIND is defined. These are aimed at
> consistency between architectures, and may or may not be acceptable for ARM.
> In any case they may be best as a separate patch.
>
> Change-Id: I515db9a04e98084e6bbb21c4a1d13579568a0904
>
> Signed-off-by: Simon Glass <[email protected]>
> ---
> ?arch/arm/Kconfig ? ? ? ? ? ? ?| ? ?4 ++++
> ?arch/arm/include/asm/bug.h ? ?| ? 40 +++++++++++++++++++++++++++++++++++++++-
> ?arch/arm/kernel/traps.c ? ? ? | ? 21 +++++++++++++++++++--
> ?arch/arm/kernel/unwind.c ? ? ?| ? ?1 +
> ?arch/arm/kernel/vmlinux.lds.S | ? 15 +++++++++++++--
> ?5 files changed, 76 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 26d45e5..d4fb0fb 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -191,6 +191,10 @@ config VECTORS_BASE
> ? ? ? ?help
> ? ? ? ? ?The base address of exception vectors.
>
> +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..bbbebe4 100644
> --- a/arch/arm/include/asm/bug.h
> +++ b/arch/arm/include/asm/bug.h
> @@ -3,6 +3,40 @@
>
>
> ?#ifdef CONFIG_BUG
> +
> +#ifdef CONFIG_GENERIC_BUG
> +
> +
> +/* A suitable undefined instruction to use for ARM bug handling */
> +#define BUG_INSTR_ARM 0xec000000
This will need a suitable separate definition for
CONFIG_THUMB2_KERNEL, where the instruction encodings are different.
It may also be a good idea to include the whole directive, e.g.
#define BUG_INSTR_ARM ".word 0xec000000"
...since for Thumb you probably want to use .hword/.short (or
inst.n/inst.w if we consider that everyone has new enough tools)
instead of .long in that case.
> +
> +
> +#ifdef CONFIG_DEBUG_BUGVERBOSE
> +#define BUG() ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> +do { ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> + ? ? ? asm volatile("1:\t.word %c3\n" ? ? ? ? ? ? ? ? ? ? ? ? ?\
> + ? ? ? ? ? ? ? ? ? ?".pushsection __bug_table,\"a\"\n" ? ? ? ? \
> + ? ? ? ? ? ? ? ? ? ?"2:\t.word 1b, %c0\n" ? ? ? ? ? ? ? ? ? ? ?\
> + ? ? ? ? ? ? ? ? ? ?"\t.hword %c1, 0\n" ? ? ? ? ? ? ? ? ? ? ? ?\
> + ? ? ? ? ? ? ? ? ? ?"\t.org 2b+%c2\n" ? ? ? ? ? ? ? ? ? ? ? ? ?\
> + ? ? ? ? ? ? ? ? ? ?".popsection" ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> + ? ? ? ? ? ? ? ? ? ?: ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> + ? ? ? ? ? ? ? ? ? ?: "i" (__FILE__), "i" (__LINE__), ? ? ? ? ?\
> + ? ? ? ? ? ? ? ? ? ?"i" (sizeof(struct bug_entry)), ? ? ? ? ? ?\
> + ? ? ? ? ? ? ? ? ? ?"i" (BUG_INSTR_ARM)); ? ? ? ? ? ? ? ? ? ? ?\
> + ? ? ? unreachable(); ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> +} while (0)
> +
> +#else
> +#define BUG() ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> +do { ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> + ? ? ? asm volatile("ud2"); ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> + ? ? ? unreachable(); ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> +} while (0)
> +#endif
> +
> +#else ?/* not CONFIG_GENERIC_BUG */
> +
> ?#ifdef CONFIG_DEBUG_BUGVERBOSE
> ?extern void __bug(const char *file, int line) __attribute__((noreturn));
>
> @@ -16,8 +50,12 @@ extern void __bug(const char *file, int line) __attribute__((noreturn));
>
> ?#endif
>
> +#endif ?/* CONFIG_GENERIC_BUG */
> +
> ?#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 ee57640..d5e5df9 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>
>
> @@ -55,7 +56,8 @@ static void dump_mem(const char *, const char *, unsigned long, unsigned long);
> ?void dump_backtrace_entry(unsigned long where, unsigned long from, unsigned long frame)
> ?{
> ?#ifdef CONFIG_KALLSYMS
> - ? ? ? printk("[<%08lx>] (%pS) from [<%08lx>] (%pS)\n", where, (void *)where, from, (void *)from);
> + ? ? ? printk(" [<%08lx>] (%pS) from [<%08lx>] (%pS)\n", where, (void *)where,
> + ? ? ? ? ? ? ?from, (void *)from);
> ?#else
> ? ? ? ?printk("Function entered at [<%08lx>] from [<%08lx>]\n", where, from);
> ?#endif
> @@ -171,7 +173,7 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
> ? ? ? ?unsigned int fp, mode;
> ? ? ? ?int ok = 1;
>
> - ? ? ? printk("Backtrace: ");
> + ? ? ? printk("Backtrace:\n");
>
> ? ? ? ?if (!tsk)
> ? ? ? ? ? ? ? ?tsk = current;
> @@ -271,6 +273,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 +306,19 @@ void arm_notify_die(const char *str, struct pt_regs *regs,
> ? ? ? ?}
> ?}
>
> +int is_valid_bugaddr(unsigned long pc)
> +{
> + ? ? ? unsigned bkpt;
> +
> + ? ? ? if (pc < PAGE_OFFSET)
> + ? ? ? ? ? ? ? return 0;
> + ? ? ? if (probe_kernel_address((unsigned *)pc, bkpt))
> + ? ? ? ? ? ? ? return 0;
> +
> + ? ? ? return bkpt == BUG_INSTR_ARM;
> +}
> +
> +
> ?static LIST_HEAD(undef_hook);
> ?static DEFINE_SPINLOCK(undef_lock);
>
> diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
> index d2cb0b3..3f065bd 100644
> --- a/arch/arm/kernel/unwind.c
> +++ b/arch/arm/kernel/unwind.c
> @@ -355,6 +355,7 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk)
> ? ? ? ?register unsigned long current_sp asm ("sp");
>
> ? ? ? ?pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
> + ? ? ? printk("Backtrace:\n");
>
> ? ? ? ?if (!tsk)
> ? ? ? ? ? ? ? ?tsk = current;
> diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
> index 86b66f3..591ab50 100644
> --- a/arch/arm/kernel/vmlinux.lds.S
> +++ b/arch/arm/kernel/vmlinux.lds.S
> @@ -72,6 +72,18 @@ SECTIONS
>
> ? ? ? ?PERCPU(PAGE_SIZE)
>
> + ? ? ? /*
> + ? ? ? ?* .exit.text is discarded at runtime, not link time, to deal with
> + ? ? ? ?* ?references from bug_table
> + ? ? ? ?*/
> + ? ? ? .exit.text : AT(ADDR(.exit.text)) {
> + ? ? ? ? ? ? ? EXIT_TEXT
> + ? ? ? }
> +
> + ? ? ? .exit.data : AT(ADDR(.exit.data)) {
> + ? ? ? ? ? ? ? EXIT_DATA
> + ? ? ? }
> +
> ?#ifndef CONFIG_XIP_KERNEL
> ? ? ? ?. = ALIGN(PAGE_SIZE);
> ? ? ? ?__init_end = .;
> @@ -246,7 +258,6 @@ SECTIONS
> ? ? ? ? ? ? ? ?__tcm_end = .;
> ? ? ? ?}
> ?#endif
> -
> ? ? ? ?BSS_SECTION(0, 0, 0)
> ? ? ? ?_end = .;
>
> @@ -254,7 +265,7 @@ SECTIONS
> ? ? ? ?.comment 0 : { *(.comment) }
>
> ? ? ? ?/* Default discards */
> - ? ? ? DISCARDS
> + ? ? ? /*DISCARDS*/
>
> ?#ifndef CONFIG_SMP_ON_UP
> ? ? ? ?/DISCARD/ : {
> --
> 1.7.3.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
On Tue, Mar 01, 2011 at 08:34:37AM -0800, Simon Glass wrote:
> It seems I am lucky with the gcc I am using. I would have thought this
> would be a pretty fundamental feature, but yes I did notice that %c
> wasn't used anywhere.
It is, and I think it's something that I found back in 2002 or so.
I've still not had the time yet to search out the bug report, and
probably won't do for some time yet.
> Would this kernel feature be acceptable as a selectable config option
> on ARM then?
If I were to ask you if you knew which GCC version supported this,
would you know that answer?
If the answer to that is no, how would you expect people configuring
the kernel to know whether they should enable or disable this option?
On Mon, Feb 28, 2011 at 04:27:43PM -0800, Simon Glass wrote:
> @@ -55,7 +56,8 @@ static void dump_mem(const char *, const char *, unsigned long, unsigned long);
> void dump_backtrace_entry(unsigned long where, unsigned long from, unsigned long frame)
> {
> #ifdef CONFIG_KALLSYMS
> - printk("[<%08lx>] (%pS) from [<%08lx>] (%pS)\n", where, (void *)where, from, (void *)from);
> + printk(" [<%08lx>] (%pS) from [<%08lx>] (%pS)\n", where, (void *)where,
> + from, (void *)from);
> #else
> printk("Function entered at [<%08lx>] from [<%08lx>]\n", where, from);
> #endif
> @@ -171,7 +173,7 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
> unsigned int fp, mode;
> int ok = 1;
>
> - printk("Backtrace: ");
> + printk("Backtrace:\n");
>
> if (!tsk)
> tsk = current;
Why are you changing the way backtraces are printed? This introduces a
useless blank line in the oops dump. The previous hunk increases the
probability of mailers wrapping the backtrace making it harder to read.
If you're going to change the formatting of the Oops dump, please do it
as a separate patch and explain carefully why the change is necessary.
> diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
> index d2cb0b3..3f065bd 100644
> --- a/arch/arm/kernel/unwind.c
> +++ b/arch/arm/kernel/unwind.c
> @@ -355,6 +355,7 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk)
> register unsigned long current_sp asm ("sp");
>
> pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
> + printk("Backtrace:\n");
Err, no. This is in the wrong place.
> diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
> index 86b66f3..591ab50 100644
> --- a/arch/arm/kernel/vmlinux.lds.S
> +++ b/arch/arm/kernel/vmlinux.lds.S
> @@ -72,6 +72,18 @@ SECTIONS
>
> PERCPU(PAGE_SIZE)
>
> + /*
> + * .exit.text is discarded at runtime, not link time, to deal with
> + * references from bug_table
> + */
> + .exit.text : AT(ADDR(.exit.text)) {
> + EXIT_TEXT
> + }
> +
> + .exit.data : AT(ADDR(.exit.data)) {
> + EXIT_DATA
> + }
> +
> #ifndef CONFIG_XIP_KERNEL
> . = ALIGN(PAGE_SIZE);
> __init_end = .;
> @@ -246,7 +258,6 @@ SECTIONS
> __tcm_end = .;
> }
> #endif
> -
> BSS_SECTION(0, 0, 0)
> _end = .;
>
> @@ -254,7 +265,7 @@ SECTIONS
> .comment 0 : { *(.comment) }
>
> /* Default discards */
> - DISCARDS
> + /*DISCARDS*/
>
> #ifndef CONFIG_SMP_ON_UP
> /DISCARD/ : {
And this is a mess.
On Tue, Mar 01, 2011 at 08:34:37AM -0800, Simon Glass wrote:
[...]
>
> It seems I am lucky with the gcc I am using. I would have thought this
> would be a pretty fundamental feature, but yes I did notice that %c
> wasn't used anywhere.
>
> Would this kernel feature be acceptable as a selectable config option
> on ARM then?
>
> Thanks,
> Simon
+/* A suitable undefined instruction to use for ARM bug handling */
+#define BUG_INSTR ".word 0xec000000"
^ This needs to be changed, since it's not an architecturally
undefined encoding -- i.e., it may get used in the future to
mean something real.
See arch/arm/kernel/{ptrace,kprobes}.c for examples of
suitable encodings.
Rather than relying on the %c inline asm feature, can we work
around it?
Since we are writing a macro anyway, we can paste most of the
arguments in when the macro is expanded, rather than relying on
the inline assembler to do it.
This works for everything except sizeof (struct bug_entry)
which the preprocessor obviously can't understand. But I suspect
we don't really need that: we need to keep the directives that
generate the bug entry in sync with the definition of struct bug_entry
anyway (don't we?) -- so the amount of extra padding needed is
currently zero and should always be a constant.
So, maybe something like this would work:
(Note -- I haven't tested this!)
+
+#ifdef CONFIG_DEBUG_BUGVERBOSE
+#define BUG() __BUG(__FILE__, __LINE__, BUG_INSTR)
+#define __BUG(__file, __line, __bug_instr) \
+do { \
+ asm volatile("1:\t" __bug_instr "\n" q \
+ ".pushsection .rodata.str, \"a\"\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)
Cheers
---Dave
On Wed, Mar 02, 2011 at 10:59:50AM +0000, Dave Martin wrote:
> + asm volatile("1:\t" __bug_instr "\n" q \
> + ".pushsection .rodata.str, \"a\"\n" \
> + "2:\t.asciz \"" #__file "\"\n" \
> + ".popsection\n" \
Doesn't this mean we end up with multiple file names?
> + ".pushsection __bug_table,\"a\"\n" \
> + "3:\t.word 1b, 2b\n" \
> + "\t.hword " #__line ", 0\n" \
> + ".popsection" \
> + unreachable(); \
> +} while (0)
Second problem is that the above produces this:
1: .word 0xec000000
.pushsection .rodata.str, "a"
2: .asciz "__FILE__"
.popsection
.pushsection __bug_table,"a"
3: .word 1b, 2b
.hword __LINE__, 0
.popsection
which is clearly not what we want. Adding another level of indirection
starts to get closer to what we desire:
1: .word 0xec000000
.pushsection .rodata.str, "a"
2: .asciz ""t.c""
.popsection
.pushsection __bug_table,"a"
3: .word 1b, 2b
.hword 19, 0
.popsection
but we're still ending up with multiple strings containing the filename,
which is going to excessively bloat the kernel no end.
On Thu, Mar 3, 2011 at 2:11 PM, Russell King - ARM Linux
<[email protected]> wrote:
> On Wed, Mar 02, 2011 at 10:59:50AM +0000, Dave Martin wrote:
>> + ? ? asm volatile("1:\t" __bug_instr "\n" ? ? ? ? ? ?q ? ? ? \
>> + ? ? ? ? ? ? ? ? ?".pushsection .rodata.str, \"a\"\n" ? ? ? ?\
>> + ? ? ? ? ? ? ? ? ?"2:\t.asciz \"" #__file "\"\n" ? ? ? ? ? ? \
>> + ? ? ? ? ? ? ? ? ?".popsection\n" ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
>
> Doesn't this mean we end up with multiple file names?
Hmmm, yes, you're right.
After a bit of digging in the documentation, I find that ELF supports
mergable string sections though, so
.pushsection .rodata.str, "aMS", 1
.asciz __FILE__
.popsection
...actually looks like it ought to do the right thing: duplicate
strings in the section get merged during linking, even if there are
duplicates from a single object.
Simple experiments suggest that the linker does this right, even
merging distinct strings which share a common suffix. Do we use this
elsewhere in the kernel? It it's not already used, it could be a win
for any large, static string tables.
>
>> + ? ? ? ? ? ? ? ? ?".pushsection __bug_table,\"a\"\n" ? ? ? ? \
>> + ? ? ? ? ? ? ? ? ?"3:\t.word 1b, 2b\n" ? ? ? ? ? ? ? ? ? ? ? \
>> + ? ? ? ? ? ? ? ? ?"\t.hword " #__line ", 0\n" ? ? ? ? ? ? ? ?\
>> + ? ? ? ? ? ? ? ? ?".popsection" ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
>> + ? ? unreachable(); ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
>> +} while (0)
>
> Second problem is that the above produces this:
>
> ? ? ? ?1: ? ? ?.word 0xec000000
> .pushsection .rodata.str, "a"
> 2: ? ? ?.asciz "__FILE__"
> .popsection
> .pushsection __bug_table,"a"
> 3: ? ? ?.word 1b, 2b
> ? ? ? ?.hword __LINE__, 0
> .popsection
>
> which is clearly not what we want.
Indeed. I did say I hadn't tested it :)
> Adding another level of indirection
> starts to get closer to what we desire:
>
> ? ? ? ?1: ? ? ?.word 0xec000000
> .pushsection .rodata.str, "a"
> 2: ? ? ?.asciz ""t.c""
> .popsection
> .pushsection __bug_table,"a"
> 3: ? ? ?.word 1b, 2b
> ? ? ? ?.hword 19, 0
> .popsection
>
> but we're still ending up with multiple strings containing the filename,
> which is going to excessively bloat the kernel no end.
>
We'd need to remove the duplicate quotes, too.
For the .org <blah> + sizeof (struct bug_entry) problem, Will pointed
out that there's a BUILD_BUG_ON() macro which we could used to avoid
silently producing a bad build if the result of that sizeof isn't what
we expect.
So I still think it could work ... but it's a bit of an ugly hack I confess.
Cheers
---Dave