kallsyms should prefer non weak symbols

PATCH In review Paulo Marques 5 participants 12 messages Dec 4, 2007 – Dec 5, 2007

Share & tools

Paulo Marques

author

2007-12-04 20:35 UTC #0 lore raw

PATCH 1/1 kallsyms should prefer non weak symbols

When resolving symbol names from addresses with aliased symbol names, kallsyms_lookup always returns the first symbol, even if it is a weak symbol.

This patch changes this by sorting the symbols with the weak symbols last before feeding them to the kernel. This way the kernel runtime isn't changed at all, only the kallsyms build system is changed.

Another side effect is that the symbols get sorted by address, too. So, even if future binutils version have some bug in "nm" that makes it fail to correctly sort symbols by address, the kernel won't be affected by this.

From: Paulo Marques <[email protected]>

Signed-off-by Mathieu Desnoyers <[email protected]>

Signature · 3 lines

Paulo Marques - www.grupopie.com

"There cannot be a crisis today; my schedule is already full."

scripts/kallsyms.c renamed from scripts/kallsyms.c.orig +34 -2

@@ -34,7 +34,7 @@
 
 struct sym_entry {
 	unsigned long long addr;
-	unsigned int len;
+	unsigned int len, start_pos;
 	unsigned char *sym;
 };

@@ -202,8 +202,10 @@ static void read_map(FILE *in)
 				exit (1);
 			}
 		}
-		if (read_symbol(in, &table[table_cnt]) == 0)
+		if (read_symbol(in, &table[table_cnt]) == 0) {
+			table[table_cnt].start_pos = table_cnt;
 			table_cnt++;
+		}
 	}
 }

@@ -507,6 +509,35 @@ static void optimize_token_table(void)
 }
 
 
+static int compare_symbols(const void *a, const void *b)
+{
+	struct sym_entry *sa, *sb;
+	int wa, wb;
+
+	sa = (struct sym_entry *) a;
+	sb = (struct sym_entry *) b;
+
+	// sort by address first
+	if (sa->addr > sb->addr)
+		return 1;
+	if (sa->addr < sb->addr)
+		return -1;
+
+	// sort by "weakness" type
+	wa = (sa->sym[0] == 'w') || (sa->sym[0] == 'W');
+	wb = (sb->sym[0] == 'w') || (sb->sym[0] == 'W');
+	if (wa != wb)
+		return wa - wb;
+
+	// sort by initial order, so that other symbols are left undisturbed
+	return sa->start_pos - sb->start_pos;
+}
+
+static void sort_symbols(void)
+{
+	qsort(table, table_cnt, sizeof(struct sym_entry), compare_symbols);
+}
+
 int main(int argc, char **argv)
 {
 	if (argc >= 2) {

@@ -527,6 +558,7 @@ int main(int argc, char **argv)
 		usage();
 
 	read_map(stdin);
+	sort_symbols();
 	optimize_token_table();
 	write_src();

Andrew Morton

2007-12-04 21:18 UTC #1 lore raw

Quoted 4 lines · On Tue, 04 Dec 2007 20:35:32 +0000 Paulo Marques <[email protected]> wrote:

When resolving symbol names from addresses with aliased symbol names, kallsyms_lookup always returns the first symbol, even if it is a weak symbol.

Quoted 9 lines

This patch changes this by sorting the symbols with the weak symbols last before feeding them to the kernel. This way the kernel runtime isn't changed at all, only the kallsyms build system is changed.
Another side effect is that the symbols get sorted by address, too. So, even if future binutils version have some bug in "nm" that makes it fail to correctly sort symbols by address, the kernel won't be affected by this.

I don't understand the reason for making this change.

Mathieu Desnoyers

2007-12-04 21:38 UTC #2 lore raw

Quoted 11 lines · * Andrew Morton ([email protected]) wrote:

Quoted 8 lines · On Tue, 04 Dec 2007 20:35:32 +0000 Paulo Marques <[email protected]> wrote:
When resolving symbol names from addresses with aliased symbol names, kallsyms_lookup always returns the first symbol, even if it is a weak symbol.
This patch changes this by sorting the symbols with the weak symbols last before feeding them to the kernel. This way the kernel runtime isn't changed at all, only the kallsyms build system is changed.

Quoted 8 lines

Quoted 5 lines
Another side effect is that the symbols get sorted by address, too. So, even if future binutils version have some bug in "nm" that makes it fail to correctly sort symbols by address, the kernel won't be affected by this.
I don't understand the reason for making this change.

I created a module in LTTng that uses kallsyms to get the symbol corresponding to a specific system call address. Unfortunately, all the unimplemented syscalls were all referring to the (same) weak symbol identifying an unrelated system call rather that sys_ni (or whatever non-weak symbol would be expected). Kallsyms was dumbly returning the first symbol that matched.

This patch makes sure kallsyms returns the non-weak symbol when there is one, which seems to be the expected result.

Mathieu

Signature · 3 lines

Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Mathieu Desnoyers

2007-12-04 21:19 UTC #3 lore raw

In reply to Paulo Marques

Quoted 9 lines · * Paulo Marques ([email protected]) wrote:

When resolving symbol names from addresses with aliased symbol names, kallsyms_lookup always returns the first symbol, even if it is a weak symbol.
This patch changes this by sorting the symbols with the weak symbols last before feeding them to the kernel. This way the kernel runtime isn't changed at all, only the kallsyms build system is changed.

Quoted 8 lines

Another side effect is that the symbols get sorted by address, too. So, even if future binutils version have some bug in "nm" that makes it fail to correctly sort symbols by address, the kernel won't be affected by this.
From: Paulo Marques <[email protected]>
Signed-off-by Mathieu Desnoyers <[email protected]>

Please wait for me to accept the changes before adding signed-off-by.

See comment below,

Quoted 31 lines

Signature · 3 lines

Paulo Marques - www.grupopie.com

"There cannot be a crisis today; my schedule is already full."

scripts/kallsyms.c renamed from scripts/kallsyms.c.orig +4 -2

@@ -34,7 +34,7 @@
 
 struct sym_entry {
 	unsigned long long addr;
-	unsigned int len;
+	unsigned int len, start_pos;
 	unsigned char *sym;
 };

@@ -202,8 +202,10 @@ static void read_map(FILE *in)
 				exit (1);
 			}
 		}
-		if (read_symbol(in, &table[table_cnt]) == 0)
+		if (read_symbol(in, &table[table_cnt]) == 0) {
+			table[table_cnt].start_pos = table_cnt;
 			table_cnt++;
+		}
 	}
 }

@@ -507,6 +509,35 @@ static void optimize_token_table(void)
 }

scripts/kallsyms.c static void optimize_token_table(void) · line 520

Quoted 10 lines

scripts/kallsyms.c renamed from scripts/kallsyms.c.orig +9 -0

 
+static int compare_symbols(const void *a, const void *b)
+{
+	struct sym_entry *sa, *sb;
+	int wa, wb;
+
+	sa = (struct sym_entry *) a;
+	sb = (struct sym_entry *) b;
+
+	// sort by address first

Comments should be /* */ , not //.

Please run through scripts/checkpatch.pl before submitting.

Quoted 32 lines

scripts/kallsyms.c renamed from scripts/kallsyms.c.orig +21 -0

+	if (sa->addr > sb->addr)
+		return 1;
+	if (sa->addr < sb->addr)
+		return -1;
+
+	// sort by "weakness" type
+	wa = (sa->sym[0] == 'w') || (sa->sym[0] == 'W');
+	wb = (sb->sym[0] == 'w') || (sb->sym[0] == 'W');
+	if (wa != wb)
+		return wa - wb;
+
+	// sort by initial order, so that other symbols are left undisturbed
+	return sa->start_pos - sb->start_pos;
+}
+
+static void sort_symbols(void)
+{
+	qsort(table, table_cnt, sizeof(struct sym_entry), compare_symbols);
+}
+
 int main(int argc, char **argv)
 {
 	if (argc >= 2) {

@@ -527,6 +558,7 @@ int main(int argc, char **argv)
 		usage();
 
 	read_map(stdin);
+	sort_symbols();
 	optimize_token_table();
 	write_src();

Signature · 3 lines

Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Paulo Marques

author

2007-12-05 12:07 UTC #4 lore raw

Quoted 8 lines · Mathieu Desnoyers

Quoted 5 lines · * Paulo Marques ([email protected]) wrote:
When resolving symbol names from addresses with aliased symbol names, kallsyms_lookup always returns the first symbol, even if it is a weak symbol.
[...]
From: Paulo Marques <[email protected]>
Signed-off-by Mathieu Desnoyers <[email protected]>
Please wait for me to accept the changes before adding signed-off-by.

My mistake was that I didn't realized you'd already corrected the original patch when you posted this:

http://lkml.org/lkml/2007/11/13/292

Since I thought it was the same patch, it already had your "Signed-off-by". Sorry for the confusion... :(

Signature · 3 lines

Paulo Marques - www.grupopie.com

"I haven't lost my mind -- it's backed up on tape somewhere."

Arjan van de Ven

2007-12-04 21:36 UTC #5 lore raw

In reply to Paulo Marques

Quoted 9 lines · On Tue, 04 Dec 2007 20:35:32 +0000 Paulo Marques <[email protected]> wrote:

When resolving symbol names from addresses with aliased symbol names, kallsyms_lookup always returns the first symbol, even if it is a weak symbol.
This patch changes this by sorting the symbols with the weak symbols last before feeding them to the kernel. This way the kernel runtime isn't changed at all, only the kallsyms build system is changed.

what user of this api is affected by this?

Signature · 3 lines

If you want to reach me at my work email, use [email protected]
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Mathieu Desnoyers

2007-12-04 22:31 UTC #6 lore raw

Quoted 8 lines · * Arjan van de Ven ([email protected]) wrote:

Quoted 5 lines · On Tue, 04 Dec 2007 20:35:32 +0000 Paulo Marques <[email protected]> wrote:
When resolving symbol names from addresses with aliased symbol names, kallsyms_lookup always returns the first symbol, even if it is a weak symbol.

Quoted 7 lines

Quoted 4 lines
This patch changes this by sorting the symbols with the weak symbols last before feeding them to the kernel. This way the kernel runtime isn't changed at all, only the kallsyms build system is changed.
what user of this api is affected by this?

grep -r kallsyms_lookup * does a pretty good job at it ;)

kernel/kprobes.c
arch/x86/kernel/traps_64.c
blackfin/kernel/traps.c
parisc/kernel/unwind.c
powerpc/xmon/xmon.c
sh64/kernel/unwind.c
kernel/kallsyms.c : sprint_symbol()

Then, sprint_symbol users :

kernel/lockdep_proc.c
kernel/softirq.c
mm/slub.c
fs/gfs2/glock.c
arch/x86/kernel/traps_32.c
arch/x86/kernel/traps_64.c
x86/kernel/syscall_64.c

Mathieu

Quoted 6 lines

Signature · 4 lines

-- 
If you want to reach me at my work email, use [email protected]
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Signature · 3 lines

Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Rusty Russell

2007-12-05 09:37 UTC #7 lore raw

Quoted 8 lines · On Wednesday 05 December 2007 09:31:23 Mathieu Desnoyers wrote:

Quoted 7 lines · * Arjan van de Ven ([email protected]) wrote:
On Tue, 04 Dec 2007 20:35:32 +0000
Quoted 4 lines · Paulo Marques
When resolving symbol names from addresses with aliased symbol names, kallsyms_lookup always returns the first symbol, even if it is a weak symbol.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
This patch changes this by sorting the symbols with the weak symbols last before feeding them to the kernel. This way the kernel runtime isn't changed at all, only the kallsyms build system is changed.
what user of this api is affected by this?
grep -r kallsyms_lookup * does a pretty good job at it ;)

Yeah, we discussed this before and agreed it was best to show strong symbols before weak ones.

Cheers,
Rusty.

Rusty Russell

2007-12-05 02:06 UTC #8 lore raw

In reply to Paulo Marques

Quoted 3 lines · On Wednesday 05 December 2007 07:35:32 Paulo Marques wrote:

When resolving symbol names from addresses with aliased symbol names, kallsyms_lookup always returns the first symbol, even if it is a weak symbol.

Thanks, this looks great.

Rusty.

Andrew Morton

2007-12-05 03:10 UTC #9 lore raw

In reply to Paulo Marques

Quoted 8 lines · On Tue, 04 Dec 2007 20:35:32 +0000 Paulo Marques <[email protected]> wrote:

When resolving symbol names from addresses with aliased symbol names, kallsyms_lookup always returns the first symbol, even if it is a weak symbol.
This patch changes this by sorting the symbols with the weak symbols last before feeding them to the kernel. This way the kernel runtime isn't changed at all, only the kallsyms build system is changed.

Quoted 7 lines

Another side effect is that the symbols get sorted by address, too. So, even if future binutils version have some bug in "nm" that makes it fail to correctly sort symbols by address, the kernel won't be affected by this.
From: Paulo Marques <[email protected]>
Signed-off-by Mathieu Desnoyers <[email protected]>

The signoffs don't make sense.

If you authored the patch then please put a From: at the top of the changelog and include your signed-off-by:.

If Mathieu worked on or forwarded the patch then include his s-o-b. If he wasn't in the delivery path but has tested/reviewed/approved the patch then Acked-by: is more appropriate. Documentation/SubmittingPatches has details...

Andrew Morton

2007-12-05 03:15 UTC #10 lore raw

In reply to Paulo Marques

Quoted 11 lines · On Tue, 04 Dec 2007 20:35:32 +0000 Paulo Marques <[email protected]> wrote:

When resolving symbol names from addresses with aliased symbol names, kallsyms_lookup always returns the first symbol, even if it is a weak symbol.
This patch changes this by sorting the symbols with the weak symbols last before feeding them to the kernel. This way the kernel runtime isn't changed at all, only the kallsyms build system is changed.
Another side effect is that the symbols get sorted by address, too. So, even if future binutils version have some bug in "nm" that makes it fail to correctly sort symbols by address, the kernel won't be affected by this.

Incremental patch:

From: Andrew Morton <[email protected]>

Fix checkpatch warnings:

ERROR: do not use C99 // comments
#72: FILE: scripts/kallsyms.c:516:
+ // sort by address first

ERROR: do not use C99 // comments
#78: FILE: scripts/kallsyms.c:522:
+ // sort by "weakness" type

ERROR: do not use C99 // comments
#84: FILE: scripts/kallsyms.c:528:
+ // sort by initial order, so that other symbols are left undisturbed

total: 3 errors, 0 warnings, 61 lines checked

Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.

Please run checkpatch prior to sending patches

And a few coding-style things which checkpatch missed.

And fix up constificiation to remove typecasting.

Cc Mathieu Desnoyers <[email protected]>

Cc Paulo Marques <[email protected]>

Cc Sam Ravnborg <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 scripts/kallsyms.c |   19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

scripts/kallsyms.c renamed from scripts/kallsyms.c~kallsyms-should-prefer-non-weak-symbols-checkpatch-fixes +9 ~~-10~~

diff -puN scripts/kallsyms.c~kallsyms-should-prefer-non-weak-symbols-checkpatch-fixes scripts/kallsyms.c
@@ -31,14 +31,13 @@
 
 #define KSYM_NAME_LEN		128
 
-
 struct sym_entry {
 	unsigned long long addr;
-	unsigned int len, start_pos;
+	unsigned int len;
+	unsigned int start_pos;
 	unsigned char *sym;
 };
 
-
 static struct sym_entry *table;
 static unsigned int table_size, table_cnt;
 static unsigned long long _text, _stext, _etext, _sinittext, _einittext;

@@ -504,28 +503,28 @@ static void optimize_token_table(void)
 	optimize_result();
 }
 
-
 static int compare_symbols(const void *a, const void *b)
 {
-	struct sym_entry *sa, *sb;
+	const struct sym_entry *sa;
+	const struct sym_entry *sb;
 	int wa, wb;
 
-	sa = (struct sym_entry *) a;
-	sb = (struct sym_entry *) b;
+	sa = a;
+	sb = b;
 
-	// sort by address first
+	/* sort by address first */
 	if (sa->addr > sb->addr)
 		return 1;
 	if (sa->addr < sb->addr)
 		return -1;
 
-	// sort by "weakness" type
+	/* sort by "weakness" type */
 	wa = (sa->sym[0] == 'w') || (sa->sym[0] == 'W');
 	wb = (sb->sym[0] == 'w') || (sb->sym[0] == 'W');
 	if (wa != wb)
 		return wa - wb;
 
-	// sort by initial order, so that other symbols are left undisturbed
+	/* sort by initial order, so that other symbols are left undisturbed */
 	return sa->start_pos - sb->start_pos;
 }

Mathieu Desnoyers

Acked-by

2007-12-05 05:01 UTC #11 lore raw

Quoted 43 lines · * Andrew Morton ([email protected]) wrote:

Quoted 11 lines · On Tue, 04 Dec 2007 20:35:32 +0000 Paulo Marques <[email protected]> wrote:
When resolving symbol names from addresses with aliased symbol names, kallsyms_lookup always returns the first symbol, even if it is a weak symbol.
This patch changes this by sorting the symbols with the weak symbols last before feeding them to the kernel. This way the kernel runtime isn't changed at all, only the kallsyms build system is changed.
Another side effect is that the symbols get sorted by address, too. So, even if future binutils version have some bug in "nm" that makes it fail to correctly sort symbols by address, the kernel won't be affected by this.
Incremental patch:
From: Andrew Morton <[email protected]>
Fix checkpatch warnings:
ERROR: do not use C99 // comments
#72: FILE: scripts/kallsyms.c:516:
(quoted diff) +1 -0
+	// sort by address first
ERROR: do not use C99 // comments
#78: FILE: scripts/kallsyms.c:522:
(quoted diff) +1 -0
+	// sort by "weakness" type
ERROR: do not use C99 // comments
#84: FILE: scripts/kallsyms.c:528:
(quoted diff) +1 -0
+	// sort by initial order, so that other symbols are left undisturbed
total: 3 errors, 0 warnings, 61 lines checked
Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.
Please run checkpatch prior to sending patches

Quoted 8 lines

And a few coding-style things which checkpatch missed.
And fix up constificiation to remove typecasting.
Cc Mathieu Desnoyers <[email protected]>
Cc Paulo Marques <[email protected]>
Cc Sam Ravnborg <[email protected]>
Signed-off-by Andrew Morton <[email protected]>

(if it's any useful at this point) The comments were the only problems I found. I've tested it and it work fine for me in LTTng.

Acked-by Mathieu Desnoyers <[email protected]>

Quoted 63 lines

---

 scripts/kallsyms.c |   19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff -puN scripts/kallsyms.c~kallsyms-should-prefer-non-weak-symbols-checkpatch-fixes scripts/kallsyms.c

scripts/kallsyms.c renamed from scripts/kallsyms.c~kallsyms-should-prefer-non-weak-symbols-checkpatch-fixes +9 ~~-10~~

@@ -31,14 +31,13 @@
 
 #define KSYM_NAME_LEN		128
 
-
 struct sym_entry {
 	unsigned long long addr;
-	unsigned int len, start_pos;
+	unsigned int len;
+	unsigned int start_pos;
 	unsigned char *sym;
 };
 
-
 static struct sym_entry *table;
 static unsigned int table_size, table_cnt;
 static unsigned long long _text, _stext, _etext, _sinittext, _einittext;

@@ -504,28 +503,28 @@ static void optimize_token_table(void)
 	optimize_result();
 }
 
-
 static int compare_symbols(const void *a, const void *b)
 {
-	struct sym_entry *sa, *sb;
+	const struct sym_entry *sa;
+	const struct sym_entry *sb;
 	int wa, wb;
 
-	sa = (struct sym_entry *) a;
-	sb = (struct sym_entry *) b;
+	sa = a;
+	sb = b;
 
-	// sort by address first
+	/* sort by address first */
 	if (sa->addr > sb->addr)
 		return 1;
 	if (sa->addr < sb->addr)
 		return -1;
 
-	// sort by "weakness" type
+	/* sort by "weakness" type */
 	wa = (sa->sym[0] == 'w') || (sa->sym[0] == 'W');
 	wb = (sb->sym[0] == 'w') || (sb->sym[0] == 'W');
 	if (wa != wb)
 		return wa - wb;
 
-	// sort by initial order, so that other symbols are left undisturbed
+	/* sort by initial order, so that other symbols are left undisturbed */
 	return sa->start_pos - sb->start_pos;
 }

Signature · 3 lines

Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68