LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Christoph Hellwig <hch@infradead.org>
To: jason.wessel@windriver.com
Cc: linux-kernel@vger.kernel.org, Ingo Molnar <mingo@elte.hu>,
	Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH 3/8] kgdb, modules: Always allow module sect info for KGDB
Date: Sat, 9 Feb 2008 12:15:03 -0500	[thread overview]
Message-ID: <20080209171503.GB15568@infradead.org> (raw)
In-Reply-To: <1202564114-18587-4-git-send-email-jason.wessel@windriver.com>

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.


  parent reply	other threads:[~2008-02-09 17:15 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-09 13:35 [PATCH 0/8] kgdb 2.6.25 version jason.wessel
2008-02-09 13:35 ` [PATCH 1/8] kgdb: core API and gdb protocol handler jason.wessel
2008-02-09 13:35   ` [PATCH 2/8] pid, kgdb: add pid_max prototype jason.wessel
2008-02-09 13:35     ` [PATCH 3/8] kgdb, modules: Always allow module sect info for KGDB jason.wessel
2008-02-09 13:35       ` [PATCH 4/8] kgdb: COPTIMIZE flag jason.wessel
2008-02-09 13:35         ` [PATCH 5/8] kgdb, x86: Add arch specfic kgdb support jason.wessel
2008-02-09 13:35           ` [PATCH 6/8] kgdb, sysrq_bugfix jason.wessel
2008-02-09 13:35             ` [PATCH][7/8] kgdb: exclusive use kgdb8250 uart I/O driver jason.wessel
2008-02-09 13:35               ` [PATCH 8/8] kgdb: kgdboc 8250 I/O module jason.wessel
2008-02-09 14:53               ` [PATCH][7/8] kgdb: exclusive use kgdb8250 uart I/O driver Jan Kiszka
2008-02-09 18:45                 ` Jason Wessel
2008-02-09 16:40               ` Jan Kiszka
2008-02-09 18:41                 ` Jason Wessel
2008-02-10 15:26               ` Pavel Machek
2008-02-09 14:33           ` [PATCH 5/8] kgdb, x86: Add arch specfic kgdb support Jan Kiszka
2008-02-09 17:16         ` [PATCH 4/8] kgdb: COPTIMIZE flag Christoph Hellwig
2008-02-09 17:15       ` Christoph Hellwig [this message]
2008-02-09 17:10     ` [PATCH 2/8] pid, kgdb: add pid_max prototype Christoph Hellwig
2008-02-09 14:27   ` [PATCH 1/8] kgdb: core API and gdb protocol handler Jan Kiszka
2008-02-09 15:29   ` Sam Ravnborg
2008-02-09 17:27   ` Christoph Hellwig
2008-02-09 19:46     ` Ray Lee
2008-02-09 21:51       ` Ray Lee
2008-02-09 17:38 ` [PATCH 0/8] kgdb 2.6.25 version Christoph Hellwig

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080209171503.GB15568@infradead.org \
    --to=hch@infradead.org \
    --cc=jason.wessel@windriver.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@elte.hu \
    --cc=tglx@linutronix.de \
    --subject='Re: [PATCH 3/8] kgdb, modules: Always allow module sect info for KGDB' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).