Re: [3/6] kgdb: core
On Sunday 10 February 2008, Ingo Molnar wrote:
>
> * Marcin Slusarz <[EMAIL PROTECTED]> wrote:
>
> > > + if (CACHE_FLUSH_IS_SAFE) {
> > > + if (current->mm && addr < TASK_SIZE) {
> > > + flush_cache_range(current->mm->mmap_cache,
> > > + addr, addr + BREAK_INSTR_SIZE);
> > > + } else {
> > > + flush_icache_range(addr, addr +
> > > + BREAK_INSTR_SIZE);
> > > + }
> > > + }
> > unneeded braces (here and in many other places)
>
> this is a small detail, but you are wrong. These braces around
> multi-line statements are unneded _for the compiler_, but are very much
> wanted by humans. You'll see akpm, me and others reject/fix patches on a
> routine basis that make this cleanliness mistake. Please watch out for
> this when writing patches ;-)
>
> > if ()
> > else if ()
> > else
> >
> > will look better
>
> nope. I consciously avoid that construct because it's dangerous: it can
> quite easily result in the wrong logic. Having _more_ braces than needed
> by the compiler is a style error in only a single, special case.
however it can be still made to:
if () {
if ()
else
}
[ not fixed in v6 ]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
* Christoph Hellwig <[EMAIL PROTECTED]> wrote: > On Sun, Feb 10, 2008 at 10:27:28AM +0100, Ingo Molnar wrote: > > > The one I noticed quickly is the __ASSEMBLY__ removal from > > > asm-x86/kgdb.h. [...] > > > > people might want to experiment with early debug code as well and > > include asm-x86/kgdb.h in assembly files. So i kept that, it's > > sensible. > > But asm-x86/kgdb.h doesn't have anythign people invoking kgdb might > need, it's just the arch interface to the kgdb core. Nor does it > compile even with the ifdef as it already contains a C enum. good point - i fixed this. (by following your suggestion of removing the _ASSEMBLY_) Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
* Marcin Slusarz <[EMAIL PROTECTED]> wrote: > if (hex_val < 0) > break; > *long_val = (*long_val << 4) | hex_val; > num++; > (*ptr)++; agreed, fixed. > > + remcom_out_buffer[0] = 'S'; > > + remcom_out_buffer[1] = hexchars[ks->signo >> 4]; > > + remcom_out_buffer[2] = hexchars[ks->signo % 16]; > use pack_hex_byte or & 0xf fixed. Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
* Marcin Slusarz <[EMAIL PROTECTED]> wrote:
> > + if (CACHE_FLUSH_IS_SAFE) {
> > + if (current->mm && addr < TASK_SIZE) {
> > + flush_cache_range(current->mm->mmap_cache,
> > + addr, addr + BREAK_INSTR_SIZE);
> > + } else {
> > + flush_icache_range(addr, addr +
> > + BREAK_INSTR_SIZE);
> > + }
> > + }
> unneeded braces (here and in many other places)
this is a small detail, but you are wrong. These braces around
multi-line statements are unneded _for the compiler_, but are very much
wanted by humans. You'll see akpm, me and others reject/fix patches on a
routine basis that make this cleanliness mistake. Please watch out for
this when writing patches ;-)
> if ()
> else if ()
> else
>
> will look better
nope. I consciously avoid that construct because it's dangerous: it can
quite easily result in the wrong logic. Having _more_ braces than needed
by the compiler is a style error in only a single, special case.
> > + if (*(ptr++) != ',') {
> > + error_packet(remcom_out_buffer, -EINVAL);
> > + return;
> > + } else {
> no else needed
agreed - fixed.
> if (!kgdb_hex2long()) {
> error_packet();
> return;
> }
fixed.
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
On Sun, Feb 10, 2008 at 02:19:06PM +0100, Jesper Juhl wrote:
> On 10/02/2008, Marcin Slusarz <[EMAIL PROTECTED]> wrote:
> > On Sun, Feb 10, 2008 at 08:13:31AM +0100, Ingo Molnar wrote:
> ...
> > > +
> > > + if (CACHE_FLUSH_IS_SAFE) {
> > > + if (current->mm && addr < TASK_SIZE) {
> > > + flush_cache_range(current->mm->mmap_cache,
> > > + addr, addr +
> > > BREAK_INSTR_SIZE);
> > > + } else {
> > > + flush_icache_range(addr, addr +
> > > + BREAK_INSTR_SIZE);
> > > + }
> > > + }
> > unneeded braces (here and in many other places)
> >
>
> While they are not strictly needed, I for one would argue they should
> probably stay.
>
> if (foo)
> bar();
>
> is not always safe in case bar() is a macro.
then fix this broken macro and leave calling code alone
> is always safe and is more robust when the code gets changed later
> since you don't accidentally end up with someone mistakenly turning it
> into
>
> if (foo)
> bar();
> baz();
following coding style and reading code before submission will
catch this kind of bugs
Marcin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
Marcin Slusarz wrote:
> On Sun, Feb 10, 2008 at 08:13:31AM +0100, Ingo Molnar wrote:
>> +} else {
>> +while (count-- > 0) {
>> +unsigned char ch;
>> +
>> +if (probe_kernel_address(mem, ch)) {
>> +kgdb_may_fault = 0;
>> +return ERR_PTR(-EINVAL);
>> +}
>> +mem++;
>> +*buf++ = hexchars[ch >> 4];
>> +*buf++ = hexchars[ch & 0xf];
> use pack_hex_byte?
Good point! kgdb introduces this helper but don't use it consequently!
>> +/*
>> + * While we find nice hex chars, build a long_val.
>> + * Return number of chars processed.
>> + */
>> +int kgdb_hex2long(char **ptr, long *long_val)
>> +{
>> +int hex_val;
>> +int num = 0;
>> +
>> +*long_val = 0;
>> +
>> +while (**ptr) {
>> +hex_val = hex(**ptr);
>> +if (hex_val >= 0) {
>> +*long_val = (*long_val << 4) | hex_val;
>> +num++;
>> +} else
>> +break;
>> +
>> +(*ptr)++;
>> +}
> if (hex_val < 0)
> break;
> *long_val = (*long_val << 4) | hex_val;
> num++;
> (*ptr)++;
Jep, will include this in the cleanup patch I'm currently baking.
Jan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
On 10/02/2008, Marcin Slusarz <[EMAIL PROTECTED]> wrote:
> On Sun, Feb 10, 2008 at 08:13:31AM +0100, Ingo Molnar wrote:
...
> > +
> > + if (CACHE_FLUSH_IS_SAFE) {
> > + if (current->mm && addr < TASK_SIZE) {
> > + flush_cache_range(current->mm->mmap_cache,
> > + addr, addr +
> > BREAK_INSTR_SIZE);
> > + } else {
> > + flush_icache_range(addr, addr +
> > + BREAK_INSTR_SIZE);
> > + }
> > + }
> unneeded braces (here and in many other places)
>
While they are not strictly needed, I for one would argue they should
probably stay.
if (foo)
bar();
is not always safe in case bar() is a macro.
if (foo) {
bar();
}
is always safe and is more robust when the code gets changed later
since you don't accidentally end up with someone mistakenly turning it
into
if (foo)
bar();
baz();
--
Jesper Juhl <[EMAIL PROTECTED]>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
On Sun, Feb 10, 2008 at 08:13:31AM +0100, Ingo Molnar wrote:
> + } else {
> + while (count-- > 0) {
> + unsigned char ch;
> +
> + if (probe_kernel_address(mem, ch)) {
> + kgdb_may_fault = 0;
> + return ERR_PTR(-EINVAL);
> + }
> + mem++;
> + *buf++ = hexchars[ch >> 4];
> + *buf++ = hexchars[ch & 0xf];
use pack_hex_byte?
> +/*
> + * While we find nice hex chars, build a long_val.
> + * Return number of chars processed.
> + */
> +int kgdb_hex2long(char **ptr, long *long_val)
> +{
> + int hex_val;
> + int num = 0;
> +
> + *long_val = 0;
> +
> + while (**ptr) {
> + hex_val = hex(**ptr);
> + if (hex_val >= 0) {
> + *long_val = (*long_val << 4) | hex_val;
> + num++;
> + } else
> + break;
> +
> + (*ptr)++;
> + }
if (hex_val < 0)
break;
*long_val = (*long_val << 4) | hex_val;
num++;
(*ptr)++;
> +/*
> + * SW breakpoint management:
> + */
> +static int kgdb_activate_sw_breakpoints(void)
> +{
> + unsigned long addr;
> + int error = 0;
> + int i;
> +
> + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) {
> + if (kgdb_break[i].state != BP_SET)
> + continue;
> +
> + addr = kgdb_break[i].bpt_addr;
> + error = kgdb_arch_set_breakpoint(addr,
> + kgdb_break[i].saved_instr);
> + if (error)
> + return error;
> +
> + if (CACHE_FLUSH_IS_SAFE) {
> + if (current->mm && addr < TASK_SIZE) {
> + flush_cache_range(current->mm->mmap_cache,
> + addr, addr + BREAK_INSTR_SIZE);
> + } else {
> + flush_icache_range(addr, addr +
> + BREAK_INSTR_SIZE);
> + }
> + }
unneeded braces (here and in many other places)
> +/* Handle the '?' status packets */
> +static void gdb_cmd_status(struct kgdb_state *ks)
> +{
> + /*
> + * We know that this packet is only sent
> + * during initial connect. So to be safe,
> + * we clear out our breakpoints now in case
> + * GDB is reconnecting.
> + */
> + remove_all_break();
> +
> + /*
> + * Also, if we haven't been able to roundup all
> + * CPUs, send an 'O' packet informing the user
> + * as much. Only need to do this once.
> + */
> + if (!ks->all_cpus_synced)
> + kgdb_msg_write("Not all CPUs have been synced for KGDB\n", 39);
> +
> + remcom_out_buffer[0] = 'S';
> + remcom_out_buffer[1] = hexchars[ks->signo >> 4];
> + remcom_out_buffer[2] = hexchars[ks->signo % 16];
use pack_hex_byte or & 0xf
> + if (ks->threadid < pid_max) {
> + kgdb_mem2hex(getthread(ks->linux_regs,
> + ks->threadid)->comm,
> + remcom_out_buffer, 16);
> + } else {
> + if (ks->threadid >= pid_max + num_online_cpus()) {
> + kgdb_shadowinfo(ks->linux_regs,
> + remcom_out_buffer,
> + ks->threadid - pid_max -
> + num_online_cpus());
> + } else {
> + static char tmpstr[23 + BUF_THREAD_ID_SIZE];
> + sprintf(tmpstr, "Shadow task %d for pid 0",
> + (int)(ks->threadid - pid_max));
> + kgdb_mem2hex(tmpstr, remcom_out_buffer,
> + strlen(tmpstr));
> + }
> + }
if ()
else if ()
else
will look better
> + if (*(ptr++) != ',') {
> + error_packet(remcom_out_buffer, -EINVAL);
> + return;
> + } else {
no else needed
> + if (kgdb_hex2long(, )) {
> + if (*(ptr++) != ',' ||
> + !kgdb_hex2long(, )) {
> + error_packet(remcom_out_buffer, -EINVAL);
> + return;
> + }
> + } else {
> + error_packet(remcom_out_buffer, -EINVAL);
> + return;
> + }
> + }
if (!kgdb_hex2long()) {
error_packet();
return;
}
if (*(ptr++) (...))
(...)
Marcin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
On Sun, Feb 10, 2008 at 10:27:28AM +0100, Ingo Molnar wrote: > > The one I noticed quickly is the __ASSEMBLY__ removal from > > asm-x86/kgdb.h. [...] > > people might want to experiment with early debug code as well and > include asm-x86/kgdb.h in assembly files. So i kept that, it's sensible. But asm-x86/kgdb.h doesn't have anythign people invoking kgdb might need, it's just the arch interface to the kgdb core. Nor does it compile even with the ifdef as it already contains a C enum. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
* Ingo Molnar <[EMAIL PROTECTED]> wrote: > > tip 2e3ebf25b0bd8646e517806073e1991be1fec8a2. Shortlog, diffstat and > > the full patch can be found below, with all relevant review feedback > > addressed. Builds, boots and works fine on x86. > > here's gdb test-output from this 2e3ebf25b0bd kernel: i should also mention that yesterday's tree passed 200 randconfig bootup tests on 32-bit and 64-bit x86. (i excluded CONFIG_WMI from ACPI, plus 3 other ACPI commits because they keept crashing boxes or broke the build) Today's kgdb updates are in the trivial category so i'd not expect them to break anything, but nevertheless, out of caution i threw the latest tree into the qa mix as well and they already passed 10 randconfig bootup tests. [ and this matches my experience with KGDB stability in the last few months while we carried and tested it in x86.git: even the old, much wider-scope and uglier/riskier patches that hooked in a lot of places never broke anything unrelated (or anything in fact) - and this matches kgdb's -mm track record as well. ] Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
> +#ifdef UART_CAP_UUE
> + if (up->capabilities & UART_CAP_UUE)
> +#else
> + if (up->port.type == PORT_XSCALE)
> +#endif
This looks very odd. Can anyone explain what's going on here?
Especially as UART_CAP_UUE is defined in drivers/serial/8250.h
unconditionally.
> diff --git a/drivers/serial/kgdboc.c b/drivers/serial/kgdboc.c
> new file mode 100644
> index 000..5079d32
> --- /dev/null
> +++ b/drivers/serial/kgdboc.c
> @@ -0,0 +1,164 @@
> +/*
> + * drivers/serial/kgdboc.c
Didn't you say there was no file left with these?
> diff --git a/include/asm-generic/kgdb.h b/include/asm-generic/kgdb.h
> new file mode 100644
> index 000..115972e
> --- /dev/null
> +++ b/include/asm-generic/kgdb.h
Didn't you agree to kill this file in one of the last mails?
> +#ifndef __ASSEMBLY__
> +static inline void arch_kgdb_breakpoint(void)
> +{
> + asm(" int $3");
> +}
> +# define BREAK_INSTR_SIZE1
> +# define CACHE_FLUSH_IS_SAFE 1
> +#endif
this ifdef should go away.
> +#endif /* _ASM_KGDB_H_ */
> +#endif /* __KERNEL__ */
and the __KERNEL__ aswell as these files are in no way exported
to userspace.
> +/*
> + * kgdb_get_shadow_thread - Get the shadowed _struct of @threadid.
> + * @regs: The pt_regs of the current thread.
> + * @threadid: The thread id of the shadowed process to get information on.
> + *
> + * RETURN:
> + * This returns a pointer to the task_struct of the shadowed
> + * thread, @threadid.
> + */
> +extern struct task_struct *kgdb_get_shadow_thread(struct pt_regs *regs,
> + int threadid);
So we have kerneldoc comments in both places now? Didn't you say
you converted these to something else?
> +++ b/kernel/kgdb.c
> @@ -0,0 +1,2019 @@
> +/*
> + * kernel/kgdb.c
Another one.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
* Christoph Hellwig <[EMAIL PROTECTED]> wrote: > > i dont think so. Which ones do you mean? I just reviewed them and > > they are either already done, or moot (for kgdb complications that i > > objected to and removed from this kgdb-x86 tree). > > The one I noticed quickly is the __ASSEMBLY__ removal from > asm-x86/kgdb.h. [...] people might want to experiment with early debug code as well and include asm-x86/kgdb.h in assembly files. So i kept that, it's sensible. > [...] I haven't looked at the serial bits because I don't think I'm > qualified to comment on those, but I'm also not seeing any replies to > any of his patches. Especially the comments on the arch interface > seem like something that should be acted upon to me. yeah - i also noticed that the serial subsystem is marked "orphaned" in the MAINTAINERS file: 8250/16?50 (AND CLONE UARTS) SERIAL DRIVER L: [EMAIL PROTECTED] W: http://serial.sourceforge.net S: Orphan and compared to the raw-lowlevel-serial-driver hackery that KGDB used to do this is a big step forward. Note that it's all dependent on CONFIG_KGDB. Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
* Ingo Molnar <[EMAIL PROTECTED]> wrote: > * Ingo Molnar <[EMAIL PROTECTED]> wrote: > > > Anyway, to resolve this i've turning them into non-docbook, > > descriptive comments. Please submit any docbook patch to > > arch/x86/kernel/kgdb.c to x86.git if you'd like more documentation. > > no need for that btw, i just added the docbook entries to > arch/x86/kernel/kgdb.c myself and fixed all of kgdb.h. Tree is at: > >git://git.kernel.org/pub/scm/linux/kernel/git/mingo/linux-2.6-kgdb.git > > tip 2e3ebf25b0bd8646e517806073e1991be1fec8a2. Shortlog, diffstat and > the full patch can be found below, with all relevant review feedback > addressed. Builds, boots and works fine on x86. here's gdb test-output from this 2e3ebf25b0bd kernel: (gdb) target remote /dev/ttyS0 Remote debugging using /dev/ttyS0 0x0046 in ?? () (gdb) i r eax0x11 17 ecx0x0 0 edx0xf4f4 62708 ebx0x0 0 esp0x80d09400 0x80d09400 ebp0xfffe37a7 0xfffe37a7 esi0x809162d0 -2137955632 edi0x -1 eip0x46 0x46 eflags 0x0 [ ] cs 0x80d093e4 -2133814300 ss 0x -1 ds 0x3fa6fe18 1067908632 es 0x8100 -32512 fs 0x3fa6fe18 1067908632 gs 0x8100 -32512 (gdb) Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
On Sun, Feb 10, 2008 at 09:57:35AM +0100, Ingo Molnar wrote: > > It would be nice if you could move the kerneldoc comments to the > > proper place at least. [...] > > i'd agree in general but this is really a special case, please look at > the context. This would duplicate all the kerneldoc headers for all > architectures. We'd have to move the same kerneldoc header to all > architecture arch/*/kernel/kgdb.c files. It's much nicer in > asm-generic/kgdb.h. Well, the point of kerneldoc comments is that they're reasily extractable. If you want to document the arch interface a pure text document in Documentation/ might be a better choice. > > Also it seems at least some of Jan's patches are missing aswell. > > i dont think so. Which ones do you mean? I just reviewed them and they > are either already done, or moot (for kgdb complications that i objected > to and removed from this kgdb-x86 tree). The one I noticed quickly is the __ASSEMBLY__ removal from asm-x86/kgdb.h. I haven't looked at the serial bits because I don't think I'm qualified to comment on those, but I'm also not seeing any replies to any of his patches. Especially the comments on the arch interface seem like something that should be acted upon to me. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
* Christoph Hellwig <[EMAIL PROTECTED]> wrote: > On Sun, Feb 10, 2008 at 09:21:32AM +0100, Ingo Molnar wrote: > > > Kerneldoc comments don't belong above the prototype of a function but > > > the function body. > > > > disagree - the best is to have it in both places - and in many > > places we do that. Anyway, this is up to maintainer discretion. > > Huh? In both places is the worst idea ever. It just means things > will 100% sure get out of sync. And the reason why it should be at > the function declaration is because that's where the kerneldoc tool > picks it up. Anyway, to resolve this i've turning them into non-docbook, descriptive comments. Please submit any docbook patch to arch/x86/kernel/kgdb.c to x86.git if you'd like more documentation. KGDB is already quite well documented. Trivially updated kgdb tree can be pulled from: git://git.kernel.org/pub/scm/linux/kernel/git/mingo/linux-2.6-kgdb.git tip a92381ae1a93b6e3bdba60a63972d2ebd6eb73f5. Shortlog and diffstat below. ( but i believe you are missing the big picture: duplicating the same information in all places, for functions that do _the same thing_ is pointless. It's much better to have a single, consistent set of information at the prototypes site. If docbook does not pick that up that's a docbook problem. Anyway, it's moot now with the latest tree. ) test-built and test-booted on x86. Ingo --> Ingo Molnar (3): pids: add pid_max prototype uaccess: add probe_kernel_write() x86: kgdb support Jan Kiszka (1): consoles: polling support, kgdboc Jason Wessel (2): kgdb: core kgdb: document parameters Documentation/kernel-parameters.txt |5 + arch/x86/Kconfig|4 + arch/x86/kernel/Makefile|1 + arch/x86/kernel/kgdb.c | 550 ++ drivers/char/tty_io.c | 47 + drivers/serial/8250.c | 62 ++ drivers/serial/Kconfig |3 + drivers/serial/Makefile |1 + drivers/serial/kgdboc.c | 164 +++ drivers/serial/serial_core.c| 67 ++- include/asm-generic/kgdb.h | 91 ++ include/asm-x86/kgdb.h | 85 ++ include/linux/kgdb.h| 333 ++ include/linux/pid.h |2 + include/linux/serial_core.h |4 + include/linux/tty_driver.h | 12 + include/linux/uaccess.h | 22 + kernel/Makefile |1 + kernel/kgdb.c | 2019 +++ kernel/sysctl.c |2 +- lib/Kconfig.debug |2 + lib/Kconfig.kgdb| 37 + 22 files changed, 3512 insertions(+), 2 deletions(-) create mode 100644 arch/x86/kernel/kgdb.c create mode 100644 drivers/serial/kgdboc.c create mode 100644 include/asm-generic/kgdb.h create mode 100644 include/asm-x86/kgdb.h create mode 100644 include/linux/kgdb.h create mode 100644 kernel/kgdb.c create mode 100644 lib/Kconfig.kgdb -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
* Christoph Hellwig <[EMAIL PROTECTED]> wrote: > [...] And while you're at it please remove all the filenames in the > top-of-file comments, not just in include/asm-generic/kgdb.h. fixed: there was just one such file remaining: include/linux/kgdb.h. > While we're at it is there a good reason to have that file at all, > it's just function prototypes, and I'd say for now they should just go > into linux/kgdb.h. If there's a a good reason why architectures > should implement them as inlines we can move them back, but looking at > the x86 implementation I doubt that's the case. agreed, done. > On Sun, Feb 10, 2008 at 09:02:25AM +0100, Ingo Molnar wrote: > > thanks - i found Sam's mail meanwhile and addressed most of the > > observations and updated the kgdb.git tree. I'll now check the threads > > above whether i missed anything. (feel free to point it out if you > > notice anything outright) As the changes have been janitorial only i > > refrain from reposting the series once again. The latest shortlog is > > below. > > It would be nice if you could move the kerneldoc comments to the > proper place at least. [...] i'd agree in general but this is really a special case, please look at the context. This would duplicate all the kerneldoc headers for all architectures. We'd have to move the same kerneldoc header to all architecture arch/*/kernel/kgdb.c files. It's much nicer in asm-generic/kgdb.h. > Also it seems at least some of Jan's patches are missing aswell. i dont think so. Which ones do you mean? I just reviewed them and they are either already done, or moot (for kgdb complications that i objected to and removed from this kgdb-x86 tree). anyway, i've implemented all these (trivial) tweaks you just mentioned and re-tested on 32-bit and 64-bit x86, backmerged the fixes to their proper places, and pushed the clean series out again to: git://git.kernel.org/pub/scm/linux/kernel/git/mingo/linux-2.6-kgdb.git tip e6ba396b65e2f08afb5d8924140b126427085203. Shortlog below. Ingo --> Ingo Molnar (3): pids: add pid_max prototype uaccess: add probe_kernel_write() x86: kgdb support Jan Kiszka (1): consoles: polling support, kgdboc Jason Wessel (2): kgdb: core kgdb: document parameters Documentation/kernel-parameters.txt |5 + arch/x86/Kconfig|4 + arch/x86/kernel/Makefile|1 + arch/x86/kernel/kgdb.c | 550 ++ drivers/char/tty_io.c | 47 + drivers/serial/8250.c | 62 ++ drivers/serial/Kconfig |3 + drivers/serial/Makefile |1 + drivers/serial/kgdboc.c | 164 +++ drivers/serial/serial_core.c| 67 ++- include/asm-generic/kgdb.h | 91 ++ include/asm-x86/kgdb.h | 85 ++ include/linux/kgdb.h| 333 ++ include/linux/pid.h |2 + include/linux/serial_core.h |4 + include/linux/tty_driver.h | 12 + include/linux/uaccess.h | 22 + kernel/Makefile |1 + kernel/kgdb.c | 2019 +++ kernel/sysctl.c |2 +- lib/Kconfig.debug |2 + lib/Kconfig.kgdb| 37 + 22 files changed, 3512 insertions(+), 2 deletions(-) create mode 100644 arch/x86/kernel/kgdb.c create mode 100644 drivers/serial/kgdboc.c create mode 100644 include/asm-generic/kgdb.h create mode 100644 include/asm-x86/kgdb.h create mode 100644 include/linux/kgdb.h create mode 100644 kernel/kgdb.c create mode 100644 lib/Kconfig.kgdb -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
On Sun, Feb 10, 2008 at 09:21:32AM +0100, Ingo Molnar wrote: > > Kerneldoc comments don't belong above the prototype of a function but > > the function body. > > disagree - the best is to have it in both places - and in many places we > do that. Anyway, this is up to maintainer discretion. Huh? In both places is the worst idea ever. It just means things will 100% sure get out of sync. And the reason why it should be at the function declaration is because that's where the kerneldoc tool picks it up. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
On Sun, Feb 10, 2008 at 09:02:25AM +0100, Ingo Molnar wrote: > thanks - i found Sam's mail meanwhile and addressed most of the > observations and updated the kgdb.git tree. I'll now check the threads > above whether i missed anything. (feel free to point it out if you > notice anything outright) As the changes have been janitorial only i > refrain from reposting the series once again. The latest shortlog is > below. It would be nice if you could move the kerneldoc comments to the proper place at least. And while you're at it please remove all the filenames in the top-of-file comments, not just in include/asm-generic/kgdb.h. While we're at it is there a good reason to have that file at all, it's just function prototypes, and I'd say for now they should just go into linux/kgdb.h. If there's a a good reason why architectures should implement them as inlines we can move them back, but looking at the x86 implementation I doubt that's the case. Also it seems at least some of Jan's patches are missing aswell. I think we really shouldn't rush this too much. Let's wait until Monday at least when Jason and Jan are back. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
* Ingo Molnar <[EMAIL PROTECTED]> wrote:
> > It's all in the thread starting with '[PATCH 0/8] kgdb 2.6.25
> > version', msgid
> > [EMAIL PROTECTED] or at
> > http://lkml.org/lkml/2008/2/9/104
>
> thanks - i found Sam's mail meanwhile and addressed most of the
> observations and updated the kgdb.git tree. I'll now check the threads
> above whether i missed anything. (feel free to point it out if you
> notice anything outright) As the changes have been janitorial only i
> refrain from reposting the series once again. The latest shortlog is
> below.
i've read all that thread now and i think all your observations are
addressed in the latest tree i posted. In fact, most of the
non-syntactic observations you made i already addressed in my series
from yesterday. Find the latest tree at:
git://git.kernel.org/pub/scm/linux/kernel/git/mingo/linux-2.6-kgdb.git
with tip commit 8fbf71f7636bd26843de01b4bdf819c9a9777427. Shortlog and
diffstat below. I backmerged all the fixlets into their original
commits, to keep the splitup clean.
Here are my replies to your feedback:
Date: Sat, 9 Feb 2008 12:10:26 -0500
From: Christoph Hellwig <[EMAIL PROTECTED]>
To: [EMAIL PROTECTED]
Subject: Re: [PATCH 2/8] pid, kgdb: add pid_max prototype
addressed.
Date: Sat, 9 Feb 2008 12:15:03 -0500
From: Christoph Hellwig <[EMAIL PROTECTED]>
Subject: Re: [PATCH 3/8] kgdb, modules: Always allow module sect info for
addressed: this was mooted by my original posting from yesterday
already - i removed this complication.
Date: Sat, 9 Feb 2008 12:16:05 -0500
From: Christoph Hellwig <[EMAIL PROTECTED]>
Subject: Re: [PATCH 4/8] kgdb: COPTIMIZE flag
addressed: this was mooted by my original posting from yesterday
already - i removed this complication.
> > + * include/asm-generic/kgdb.h
>
> Please don't mention the file name in the top-of-file comments. This
> information is redundant and will easily get out of date when moving
> files around or copying them. Note that this applies to basically any
> file in this patch.
fixed.
> > +#ifdef CONFIG_X86
> > +/**
> > + * kgdb_skipexception - Bail of of KGDB when we've been triggered.
>
> arch ifdefs don't belong into an asm-generic/ file. Please have a
> proper asm-x86/kgdb.h that defines these things.
addressed: this was fixed in my submission yesterday.
> Kerneldoc comments don't belong above the prototype of a function but
> the function body.
disagree - the best is to have it in both places - and in many places we
do that. Anyway, this is up to maintainer discretion.
> > +#ifdef CONFIG_KGDB_ARCH_HAS_SHADOW_INFO
> > +/**
> > + * kgdb_shadowinfo - Get shadowed information on @threadid.
> > + * @regs: The pt_regs of the current process.
> > + * @buffer: A buffer of %BUFMAX size.
> > + * @threadid: The thread id of the shadowed process to get information on.
> > + */
> > +extern void kgdb_shadowinfo(struct pt_regs *regs, char *buffer,
> > + unsigned threadid);
>
> I don't really thing this belongs into an asm-generic header, again.
> ARchitectures having shadow info should just provide this in their own
> asm-foo/kgdb.h. Or better yet just kill it for the first submission.
disagree. Many architectures will select shadow-info support so having
this in asm-generic/kgdb.h is straightforward. I am actually an
architecture who had to deal with this stuff in 32-bit (no shadow info
support) and 64-bit (shadow info support) and this was handy and
obvious. (But note that the patch submitted by Jason had a few
uglinesses in this area that i fixed so please re-check the ones in my
tree.)
> > +struct debuggerinfo_struct {
> > + void*debuggerinfo;
> > + struct task_struct *task;
> > +} kgdb_info[NR_CPUS];
>
> shouldn't this use per-cpu data? Or is that in some way to fragile
> for a debugger?
yes, eventually we might want to use kgdb earlier than the per CPU areas
are set up.
> > +/* reboot notifier block */
> > +static struct notifier_block kgdb_reboot_notifier = {
> > + .notifier_call = kgdb_notify_reboot,
> > + .next = NULL,
> > + .priority = INT_MAX,
> > +};
>
> No need to initialize fields to 0 or NULL in static variables.
agreed, fixed.
> > + 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;
>
> lots of superflous braces. More of them later in this file in the
> same style.
maintainer discretion item. I prefer having such clarity in operator
ordering.
> > +#ifdef __BIG_ENDIAN
> > + *buf++ = hexchars[(tmp_s >> 12) & 0xf];
> > + *buf++ = hexchars[(tmp_s >> 8) & 0xf];
> > + *buf++ = hexchars[(tmp_s >> 4) & 0xf];
> > + *buf++ = hexchars[tmp_s & 0xf];
> > +#else
> > + *buf++ = hexchars[(tmp_s >> 4) & 0xf];
> > +
Re: [3/6] kgdb: core
* Christoph Hellwig <[EMAIL PROTECTED]> wrote: > On Sun, Feb 10, 2008 at 08:43:52AM +0100, Ingo Molnar wrote: > > > > * Christoph Hellwig <[EMAIL PROTECTED]> wrote: > > > > > This still doesn't address a lot of the review comments from Jason's > > > last posting. > > > > sorry, which mails are those? > > It's all in the thread starting with '[PATCH 0/8] kgdb 2.6.25 > version', msgid > [EMAIL PROTECTED] or at > http://lkml.org/lkml/2008/2/9/104 thanks - i found Sam's mail meanwhile and addressed most of the observations and updated the kgdb.git tree. I'll now check the threads above whether i missed anything. (feel free to point it out if you notice anything outright) As the changes have been janitorial only i refrain from reposting the series once again. The latest shortlog is below. Ingo --> Ingo Molnar (3): pids: add pid_max prototype uaccess: add probe_kernel_write() x86: kgdb support Jan Kiszka (1): consoles: polling support, kgdboc Jason Wessel (2): kgdb: core kgdb: document parameters Documentation/kernel-parameters.txt |5 + arch/x86/Kconfig|4 + arch/x86/kernel/Makefile|1 + arch/x86/kernel/kgdb.c | 550 ++ drivers/char/tty_io.c | 47 + drivers/serial/8250.c | 62 ++ drivers/serial/Kconfig |3 + drivers/serial/Makefile |1 + drivers/serial/kgdboc.c | 164 +++ drivers/serial/serial_core.c| 67 ++- include/asm-generic/kgdb.h | 93 ++ include/asm-x86/kgdb.h | 87 ++ include/linux/kgdb.h| 264 + include/linux/pid.h |2 + include/linux/serial_core.h |4 + include/linux/tty_driver.h | 12 + include/linux/uaccess.h | 22 + kernel/Makefile |1 + kernel/kgdb.c | 2020 +++ kernel/sysctl.c |2 +- lib/Kconfig.debug |2 + lib/Kconfig.kgdb| 37 + 22 files changed, 3448 insertions(+), 2 deletions(-) create mode 100644 arch/x86/kernel/kgdb.c create mode 100644 drivers/serial/kgdboc.c create mode 100644 include/asm-generic/kgdb.h create mode 100644 include/asm-x86/kgdb.h create mode 100644 include/linux/kgdb.h create mode 100644 kernel/kgdb.c create mode 100644 lib/Kconfig.kgdb -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
* Sam Ravnborg <[EMAIL PROTECTED]> wrote:
> I see that only a very few of my comments posted yesterday got
> addressed. On purpose or did you miss them?
no, they went into another thread :-)
i've now read your mail and addressed the majority of them - see the
details below.
i've trickled all these fixes back to keep a clean split, test-built and
test-booted the result, and updated the kgdb.git tree, which can be
pulled from:
git://git.kernel.org/pub/scm/linux/kernel/git/mingo/linux-2.6-kgdb.git
updated shortlog further below. I've re-tested kgdb on x86 and it still
works (as expected).
> > +struct debuggerinfo_struct {
> > + void*debuggerinfo;
> > + struct task_struct *task;
> > +} kgdb_info[NR_CPUS];
> static?
fixed.
> > +
> > +/* Is a host GDB connected to us? */
> > +intkgdb_connected;
> > +EXPORT_SYMBOL_GPL(kgdb_connected);
> Drop additional spaces.
> Add kernel-doc comments explaining the usage.
if you look at the resulting kernel/kgdb.c not the patch itself then
you'll see that this is consistent style that aligns this variable with
other fields. I agree that it looks ugly in isolation in the quote
above.
> > +/* All the KGDB handlers are installed */
> > +intkgdb_io_module_registered;
> static? drop spaces.
static: fixed. Spaces: see above.
> > +/* Guard for recursive entry */
> > +static int exception_level;
> drop spaces. In more places below - but they are obvious.
really, please look at the resulting kernel/kgdb.c file. It's visually
consistent.
> > +struct kgdb_bkpt kgdb_break[KGDB_MAX_BREAKPOINTS] = {
> > + [0 ... KGDB_MAX_BREAKPOINTS-1] = { .state = BP_UNDEFINED }
> > +};
> static?
fixed.
> > +
> > +extern int pid_max;
>
> extern must be moved to a .h file.
i did that in my series.
> > +atomic_t kgdb_setting_breakpoint;
> static?
fixed.
> Many more variables are static candidates. I will not repeat it.
i think i fixed all of them.
> > +#ifdef __BIG_ENDIAN
> > + *buf++ = hexchars[(tmp_s >> 12) & 0xf];
> > + *buf++ = hexchars[(tmp_s >> 8) & 0xf];
> > + *buf++ = hexchars[(tmp_s >> 4) & 0xf];
> > + *buf++ = hexchars[tmp_s & 0xf];
> > +#else
> > + *buf++ = hexchars[(tmp_s >> 4) & 0xf];
> > + *buf++ = hexchars[tmp_s & 0xf];
> > + *buf++ = hexchars[(tmp_s >> 12) & 0xf];
> > + *buf++ = hexchars[(tmp_s >> 8) & 0xf];
> > +#endif
> small helper function?
this is already part of a small helper function. (kgdb_mem2hex())
> > +int kgdb_isremovedbreak(unsigned long addr)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) {
> > + if ((kgdb_break[i].state == BP_REMOVED) &&
> > + (kgdb_break[i].bpt_addr == addr))
> > + return 1;
> > + }
> > + return 0;
> > +}
> static?
no, needed by architectures.
> > +int remove_all_break(void)
> static?
no.
> > +int kgdb_io_ready(int print_wait)
> static?
yes, fixed.
> > + bool "KGDB: kernel debugging with remote gdb"
> > + select KGDB_ARCH_HAS_SHADOW_INFO if X86_64
> > + select DEBUG_INFO
> > + select FRAME_POINTER
> > + depends on DEBUG_KERNEL && ADD_A_KGDB_ARCH
>
> Replace ADD_A_...
> with
> HAVE_KGDB
fixed.
Ingo
-->
Ingo Molnar (3):
pids: add pid_max prototype
uaccess: add probe_kernel_write()
x86: kgdb support
Jan Kiszka (1):
consoles: polling support, kgdboc
Jason Wessel (2):
kgdb: core
kgdb: document parameters
Documentation/kernel-parameters.txt |5 +
arch/x86/Kconfig|4 +
arch/x86/kernel/Makefile|1 +
arch/x86/kernel/kgdb.c | 550 ++
drivers/char/tty_io.c | 47 +
drivers/serial/8250.c | 62 ++
drivers/serial/Kconfig |3 +
drivers/serial/Makefile |1 +
drivers/serial/kgdboc.c | 164 +++
drivers/serial/serial_core.c| 67 ++-
include/asm-generic/kgdb.h | 93 ++
include/asm-x86/kgdb.h | 87 ++
include/linux/kgdb.h| 264 +
include/linux/pid.h |2 +
include/linux/serial_core.h |4 +
include/linux/tty_driver.h | 12 +
include/linux/uaccess.h | 22 +
kernel/Makefile |1 +
kernel/kgdb.c | 2020 +++
kernel/sysctl.c |2 +-
lib/Kconfig.debug |2 +
lib/Kconfig.kgdb| 37 +
22 files changed, 3448 insertions(+), 2 deletions(-)
create mode 100644 arch/x86/kernel/kgdb.c
create mode 100644 drivers/serial/kgdboc.c
create mode 100644 include/asm-generic/kgdb.h
create mode 100644 include/asm-x86/kgdb.h
create mode 100644 include/linux/kgdb.h
create mode
Re: [3/6] kgdb: core
* Sam Ravnborg [EMAIL PROTECTED] wrote:
I see that only a very few of my comments posted yesterday got
addressed. On purpose or did you miss them?
no, they went into another thread :-)
i've now read your mail and addressed the majority of them - see the
details below.
i've trickled all these fixes back to keep a clean split, test-built and
test-booted the result, and updated the kgdb.git tree, which can be
pulled from:
git://git.kernel.org/pub/scm/linux/kernel/git/mingo/linux-2.6-kgdb.git
updated shortlog further below. I've re-tested kgdb on x86 and it still
works (as expected).
+struct debuggerinfo_struct {
+ void*debuggerinfo;
+ struct task_struct *task;
+} kgdb_info[NR_CPUS];
static?
fixed.
+
+/* Is a host GDB connected to us? */
+intkgdb_connected;
+EXPORT_SYMBOL_GPL(kgdb_connected);
Drop additional spaces.
Add kernel-doc comments explaining the usage.
if you look at the resulting kernel/kgdb.c not the patch itself then
you'll see that this is consistent style that aligns this variable with
other fields. I agree that it looks ugly in isolation in the quote
above.
+/* All the KGDB handlers are installed */
+intkgdb_io_module_registered;
static? drop spaces.
static: fixed. Spaces: see above.
+/* Guard for recursive entry */
+static int exception_level;
drop spaces. In more places below - but they are obvious.
really, please look at the resulting kernel/kgdb.c file. It's visually
consistent.
+struct kgdb_bkpt kgdb_break[KGDB_MAX_BREAKPOINTS] = {
+ [0 ... KGDB_MAX_BREAKPOINTS-1] = { .state = BP_UNDEFINED }
+};
static?
fixed.
+
+extern int pid_max;
extern must be moved to a .h file.
i did that in my series.
+atomic_t kgdb_setting_breakpoint;
static?
fixed.
Many more variables are static candidates. I will not repeat it.
i think i fixed all of them.
+#ifdef __BIG_ENDIAN
+ *buf++ = hexchars[(tmp_s 12) 0xf];
+ *buf++ = hexchars[(tmp_s 8) 0xf];
+ *buf++ = hexchars[(tmp_s 4) 0xf];
+ *buf++ = hexchars[tmp_s 0xf];
+#else
+ *buf++ = hexchars[(tmp_s 4) 0xf];
+ *buf++ = hexchars[tmp_s 0xf];
+ *buf++ = hexchars[(tmp_s 12) 0xf];
+ *buf++ = hexchars[(tmp_s 8) 0xf];
+#endif
small helper function?
this is already part of a small helper function. (kgdb_mem2hex())
+int kgdb_isremovedbreak(unsigned long addr)
+{
+ int i;
+
+ for (i = 0; i KGDB_MAX_BREAKPOINTS; i++) {
+ if ((kgdb_break[i].state == BP_REMOVED)
+ (kgdb_break[i].bpt_addr == addr))
+ return 1;
+ }
+ return 0;
+}
static?
no, needed by architectures.
+int remove_all_break(void)
static?
no.
+int kgdb_io_ready(int print_wait)
static?
yes, fixed.
+ bool KGDB: kernel debugging with remote gdb
+ select KGDB_ARCH_HAS_SHADOW_INFO if X86_64
+ select DEBUG_INFO
+ select FRAME_POINTER
+ depends on DEBUG_KERNEL ADD_A_KGDB_ARCH
Replace ADD_A_...
with
HAVE_KGDB
fixed.
Ingo
--
Ingo Molnar (3):
pids: add pid_max prototype
uaccess: add probe_kernel_write()
x86: kgdb support
Jan Kiszka (1):
consoles: polling support, kgdboc
Jason Wessel (2):
kgdb: core
kgdb: document parameters
Documentation/kernel-parameters.txt |5 +
arch/x86/Kconfig|4 +
arch/x86/kernel/Makefile|1 +
arch/x86/kernel/kgdb.c | 550 ++
drivers/char/tty_io.c | 47 +
drivers/serial/8250.c | 62 ++
drivers/serial/Kconfig |3 +
drivers/serial/Makefile |1 +
drivers/serial/kgdboc.c | 164 +++
drivers/serial/serial_core.c| 67 ++-
include/asm-generic/kgdb.h | 93 ++
include/asm-x86/kgdb.h | 87 ++
include/linux/kgdb.h| 264 +
include/linux/pid.h |2 +
include/linux/serial_core.h |4 +
include/linux/tty_driver.h | 12 +
include/linux/uaccess.h | 22 +
kernel/Makefile |1 +
kernel/kgdb.c | 2020 +++
kernel/sysctl.c |2 +-
lib/Kconfig.debug |2 +
lib/Kconfig.kgdb| 37 +
22 files changed, 3448 insertions(+), 2 deletions(-)
create mode 100644 arch/x86/kernel/kgdb.c
create mode 100644 drivers/serial/kgdboc.c
create mode 100644 include/asm-generic/kgdb.h
create mode 100644 include/asm-x86/kgdb.h
create mode 100644 include/linux/kgdb.h
create mode 100644 kernel/kgdb.c
create mode 100644 lib/Kconfig.kgdb
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body
Re: [3/6] kgdb: core
* Christoph Hellwig [EMAIL PROTECTED] wrote: On Sun, Feb 10, 2008 at 08:43:52AM +0100, Ingo Molnar wrote: * Christoph Hellwig [EMAIL PROTECTED] wrote: This still doesn't address a lot of the review comments from Jason's last posting. sorry, which mails are those? It's all in the thread starting with '[PATCH 0/8] kgdb 2.6.25 version', msgid [EMAIL PROTECTED] or at http://lkml.org/lkml/2008/2/9/104 thanks - i found Sam's mail meanwhile and addressed most of the observations and updated the kgdb.git tree. I'll now check the threads above whether i missed anything. (feel free to point it out if you notice anything outright) As the changes have been janitorial only i refrain from reposting the series once again. The latest shortlog is below. Ingo -- Ingo Molnar (3): pids: add pid_max prototype uaccess: add probe_kernel_write() x86: kgdb support Jan Kiszka (1): consoles: polling support, kgdboc Jason Wessel (2): kgdb: core kgdb: document parameters Documentation/kernel-parameters.txt |5 + arch/x86/Kconfig|4 + arch/x86/kernel/Makefile|1 + arch/x86/kernel/kgdb.c | 550 ++ drivers/char/tty_io.c | 47 + drivers/serial/8250.c | 62 ++ drivers/serial/Kconfig |3 + drivers/serial/Makefile |1 + drivers/serial/kgdboc.c | 164 +++ drivers/serial/serial_core.c| 67 ++- include/asm-generic/kgdb.h | 93 ++ include/asm-x86/kgdb.h | 87 ++ include/linux/kgdb.h| 264 + include/linux/pid.h |2 + include/linux/serial_core.h |4 + include/linux/tty_driver.h | 12 + include/linux/uaccess.h | 22 + kernel/Makefile |1 + kernel/kgdb.c | 2020 +++ kernel/sysctl.c |2 +- lib/Kconfig.debug |2 + lib/Kconfig.kgdb| 37 + 22 files changed, 3448 insertions(+), 2 deletions(-) create mode 100644 arch/x86/kernel/kgdb.c create mode 100644 drivers/serial/kgdboc.c create mode 100644 include/asm-generic/kgdb.h create mode 100644 include/asm-x86/kgdb.h create mode 100644 include/linux/kgdb.h create mode 100644 kernel/kgdb.c create mode 100644 lib/Kconfig.kgdb -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
* Ingo Molnar [EMAIL PROTECTED] wrote:
It's all in the thread starting with '[PATCH 0/8] kgdb 2.6.25
version', msgid
[EMAIL PROTECTED] or at
http://lkml.org/lkml/2008/2/9/104
thanks - i found Sam's mail meanwhile and addressed most of the
observations and updated the kgdb.git tree. I'll now check the threads
above whether i missed anything. (feel free to point it out if you
notice anything outright) As the changes have been janitorial only i
refrain from reposting the series once again. The latest shortlog is
below.
i've read all that thread now and i think all your observations are
addressed in the latest tree i posted. In fact, most of the
non-syntactic observations you made i already addressed in my series
from yesterday. Find the latest tree at:
git://git.kernel.org/pub/scm/linux/kernel/git/mingo/linux-2.6-kgdb.git
with tip commit 8fbf71f7636bd26843de01b4bdf819c9a9777427. Shortlog and
diffstat below. I backmerged all the fixlets into their original
commits, to keep the splitup clean.
Here are my replies to your feedback:
Date: Sat, 9 Feb 2008 12:10:26 -0500
From: Christoph Hellwig [EMAIL PROTECTED]
To: [EMAIL PROTECTED]
Subject: Re: [PATCH 2/8] pid, kgdb: add pid_max prototype
addressed.
Date: Sat, 9 Feb 2008 12:15:03 -0500
From: Christoph Hellwig [EMAIL PROTECTED]
Subject: Re: [PATCH 3/8] kgdb, modules: Always allow module sect info for
addressed: this was mooted by my original posting from yesterday
already - i removed this complication.
Date: Sat, 9 Feb 2008 12:16:05 -0500
From: Christoph Hellwig [EMAIL PROTECTED]
Subject: Re: [PATCH 4/8] kgdb: COPTIMIZE flag
addressed: this was mooted by my original posting from yesterday
already - i removed this complication.
+ * include/asm-generic/kgdb.h
Please don't mention the file name in the top-of-file comments. This
information is redundant and will easily get out of date when moving
files around or copying them. Note that this applies to basically any
file in this patch.
fixed.
+#ifdef CONFIG_X86
+/**
+ * kgdb_skipexception - Bail of of KGDB when we've been triggered.
arch ifdefs don't belong into an asm-generic/ file. Please have a
proper asm-x86/kgdb.h that defines these things.
addressed: this was fixed in my submission yesterday.
Kerneldoc comments don't belong above the prototype of a function but
the function body.
disagree - the best is to have it in both places - and in many places we
do that. Anyway, this is up to maintainer discretion.
+#ifdef CONFIG_KGDB_ARCH_HAS_SHADOW_INFO
+/**
+ * kgdb_shadowinfo - Get shadowed information on @threadid.
+ * @regs: The struct pt_regs of the current process.
+ * @buffer: A buffer of %BUFMAX size.
+ * @threadid: The thread id of the shadowed process to get information on.
+ */
+extern void kgdb_shadowinfo(struct pt_regs *regs, char *buffer,
+ unsigned threadid);
I don't really thing this belongs into an asm-generic header, again.
ARchitectures having shadow info should just provide this in their own
asm-foo/kgdb.h. Or better yet just kill it for the first submission.
disagree. Many architectures will select shadow-info support so having
this in asm-generic/kgdb.h is straightforward. I am actually an
architecture who had to deal with this stuff in 32-bit (no shadow info
support) and 64-bit (shadow info support) and this was handy and
obvious. (But note that the patch submitted by Jason had a few
uglinesses in this area that i fixed so please re-check the ones in my
tree.)
+struct debuggerinfo_struct {
+ void*debuggerinfo;
+ struct task_struct *task;
+} kgdb_info[NR_CPUS];
shouldn't this use per-cpu data? Or is that in some way to fragile
for a debugger?
yes, eventually we might want to use kgdb earlier than the per CPU areas
are set up.
+/* reboot notifier block */
+static struct notifier_block kgdb_reboot_notifier = {
+ .notifier_call = kgdb_notify_reboot,
+ .next = NULL,
+ .priority = INT_MAX,
+};
No need to initialize fields to 0 or NULL in static variables.
agreed, fixed.
+ 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;
lots of superflous braces. More of them later in this file in the
same style.
maintainer discretion item. I prefer having such clarity in operator
ordering.
+#ifdef __BIG_ENDIAN
+ *buf++ = hexchars[(tmp_s 12) 0xf];
+ *buf++ = hexchars[(tmp_s 8) 0xf];
+ *buf++ = hexchars[(tmp_s 4) 0xf];
+ *buf++ = hexchars[tmp_s 0xf];
+#else
+ *buf++ = hexchars[(tmp_s 4) 0xf];
+ *buf++ = hexchars[tmp_s 0xf];
+ *buf++ = hexchars[(tmp_s 12) 0xf];
+ *buf++ = hexchars[(tmp_s 8) 0xf];
Re: [3/6] kgdb: core
On Sun, Feb 10, 2008 at 09:02:25AM +0100, Ingo Molnar wrote: thanks - i found Sam's mail meanwhile and addressed most of the observations and updated the kgdb.git tree. I'll now check the threads above whether i missed anything. (feel free to point it out if you notice anything outright) As the changes have been janitorial only i refrain from reposting the series once again. The latest shortlog is below. It would be nice if you could move the kerneldoc comments to the proper place at least. And while you're at it please remove all the filenames in the top-of-file comments, not just in include/asm-generic/kgdb.h. While we're at it is there a good reason to have that file at all, it's just function prototypes, and I'd say for now they should just go into linux/kgdb.h. If there's a a good reason why architectures should implement them as inlines we can move them back, but looking at the x86 implementation I doubt that's the case. Also it seems at least some of Jan's patches are missing aswell. I think we really shouldn't rush this too much. Let's wait until Monday at least when Jason and Jan are back. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
On Sun, Feb 10, 2008 at 09:21:32AM +0100, Ingo Molnar wrote: Kerneldoc comments don't belong above the prototype of a function but the function body. disagree - the best is to have it in both places - and in many places we do that. Anyway, this is up to maintainer discretion. Huh? In both places is the worst idea ever. It just means things will 100% sure get out of sync. And the reason why it should be at the function declaration is because that's where the kerneldoc tool picks it up. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
* Christoph Hellwig [EMAIL PROTECTED] wrote: [...] And while you're at it please remove all the filenames in the top-of-file comments, not just in include/asm-generic/kgdb.h. fixed: there was just one such file remaining: include/linux/kgdb.h. While we're at it is there a good reason to have that file at all, it's just function prototypes, and I'd say for now they should just go into linux/kgdb.h. If there's a a good reason why architectures should implement them as inlines we can move them back, but looking at the x86 implementation I doubt that's the case. agreed, done. On Sun, Feb 10, 2008 at 09:02:25AM +0100, Ingo Molnar wrote: thanks - i found Sam's mail meanwhile and addressed most of the observations and updated the kgdb.git tree. I'll now check the threads above whether i missed anything. (feel free to point it out if you notice anything outright) As the changes have been janitorial only i refrain from reposting the series once again. The latest shortlog is below. It would be nice if you could move the kerneldoc comments to the proper place at least. [...] i'd agree in general but this is really a special case, please look at the context. This would duplicate all the kerneldoc headers for all architectures. We'd have to move the same kerneldoc header to all architecture arch/*/kernel/kgdb.c files. It's much nicer in asm-generic/kgdb.h. Also it seems at least some of Jan's patches are missing aswell. i dont think so. Which ones do you mean? I just reviewed them and they are either already done, or moot (for kgdb complications that i objected to and removed from this kgdb-x86 tree). anyway, i've implemented all these (trivial) tweaks you just mentioned and re-tested on 32-bit and 64-bit x86, backmerged the fixes to their proper places, and pushed the clean series out again to: git://git.kernel.org/pub/scm/linux/kernel/git/mingo/linux-2.6-kgdb.git tip e6ba396b65e2f08afb5d8924140b126427085203. Shortlog below. Ingo -- Ingo Molnar (3): pids: add pid_max prototype uaccess: add probe_kernel_write() x86: kgdb support Jan Kiszka (1): consoles: polling support, kgdboc Jason Wessel (2): kgdb: core kgdb: document parameters Documentation/kernel-parameters.txt |5 + arch/x86/Kconfig|4 + arch/x86/kernel/Makefile|1 + arch/x86/kernel/kgdb.c | 550 ++ drivers/char/tty_io.c | 47 + drivers/serial/8250.c | 62 ++ drivers/serial/Kconfig |3 + drivers/serial/Makefile |1 + drivers/serial/kgdboc.c | 164 +++ drivers/serial/serial_core.c| 67 ++- include/asm-generic/kgdb.h | 91 ++ include/asm-x86/kgdb.h | 85 ++ include/linux/kgdb.h| 333 ++ include/linux/pid.h |2 + include/linux/serial_core.h |4 + include/linux/tty_driver.h | 12 + include/linux/uaccess.h | 22 + kernel/Makefile |1 + kernel/kgdb.c | 2019 +++ kernel/sysctl.c |2 +- lib/Kconfig.debug |2 + lib/Kconfig.kgdb| 37 + 22 files changed, 3512 insertions(+), 2 deletions(-) create mode 100644 arch/x86/kernel/kgdb.c create mode 100644 drivers/serial/kgdboc.c create mode 100644 include/asm-generic/kgdb.h create mode 100644 include/asm-x86/kgdb.h create mode 100644 include/linux/kgdb.h create mode 100644 kernel/kgdb.c create mode 100644 lib/Kconfig.kgdb -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
On Sun, Feb 10, 2008 at 09:57:35AM +0100, Ingo Molnar wrote: It would be nice if you could move the kerneldoc comments to the proper place at least. [...] i'd agree in general but this is really a special case, please look at the context. This would duplicate all the kerneldoc headers for all architectures. We'd have to move the same kerneldoc header to all architecture arch/*/kernel/kgdb.c files. It's much nicer in asm-generic/kgdb.h. Well, the point of kerneldoc comments is that they're reasily extractable. If you want to document the arch interface a pure text document in Documentation/ might be a better choice. Also it seems at least some of Jan's patches are missing aswell. i dont think so. Which ones do you mean? I just reviewed them and they are either already done, or moot (for kgdb complications that i objected to and removed from this kgdb-x86 tree). The one I noticed quickly is the __ASSEMBLY__ removal from asm-x86/kgdb.h. I haven't looked at the serial bits because I don't think I'm qualified to comment on those, but I'm also not seeing any replies to any of his patches. Especially the comments on the arch interface seem like something that should be acted upon to me. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
* Ingo Molnar [EMAIL PROTECTED] wrote: * Ingo Molnar [EMAIL PROTECTED] wrote: Anyway, to resolve this i've turning them into non-docbook, descriptive comments. Please submit any docbook patch to arch/x86/kernel/kgdb.c to x86.git if you'd like more documentation. no need for that btw, i just added the docbook entries to arch/x86/kernel/kgdb.c myself and fixed all of kgdb.h. Tree is at: git://git.kernel.org/pub/scm/linux/kernel/git/mingo/linux-2.6-kgdb.git tip 2e3ebf25b0bd8646e517806073e1991be1fec8a2. Shortlog, diffstat and the full patch can be found below, with all relevant review feedback addressed. Builds, boots and works fine on x86. here's gdb test-output from this 2e3ebf25b0bd kernel: (gdb) target remote /dev/ttyS0 Remote debugging using /dev/ttyS0 0x0046 in ?? () (gdb) i r eax0x11 17 ecx0x0 0 edx0xf4f4 62708 ebx0x0 0 esp0x80d09400 0x80d09400 ebp0xfffe37a7 0xfffe37a7 esi0x809162d0 -2137955632 edi0x -1 eip0x46 0x46 eflags 0x0 [ ] cs 0x80d093e4 -2133814300 ss 0x -1 ds 0x3fa6fe18 1067908632 es 0x8100 -32512 fs 0x3fa6fe18 1067908632 gs 0x8100 -32512 (gdb) Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
* Christoph Hellwig [EMAIL PROTECTED] wrote: On Sun, Feb 10, 2008 at 09:21:32AM +0100, Ingo Molnar wrote: Kerneldoc comments don't belong above the prototype of a function but the function body. disagree - the best is to have it in both places - and in many places we do that. Anyway, this is up to maintainer discretion. Huh? In both places is the worst idea ever. It just means things will 100% sure get out of sync. And the reason why it should be at the function declaration is because that's where the kerneldoc tool picks it up. Anyway, to resolve this i've turning them into non-docbook, descriptive comments. Please submit any docbook patch to arch/x86/kernel/kgdb.c to x86.git if you'd like more documentation. KGDB is already quite well documented. Trivially updated kgdb tree can be pulled from: git://git.kernel.org/pub/scm/linux/kernel/git/mingo/linux-2.6-kgdb.git tip a92381ae1a93b6e3bdba60a63972d2ebd6eb73f5. Shortlog and diffstat below. ( but i believe you are missing the big picture: duplicating the same information in all places, for functions that do _the same thing_ is pointless. It's much better to have a single, consistent set of information at the prototypes site. If docbook does not pick that up that's a docbook problem. Anyway, it's moot now with the latest tree. ) test-built and test-booted on x86. Ingo -- Ingo Molnar (3): pids: add pid_max prototype uaccess: add probe_kernel_write() x86: kgdb support Jan Kiszka (1): consoles: polling support, kgdboc Jason Wessel (2): kgdb: core kgdb: document parameters Documentation/kernel-parameters.txt |5 + arch/x86/Kconfig|4 + arch/x86/kernel/Makefile|1 + arch/x86/kernel/kgdb.c | 550 ++ drivers/char/tty_io.c | 47 + drivers/serial/8250.c | 62 ++ drivers/serial/Kconfig |3 + drivers/serial/Makefile |1 + drivers/serial/kgdboc.c | 164 +++ drivers/serial/serial_core.c| 67 ++- include/asm-generic/kgdb.h | 91 ++ include/asm-x86/kgdb.h | 85 ++ include/linux/kgdb.h| 333 ++ include/linux/pid.h |2 + include/linux/serial_core.h |4 + include/linux/tty_driver.h | 12 + include/linux/uaccess.h | 22 + kernel/Makefile |1 + kernel/kgdb.c | 2019 +++ kernel/sysctl.c |2 +- lib/Kconfig.debug |2 + lib/Kconfig.kgdb| 37 + 22 files changed, 3512 insertions(+), 2 deletions(-) create mode 100644 arch/x86/kernel/kgdb.c create mode 100644 drivers/serial/kgdboc.c create mode 100644 include/asm-generic/kgdb.h create mode 100644 include/asm-x86/kgdb.h create mode 100644 include/linux/kgdb.h create mode 100644 kernel/kgdb.c create mode 100644 lib/Kconfig.kgdb -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
* Christoph Hellwig [EMAIL PROTECTED] wrote: i dont think so. Which ones do you mean? I just reviewed them and they are either already done, or moot (for kgdb complications that i objected to and removed from this kgdb-x86 tree). The one I noticed quickly is the __ASSEMBLY__ removal from asm-x86/kgdb.h. [...] people might want to experiment with early debug code as well and include asm-x86/kgdb.h in assembly files. So i kept that, it's sensible. [...] I haven't looked at the serial bits because I don't think I'm qualified to comment on those, but I'm also not seeing any replies to any of his patches. Especially the comments on the arch interface seem like something that should be acted upon to me. yeah - i also noticed that the serial subsystem is marked orphaned in the MAINTAINERS file: 8250/16?50 (AND CLONE UARTS) SERIAL DRIVER L: [EMAIL PROTECTED] W: http://serial.sourceforge.net S: Orphan and compared to the raw-lowlevel-serial-driver hackery that KGDB used to do this is a big step forward. Note that it's all dependent on CONFIG_KGDB. Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
+#ifdef UART_CAP_UUE
+ if (up-capabilities UART_CAP_UUE)
+#else
+ if (up-port.type == PORT_XSCALE)
+#endif
This looks very odd. Can anyone explain what's going on here?
Especially as UART_CAP_UUE is defined in drivers/serial/8250.h
unconditionally.
diff --git a/drivers/serial/kgdboc.c b/drivers/serial/kgdboc.c
new file mode 100644
index 000..5079d32
--- /dev/null
+++ b/drivers/serial/kgdboc.c
@@ -0,0 +1,164 @@
+/*
+ * drivers/serial/kgdboc.c
Didn't you say there was no file left with these?
diff --git a/include/asm-generic/kgdb.h b/include/asm-generic/kgdb.h
new file mode 100644
index 000..115972e
--- /dev/null
+++ b/include/asm-generic/kgdb.h
Didn't you agree to kill this file in one of the last mails?
+#ifndef __ASSEMBLY__
+static inline void arch_kgdb_breakpoint(void)
+{
+ asm( int $3);
+}
+# define BREAK_INSTR_SIZE1
+# define CACHE_FLUSH_IS_SAFE 1
+#endif
this ifdef should go away.
+#endif /* _ASM_KGDB_H_ */
+#endif /* __KERNEL__ */
and the __KERNEL__ aswell as these files are in no way exported
to userspace.
+/*
+ * kgdb_get_shadow_thread - Get the shadowed task_struct of @threadid.
+ * @regs: The struct pt_regs of the current thread.
+ * @threadid: The thread id of the shadowed process to get information on.
+ *
+ * RETURN:
+ * This returns a pointer to the struct task_struct of the shadowed
+ * thread, @threadid.
+ */
+extern struct task_struct *kgdb_get_shadow_thread(struct pt_regs *regs,
+ int threadid);
So we have kerneldoc comments in both places now? Didn't you say
you converted these to something else?
+++ b/kernel/kgdb.c
@@ -0,0 +1,2019 @@
+/*
+ * kernel/kgdb.c
Another one.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
On Sun, Feb 10, 2008 at 10:27:28AM +0100, Ingo Molnar wrote: The one I noticed quickly is the __ASSEMBLY__ removal from asm-x86/kgdb.h. [...] people might want to experiment with early debug code as well and include asm-x86/kgdb.h in assembly files. So i kept that, it's sensible. But asm-x86/kgdb.h doesn't have anythign people invoking kgdb might need, it's just the arch interface to the kgdb core. Nor does it compile even with the ifdef as it already contains a C enum. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
* Ingo Molnar [EMAIL PROTECTED] wrote: tip 2e3ebf25b0bd8646e517806073e1991be1fec8a2. Shortlog, diffstat and the full patch can be found below, with all relevant review feedback addressed. Builds, boots and works fine on x86. here's gdb test-output from this 2e3ebf25b0bd kernel: i should also mention that yesterday's tree passed 200 randconfig bootup tests on 32-bit and 64-bit x86. (i excluded CONFIG_WMI from ACPI, plus 3 other ACPI commits because they keept crashing boxes or broke the build) Today's kgdb updates are in the trivial category so i'd not expect them to break anything, but nevertheless, out of caution i threw the latest tree into the qa mix as well and they already passed 10 randconfig bootup tests. [ and this matches my experience with KGDB stability in the last few months while we carried and tested it in x86.git: even the old, much wider-scope and uglier/riskier patches that hooked in a lot of places never broke anything unrelated (or anything in fact) - and this matches kgdb's -mm track record as well. ] Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
On Sun, Feb 10, 2008 at 08:13:31AM +0100, Ingo Molnar wrote:
+ } else {
+ while (count-- 0) {
+ unsigned char ch;
+
+ if (probe_kernel_address(mem, ch)) {
+ kgdb_may_fault = 0;
+ return ERR_PTR(-EINVAL);
+ }
+ mem++;
+ *buf++ = hexchars[ch 4];
+ *buf++ = hexchars[ch 0xf];
use pack_hex_byte?
+/*
+ * While we find nice hex chars, build a long_val.
+ * Return number of chars processed.
+ */
+int kgdb_hex2long(char **ptr, long *long_val)
+{
+ int hex_val;
+ int num = 0;
+
+ *long_val = 0;
+
+ while (**ptr) {
+ hex_val = hex(**ptr);
+ if (hex_val = 0) {
+ *long_val = (*long_val 4) | hex_val;
+ num++;
+ } else
+ break;
+
+ (*ptr)++;
+ }
if (hex_val 0)
break;
*long_val = (*long_val 4) | hex_val;
num++;
(*ptr)++;
+/*
+ * SW breakpoint management:
+ */
+static int kgdb_activate_sw_breakpoints(void)
+{
+ unsigned long addr;
+ int error = 0;
+ int i;
+
+ for (i = 0; i KGDB_MAX_BREAKPOINTS; i++) {
+ if (kgdb_break[i].state != BP_SET)
+ continue;
+
+ addr = kgdb_break[i].bpt_addr;
+ error = kgdb_arch_set_breakpoint(addr,
+ kgdb_break[i].saved_instr);
+ if (error)
+ return error;
+
+ if (CACHE_FLUSH_IS_SAFE) {
+ if (current-mm addr TASK_SIZE) {
+ flush_cache_range(current-mm-mmap_cache,
+ addr, addr + BREAK_INSTR_SIZE);
+ } else {
+ flush_icache_range(addr, addr +
+ BREAK_INSTR_SIZE);
+ }
+ }
unneeded braces (here and in many other places)
+/* Handle the '?' status packets */
+static void gdb_cmd_status(struct kgdb_state *ks)
+{
+ /*
+ * We know that this packet is only sent
+ * during initial connect. So to be safe,
+ * we clear out our breakpoints now in case
+ * GDB is reconnecting.
+ */
+ remove_all_break();
+
+ /*
+ * Also, if we haven't been able to roundup all
+ * CPUs, send an 'O' packet informing the user
+ * as much. Only need to do this once.
+ */
+ if (!ks-all_cpus_synced)
+ kgdb_msg_write(Not all CPUs have been synced for KGDB\n, 39);
+
+ remcom_out_buffer[0] = 'S';
+ remcom_out_buffer[1] = hexchars[ks-signo 4];
+ remcom_out_buffer[2] = hexchars[ks-signo % 16];
use pack_hex_byte or 0xf
+ if (ks-threadid pid_max) {
+ kgdb_mem2hex(getthread(ks-linux_regs,
+ ks-threadid)-comm,
+ remcom_out_buffer, 16);
+ } else {
+ if (ks-threadid = pid_max + num_online_cpus()) {
+ kgdb_shadowinfo(ks-linux_regs,
+ remcom_out_buffer,
+ ks-threadid - pid_max -
+ num_online_cpus());
+ } else {
+ static char tmpstr[23 + BUF_THREAD_ID_SIZE];
+ sprintf(tmpstr, Shadow task %d for pid 0,
+ (int)(ks-threadid - pid_max));
+ kgdb_mem2hex(tmpstr, remcom_out_buffer,
+ strlen(tmpstr));
+ }
+ }
if ()
else if ()
else
will look better
+ if (*(ptr++) != ',') {
+ error_packet(remcom_out_buffer, -EINVAL);
+ return;
+ } else {
no else needed
+ if (kgdb_hex2long(ptr, addr)) {
+ if (*(ptr++) != ',' ||
+ !kgdb_hex2long(ptr, length)) {
+ error_packet(remcom_out_buffer, -EINVAL);
+ return;
+ }
+ } else {
+ error_packet(remcom_out_buffer, -EINVAL);
+ return;
+ }
+ }
if (!kgdb_hex2long()) {
error_packet();
return;
}
if (*(ptr++) (...))
(...)
Marcin
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
Marcin Slusarz wrote:
On Sun, Feb 10, 2008 at 08:13:31AM +0100, Ingo Molnar wrote:
+} else {
+while (count-- 0) {
+unsigned char ch;
+
+if (probe_kernel_address(mem, ch)) {
+kgdb_may_fault = 0;
+return ERR_PTR(-EINVAL);
+}
+mem++;
+*buf++ = hexchars[ch 4];
+*buf++ = hexchars[ch 0xf];
use pack_hex_byte?
Good point! kgdb introduces this helper but don't use it consequently!
+/*
+ * While we find nice hex chars, build a long_val.
+ * Return number of chars processed.
+ */
+int kgdb_hex2long(char **ptr, long *long_val)
+{
+int hex_val;
+int num = 0;
+
+*long_val = 0;
+
+while (**ptr) {
+hex_val = hex(**ptr);
+if (hex_val = 0) {
+*long_val = (*long_val 4) | hex_val;
+num++;
+} else
+break;
+
+(*ptr)++;
+}
if (hex_val 0)
break;
*long_val = (*long_val 4) | hex_val;
num++;
(*ptr)++;
Jep, will include this in the cleanup patch I'm currently baking.
Jan
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
On 10/02/2008, Marcin Slusarz [EMAIL PROTECTED] wrote:
On Sun, Feb 10, 2008 at 08:13:31AM +0100, Ingo Molnar wrote:
...
+
+ if (CACHE_FLUSH_IS_SAFE) {
+ if (current-mm addr TASK_SIZE) {
+ flush_cache_range(current-mm-mmap_cache,
+ addr, addr +
BREAK_INSTR_SIZE);
+ } else {
+ flush_icache_range(addr, addr +
+ BREAK_INSTR_SIZE);
+ }
+ }
unneeded braces (here and in many other places)
While they are not strictly needed, I for one would argue they should
probably stay.
if (foo)
bar();
is not always safe in case bar() is a macro.
if (foo) {
bar();
}
is always safe and is more robust when the code gets changed later
since you don't accidentally end up with someone mistakenly turning it
into
if (foo)
bar();
baz();
--
Jesper Juhl [EMAIL PROTECTED]
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
On Sun, Feb 10, 2008 at 02:19:06PM +0100, Jesper Juhl wrote:
On 10/02/2008, Marcin Slusarz [EMAIL PROTECTED] wrote:
On Sun, Feb 10, 2008 at 08:13:31AM +0100, Ingo Molnar wrote:
...
+
+ if (CACHE_FLUSH_IS_SAFE) {
+ if (current-mm addr TASK_SIZE) {
+ flush_cache_range(current-mm-mmap_cache,
+ addr, addr +
BREAK_INSTR_SIZE);
+ } else {
+ flush_icache_range(addr, addr +
+ BREAK_INSTR_SIZE);
+ }
+ }
unneeded braces (here and in many other places)
While they are not strictly needed, I for one would argue they should
probably stay.
if (foo)
bar();
is not always safe in case bar() is a macro.
then fix this broken macro and leave calling code alone
is always safe and is more robust when the code gets changed later
since you don't accidentally end up with someone mistakenly turning it
into
if (foo)
bar();
baz();
following coding style and reading code before submission will
catch this kind of bugs
Marcin
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
* Marcin Slusarz [EMAIL PROTECTED] wrote:
+ if (CACHE_FLUSH_IS_SAFE) {
+ if (current-mm addr TASK_SIZE) {
+ flush_cache_range(current-mm-mmap_cache,
+ addr, addr + BREAK_INSTR_SIZE);
+ } else {
+ flush_icache_range(addr, addr +
+ BREAK_INSTR_SIZE);
+ }
+ }
unneeded braces (here and in many other places)
this is a small detail, but you are wrong. These braces around
multi-line statements are unneded _for the compiler_, but are very much
wanted by humans. You'll see akpm, me and others reject/fix patches on a
routine basis that make this cleanliness mistake. Please watch out for
this when writing patches ;-)
if ()
else if ()
else
will look better
nope. I consciously avoid that construct because it's dangerous: it can
quite easily result in the wrong logic. Having _more_ braces than needed
by the compiler is a style error in only a single, special case.
+ if (*(ptr++) != ',') {
+ error_packet(remcom_out_buffer, -EINVAL);
+ return;
+ } else {
no else needed
agreed - fixed.
if (!kgdb_hex2long()) {
error_packet();
return;
}
fixed.
Ingo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
* Marcin Slusarz [EMAIL PROTECTED] wrote: if (hex_val 0) break; *long_val = (*long_val 4) | hex_val; num++; (*ptr)++; agreed, fixed. + remcom_out_buffer[0] = 'S'; + remcom_out_buffer[1] = hexchars[ks-signo 4]; + remcom_out_buffer[2] = hexchars[ks-signo % 16]; use pack_hex_byte or 0xf fixed. Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
* Christoph Hellwig [EMAIL PROTECTED] wrote: On Sun, Feb 10, 2008 at 10:27:28AM +0100, Ingo Molnar wrote: The one I noticed quickly is the __ASSEMBLY__ removal from asm-x86/kgdb.h. [...] people might want to experiment with early debug code as well and include asm-x86/kgdb.h in assembly files. So i kept that, it's sensible. But asm-x86/kgdb.h doesn't have anythign people invoking kgdb might need, it's just the arch interface to the kgdb core. Nor does it compile even with the ifdef as it already contains a C enum. good point - i fixed this. (by following your suggestion of removing the _ASSEMBLY_) Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
On Sunday 10 February 2008, Ingo Molnar wrote:
* Marcin Slusarz [EMAIL PROTECTED] wrote:
+ if (CACHE_FLUSH_IS_SAFE) {
+ if (current-mm addr TASK_SIZE) {
+ flush_cache_range(current-mm-mmap_cache,
+ addr, addr + BREAK_INSTR_SIZE);
+ } else {
+ flush_icache_range(addr, addr +
+ BREAK_INSTR_SIZE);
+ }
+ }
unneeded braces (here and in many other places)
this is a small detail, but you are wrong. These braces around
multi-line statements are unneded _for the compiler_, but are very much
wanted by humans. You'll see akpm, me and others reject/fix patches on a
routine basis that make this cleanliness mistake. Please watch out for
this when writing patches ;-)
if ()
else if ()
else
will look better
nope. I consciously avoid that construct because it's dangerous: it can
quite easily result in the wrong logic. Having _more_ braces than needed
by the compiler is a style error in only a single, special case.
however it can be still made to:
if () {
if ()
else
}
[ not fixed in v6 ]
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
On Sun, Feb 10, 2008 at 08:43:52AM +0100, Ingo Molnar wrote: > > * Christoph Hellwig <[EMAIL PROTECTED]> wrote: > > > This still doesn't address a lot of the review comments from Jason's > > last posting. > > sorry, which mails are those? It's all in the thread starting with '[PATCH 0/8] kgdb 2.6.25 version', msgid [EMAIL PROTECTED] or at http://lkml.org/lkml/2008/2/9/104 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
* Christoph Hellwig <[EMAIL PROTECTED]> wrote: > This still doesn't address a lot of the review comments from Jason's > last posting. sorry, which mails are those? Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
This still doesn't address a lot of the review comments from Jason's last posting. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
On Sun, Feb 10, 2008 at 08:13:31AM +0100, Ingo Molnar wrote: > From: Jason Wessel <[EMAIL PROTECTED]> > > kgdb core code. Handles the protocol and the arch details. > > [ [EMAIL PROTECTED]: heavily modified, simplified and cleaned up. ] Hi Ingo. I see that only a very few of my comments posted yesterday got addressed. On purpose or did you miss them? Sam -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
On Sun, Feb 10, 2008 at 08:13:31AM +0100, Ingo Molnar wrote: From: Jason Wessel [EMAIL PROTECTED] kgdb core code. Handles the protocol and the arch details. [ [EMAIL PROTECTED]: heavily modified, simplified and cleaned up. ] Hi Ingo. I see that only a very few of my comments posted yesterday got addressed. On purpose or did you miss them? Sam -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
On Sun, Feb 10, 2008 at 08:43:52AM +0100, Ingo Molnar wrote: * Christoph Hellwig [EMAIL PROTECTED] wrote: This still doesn't address a lot of the review comments from Jason's last posting. sorry, which mails are those? It's all in the thread starting with '[PATCH 0/8] kgdb 2.6.25 version', msgid [EMAIL PROTECTED] or at http://lkml.org/lkml/2008/2/9/104 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [3/6] kgdb: core
* Christoph Hellwig [EMAIL PROTECTED] wrote: This still doesn't address a lot of the review comments from Jason's last posting. sorry, which mails are those? Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
