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

Tom Rini
2004-03-08T15:19:27Z

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" <amitkale@emsyssoft.com> wrote:
> > > > On Monday 08 Mar 2004 3:56 pm, Andrew Morton wrote:
> > > > > "Amit S. Kale" <amitkale@emsyssoft.com> 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