OProfile cannot be loaded as module...

OProfile cannot be loaded as module...

[David Daney]

arch/mips/oprofile/common.c defines several symbols (op_model_mipsxx and 
op_model_rm9000) with __attribute__((weak)).  It then assumes that ELF 
linking conventions will prevail and there will be no problems if they 
are undefined.

The problem is if you try to load oprofile as a module.  The kernel 
module linker evidentially does not understand weak symbols and refuses 
to load the module because they are undefined.

Perhaps a single

extern struct op_mips_model plat_op_model;

That must be defined by each different implementation.  Deciding one 
which implementation would then be done at compile time instead of runtime.

I don't have a patch for this yet, but that is what I am thinking of doing.

David Daney.

Re: OProfile cannot be loaded as module...

[David Daney]

[-- Attachment #1: Type: text/plain, Size: 855 bytes --]

David Daney wrote:
> arch/mips/oprofile/common.c defines several symbols (op_model_mipsxx and 
> op_model_rm9000) with __attribute__((weak)).  It then assumes that ELF 
> linking conventions will prevail and there will be no problems if they 
> are undefined.
> 
> The problem is if you try to load oprofile as a module.  The kernel 
> module linker evidentially does not understand weak symbols and refuses 
> to load the module because they are undefined.
> 
> Perhaps a single
> 
> extern struct op_mips_model plat_op_model;
> 
> That must be defined by each different implementation.  Deciding one 
> which implementation would then be done at compile time instead of runtime.
> 
> I don't have a patch for this yet, but that is what I am thinking of doing.
> 

Ok, attached is my untested (but it compiles for some architectures) patch.

David Daney

[-- Attachment #2: op.d --]
[-- Type: text/plain, Size: 3889 bytes --]

diff --git a/arch/mips/oprofile/common.c b/arch/mips/oprofile/common.c
--- a/arch/mips/oprofile/common.c
+++ b/arch/mips/oprofile/common.c
@@ -14,20 +14,17 @@
 
 #include "op_impl.h"
 
-extern struct op_mips_model op_model_mipsxx __attribute__((weak));
-extern struct op_mips_model op_model_rm9000 __attribute__((weak));
-
-static struct op_mips_model *model;
+extern struct op_mips_model *plat_op_model;
 
 static struct op_counter_config ctr[20];
 
 static int op_mips_setup(void)
 {
 	/* Pre-compute the values to stuff in the hardware registers.  */
-	model->reg_setup(ctr);
+	plat_op_model->reg_setup(ctr);
 
 	/* Configure the registers on all cpus.  */
-	on_each_cpu(model->cpu_setup, 0, 0, 1);
+	on_each_cpu(plat_op_model->cpu_setup, 0, 0, 1);
 
         return 0;
 }
@@ -36,7 +33,7 @@ static int op_mips_create_files(struct s
 {
 	int i;
 
-	for (i = 0; i < model->num_counters; ++i) {
+	for (i = 0; i < plat_op_model->num_counters; ++i) {
 		struct dentry *dir;
 		char buf[3];
 
@@ -58,7 +55,7 @@ static int op_mips_create_files(struct s
 
 static int op_mips_start(void)
 {
-	on_each_cpu(model->cpu_start, NULL, 0, 1);
+	on_each_cpu(plat_op_model->cpu_start, NULL, 0, 1);
 
 	return 0;
 }
@@ -66,49 +63,33 @@ static int op_mips_start(void)
 static void op_mips_stop(void)
 {
 	/* Disable performance monitoring for all counters.  */
-	on_each_cpu(model->cpu_stop, NULL, 0, 1);
+	on_each_cpu(plat_op_model->cpu_stop, NULL, 0, 1);
 }
 
 int __init oprofile_arch_init(struct oprofile_operations *ops)
 {
-	struct op_mips_model *lmodel = NULL;
 	int res;
 
 	printk(KERN_INFO "oprofile: In oprofile_arch_init.\n");
 
-	switch (current_cpu_data.cputype) {
-	case CPU_24K:
-		lmodel = &op_model_mipsxx;
-		break;
-
-	case CPU_RM9000:
-		lmodel = &op_model_rm9000;
-		break;
-	};
-
-	if (!lmodel)
-		return -ENODEV;
-
-	res = lmodel->init();
+	res = plat_op_model->init();
 	if (res)
 		return res;
 
-	model = lmodel;
-
 	ops->create_files	= op_mips_create_files;
 	ops->setup		= op_mips_setup;
 	//ops->shutdown         = op_mips_shutdown;
 	ops->start		= op_mips_start;
 	ops->stop		= op_mips_stop;
-	ops->cpu_type		= lmodel->cpu_type;
+	ops->cpu_type		= plat_op_model->cpu_type;
 
 	printk(KERN_INFO "oprofile: using %s performance monitoring.\n",
-	       lmodel->cpu_type);
+	       plat_op_model->cpu_type);
 
 	return 0;
 }
 
 void oprofile_arch_exit(void)
 {
-	model->exit();
+	plat_op_model->exit();
 }
diff --git a/arch/mips/oprofile/op_model_mipsxx.c b/arch/mips/oprofile/op_model_mipsxx.c
--- a/arch/mips/oprofile/op_model_mipsxx.c
+++ b/arch/mips/oprofile/op_model_mipsxx.c
@@ -23,7 +23,7 @@
 
 #define M_COUNTER_OVERFLOW		(1UL    << 31)
 
-struct op_mips_model op_model_mipsxx;
+static struct op_mips_model op_model_mipsxx;
 
 static struct mipsxx_register_config {
 	unsigned int control[4];
@@ -205,7 +205,7 @@ static void mipsxx_exit(void)
 	perf_irq = null_perf_irq;
 }
 
-struct op_mips_model op_model_mipsxx = {
+static struct op_mips_model op_model_mipsxx = {
 	.reg_setup	= mipsxx_reg_setup,
 	.cpu_setup	= mipsxx_cpu_setup,
 	.init		= mipsxx_init,
@@ -213,3 +213,5 @@ struct op_mips_model op_model_mipsxx = {
 	.cpu_start	= mipsxx_cpu_start,
 	.cpu_stop	= mipsxx_cpu_stop,
 };
+
+struct op_mips_model *plat_op_model = &op_model_mipsxx;
diff --git a/arch/mips/oprofile/op_model_rm9000.c b/arch/mips/oprofile/op_model_rm9000.c
--- a/arch/mips/oprofile/op_model_rm9000.c
+++ b/arch/mips/oprofile/op_model_rm9000.c
@@ -126,7 +126,7 @@ static void rm9000_exit(void)
 	free_irq(rm9000_perfcount_irq, NULL);
 }
 
-struct op_mips_model op_model_rm9000 = {
+static struct op_mips_model op_model_rm9000 = {
 	.reg_setup	= rm9000_reg_setup,
 	.cpu_setup	= rm9000_cpu_setup,
 	.init		= rm9000_init,
@@ -136,3 +136,5 @@ struct op_mips_model op_model_rm9000 = {
 	.cpu_type	= "mips/rm9000",
 	.num_counters	= 2
 };
+
+struct op_mips_model *plat_op_model = &op_model_rm9000;

Re: OProfile cannot be loaded as module...

[Ralf Baechle]

On Fri, Oct 07, 2005 at 04:59:11PM -0700, David Daney wrote:

> arch/mips/oprofile/common.c defines several symbols (op_model_mipsxx and 
> op_model_rm9000) with __attribute__((weak)).  It then assumes that ELF 
> linking conventions will prevail and there will be no problems if they 
> are undefined.
> 
> The problem is if you try to load oprofile as a module.  The kernel 
> module linker evidentially does not understand weak symbols and refuses 
> to load the module because they are undefined.

Actually it contains code to handle weak symbols so this is a bit
surprising not last because STB_WEAK handling happen in the generic
module loader code and is being used by other architectures as well.

So if there's a problem with the module loader I'd prefer to solve that
instead of starting to kludge around it.

What compiler exactly are you using btw?

  Ralf

Re: OProfile cannot be loaded as module...

[David Daney]

Ralf Baechle wrote:
> On Fri, Oct 07, 2005 at 04:59:11PM -0700, David Daney wrote:
> 
> 
>>arch/mips/oprofile/common.c defines several symbols (op_model_mipsxx and 
>>op_model_rm9000) with __attribute__((weak)).  It then assumes that ELF 
>>linking conventions will prevail and there will be no problems if they 
>>are undefined.
>>
>>The problem is if you try to load oprofile as a module.  The kernel 
>>module linker evidentially does not understand weak symbols and refuses 
>>to load the module because they are undefined.
> 
> 
> Actually it contains code to handle weak symbols so this is a bit
> surprising not last because STB_WEAK handling happen in the generic
> module loader code and is being used by other architectures as well.
> 
> So if there's a problem with the module loader I'd prefer to solve that
> instead of starting to kludge around it.
> 

Fine, but what exactly are the semantics of __attribute__((weak)) in 
modules?  It gets resolved when linking with other objects that make up 
the module.  But what if the weak symbol can be resolved at module load 
time against symbols in either the kernel proper or other modules?  What 
happens if the weak symbol can be resolved by a symbol in a module that 
is loaded after the one with the weak symbol?  Does it get fixed up when 
the new module is loaded?


> What compiler exactly are you using btw?
> 

GCC-3.4.3 / binutils 2.16.91 20050817

David Daney.

Re: OProfile cannot be loaded as module...

[Ralf Baechle]

On Mon, Oct 17, 2005 at 01:14:01PM -0700, David Daney wrote:

> >>arch/mips/oprofile/common.c defines several symbols (op_model_mipsxx and 
> >>op_model_rm9000) with __attribute__((weak)).  It then assumes that ELF 
> >>linking conventions will prevail and there will be no problems if they 
> >>are undefined.
> >>
> >>The problem is if you try to load oprofile as a module.  The kernel 
> >>module linker evidentially does not understand weak symbols and refuses 
> >>to load the module because they are undefined.
> >
> >
> >Actually it contains code to handle weak symbols so this is a bit
> >surprising not last because STB_WEAK handling happen in the generic
> >module loader code and is being used by other architectures as well.
> >
> >So if there's a problem with the module loader I'd prefer to solve that
> >instead of starting to kludge around it.
> >
> 
> Fine, but what exactly are the semantics of __attribute__((weak)) in 
> modules?  It gets resolved when linking with other objects that make up 
> the module.  But what if the weak symbol can be resolved at module load 
> time against symbols in either the kernel proper or other modules?

Yes.

> What happens if the weak symbol can be resolved by a symbol in a module
> that is loaded after the one with the weak symbol?  Does it get fixed up
> when the new module is loaded?

No, it won't - and I don't think that would be a good idea.  The potencial
for bugs is just too large.

  Ralf

Re: OProfile cannot be loaded as module...

[David Daney]

Ralf Baechle wrote:
> On Mon, Oct 17, 2005 at 01:14:01PM -0700, David Daney wrote:

>>Fine, but what exactly are the semantics of __attribute__((weak)) in 
>>modules?  It gets resolved when linking with other objects that make up 
>>the module.  But what if the weak symbol can be resolved at module load 
>>time against symbols in either the kernel proper or other modules?
> 
> 
> Yes.
> 
> 
>>What happens if the weak symbol can be resolved by a symbol in a module
>>that is loaded after the one with the weak symbol?  Does it get fixed up
>>when the new module is loaded?
> 
> 
> No, it won't - and I don't think that would be a good idea.  The potencial
> for bugs is just too large.
> 

Given your 'yes' and 'no' answers, the behavior of a module could depend 
on the order in which the modules are loaded, as they can be linked 
differently depending on which modules are already present.

That doesn't seem like a good way of doing things.

If if were up to me (and I know that it is not), I would disallow 
linking of weak symbols at module load time altogether.

David Daney.

Re: OProfile cannot be loaded as module...

[Ralf Baechle]

On Tue, Oct 18, 2005 at 08:46:20AM -0700, David Daney wrote:

> Given your 'yes' and 'no' answers, the behavior of a module could depend 
> on the order in which the modules are loaded, as they can be linked 
> differently depending on which modules are already present.
> 
> That doesn't seem like a good way of doing things.
> 
> If if were up to me (and I know that it is not), I would disallow 
> linking of weak symbols at module load time altogether.

The semantics were choosen by Rusty who maintains the generic part of the
module loader.  Ensuring the right load order is the job of depmod
and modprobe.

  Ralf

Re: OProfile cannot be loaded as module...

[Atsushi Nemoto]

On Thu, 13 Oct 2005 23:55:21 +0100, Ralf Baechle <ralf@linux-mips.org> wrote:
> > The problem is if you try to load oprofile as a module.  The kernel 
> > module linker evidentially does not understand weak symbols and refuses 
> > to load the module because they are undefined.
> 
> Actually it contains code to handle weak symbols so this is a bit
> surprising not last because STB_WEAK handling happen in the generic
> module loader code and is being used by other architectures as well.

This is still unresolved.  The "oprofile: Unknown symbol" message is
printed in arch/mips/kernel/module.c file.  How about this patch?


[PATCH] Ignore unresolved weak symbols in module.

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

diff --git a/arch/mips/kernel/module.c b/arch/mips/kernel/module.c
index e54a7f4..d7bf021 100644
--- a/arch/mips/kernel/module.c
+++ b/arch/mips/kernel/module.c
@@ -288,6 +288,9 @@ int apply_relocate(Elf_Shdr *sechdrs, co
 		sym = (Elf_Sym *)sechdrs[symindex].sh_addr
 			+ ELF_MIPS_R_SYM(rel[i]);
 		if (!sym->st_value) {
+			/* Ignore unresolved weak symbol */
+			if (ELF_ST_BIND(sym->st_info) == STB_WEAK)
+				continue;
 			printk(KERN_WARNING "%s: Unknown symbol %s\n",
 			       me->name, strtab + sym->st_name);
 			return -ENOENT;
@@ -325,6 +328,9 @@ int apply_relocate_add(Elf_Shdr *sechdrs
 		sym = (Elf_Sym *)sechdrs[symindex].sh_addr
 			+ ELF_MIPS_R_SYM(rel[i]);
 		if (!sym->st_value) {
+			/* Ignore unresolved weak symbol */
+			if (ELF_ST_BIND(sym->st_info) == STB_WEAK)
+				continue;
 			printk(KERN_WARNING "%s: Unknown symbol %s\n",
 			       me->name, strtab + sym->st_name);
 			return -ENOENT;