[PATCH] generic show_stack facility

[PATCH] generic show_stack facility

[Christoph Hellwig]

This patch adds a prototype for show_stack to sched.h and exports
it.  Andrea's VM updates want this, as does Tux and I think it's a facility
we want to proive genericly.

Note that some architectures (e.g. ia64) have a conflicting prototype and
some (e.g. sparc) don't have show_stack at all, but I think -pre5 is early
enough in the 2.4.19 cycle to let the maintainers fix it.

Could you apply the patch to your tree?

	Christoph

diff -uNr -Xdontdiff ../master/linux-2.4.19-pre4/arch/i386/kernel/irq.c linux/arch/i386/kernel/irq.c
--- ../master/linux-2.4.19-pre4/arch/i386/kernel/irq.c	Wed Nov  7 20:09:56 2001
+++ linux/arch/i386/kernel/irq.c	Fri Mar 29 15:55:39 2002
@@ -192,8 +192,6 @@
 unsigned char global_irq_holder = NO_PROC_ID;
 unsigned volatile long global_irq_lock; /* pendantic: long for set_bit --RR */
 
-extern void show_stack(unsigned long* esp);
-
 static void show(char * str)
 {
 	int i;
@@ -224,7 +222,7 @@
 		}
 		esp &= ~(THREAD_SIZE-1);
 		esp += sizeof(struct task_struct);
-		show_stack((void*)esp);
+		show_stack((void *)esp);
  	}
 	printk("\nCPU %d:",cpu);
 	show_stack(NULL);
diff -uNr -Xdontdiff ../master/linux-2.4.19-pre4/arch/ia64/kernel/irq.c linux/arch/ia64/kernel/irq.c
--- ../master/linux-2.4.19-pre4/arch/ia64/kernel/irq.c	Thu Mar 21 15:24:10 2002
+++ linux/arch/ia64/kernel/irq.c	Fri Mar 29 15:56:13 2002
@@ -192,8 +192,6 @@
 unsigned int global_irq_holder = NO_PROC_ID;
 unsigned volatile long global_irq_lock; /* pedantic: long for set_bit --RR */
 
-extern void show_stack(unsigned long* esp);
-
 static void show(char * str)
 {
 	int i;
diff -uNr -Xdontdiff ../master/linux-2.4.19-pre4/arch/mips/kernel/traps.c linux/arch/mips/kernel/traps.c
--- ../master/linux-2.4.19-pre4/arch/mips/kernel/traps.c	Thu Mar 21 15:24:13 2002
+++ linux/arch/mips/kernel/traps.c	Fri Mar 29 15:53:53 2002
@@ -182,12 +182,12 @@
  * This routine abuses get_user()/put_user() to reference pointers
  * with at least a bit of error checking ...
  */
-void show_stack(unsigned int *sp)
+void show_stack(unsigned long *sp)
 {
 	int i;
-	unsigned int *stack;
+	unsigned long *stack;
 
-	stack = sp ? sp : (unsigned int *)&sp;
+	stack = sp ? sp : (unsigned long *)&sp;
 	i = 0;
 
 	printk("Stack:");
diff -uNr -Xdontdiff ../master/linux-2.4.19-pre4/include/linux/sched.h linux/include/linux/sched.h
--- ../master/linux-2.4.19-pre4/include/linux/sched.h	Thu Mar 21 15:24:23 2002
+++ linux/include/linux/sched.h	Fri Mar 29 15:59:51 2002
@@ -145,6 +145,7 @@
 extern void sched_init(void);
 extern void init_idle(void);
 extern void show_state(void);
+extern void show_stack(unsigned long *esp);
 extern void cpu_init (void);
 extern void trap_init(void);
 extern void update_process_times(int user);
diff -uNr -Xdontdiff ../master/linux-2.4.19-pre4/kernel/ksyms.c linux/kernel/ksyms.c
--- ../master/linux-2.4.19-pre4/kernel/ksyms.c	Thu Mar 21 15:24:23 2002
+++ linux/kernel/ksyms.c	Fri Mar 29 15:52:20 2002
@@ -487,6 +487,7 @@
 EXPORT_SYMBOL(seq_release);
 EXPORT_SYMBOL(seq_read);
 EXPORT_SYMBOL(seq_lseek);
+EXPORT_SYMBOL(show_stack);
 
 /* Program loader interfaces */
 EXPORT_SYMBOL(setup_arg_pages);

Re: [PATCH] generic show_stack facility

[David Mosberger]

>>>>> On Fri, 29 Mar 2002 15:23:14 +0000, Christoph Hellwig <hch@infradead.org> said:

  Christoph> This patch adds a prototype for show_stack to sched.h and
  Christoph> exports it.  Andrea's VM updates want this, as does Tux
  Christoph> and I think it's a facility we want to proive genericly.

  Christoph> Note that some architectures (e.g. ia64) have a
  Christoph> conflicting prototype and some (e.g. sparc) don't have
  Christoph> show_stack at all, but I think -pre5 is early enough in
  Christoph> the 2.4.19 cycle to let the maintainers fix it.

  Christoph> Could you apply the patch to your tree?

Please don't.  The patch is broken.

Christoph, why do you think the prototype for ia64 is different?  It's
because it *has to be*.  In general, you can't do a backtrace without
having the full (preserved) state of the CPU at the point of which the
backtrace begins.

	--david

Re: [PATCH] generic show_stack facility

[Christoph Hellwig]

On Fri, Mar 29, 2002 at 07:46:07AM -0800, David Mosberger wrote:
> Christoph, why do you think the prototype for ia64 is different?

I have stopped to wonder why ia64 does things differently.

>  It's
> because it *has to be*.  In general, you can't do a backtrace without
> having the full (preserved) state of the CPU at the point of which the
> backtrace begins.

So your suggestion is to move the other architectures to the ia64 prototype
or to not have an architecture-independand stack-traceback facility at all?

	Christoph

Re: [PATCH] generic show_stack facility

[David Mosberger]

>>>>> On Fri, 29 Mar 2002 16:06:18 +0000, Christoph Hellwig <hch@infradead.org> said:

  Christoph> On Fri, Mar 29, 2002 at 07:46:07AM -0800, David Mosberger
  Christoph> wrote:
  >> Christoph, why do you think the prototype for ia64 is different?

  Christoph> I have stopped to wonder why ia64 does things
  Christoph> differently.

You might want to reconsider that stance.  It could open your mind. ;-)

BTW: this is not at all an ia64-specific issue.  It applies to any
arch that doesn't maintain a frame pointer on the stack.  Basic
compiler technology.

  >> It's because it *has to be*.  In general, you can't do a
  >> backtrace without having the full (preserved) state of the CPU at
  >> the point of which the backtrace begins.

  Christoph> So your suggestion is to move the other architectures to
  Christoph> the ia64 prototype or to not have an
  Christoph> architecture-independand stack-traceback facility at all?

I haven't done such an investigation.  I believe the ia64 prototype is
reasonable and probably implementable on all platforms that can unwind
the stack in the first place, but before making a change to the stable
kernel API, I'd think someone would have to investigate this a bit
further.

One issue to consider is whether it's safe to call the routine on a
task that is running on another processor.  The ia64 implementation
can handle this gracefully, because it's sometimes better to print a
meaningless (garbled) stack trace than to make provably certain that
the task won't be running on any other CPU.  A related question is
whether the routine can be called with interrupts masked.

My sense is this is something that should be considered for 2.5 first.

	--david

Re: [PATCH] generic show_stack facility

[arjan]

In article <15524.40817.306204.292158@napali.hpl.hp.com> you wrote:
>>>>>> On Fri, 29 Mar 2002 16:06:18 +0000, Christoph Hellwig <hch@infradead.org> said:
> 
>  Christoph> On Fri, Mar 29, 2002 at 07:46:07AM -0800, David Mosberger
>  Christoph> wrote:
>  >> Christoph, why do you think the prototype for ia64 is different?
> 
>  Christoph> I have stopped to wonder why ia64 does things
>  Christoph> differently.
> 
> You might want to reconsider that stance.  It could open your mind. ;-)
> 
> BTW: this is not at all an ia64-specific issue.  It applies to any
> arch that doesn't maintain a frame pointer on the stack.  Basic
> compiler technology.

oh you mean like x86 ?

Re: [PATCH] generic show_stack facility

[David Mosberger]

>>>>> On Fri, 29 Mar 2002 17:16:23 GMT, arjan@fenrus.demon.nl said:

  >> BTW: this is not at all an ia64-specific issue.  It applies to
  >> any arch that doesn't maintain a frame pointer on the stack.
  >> Basic compiler technology.

  Arjan> oh you mean like x86 ?

As far as I know, the x86 version of show_trace() still relies on the
fact that (a) return addresses are stored on the memory stack, (b)
they are stored in the order in which the routines were called, and
(c) that there aren't too many other values on the stack that look
like kernel text addresses.  As long as an x86 compiler uses the CALL
instruction, that should be the case.  But this certainly isn't the
case for all architectures (though I do agree that it is a heuristic
should work reasonably well for several architectures).

	--david

Re: [PATCH] generic show_stack facility

[Kai Henningsen]

davidm@napali.hpl.hp.com (David Mosberger)  wrote on 29.03.02 in <15524.45472.231096.377756@napali.hpl.hp.com>:

> As far as I know, the x86 version of show_trace() still relies on the
> fact that (a) return addresses are stored on the memory stack, (b)
> they are stored in the order in which the routines were called, and
> (c) that there aren't too many other values on the stack that look
> like kernel text addresses.  As long as an x86 compiler uses the CALL
> instruction, that should be the case.

(b) is certainly not necessarily true for architectures like PPC, but the  
rest seems fairly unobjectionable - assuming that you first flush out your  
registers so any addresses only in registers get put on the stack, too. At  
least outside of Forth, separate return stacks seem to be extremely rare.

And as for the possibility of using several registers for return addresses  
such that dumping them leaves them on the stack out of order, well, the  
only thing I can see to get the original order back is to consult debug  
information that says where, on every call, to look for the next-up return  
address. Which doesn't seem quite feasible with anything less than a full  
gdb to do it.

What I don't see is what connection this can possibly have to the  
prototype of a stack dump routine.

MfG Kai

Re: [PATCH] generic show_stack facility

[Andrew Morton]

David Mosberger wrote:
> 
> ..
>   Christoph> So your suggestion is to move the other architectures to
>   Christoph> the ia64 prototype or to not have an
>   Christoph> architecture-independand stack-traceback facility at all?
> 
> I haven't done such an investigation.  I believe the ia64 prototype is
> reasonable and probably implementable on all platforms that can unwind
> the stack in the first place, but before making a change to the stable
> kernel API, I'd think someone would have to investigate this a bit
> further.
> 

The way I ended up resolving these sorts of issues was to 
make the generic function

	void dump_stack(void);

under the (hopefully valid) assumption that all architectures
can somehow, in some manner, manage to spit out something
useful.

For a transitional/compatibility think, there's lib/dump_stack.c
which just prints "I'm broken".

Here's the diff.  Comments?


--- 2.4.19-pre4/include/linux/kernel.h~aa-094-dump_stack	Tue Mar 26 23:11:27 2002
+++ 2.4.19-pre4-akpm/include/linux/kernel.h	Tue Mar 26 23:11:27 2002
@@ -106,6 +106,8 @@ extern int oops_in_progress;		/* If set,
 extern int tainted;
 extern const char *print_tainted(void);
 
+extern void dump_stack(void);
+
 #if DEBUG
 #define pr_debug(fmt,arg...) \
 	printk(KERN_DEBUG fmt,##arg)
--- /dev/null	Thu Aug 30 13:30:55 2001
+++ 2.4.19-pre4-akpm/lib/dump_stack.c	Tue Mar 26 23:11:27 2002
@@ -0,0 +1,13 @@
+/*
+ * Provide a default dump_stack() function for architectures
+ * which don't implement their own.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+void dump_stack(void)
+{
+	printk(KERN_NOTICE
+		"This architecture does not implement dump_stack()\n");
+}
--- 2.4.19-pre4/lib/Makefile~aa-094-dump_stack	Tue Mar 26 23:11:27 2002
+++ 2.4.19-pre4-akpm/lib/Makefile	Tue Mar 26 23:11:27 2002
@@ -10,7 +10,8 @@ L_TARGET := lib.a
 
 export-objs := cmdline.o dec_and_lock.o rwsem-spinlock.o rwsem.o rbtree.o
 
-obj-y := errno.o ctype.o string.o vsprintf.o brlock.o cmdline.o bust_spinlocks.o rbtree.o
+obj-y := errno.o ctype.o string.o vsprintf.o brlock.o cmdline.o \
+		bust_spinlocks.o rbtree.o dump_stack.o
 
 obj-$(CONFIG_RWSEM_GENERIC_SPINLOCK) += rwsem-spinlock.o
 obj-$(CONFIG_RWSEM_XCHGADD_ALGORITHM) += rwsem.o
--- 2.4.19-pre4/kernel/ksyms.c~aa-094-dump_stack	Tue Mar 26 23:11:27 2002
+++ 2.4.19-pre4-akpm/kernel/ksyms.c	Tue Mar 26 23:11:27 2002
@@ -560,3 +560,6 @@ EXPORT_SYMBOL(init_task_union);
 
 EXPORT_SYMBOL(tasklist_lock);
 EXPORT_SYMBOL(pidhash);
+
+/* debug */
+EXPORT_SYMBOL(dump_stack);
--- 2.4.19-pre4/arch/i386/kernel/traps.c~aa-094-dump_stack	Tue Mar 26 23:11:27 2002
+++ 2.4.19-pre4-akpm/arch/i386/kernel/traps.c	Tue Mar 26 23:11:27 2002
@@ -186,6 +186,14 @@ void show_stack(unsigned long * esp)
 	show_trace(esp);
 }
 
+/*
+ * The architecture-independent backtrace generator
+ */
+void dump_stack(void)
+{
+	show_stack(0);
+}
+
 void show_registers(struct pt_regs *regs)
 {
 	int i;

Re: [PATCH] generic show_stack facility

[David Mosberger]

>>>>> On Fri, 29 Mar 2002 09:36:26 -0800, Andrew Morton <akpm@zip.com.au> said:

  Andrew> The way I ended up resolving these sorts of issues was to
  Andrew> make the generic function

  Andrew> 	void dump_stack(void);

  Andrew> under the (hopefully valid) assumption that all
  Andrew> architectures can somehow, in some manner, manage to spit
  Andrew> out something useful.

  Andrew> For a transitional/compatibility think, there's
  Andrew> lib/dump_stack.c which just prints "I'm broken".

  Andrew> Here's the diff.  Comments?

Looks good to me.

Thanks,

	--david

Re: [PATCH] generic show_stack facility

[Christoph Hellwig]

On Fri, Mar 29, 2002 at 09:36:26AM -0800, Andrew Morton wrote:
> Here's the diff.  Comments?

I don't see who having to independand declaration in the same kernel
image are supposed to work..

I think you really want some HAVE_ARCH_SHOW_STACK define to disable
the generic version..

	Christoph

Re: [PATCH] generic show_stack facility

[Andrew Morton]

Christoph Hellwig wrote:
> 
> On Fri, Mar 29, 2002 at 09:36:26AM -0800, Andrew Morton wrote:
> > Here's the diff.  Comments?
> 
> I don't see who having to independand declaration in the same kernel
> image are supposed to work..

It goes in lib/lib.a.  The linker will only pick up
the default version if the architecture doesn't
have its own dump_stack().

bust_spinlocks() has worked that way for quite some time.

> I think you really want some HAVE_ARCH_SHOW_STACK define to disable
> the generic version..

Yup.  But it's nice to be able to slot the default version
into lib.a.   In the bust_spinlocks case, architectures only
need to implement a version if they have special needs.

-

Re: [PATCH] generic show_stack facility

[Keith Owens]

On Fri, 29 Mar 2002 10:41:11 -0800, 
Andrew Morton <akpm@zip.com.au> wrote:
>Christoph Hellwig wrote:
>> 
>> On Fri, Mar 29, 2002 at 09:36:26AM -0800, Andrew Morton wrote:
>> > Here's the diff.  Comments?
>> 
>> I don't see who having to independand declaration in the same kernel
>> image are supposed to work..
>
>It goes in lib/lib.a.  The linker will only pick up
>the default version if the architecture doesn't
>have its own dump_stack().
>
>bust_spinlocks() has worked that way for quite some time.

I have a problem with putting routines in lib.a and relying on the
linker to pull them out by default.  It does not work for routines
called from modules, modules do not include lib.a.  Remember the recent
problems with crc32.o?

bust_spinlocks() is not an issue because it is only called from built
in code.  show_stack() has been used as a debugging facility and it
could be called from a module.

Re: [PATCH] generic show_stack facility

[Andrew Morton]

Keith Owens wrote:
> 
> On Fri, 29 Mar 2002 10:41:11 -0800,
> Andrew Morton <akpm@zip.com.au> wrote:
> >Christoph Hellwig wrote:
> >>
> >> On Fri, Mar 29, 2002 at 09:36:26AM -0800, Andrew Morton wrote:
> >> > Here's the diff.  Comments?
> >>
> >> I don't see who having to independand declaration in the same kernel
> >> image are supposed to work..
> >
> >It goes in lib/lib.a.  The linker will only pick up
> >the default version if the architecture doesn't
> >have its own dump_stack().
> >
> >bust_spinlocks() has worked that way for quite some time.
> 
> I have a problem with putting routines in lib.a and relying on the
> linker to pull them out by default.  It does not work for routines
> called from modules, modules do not include lib.a.  Remember the recent
> problems with crc32.o?
> 
> bust_spinlocks() is not an issue because it is only called from built
> in code.  show_stack() has been used as a debugging facility and it
> could be called from a module.

Yes, that's a good point.  We're safe as long as core kernel
always contains a call to dump_stack(). Which is the case,
but that's a bit subtle for general usage.