2004-03-08 09:44:50

by Amit S. Kale

[permalink] [raw]
Subject: kgdb for mainline kernel: core-lite [patch 1/3]

Hi Andrew,

Here is kgdb for mainline kernel in three patches. This is a lite
version of kgdb available from kgdb.sourceforge.net. I believe that all of us
agree on this lite kgdb.

It supports basic debugging of i386 architecture and debugging over a serial
line. Contents of these patches are as follows:

[1] core-lite.patch: architecture indepndent code
[2] i386-lite.patch: i386 architecture dependent code
[3] 8250.patch: support for generic serial driver

Thanks.
-Amit



Attachments:
(No filename) (482.00 B)
core-lite.patch (40.83 kB)
Download all attachments

2004-03-08 09:54:42

by Andrew Morton

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

"Amit S. Kale" <[email protected]> wrote:
>
> Here is kgdb for mainline kernel in three patches.

Thanks for working on this.

> This is a lite
> version of kgdb available from kgdb.sourceforge.net. I believe that all of us
> agree on this lite kgdb.

What is "lite" about it? As in, what features have been removed?

> It supports basic debugging of i386 architecture and debugging over a serial
> line. Contents of these patches are as follows:
>
> [1] core-lite.patch: architecture indepndent code
> [2] i386-lite.patch: i386 architecture dependent code
> [3] 8250.patch: support for generic serial driver

What is the story on kgdboe?

2004-03-08 10:15:30

by Amit S. Kale

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

On Monday 08 Mar 2004 3:24 pm, Andrew Morton wrote:
> "Amit S. Kale" <[email protected]> wrote:
> > Here is kgdb for mainline kernel in three patches.
>
> Thanks for working on this.

Credits go to all the kgdb developers.

>
> > This is a lite
> > version of kgdb available from kgdb.sourceforge.net. I believe that all
> > of us agree on this lite kgdb.
>
> What is "lite" about it? As in, what features have been removed?

Here are features that are present only in full kgdb:
1. Thread support (aka info threads)
2. console messages through gdb
3. Automatic loading of modules in gdb
4. Support for x86_64
5. Support for powerpc
6. kgdb over ethernet [This isn't ready in the full version as well at this
point of time]

-Amit

>
> > It supports basic debugging of i386 architecture and debugging over a
> > serial line. Contents of these patches are as follows:
> >
> > [1] core-lite.patch: architecture indepndent code
> > [2] i386-lite.patch: i386 architecture dependent code
> > [3] 8250.patch: support for generic serial driver
>
> What is the story on kgdboe?

2004-03-08 10:26:03

by Andrew Morton

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

"Amit S. Kale" <[email protected]> wrote:
>
> Here are features that are present only in full kgdb:
> 1. Thread support (aka info threads)

argh, disaster. I discussed this with Tom a week or so ago when it looked
like this it was being chopped out and I recall being told that the
discussion was referring to something else.

Ho-hum, sorry. Can we please put this back in?

> 2. console messages through gdb

hm, it was occasionally handy. Is there a lot of code involved?

> 3. Automatic loading of modules in gdb

OK. I think. What does this feature actually do?

> 4. Support for x86_64
> 5. Support for powerpc

These are planned, I assume?

> 6. kgdb over ethernet [This isn't ready in the full version as well at this
> point of time]

OK. But the version in -mm and -mpm works OK, does it not? Is there some
difference in implementation which causes it to be broken in your tree?

2004-03-08 10:49:32

by Amit S. Kale

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

On Monday 08 Mar 2004 3:56 pm, Andrew Morton wrote:
> "Amit S. Kale" <[email protected]> wrote:
> > Here are features that are present only in full kgdb:
> > 1. Thread support (aka info threads)
>
> argh, disaster. I discussed this with Tom a week or so ago when it looked
> like this it was being chopped out and I recall being told that the
> discussion was referring to something else.
>
> Ho-hum, sorry. Can we please put this back in?

Err., well this is one of the particularly dirty parts of kgdb. That's why
it's been kept away. It takes care of correct thread backtraces in some rare
cases.

If you consider it an absolutely must, we can do something so that the dirty
part is kept away and info threads almost always works.

>
> > 2. console messages through gdb
>
> hm, it was occasionally handy. Is there a lot of code involved?

Nope. Code is already in, it's just a matter of adding a config option.

>
> > 3. Automatic loading of modules in gdb
>
> OK. I think. What does this feature actually do?

This feature lets gdb hook onto a kernel function to detect loading and
unloading of modules and preserves module section information for later use
by gdb. At present this is broken for 2.6 kernels. I am working on this.

>
> > 4. Support for x86_64
> > 5. Support for powerpc
>
> These are planned, I assume?

Yes. As soon as i386 goes in. x86_64 is already very clean. There was another
opinion about whether x86_64 should be the first one to go in!

ppc might need some work.

> > 6. kgdb over ethernet [This isn't ready in the full version as well at
> > this point of time]
>
> OK. But the version in -mm and -mpm works OK, does it not? Is there some
> difference in implementation which causes it to be broken in your tree?

There are some differences. The one in my tree also apparently works to some
extent. It's being worked on.

-Amit

2004-03-08 11:07:20

by Andrew Morton

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

"Amit S. Kale" <[email protected]> wrote:
>
> On Monday 08 Mar 2004 3:56 pm, Andrew Morton wrote:
> > "Amit S. Kale" <[email protected]> wrote:
> > > Here are features that are present only in full kgdb:
> > > 1. Thread support (aka info threads)
> >
> > argh, disaster. I discussed this with Tom a week or so ago when it looked
> > like this it was being chopped out and I recall being told that the
> > discussion was referring to something else.
> >
> > Ho-hum, sorry. Can we please put this back in?
>
> Err., well this is one of the particularly dirty parts of kgdb. That's why
> it's been kept away. It takes care of correct thread backtraces in some rare
> cases.

Let me just make sure we're taking about the same thing here. Are you
saying that with kgdb-lite, `info threads' is completely missing, or does
it just not work correctly with threads (as opposed to heavyweight
processes)?

> If you consider it an absolutely must, we can do something so that the dirty
> part is kept away and info threads almost always works.

Yes, I'd consider `info threads' support a must-have. I'm rather surprised
that others do not?

2004-03-08 11:20:35

by Amit S. Kale

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

On Monday 08 Mar 2004 4:37 pm, Andrew Morton wrote:
> "Amit S. Kale" <[email protected]> wrote:
> > On Monday 08 Mar 2004 3:56 pm, Andrew Morton wrote:
> > > "Amit S. Kale" <[email protected]> wrote:
> > > > Here are features that are present only in full kgdb:
> > > > 1. Thread support (aka info threads)
> > >
> > > argh, disaster. I discussed this with Tom a week or so ago when it
> > > looked like this it was being chopped out and I recall being told that
> > > the discussion was referring to something else.
> > >
> > > Ho-hum, sorry. Can we please put this back in?
> >
> > Err., well this is one of the particularly dirty parts of kgdb. That's
> > why it's been kept away. It takes care of correct thread backtraces in
> > some rare cases.
>
> Let me just make sure we're taking about the same thing here. Are you
> saying that with kgdb-lite, `info threads' is completely missing, or does
> it just not work correctly with threads (as opposed to heavyweight
> processes)?

info threads shows a list of threads. Heavy/light weight processes doesn't
matter. Thread frame shown is incorrect.

I looked at i386 dependent code again. Following code in it is incorrect. I
never noticed it because this code is rarely used in full version of kgdb:

+void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct
*p)
....
+ gdb_regs[_EBP] = *(int *)p->thread.esp;

We can't guss ebp this way. This line should be removed.

+ gdb_regs[_DS] = __KERNEL_DS;
+ gdb_regs[_ES] = __KERNEL_DS;
+ gdb_regs[_PS] = 0;
+ gdb_regs[_CS] = __KERNEL_CS;
+ gdb_regs[_PC] = p->thread.eip;
+ gdb_regs[_ESP] = p->thread.esp;

This should be gdb_regs[_ESP] = &p->thread.esp

>
> > If you consider it an absolutely must, we can do something so that the
> > dirty part is kept away and info threads almost always works.
>
> Yes, I'd consider `info threads' support a must-have. I'm rather surprised
> that others do not?

Present threads support code changes calling convention of do_IRQ. Most
believe that to be an absolute no.

Since you consider it a must-have, I'll check whether above changes suggested
by me make info threads listing correct in most cases.

-Amit

2004-03-08 11:44:24

by Amit S. Kale

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

On Monday 08 Mar 2004 4:50 pm, Amit S. Kale wrote:
> On Monday 08 Mar 2004 4:37 pm, Andrew Morton wrote:
> > "Amit S. Kale" <[email protected]> wrote:
> > > On Monday 08 Mar 2004 3:56 pm, Andrew Morton wrote:
> > > > "Amit S. Kale" <[email protected]> wrote:
> > > > > Here are features that are present only in full kgdb:
> > > > > 1. Thread support (aka info threads)
> > > >
> > > > argh, disaster. I discussed this with Tom a week or so ago when it
> > > > looked like this it was being chopped out and I recall being told
> > > > that the discussion was referring to something else.
> > > >
> > > > Ho-hum, sorry. Can we please put this back in?
> > >
> > > Err., well this is one of the particularly dirty parts of kgdb. That's
> > > why it's been kept away. It takes care of correct thread backtraces in
> > > some rare cases.
> >
> > Let me just make sure we're taking about the same thing here. Are you
> > saying that with kgdb-lite, `info threads' is completely missing, or does
> > it just not work correctly with threads (as opposed to heavyweight
> > processes)?
>
> info threads shows a list of threads. Heavy/light weight processes doesn't
> matter. Thread frame shown is incorrect.
>
> I looked at i386 dependent code again. Following code in it is incorrect. I
> never noticed it because this code is rarely used in full version of kgdb:
>
> +void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct
> task_struct *p)
> ....
> + gdb_regs[_EBP] = *(int *)p->thread.esp;
>
> We can't guss ebp this way. This line should be removed.
>
> + gdb_regs[_DS] = __KERNEL_DS;
> + gdb_regs[_ES] = __KERNEL_DS;
> + gdb_regs[_PS] = 0;
> + gdb_regs[_CS] = __KERNEL_CS;
> + gdb_regs[_PC] = p->thread.eip;
> + gdb_regs[_ESP] = p->thread.esp;
>
> This should be gdb_regs[_ESP] = &p->thread.esp

That's not correct it. It should be gdb_regs[_ESP] = p->thread.esp;
Even with these changes I can't get thread listing correctly.

Here is the intrusive piece of code that helps get thread state correctly. Any
ideas on cleaning it?

--- linux-2.6.3-kgdb.orig/include/linux/sched.h 2004-02-24 10:44:47.000000000
+0530
+++ linux-2.6.3-kgdb/include/linux/sched.h 2004-03-04 18:42:56.324188184 +0530
@@ -173,7 +173,9 @@

#define MAX_SCHEDULE_TIMEOUT LONG_MAX
extern signed long FASTCALL(schedule_timeout(signed long timeout));
-asmlinkage void schedule(void);
+asmlinkage void do_schedule(void);
+asmlinkage void kern_schedule(void);
+asmlinkage void kern_do_schedule(struct pt_regs);

struct namespace;

@@ -907,6 +909,15 @@

#endif /* CONFIG_SMP */

+static inline void schedule(void)
+{
+#ifdef CONFIG_KGDB_THREAD
+ kern_schedule();
+#else
+ do_schedule();
+#endif
+}
+
--- linux-2.6.3-kgdb.orig/kernel/sched.c 2004-03-04 13:38:03.000000000 +0530
+++ linux-2.6.3-kgdb/kernel/sched.c 2004-03-04 18:42:56.327187728 +0530
@@ -1592,7 +1592,7 @@

/*
* schedule() is the main scheduler function.
*/
-asmlinkage void schedule(void)
+asmlinkage void do_schedule(void)
{
long *switch_count;
task_t *prev, *next;
@@ -1764,6 +1764,23 @@

EXPORT_SYMBOL(default_wake_function);

+asmlinkage void user_schedule(void)
+{
+#ifdef CONFIG_KGDB_THREAD
+ current->thread.debuggerinfo = NULL;
+#endif
+ do_schedule();
+}
+
+#ifdef CONFIG_KGDB_THREAD
+asmlinkage void kern_do_schedule(struct pt_regs regs)
+{
+ current->thread.debuggerinfo = &regs;
+ do_schedule();
+ current->thread.debuggerinfo = NULL;
+}
+#endif
+


-Amit

>
> > > If you consider it an absolutely must, we can do something so that the
> > > dirty part is kept away and info threads almost always works.
> >
> > Yes, I'd consider `info threads' support a must-have. I'm rather
> > surprised that others do not?
>
> Present threads support code changes calling convention of do_IRQ. Most
> believe that to be an absolute no.
>
> Since you consider it a must-have, I'll check whether above changes
> suggested by me make info threads listing correct in most cases.
>
> -Amit

2004-03-08 11:48:37

by Andrew Morton

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

"Amit S. Kale" <[email protected]> wrote:
>
> > Let me just make sure we're taking about the same thing here. Are you
> > saying that with kgdb-lite, `info threads' is completely missing, or does
> > it just not work correctly with threads (as opposed to heavyweight
> > processes)?
>
> info threads shows a list of threads. Heavy/light weight processes doesn't
> matter. Thread frame shown is incorrect.

It is? I haven't noticed any problems with it here. George recently
changed it to also display the process name in the gdb output, which is
valuable.

> I looked at i386 dependent code again. Following code in it is incorrect. I
> never noticed it because this code is rarely used in full version of kgdb:
>
> +void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct
> *p)

There is no such function in the stub in -mm kernels.

>
> Present threads support code changes calling convention of do_IRQ. Most
> believe that to be an absolute no.

I see no such change in George's stub, unless I'm missing something again.

> Since you consider it a must-have, I'll check whether above changes suggested
> by me make info threads listing correct in most cases.

The only problem I have with it is that sometimes after listing all threads
the debugger can lose control of the target and will start complaining
about communication errors. I assume the target has died. This happens
very rarely. Usually when you're about to find the bug ;)

2004-03-08 11:54:15

by Andrew Morton

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

"Amit S. Kale" <[email protected]> wrote:
>
> Here is the intrusive piece of code that helps get thread state correctly. Any
> ideas on cleaning it?

I think George's stub has diverged rather a lot from yours. Much more than
I was aware.


> --- linux-2.6.3-kgdb.orig/include/linux/sched.h 2004-02-24 10:44:47.000000000
> +0530
> +++ linux-2.6.3-kgdb/include/linux/sched.h 2004-03-04 18:42:56.324188184 +0530
> @@ -173,7 +173,9 @@
>
> #define MAX_SCHEDULE_TIMEOUT LONG_MAX
> extern signed long FASTCALL(schedule_timeout(signed long timeout));
> -asmlinkage void schedule(void);
> +asmlinkage void do_schedule(void);
> +asmlinkage void kern_schedule(void);
> +asmlinkage void kern_do_schedule(struct pt_regs);

The stub in -mm has only a single change to sched.c:

diff -puN kernel/sched.c~kgdb-ga kernel/sched.c
--- 25/kernel/sched.c~kgdb-ga 2004-03-07 01:57:48.000000000 -0800
+++ 25-akpm/kernel/sched.c 2004-03-07 01:57:48.000000000 -0800
@@ -2015,6 +2015,13 @@ out_unlock:

EXPORT_SYMBOL(set_user_nice);

+#if defined( CONFIG_KGDB)
+struct task_struct * kgdb_get_idle(int this_cpu)
+{
+ return cpu_rq(this_cpu)->idle;
+}
+#endif
+
#ifndef __alpha__

/*

2004-03-08 12:01:20

by Amit S. Kale

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

On Monday 08 Mar 2004 5:24 pm, Andrew Morton wrote:
> "Amit S. Kale" <[email protected]> wrote:
> > Here is the intrusive piece of code that helps get thread state
> > correctly. Any ideas on cleaning it?
>
> I think George's stub has diverged rather a lot from yours. Much more than
> I was aware.

Yes. It has.
At this point of time no one knows precisely where all they differ and
how/whether they could be merged.

The threads code in my version saves all registers during a context switch.
This provides two benefits:
1. GDB doesn't lose track of registers when going up a stack trace starting
with switch_to.
2. If a thread calls schedule, the thread backtrace starts from caller of
schedule rather than schedule itself. So info threads command shows a more
meaningful output.

-Amit

>
> > --- linux-2.6.3-kgdb.orig/include/linux/sched.h 2004-02-24
> > 10:44:47.000000000 +0530
> > +++ linux-2.6.3-kgdb/include/linux/sched.h 2004-03-04 18:42:56.324188184
> > +0530 @@ -173,7 +173,9 @@
> >
> > #define MAX_SCHEDULE_TIMEOUT LONG_MAX
> > extern signed long FASTCALL(schedule_timeout(signed long timeout));
> > -asmlinkage void schedule(void);
> > +asmlinkage void do_schedule(void);
> > +asmlinkage void kern_schedule(void);
> > +asmlinkage void kern_do_schedule(struct pt_regs);
>
> The stub in -mm has only a single change to sched.c:
>
> diff -puN kernel/sched.c~kgdb-ga kernel/sched.c
> --- 25/kernel/sched.c~kgdb-ga 2004-03-07 01:57:48.000000000 -0800
> +++ 25-akpm/kernel/sched.c 2004-03-07 01:57:48.000000000 -0800
> @@ -2015,6 +2015,13 @@ out_unlock:
>
> EXPORT_SYMBOL(set_user_nice);
>
> +#if defined( CONFIG_KGDB)
> +struct task_struct * kgdb_get_idle(int this_cpu)
> +{
> + return cpu_rq(this_cpu)->idle;
> +}
> +#endif
> +
> #ifndef __alpha__
>
> /*

2004-03-08 15:19:27

by Tom Rini

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

On Mon, Mar 08, 2004 at 05:14:05PM +0530, Amit S. Kale wrote:
> On Monday 08 Mar 2004 4:50 pm, Amit S. Kale wrote:
> > On Monday 08 Mar 2004 4:37 pm, Andrew Morton wrote:
> > > "Amit S. Kale" <[email protected]> wrote:
> > > > On Monday 08 Mar 2004 3:56 pm, Andrew Morton wrote:
> > > > > "Amit S. Kale" <[email protected]> wrote:
> > > > > > Here are features that are present only in full kgdb:
> > > > > > 1. Thread support (aka info threads)
> > > > >
> > > > > argh, disaster. I discussed this with Tom a week or so ago when it
> > > > > looked like this it was being chopped out and I recall being told
> > > > > that the discussion was referring to something else.
> > > > >
> > > > > Ho-hum, sorry. Can we please put this back in?
> > > >
> > > > Err., well this is one of the particularly dirty parts of kgdb. That's
> > > > why it's been kept away. It takes care of correct thread backtraces in
> > > > some rare cases.
> > >
> > > Let me just make sure we're taking about the same thing here. Are you
> > > saying that with kgdb-lite, `info threads' is completely missing, or does
> > > it just not work correctly with threads (as opposed to heavyweight
> > > processes)?
> >
> > info threads shows a list of threads. Heavy/light weight processes doesn't
> > matter. Thread frame shown is incorrect.
> >
> > I looked at i386 dependent code again. Following code in it is incorrect. I
> > never noticed it because this code is rarely used in full version of kgdb:
> >
> > +void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct
> > task_struct *p)
> > ....
> > + gdb_regs[_EBP] = *(int *)p->thread.esp;
> >
> > We can't guss ebp this way. This line should be removed.
> >
> > + gdb_regs[_DS] = __KERNEL_DS;
> > + gdb_regs[_ES] = __KERNEL_DS;
> > + gdb_regs[_PS] = 0;
> > + gdb_regs[_CS] = __KERNEL_CS;
> > + gdb_regs[_PC] = p->thread.eip;
> > + gdb_regs[_ESP] = p->thread.esp;
> >
> > This should be gdb_regs[_ESP] = &p->thread.esp
>
> That's not correct it. It should be gdb_regs[_ESP] = p->thread.esp;
> Even with these changes I can't get thread listing correctly.
>
> Here is the intrusive piece of code that helps get thread state correctly. Any
> ideas on cleaning it?

Here's where what Andi said about being able to get the pt_regs stuff
off the stack (I think that's what he said at least) started to confuse
me slightly. But if I understand it right (and I never got around to
testing this) we can replace the do_schedule() stuffs at least with
something like kgdb_get_pt_regs(), since (and I lost my notes on this,
so it's probably not quite right) (thread_info->esp0)-1

--
Tom Rini
http://gate.crashing.org/~trini/

2004-03-08 15:22:19

by Tom Rini

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

On Mon, Mar 08, 2004 at 04:50:18PM +0530, Amit S. Kale wrote:
> On Monday 08 Mar 2004 4:37 pm, Andrew Morton wrote:
> > "Amit S. Kale" <[email protected]> wrote:
[snip]
> > > If you consider it an absolutely must, we can do something so that the
> > > dirty part is kept away and info threads almost always works.
> >
> > Yes, I'd consider `info threads' support a must-have. I'm rather surprised
> > that others do not?
>
> Present threads support code changes calling convention of do_IRQ. Most
> believe that to be an absolute no.

I believe that George's version does something totally different, with
some macros at compile time (and binutils support, I _think_) to not
have to change do_IRQ.

--
Tom Rini
http://gate.crashing.org/~trini/

2004-03-08 16:32:29

by Amit S. Kale

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

On Monday 08 Mar 2004 8:52 pm, Tom Rini wrote:
> On Mon, Mar 08, 2004 at 04:50:18PM +0530, Amit S. Kale wrote:
> > On Monday 08 Mar 2004 4:37 pm, Andrew Morton wrote:
> > > "Amit S. Kale" <[email protected]> wrote:
>
> [snip]
>
> > > > If you consider it an absolutely must, we can do something so that
> > > > the dirty part is kept away and info threads almost always works.
> > >
> > > Yes, I'd consider `info threads' support a must-have. I'm rather
> > > surprised that others do not?
> >
> > Present threads support code changes calling convention of do_IRQ. Most
> > believe that to be an absolute no.
>
> I believe that George's version does something totally different, with
> some macros at compile time (and binutils support, I _think_) to not
> have to change do_IRQ.

OOPS! Present code doesn't change calling convention of do_IRQ. I changed that
some time ago.

-Amit

2004-03-08 16:57:53

by Andi Kleen

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

Tom Rini <[email protected]> writes:
>
> Here's where what Andi said about being able to get the pt_regs stuff
> off the stack (I think that's what he said at least) started to confuse
> me slightly. But if I understand it right (and I never got around to
> testing this) we can replace the do_schedule() stuffs at least with
> something like kgdb_get_pt_regs(), since (and I lost my notes on this,
> so it's probably not quite right) (thread_info->esp0)-1

No, that's the user space registers.

You don't need these registers really as long as you have the
correct dwarf2 CFI description of all the code involved. gdb
is then able to reconstruct them using the C stack by itself.

All it needs for that is esp and eip.

-Andi

2004-03-08 17:18:34

by Tom Rini

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

On Mon, Mar 08, 2004 at 05:57:26PM +0100, Andi Kleen wrote:
> Tom Rini <[email protected]> writes:
> >
> > Here's where what Andi said about being able to get the pt_regs stuff
> > off the stack (I think that's what he said at least) started to confuse
> > me slightly. But if I understand it right (and I never got around to
> > testing this) we can replace the do_schedule() stuffs at least with
> > something like kgdb_get_pt_regs(), since (and I lost my notes on this,
> > so it's probably not quite right) (thread_info->esp0)-1
>
> No, that's the user space registers.
>
> You don't need these registers really as long as you have the
> correct dwarf2 CFI description of all the code involved. gdb
> is then able to reconstruct them using the C stack by itself.
>
> All it needs for that is esp and eip.

Ah, OK. Amit, how about for the expanded T-packet thing, we do what you
suggested, except call it kgdb_arch_extra_regs() and let the arch fill
out whatever regs it needs to. ESP/EIP for x86_64/i386, PC/SP for
ppc32, etc, etc?

--
Tom Rini
http://gate.crashing.org/~trini/

2004-03-08 22:15:48

by George Anzinger

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

Amit S. Kale wrote:
> On Monday 08 Mar 2004 3:56 pm, Andrew Morton wrote:
>
>>"Amit S. Kale" <[email protected]> wrote:
>>
>>>Here are features that are present only in full kgdb:
>>> 1. Thread support (aka info threads)
>>
>>argh, disaster. I discussed this with Tom a week or so ago when it looked
>>like this it was being chopped out and I recall being told that the
>>discussion was referring to something else.
>>
>>Ho-hum, sorry. Can we please put this back in?
>
>
> Err., well this is one of the particularly dirty parts of kgdb. That's why
> it's been kept away. It takes care of correct thread backtraces in some rare
> cases.
>
> If you consider it an absolutely must, we can do something so that the dirty
> part is kept away and info threads almost always works.
>

Amit,

I think we should just put the info threads in the core. No attempt to do any
trace back from kgdb. Let them all show up in the switch code. I have a script
(gdb macro) that will give a rather decent "info threads" display. Oh, we need
to add one other responce to kgdb for the process info gdb command.

* This query allows the target stub to return an arbitrary string
* (or strings) giving arbitrary information about the target process.
* This is optional; the target stub isn't required to implement it.
*
* Syntax: qfProcessInfo request first string
* qsProcessInfo request subsequent string
* reply: 'O'<hex-encoded-string>
* 'l' last reply (empty)
*/
What we want here is the thread name.

Here is the macro set:

set var $low_sched=0

define do_threads
if (void)$low_sched==(void)0
set_b
end
thread apply all do_th_lines
end

define do_th_lines
set var $do_th_co=0
while ($pc > $low_sched) && ($pc < $high_sched)
up-silent
set var $do_th_co=$do_th_co+1
end
if $do_th_co==0
info remote-process
bt
else
up-silent
info remote-process
down
end
end

define set_b
set var $low_sched=scheduling_functions_start_here
set var $high_sched=scheduling_functions_end_here
end


~

--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

2004-03-08 22:19:34

by George Anzinger

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

Amit S. Kale wrote:
> On Monday 08 Mar 2004 4:50 pm, Amit S. Kale wrote:
>
>>On Monday 08 Mar 2004 4:37 pm, Andrew Morton wrote:
>>
>>>"Amit S. Kale" <[email protected]> wrote:
>>>
>>>>On Monday 08 Mar 2004 3:56 pm, Andrew Morton wrote:
>>>> > "Amit S. Kale" <[email protected]> wrote:
>>>> > > Here are features that are present only in full kgdb:
>>>> > > 1. Thread support (aka info threads)
>>>> >
>>>> > argh, disaster. I discussed this with Tom a week or so ago when it
>>>> > looked like this it was being chopped out and I recall being told
>>>> > that the discussion was referring to something else.
>>>> >
>>>> > Ho-hum, sorry. Can we please put this back in?
>>>>
>>>> Err., well this is one of the particularly dirty parts of kgdb. That's
>>>>why it's been kept away. It takes care of correct thread backtraces in
>>>>some rare cases.
>>>
>>>Let me just make sure we're taking about the same thing here. Are you
>>>saying that with kgdb-lite, `info threads' is completely missing, or does
>>>it just not work correctly with threads (as opposed to heavyweight
>>>processes)?
>>
>>info threads shows a list of threads. Heavy/light weight processes doesn't
>>matter. Thread frame shown is incorrect.
>>
>>I looked at i386 dependent code again. Following code in it is incorrect. I
>>never noticed it because this code is rarely used in full version of kgdb:
>>
>>+void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct
>>task_struct *p)
>>....
>>+ gdb_regs[_EBP] = *(int *)p->thread.esp;
>>
>>We can't guss ebp this way. This line should be removed.
>>
>>+ gdb_regs[_DS] = __KERNEL_DS;
>>+ gdb_regs[_ES] = __KERNEL_DS;
>>+ gdb_regs[_PS] = 0;
>>+ gdb_regs[_CS] = __KERNEL_CS;
>>+ gdb_regs[_PC] = p->thread.eip;
>>+ gdb_regs[_ESP] = p->thread.esp;
>>
>>This should be gdb_regs[_ESP] = &p->thread.esp
>
>
> That's not correct it. It should be gdb_regs[_ESP] = p->thread.esp;
> Even with these changes I can't get thread listing correctly.
>
> Here is the intrusive piece of code that helps get thread state correctly. Any
> ideas on cleaning it?

I wonder what version of gdb you are using. What is it that you see?

You really do need a gdb that handles the dwarft2 frames. This is a rather new
gdb (I use the CVS version).

-g
~

--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

2004-03-08 22:21:59

by George Anzinger

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

Andrew Morton wrote:
> "Amit S. Kale" <[email protected]> wrote:
>
>>>Let me just make sure we're taking about the same thing here. Are you
>>>saying that with kgdb-lite, `info threads' is completely missing, or does
>>>it just not work correctly with threads (as opposed to heavyweight
>>>processes)?
>>
>>info threads shows a list of threads. Heavy/light weight processes doesn't
>>matter. Thread frame shown is incorrect.
>
>
> It is? I haven't noticed any problems with it here. George recently
> changed it to also display the process name in the gdb output, which is
> valuable.
>
>
>>I looked at i386 dependent code again. Following code in it is incorrect. I
>>never noticed it because this code is rarely used in full version of kgdb:
>>
>>+void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct
>>*p)
>
>
> There is no such function in the stub in -mm kernels.
>
>
>>Present threads support code changes calling convention of do_IRQ. Most
>>believe that to be an absolute no.
>
>
> I see no such change in George's stub, unless I'm missing something again.

Amit and I went after the problem in rather different ways...
>
>
>>Since you consider it a must-have, I'll check whether above changes suggested
>>by me make info threads listing correct in most cases.
>
>
> The only problem I have with it is that sometimes after listing all threads
> the debugger can lose control of the target and will start complaining
> about communication errors. I assume the target has died. This happens
> very rarely. Usually when you're about to find the bug ;)

Yeah, you won't believe how much work it took to detect that ;)

--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

2004-03-08 22:26:24

by George Anzinger

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

Tom Rini wrote:
> On Mon, Mar 08, 2004 at 05:14:05PM +0530, Amit S. Kale wrote:
>
>>On Monday 08 Mar 2004 4:50 pm, Amit S. Kale wrote:
>>
>>>On Monday 08 Mar 2004 4:37 pm, Andrew Morton wrote:
>>>
>>>>"Amit S. Kale" <[email protected]> wrote:
>>>>
>>>>>On Monday 08 Mar 2004 3:56 pm, Andrew Morton wrote:
>>>>> > "Amit S. Kale" <[email protected]> wrote:
>>>>> > > Here are features that are present only in full kgdb:
>>>>> > > 1. Thread support (aka info threads)
>>>>> >
>>>>> > argh, disaster. I discussed this with Tom a week or so ago when it
>>>>> > looked like this it was being chopped out and I recall being told
>>>>> > that the discussion was referring to something else.
>>>>> >
>>>>> > Ho-hum, sorry. Can we please put this back in?
>>>>>
>>>>> Err., well this is one of the particularly dirty parts of kgdb. That's
>>>>>why it's been kept away. It takes care of correct thread backtraces in
>>>>>some rare cases.
>>>>
>>>>Let me just make sure we're taking about the same thing here. Are you
>>>>saying that with kgdb-lite, `info threads' is completely missing, or does
>>>>it just not work correctly with threads (as opposed to heavyweight
>>>>processes)?
>>>
>>>info threads shows a list of threads. Heavy/light weight processes doesn't
>>>matter. Thread frame shown is incorrect.
>>>
>>>I looked at i386 dependent code again. Following code in it is incorrect. I
>>>never noticed it because this code is rarely used in full version of kgdb:
>>>
>>>+void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct
>>>task_struct *p)
>>>....
>>>+ gdb_regs[_EBP] = *(int *)p->thread.esp;
>>>
>>>We can't guss ebp this way. This line should be removed.
>>>
>>>+ gdb_regs[_DS] = __KERNEL_DS;
>>>+ gdb_regs[_ES] = __KERNEL_DS;
>>>+ gdb_regs[_PS] = 0;
>>>+ gdb_regs[_CS] = __KERNEL_CS;
>>>+ gdb_regs[_PC] = p->thread.eip;
>>>+ gdb_regs[_ESP] = p->thread.esp;
>>>
>>>This should be gdb_regs[_ESP] = &p->thread.esp
>>
>>That's not correct it. It should be gdb_regs[_ESP] = p->thread.esp;
>>Even with these changes I can't get thread listing correctly.
>>
>>Here is the intrusive piece of code that helps get thread state correctly. Any
>>ideas on cleaning it?
>
>
> Here's where what Andi said about being able to get the pt_regs stuff
> off the stack (I think that's what he said at least) started to confuse
> me slightly. But if I understand it right (and I never got around to
> testing this) we can replace the do_schedule() stuffs at least with
> something like kgdb_get_pt_regs(), since (and I lost my notes on this,
> so it's probably not quite right) (thread_info->esp0)-1

I don't see any problem here. All I do is to detect that the thread is not
current on ANY cpu and send up the regs that switch to saved in the task struct.
GDB can then figure out the rest from the dwarft2 frames. At this time it
helps to have the compile option to do frames turned on. This is so that gdb
will use the frame register to back out of switch to. It all works well in my kgdb.
>

--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

2004-03-08 22:29:20

by George Anzinger

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

Tom Rini wrote:
> On Mon, Mar 08, 2004 at 04:50:18PM +0530, Amit S. Kale wrote:
>
>>On Monday 08 Mar 2004 4:37 pm, Andrew Morton wrote:
>>
>>>"Amit S. Kale" <[email protected]> wrote:
>
> [snip]
>
>>>> If you consider it an absolutely must, we can do something so that the
>>>>dirty part is kept away and info threads almost always works.
>>>
>>>Yes, I'd consider `info threads' support a must-have. I'm rather surprised
>>>that others do not?
>>
>>Present threads support code changes calling convention of do_IRQ. Most
>>believe that to be an absolute no.
>
>
> I believe that George's version does something totally different, with
> some macros at compile time (and binutils support, I _think_) to not
> have to change do_IRQ.

No, nothing at compile time, at least WRT the threads issue. There is a
completely different problem with backtracing through an interrupt or trap. I
have sent the patch for that which makes only minimal changes to code (one line
I think, and that an asm line). The rest is a dwarft2 set of code to build the
frame description for the trap/interrupt frame.

>

--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

2004-03-09 04:29:43

by Amit S. Kale

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

On Monday 08 Mar 2004 10:48 pm, Tom Rini wrote:
> On Mon, Mar 08, 2004 at 05:57:26PM +0100, Andi Kleen wrote:
> > Tom Rini <[email protected]> writes:
> > > Here's where what Andi said about being able to get the pt_regs stuff
> > > off the stack (I think that's what he said at least) started to confuse
> > > me slightly. But if I understand it right (and I never got around to
> > > testing this) we can replace the do_schedule() stuffs at least with
> > > something like kgdb_get_pt_regs(), since (and I lost my notes on this,
> > > so it's probably not quite right) (thread_info->esp0)-1
> >
> > No, that's the user space registers.
> >
> > You don't need these registers really as long as you have the
> > correct dwarf2 CFI description of all the code involved. gdb
> > is then able to reconstruct them using the C stack by itself.
> >
> > All it needs for that is esp and eip.
>
> Ah, OK. Amit, how about for the expanded T-packet thing, we do what you
> suggested, except call it kgdb_arch_extra_regs() and let the arch fill
> out whatever regs it needs to. ESP/EIP for x86_64/i386, PC/SP for
> ppc32, etc, etc?

That'll be great, though it won't provide any more info to gdb, as gdb can get
that info by 'g' packet.

Thread listing problem is with 'g' packet returning incomplete information for
threads other than the current thread.

-Amit

2004-03-09 04:36:18

by Amit S. Kale

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

On Monday 08 Mar 2004 10:27 pm, Andi Kleen wrote:
> Tom Rini <[email protected]> writes:
> > Here's where what Andi said about being able to get the pt_regs stuff
> > off the stack (I think that's what he said at least) started to confuse
> > me slightly. But if I understand it right (and I never got around to
> > testing this) we can replace the do_schedule() stuffs at least with
> > something like kgdb_get_pt_regs(), since (and I lost my notes on this,
> > so it's probably not quite right) (thread_info->esp0)-1
>
> No, that's the user space registers.
>
> You don't need these registers really as long as you have the
> correct dwarf2 CFI description of all the code involved. gdb
> is then able to reconstruct them using the C stack by itself.
>
> All it needs for that is esp and eip.

Yes. But as things stand, gdb 6.0 doesn't show stack traces correctly with esp
and eip got from switch_to and gas 2.14 can't handle i386 dwarf2 CFI. Do we
want to enforce getting a CVS version of gdb _and_ gas to build kgdb?
Certainly not.

Current kgdb has a dependency on gdb 6.0. My RH9 gdb (5.3.x) can't talk with
kgdb and complains about too long [sf]ThreadInfo packets. I don't like that
either, but gdb 6.x should be standard on all distributions based on 2.6
kernels, so that's acceptable.

-Amit

2004-03-09 04:46:58

by Amit S. Kale

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

On Tuesday 09 Mar 2004 3:45 am, George Anzinger wrote:
> Amit S. Kale wrote:
> > On Monday 08 Mar 2004 3:56 pm, Andrew Morton wrote:
> >>"Amit S. Kale" <[email protected]> wrote:
> >>>Here are features that are present only in full kgdb:
> >>> 1. Thread support (aka info threads)
> >>
> >>argh, disaster. I discussed this with Tom a week or so ago when it
> >> looked like this it was being chopped out and I recall being told that
> >> the discussion was referring to something else.
> >>
> >>Ho-hum, sorry. Can we please put this back in?
> >
> > Err., well this is one of the particularly dirty parts of kgdb. That's
> > why it's been kept away. It takes care of correct thread backtraces in
> > some rare cases.
> >
> > If you consider it an absolutely must, we can do something so that the
> > dirty part is kept away and info threads almost always works.
>
> Amit,
>
> I think we should just put the info threads in the core. No attempt to do
> any trace back from kgdb. Let them all show up in the switch code. I have
> a script (gdb macro) that will give a rather decent "info threads" display.
> Oh, we need to add one other responce to kgdb for the process info gdb
> command.

qfProcessInfo looks like a very useful command.

This is still a hack. These kinds of hacks are difficult to maintain over a
long time because they are based on undocumented information.

While setting up kgdb for the first time isn't easy, macros add to that
complexity.

Have you experienced "shakyness" of macros? I have used process information
macros at kgdb.sourceforge.net to display a "ps" command like output since
the days of 2.4.0. I had to change them 3-4 times till now because of kernel
structure changes. The "changing" part is typically a 2-3 hour regorous
exercise :-)

-Amit

> * This query allows the target stub to return an arbitrary string
> * (or strings) giving arbitrary information about the target process.
> * This is optional; the target stub isn't required to implement it.
> *
> * Syntax: qfProcessInfo request first string
> * qsProcessInfo request subsequent string
> * reply: 'O'<hex-encoded-string>
> * 'l' last reply (empty)
> */
> What we want here is the thread name.
>
> Here is the macro set:
>
> set var $low_sched=0
>
> define do_threads
> if (void)$low_sched==(void)0
> set_b
> end
> thread apply all do_th_lines
> end
>
> define do_th_lines
> set var $do_th_co=0
> while ($pc > $low_sched) && ($pc < $high_sched)
> up-silent
> set var $do_th_co=$do_th_co+1
> end
> if $do_th_co==0
> info remote-process
> bt
> else
> up-silent
> info remote-process
> down
> end
> end
>
> define set_b
> set var $low_sched=scheduling_functions_start_here
> set var $high_sched=scheduling_functions_end_here
> end
>
>
> ~

2004-03-09 05:09:24

by Amit S. Kale

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

On Tuesday 09 Mar 2004 3:49 am, George Anzinger wrote:
> Amit S. Kale wrote:
> > On Monday 08 Mar 2004 4:50 pm, Amit S. Kale wrote:
> >>On Monday 08 Mar 2004 4:37 pm, Andrew Morton wrote:
> >>>"Amit S. Kale" <[email protected]> wrote:
> >>>>On Monday 08 Mar 2004 3:56 pm, Andrew Morton wrote:
> >>>> > "Amit S. Kale" <[email protected]> wrote:
> >>>> > > Here are features that are present only in full kgdb:
> >>>> > > 1. Thread support (aka info threads)
> >>>> >
> >>>> > argh, disaster. I discussed this with Tom a week or so ago when it
> >>>> > looked like this it was being chopped out and I recall being told
> >>>> > that the discussion was referring to something else.
> >>>> >
> >>>> > Ho-hum, sorry. Can we please put this back in?
> >>>>
> >>>> Err., well this is one of the particularly dirty parts of kgdb. That's
> >>>>why it's been kept away. It takes care of correct thread backtraces in
> >>>>some rare cases.
> >>>
> >>>Let me just make sure we're taking about the same thing here. Are you
> >>>saying that with kgdb-lite, `info threads' is completely missing, or
> >>> does it just not work correctly with threads (as opposed to heavyweight
> >>> processes)?
> >>
> >>info threads shows a list of threads. Heavy/light weight processes
> >> doesn't matter. Thread frame shown is incorrect.
> >>
> >>I looked at i386 dependent code again. Following code in it is incorrect.
> >> I never noticed it because this code is rarely used in full version of
> >> kgdb:
> >>
> >>+void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct
> >>task_struct *p)
> >>....
> >>+ gdb_regs[_EBP] = *(int *)p->thread.esp;
> >>
> >>We can't guss ebp this way. This line should be removed.
> >>
> >>+ gdb_regs[_DS] = __KERNEL_DS;
> >>+ gdb_regs[_ES] = __KERNEL_DS;
> >>+ gdb_regs[_PS] = 0;
> >>+ gdb_regs[_CS] = __KERNEL_CS;
> >>+ gdb_regs[_PC] = p->thread.eip;
> >>+ gdb_regs[_ESP] = p->thread.esp;
> >>
> >>This should be gdb_regs[_ESP] = &p->thread.esp
> >
> > That's not correct it. It should be gdb_regs[_ESP] = p->thread.esp;
> > Even with these changes I can't get thread listing correctly.
> >
> > Here is the intrusive piece of code that helps get thread state
> > correctly. Any ideas on cleaning it?
>
> I wonder what version of gdb you are using. What is it that you see?

I am pasting below the output I see with
gdb_regs[_PC] = p->thread.eip;
gdb_regs[_ESP] = p->thread.esp;
providing thread state information.

>
> You really do need a gdb that handles the dwarft2 frames. This is a rather
> new gdb (I use the CVS version).

Let's just stick to gdb 6.0 and binutils 2.14. CVS versions of gdb and
binutils are too risky for someone who is trying to learn linux kernel by
using kgdb.

Let's face it, more people use kgdb to _learn_ some part of the kernel than
kernel gurus :-)

-Amit

(gdb) info thr
42 Thread 32768 (Shadow task 0 for pid 0) 0xc011a27f in schedule ()
at /home/amitkale/work/linux-2.6.4-rc2-bk3-kgdb/kernel/sched.c:920
41 Thread 1129 (cupsd) 0xc1146000 in ?? ()
40 Thread 1128 (initlog) 0xc1146000 in ?? ()
39 Thread 1123 (S90cups) 0xc1146000 in ?? ()
38 Thread 1119 (crond) 0xc1146000 in ?? ()
37 Thread 1110 (gpm) 0xc1146000 in ?? ()
36 Thread 1100 (sendmail) 0xc1146000 in ?? ()
35 Thread 1090 (sendmail) 0xc1146000 in ?? ()
34 Thread 1068 (rpc.mountd) 0xc1146000 in ?? ()
33 Thread 1062 (rpciod) 0xc1146000 in ?? ()
32 Thread 1061 (lockd) 0xc1146000 in ?? ()
31 Thread 1060 (nfsd) 0xc1146000 in ?? ()
30 Thread 1059 (nfsd) 0xc1146000 in ?? ()
29 Thread 1058 (nfsd) 0xc1146000 in ?? ()
28 Thread 1057 (nfsd) 0xc1146000 in ?? ()
27 Thread 1056 (nfsd) 0xc1146000 in ?? ()
26 Thread 1055 (nfsd) 0xc1146000 in ?? ()
25 Thread 1054 (nfsd) 0xc1146000 in ?? ()
24 Thread 1053 (nfsd) 0xc1146000 in ?? ()
23 Thread 1049 (rpc.rquotad) 0xc1146000 in ?? ()
22 Thread 1036 (xinetd) 0xc1146000 in ?? ()
21 Thread 1021 (sshd) 0xc1146000 in ?? ()
---Type <return> to continue, or q <return> to quit---
20 Thread 925 (rpc.statd) 0xc1146000 in ?? ()
19 Thread 906 (portmap) 0xc1146000 in ?? ()
18 Thread 888 (klogd) 0xc1146000 in ?? ()
17 Thread 884 (syslogd) 0xc1146000 in ?? ()
16 Thread 706 (rc) 0xc1146000 in ?? ()
15 Thread 626 (kjournald) 0xc1146000 in ?? ()
14 Thread 625 (kjournald) 0xc1146000 in ?? ()
13 Thread 624 (kjournald) 0xc1146000 in ?? ()
12 Thread 11 (kjournald) 0xc1146000 in ?? ()
11 Thread 10 (kseriod) 0xc011a27f in schedule ()
at /home/amitkale/work/linux-2.6.4-rc2-bk3-kgdb/kernel/sched.c:920
10 Thread 9 (aio/0) 0xc011a27f in schedule ()
at /home/amitkale/work/linux-2.6.4-rc2-bk3-kgdb/kernel/sched.c:920
9 Thread 8 (kswapd0) 0xc011a27f in schedule ()
at /home/amitkale/work/linux-2.6.4-rc2-bk3-kgdb/kernel/sched.c:920
8 Thread 7 (pdflush) 0xc011a27f in schedule ()
at /home/amitkale/work/linux-2.6.4-rc2-bk3-kgdb/kernel/sched.c:920
7 Thread 6 (pdflush) 0xc011a27f in schedule ()
at /home/amitkale/work/linux-2.6.4-rc2-bk3-kgdb/kernel/sched.c:920
6 Thread 5 (khubd) 0xc011a27f in schedule ()
at /home/amitkale/work/linux-2.6.4-rc2-bk3-kgdb/kernel/sched.c:920
5 Thread 4 (kblockd/0) 0xc011a27f in schedule ()
at /home/amitkale/work/linux-2.6.4-rc2-bk3-kgdb/kernel/sched.c:920
---Type <return> to continue, or q <return> to quit---
4 Thread 3 (events/0) 0xc011a27f in schedule ()
at /home/amitkale/work/linux-2.6.4-rc2-bk3-kgdb/kernel/sched.c:920
3 Thread 2 (ksoftirqd/0) 0xc011a27f in schedule ()
at /home/amitkale/work/linux-2.6.4-rc2-bk3-kgdb/kernel/sched.c:920
2 Thread 1 (init) 0xc1146000 in ?? ()
* 1 Thread 1130 (cupsd) breakpoint ()
at /home/amitkale/work/linux-2.6.4-rc2-bk3-kgdb/kernel/kgdb.c:1088
(gdb) thr 11
[Switching to thread 11 (Thread 10)]#0 0xc011a27f in schedule ()
at /home/amitkale/work/linux-2.6.4-rc2-bk3-kgdb/kernel/sched.c:920
920 switch_to(prev, next, prev);
(gdb) bt
#0 0xc011a27f in schedule ()
at /home/amitkale/work/linux-2.6.4-rc2-bk3-kgdb/kernel/sched.c:920
Cannot access memory at address 0x4
(gdb) info fr
Stack level 0, frame at 0x8:
eip = 0xc011a27f in schedule
(/home/amitkale/work/linux-2.6.4-rc2-bk3-kgdb/kernel/sched.c:920);
saved eip Cannot access memory at address 0x4
(gdb) info regi
eax 0x0 0
ecx 0x0 0
edx 0x0 0
ebx 0x0 0
esp 0xc7da3f5c 0xc7da3f5c
ebp 0x0 0x0
esi 0x0 0
edi 0x0 0
eip 0xc011a27f 0xc011a27f
eflags 0x0 0
cs 0x60 96
ss 0x68 104
ds 0x68 104
es 0x68 104
fs 0xffff 65535
gs 0xffff 65535
(gdb) disas 0xc011a270 0xc011a288
Dump of assembler code from 0xc011a270 to 0xc011a288:
0xc011a270 <schedule+784>: jg 0xc011a214 <schedule+692>
0xc011a272 <schedule+786>: adc %eax,%eax
0xc011a274 <schedule+788>: pushl 0x350(%esi)
0xc011a27a <schedule+794>: jmp 0xc01079d0 <__switch_to>
0xc011a27f <schedule+799>: pop %ebp
0xc011a280 <schedule+800>: popf
0xc011a281 <schedule+801>: mov %eax,0xffffffd0(%ebp)
0xc011a284 <schedule+804>: xor %edx,%edx
0xc011a286 <schedule+806>: mov 0xc04b8edc,%esi
End of assembler dump.
(gdb)

2004-03-09 08:59:05

by Amit S. Kale

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

On Monday 08 Mar 2004 8:49 pm, Tom Rini wrote:
> On Mon, Mar 08, 2004 at 05:14:05PM +0530, Amit S. Kale wrote:
> > On Monday 08 Mar 2004 4:50 pm, Amit S. Kale wrote:
> > > On Monday 08 Mar 2004 4:37 pm, Andrew Morton wrote:
> > > > "Amit S. Kale" <[email protected]> wrote:
> > > > > On Monday 08 Mar 2004 3:56 pm, Andrew Morton wrote:
> > > > > > "Amit S. Kale" <[email protected]> wrote:
> > > > > > > Here are features that are present only in full kgdb:
> > > > > > > 1. Thread support (aka info threads)
> > > > > >
> > > > > > argh, disaster. I discussed this with Tom a week or so ago when
> > > > > > it looked like this it was being chopped out and I recall being
> > > > > > told that the discussion was referring to something else.
> > > > > >
> > > > > > Ho-hum, sorry. Can we please put this back in?
> > > > >
> > > > > Err., well this is one of the particularly dirty parts of kgdb.
> > > > > That's why it's been kept away. It takes care of correct thread
> > > > > backtraces in some rare cases.
> > > >
> > > > Let me just make sure we're taking about the same thing here. Are
> > > > you saying that with kgdb-lite, `info threads' is completely missing,
> > > > or does it just not work correctly with threads (as opposed to
> > > > heavyweight processes)?
> > >
> > > info threads shows a list of threads. Heavy/light weight processes
> > > doesn't matter. Thread frame shown is incorrect.
> > >
> > > I looked at i386 dependent code again. Following code in it is
> > > incorrect. I never noticed it because this code is rarely used in full
> > > version of kgdb:
> > >
> > > +void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct
> > > task_struct *p)
> > > ....
> > > + gdb_regs[_EBP] = *(int *)p->thread.esp;
> > >
> > > We can't guss ebp this way. This line should be removed.
> > >
> > > + gdb_regs[_DS] = __KERNEL_DS;
> > > + gdb_regs[_ES] = __KERNEL_DS;
> > > + gdb_regs[_PS] = 0;
> > > + gdb_regs[_CS] = __KERNEL_CS;
> > > + gdb_regs[_PC] = p->thread.eip;
> > > + gdb_regs[_ESP] = p->thread.esp;
> > >
> > > This should be gdb_regs[_ESP] = &p->thread.esp
> >
> > That's not correct it. It should be gdb_regs[_ESP] = p->thread.esp;
> > Even with these changes I can't get thread listing correctly.
> >
> > Here is the intrusive piece of code that helps get thread state
> > correctly. Any ideas on cleaning it?
>
> Here's where what Andi said about being able to get the pt_regs stuff
> off the stack (I think that's what he said at least) started to confuse
> me slightly. But if I understand it right (and I never got around to

That's an in interesting idea. We can at least get first pt_regs from the
stack. That way we can get to user space eip in case a thread is in user
space.

The do_schedule and save all regs is definitely required if we want _exact_
state of all kernel threads. [exact state: all registers]

Finding function frames below pt_regs becomes difficult or impossible, as
exact dept of stack for each function in a stack is not known. EBP chains
allow walking "up" but not "down".
-Amit


> testing this) we can replace the do_schedule() stuffs at least with
> something like kgdb_get_pt_regs(), since (and I lost my notes on this,
> so it's probably not quite right) (thread_info->esp0)-1

2004-03-09 09:34:24

by Amit S. Kale

[permalink] [raw]
Subject: i386-lite [patch 2/3] [Re: kgdb for mainline kernel: core-lite [patch 1/3]]

i386 part with this email
-Amit

On Tuesday 09 Mar 2004 2:59 pm, Amit S. Kale wrote:
> I attempted it and found that it works better than my expectation! I am
> attaching revised core-lite.patch with this email and sending
> i386-lite.patch as a reply.

Index: linux-2.6.4-rc2-bk3-kgdb/arch/i386/Kconfig
===================================================================
--- linux-2.6.4-rc2-bk3-kgdb.orig/arch/i386/Kconfig 2004-03-08
14:30:07.000000000 +0530
+++ linux-2.6.4-rc2-bk3-kgdb/arch/i386/Kconfig 2004-03-09 14:16:55.226362584
+0530
@@ -1253,6 +1253,8 @@
If you say Y here, various routines which may sleep will become very
noisy if they are called with a spinlock held.

+source "kernel/Kconfig.kgdb"
+
config FRAME_POINTER
bool "Compile the kernel with frame pointers"
help
Index: linux-2.6.4-rc2-bk3-kgdb/arch/i386/kernel/Makefile
===================================================================
--- linux-2.6.4-rc2-bk3-kgdb.orig/arch/i386/kernel/Makefile 2004-03-08
14:30:06.000000000 +0530
+++ linux-2.6.4-rc2-bk3-kgdb/arch/i386/kernel/Makefile 2004-03-09
14:16:55.241360304 +0530
@@ -31,6 +31,7 @@
obj-$(CONFIG_ACPI_SRAT) += srat.o
obj-$(CONFIG_HPET_TIMER) += time_hpet.o
obj-$(CONFIG_EFI) += efi.o efi_stub.o
+obj-$(CONFIG_KGDB) += kgdb.o

EXTRA_AFLAGS := -traditional

Index: linux-2.6.4-rc2-bk3-kgdb/arch/i386/kernel/irq.c
===================================================================
--- linux-2.6.4-rc2-bk3-kgdb.orig/arch/i386/kernel/irq.c 2004-03-08
14:30:06.000000000 +0530
+++ linux-2.6.4-rc2-bk3-kgdb/arch/i386/kernel/irq.c 2004-03-09
14:16:55.242360152 +0530
@@ -34,6 +34,7 @@
#include <linux/proc_fs.h>
#include <linux/seq_file.h>
#include <linux/kallsyms.h>
+#include <linux/kgdb.h>

#include <asm/atomic.h>
#include <asm/io.h>
@@ -508,6 +509,8 @@

irq_exit();

+ kgdb_process_breakpoint();
+
return 1;
}

Index: linux-2.6.4-rc2-bk3-kgdb/arch/i386/kernel/kgdb.c
===================================================================
--- linux-2.6.4-rc2-bk3-kgdb.orig/arch/i386/kernel/kgdb.c 2003-01-30
15:54:37.000000000 +0530
+++ linux-2.6.4-rc2-bk3-kgdb/arch/i386/kernel/kgdb.c 2004-03-09
14:19:34.200194888 +0530
@@ -0,0 +1,380 @@
+/*
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2, or (at your option) any
+ * later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ */
+
+/*
+ * Copyright (C) 2000-2001 VERITAS Software Corporation.
+ */
+/*
+ * Contributor: Lake Stevens Instrument Division$
+ * Written by: Glenn Engel $
+ * Updated by: Amit Kale<[email protected]>
+ * Modified for 386 by Jim Kingdon, Cygnus Support.
+ * Origianl kgdb, compatibility with 2.1.xx kernel by David Grothe
<[email protected]>
+ */
+
+#include <linux/string.h>
+#include <linux/kernel.h>
+#include <linux/sched.h>
+#include <linux/smp.h>
+#include <linux/spinlock.h>
+#include <linux/delay.h>
+#include <asm/vm86.h>
+#include <asm/system.h>
+#include <asm/ptrace.h> /* for linux pt_regs struct */
+#include <linux/kgdb.h>
+#ifdef CONFIG_GDB_CONSOLE
+#include <linux/console.h>
+#endif
+#include <linux/init.h>
+#include <linux/debugger.h>
+
+/* Put the error code here just in case the user cares. */
+int gdb_i386errcode;
+/* Likewise, the vector number here (since GDB only gets the signal
+ number through the usual means, and that's not very specific). */
+int gdb_i386vector = -1;
+
+#if KGDB_MAX_NO_CPUS != 8
+#error change the definition of slavecpulocks
+#endif
+
+void regs_to_gdb_regs(unsigned long *gdb_regs, struct pt_regs *regs)
+{
+ gdb_regs[_EAX] = regs->eax;
+ gdb_regs[_EBX] = regs->ebx;
+ gdb_regs[_ECX] = regs->ecx;
+ gdb_regs[_EDX] = regs->edx;
+ gdb_regs[_ESI] = regs->esi;
+ gdb_regs[_EDI] = regs->edi;
+ gdb_regs[_EBP] = regs->ebp;
+ gdb_regs[_DS] = regs->xds;
+ gdb_regs[_ES] = regs->xes;
+ gdb_regs[_PS] = regs->eflags;
+ gdb_regs[_CS] = regs->xcs;
+ gdb_regs[_PC] = regs->eip;
+ gdb_regs[_ESP] = (int)(&regs->esp);
+ gdb_regs[_SS] = __KERNEL_DS;
+ gdb_regs[_FS] = 0xFFFF;
+ gdb_regs[_GS] = 0xFFFF;
+}
+
+/*
+ * Extracts ebp, esp and eip values understandable by gdb from the values
+ * saved by switch_to.
+ * thread.esp points to ebp. flags and ebp are pushed in switch_to hence esp
+ * prior to entering switch_to is 8 greater then the value that is saved.
+ * If switch_to changes, change following code appropriately.
+ */
+void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct
*p)
+{
+ gdb_regs[_EAX] = 0;
+ gdb_regs[_EBX] = 0;
+ gdb_regs[_ECX] = 0;
+ gdb_regs[_EDX] = 0;
+ gdb_regs[_ESI] = 0;
+ gdb_regs[_EDI] = 0;
+ gdb_regs[_EBP] = *(unsigned long *)p->thread.esp;
+ gdb_regs[_DS] = __KERNEL_DS;
+ gdb_regs[_ES] = __KERNEL_DS;
+ gdb_regs[_PS] = 0;
+ gdb_regs[_CS] = __KERNEL_CS;
+ gdb_regs[_PC] = p->thread.eip;
+ gdb_regs[_ESP] = p->thread.esp + 8;
+ gdb_regs[_SS] = __KERNEL_DS;
+ gdb_regs[_FS] = 0xFFFF;
+ gdb_regs[_GS] = 0xFFFF;
+}
+
+void gdb_regs_to_regs(unsigned long *gdb_regs, struct pt_regs *regs)
+{
+ regs->eax = gdb_regs[_EAX];
+ regs->ebx = gdb_regs[_EBX];
+ regs->ecx = gdb_regs[_ECX];
+ regs->edx = gdb_regs[_EDX];
+ regs->esi = gdb_regs[_ESI];
+ regs->edi = gdb_regs[_EDI];
+ regs->ebp = gdb_regs[_EBP];
+ regs->xds = gdb_regs[_DS];
+ regs->xes = gdb_regs[_ES];
+ regs->eflags = gdb_regs[_PS];
+ regs->xcs = gdb_regs[_CS];
+ regs->eip = gdb_regs[_PC];
+#if 0 /* can't change these */
+ regs->esp = gdb_regs[_ESP];
+ regs->xss = gdb_regs[_SS];
+ regs->fs = gdb_regs[_FS];
+ regs->gs = gdb_regs[_GS];
+#endif
+
+}
+
+struct hw_breakpoint {
+ unsigned enabled;
+ unsigned type;
+ unsigned len;
+ unsigned addr;
+} breakinfo[4] = { {
+enabled:0}, {
+enabled:0}, {
+enabled:0}, {
+enabled:0}};
+
+void kgdb_correct_hw_break(void)
+{
+ int breakno;
+ int correctit;
+ int breakbit;
+ unsigned dr7;
+
+ asm volatile ("movl %%db7, %0\n":"=r" (dr7)
+ :);
+ do {
+ unsigned addr0, addr1, addr2, addr3;
+ asm volatile ("movl %%db0, %0\n"
+ "movl %%db1, %1\n"
+ "movl %%db2, %2\n"
+ "movl %%db3, %3\n":"=r" (addr0), "=r"(addr1),
+ "=r"(addr2), "=r"(addr3):);
+ } while (0);
+ correctit = 0;
+ for (breakno = 0; breakno < 3; breakno++) {
+ breakbit = 2 << (breakno << 1);
+ if (!(dr7 & breakbit) && breakinfo[breakno].enabled) {
+ correctit = 1;
+ dr7 |= breakbit;
+ dr7 &= ~(0xf0000 << (breakno << 2));
+ dr7 |= (((breakinfo[breakno].len << 2) |
+ breakinfo[breakno].type) << 16) <<
+ (breakno << 2);
+ switch (breakno) {
+ case 0:
+ asm volatile ("movl %0, %%dr0\n"::"r"
+ (breakinfo[breakno].addr));
+ break;
+
+ case 1:
+ asm volatile ("movl %0, %%dr1\n"::"r"
+ (breakinfo[breakno].addr));
+ break;
+
+ case 2:
+ asm volatile ("movl %0, %%dr2\n"::"r"
+ (breakinfo[breakno].addr));
+ break;
+
+ case 3:
+ asm volatile ("movl %0, %%dr3\n"::"r"
+ (breakinfo[breakno].addr));
+ break;
+ }
+ } else if ((dr7 & breakbit) && !breakinfo[breakno].enabled) {
+ correctit = 1;
+ dr7 &= ~breakbit;
+ dr7 &= ~(0xf0000 << (breakno << 2));
+ }
+ }
+ if (correctit) {
+ asm volatile ("movl %0, %%db7\n"::"r" (dr7));
+ }
+}
+
+int kgdb_remove_hw_break(unsigned long addr, int type)
+{
+ int i, idx = -1;
+ for (i = 0; i < 4; i++) {
+ if (breakinfo[i].addr == addr && breakinfo[i].enabled) {
+ idx = i;
+ break;
+ }
+ }
+ if (idx == -1)
+ return -1;
+
+ breakinfo[idx].enabled = 0;
+ return 0;
+}
+
+int kgdb_set_hw_break(unsigned long addr, int type)
+{
+ int i, idx = -1;
+ for (i = 0; i < 4; i++) {
+ if (!breakinfo[i].enabled) {
+ idx = i;
+ break;
+ }
+ }
+ if (idx == -1)
+ return -1;
+
+ breakinfo[idx].enabled = 1;
+ breakinfo[idx].type = type;
+ breakinfo[idx].len = 1;
+ breakinfo[idx].addr = addr;
+ return 0;
+}
+
+int remove_hw_break(unsigned breakno)
+{
+ if (!breakinfo[breakno].enabled) {
+ return -1;
+ }
+ breakinfo[breakno].enabled = 0;
+ return 0;
+}
+
+int set_hw_break(unsigned breakno, unsigned type, unsigned len, unsigned
addr)
+{
+ if (breakinfo[breakno].enabled) {
+ return -1;
+ }
+ breakinfo[breakno].enabled = 1;
+ breakinfo[breakno].type = type;
+ breakinfo[breakno].len = len;
+ breakinfo[breakno].addr = addr;
+ return 0;
+}
+
+void kgdb_printexceptioninfo(int exceptionNo, int errorcode, char *buffer)
+{
+ unsigned dr6;
+ int i;
+ switch (exceptionNo) {
+ case 1: /* debug exception */
+ break;
+ case 3: /* breakpoint */
+ sprintf(buffer, "Software breakpoint");
+ return;
+ default:
+ sprintf(buffer, "Details not available");
+ return;
+ }
+ asm volatile ("movl %%db6, %0\n":"=r" (dr6)
+ :);
+ if (dr6 & 0x4000) {
+ sprintf(buffer, "Single step");
+ return;
+ }
+ for (i = 0; i < 4; ++i) {
+ if (dr6 & (1 << i)) {
+ sprintf(buffer, "Hardware breakpoint %d", i);
+ return;
+ }
+ }
+ sprintf(buffer, "Unknown trap");
+ return;
+}
+
+void kgdb_disable_hw_debug(struct pt_regs *regs)
+{
+ /* Disable hardware debugging while we are in kgdb */
+ asm volatile ("movl %0,%%db7": /* no output */ :"r" (0));
+}
+
+void kgdb_post_master_code(struct pt_regs *regs, int eVector, int err_code)
+{
+ /* Master processor is completely in the debugger */
+ gdb_i386vector = eVector;
+ gdb_i386errcode = err_code;
+}
+
+int kgdb_arch_handle_exception(int exceptionVector, int signo,
+ int err_code, char *remcom_in_buffer,
+ char *remcom_out_buffer,
+ struct pt_regs *linux_regs)
+{
+ long addr, length;
+ long breakno, breaktype;
+ char *ptr;
+ int newPC;
+ int dr6;
+
+ switch (remcom_in_buffer[0]) {
+ case 'c':
+ case 's':
+ if (kgdb_contthread && kgdb_contthread != current) {
+ strcpy(remcom_out_buffer, "E00");
+ break;
+ }
+
+ kgdb_contthread = NULL;
+
+ /* try to read optional parameter, pc unchanged if no parm */
+ ptr = &remcom_in_buffer[1];
+ if (kgdb_hex2long(&ptr, &addr)) {
+ linux_regs->eip = addr;
+ }
+ newPC = linux_regs->eip;
+
+ /* clear the trace bit */
+ linux_regs->eflags &= 0xfffffeff;
+
+ /* set the trace bit if we're stepping */
+ if (remcom_in_buffer[0] == 's') {
+ linux_regs->eflags |= 0x100;
+ debugger_step = 1;
+ }
+
+ asm volatile ("movl %%db6, %0\n":"=r" (dr6));
+ if (!(dr6 & 0x4000)) {
+ for (breakno = 0; breakno < 4; ++breakno) {
+ if (dr6 & (1 << breakno)) {
+ if (breakinfo[breakno].type == 0) {
+ /* Set restore flag */
+ linux_regs->eflags |= 0x10000;
+ break;
+ }
+ }
+ }
+ }
+ kgdb_correct_hw_break();
+ asm volatile ("movl %0, %%db6\n"::"r" (0));
+
+ return (0);
+
+ case 'Y':
+ ptr = &remcom_in_buffer[1];
+ kgdb_hex2long(&ptr, &breakno);
+ ptr++;
+ kgdb_hex2long(&ptr, &breaktype);
+ ptr++;
+ kgdb_hex2long(&ptr, &length);
+ ptr++;
+ kgdb_hex2long(&ptr, &addr);
+ if (set_hw_break(breakno & 0x3, breaktype & 0x3,
+ length & 0x3, addr) == 0) {
+ strcpy(remcom_out_buffer, "OK");
+ } else {
+ strcpy(remcom_out_buffer, "ERROR");
+ }
+ break;
+
+ /* Remove hardware breakpoint */
+ case 'y':
+ ptr = &remcom_in_buffer[1];
+ kgdb_hex2long(&ptr, &breakno);
+ if (remove_hw_break(breakno & 0x3) == 0) {
+ strcpy(remcom_out_buffer, "OK");
+ } else {
+ strcpy(remcom_out_buffer, "ERROR");
+ }
+ break;
+
+ } /* switch */
+ return -1; /* this means that we do not want to exit from the handler */
+}
+
+struct kgdb_arch arch_kgdb_ops = {
+ .gdb_bpt_instr = {0xcc},
+ .flags = KGDB_HW_BREAKPOINT,
+};
Index: linux-2.6.4-rc2-bk3-kgdb/arch/i386/kernel/nmi.c
===================================================================
--- linux-2.6.4-rc2-bk3-kgdb.orig/arch/i386/kernel/nmi.c 2004-03-08
14:30:06.000000000 +0530
+++ linux-2.6.4-rc2-bk3-kgdb/arch/i386/kernel/nmi.c 2004-03-09
14:16:55.260357416 +0530
@@ -25,6 +25,7 @@
#include <linux/module.h>
#include <linux/nmi.h>
#include <linux/sysdev.h>
+#include <linux/debugger.h>

#include <asm/smp.h>
#include <asm/mtrr.h>
@@ -420,14 +421,25 @@
int sum, cpu = smp_processor_id();

sum = irq_stat[cpu].apic_timer_irqs;
+ if (atomic_read(&debugger_active)) {

- if (last_irq_sums[cpu] == sum) {
+ /*
+ * The machine is in debugger, hold this cpu if already
+ * not held.
+ */
+ debugger_nmihook(cpu, regs);
+ alert_counter[cpu] = 0;
+
+ } else if (last_irq_sums[cpu] == sum) {
/*
* Ayiee, looks like this CPU is stuck ...
* wait a few IRQs (5 seconds) before doing the oops ...
*/
alert_counter[cpu]++;
if (alert_counter[cpu] == 5*nmi_hz) {
+
+ CHK_DEBUGGER(2,SIGSEGV,0,regs,)
+
spin_lock(&nmi_print_lock);
/*
* We are in trouble anyway, lets at least try
Index: linux-2.6.4-rc2-bk3-kgdb/arch/i386/kernel/signal.c
===================================================================
--- linux-2.6.4-rc2-bk3-kgdb.orig/arch/i386/kernel/signal.c 2004-03-08
14:30:06.000000000 +0530
+++ linux-2.6.4-rc2-bk3-kgdb/arch/i386/kernel/signal.c 2004-03-09
14:16:55.274355288 +0530
@@ -580,7 +580,8 @@
* have been cleared if the watchpoint triggered
* inside the kernel.
*/
- __asm__("movl %0,%%db7" : : "r" (current->thread.debugreg[7]));
+ if (current->thread.debugreg[7])
+ __asm__("movl %0,%%db7" : : "r" (current->thread.debugreg[7]));

/* Whee! Actually deliver the signal. */
handle_signal(signr, &info, oldset, regs);
Index: linux-2.6.4-rc2-bk3-kgdb/arch/i386/kernel/traps.c
===================================================================
--- linux-2.6.4-rc2-bk3-kgdb.orig/arch/i386/kernel/traps.c 2004-03-08
14:30:06.000000000 +0530
+++ linux-2.6.4-rc2-bk3-kgdb/arch/i386/kernel/traps.c 2004-03-09
14:16:55.275355136 +0530
@@ -51,6 +51,7 @@

#include <linux/irq.h>
#include <linux/module.h>
+#include <linux/debugger.h>

#include "mach_traps.h"

@@ -256,6 +257,7 @@
{
static int die_counter;

+ CHK_DEBUGGER(1,SIGTRAP,err,regs,)
console_verbose();
spin_lock_irq(&die_lock);
bust_spinlocks(1);
@@ -330,6 +332,7 @@
#define DO_ERROR(trapnr, signr, str, name) \
asmlinkage void do_##name(struct pt_regs * regs, long error_code) \
{ \
+ CHK_DEBUGGER(trapnr,signr,error_code,regs,)\
do_trap(trapnr, signr, str, 0, regs, error_code, NULL); \
}

@@ -347,7 +350,10 @@
#define DO_VM86_ERROR(trapnr, signr, str, name) \
asmlinkage void do_##name(struct pt_regs * regs, long error_code) \
{ \
+ CHK_DEBUGGER(trapnr,signr,error_code,regs,goto skip_trap)\
do_trap(trapnr, signr, str, 1, regs, error_code, NULL); \
+skip_trap: \
+ return; \
}

#define DO_VM86_ERROR_INFO(trapnr, signr, str, name, sicode, siaddr) \
@@ -547,7 +553,7 @@
tsk->thread.debugreg[6] = condition;

/* Mask out spurious TF errors due to lazy TF clearing */
- if (condition & DR_STEP) {
+ if (condition & DR_STEP && !debugger_step) {
/*
* The TF error should be masked out only if the current
* process is not traced and if the TRAP flag has been set
@@ -570,11 +576,13 @@
info.si_errno = 0;
info.si_code = TRAP_BRKPT;

- /* If this is a kernel mode trap, save the user PC on entry to
- * the kernel, that's what the debugger can make sense of.
- */
- info.si_addr = ((regs->xcs & 3) == 0) ? (void *)tsk->thread.eip :
- (void *)regs->eip;
+
+ /* If this is a kernel mode trap, we need to reset db7 to allow us
+ * to continue sanely */
+ if ((regs->xcs & 3) == 0)
+ goto clear_dr7;
+
+ info.si_addr = (void *)regs->eip;
force_sig_info(SIGTRAP, &info, tsk);

/* Disable additional traps. They'll be re-enabled when
@@ -584,6 +592,7 @@
__asm__("movl %0,%%db7"
: /* no output */
: "r" (0));
+ CHK_DEBUGGER(1,SIGTRAP,error_code,regs,)
return;

debug_vm86:
Index: linux-2.6.4-rc2-bk3-kgdb/arch/i386/mm/fault.c
===================================================================
--- linux-2.6.4-rc2-bk3-kgdb.orig/arch/i386/mm/fault.c 2004-03-08
14:30:06.000000000 +0530
+++ linux-2.6.4-rc2-bk3-kgdb/arch/i386/mm/fault.c 2004-03-09
14:16:55.284353768 +0530
@@ -2,6 +2,11 @@
* linux/arch/i386/mm/fault.c
*
* Copyright (C) 1995 Linus Torvalds
+ *
+ * Change History
+ *
+ * Tigran Aivazian <[email protected]> Remote debugging support.
+ *
*/

#include <linux/signal.h>
@@ -21,6 +26,7 @@
#include <linux/vt_kern.h> /* For unblank_screen() */
#include <linux/highmem.h>
#include <linux/module.h>
+#include <linux/debugger.h>

#include <asm/system.h>
#include <asm/uaccess.h>
@@ -399,6 +405,8 @@
if (is_prefetch(regs, address))
return;

+ CHK_DEBUGGER(14, SIGSEGV,error_code, regs,)
+
/*
* Oops. The kernel tried to access some bad page. We'll have to
* terminate things with extreme prejudice.
@@ -406,6 +414,7 @@

bust_spinlocks(1);

+
if (address < PAGE_SIZE)
printk(KERN_ALERT "Unable to handle kernel NULL pointer dereference");
else
Index: linux-2.6.4-rc2-bk3-kgdb/include/asm-i386/kgdb.h
===================================================================
--- linux-2.6.4-rc2-bk3-kgdb.orig/include/asm-i386/kgdb.h 2003-01-30
15:54:37.000000000 +0530
+++ linux-2.6.4-rc2-bk3-kgdb/include/asm-i386/kgdb.h 2004-03-09
14:16:55.284353768 +0530
@@ -0,0 +1,49 @@
+#ifndef _ASM_KGDB_H_
+#define _ASM_KGDB_H_
+
+/*
+ * Copyright (C) 2001-2004 Amit S. Kale
+ */
+
+#include <linux/ptrace.h>
+
+/* gdb locks */
+#define KGDB_MAX_NO_CPUS 8
+
+/************************************************************************/
+/* BUFMAX defines the maximum number of characters in inbound/outbound
buffers*/
+/* at least NUMREGBYTES*2 are needed for register packets */
+/* Longer buffer is needed to list all threads */
+#define BUFMAX 1024
+
+/* Number of bytes of registers. */
+#define NUMREGBYTES 64
+/*
+ * Note that this register image is in a different order than
+ * the register image that Linux produces at interrupt time.
+ *
+ * Linux's register image is defined by struct pt_regs in ptrace.h.
+ * Just why GDB uses a different order is a historical mystery.
+ */
+enum regnames { _EAX, /* 0 */
+ _ECX, /* 1 */
+ _EDX, /* 2 */
+ _EBX, /* 3 */
+ _ESP, /* 4 */
+ _EBP, /* 5 */
+ _ESI, /* 6 */
+ _EDI, /* 7 */
+ _PC, /* 8 also known as eip */
+ _PS, /* 9 also known as eflags */
+ _CS, /* 10 */
+ _SS, /* 11 */
+ _DS, /* 12 */
+ _ES, /* 13 */
+ _FS, /* 14 */
+ _GS /* 15 */
+};
+
+#define BREAKPOINT() asm(" int $3");
+#define BREAK_INSTR_SIZE 1
+
+#endif /* _ASM_KGDB_H_ */
Index: linux-2.6.4-rc2-bk3-kgdb/include/asm-i386/processor.h
===================================================================
--- linux-2.6.4-rc2-bk3-kgdb.orig/include/asm-i386/processor.h 2004-03-08
14:30:19.000000000 +0530
+++ linux-2.6.4-rc2-bk3-kgdb/include/asm-i386/processor.h 2004-03-09
14:16:55.290352856 +0530
@@ -425,6 +425,8 @@
unsigned int saved_fs, saved_gs;
/* IO permissions */
unsigned long *io_bitmap_ptr;
+ void * debuggerinfo;
+
};

#define INIT_THREAD { \
Index: linux-2.6.4-rc2-bk3-kgdb/include/asm-i386/system.h
===================================================================
--- linux-2.6.4-rc2-bk3-kgdb.orig/include/asm-i386/system.h 2004-03-08
14:30:19.000000000 +0530
+++ linux-2.6.4-rc2-bk3-kgdb/include/asm-i386/system.h 2004-03-09
14:16:55.297351792 +0530
@@ -12,6 +12,8 @@
struct task_struct; /* one of the stranger aspects of C forward
declarations.. */
extern struct task_struct * FASTCALL(__switch_to(struct task_struct *prev,
struct task_struct *next));

+/* sleeping_thread_to_gdb_regs depends on this code. Correct it if you change
+ * any of the following */
#define switch_to(prev,next,last) do { \
unsigned long esi,edi; \
asm volatile("pushfl\n\t" \

2004-03-09 09:33:00

by Amit S. Kale

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

On Monday 08 Mar 2004 4:19 pm, Amit S. Kale wrote:
> On Monday 08 Mar 2004 3:56 pm, Andrew Morton wrote:
> > "Amit S. Kale" <[email protected]> wrote:
> > > Here are features that are present only in full kgdb:
> > > 1. Thread support (aka info threads)
> >
> > argh, disaster. I discussed this with Tom a week or so ago when it
> > looked like this it was being chopped out and I recall being told that
> > the discussion was referring to something else.
> >
> > Ho-hum, sorry. Can we please put this back in?
>
> Err., well this is one of the particularly dirty parts of kgdb. That's why
> it's been kept away. It takes care of correct thread backtraces in some
> rare cases.
>
> If you consider it an absolutely must, we can do something so that the
> dirty part is kept away and info threads almost always works.

I attempted it and found that it works better than my expectation! I am
attaching revised core-lite.patch with this email and sending i386-lite.patch
as a reply.

Here is the code that extract registers of a sleeping thread:
/*
* Extracts ebp, esp and eip values understandable by gdb from the values
* saved by switch_to.
* thread.esp points to ebp. flags and ebp are pushed in switch_to hence esp
* prior to entering switch_to is 8 greater then the value that is saved.
* If switch_to changes, change following code appropriately.
*/
void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct
*p)
{
gdb_regs[_EAX] = 0;
gdb_regs[_EBX] = 0;
gdb_regs[_ECX] = 0;
gdb_regs[_EDX] = 0;
gdb_regs[_ESI] = 0;
gdb_regs[_EDI] = 0;
gdb_regs[_EBP] = *(unsigned long *)p->thread.esp;
gdb_regs[_DS] = __KERNEL_DS;
gdb_regs[_ES] = __KERNEL_DS;
gdb_regs[_PS] = 0;
gdb_regs[_CS] = __KERNEL_CS;
gdb_regs[_PC] = p->thread.eip;
gdb_regs[_ESP] = p->thread.esp + 8;
gdb_regs[_SS] = __KERNEL_DS;
gdb_regs[_FS] = 0xFFFF;
gdb_regs[_GS] = 0xFFFF;
}

Here is the prologue for schedule
0xc0119f70 <schedule+0>: push %ebp
0xc0119f71 <schedule+1>: mov %esp,%ebp
0xc0119f73 <schedule+3>: mov $0xffffe000,%edx
0xc0119f78 <schedule+8>: push %edi
0xc0119f79 <schedule+9>: push %esi
0xc0119f7a <schedule+10>: push %ebx
0xc0119f7b <schedule+11>: sub $0x3c,%esp

gdb is capable of parsing this prolog and figure out saved registers even in
absence of dwarf debugging information. edi, esi, ebx and ebp are saved in
standard pattern. eax, ecx and edx are call clobbered, hence not used to
store locals in a function calling schedule. Hence gdb can print correct
backtraces with _all_ locals even in callers of schedule!!

-Amit


Attachments:
(No filename) (2.57 kB)
core-lite.patch (40.90 kB)
Download all attachments

2004-03-09 15:06:35

by Tom Rini

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

On Tue, Mar 09, 2004 at 02:59:54PM +0530, Amit S. Kale wrote:
[snip]
> I attempted it and found that it works better than my expectation! I am
> attaching revised core-lite.patch with this email and sending i386-lite.patch
> as a reply.
[snip]
> Index: linux-2.6.4-rc2-bk3-kgdb/include/linux/kgdb.h
[snip]
> +#ifndef KGDB_MAX_NO_CPUS
> +#if CONFIG_NR_CPUS > 8
> +#error KGDB can handle max 8 CPUs
> +#endif
> +#define KGDB_MAX_NO_CPUS 8
> +#endif

We need to remove all of that in favor of s/KGDB_MAX_NO_CPUS/NR_CPUS/g,
and remove the check on 8.

--
Tom Rini
http://gate.crashing.org/~trini/

2004-03-09 15:37:44

by Tom Rini

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

On Tue, Mar 09, 2004 at 10:38:55AM +0530, Amit S. Kale wrote:
> On Tuesday 09 Mar 2004 3:49 am, George Anzinger wrote:
[snip]
> > You really do need a gdb that handles the dwarft2 frames. This is a rather
> > new gdb (I use the CVS version).
>
> Let's just stick to gdb 6.0 and binutils 2.14. CVS versions of gdb and
> binutils are too risky for someone who is trying to learn linux kernel by
> using kgdb.

I think that depends on the level of crap we have to do compared to how
it would be done with a newer version of gdb or binutils. GDB 6.1 for
example should be out soon.

--
Tom Rini
http://gate.crashing.org/~trini/

2004-03-10 04:07:29

by Amit S. Kale

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

On Tuesday 09 Mar 2004 8:36 pm, Tom Rini wrote:
> On Tue, Mar 09, 2004 at 02:59:54PM +0530, Amit S. Kale wrote:
> [snip]
>
> > I attempted it and found that it works better than my expectation! I am
> > attaching revised core-lite.patch with this email and sending
> > i386-lite.patch as a reply.
>
> [snip]
>
> > Index: linux-2.6.4-rc2-bk3-kgdb/include/linux/kgdb.h
>
> [snip]
>
> > +#ifndef KGDB_MAX_NO_CPUS
> > +#if CONFIG_NR_CPUS > 8
> > +#error KGDB can handle max 8 CPUs
> > +#endif
> > +#define KGDB_MAX_NO_CPUS 8
> > +#endif
>
> We need to remove all of that in favor of s/KGDB_MAX_NO_CPUS/NR_CPUS/g,
> and remove the check on 8.

Yes. I'll do that.
-Amit


2004-03-10 12:39:08

by Andi Kleen

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

> Yes. But as things stand, gdb 6.0 doesn't show stack traces correctly with esp
> and eip got from switch_to and gas 2.14 can't handle i386 dwarf2 CFI. Do we
> want to enforce getting a CVS version of gdb _and_ gas to build kgdb?
> Certainly not.

binutils 2.15 should be released soon anyways AFAIK. And for x86-64 this all
works just fine (as demonstrated by Jim's/George's stub), so please get
rid of it at least for x86-64. I really don't want user_schedule there,
because it's completely unnecessary.

-Andi

2004-03-10 15:27:52

by Tom Rini

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

On Wed, Mar 10, 2004 at 01:36:05PM +0100, Andi Kleen wrote:
> > Yes. But as things stand, gdb 6.0 doesn't show stack traces correctly with esp
> > and eip got from switch_to and gas 2.14 can't handle i386 dwarf2 CFI. Do we
> > want to enforce getting a CVS version of gdb _and_ gas to build kgdb?
> > Certainly not.
>
> binutils 2.15 should be released soon anyways AFAIK. And for x86-64 this all
> works just fine (as demonstrated by Jim's/George's stub), so please get
> rid of it at least for x86-64. I really don't want user_schedule there,
> because it's completely unnecessary.

I think more importantly, it's probably going to be one of those ugly
things that will make it so much harder to get it into Linus' tree. So
lets just say it'll require gdb 6.1 / binutils 2.15 for KGDB to work
best.

--
Tom Rini
http://gate.crashing.org/~trini/

2004-03-11 04:58:35

by Amit S. Kale

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

On Wednesday 10 Mar 2004 8:57 pm, Tom Rini wrote:
> On Wed, Mar 10, 2004 at 01:36:05PM +0100, Andi Kleen wrote:
> > > Yes. But as things stand, gdb 6.0 doesn't show stack traces correctly
> > > with esp and eip got from switch_to and gas 2.14 can't handle i386
> > > dwarf2 CFI. Do we want to enforce getting a CVS version of gdb _and_
> > > gas to build kgdb? Certainly not.
> >
> > binutils 2.15 should be released soon anyways AFAIK. And for x86-64 this
> > all works just fine (as demonstrated by Jim's/George's stub), so please
> > get rid of it at least for x86-64. I really don't want user_schedule
> > there, because it's completely unnecessary.
>
> I think more importantly, it's probably going to be one of those ugly
> things that will make it so much harder to get it into Linus' tree. So
> lets just say it'll require gdb 6.1 / binutils 2.15 for KGDB to work
> best.

If we are enforcing this we need to do it correctly: is there any way to check
from source code whether binutils version is correct (even a gas check will
suffice)

-Amit

2004-03-11 05:10:01

by Randy.Dunlap

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

On Thu, 11 Mar 2004 10:27:51 +0530 "Amit S. Kale" <[email protected]> wrote:

| On Wednesday 10 Mar 2004 8:57 pm, Tom Rini wrote:
| > On Wed, Mar 10, 2004 at 01:36:05PM +0100, Andi Kleen wrote:
| > > > Yes. But as things stand, gdb 6.0 doesn't show stack traces correctly
| > > > with esp and eip got from switch_to and gas 2.14 can't handle i386
| > > > dwarf2 CFI. Do we want to enforce getting a CVS version of gdb _and_
| > > > gas to build kgdb? Certainly not.
| > >
| > > binutils 2.15 should be released soon anyways AFAIK. And for x86-64 this
| > > all works just fine (as demonstrated by Jim's/George's stub), so please
| > > get rid of it at least for x86-64. I really don't want user_schedule
| > > there, because it's completely unnecessary.
| >
| > I think more importantly, it's probably going to be one of those ugly
| > things that will make it so much harder to get it into Linus' tree. So
| > lets just say it'll require gdb 6.1 / binutils 2.15 for KGDB to work
| > best.
|
| If we are enforcing this we need to do it correctly: is there any way to check
| from source code whether binutils version is correct (even a gas check will
| suffice)

Documentation/Changes says to use "ld -v" for binutils version.

--
~Randy

2004-03-11 05:11:24

by Andi Kleen

[permalink] [raw]
Subject: Re: kgdb for mainline kernel: core-lite [patch 1/3]

> If we are enforcing this we need to do it correctly: is there any way to check
> from source code whether binutils version is correct (even a gas check will
> suffice)

The latest 2.6 kernel has a makefile macro to check the gcc version. You could
probably adapt that to check binutils too.

-Andi