Messages in this thread | | | Date | Sat, 9 Feb 2008 12:15:03 -0500 | From | Christoph Hellwig <> | Subject | Re: [PATCH 3/8] kgdb, modules: Always allow module sect info for KGDB |
| |
On Sat, Feb 09, 2008 at 07:35:09AM -0600, jason.wessel@windriver.com wrote: > From: Jason Wessel <jason.wessel@windriver.com> > > With more information in the kernel, gdb can be modified in such a way > as to automatically read the kernel module section information and > allow for easy kernel module debugging. > > In gdb the solib-search-path must contain the location of any module > to be debugged. When a module is loaded GDB acts like a shared > library has been loaded and will collect the information about the > memory location so the kernel module can be debugged or inspected. > > Even when not using kgdb+gdb, it is quite useful for a > debugger+ICE/jtag to have the module section information.
This patches doesn't match the description and seems quite odd in various ways:
> --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -342,13 +342,16 @@ struct module > unsigned long num_symtab; > char *strtab; > > - /* Section attributes */ > - struct module_sect_attrs *sect_attrs; > - > /* Notes attributes */ > struct module_notes_attrs *notes_attrs; > #endif > > +#if defined(CONFIG_KALLSYMS) || defined(CONFIG_KGDB) > + /* Section attributes */ > + struct module_sect_attrs *sect_attrs; > +#endif
So here we make a member of struct module conditional. Fine if that saves memory for non-debug configs, but that's not what the description mentions. Also is there any reason why you'd want to build KGDB without KALLSYMS?
> diff --git a/kernel/module.c b/kernel/module.c > index 4202da9..4a94d1a 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -602,6 +602,9 @@ static void module_unload_free(struct module *mod) > } > } > } > + blocking_notifier_call_chain(&module_notify_list, > + MODULE_STATE_GOING, > + mod);
this adds a notifier for a module beeing unloaded. This could vaguely be related to what's in the description at least..
> +#if defined(CONFIG_KALLSYMS) || defined(CONFIG_KGDB) > #ifdef CONFIG_KALLSYMS > static ssize_t module_sect_show(struct module_attribute *mattr, > struct module *mod, char *buf) > @@ -1000,6 +1004,7 @@ static ssize_t module_sect_show(struct module_attribute *mattr, > container_of(mattr, struct module_sect_attr, mattr); > return sprintf(buf, "0x%lx\n", sattr->address); > } > +#endif /* CONFIG_KALLSYMS */
Now you add a CONFIG_KALLSYMS || KGDB ifdef around an CONFIG_KALLSYMS block that you shorten at the same time. If you really want to allow kgdb without kallsyms the KALLSYMS should be left for this function and the KALLSYMS || KGDB move below it. But with this ifdef mess I can only repeat that you should simply make kallsysm mandatory for kgdb.
| |