Re: kgdb: core

Re: kgdb: core

[Andrew Morton]

On Fri, 18 Apr 2008 17:41:29 GMT
Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote:

> Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=dc7d552705215ac50a0617fcf51bb9c736255b8e
> Commit:     dc7d552705215ac50a0617fcf51bb9c736255b8e
> Parent:     c33fa9f5609e918824446ef9a75319d4a802f1f4
> Author:     Jason Wessel <jason.wessel@windriver.com>
> AuthorDate: Thu Apr 17 20:05:37 2008 +0200
> Committer:  Ingo Molnar <mingo@elte.hu>
> CommitDate: Thu Apr 17 20:05:37 2008 +0200
> 
>     kgdb: core
>     
>     kgdb core code. Handles the protocol and the arch details.
>     
>     [ mingo@elte.hu: heavily modified, simplified and cleaned up. ]
>     [ xemul@openvz.org: use find_task_by_pid_ns ]
>     
> ...
>
> +
> +/*
> + *	kgdb_skipexception - Bail out of KGDB when we've been triggered.
> + *	@exception: Exception vector number
> + *	@regs: Current &struct pt_regs.
> + *
> + *	On some architectures we need to skip a breakpoint exception when
> + *	it occurs after a breakpoint has been removed.
> + */
> +extern int kgdb_skipexception(int exception, struct pt_regs *regs);

Please just nuke all the interface comments in the header files.  They
duplicate the kernedoc comments at the definition site and we don't want to
have to update both versions whenever we change something.

> +/*
> + * Functions each KGDB-supporting architecture must provide:
> + */
> +
> +/*
> + *	kgdb_arch_init - Perform any architecture specific initalization.
> + *
> + *	This function will handle the initalization of any architecture
> + *	specific callbacks.
> + */
> +extern int kgdb_arch_init(void);

Well, these are trickier because there is an implementation of this
function within each architecture.  So I think that in this case it _does_
make sense to document the function in a common place, and the only common
place is this header file.

So please

a) make this a kerneldoc comment and

b) remove the kerneldoc at the definition site(s).

(alternative: teach the kerneldoc system to go fishing in the various arch
directories to find the appropriate documentation, but I don't know enough
about kerneldoc to be able say anything about that).

> +
> +/*
> + * struct kgdb_arch - Describe architecture specific values.
> + * @gdb_bpt_instr: The instruction to trigger a breakpoint.
> + * @flags: Flags for the breakpoint, currently just %KGDB_HW_BREAKPOINT.
> + * @set_breakpoint: Allow an architecture to specify how to set a software
> + * breakpoint.
> + * @remove_breakpoint: Allow an architecture to specify how to remove a
> + * software breakpoint.
> + * @set_hw_breakpoint: Allow an architecture to specify how to set a hardware
> + * breakpoint.
> + * @remove_hw_breakpoint: Allow an architecture to specify how to remove a
> + * hardware breakpoint.
> + * @remove_all_hw_break: Allow an architecture to specify how to remove all
> + * hardware breakpoints.
> + * @correct_hw_break: Allow an architecture to specify how to correct the
> + * hardware debug registers.
> + */

This should become a kernedoc comment, as this is the only place we can
document it.  So please add the leading /**

> +struct kgdb_arch {
> +	unsigned char		gdb_bpt_instr[BREAK_INSTR_SIZE];
> +	unsigned long		flags;
> +
> +	int	(*set_breakpoint)(unsigned long, char *);
> +	int	(*remove_breakpoint)(unsigned long, char *);
> +	int	(*set_hw_breakpoint)(unsigned long, int, enum kgdb_bptype);
> +	int	(*remove_hw_breakpoint)(unsigned long, int, enum kgdb_bptype);
> +	void	(*remove_all_hw_break)(void);
> +	void	(*correct_hw_break)(void);
> +};
> +
> +/*
> + * struct kgdb_io - Describe the interface for an I/O driver to talk with KGDB.
> + * @name: Name of the I/O driver.
> + * @read_char: Pointer to a function that will return one char.
> + * @write_char: Pointer to a function that will write one char.
> + * @flush: Pointer to a function that will flush any pending writes.
> + * @init: Pointer to a function that will initialize the device.
> + * @pre_exception: Pointer to a function that will do any prep work for
> + * the I/O driver.
> + * @post_exception: Pointer to a function that will do any cleanup work
> + * for the I/O driver.
> + */

ditto

> +struct kgdb_io {
> +	const char		*name;
> +	int			(*read_char) (void);
> +	void			(*write_char) (u8);
> +	void			(*flush) (void);
> +	int			(*init) (void);
> +	void			(*pre_exception) (void);
> +	void			(*post_exception) (void);
> +};
>
> ...
>
> +static struct debuggerinfo_struct {
> +	void			*debuggerinfo;
> +	struct task_struct	*task;
> +} kgdb_info[NR_CPUS];

We're slowly trying to weed out dependencies on NR_CPUS: switching to
num_online_cpus, num_possible_cpus, etc.  This one takes us backwards.

Fixable?

> +static atomic_t			passive_cpu_wait[NR_CPUS];
> +static atomic_t			cpu_in_kgdb[NR_CPUS];

etc.

> +static char			remcom_in_buffer[BUFMAX];
> +static char			remcom_out_buffer[BUFMAX];

Surprisingly, kgdb appears to be the only part of the kernel which is using
BUFMAX.  But as a kernel-wide identifier, it isn't a very well-chosen one.

> +
> +/*
> + * GDB remote protocol parser:
> + */
> +
> +static const char	hexchars[] = "0123456789abcdef";
> +
> +static int hex(char ch)
> +{
> +	if ((ch >= 'a') && (ch <= 'f'))
> +		return ch - 'a' + 10;
> +	if ((ch >= '0') && (ch <= '9'))
> +		return ch - '0';
> +	if ((ch >= 'A') && (ch <= 'F'))
> +		return ch - 'A' + 10;
> +	return -1;
> +}

How many are we up to now?

akpm:/usr/src/linux-2.6.25> grep -ri '"0123456789abcdef"' . | wc -l
40

lol.



Nice-looking code - kgb has improved rather a lot.  I'm glad we finally got
it in.  Maybe one day I'll get to use it again :(

Re: kgdb: core

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> > +/*
> > + *	kgdb_skipexception - Bail out of KGDB when we've been triggered.
> > + *	@exception: Exception vector number
> > + *	@regs: Current &struct pt_regs.
> > + *
> > + *	On some architectures we need to skip a breakpoint exception when
> > + *	it occurs after a breakpoint has been removed.
> > + */
> > +extern int kgdb_skipexception(int exception, struct pt_regs *regs);
> 
> Please just nuke all the interface comments in the header files.  They 
> duplicate the kernedoc comments at the definition site and we don't 
> want to have to update both versions whenever we change something.

well that way we'll have to update _all_ arch versions whenever we 
change something - while the reference prototype in kgdb.h should all 
cover it. Do we really want to do that?

> > +/*
> > + * Functions each KGDB-supporting architecture must provide:
> > + */
> > +
> > +/*
> > + *	kgdb_arch_init - Perform any architecture specific initalization.
> > + *
> > + *	This function will handle the initalization of any architecture
> > + *	specific callbacks.
> > + */
> > +extern int kgdb_arch_init(void);
> 
> Well, these are trickier because there is an implementation of this 
> function within each architecture.  So I think that in this case it 
> _does_ make sense to document the function in a common place, and the 
> only common place is this header file.
> 
> So please
> 
> a) make this a kerneldoc comment and
> 
> b) remove the kerneldoc at the definition site(s).
> 
> (alternative: teach the kerneldoc system to go fishing in the various 
> arch directories to find the appropriate documentation, but I don't 
> know enough about kerneldoc to be able say anything about that).

well there's lkml feedback ping-pong effect here. It was pointed out in 
earlier kgdb review that it's an "error" to put kerneldoc into header 
files. I pointed out that it makes no sense to do otherwise but removed 
the kerneldoc annotation to resolve the "objection".

> This should become a kernedoc comment, as this is the only place we 
> can document it.  So please add the leading /**

same deal - it was objected to in review.

> > +static const char	hexchars[] = "0123456789abcdef";
> > +
> > +static int hex(char ch)
> > +{
> > +	if ((ch >= 'a') && (ch <= 'f'))
> > +		return ch - 'a' + 10;
> > +	if ((ch >= '0') && (ch <= '9'))
> > +		return ch - '0';
> > +	if ((ch >= 'A') && (ch <= 'F'))
> > +		return ch - 'A' + 10;
> > +	return -1;
> > +}
> 
> How many are we up to now?
> 
> akpm:/usr/src/linux-2.6.25> grep -ri '"0123456789abcdef"' . | wc -l
> 40
> 
> lol.

okay, hex_asc() it should use. Probably KGDB's code predates that of 
kernel.h though ;-)

> Nice-looking code - kgb has improved rather a lot.  I'm glad we 
> finally got it in.  [...]

thanks :)

> [...] Maybe one day I'll get to use it again :(

/me duly notes this request to break Andrew's systems even more frequently ;-)

	Ingo

Re: kgdb: core

[Andrew Morton]

> On Mon, 21 Apr 2008 16:12:52 +0200 Ingo Molnar <mingo@elte.hu> wrote:
> > So please
> > 
> > a) make this a kerneldoc comment and
> > 
> > b) remove the kerneldoc at the definition site(s).
> > 
> > (alternative: teach the kerneldoc system to go fishing in the various 
> > arch directories to find the appropriate documentation, but I don't 
> > know enough about kerneldoc to be able say anything about that).
> 
> well there's lkml feedback ping-pong effect here. It was pointed out in 
> earlier kgdb review that it's an "error" to put kerneldoc into header 
> files.

It is, normally.  Nobody thought about this case.

> I pointed out that it makes no sense to do otherwise but removed 
> the kerneldoc annotation to resolve the "objection".

Duplicating the same stuff in multiple places is the larger sin.  It sounds
like the best compromise would be to kernel-doc the interface in the .h
file and remove the duplicated comments from .c.

Or perhaps we kernel-doc the interface in the x86 .c files and leave it at
that - people should go there to find the docs.  Problem is, this will
presumably generate bad results if one builds the formal kerneldoc output
for a different architecture.  The kerneldoc system could of course fix
this somehow, but I don't know what shape it would take nor how much work
it would be.

Re: kgdb: core

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> > On Mon, 21 Apr 2008 16:12:52 +0200 Ingo Molnar <mingo@elte.hu> wrote:
> > > So please
> > > 
> > > a) make this a kerneldoc comment and
> > > 
> > > b) remove the kerneldoc at the definition site(s).
> > > 
> > > (alternative: teach the kerneldoc system to go fishing in the various 
> > > arch directories to find the appropriate documentation, but I don't 
> > > know enough about kerneldoc to be able say anything about that).
> > 
> > well there's lkml feedback ping-pong effect here. It was pointed out 
> > in earlier kgdb review that it's an "error" to put kerneldoc into 
> > header files.
> 
> It is, normally.  Nobody thought about this case.

in that review discussion i pointed out much of the same arguments you 
did in this mail.

> > I pointed out that it makes no sense to do otherwise but removed the 
> > kerneldoc annotation to resolve the "objection".
> 
> Duplicating the same stuff in multiple places is the larger sin.  It 
> sounds like the best compromise would be to kernel-doc the interface 
> in the .h file and remove the duplicated comments from .c.
> 
> Or perhaps we kernel-doc the interface in the x86 .c files and leave 
> it at that - people should go there to find the docs.  Problem is, 
> this will presumably generate bad results if one builds the formal 
> kerneldoc output for a different architecture.  The kerneldoc system 
> could of course fix this somehow, but I don't know what shape it would 
> take nor how much work it would be.

i'd rather just have it in the .h, to put KernelDoc into the position of 
getting fixed with a real testcase?

( _some_ comments in the arch file might be appropriate as well, but
  only if the arch implementation deviates from the common pattern in
  some way. )

It's not like this is a big issue, people writing KGDB arch support are 
not the typical readers of KernelDoc PDFs.

	Ingo

Re: kgdb: core

[Randy Dunlap]

On Mon, 21 Apr 2008 21:46:36 -0700 Andrew Morton wrote:

> > On Mon, 21 Apr 2008 16:12:52 +0200 Ingo Molnar <mingo@elte.hu> wrote:
> > > So please
> > > 
> > > a) make this a kerneldoc comment and
> > > 
> > > b) remove the kerneldoc at the definition site(s).
> > > 
> > > (alternative: teach the kerneldoc system to go fishing in the various 
> > > arch directories to find the appropriate documentation, but I don't 
> > > know enough about kerneldoc to be able say anything about that).
> > 
> > well there's lkml feedback ping-pong effect here. It was pointed out in 
> > earlier kgdb review that it's an "error" to put kerneldoc into header 
> > files.

As Andrew has pointed out in various emails, it is convention to
put kernel-doc near definitions (implementations), not near
declarations (headers).  [Boy, I hope I didn't get those reversed.]
But it's just convention for the sake of consistency AFAIK.
kernel-doc will read header files if that's what we tell it to do.

> It is, normally.  Nobody thought about this case.
> 
> > I pointed out that it makes no sense to do otherwise but removed 
> > the kerneldoc annotation to resolve the "objection".
> 
> Duplicating the same stuff in multiple places is the larger sin.  It sounds
> like the best compromise would be to kernel-doc the interface in the .h
> file and remove the duplicated comments from .c.

Yes, duplication would be Bad.

> Or perhaps we kernel-doc the interface in the x86 .c files and leave it at
> that - people should go there to find the docs.  Problem is, this will

That's (using x86) fairly common also, esp. for functions that header
files (inline functions and macros).

> presumably generate bad results if one builds the formal kerneldoc output
> for a different architecture.  The kerneldoc system could of course fix
> this somehow, but I don't know what shape it would take nor how much work
> it would be.

kernel-doc output doesn't depend on build $ARCH.


---
~Randy

Re: kgdb: core

[Andrew Morton]

> On Tue, 22 Apr 2008 08:25:19 -0700 Randy Dunlap <randy.dunlap@oracle.com> wrote:
>
> kernel-doc output doesn't depend on build $ARCH.

Ah, interesting.  I made an ass of u and me.

In that case documentng them in x86 .c and leaving all other definitions
and declarations uncommented sounds the way to go?  

Re: kgdb: core

[Randy Dunlap]

--- Original Message --- (akpm wrote:)
> > On Tue, 22 Apr 2008 08:25:19 -0700 Randy Dunlap <randy.dunlap@oracle.com> wrote:
> >
> > kernel-doc output doesn't depend on build $ARCH.
> 
> Ah, interesting.  I made an ass of u and me.
> 
> In that case documentng them in x86 .c and leaving all other definitions
> and declarations uncommented sounds the way to go?  

Yes, that would be fine and traditional (historical).

~Randy

Re: kgdb: core

[Jason Wessel]

Randy Dunlap wrote:
> --- Original Message --- (akpm wrote:)
>>> On Tue, 22 Apr 2008 08:25:19 -0700 Randy Dunlap
<randy.dunlap@oracle.com> wrote:
>>>
>>> kernel-doc output doesn't depend on build $ARCH.
>> Ah, interesting.  I made an ass of u and me.
>>
>> In that case documentng them in x86 .c and leaving all other definitions
>> and declarations uncommented sounds the way to go?  
>
> Yes, that would be fine and traditional (historical).
>
> ~Randy
>

So is the summary here that we cannot just document the actual
interface API in the incude/linux/kgdb.h?

The problem I had here was pretty simple in that not all of the
functions in the API are used by all the archs.  It seemed pretty
reasonable at the time to just document everything in the .h file
because it was more for the folks who wanted to add kgdb support to an
arch and of little to no use to end users of kgdb.

The mainline only has a small chunk of the docs right now because not
all of kgdb is merged.  I was just hoping it was real easy to generate
some reasonable looking docs.  The docs for all the kgdb code that
exists (in mainline and not in mainline) can be viewed here as an
example.

http://kernel.org/pub/linux/kernel/people/jwessel/kgdb/

Jason.

Re: kgdb: core

[Randy Dunlap]

On Tue, 22 Apr 2008 16:19:55 -0500 Jason Wessel wrote:

> Randy Dunlap wrote:
> > --- Original Message --- (akpm wrote:)
> >>> On Tue, 22 Apr 2008 08:25:19 -0700 Randy Dunlap
> <randy.dunlap@oracle.com> wrote:
> >>>
> >>> kernel-doc output doesn't depend on build $ARCH.
> >> Ah, interesting.  I made an ass of u and me.
> >>
> >> In that case documentng them in x86 .c and leaving all other definitions
> >> and declarations uncommented sounds the way to go?  
> >
> > Yes, that would be fine and traditional (historical).
> >
> > ~Randy
> >
> 
> So is the summary here that we cannot just document the actual
> interface API in the incude/linux/kgdb.h?

The summary is that we prefer for kernel-doc to be placed at (function
etc.) definition/implementation sites, not at declaration sites.
However, I'm sure that there are exceptions to this.

> The problem I had here was pretty simple in that not all of the
> functions in the API are used by all the archs.  It seemed pretty
> reasonable at the time to just document everything in the .h file
> because it was more for the folks who wanted to add kgdb support to an
> arch and of little to no use to end users of kgdb.

I see.  That could pose a problem.

And there is not any one $arch that does use the complete API?
If so, all of the kernel-doc could be in that $arch.

> The mainline only has a small chunk of the docs right now because not
> all of kgdb is merged.  I was just hoping it was real easy to generate
> some reasonable looking docs.  The docs for all the kgdb code that
> exists (in mainline and not in mainline) can be viewed here as an
> example.
> 
> http://kernel.org/pub/linux/kernel/people/jwessel/kgdb/

If it poses an extreme hardship on you to conform to the
norms/expectations, so be it.  It won't kill anything AFAIK.
(i.e., I won't object.)

---
~Randy