--- ./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();
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.
>
>
I don't understand the reason for making this change.
* 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.
>
>
> 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,
> --
> Paulo Marques - http://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
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?
--
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
* Andrew Morton ([email protected]) wrote:
> 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.
> >
> >
>
> 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
* Arjan van de Ven ([email protected]) wrote:
> 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?
>
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 [email protected]
> 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
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.
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.
>
>
> 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...
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(-)
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;
}
_
* Andrew Morton ([email protected]) wrote:
> 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]>
(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]>
> ---
>
> 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
On Wednesday 05 December 2007 09:31:23 Mathieu Desnoyers wrote:
> * Arjan van de Ven ([email protected]) wrote:
> > 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?
>
> 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.
Mathieu Desnoyers wrote:
> * 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... :(
--
Paulo Marques - http://www.grupopie.com
"I haven't lost my mind -- it's backed up on tape somewhere."