Re: [PATCH 3/8] kgdb, modules: Always allow module sect info forKGDB

[Christoph Hellwig]

On Sat, Feb 09, 2008 at 07:35:09AM -0600, jason.wessel@xxxxxxxxxxxxx wrote:
> From: Jason Wessel <jason.wessel@xxxxxxxxxxxxx>
> 
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/