On Sat, Feb 09, 2008 at 07:35:09AM -0600, [EMAIL PROTECTED] wrote:
> From: Jason Wessel <[EMAIL PROTECTED]>
> 
> 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.

--
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/

Reply via email to