2009-03-11 09:38:44

by Lai Jiangshan

[permalink] [raw]
Subject: [PATCH] kallsyms, tracing: output more proper symbol name


Impact: bugfix, output reliable result

Debug tools(dump_stack(), ftrace...) are like to print out symbols.
But it is always print out the first aliased symbol.(Aliased symbols
are symbols with the same address), and the first aliased symbol is
sometime not proper.

# echo function_graph > current_tracer
# cat trace
......
1) 1.923 us | select_nohz_load_balancer();
1) + 76.692 us | }
1) | default_idle() {
1) ==========> | __irqentry_text_start() {
1) 0.000 us | native_apic_mem_write();
1) | irq_enter() {
1) 0.000 us | idle_cpu();
1) | tick_check_idle() {
1) 0.000 us | tick_check_oneshot_broadcast();
1) | tick_nohz_stop_idle() {
......

It's very embarrassing, it ouputs "__irqentry_text_start()",
*actually, it should output "smp_apic_timer_interrupt()"*.
(these two symbol are the same address, but "__irqentry_text_start"
is deemed to the first aliased symbol by scripts/kallsyms)

This patch puts symbols like "__irqentry_text_start" to the second
aliased symbols. And a more proper symbol name becomes the first.

A table is added in scripts/kallsyms.c, and the symbols in this table
have lower priority than other symbols(which are the same address).

This table is statically defined in scripts/kallsyms.c, is not
automatically generated by a script when kernel is being built.
It's for these reasons:
This table is(will be) updated very infrequently.
This table is short.
I don't want to add complexity to kernel-building

Signed-off-by: Lai Jiangshan <[email protected]>
---
diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index ad2434b..96717dd 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -500,11 +500,14 @@ static void optimize_token_table(void)
optimize_result();
}

+static void prepare_hide_symbols(void);
+static int is_hide_symbol(const char *symbol);
+
static int compare_symbols(const void *a, const void *b)
{
const struct sym_entry *sa;
const struct sym_entry *sb;
- int wa, wb;
+ int wa, wb, ha, hb;

sa = a;
sb = b;
@@ -521,12 +524,18 @@ static int compare_symbols(const void *a, const void *b)
if (wa != wb)
return wa - wb;

+ ha = is_hide_symbol((char *)sa->sym + 1);
+ hb = is_hide_symbol((char *)sb->sym + 1);
+ if (ha != hb)
+ return ha - hb;
+
/* sort by initial order, so that other symbols are left undisturbed */
return sa->start_pos - sb->start_pos;
}

static void sort_symbols(void)
{
+ prepare_hide_symbols();
qsort(table, table_cnt, sizeof(struct sym_entry), compare_symbols);
}

@@ -556,3 +565,125 @@ int main(int argc, char **argv)

return 0;
}
+
+#define VMLINUX_SYMBOL(_sym_) #_sym_
+
+static const char *hide_symbols[] = {
+ /* misc symbols */
+ "_text",
+ "_stext",
+ "_etext",
+ "_sinittext",
+ "_einittext",
+
+ /* symbols from include/asm-generic/vmlinux.lds.h */
+ VMLINUX_SYMBOL(__start_mcount_loc),
+ VMLINUX_SYMBOL(__stop_mcount_loc),
+ VMLINUX_SYMBOL(__start_annotated_branch_profile),
+ VMLINUX_SYMBOL(__stop_annotated_branch_profile),
+ VMLINUX_SYMBOL(__start_branch_profile),
+ VMLINUX_SYMBOL(__stop_branch_profile),
+ VMLINUX_SYMBOL(__start___markers),
+ VMLINUX_SYMBOL(__stop___markers),
+ VMLINUX_SYMBOL(__start___tracepoints),
+ VMLINUX_SYMBOL(__stop___tracepoints),
+ VMLINUX_SYMBOL(__start_rodata),
+ VMLINUX_SYMBOL(__start_pci_fixups_early),
+ VMLINUX_SYMBOL(__end_pci_fixups_early),
+ VMLINUX_SYMBOL(__start_pci_fixups_header),
+ VMLINUX_SYMBOL(__end_pci_fixups_header),
+ VMLINUX_SYMBOL(__start_pci_fixups_final),
+ VMLINUX_SYMBOL(__end_pci_fixups_final),
+ VMLINUX_SYMBOL(__start_pci_fixups_enable),
+ VMLINUX_SYMBOL(__end_pci_fixups_enable),
+ VMLINUX_SYMBOL(__start_pci_fixups_resume),
+ VMLINUX_SYMBOL(__end_pci_fixups_resume),
+ VMLINUX_SYMBOL(__start_pci_fixups_resume_early),
+ VMLINUX_SYMBOL(__end_pci_fixups_resume_early),
+ VMLINUX_SYMBOL(__start_pci_fixups_suspend),
+ VMLINUX_SYMBOL(__end_pci_fixups_suspend),
+ VMLINUX_SYMBOL(__start_builtin_fw),
+ VMLINUX_SYMBOL(__end_builtin_fw),
+ VMLINUX_SYMBOL(__start_rio_route_ops),
+ VMLINUX_SYMBOL(__end_rio_route_ops),
+ VMLINUX_SYMBOL(__start___ksymtab),
+ VMLINUX_SYMBOL(__stop___ksymtab),
+ VMLINUX_SYMBOL(__start___ksymtab_gpl),
+ VMLINUX_SYMBOL(__stop___ksymtab_gpl),
+ VMLINUX_SYMBOL(__start___ksymtab_unused),
+ VMLINUX_SYMBOL(__stop___ksymtab_unused),
+ VMLINUX_SYMBOL(__start___ksymtab_unused_gpl),
+ VMLINUX_SYMBOL(__stop___ksymtab_unused_gpl),
+ VMLINUX_SYMBOL(__start___ksymtab_gpl_future),
+ VMLINUX_SYMBOL(__stop___ksymtab_gpl_future),
+ VMLINUX_SYMBOL(__start___kcrctab),
+ VMLINUX_SYMBOL(__stop___kcrctab),
+ VMLINUX_SYMBOL(__start___kcrctab_gpl),
+ VMLINUX_SYMBOL(__stop___kcrctab_gpl),
+ VMLINUX_SYMBOL(__start___kcrctab_unused),
+ VMLINUX_SYMBOL(__stop___kcrctab_unused),
+ VMLINUX_SYMBOL(__start___kcrctab_unused_gpl),
+ VMLINUX_SYMBOL(__stop___kcrctab_unused_gpl),
+ VMLINUX_SYMBOL(__start___kcrctab_gpl_future),
+ VMLINUX_SYMBOL(__stop___kcrctab_gpl_future),
+ VMLINUX_SYMBOL(__start___param),
+ VMLINUX_SYMBOL(__stop___param),
+ VMLINUX_SYMBOL(__end_rodata),
+ VMLINUX_SYMBOL(__security_initcall_start),
+ VMLINUX_SYMBOL(__security_initcall_end),
+ VMLINUX_SYMBOL(__sched_text_start),
+ VMLINUX_SYMBOL(__sched_text_end),
+ VMLINUX_SYMBOL(__lock_text_start),
+ VMLINUX_SYMBOL(__lock_text_end),
+ VMLINUX_SYMBOL(__kprobes_text_start),
+ VMLINUX_SYMBOL(__kprobes_text_end),
+ VMLINUX_SYMBOL(__irqentry_text_start),
+ VMLINUX_SYMBOL(__irqentry_text_end),
+ VMLINUX_SYMBOL(__start___verbose_strings),
+ VMLINUX_SYMBOL(__stop___verbose_strings),
+ VMLINUX_SYMBOL(__start___verbose),
+ VMLINUX_SYMBOL(__stop___verbose),
+ VMLINUX_SYMBOL(__start___bug_table),
+ VMLINUX_SYMBOL(__stop___bug_table),
+ VMLINUX_SYMBOL(__tracedata_start),
+ VMLINUX_SYMBOL(__tracedata_end),
+ VMLINUX_SYMBOL(__start_notes),
+ VMLINUX_SYMBOL(__stop_notes),
+ VMLINUX_SYMBOL(__early_initcall_end),
+ VMLINUX_SYMBOL(__per_cpu_start),
+ VMLINUX_SYMBOL(__per_cpu_end)
+};
+
+
+static int cmp_str(const void *p1, const void *p2)
+{
+ const char *str1 = *(const char * const *)p1;
+ const char *str2 = *(const char * const *)p2;
+
+ return strcmp(str1, str2);
+}
+
+static void prepare_hide_symbols(void)
+{
+ qsort(hide_symbols, sizeof(hide_symbols) / sizeof(hide_symbols[0]),
+ sizeof(hide_symbols[0]), cmp_str);
+}
+
+static int is_hide_symbol(const char *symbol)
+{
+ int low = 0;
+ int high = sizeof(hide_symbols) / sizeof(hide_symbols[0]);
+
+ while (high - low > 0) {
+ int mid = low + (high - low) / 2;
+ int eq = strcmp(hide_symbols[mid], symbol);
+ if (eq == 0)
+ return 1;
+ else if (eq < 0)
+ low = mid + 1;
+ else
+ high = mid;
+ }
+ return 0;
+}
+




2009-03-11 09:47:20

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH] kallsyms, tracing: output more proper symbol name


* Lai Jiangshan <[email protected]> wrote:

> +#define VMLINUX_SYMBOL(_sym_) #_sym_
> +
> +static const char *hide_symbols[] = {
> + /* misc symbols */
> + "_text",
> + "_stext",
> + "_etext",
> + "_sinittext",
> + "_einittext",
> +
> + /* symbols from include/asm-generic/vmlinux.lds.h */
> + VMLINUX_SYMBOL(__start_mcount_loc),
> + VMLINUX_SYMBOL(__stop_mcount_loc),
> + VMLINUX_SYMBOL(__start_annotated_branch_profile),
> + VMLINUX_SYMBOL(__stop_annotated_branch_profile),
> + VMLINUX_SYMBOL(__start_branch_profile),
> + VMLINUX_SYMBOL(__stop_branch_profile),
> + VMLINUX_SYMBOL(__start___markers),
> + VMLINUX_SYMBOL(__stop___markers),
> + VMLINUX_SYMBOL(__start___tracepoints),
> + VMLINUX_SYMBOL(__stop___tracepoints),
> + VMLINUX_SYMBOL(__start_rodata),
> + VMLINUX_SYMBOL(__start_pci_fixups_early),
> + VMLINUX_SYMBOL(__end_pci_fixups_early),
> + VMLINUX_SYMBOL(__start_pci_fixups_header),
> + VMLINUX_SYMBOL(__end_pci_fixups_header),
> + VMLINUX_SYMBOL(__start_pci_fixups_final),
> + VMLINUX_SYMBOL(__end_pci_fixups_final),
> + VMLINUX_SYMBOL(__start_pci_fixups_enable),
> + VMLINUX_SYMBOL(__end_pci_fixups_enable),
> + VMLINUX_SYMBOL(__start_pci_fixups_resume),
> + VMLINUX_SYMBOL(__end_pci_fixups_resume),
> + VMLINUX_SYMBOL(__start_pci_fixups_resume_early),
> + VMLINUX_SYMBOL(__end_pci_fixups_resume_early),
> + VMLINUX_SYMBOL(__start_pci_fixups_suspend),
> + VMLINUX_SYMBOL(__end_pci_fixups_suspend),
> + VMLINUX_SYMBOL(__start_builtin_fw),
> + VMLINUX_SYMBOL(__end_builtin_fw),
> + VMLINUX_SYMBOL(__start_rio_route_ops),
> + VMLINUX_SYMBOL(__end_rio_route_ops),
> + VMLINUX_SYMBOL(__start___ksymtab),
> + VMLINUX_SYMBOL(__stop___ksymtab),
> + VMLINUX_SYMBOL(__start___ksymtab_gpl),
> + VMLINUX_SYMBOL(__stop___ksymtab_gpl),
> + VMLINUX_SYMBOL(__start___ksymtab_unused),
> + VMLINUX_SYMBOL(__stop___ksymtab_unused),
> + VMLINUX_SYMBOL(__start___ksymtab_unused_gpl),
> + VMLINUX_SYMBOL(__stop___ksymtab_unused_gpl),
> + VMLINUX_SYMBOL(__start___ksymtab_gpl_future),
> + VMLINUX_SYMBOL(__stop___ksymtab_gpl_future),
> + VMLINUX_SYMBOL(__start___kcrctab),
> + VMLINUX_SYMBOL(__stop___kcrctab),
> + VMLINUX_SYMBOL(__start___kcrctab_gpl),
> + VMLINUX_SYMBOL(__stop___kcrctab_gpl),
> + VMLINUX_SYMBOL(__start___kcrctab_unused),
> + VMLINUX_SYMBOL(__stop___kcrctab_unused),
> + VMLINUX_SYMBOL(__start___kcrctab_unused_gpl),
> + VMLINUX_SYMBOL(__stop___kcrctab_unused_gpl),
> + VMLINUX_SYMBOL(__start___kcrctab_gpl_future),
> + VMLINUX_SYMBOL(__stop___kcrctab_gpl_future),
> + VMLINUX_SYMBOL(__start___param),
> + VMLINUX_SYMBOL(__stop___param),
> + VMLINUX_SYMBOL(__end_rodata),
> + VMLINUX_SYMBOL(__security_initcall_start),
> + VMLINUX_SYMBOL(__security_initcall_end),
> + VMLINUX_SYMBOL(__sched_text_start),
> + VMLINUX_SYMBOL(__sched_text_end),
> + VMLINUX_SYMBOL(__lock_text_start),
> + VMLINUX_SYMBOL(__lock_text_end),
> + VMLINUX_SYMBOL(__kprobes_text_start),
> + VMLINUX_SYMBOL(__kprobes_text_end),
> + VMLINUX_SYMBOL(__irqentry_text_start),
> + VMLINUX_SYMBOL(__irqentry_text_end),
> + VMLINUX_SYMBOL(__start___verbose_strings),
> + VMLINUX_SYMBOL(__stop___verbose_strings),
> + VMLINUX_SYMBOL(__start___verbose),
> + VMLINUX_SYMBOL(__stop___verbose),
> + VMLINUX_SYMBOL(__start___bug_table),
> + VMLINUX_SYMBOL(__stop___bug_table),
> + VMLINUX_SYMBOL(__tracedata_start),
> + VMLINUX_SYMBOL(__tracedata_end),
> + VMLINUX_SYMBOL(__start_notes),
> + VMLINUX_SYMBOL(__stop_notes),
> + VMLINUX_SYMBOL(__early_initcall_end),
> + VMLINUX_SYMBOL(__per_cpu_start),
> + VMLINUX_SYMBOL(__per_cpu_end)
> +};

I like how you try to solve this at symbol table generation
time.

Instead of this hardcoded table, couldnt we use some more
flexible and more future-proof method? Such as ordering
same-address symbols by underscores:

[same address]

non-underscore symbols first XYZ
single-undescroe symbols second _XYZ
double-underscore symbols third __XYZ

that way the scheme would be more or less self-maintaining as an
underscore already carries a "this is a special, internal
symbol" notion.

Ingo

2009-03-11 10:29:31

by Frederic Weisbecker

[permalink] [raw]
Subject: Re: [PATCH] kallsyms, tracing: output more proper symbol name

On Wed, Mar 11, 2009 at 10:46:21AM +0100, Ingo Molnar wrote:
>
> * Lai Jiangshan <[email protected]> wrote:
>
> > +#define VMLINUX_SYMBOL(_sym_) #_sym_
> > +
> > +static const char *hide_symbols[] = {
> > + /* misc symbols */
> > + "_text",
> > + "_stext",
> > + "_etext",
> > + "_sinittext",
> > + "_einittext",
> > +
> > + /* symbols from include/asm-generic/vmlinux.lds.h */
> > + VMLINUX_SYMBOL(__start_mcount_loc),
> > + VMLINUX_SYMBOL(__stop_mcount_loc),
> > + VMLINUX_SYMBOL(__start_annotated_branch_profile),
> > + VMLINUX_SYMBOL(__stop_annotated_branch_profile),
> > + VMLINUX_SYMBOL(__start_branch_profile),
> > + VMLINUX_SYMBOL(__stop_branch_profile),
> > + VMLINUX_SYMBOL(__start___markers),
> > + VMLINUX_SYMBOL(__stop___markers),
> > + VMLINUX_SYMBOL(__start___tracepoints),
> > + VMLINUX_SYMBOL(__stop___tracepoints),
> > + VMLINUX_SYMBOL(__start_rodata),
> > + VMLINUX_SYMBOL(__start_pci_fixups_early),
> > + VMLINUX_SYMBOL(__end_pci_fixups_early),
> > + VMLINUX_SYMBOL(__start_pci_fixups_header),
> > + VMLINUX_SYMBOL(__end_pci_fixups_header),
> > + VMLINUX_SYMBOL(__start_pci_fixups_final),
> > + VMLINUX_SYMBOL(__end_pci_fixups_final),
> > + VMLINUX_SYMBOL(__start_pci_fixups_enable),
> > + VMLINUX_SYMBOL(__end_pci_fixups_enable),
> > + VMLINUX_SYMBOL(__start_pci_fixups_resume),
> > + VMLINUX_SYMBOL(__end_pci_fixups_resume),
> > + VMLINUX_SYMBOL(__start_pci_fixups_resume_early),
> > + VMLINUX_SYMBOL(__end_pci_fixups_resume_early),
> > + VMLINUX_SYMBOL(__start_pci_fixups_suspend),
> > + VMLINUX_SYMBOL(__end_pci_fixups_suspend),
> > + VMLINUX_SYMBOL(__start_builtin_fw),
> > + VMLINUX_SYMBOL(__end_builtin_fw),
> > + VMLINUX_SYMBOL(__start_rio_route_ops),
> > + VMLINUX_SYMBOL(__end_rio_route_ops),
> > + VMLINUX_SYMBOL(__start___ksymtab),
> > + VMLINUX_SYMBOL(__stop___ksymtab),
> > + VMLINUX_SYMBOL(__start___ksymtab_gpl),
> > + VMLINUX_SYMBOL(__stop___ksymtab_gpl),
> > + VMLINUX_SYMBOL(__start___ksymtab_unused),
> > + VMLINUX_SYMBOL(__stop___ksymtab_unused),
> > + VMLINUX_SYMBOL(__start___ksymtab_unused_gpl),
> > + VMLINUX_SYMBOL(__stop___ksymtab_unused_gpl),
> > + VMLINUX_SYMBOL(__start___ksymtab_gpl_future),
> > + VMLINUX_SYMBOL(__stop___ksymtab_gpl_future),
> > + VMLINUX_SYMBOL(__start___kcrctab),
> > + VMLINUX_SYMBOL(__stop___kcrctab),
> > + VMLINUX_SYMBOL(__start___kcrctab_gpl),
> > + VMLINUX_SYMBOL(__stop___kcrctab_gpl),
> > + VMLINUX_SYMBOL(__start___kcrctab_unused),
> > + VMLINUX_SYMBOL(__stop___kcrctab_unused),
> > + VMLINUX_SYMBOL(__start___kcrctab_unused_gpl),
> > + VMLINUX_SYMBOL(__stop___kcrctab_unused_gpl),
> > + VMLINUX_SYMBOL(__start___kcrctab_gpl_future),
> > + VMLINUX_SYMBOL(__stop___kcrctab_gpl_future),
> > + VMLINUX_SYMBOL(__start___param),
> > + VMLINUX_SYMBOL(__stop___param),
> > + VMLINUX_SYMBOL(__end_rodata),
> > + VMLINUX_SYMBOL(__security_initcall_start),
> > + VMLINUX_SYMBOL(__security_initcall_end),
> > + VMLINUX_SYMBOL(__sched_text_start),
> > + VMLINUX_SYMBOL(__sched_text_end),
> > + VMLINUX_SYMBOL(__lock_text_start),
> > + VMLINUX_SYMBOL(__lock_text_end),
> > + VMLINUX_SYMBOL(__kprobes_text_start),
> > + VMLINUX_SYMBOL(__kprobes_text_end),
> > + VMLINUX_SYMBOL(__irqentry_text_start),
> > + VMLINUX_SYMBOL(__irqentry_text_end),
> > + VMLINUX_SYMBOL(__start___verbose_strings),
> > + VMLINUX_SYMBOL(__stop___verbose_strings),
> > + VMLINUX_SYMBOL(__start___verbose),
> > + VMLINUX_SYMBOL(__stop___verbose),
> > + VMLINUX_SYMBOL(__start___bug_table),
> > + VMLINUX_SYMBOL(__stop___bug_table),
> > + VMLINUX_SYMBOL(__tracedata_start),
> > + VMLINUX_SYMBOL(__tracedata_end),
> > + VMLINUX_SYMBOL(__start_notes),
> > + VMLINUX_SYMBOL(__stop_notes),
> > + VMLINUX_SYMBOL(__early_initcall_end),
> > + VMLINUX_SYMBOL(__per_cpu_start),
> > + VMLINUX_SYMBOL(__per_cpu_end)
> > +};
>
> I like how you try to solve this at symbol table generation
> time.
>
> Instead of this hardcoded table, couldnt we use some more
> flexible and more future-proof method? Such as ordering
> same-address symbols by underscores:
>
> [same address]
>
> non-underscore symbols first XYZ
> single-undescroe symbols second _XYZ
> double-underscore symbols third __XYZ
>
> that way the scheme would be more or less self-maintaining as an
> underscore already carries a "this is a special, internal
> symbol" notion.
>
> Ingo

Thanks Lai!
I didn't know how to solve properly this problem.
But indeed, the underscore priority sorted symbols seems to me a good
and scalable idea.

2009-03-11 14:27:20

by Paulo Marques

[permalink] [raw]
Subject: Re: [PATCH] kallsyms, tracing: output more proper symbol name

Ingo Molnar wrote:
> * Lai Jiangshan <[email protected]> wrote:
>
>> +#define VMLINUX_SYMBOL(_sym_) #_sym_
>> +
>> +static const char *hide_symbols[] = {
>> + /* misc symbols */
>> + "_text",
>> + "_stext",
>> + "_etext",
>> + "_sinittext",
>> + "_einittext",
>> +
>> + /* symbols from include/asm-generic/vmlinux.lds.h */
>> + VMLINUX_SYMBOL(__start_mcount_loc),
>> + VMLINUX_SYMBOL(__stop_mcount_loc),
[...]
>> + VMLINUX_SYMBOL(__early_initcall_end),
>> + VMLINUX_SYMBOL(__per_cpu_start),
>> + VMLINUX_SYMBOL(__per_cpu_end)
>> +};
>
> I like how you try to solve this at symbol table generation
> time.

Yes, this is the proper way to do it.

> Instead of this hardcoded table, couldnt we use some more
> flexible and more future-proof method? Such as ordering
> same-address symbols by underscores:
>
> [same address]
>
> non-underscore symbols first XYZ
> single-undescroe symbols second _XYZ
> double-underscore symbols third __XYZ
>
> that way the scheme would be more or less self-maintaining as an
> underscore already carries a "this is a special, internal
> symbol" notion.

I agree with this approach, but to be on the side we should skim through
the alias and see if this works for most symbols or not.

I just did this for the symbols on my running kernel and it seems to
work. The hierarchy for '_' and '__' is in fact necessary:

> ffffffff80447418 T __sched_text_start
> ffffffff80447418 t sleep_on_common
> ffffffff80449830 T __lock_text_start
> ffffffff80449830 T __sched_text_end
> ffffffff80449830 T _spin_trylock
> ffffffff80453d50 R __stop___ex_table
> ffffffff80453d50 T __start_notes
> ffffffff8045b000 R __start_rodata
> ffffffff8045b000 R linux_banner

or that "_spin_trylock" might be displayed incorrectly as
"__sched_text_end" or something like that.

--
Paulo Marques - http://www.grupopie.com

"Who is general Failure and why is he reading my disk?"

2009-03-12 02:45:24

by Lai Jiangshan

[permalink] [raw]
Subject: Re: [PATCH] kallsyms, tracing: output more proper symbol name

Paulo Marques wrote:
> Ingo Molnar wrote:
>> I like how you try to solve this at symbol table generation
>> time.
>
> Yes, this is the proper way to do it.
>
>> Instead of this hardcoded table, couldnt we use some more
>> flexible and more future-proof method? Such as ordering
>> same-address symbols by underscores:
>>
>> [same address]
>>
>> non-underscore symbols first XYZ
>> single-undescroe symbols second _XYZ
>> double-underscore symbols third __XYZ
>>
>> that way the scheme would be more or less self-maintaining as an
>> underscore already carries a "this is a special, internal
>> symbol" notion.
>
> I agree with this approach, but to be on the side we should skim through
> the alias and see if this works for most symbols or not.
>
> I just did this for the symbols on my running kernel and it seems to
> work. The hierarchy for '_' and '__' is in fact necessary:
>
>> ffffffff80447418 T __sched_text_start
>> ffffffff80447418 t sleep_on_common
>> ffffffff80449830 T __lock_text_start
>> ffffffff80449830 T __sched_text_end
>> ffffffff80449830 T _spin_trylock
>> ffffffff80453d50 R __stop___ex_table
>> ffffffff80453d50 T __start_notes
>> ffffffff8045b000 R __start_rodata
>> ffffffff8045b000 R linux_banner
>
> or that "_spin_trylock" might be displayed incorrectly as
> "__sched_text_end" or something like that.
>

sort by underscore may be incorrect. A lots of symbol in .c
file is '_' or '__' prefix, human are happy to see these symbol.

Symbols in my table(in V1 patch) are linker-script-provide symbols.
Aliased symbols mostly come from linker script. Now, v2 patch's ordering:

[same address]
symbol defined in .c or .S
symbol defined in linker script

From: Lai Jiangshan <[email protected]>

Impact: bugfix, output reliable result

Debug tools(dump_stack(), ftrace...) are like to print out symbols.
But it is always print out the first aliased symbol.(Aliased symbols
are symbols with the same address), and the first aliased symbol is
sometime not proper.

# echo function_graph > current_tracer
# cat trace
......
1) 1.923 us | select_nohz_load_balancer();
1) + 76.692 us | }
1) | default_idle() {
1) ==========> | __irqentry_text_start() {
1) 0.000 us | native_apic_mem_write();
1) | irq_enter() {
1) 0.000 us | idle_cpu();
1) | tick_check_idle() {
1) 0.000 us | tick_check_oneshot_broadcast();
1) | tick_nohz_stop_idle() {
......

It's very embarrassing, it ouputs "__irqentry_text_start()",
actually, it should output "smp_apic_timer_interrupt()".
(these two symbol are the same address, but "__irqentry_text_start"
is deemed to the first aliased symbol by scripts/kallsyms)

This patch puts symbols like "__irqentry_text_start" to the second
aliased symbols. And a more proper symbol name becomes the first.

Aliased symbols mostly come from linker script. The solution is
guessing "is this symbol defined in linker script", the symbols
defined in linker script will not become the first aliased symbol.

Signed-off-by: Lai Jiangshan <[email protected]>
---
diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index ad2434b..c6924d4 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -500,11 +500,46 @@ static void optimize_token_table(void)
optimize_result();
}

+/* guess for "linker script provide" symbol */
+static int may_be_linker_script_provide_symbol(const struct sym_entry *se)
+{
+ const char *symbol = (char *)se->sym + 1;
+ int len = se->len - 1;
+
+ if (len < 8)
+ return 0;
+
+ if (symbol[0] != '_' || symbol[1] != '_')
+ return 0;
+
+ /* __start_XXXXX */
+ if (!memcmp(symbol + 2, "start_", 6))
+ return 1;
+
+ /* __stop_XXXXX */
+ if (!memcmp(symbol + 2, "stop_", 5))
+ return 1;
+
+ /* __end_XXXXX */
+ if (!memcmp(symbol + 2, "end_", 4))
+ return 1;
+
+ /* __XXXXX_start */
+ if (!memcmp(symbol + len - 6, "_start", 6))
+ return 1;
+
+ /* __XXXXX_end */
+ if (!memcmp(symbol + len - 4, "_end", 4))
+ return 1;
+
+ return 0;
+}
+
static int compare_symbols(const void *a, const void *b)
{
const struct sym_entry *sa;
const struct sym_entry *sb;
- int wa, wb;
+ int wa, wb, la, lb;

sa = a;
sb = b;
@@ -521,6 +556,12 @@ static int compare_symbols(const void *a, const void *b)
if (wa != wb)
return wa - wb;

+ /* sort by "linker script provide" type */
+ la = may_be_linker_script_provide_symbol(sa);
+ lb = may_be_linker_script_provide_symbol(sb);
+ if (la != lb)
+ return la - lb;
+
/* sort by initial order, so that other symbols are left undisturbed */
return sa->start_pos - sb->start_pos;
}

2009-03-12 15:33:15

by Paulo Marques

[permalink] [raw]
Subject: Re: [PATCH] kallsyms, tracing: output more proper symbol name

Lai Jiangshan wrote:
> Paulo Marques wrote:
>> Ingo Molnar wrote:
>>> I like how you try to solve this at symbol table generation
>>> time.
>> Yes, this is the proper way to do it.
>>
>>> Instead of this hardcoded table, couldnt we use some more
>>> flexible and more future-proof method? Such as ordering
>>> same-address symbols by underscores:
>>>
>>> [same address]
>>>
>>> non-underscore symbols first XYZ
>>> single-undescroe symbols second _XYZ
>>> double-underscore symbols third __XYZ
>>>
>>> that way the scheme would be more or less self-maintaining as an
>>> underscore already carries a "this is a special, internal
>>> symbol" notion.
>> I agree with this approach, but to be on the side we should skim through
>> the alias and see if this works for most symbols or not.
>>
>> I just did this for the symbols on my running kernel and it seems to
>> work. The hierarchy for '_' and '__' is in fact necessary:
>>
>>> ffffffff80447418 T __sched_text_start
>>> ffffffff80447418 t sleep_on_common
>>> ffffffff80449830 T __lock_text_start
>>> ffffffff80449830 T __sched_text_end
>>> ffffffff80449830 T _spin_trylock
>>> ffffffff80453d50 R __stop___ex_table
>>> ffffffff80453d50 T __start_notes
>>> ffffffff8045b000 R __start_rodata
>>> ffffffff8045b000 R linux_banner
>> or that "_spin_trylock" might be displayed incorrectly as
>> "__sched_text_end" or something like that.
>>
>
> sort by underscore may be incorrect. A lots of symbol in .c
> file is '_' or '__' prefix, human are happy to see these symbol.

Since this is just a secondary criteria that applies only to _aliased_
symbols, this shouldn't be a big problem. Symbols with different
addresses are properly placed in the correct order.

> Symbols in my table(in V1 patch) are linker-script-provide symbols.
> Aliased symbols mostly come from linker script. Now, v2 patch's ordering:
>
> [same address]
> symbol defined in .c or .S
> symbol defined in linker script
>
> From: Lai Jiangshan <[email protected]>
>
[...]
> diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
> index ad2434b..c6924d4 100644
> --- a/scripts/kallsyms.c
> +++ b/scripts/kallsyms.c
> @@ -500,11 +500,46 @@ static void optimize_token_table(void)
> optimize_result();
> }
>
> +/* guess for "linker script provide" symbol */
> +static int may_be_linker_script_provide_symbol(const struct sym_entry *se)
> +{
> + const char *symbol = (char *)se->sym + 1;
> + int len = se->len - 1;
> +
> + if (len < 8)
> + return 0;
> +
> + if (symbol[0] != '_' || symbol[1] != '_')
> + return 0;
> +
> + /* __start_XXXXX */
> + if (!memcmp(symbol + 2, "start_", 6))
> + return 1;
> +
> + /* __stop_XXXXX */
> + if (!memcmp(symbol + 2, "stop_", 5))
> + return 1;
> +
> + /* __end_XXXXX */
> + if (!memcmp(symbol + 2, "end_", 4))
> + return 1;
> +
> + /* __XXXXX_start */
> + if (!memcmp(symbol + len - 6, "_start", 6))
> + return 1;
> +
> + /* __XXXXX_end */
> + if (!memcmp(symbol + len - 4, "_end", 4))
> + return 1;
> +
> + return 0;
> +}
> +
> static int compare_symbols(const void *a, const void *b)
> {
> const struct sym_entry *sa;
> const struct sym_entry *sb;
> - int wa, wb;
> + int wa, wb, la, lb;
>
> sa = a;
> sb = b;
> @@ -521,6 +556,12 @@ static int compare_symbols(const void *a, const void *b)
> if (wa != wb)
> return wa - wb;
>
> + /* sort by "linker script provide" type */
> + la = may_be_linker_script_provide_symbol(sa);
> + lb = may_be_linker_script_provide_symbol(sb);
> + if (la != lb)
> + return la - lb;
> +
> /* sort by initial order, so that other symbols are left undisturbed */
> return sa->start_pos - sb->start_pos;
> }

I would probably still place another test to compare the number of
underscores, after both symbols are found to be equal in the "linker
script provided" criteria.

The way it is now, for my current kallsyms table:

> ffffffff80200000 A _text
> ffffffff80200000 T startup_64
> ffffffff80209000 T _stext
> ffffffff80209000 t init_post

a "startup_64" address would display as "_text" and a "init_post"
address would display as "_stext".

You can add all the stext/etext symbols as special cases to the
"may_be_linker_script_provide_symbol" function (like it was on your v1
patch), but I'm just afraid that we'll find more cases in the future
that are not automatically caught by these rules...

--
Paulo Marques - http://www.grupopie.com

"All generalizations are false."

2009-03-13 07:12:18

by Lai Jiangshan

[permalink] [raw]
Subject: [PATCH V3] kallsyms, tracing: output more proper symbol name

Paulo Marques wrote:
> I would probably still place another test to compare the number of
> underscores, after both symbols are found to be equal in the "linker
> script provided" criteria.
>
> The way it is now, for my current kallsyms table:
>
>> ffffffff80200000 A _text
>> ffffffff80200000 T startup_64
>> ffffffff80209000 T _stext
>> ffffffff80209000 t init_post
>
> a "startup_64" address would display as "_text" and a "init_post"
> address would display as "_stext".
>
> You can add all the stext/etext symbols as special cases to the
> "may_be_linker_script_provide_symbol" function (like it was on your v1
> patch), but I'm just afraid that we'll find more cases in the future
> that are not automatically caught by these rules...
>

Subject: kallsyms, tracing: output more proper symbol name

Impact: bugfix, output reliable result

Debug tools(dump_stack(), ftrace...) are like to print out symbols.
But it is always print out the first aliased symbol.(Aliased symbols
are symbols with the same address), and the first aliased symbol is
sometime not proper.

# echo function_graph > current_tracer
# cat trace
......
1) 1.923 us | select_nohz_load_balancer();
1) + 76.692 us | }
1) | default_idle() {
1) ==========> | __irqentry_text_start() {
1) 0.000 us | native_apic_mem_write();
1) | irq_enter() {
1) 0.000 us | idle_cpu();
1) | tick_check_idle() {
1) 0.000 us | tick_check_oneshot_broadcast();
1) | tick_nohz_stop_idle() {
......

It's very embarrassing, it ouputs "__irqentry_text_start()",
actually, it should output "smp_apic_timer_interrupt()".
(these two symbol are the same address, but "__irqentry_text_start"
is deemed to the first aliased symbol by scripts/kallsyms)

This patch puts symbols like "__irqentry_text_start" to the second
aliased symbols. And a more proper symbol name becomes the first.

Aliased symbols mostly come from linker script. The solution is
guessing "is this symbol defined in linker script", the symbols
defined in linker script will not become the first aliased symbol.

And if symbols are found to be equal in this "linker script provided"
criteria, symbols are sorted by the number of prefix underscores.

Signed-off-by: Lai Jiangshan <[email protected]>
---
diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index ad2434b..6654cbe 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -500,6 +500,51 @@ static void optimize_token_table(void)
optimize_result();
}

+/* guess for "linker script provide" symbol */
+static int may_be_linker_script_provide_symbol(const struct sym_entry *se)
+{
+ const char *symbol = (char *)se->sym + 1;
+ int len = se->len - 1;
+
+ if (len < 8)
+ return 0;
+
+ if (symbol[0] != '_' || symbol[1] != '_')
+ return 0;
+
+ /* __start_XXXXX */
+ if (!memcmp(symbol + 2, "start_", 6))
+ return 1;
+
+ /* __stop_XXXXX */
+ if (!memcmp(symbol + 2, "stop_", 5))
+ return 1;
+
+ /* __end_XXXXX */
+ if (!memcmp(symbol + 2, "end_", 4))
+ return 1;
+
+ /* __XXXXX_start */
+ if (!memcmp(symbol + len - 6, "_start", 6))
+ return 1;
+
+ /* __XXXXX_end */
+ if (!memcmp(symbol + len - 4, "_end", 4))
+ return 1;
+
+ return 0;
+}
+
+static int prefix_underscores_count(const char *str)
+{
+ const char *tail = str;
+
+ while (*tail != '_')
+ tail++;
+
+ return tail - str;
+}
+
static int compare_symbols(const void *a, const void *b)
{
const struct sym_entry *sa;
@@ -521,6 +566,18 @@ static int compare_symbols(const void *a, const void *b)
if (wa != wb)
return wa - wb;

+ /* sort by "linker script provide" type */
+ wa = may_be_linker_script_provide_symbol(sa);
+ wb = may_be_linker_script_provide_symbol(sb);
+ if (wa != wb)
+ return wa - wb;
+
+ /* sort by the number of prefix underscores */
+ wa = prefix_underscores_count((const char *)sa->sym + 1);
+ wb = prefix_underscores_count((const char *)sb->sym + 1);
+ if (wa != wb)
+ return wa - wb;
+
/* sort by initial order, so that other symbols are left undisturbed */
return sa->start_pos - sb->start_pos;
}

2009-03-13 09:29:18

by Lai Jiangshan

[permalink] [raw]
Subject: Re: [PATCH V3] kallsyms, tracing: output more proper symbol name

Lai Jiangshan wrote:
> Paulo Marques wrote:
>> I would probably still place another test to compare the number of
>> underscores, after both symbols are found to be equal in the "linker
>> script provided" criteria.
>>
>> The way it is now, for my current kallsyms table:
>>
>>> ffffffff80200000 A _text
>>> ffffffff80200000 T startup_64
>>> ffffffff80209000 T _stext
>>> ffffffff80209000 t init_post
>> a "startup_64" address would display as "_text" and a "init_post"
>> address would display as "_stext".
>>
>> You can add all the stext/etext symbols as special cases to the
>> "may_be_linker_script_provide_symbol" function (like it was on your v1
>> patch), but I'm just afraid that we'll find more cases in the future
>> that are not automatically caught by these rules...
>>
>

[resend, previous v3 has some mistakes]

Subject: kallsyms, tracing: output more proper symbol name

Impact: bugfix, output reliable result

Debug tools(dump_stack(), ftrace...) are like to print out symbols.
But it is always print out the first aliased symbol.(Aliased symbols
are symbols with the same address), and the first aliased symbol is
sometime not proper.

# echo function_graph > current_tracer
# cat trace
......
1) 1.923 us | select_nohz_load_balancer();
1) + 76.692 us | }
1) | default_idle() {
1) ==========> | __irqentry_text_start() {
1) 0.000 us | native_apic_mem_write();
1) | irq_enter() {
1) 0.000 us | idle_cpu();
1) | tick_check_idle() {
1) 0.000 us | tick_check_oneshot_broadcast();
1) | tick_nohz_stop_idle() {
......

It's very embarrassing, it ouputs "__irqentry_text_start()",
actually, it should output "smp_apic_timer_interrupt()".
(these two symbol are the same address, but "__irqentry_text_start"
is deemed to the first aliased symbol by scripts/kallsyms)

This patch puts symbols like "__irqentry_text_start" to the second
aliased symbols. And a more proper symbol name becomes the first.

Aliased symbols mostly come from linker script. The solution is
guessing "is this symbol defined in linker script", the symbols
defined in linker script will not become the first aliased symbol.

And if symbols are found to be equal in this "linker script provided"
criteria, symbols are sorted by the number of prefix underscores.

Signed-off-by: Lai Jiangshan <[email protected]>
---
diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index ad2434b..b187edc 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -500,6 +500,51 @@ static void optimize_token_table(void)
optimize_result();
}

+/* guess for "linker script provide" symbol */
+static int may_be_linker_script_provide_symbol(const struct sym_entry *se)
+{
+ const char *symbol = (char *)se->sym + 1;
+ int len = se->len - 1;
+
+ if (len < 8)
+ return 0;
+
+ if (symbol[0] != '_' || symbol[1] != '_')
+ return 0;
+
+ /* __start_XXXXX */
+ if (!memcmp(symbol + 2, "start_", 6))
+ return 1;
+
+ /* __stop_XXXXX */
+ if (!memcmp(symbol + 2, "stop_", 5))
+ return 1;
+
+ /* __end_XXXXX */
+ if (!memcmp(symbol + 2, "end_", 4))
+ return 1;
+
+ /* __XXXXX_start */
+ if (!memcmp(symbol + len - 6, "_start", 6))
+ return 1;
+
+ /* __XXXXX_end */
+ if (!memcmp(symbol + len - 4, "_end", 4))
+ return 1;
+
+ return 0;
+}
+
+static int prefix_underscores_count(const char *str)
+{
+ const char *tail = str;
+
+ while (*tail == '_')
+ tail++;
+
+ return tail - str;
+}
+
static int compare_symbols(const void *a, const void *b)
{
const struct sym_entry *sa;
@@ -521,6 +566,18 @@ static int compare_symbols(const void *a, const void *b)
if (wa != wb)
return wa - wb;

+ /* sort by "linker script provide" type */
+ wa = may_be_linker_script_provide_symbol(sa);
+ wb = may_be_linker_script_provide_symbol(sb);
+ if (wa != wb)
+ return wa - wb;
+
+ /* sort by the number of prefix underscores */
+ wa = prefix_underscores_count((const char *)sa->sym + 1);
+ wb = prefix_underscores_count((const char *)sb->sym + 1);
+ if (wa != wb)
+ return wa - wb;
+
/* sort by initial order, so that other symbols are left undisturbed */
return sa->start_pos - sb->start_pos;
}

2009-03-13 19:26:23

by Paulo Marques

[permalink] [raw]
Subject: Re: [PATCH V3] kallsyms, tracing: output more proper symbol name

Lai Jiangshan wrote:
> [...]
> [resend, previous v3 has some mistakes]
>
> Subject: kallsyms, tracing: output more proper symbol name
>
> Impact: bugfix, output reliable result
>
> Debug tools(dump_stack(), ftrace...) are like to print out symbols.
> But it is always print out the first aliased symbol.(Aliased symbols
> are symbols with the same address), and the first aliased symbol is
> sometime not proper.
> [...]
> Aliased symbols mostly come from linker script. The solution is
> guessing "is this symbol defined in linker script", the symbols
> defined in linker script will not become the first aliased symbol.
>
> And if symbols are found to be equal in this "linker script provided"
> criteria, symbols are sorted by the number of prefix underscores.
>
> Signed-off-by: Lai Jiangshan <[email protected]>
> ---
> [...]
> +static int prefix_underscores_count(const char *str)
> +{
> + const char *tail = str;
> +
> + while (*tail == '_')
> + tail++;
> +
> + return tail - str;
> +}
> +
> static int compare_symbols(const void *a, const void *b)
> {
> const struct sym_entry *sa;
> @@ -521,6 +566,18 @@ static int compare_symbols(const void *a, const void *b)
> if (wa != wb)
> return wa - wb;
>
> + /* sort by "linker script provide" type */
> + wa = may_be_linker_script_provide_symbol(sa);
> + wb = may_be_linker_script_provide_symbol(sb);
> + if (wa != wb)
> + return wa - wb;
> +
> + /* sort by the number of prefix underscores */
> + wa = prefix_underscores_count((const char *)sa->sym + 1);
> + wb = prefix_underscores_count((const char *)sb->sym + 1);
> + if (wa != wb)
> + return wa - wb;
> +
> /* sort by initial order, so that other symbols are left undisturbed */
> return sa->start_pos - sb->start_pos;
> }

The code seems fine. I have no time to compile and test it right now,
but there are no obvious problems as far as I can see.

Reviewed-by: Paulo Marques <[email protected]>

--
Paulo Marques - http://www.grupopie.com

"Measure with laser, mark with chalk, cut with chainsaw."

2009-03-13 19:38:25

by Sam Ravnborg

[permalink] [raw]
Subject: Re: [PATCH V3] kallsyms, tracing: output more proper symbol name

On Fri, Mar 13, 2009 at 05:27:32PM +0800, Lai Jiangshan wrote:
> Lai Jiangshan wrote:
> > Paulo Marques wrote:
> >> I would probably still place another test to compare the number of
> >> underscores, after both symbols are found to be equal in the "linker
> >> script provided" criteria.
> >>
> >> The way it is now, for my current kallsyms table:
> >>
> >>> ffffffff80200000 A _text
> >>> ffffffff80200000 T startup_64
> >>> ffffffff80209000 T _stext
> >>> ffffffff80209000 t init_post
> >> a "startup_64" address would display as "_text" and a "init_post"
> >> address would display as "_stext".
> >>
> >> You can add all the stext/etext symbols as special cases to the
> >> "may_be_linker_script_provide_symbol" function (like it was on your v1
> >> patch), but I'm just afraid that we'll find more cases in the future
> >> that are not automatically caught by these rules...
> >>
> >
>
> [resend, previous v3 has some mistakes]
>
> Subject: kallsyms, tracing: output more proper symbol name
>
> Impact: bugfix, output reliable result
>
> Debug tools(dump_stack(), ftrace...) are like to print out symbols.
> But it is always print out the first aliased symbol.(Aliased symbols
> are symbols with the same address), and the first aliased symbol is
> sometime not proper.
>
> # echo function_graph > current_tracer
> # cat trace
> ......
> 1) 1.923 us | select_nohz_load_balancer();
> 1) + 76.692 us | }
> 1) | default_idle() {
> 1) ==========> | __irqentry_text_start() {
> 1) 0.000 us | native_apic_mem_write();
> 1) | irq_enter() {
> 1) 0.000 us | idle_cpu();
> 1) | tick_check_idle() {
> 1) 0.000 us | tick_check_oneshot_broadcast();
> 1) | tick_nohz_stop_idle() {
> ......
>
> It's very embarrassing, it ouputs "__irqentry_text_start()",
> actually, it should output "smp_apic_timer_interrupt()".
> (these two symbol are the same address, but "__irqentry_text_start"
> is deemed to the first aliased symbol by scripts/kallsyms)
>
> This patch puts symbols like "__irqentry_text_start" to the second
> aliased symbols. And a more proper symbol name becomes the first.
>
> Aliased symbols mostly come from linker script. The solution is
> guessing "is this symbol defined in linker script", the symbols
> defined in linker script will not become the first aliased symbol.
>
> And if symbols are found to be equal in this "linker script provided"
> criteria, symbols are sorted by the number of prefix underscores.
>
> Signed-off-by: Lai Jiangshan <[email protected]>

Looks good to me.

Acked-by: Sam Ravnborg <[email protected]>

2009-03-13 20:12:43

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH V3] kallsyms, tracing: output more proper symbol name

On Fri, 13 Mar 2009 20:40:13 +0100
Sam Ravnborg <[email protected]> wrote:

> > This patch puts symbols like "__irqentry_text_start" to the second
> > aliased symbols. And a more proper symbol name becomes the first.
> >
> > Aliased symbols mostly come from linker script. The solution is
> > guessing "is this symbol defined in linker script", the symbols
> > defined in linker script will not become the first aliased symbol.
> >
> > And if symbols are found to be equal in this "linker script provided"
> > criteria, symbols are sorted by the number of prefix underscores.
> >
> > Signed-off-by: Lai Jiangshan <[email protected]>
>
> Looks good to me.
>
> Acked-by: Sam Ravnborg <[email protected]>

So... do we need this in 2.6.29?

I'm about to jump on a plane and shall be a bit intermittent for a week.

2009-03-13 20:34:24

by Paulo Marques

[permalink] [raw]
Subject: Re: [PATCH V3] kallsyms, tracing: output more proper symbol name

Andrew Morton wrote:
> On Fri, 13 Mar 2009 20:40:13 +0100
> Sam Ravnborg <[email protected]> wrote:
>
>>> This patch puts symbols like "__irqentry_text_start" to the second
>>> aliased symbols. And a more proper symbol name becomes the first.
>>>
>>> Aliased symbols mostly come from linker script. The solution is
>>> guessing "is this symbol defined in linker script", the symbols
>>> defined in linker script will not become the first aliased symbol.
>>>
>>> And if symbols are found to be equal in this "linker script provided"
>>> criteria, symbols are sorted by the number of prefix underscores.
>>>
>>> Signed-off-by: Lai Jiangshan <[email protected]>
>> Looks good to me.
>>
>> Acked-by: Sam Ravnborg <[email protected]>
>
> So... do we need this in 2.6.29?

IMHO, no. I think the way the kernel handles aliased symbols hasn't
changed in a very long time so one more release shouldn't make a big
difference.

Even if the risk of the patch breaking something is small, taking that
chance for such a small gain this late in the 2.6.29 cycle is just not
worth it.

--
Paulo Marques - http://www.grupopie.com

"You're just jealous because the voices only talk to me."

2009-03-14 08:55:45

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH V3] kallsyms, tracing: output more proper symbol name


* Andrew Morton <[email protected]> wrote:

> On Fri, 13 Mar 2009 20:40:13 +0100
> Sam Ravnborg <[email protected]> wrote:
>
> > > This patch puts symbols like "__irqentry_text_start" to the second
> > > aliased symbols. And a more proper symbol name becomes the first.
> > >
> > > Aliased symbols mostly come from linker script. The solution is
> > > guessing "is this symbol defined in linker script", the symbols
> > > defined in linker script will not become the first aliased symbol.
> > >
> > > And if symbols are found to be equal in this "linker script provided"
> > > criteria, symbols are sorted by the number of prefix underscores.
> > >
> > > Signed-off-by: Lai Jiangshan <[email protected]>
> >
> > Looks good to me.
> >
> > Acked-by: Sam Ravnborg <[email protected]>
>
> So... do we need this in 2.6.29?

no. This got raised in the context of new tracing changes queued
up for 2.6.30.

Ingo

2009-03-14 08:58:19

by Lai Jiangshan

[permalink] [raw]
Subject: [tip:tracing/ftrace] kallsyms, tracing: output more proper symbol name

Commit-ID: b478b782e110fdb4135caa3062b6d687e989d994
Gitweb: http://git.kernel.org/tip/b478b782e110fdb4135caa3062b6d687e989d994
Author: Lai Jiangshan <[email protected]>
AuthorDate: Fri, 13 Mar 2009 15:10:26 +0800
Commit: Ingo Molnar <[email protected]>
CommitDate: Sat, 14 Mar 2009 09:55:04 +0100

kallsyms, tracing: output more proper symbol name

Impact: bugfix, output more reliable symbol lookup result

Debug tools(dump_stack(), ftrace...) are like to print out symbols.
But it is always print out the first aliased symbol.(Aliased symbols
are symbols with the same address), and the first aliased symbol is
sometime not proper.

# echo function_graph > current_tracer
# cat trace
.....
1) 1.923 us | select_nohz_load_balancer();
1) + 76.692 us | }
1) | default_idle() {
1) ==========> | __irqentry_text_start() {
1) 0.000 us | native_apic_mem_write();
1) | irq_enter() {
1) 0.000 us | idle_cpu();
1) | tick_check_idle() {
1) 0.000 us | tick_check_oneshot_broadcast();
1) | tick_nohz_stop_idle() {
.....

It's very embarrassing, it ouputs "__irqentry_text_start()",
actually, it should output "smp_apic_timer_interrupt()".
(these two symbol are the same address, but "__irqentry_text_start"
is deemed to the first aliased symbol by scripts/kallsyms)

This patch puts symbols like "__irqentry_text_start" to the second
aliased symbols. And a more proper symbol name becomes the first.

Aliased symbols mostly come from linker script. The solution is
guessing "is this symbol defined in linker script", the symbols
defined in linker script will not become the first aliased symbol.

And if symbols are found to be equal in this "linker script provided"
criteria, symbols are sorted by the number of prefix underscores.

Signed-off-by: Lai Jiangshan <[email protected]>
Acked-by: Sam Ravnborg <[email protected]>
Reviewed-by: Paulo Marques <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>


---
scripts/kallsyms.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 57 insertions(+), 0 deletions(-)

diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index ad2434b..6654cbe 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -500,6 +500,51 @@ static void optimize_token_table(void)
optimize_result();
}

+/* guess for "linker script provide" symbol */
+static int may_be_linker_script_provide_symbol(const struct sym_entry *se)
+{
+ const char *symbol = (char *)se->sym + 1;
+ int len = se->len - 1;
+
+ if (len < 8)
+ return 0;
+
+ if (symbol[0] != '_' || symbol[1] != '_')
+ return 0;
+
+ /* __start_XXXXX */
+ if (!memcmp(symbol + 2, "start_", 6))
+ return 1;
+
+ /* __stop_XXXXX */
+ if (!memcmp(symbol + 2, "stop_", 5))
+ return 1;
+
+ /* __end_XXXXX */
+ if (!memcmp(symbol + 2, "end_", 4))
+ return 1;
+
+ /* __XXXXX_start */
+ if (!memcmp(symbol + len - 6, "_start", 6))
+ return 1;
+
+ /* __XXXXX_end */
+ if (!memcmp(symbol + len - 4, "_end", 4))
+ return 1;
+
+ return 0;
+}
+
+static int prefix_underscores_count(const char *str)
+{
+ const char *tail = str;
+
+ while (*tail != '_')
+ tail++;
+
+ return tail - str;
+}
+
static int compare_symbols(const void *a, const void *b)
{
const struct sym_entry *sa;
@@ -521,6 +566,18 @@ static int compare_symbols(const void *a, const void *b)
if (wa != wb)
return wa - wb;

+ /* sort by "linker script provide" type */
+ wa = may_be_linker_script_provide_symbol(sa);
+ wb = may_be_linker_script_provide_symbol(sb);
+ if (wa != wb)
+ return wa - wb;
+
+ /* sort by the number of prefix underscores */
+ wa = prefix_underscores_count((const char *)sa->sym + 1);
+ wb = prefix_underscores_count((const char *)sb->sym + 1);
+ if (wa != wb)
+ return wa - wb;
+
/* sort by initial order, so that other symbols are left undisturbed */
return sa->start_pos - sb->start_pos;
}