[-mm PATCH] kallsyms should prefer non weak symbols

[-mm PATCH] kallsyms should prefer non weak symbols

[Paulo Marques]

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


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 <pmarques@grupopie.com>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>

-- 
Paulo Marques - www.grupopie.com

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

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1467 bytes --]

--- ./scripts/kallsyms.c.orig	2007-10-30 18:51:28.000000000 +0000
+++ ./scripts/kallsyms.c	2007-10-30 19:07:58.000000000 +0000
@@ -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();
 

Re: [-mm PATCH] kallsyms should prefer non weak symbols

[Mathieu Desnoyers]

* Paulo Marques (pmarques@grupopie.com) 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.
>
>
> From: Paulo Marques <pmarques@grupopie.com>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
>

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

See comment below,

> -- 
> Paulo Marques - www.grupopie.com
>
> "There cannot be a crisis today; my schedule is already full."

> --- ./scripts/kallsyms.c.orig	2007-10-30 18:51:28.000000000 +0000
> +++ ./scripts/kallsyms.c	2007-10-30 19:07:58.000000000 +0000
> @@ -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

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

Please run through scripts/checkpatch.pl before submitting.

> +	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();
>  
> 


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

Re: [-mm PATCH] kallsyms should prefer non weak symbols

[Paulo Marques]

Mathieu Desnoyers wrote:
> * Paulo Marques (pmarques@grupopie.com) 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 <pmarques@grupopie.com>
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
> 
> 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... :(

-- 
Paulo Marques - www.grupopie.com

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

Re: [-mm PATCH] kallsyms should prefer non weak symbols

[Andrew Morton]

On Tue, 04 Dec 2007 20:35:32 +0000
Paulo Marques <pmarques@grupopie.com> 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.
> 
> 

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

Re: [-mm PATCH] kallsyms should prefer non weak symbols

[Mathieu Desnoyers]

* Andrew Morton (akpm@linux-foundation.org) wrote:
> On Tue, 04 Dec 2007 20:35:32 +0000
> Paulo Marques <pmarques@grupopie.com> 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.
> > 
> > 
> 
> 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

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

Re: [-mm PATCH] kallsyms should prefer non weak symbols

[Arjan van de Ven]

On Tue, 04 Dec 2007 20:35:32 +0000
Paulo Marques <pmarques@grupopie.com> 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?


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [-mm PATCH] kallsyms should prefer non weak symbols

[Mathieu Desnoyers]

* Arjan van de Ven (arjan@infradead.org) wrote:
> On Tue, 04 Dec 2007 20:35:32 +0000
> Paulo Marques <pmarques@grupopie.com> 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?
> 

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

> 
> -- 
> If you want to reach me at my work email, use arjan@linux.intel.com
> For development, discussion and tips for power savings, 
> visit http://www.lesswatts.org
> 

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

Re: [-mm PATCH] kallsyms should prefer non weak symbols

[Rusty Russell]

On Wednesday 05 December 2007 09:31:23 Mathieu Desnoyers wrote:
> * Arjan van de Ven (arjan@infradead.org) wrote:
> > On Tue, 04 Dec 2007 20:35:32 +0000
> >
> > Paulo Marques <pmarques@grupopie.com> 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?
>
> 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.

Re: [-mm PATCH] kallsyms should prefer non weak symbols

[Rusty Russell]

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.

Re: [-mm PATCH] kallsyms should prefer non weak symbols

[Andrew Morton]

On Tue, 04 Dec 2007 20:35:32 +0000 Paulo Marques <pmarques@grupopie.com> 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.
> 
> 
> From: Paulo Marques <pmarques@grupopie.com>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>

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...

Re: [-mm PATCH] kallsyms should prefer non weak symbols

[Andrew Morton]

On Tue, 04 Dec 2007 20:35:32 +0000 Paulo Marques <pmarques@grupopie.com> 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 <akpm@linux-foundation.org>

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 <mathieu.desnoyers@polymtl.ca>
Cc: Paulo Marques <pmarques@grupopie.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 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
--- a/scripts/kallsyms.c~kallsyms-should-prefer-non-weak-symbols-checkpatch-fixes
+++ a/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;
 }
 
_

Re: [-mm PATCH] kallsyms should prefer non weak symbols

[Mathieu Desnoyers]

* Andrew Morton (akpm@linux-foundation.org) wrote:
> On Tue, 04 Dec 2007 20:35:32 +0000 Paulo Marques <pmarques@grupopie.com> 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 <akpm@linux-foundation.org>
> 
> 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 <mathieu.desnoyers@polymtl.ca>
> Cc: Paulo Marques <pmarques@grupopie.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

(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 <mathieu.desnoyers@polymtl.ca>

> ---
> 
>  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
> --- a/scripts/kallsyms.c~kallsyms-should-prefer-non-weak-symbols-checkpatch-fixes
> +++ a/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
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68