Skip to contentSkip to conversation

OProfile cannot be loaded as module...

DISCUSSION David Daney 3 participants 8 messages Oct 7, 2005 – Apr 21, 2006 MIPS

lore
Share & tools
lore

David Daney

author
#0 lore raw

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.

author
#1 lore raw
Quoted 4 lines · 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.

Quoted 8 lines

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

arch/mips/oprofile/common.c +10 -29
@@ -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();
 }
arch/mips/oprofile/op_model_mipsxx.c +4 -2
@@ -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;
arch/mips/oprofile/op_model_rm9000.c +3 -1
@@ -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;
#2 lore raw
In reply to David Daney
Quoted 8 lines · 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.

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

author
#3 lore raw
Quoted 8 lines · Ralf Baechle
Quoted 3 lines · 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.

Quoted 7 lines

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?

Quoted 2 lines

What compiler exactly are you using btw?

GCC-3.4.3 / binutils 2.16.91 20050817

David Daney.

#4 lore raw
Quoted 14 lines · David Daney
Quoted 14 lines
Quoted 8 lines

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.

Quoted 8 lines
Quoted 3 lines

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.

Quoted 3 lines

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

author
#5 lore raw
Quoted 8 lines · Ralf Baechle
Quoted 1 line · David Daney

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.

Quoted 8 lines
Quoted 3 lines

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.

#6 lore raw
Quoted 8 lines · David Daney

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

#7 lore raw
In reply to Ralf Baechle
Quoted 7 lines · Ralf Baechle
Quoted 3 lines

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 <[email protected]>
arch/mips/kernel/module.c +6 -0
index e54a7f4..d7bf021 100644
@@ -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;