[v9] kallsyms: Add self-test facility

  Added test cases for basic functions and performance of functions
kallsyms_lookup_name(), kallsyms_on_each_symbol() and
kallsyms_on_each_match_symbol(). It also calculates the compression rate
of the kallsyms compression algorithm for the current symbol set.

The basic functions test begins by testing a set of symbols whose address
values are known. Then, traverse all symbol addresses and find the
corresponding symbol name based on the address. It's impossible to
determine whether these addresses are correct, but we can use the above
three functions along with the addresses to test each other. Due to the
traversal operation of kallsyms_on_each_symbol() is too slow, only 60
symbols can be tested in one second, so let it test on average once
every 128 symbols. The other two functions validate all symbols.

If the basic functions test is passed, print only performance test
results. If the test fails, print error information, but do not perform
subsequent performance tests.

Start self-test automatically after system startup if
CONFIG_KALLSYMS_SELFTEST=y.

Example of output content: (prefix 'kallsyms_selftest:' is omitted
 start
  ---------------------------------------------------------
 | nr_symbols | compressed size | original size | ratio(%) |
 |---------------------------------------------------------|
 |     107543 |       1357912   |      2407433  |  56.40   |
  ---------------------------------------------------------
 kallsyms_lookup_name() looked up 107543 symbols
 The time spent on each symbol is (ns): min=630, max=35295, avg=7353
 kallsyms_on_each_symbol() traverse all: 11782628 ns
 kallsyms_on_each_match_symbol() traverse all: 9261 ns
 finish

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 include/linux/kallsyms.h   |   1 +
 init/Kconfig               |  13 +
 kernel/Makefile            |   1 +
 kernel/kallsyms.c          |   2 +-
 kernel/kallsyms_selftest.c | 485 +++++++++++++++++++++++++++++++++++++
 kernel/kallsyms_selftest.h |  13 +
 6 files changed, 514 insertions(+), 1 deletion(-)
 create mode 100644 kernel/kallsyms_selftest.c
 create mode 100644 kernel/kallsyms_selftest.h
  

On Tue, Nov 15, 2022 at 04:33:48PM +0800, Zhen Lei wrote:
> Added test cases for basic functions and performance of functions
> kallsyms_lookup_name(), kallsyms_on_each_symbol() and
> kallsyms_on_each_match_symbol(). It also calculates the compression rate
> of the kallsyms compression algorithm for the current symbol set.
> 
> The basic functions test begins by testing a set of symbols whose address
> values are known. Then, traverse all symbol addresses and find the
> corresponding symbol name based on the address. It's impossible to
> determine whether these addresses are correct, but we can use the above
> three functions along with the addresses to test each other. Due to the
> traversal operation of kallsyms_on_each_symbol() is too slow, only 60
> symbols can be tested in one second, so let it test on average once
> every 128 symbols. The other two functions validate all symbols.
> 
> If the basic functions test is passed, print only performance test
> results. If the test fails, print error information, but do not perform
> subsequent performance tests.
> 
> Start self-test automatically after system startup if
> CONFIG_KALLSYMS_SELFTEST=y.
> 
> Example of output content: (prefix 'kallsyms_selftest:' is omitted
>  start
>   ---------------------------------------------------------
>  | nr_symbols | compressed size | original size | ratio(%) |
>  |---------------------------------------------------------|
>  |     107543 |       1357912   |      2407433  |  56.40   |
>   ---------------------------------------------------------
>  kallsyms_lookup_name() looked up 107543 symbols
>  The time spent on each symbol is (ns): min=630, max=35295, avg=7353
>  kallsyms_on_each_symbol() traverse all: 11782628 ns
>  kallsyms_on_each_match_symbol() traverse all: 9261 ns
>  finish
> 
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>

Thanks! Queued onto module-next.

  Luis
  

On 2022/11/15 16:33, Zhen Lei wrote:
> Added test cases for basic functions and performance of functions
> kallsyms_lookup_name(), kallsyms_on_each_symbol() and
> kallsyms_on_each_match_symbol(). It also calculates the compression rate
> of the kallsyms compression algorithm for the current symbol set.
> 
> The basic functions test begins by testing a set of symbols whose address
> values are known. Then, traverse all symbol addresses and find the
> corresponding symbol name based on the address. It's impossible to
> determine whether these addresses are correct, but we can use the above
> three functions along with the addresses to test each other. Due to the
> traversal operation of kallsyms_on_each_symbol() is too slow, only 60
> symbols can be tested in one second, so let it test on average once
> every 128 symbols. The other two functions validate all symbols.
> 
> If the basic functions test is passed, print only performance test
> results. If the test fails, print error information, but do not perform
> subsequent performance tests.
> 
> Start self-test automatically after system startup if
> CONFIG_KALLSYMS_SELFTEST=y.
> 
> Example of output content: (prefix 'kallsyms_selftest:' is omitted
>  start
>   ---------------------------------------------------------
>  | nr_symbols | compressed size | original size | ratio(%) |
>  |---------------------------------------------------------|
>  |     107543 |       1357912   |      2407433  |  56.40   |
>   ---------------------------------------------------------
>  kallsyms_lookup_name() looked up 107543 symbols
>  The time spent on each symbol is (ns): min=630, max=35295, avg=7353
>  kallsyms_on_each_symbol() traverse all: 11782628 ns
>  kallsyms_on_each_match_symbol() traverse all: 9261 ns
>  finish
> 
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>  include/linux/kallsyms.h   |   1 +
>  init/Kconfig               |  13 +
>  kernel/Makefile            |   1 +
>  kernel/kallsyms.c          |   2 +-
>  kernel/kallsyms_selftest.c | 485 +++++++++++++++++++++++++++++++++++++
>  kernel/kallsyms_selftest.h |  13 +
>  6 files changed, 514 insertions(+), 1 deletion(-)
>  create mode 100644 kernel/kallsyms_selftest.c
>  create mode 100644 kernel/kallsyms_selftest.h

v8 --> v9:
[v8] https://lkml.org/lkml/2022/11/2/225

Adjust the prototype of some callback functions. Because 7/9 and 8/9
in v8 are dropped.
  

Hi Zhen,

On Tue, Nov 15, 2022 at 9:41 AM Zhen Lei <thunder.leizhen@huawei.com> wrote:
> Added test cases for basic functions and performance of functions
> kallsyms_lookup_name(), kallsyms_on_each_symbol() and
> kallsyms_on_each_match_symbol(). It also calculates the compression rate
> of the kallsyms compression algorithm for the current symbol set.
>
> The basic functions test begins by testing a set of symbols whose address
> values are known. Then, traverse all symbol addresses and find the
> corresponding symbol name based on the address. It's impossible to
> determine whether these addresses are correct, but we can use the above
> three functions along with the addresses to test each other. Due to the
> traversal operation of kallsyms_on_each_symbol() is too slow, only 60
> symbols can be tested in one second, so let it test on average once
> every 128 symbols. The other two functions validate all symbols.
>
> If the basic functions test is passed, print only performance test
> results. If the test fails, print error information, but do not perform
> subsequent performance tests.
>
> Start self-test automatically after system startup if
> CONFIG_KALLSYMS_SELFTEST=y.
>
> Example of output content: (prefix 'kallsyms_selftest:' is omitted
>  start
>   ---------------------------------------------------------
>  | nr_symbols | compressed size | original size | ratio(%) |
>  |---------------------------------------------------------|
>  |     107543 |       1357912   |      2407433  |  56.40   |
>   ---------------------------------------------------------
>  kallsyms_lookup_name() looked up 107543 symbols
>  The time spent on each symbol is (ns): min=630, max=35295, avg=7353
>  kallsyms_on_each_symbol() traverse all: 11782628 ns
>  kallsyms_on_each_match_symbol() traverse all: 9261 ns
>  finish
>
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>

Thanks for your patch, which is now commit 30f3bb09778de64e ("kallsyms:
Add self-test facility") in linus/master.

I gave this a try on m68k (atari_defconfig + CONFIG_KALLSYMS_SELFTEST=y),
but it failed:

    start
    kallsyms_lookup_name() for kallsyms_test_func_static failed:
addr=0, expect 60ab0
    kallsyms_lookup_name() for kallsyms_test_func failed: addr=0, expect 60ac0
    kallsyms_lookup_name() for kallsyms_test_func_weak failed: addr=0,
expect 60ac2
    kallsyms_lookup_name() for vmalloc failed: addr=0, expect c272a
    kallsyms_lookup_name() for vfree failed: addr=0, expect c2142
    kallsyms_on_each_match_symbol() for kallsyms_test_func_static
failed: count=0, addr=0, expect 60ab0
    kallsyms_on_each_match_symbol() for kallsyms_test_func failed:
count=0, addr=0, expect 60ac0
    kallsyms_on_each_match_symbol() for kallsyms_test_func_weak
failed: count=0, addr=0, expect 60ac2
    kallsyms_on_each_match_symbol() for vmalloc failed: count=0,
addr=0, expect c272a
    kallsyms_on_each_match_symbol() for vfree failed: count=0, addr=0,
expect c2142
    abort

Given all addresses are zero, it looks like some required functionality
or config option is missing.

$ grep SYM .config
CONFIG_KALLSYMS=y
CONFIG_KALLSYMS_SELFTEST=y
CONFIG_KALLSYMS_BASE_RELATIVE=y
# CONFIG_ASYMMETRIC_KEY_TYPE is not set
CONFIG_SYMBOLIC_ERRNAME=y
# CONFIG_STRIP_ASM_SYMS is not set
CONFIG_KALLSYMS_SELFTEST

Do you have a clue?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
  

On 2022/12/15 16:50, Geert Uytterhoeven wrote:
> Hi Zhen,
> 
> On Tue, Nov 15, 2022 at 9:41 AM Zhen Lei <thunder.leizhen@huawei.com> wrote:
>> Added test cases for basic functions and performance of functions
>> kallsyms_lookup_name(), kallsyms_on_each_symbol() and
>> kallsyms_on_each_match_symbol(). It also calculates the compression rate
>> of the kallsyms compression algorithm for the current symbol set.
>>
>> The basic functions test begins by testing a set of symbols whose address
>> values are known. Then, traverse all symbol addresses and find the
>> corresponding symbol name based on the address. It's impossible to
>> determine whether these addresses are correct, but we can use the above
>> three functions along with the addresses to test each other. Due to the
>> traversal operation of kallsyms_on_each_symbol() is too slow, only 60
>> symbols can be tested in one second, so let it test on average once
>> every 128 symbols. The other two functions validate all symbols.
>>
>> If the basic functions test is passed, print only performance test
>> results. If the test fails, print error information, but do not perform
>> subsequent performance tests.
>>
>> Start self-test automatically after system startup if
>> CONFIG_KALLSYMS_SELFTEST=y.
>>
>> Example of output content: (prefix 'kallsyms_selftest:' is omitted
>>  start
>>   ---------------------------------------------------------
>>  | nr_symbols | compressed size | original size | ratio(%) |
>>  |---------------------------------------------------------|
>>  |     107543 |       1357912   |      2407433  |  56.40   |
>>   ---------------------------------------------------------
>>  kallsyms_lookup_name() looked up 107543 symbols
>>  The time spent on each symbol is (ns): min=630, max=35295, avg=7353
>>  kallsyms_on_each_symbol() traverse all: 11782628 ns
>>  kallsyms_on_each_match_symbol() traverse all: 9261 ns
>>  finish
>>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> 
> Thanks for your patch, which is now commit 30f3bb09778de64e ("kallsyms:
> Add self-test facility") in linus/master.
> 
> I gave this a try on m68k (atari_defconfig + CONFIG_KALLSYMS_SELFTEST=y),
> but it failed:
> 
>     start
>     kallsyms_lookup_name() for kallsyms_test_func_static failed:
> addr=0, expect 60ab0
>     kallsyms_lookup_name() for kallsyms_test_func failed: addr=0, expect 60ac0
>     kallsyms_lookup_name() for kallsyms_test_func_weak failed: addr=0,
> expect 60ac2
>     kallsyms_lookup_name() for vmalloc failed: addr=0, expect c272a
>     kallsyms_lookup_name() for vfree failed: addr=0, expect c2142
>     kallsyms_on_each_match_symbol() for kallsyms_test_func_static
> failed: count=0, addr=0, expect 60ab0
>     kallsyms_on_each_match_symbol() for kallsyms_test_func failed:
> count=0, addr=0, expect 60ac0
>     kallsyms_on_each_match_symbol() for kallsyms_test_func_weak
> failed: count=0, addr=0, expect 60ac2
>     kallsyms_on_each_match_symbol() for vmalloc failed: count=0,
> addr=0, expect c272a
>     kallsyms_on_each_match_symbol() for vfree failed: count=0, addr=0,
> expect c2142
>     abort
> 
> Given all addresses are zero, it looks like some required functionality
> or config option is missing.
> 
> $ grep SYM .config
> CONFIG_KALLSYMS=y
> CONFIG_KALLSYMS_SELFTEST=y
> CONFIG_KALLSYMS_BASE_RELATIVE=y
> # CONFIG_ASYMMETRIC_KEY_TYPE is not set
> CONFIG_SYMBOLIC_ERRNAME=y
> # CONFIG_STRIP_ASM_SYMS is not set
> CONFIG_KALLSYMS_SELFTEST
> 
> Do you have a clue?

cat /proc/kallsyms | grep kallsyms_test_func
Let's see if the compiler-generated symbols have some special suffixes.


> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 
> .
>
  

Hi Zhen,

On Thu, Dec 15, 2022 at 10:16 AM Leizhen (ThunderTown)
<thunder.leizhen@huawei.com> wrote:
> On 2022/12/15 16:50, Geert Uytterhoeven wrote:
> > On Tue, Nov 15, 2022 at 9:41 AM Zhen Lei <thunder.leizhen@huawei.com> wrote:
> >> Added test cases for basic functions and performance of functions
> >> kallsyms_lookup_name(), kallsyms_on_each_symbol() and
> >> kallsyms_on_each_match_symbol(). It also calculates the compression rate
> >> of the kallsyms compression algorithm for the current symbol set.
> >>
> >> The basic functions test begins by testing a set of symbols whose address
> >> values are known. Then, traverse all symbol addresses and find the
> >> corresponding symbol name based on the address. It's impossible to
> >> determine whether these addresses are correct, but we can use the above
> >> three functions along with the addresses to test each other. Due to the
> >> traversal operation of kallsyms_on_each_symbol() is too slow, only 60
> >> symbols can be tested in one second, so let it test on average once
> >> every 128 symbols. The other two functions validate all symbols.
> >>
> >> If the basic functions test is passed, print only performance test
> >> results. If the test fails, print error information, but do not perform
> >> subsequent performance tests.
> >>
> >> Start self-test automatically after system startup if
> >> CONFIG_KALLSYMS_SELFTEST=y.
> >>
> >> Example of output content: (prefix 'kallsyms_selftest:' is omitted
> >>  start
> >>   ---------------------------------------------------------
> >>  | nr_symbols | compressed size | original size | ratio(%) |
> >>  |---------------------------------------------------------|
> >>  |     107543 |       1357912   |      2407433  |  56.40   |
> >>   ---------------------------------------------------------
> >>  kallsyms_lookup_name() looked up 107543 symbols
> >>  The time spent on each symbol is (ns): min=630, max=35295, avg=7353
> >>  kallsyms_on_each_symbol() traverse all: 11782628 ns
> >>  kallsyms_on_each_match_symbol() traverse all: 9261 ns
> >>  finish
> >>
> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> >
> > Thanks for your patch, which is now commit 30f3bb09778de64e ("kallsyms:
> > Add self-test facility") in linus/master.
> >
> > I gave this a try on m68k (atari_defconfig + CONFIG_KALLSYMS_SELFTEST=y),
> > but it failed:
> >
> >     start
> >     kallsyms_lookup_name() for kallsyms_test_func_static failed:
> > addr=0, expect 60ab0
> >     kallsyms_lookup_name() for kallsyms_test_func failed: addr=0, expect 60ac0
> >     kallsyms_lookup_name() for kallsyms_test_func_weak failed: addr=0,
> > expect 60ac2
> >     kallsyms_lookup_name() for vmalloc failed: addr=0, expect c272a
> >     kallsyms_lookup_name() for vfree failed: addr=0, expect c2142
> >     kallsyms_on_each_match_symbol() for kallsyms_test_func_static
> > failed: count=0, addr=0, expect 60ab0
> >     kallsyms_on_each_match_symbol() for kallsyms_test_func failed:
> > count=0, addr=0, expect 60ac0
> >     kallsyms_on_each_match_symbol() for kallsyms_test_func_weak
> > failed: count=0, addr=0, expect 60ac2
> >     kallsyms_on_each_match_symbol() for vmalloc failed: count=0,
> > addr=0, expect c272a
> >     kallsyms_on_each_match_symbol() for vfree failed: count=0, addr=0,
> > expect c2142
> >     abort
> >
> > Given all addresses are zero, it looks like some required functionality
> > or config option is missing.
> >
> > $ grep SYM .config
> > CONFIG_KALLSYMS=y
> > CONFIG_KALLSYMS_SELFTEST=y
> > CONFIG_KALLSYMS_BASE_RELATIVE=y
> > # CONFIG_ASYMMETRIC_KEY_TYPE is not set
> > CONFIG_SYMBOLIC_ERRNAME=y
> > # CONFIG_STRIP_ASM_SYMS is not set
> > CONFIG_KALLSYMS_SELFTEST
> >
> > Do you have a clue?
>
> cat /proc/kallsyms | grep kallsyms_test_func
> Let's see if the compiler-generated symbols have some special suffixes.

Thanks, looks normal to me:

    atari:~# cat /proc/kallsyms | grep kallsyms_test_func
    00060ab0 t kallsyms_test_func_static
    00060ac0 T kallsyms_test_func
    00060ac2 W kallsyms_test_func_weak
    atari:~#

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
  

On 2022/12/15 17:39, Geert Uytterhoeven wrote:
> Hi Zhen,
> 
> On Thu, Dec 15, 2022 at 10:16 AM Leizhen (ThunderTown)
> <thunder.leizhen@huawei.com> wrote:
>> On 2022/12/15 16:50, Geert Uytterhoeven wrote:
>>> On Tue, Nov 15, 2022 at 9:41 AM Zhen Lei <thunder.leizhen@huawei.com> wrote:
>>>> Added test cases for basic functions and performance of functions
>>>> kallsyms_lookup_name(), kallsyms_on_each_symbol() and
>>>> kallsyms_on_each_match_symbol(). It also calculates the compression rate
>>>> of the kallsyms compression algorithm for the current symbol set.
>>>>
>>>> The basic functions test begins by testing a set of symbols whose address
>>>> values are known. Then, traverse all symbol addresses and find the
>>>> corresponding symbol name based on the address. It's impossible to
>>>> determine whether these addresses are correct, but we can use the above
>>>> three functions along with the addresses to test each other. Due to the
>>>> traversal operation of kallsyms_on_each_symbol() is too slow, only 60
>>>> symbols can be tested in one second, so let it test on average once
>>>> every 128 symbols. The other two functions validate all symbols.
>>>>
>>>> If the basic functions test is passed, print only performance test
>>>> results. If the test fails, print error information, but do not perform
>>>> subsequent performance tests.
>>>>
>>>> Start self-test automatically after system startup if
>>>> CONFIG_KALLSYMS_SELFTEST=y.
>>>>
>>>> Example of output content: (prefix 'kallsyms_selftest:' is omitted
>>>>  start
>>>>   ---------------------------------------------------------
>>>>  | nr_symbols | compressed size | original size | ratio(%) |
>>>>  |---------------------------------------------------------|
>>>>  |     107543 |       1357912   |      2407433  |  56.40   |
>>>>   ---------------------------------------------------------
>>>>  kallsyms_lookup_name() looked up 107543 symbols
>>>>  The time spent on each symbol is (ns): min=630, max=35295, avg=7353
>>>>  kallsyms_on_each_symbol() traverse all: 11782628 ns
>>>>  kallsyms_on_each_match_symbol() traverse all: 9261 ns
>>>>  finish
>>>>
>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>
>>> Thanks for your patch, which is now commit 30f3bb09778de64e ("kallsyms:
>>> Add self-test facility") in linus/master.
>>>
>>> I gave this a try on m68k (atari_defconfig + CONFIG_KALLSYMS_SELFTEST=y),
>>> but it failed:
>>>
>>>     start
>>>     kallsyms_lookup_name() for kallsyms_test_func_static failed:
>>> addr=0, expect 60ab0
>>>     kallsyms_lookup_name() for kallsyms_test_func failed: addr=0, expect 60ac0
>>>     kallsyms_lookup_name() for kallsyms_test_func_weak failed: addr=0,
>>> expect 60ac2
>>>     kallsyms_lookup_name() for vmalloc failed: addr=0, expect c272a
>>>     kallsyms_lookup_name() for vfree failed: addr=0, expect c2142
>>>     kallsyms_on_each_match_symbol() for kallsyms_test_func_static
>>> failed: count=0, addr=0, expect 60ab0
>>>     kallsyms_on_each_match_symbol() for kallsyms_test_func failed:
>>> count=0, addr=0, expect 60ac0
>>>     kallsyms_on_each_match_symbol() for kallsyms_test_func_weak
>>> failed: count=0, addr=0, expect 60ac2
>>>     kallsyms_on_each_match_symbol() for vmalloc failed: count=0,
>>> addr=0, expect c272a
>>>     kallsyms_on_each_match_symbol() for vfree failed: count=0, addr=0,
>>> expect c2142
>>>     abort
>>>
>>> Given all addresses are zero, it looks like some required functionality
>>> or config option is missing.
>>>
>>> $ grep SYM .config
>>> CONFIG_KALLSYMS=y
>>> CONFIG_KALLSYMS_SELFTEST=y
>>> CONFIG_KALLSYMS_BASE_RELATIVE=y
>>> # CONFIG_ASYMMETRIC_KEY_TYPE is not set
>>> CONFIG_SYMBOLIC_ERRNAME=y
>>> # CONFIG_STRIP_ASM_SYMS is not set
>>> CONFIG_KALLSYMS_SELFTEST
>>>
>>> Do you have a clue?
>>
>> cat /proc/kallsyms | grep kallsyms_test_func
>> Let's see if the compiler-generated symbols have some special suffixes.
> 
> Thanks, looks normal to me:
> 
>     atari:~# cat /proc/kallsyms | grep kallsyms_test_func
>     00060ab0 t kallsyms_test_func_static
>     00060ac0 T kallsyms_test_func
>     00060ac2 W kallsyms_test_func_weak
>     atari:~#

It's incredible. I don't have a m68k environment and I'm trying to build a qemu
environment. If you're in a hurry and willing, you can apply the debugging patch
in the attachment. I'd like to see what's wrong. Use "dmesg | grep tst" to collect
the output information.

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 
> .
>
  

Hi Zhen,

CC Jason

On Thu, Dec 15, 2022 at 1:34 PM Leizhen (ThunderTown)
<thunder.leizhen@huawei.com> wrote:
> On 2022/12/15 17:39, Geert Uytterhoeven wrote:
> > On Thu, Dec 15, 2022 at 10:16 AM Leizhen (ThunderTown)
> > <thunder.leizhen@huawei.com> wrote:
> >> On 2022/12/15 16:50, Geert Uytterhoeven wrote:
> >>> On Tue, Nov 15, 2022 at 9:41 AM Zhen Lei <thunder.leizhen@huawei.com> wrote:
> >>>> Added test cases for basic functions and performance of functions
> >>>> kallsyms_lookup_name(), kallsyms_on_each_symbol() and
> >>>> kallsyms_on_each_match_symbol(). It also calculates the compression rate
> >>>> of the kallsyms compression algorithm for the current symbol set.
> >>>>
> >>>> The basic functions test begins by testing a set of symbols whose address
> >>>> values are known. Then, traverse all symbol addresses and find the
> >>>> corresponding symbol name based on the address. It's impossible to
> >>>> determine whether these addresses are correct, but we can use the above
> >>>> three functions along with the addresses to test each other. Due to the
> >>>> traversal operation of kallsyms_on_each_symbol() is too slow, only 60
> >>>> symbols can be tested in one second, so let it test on average once
> >>>> every 128 symbols. The other two functions validate all symbols.
> >>>>
> >>>> If the basic functions test is passed, print only performance test
> >>>> results. If the test fails, print error information, but do not perform
> >>>> subsequent performance tests.
> >>>>
> >>>> Start self-test automatically after system startup if
> >>>> CONFIG_KALLSYMS_SELFTEST=y.
> >>>>
> >>>> Example of output content: (prefix 'kallsyms_selftest:' is omitted
> >>>>  start
> >>>>   ---------------------------------------------------------
> >>>>  | nr_symbols | compressed size | original size | ratio(%) |
> >>>>  |---------------------------------------------------------|
> >>>>  |     107543 |       1357912   |      2407433  |  56.40   |
> >>>>   ---------------------------------------------------------
> >>>>  kallsyms_lookup_name() looked up 107543 symbols
> >>>>  The time spent on each symbol is (ns): min=630, max=35295, avg=7353
> >>>>  kallsyms_on_each_symbol() traverse all: 11782628 ns
> >>>>  kallsyms_on_each_match_symbol() traverse all: 9261 ns
> >>>>  finish
> >>>>
> >>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> >>>
> >>> Thanks for your patch, which is now commit 30f3bb09778de64e ("kallsyms:
> >>> Add self-test facility") in linus/master.
> >>>
> >>> I gave this a try on m68k (atari_defconfig + CONFIG_KALLSYMS_SELFTEST=y),
> >>> but it failed:
> >>>
> >>>     start
> >>>     kallsyms_lookup_name() for kallsyms_test_func_static failed:
> >>> addr=0, expect 60ab0
> >>>     kallsyms_lookup_name() for kallsyms_test_func failed: addr=0, expect 60ac0
> >>>     kallsyms_lookup_name() for kallsyms_test_func_weak failed: addr=0,
> >>> expect 60ac2
> >>>     kallsyms_lookup_name() for vmalloc failed: addr=0, expect c272a
> >>>     kallsyms_lookup_name() for vfree failed: addr=0, expect c2142
> >>>     kallsyms_on_each_match_symbol() for kallsyms_test_func_static
> >>> failed: count=0, addr=0, expect 60ab0
> >>>     kallsyms_on_each_match_symbol() for kallsyms_test_func failed:
> >>> count=0, addr=0, expect 60ac0
> >>>     kallsyms_on_each_match_symbol() for kallsyms_test_func_weak
> >>> failed: count=0, addr=0, expect 60ac2
> >>>     kallsyms_on_each_match_symbol() for vmalloc failed: count=0,
> >>> addr=0, expect c272a
> >>>     kallsyms_on_each_match_symbol() for vfree failed: count=0, addr=0,
> >>> expect c2142
> >>>     abort
> >>>
> >>> Given all addresses are zero, it looks like some required functionality
> >>> or config option is missing.
> >>>
> >>> $ grep SYM .config
> >>> CONFIG_KALLSYMS=y
> >>> CONFIG_KALLSYMS_SELFTEST=y
> >>> CONFIG_KALLSYMS_BASE_RELATIVE=y
> >>> # CONFIG_ASYMMETRIC_KEY_TYPE is not set
> >>> CONFIG_SYMBOLIC_ERRNAME=y
> >>> # CONFIG_STRIP_ASM_SYMS is not set
> >>> CONFIG_KALLSYMS_SELFTEST
> >>>
> >>> Do you have a clue?
> >>
> >> cat /proc/kallsyms | grep kallsyms_test_func
> >> Let's see if the compiler-generated symbols have some special suffixes.
> >
> > Thanks, looks normal to me:
> >
> >     atari:~# cat /proc/kallsyms | grep kallsyms_test_func
> >     00060ab0 t kallsyms_test_func_static
> >     00060ac0 T kallsyms_test_func
> >     00060ac2 W kallsyms_test_func_weak
> >     atari:~#
>
> It's incredible. I don't have a m68k environment and I'm trying to build a qemu
> environment. If you're in a hurry and willing, you can apply the debugging patch
> in the attachment. I'd like to see what's wrong. Use "dmesg | grep tst" to collect
> the output information.

Still fails:

    tst: found kallsyms_test_func at index=12845
    tst: [12533] = kallsyms_test_func, seq=17370, offset=191223
    tst: [18800] = kallsyms_test_func, seq=23193, offset=259263
    tst: [21934] = kallsyms_test_func, seq=2527, offset=22331
    tst: [23501] = kallsyms_test_func, seq=11792, offset=126366
    tst: [24284] = kallsyms_test_func, seq=8427, offset=87395
    tst: [24676] = kallsyms_test_func, seq=21896, offset=243536
    tst: [24872] = kallsyms_test_func, seq=22571, offset=251856
    tst: [24970] = kallsyms_test_func, seq=23264, offset=260074
    tst: [25019] = kallsyms_test_func, seq=9003, offset=93752
    tst: [25043] = kallsyms_test_func, seq=14324, offset=155117
    tst: [25055] = kallsyms_test_func, seq=5942, offset=62266
    tst: [25061] = kallsyms_test_func, seq=14347, offset=155467
    tst: [25064] = kallsyms_test_func, seq=14350, offset=155512
    tst: [25066] = kallsyms_test_func, seq=14346, offset=155457
    tst: [25067] = kallsyms_test_func, seq=14354, offset=155565

However, the binary search sequence looks very suspicious.
After investigation, it turns out compare_symbol_name() and strcmp()
always return positive numbers.

Looks like commit 3bc753c06dd02a35 ("kbuild: treat char as always
unsigned") is to blame.

Changing:

    --- a/arch/m68k/include/asm/string.h
    +++ b/arch/m68k/include/asm/string.h
    @@ -42,7 +42,7 @@ static inline char *strncpy(char *dest, const
char *src, size_t n)
     #define __HAVE_ARCH_STRCMP
     static inline int strcmp(const char *cs, const char *ct)
     {
    -       char res;
    +       signed char res;

            asm ("\n"
                    "1:     move.b  (%0)+,%2\n"     /* get *cs */

fixes strcmp, but the test still fails:

    tst: kallsyms_lookup_names() is OK, name=kallsyms_test_func, i=0
    kallsyms_selftest: kallsyms_lookup_name() for
kallsyms_test_func_static failed: addr=8e1c, expect 60cd4
    kallsyms_selftest: kallsyms_lookup_name() for kallsyms_test_func
failed: addr=8e1c, expect 60ce4
    kallsyms_selftest: kallsyms_lookup_name() for
kallsyms_test_func_weak failed: addr=8e1c, expect 60ce6
    kallsyms_selftest: kallsyms_lookup_name() for vmalloc failed:
addr=8e1c, expect c2946
    kallsyms_selftest: kallsyms_lookup_name() for vfree failed:
addr=8e1c, expect c235e
    kallsyms_selftest: kallsyms_on_each_match_symbol() for
kallsyms_test_func_static failed: count=25068, addr=1f3d3c, expect
60cd4
    kallsyms_selftest: kallsyms_on_each_match_symbol() for
kallsyms_test_func failed: count=25068, addr=1f3d3c, expect 60ce4
    kallsyms_selftest: kallsyms_on_each_match_symbol() for
kallsyms_test_func_weak failed: count=25068, addr=1f3d3c, expect 60ce6
    kallsyms_selftest: kallsyms_on_each_match_symbol() for vmalloc
failed: count=25068, addr=1f3d3c, expect c2946
    kallsyms_selftest: kallsyms_on_each_match_symbol() for vfree
failed: count=25068, addr=1f3d3c, expect c235e
    kallsyms_selftest: abort

Dropping -funsigned-char doesn't improve upon that...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
  

On 2022/12/15 21:24, Geert Uytterhoeven wrote:
> Hi Zhen,
> 
> CC Jason
> 
> On Thu, Dec 15, 2022 at 1:34 PM Leizhen (ThunderTown)
> <thunder.leizhen@huawei.com> wrote:
>> On 2022/12/15 17:39, Geert Uytterhoeven wrote:
>>> On Thu, Dec 15, 2022 at 10:16 AM Leizhen (ThunderTown)
>>> <thunder.leizhen@huawei.com> wrote:
>>>> On 2022/12/15 16:50, Geert Uytterhoeven wrote:
>>>>> On Tue, Nov 15, 2022 at 9:41 AM Zhen Lei <thunder.leizhen@huawei.com> wrote:
>>>>>> Added test cases for basic functions and performance of functions
>>>>>> kallsyms_lookup_name(), kallsyms_on_each_symbol() and
>>>>>> kallsyms_on_each_match_symbol(). It also calculates the compression rate
>>>>>> of the kallsyms compression algorithm for the current symbol set.
>>>>>>
>>>>>> The basic functions test begins by testing a set of symbols whose address
>>>>>> values are known. Then, traverse all symbol addresses and find the
>>>>>> corresponding symbol name based on the address. It's impossible to
>>>>>> determine whether these addresses are correct, but we can use the above
>>>>>> three functions along with the addresses to test each other. Due to the
>>>>>> traversal operation of kallsyms_on_each_symbol() is too slow, only 60
>>>>>> symbols can be tested in one second, so let it test on average once
>>>>>> every 128 symbols. The other two functions validate all symbols.
>>>>>>
>>>>>> If the basic functions test is passed, print only performance test
>>>>>> results. If the test fails, print error information, but do not perform
>>>>>> subsequent performance tests.
>>>>>>
>>>>>> Start self-test automatically after system startup if
>>>>>> CONFIG_KALLSYMS_SELFTEST=y.
>>>>>>
>>>>>> Example of output content: (prefix 'kallsyms_selftest:' is omitted
>>>>>>  start
>>>>>>   ---------------------------------------------------------
>>>>>>  | nr_symbols | compressed size | original size | ratio(%) |
>>>>>>  |---------------------------------------------------------|
>>>>>>  |     107543 |       1357912   |      2407433  |  56.40   |
>>>>>>   ---------------------------------------------------------
>>>>>>  kallsyms_lookup_name() looked up 107543 symbols
>>>>>>  The time spent on each symbol is (ns): min=630, max=35295, avg=7353
>>>>>>  kallsyms_on_each_symbol() traverse all: 11782628 ns
>>>>>>  kallsyms_on_each_match_symbol() traverse all: 9261 ns
>>>>>>  finish
>>>>>>
>>>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>>>
>>>>> Thanks for your patch, which is now commit 30f3bb09778de64e ("kallsyms:
>>>>> Add self-test facility") in linus/master.
>>>>>
>>>>> I gave this a try on m68k (atari_defconfig + CONFIG_KALLSYMS_SELFTEST=y),
>>>>> but it failed:
>>>>>
>>>>>     start
>>>>>     kallsyms_lookup_name() for kallsyms_test_func_static failed:
>>>>> addr=0, expect 60ab0
>>>>>     kallsyms_lookup_name() for kallsyms_test_func failed: addr=0, expect 60ac0
>>>>>     kallsyms_lookup_name() for kallsyms_test_func_weak failed: addr=0,
>>>>> expect 60ac2
>>>>>     kallsyms_lookup_name() for vmalloc failed: addr=0, expect c272a
>>>>>     kallsyms_lookup_name() for vfree failed: addr=0, expect c2142
>>>>>     kallsyms_on_each_match_symbol() for kallsyms_test_func_static
>>>>> failed: count=0, addr=0, expect 60ab0
>>>>>     kallsyms_on_each_match_symbol() for kallsyms_test_func failed:
>>>>> count=0, addr=0, expect 60ac0
>>>>>     kallsyms_on_each_match_symbol() for kallsyms_test_func_weak
>>>>> failed: count=0, addr=0, expect 60ac2
>>>>>     kallsyms_on_each_match_symbol() for vmalloc failed: count=0,
>>>>> addr=0, expect c272a
>>>>>     kallsyms_on_each_match_symbol() for vfree failed: count=0, addr=0,
>>>>> expect c2142
>>>>>     abort
>>>>>
>>>>> Given all addresses are zero, it looks like some required functionality
>>>>> or config option is missing.
>>>>>
>>>>> $ grep SYM .config
>>>>> CONFIG_KALLSYMS=y
>>>>> CONFIG_KALLSYMS_SELFTEST=y
>>>>> CONFIG_KALLSYMS_BASE_RELATIVE=y
>>>>> # CONFIG_ASYMMETRIC_KEY_TYPE is not set
>>>>> CONFIG_SYMBOLIC_ERRNAME=y
>>>>> # CONFIG_STRIP_ASM_SYMS is not set
>>>>> CONFIG_KALLSYMS_SELFTEST
>>>>>
>>>>> Do you have a clue?
>>>>
>>>> cat /proc/kallsyms | grep kallsyms_test_func
>>>> Let's see if the compiler-generated symbols have some special suffixes.
>>>
>>> Thanks, looks normal to me:
>>>
>>>     atari:~# cat /proc/kallsyms | grep kallsyms_test_func
>>>     00060ab0 t kallsyms_test_func_static
>>>     00060ac0 T kallsyms_test_func
>>>     00060ac2 W kallsyms_test_func_weak
>>>     atari:~#
>>
>> It's incredible. I don't have a m68k environment and I'm trying to build a qemu
>> environment. If you're in a hurry and willing, you can apply the debugging patch
>> in the attachment. I'd like to see what's wrong. Use "dmesg | grep tst" to collect
>> the output information.
> 
> Still fails:
> 
>     tst: found kallsyms_test_func at index=12845
>     tst: [12533] = kallsyms_test_func, seq=17370, offset=191223
>     tst: [18800] = kallsyms_test_func, seq=23193, offset=259263
>     tst: [21934] = kallsyms_test_func, seq=2527, offset=22331
>     tst: [23501] = kallsyms_test_func, seq=11792, offset=126366
>     tst: [24284] = kallsyms_test_func, seq=8427, offset=87395
>     tst: [24676] = kallsyms_test_func, seq=21896, offset=243536
>     tst: [24872] = kallsyms_test_func, seq=22571, offset=251856
>     tst: [24970] = kallsyms_test_func, seq=23264, offset=260074
>     tst: [25019] = kallsyms_test_func, seq=9003, offset=93752
>     tst: [25043] = kallsyms_test_func, seq=14324, offset=155117
>     tst: [25055] = kallsyms_test_func, seq=5942, offset=62266
>     tst: [25061] = kallsyms_test_func, seq=14347, offset=155467
>     tst: [25064] = kallsyms_test_func, seq=14350, offset=155512
>     tst: [25066] = kallsyms_test_func, seq=14346, offset=155457
>     tst: [25067] = kallsyms_test_func, seq=14354, offset=155565

-               pr_warn("tst: [%d] = %s, seq=%d, offset=%d\n", mid, name, seq, off);
+               pr_warn("tst: [%d] = %s, seq=%d, offset=%d\n", mid, namebuf, seq, off);

Sorry, a variable in the debugging code is incorrectly written. 'name' should
be replaced with 'namebuf', then we can see which function the comparison is wrong.

> 
> However, the binary search sequence looks very suspicious.
> After investigation, it turns out compare_symbol_name() and strcmp()
> always return positive numbers.
> 
> Looks like commit 3bc753c06dd02a35 ("kbuild: treat char as always
> unsigned") is to blame.

Oh, maybe you can "git reset --hard 30f3bb09778de64" and try again.
30f3bb09778de64 kallsyms: Add self-test facility

But the latest kernel is OK on x86. So other patches are unlikely to
affect this function.

Is m68k big-endian?

> 
> Changing:
> 
>     --- a/arch/m68k/include/asm/string.h
>     +++ b/arch/m68k/include/asm/string.h
>     @@ -42,7 +42,7 @@ static inline char *strncpy(char *dest, const
> char *src, size_t n)
>      #define __HAVE_ARCH_STRCMP
>      static inline int strcmp(const char *cs, const char *ct)
>      {
>     -       char res;
>     +       signed char res;
> 
>             asm ("\n"
>                     "1:     move.b  (%0)+,%2\n"     /* get *cs */
> 
> fixes strcmp, but the test still fails:
> 
>     tst: kallsyms_lookup_names() is OK, name=kallsyms_test_func, i=0

i=0, i is impossible zero. Maybe kallsyms_lookup_names() always return success now.

>     kallsyms_selftest: kallsyms_lookup_name() for
> kallsyms_test_func_static failed: addr=8e1c, expect 60cd4
>     kallsyms_selftest: kallsyms_lookup_name() for kallsyms_test_func
> failed: addr=8e1c, expect 60ce4
>     kallsyms_selftest: kallsyms_lookup_name() for
> kallsyms_test_func_weak failed: addr=8e1c, expect 60ce6
>     kallsyms_selftest: kallsyms_lookup_name() for vmalloc failed:
> addr=8e1c, expect c2946
>     kallsyms_selftest: kallsyms_lookup_name() for vfree failed:
> addr=8e1c, expect c235e
>     kallsyms_selftest: kallsyms_on_each_match_symbol() for
> kallsyms_test_func_static failed: count=25068, addr=1f3d3c, expect
> 60cd4
>     kallsyms_selftest: kallsyms_on_each_match_symbol() for
> kallsyms_test_func failed: count=25068, addr=1f3d3c, expect 60ce4
>     kallsyms_selftest: kallsyms_on_each_match_symbol() for
> kallsyms_test_func_weak failed: count=25068, addr=1f3d3c, expect 60ce6
>     kallsyms_selftest: kallsyms_on_each_match_symbol() for vmalloc
> failed: count=25068, addr=1f3d3c, expect c2946
>     kallsyms_selftest: kallsyms_on_each_match_symbol() for vfree
> failed: count=25068, addr=1f3d3c, expect c235e
>     kallsyms_selftest: abort
> 
> Dropping -funsigned-char doesn't improve upon that...
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 
> .
>
  

On 2022/12/15 21:58, Leizhen (ThunderTown) wrote:
> 
> 
> On 2022/12/15 21:24, Geert Uytterhoeven wrote:
>> Hi Zhen,
>>
>> CC Jason
>>
>> On Thu, Dec 15, 2022 at 1:34 PM Leizhen (ThunderTown)
>> <thunder.leizhen@huawei.com> wrote:
>>> On 2022/12/15 17:39, Geert Uytterhoeven wrote:
>>>> On Thu, Dec 15, 2022 at 10:16 AM Leizhen (ThunderTown)
>>>> <thunder.leizhen@huawei.com> wrote:
>>>>> On 2022/12/15 16:50, Geert Uytterhoeven wrote:
>>>>>> On Tue, Nov 15, 2022 at 9:41 AM Zhen Lei <thunder.leizhen@huawei.com> wrote:
>>>>>>> Added test cases for basic functions and performance of functions
>>>>>>> kallsyms_lookup_name(), kallsyms_on_each_symbol() and
>>>>>>> kallsyms_on_each_match_symbol(). It also calculates the compression rate
>>>>>>> of the kallsyms compression algorithm for the current symbol set.
>>>>>>>
>>>>>>> The basic functions test begins by testing a set of symbols whose address
>>>>>>> values are known. Then, traverse all symbol addresses and find the
>>>>>>> corresponding symbol name based on the address. It's impossible to
>>>>>>> determine whether these addresses are correct, but we can use the above
>>>>>>> three functions along with the addresses to test each other. Due to the
>>>>>>> traversal operation of kallsyms_on_each_symbol() is too slow, only 60
>>>>>>> symbols can be tested in one second, so let it test on average once
>>>>>>> every 128 symbols. The other two functions validate all symbols.
>>>>>>>
>>>>>>> If the basic functions test is passed, print only performance test
>>>>>>> results. If the test fails, print error information, but do not perform
>>>>>>> subsequent performance tests.
>>>>>>>
>>>>>>> Start self-test automatically after system startup if
>>>>>>> CONFIG_KALLSYMS_SELFTEST=y.
>>>>>>>
>>>>>>> Example of output content: (prefix 'kallsyms_selftest:' is omitted
>>>>>>>  start
>>>>>>>   ---------------------------------------------------------
>>>>>>>  | nr_symbols | compressed size | original size | ratio(%) |
>>>>>>>  |---------------------------------------------------------|
>>>>>>>  |     107543 |       1357912   |      2407433  |  56.40   |
>>>>>>>   ---------------------------------------------------------
>>>>>>>  kallsyms_lookup_name() looked up 107543 symbols
>>>>>>>  The time spent on each symbol is (ns): min=630, max=35295, avg=7353
>>>>>>>  kallsyms_on_each_symbol() traverse all: 11782628 ns
>>>>>>>  kallsyms_on_each_match_symbol() traverse all: 9261 ns
>>>>>>>  finish
>>>>>>>
>>>>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>>>>
>>>>>> Thanks for your patch, which is now commit 30f3bb09778de64e ("kallsyms:
>>>>>> Add self-test facility") in linus/master.
>>>>>>
>>>>>> I gave this a try on m68k (atari_defconfig + CONFIG_KALLSYMS_SELFTEST=y),
>>>>>> but it failed:
>>>>>>
>>>>>>     start
>>>>>>     kallsyms_lookup_name() for kallsyms_test_func_static failed:
>>>>>> addr=0, expect 60ab0
>>>>>>     kallsyms_lookup_name() for kallsyms_test_func failed: addr=0, expect 60ac0
>>>>>>     kallsyms_lookup_name() for kallsyms_test_func_weak failed: addr=0,
>>>>>> expect 60ac2
>>>>>>     kallsyms_lookup_name() for vmalloc failed: addr=0, expect c272a
>>>>>>     kallsyms_lookup_name() for vfree failed: addr=0, expect c2142
>>>>>>     kallsyms_on_each_match_symbol() for kallsyms_test_func_static
>>>>>> failed: count=0, addr=0, expect 60ab0
>>>>>>     kallsyms_on_each_match_symbol() for kallsyms_test_func failed:
>>>>>> count=0, addr=0, expect 60ac0
>>>>>>     kallsyms_on_each_match_symbol() for kallsyms_test_func_weak
>>>>>> failed: count=0, addr=0, expect 60ac2
>>>>>>     kallsyms_on_each_match_symbol() for vmalloc failed: count=0,
>>>>>> addr=0, expect c272a
>>>>>>     kallsyms_on_each_match_symbol() for vfree failed: count=0, addr=0,
>>>>>> expect c2142
>>>>>>     abort
>>>>>>
>>>>>> Given all addresses are zero, it looks like some required functionality
>>>>>> or config option is missing.
>>>>>>
>>>>>> $ grep SYM .config
>>>>>> CONFIG_KALLSYMS=y
>>>>>> CONFIG_KALLSYMS_SELFTEST=y
>>>>>> CONFIG_KALLSYMS_BASE_RELATIVE=y
>>>>>> # CONFIG_ASYMMETRIC_KEY_TYPE is not set
>>>>>> CONFIG_SYMBOLIC_ERRNAME=y
>>>>>> # CONFIG_STRIP_ASM_SYMS is not set
>>>>>> CONFIG_KALLSYMS_SELFTEST
>>>>>>
>>>>>> Do you have a clue?
>>>>>
>>>>> cat /proc/kallsyms | grep kallsyms_test_func
>>>>> Let's see if the compiler-generated symbols have some special suffixes.
>>>>
>>>> Thanks, looks normal to me:
>>>>
>>>>     atari:~# cat /proc/kallsyms | grep kallsyms_test_func
>>>>     00060ab0 t kallsyms_test_func_static
>>>>     00060ac0 T kallsyms_test_func
>>>>     00060ac2 W kallsyms_test_func_weak
>>>>     atari:~#
>>>
>>> It's incredible. I don't have a m68k environment and I'm trying to build a qemu
>>> environment. If you're in a hurry and willing, you can apply the debugging patch
>>> in the attachment. I'd like to see what's wrong. Use "dmesg | grep tst" to collect
>>> the output information.
>>
>> Still fails:
>>
>>     tst: found kallsyms_test_func at index=12845
>>     tst: [12533] = kallsyms_test_func, seq=17370, offset=191223
>>     tst: [18800] = kallsyms_test_func, seq=23193, offset=259263
>>     tst: [21934] = kallsyms_test_func, seq=2527, offset=22331
>>     tst: [23501] = kallsyms_test_func, seq=11792, offset=126366
>>     tst: [24284] = kallsyms_test_func, seq=8427, offset=87395
>>     tst: [24676] = kallsyms_test_func, seq=21896, offset=243536
>>     tst: [24872] = kallsyms_test_func, seq=22571, offset=251856
>>     tst: [24970] = kallsyms_test_func, seq=23264, offset=260074
>>     tst: [25019] = kallsyms_test_func, seq=9003, offset=93752
>>     tst: [25043] = kallsyms_test_func, seq=14324, offset=155117
>>     tst: [25055] = kallsyms_test_func, seq=5942, offset=62266
>>     tst: [25061] = kallsyms_test_func, seq=14347, offset=155467
>>     tst: [25064] = kallsyms_test_func, seq=14350, offset=155512
>>     tst: [25066] = kallsyms_test_func, seq=14346, offset=155457
>>     tst: [25067] = kallsyms_test_func, seq=14354, offset=155565
> 
> -               pr_warn("tst: [%d] = %s, seq=%d, offset=%d\n", mid, name, seq, off);
> +               pr_warn("tst: [%d] = %s, seq=%d, offset=%d\n", mid, namebuf, seq, off);
> 
> Sorry, a variable in the debugging code is incorrectly written. 'name' should
> be replaced with 'namebuf', then we can see which function the comparison is wrong.

I attached debug patch v2.

> 
>>
>> However, the binary search sequence looks very suspicious.
>> After investigation, it turns out compare_symbol_name() and strcmp()
>> always return positive numbers.
>>
>> Looks like commit 3bc753c06dd02a35 ("kbuild: treat char as always
>> unsigned") is to blame.
> 
> Oh, maybe you can "git reset --hard 30f3bb09778de64" and try again.
> 30f3bb09778de64 kallsyms: Add self-test facility
> 
> But the latest kernel is OK on x86. So other patches are unlikely to
> affect this function.
> 
> Is m68k big-endian?
> 
>>
>> Changing:
>>
>>     --- a/arch/m68k/include/asm/string.h
>>     +++ b/arch/m68k/include/asm/string.h
>>     @@ -42,7 +42,7 @@ static inline char *strncpy(char *dest, const
>> char *src, size_t n)
>>      #define __HAVE_ARCH_STRCMP
>>      static inline int strcmp(const char *cs, const char *ct)
>>      {
>>     -       char res;
>>     +       signed char res;
>>
>>             asm ("\n"
>>                     "1:     move.b  (%0)+,%2\n"     /* get *cs */
>>
>> fixes strcmp, but the test still fails:
>>
>>     tst: kallsyms_lookup_names() is OK, name=kallsyms_test_func, i=0
> 
> i=0, i is impossible zero. Maybe kallsyms_lookup_names() always return success now.
> 
>>     kallsyms_selftest: kallsyms_lookup_name() for
>> kallsyms_test_func_static failed: addr=8e1c, expect 60cd4
>>     kallsyms_selftest: kallsyms_lookup_name() for kallsyms_test_func
>> failed: addr=8e1c, expect 60ce4
>>     kallsyms_selftest: kallsyms_lookup_name() for
>> kallsyms_test_func_weak failed: addr=8e1c, expect 60ce6
>>     kallsyms_selftest: kallsyms_lookup_name() for vmalloc failed:
>> addr=8e1c, expect c2946
>>     kallsyms_selftest: kallsyms_lookup_name() for vfree failed:
>> addr=8e1c, expect c235e
>>     kallsyms_selftest: kallsyms_on_each_match_symbol() for
>> kallsyms_test_func_static failed: count=25068, addr=1f3d3c, expect
>> 60cd4
>>     kallsyms_selftest: kallsyms_on_each_match_symbol() for
>> kallsyms_test_func failed: count=25068, addr=1f3d3c, expect 60ce4
>>     kallsyms_selftest: kallsyms_on_each_match_symbol() for
>> kallsyms_test_func_weak failed: count=25068, addr=1f3d3c, expect 60ce6
>>     kallsyms_selftest: kallsyms_on_each_match_symbol() for vmalloc
>> failed: count=25068, addr=1f3d3c, expect c2946
>>     kallsyms_selftest: kallsyms_on_each_match_symbol() for vfree
>> failed: count=25068, addr=1f3d3c, expect c235e
>>     kallsyms_selftest: abort
>>
>> Dropping -funsigned-char doesn't improve upon that...
>>
>> Gr{oetje,eeting}s,
>>
>>                         Geert
>>
>> --
>> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>>
>> In personal conversations with technical people, I call myself a hacker. But
>> when I'm talking to journalists I just say "programmer" or something like that.
>>                                 -- Linus Torvalds
>>
>> .
>>
>
  

Hi Zhen,

On Thu, Dec 15, 2022 at 2:58 PM Leizhen (ThunderTown)
<thunder.leizhen@huawei.com> wrote:
> On 2022/12/15 21:24, Geert Uytterhoeven wrote:
> > On Thu, Dec 15, 2022 at 1:34 PM Leizhen (ThunderTown)
> > <thunder.leizhen@huawei.com> wrote:
> >> On 2022/12/15 17:39, Geert Uytterhoeven wrote:
> >>> On Thu, Dec 15, 2022 at 10:16 AM Leizhen (ThunderTown)
> >>> <thunder.leizhen@huawei.com> wrote:
> >>>> On 2022/12/15 16:50, Geert Uytterhoeven wrote:
> >>>>> On Tue, Nov 15, 2022 at 9:41 AM Zhen Lei <thunder.leizhen@huawei.com> wrote:
> >>>>>> Added test cases for basic functions and performance of functions
> >>>>>> kallsyms_lookup_name(), kallsyms_on_each_symbol() and
> >>>>>> kallsyms_on_each_match_symbol(). It also calculates the compression rate
> >>>>>> of the kallsyms compression algorithm for the current symbol set.
> >>>>>>
> >>>>>> The basic functions test begins by testing a set of symbols whose address
> >>>>>> values are known. Then, traverse all symbol addresses and find the
> >>>>>> corresponding symbol name based on the address. It's impossible to
> >>>>>> determine whether these addresses are correct, but we can use the above
> >>>>>> three functions along with the addresses to test each other. Due to the
> >>>>>> traversal operation of kallsyms_on_each_symbol() is too slow, only 60
> >>>>>> symbols can be tested in one second, so let it test on average once
> >>>>>> every 128 symbols. The other two functions validate all symbols.
> >>>>>>
> >>>>>> If the basic functions test is passed, print only performance test
> >>>>>> results. If the test fails, print error information, but do not perform
> >>>>>> subsequent performance tests.
> >>>>>>
> >>>>>> Start self-test automatically after system startup if
> >>>>>> CONFIG_KALLSYMS_SELFTEST=y.
> >>>>>>
> >>>>>> Example of output content: (prefix 'kallsyms_selftest:' is omitted
> >>>>>>  start
> >>>>>>   ---------------------------------------------------------
> >>>>>>  | nr_symbols | compressed size | original size | ratio(%) |
> >>>>>>  |---------------------------------------------------------|
> >>>>>>  |     107543 |       1357912   |      2407433  |  56.40   |
> >>>>>>   ---------------------------------------------------------
> >>>>>>  kallsyms_lookup_name() looked up 107543 symbols
> >>>>>>  The time spent on each symbol is (ns): min=630, max=35295, avg=7353
> >>>>>>  kallsyms_on_each_symbol() traverse all: 11782628 ns
> >>>>>>  kallsyms_on_each_match_symbol() traverse all: 9261 ns
> >>>>>>  finish
> >>>>>>
> >>>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> >>>>>
> >>>>> Thanks for your patch, which is now commit 30f3bb09778de64e ("kallsyms:
> >>>>> Add self-test facility") in linus/master.
> >>>>>
> >>>>> I gave this a try on m68k (atari_defconfig + CONFIG_KALLSYMS_SELFTEST=y),
> >>>>> but it failed:
> >>>>>
> >>>>>     start
> >>>>>     kallsyms_lookup_name() for kallsyms_test_func_static failed:
> >>>>> addr=0, expect 60ab0
> >>>>>     kallsyms_lookup_name() for kallsyms_test_func failed: addr=0, expect 60ac0
> >>>>>     kallsyms_lookup_name() for kallsyms_test_func_weak failed: addr=0,
> >>>>> expect 60ac2
> >>>>>     kallsyms_lookup_name() for vmalloc failed: addr=0, expect c272a
> >>>>>     kallsyms_lookup_name() for vfree failed: addr=0, expect c2142
> >>>>>     kallsyms_on_each_match_symbol() for kallsyms_test_func_static
> >>>>> failed: count=0, addr=0, expect 60ab0
> >>>>>     kallsyms_on_each_match_symbol() for kallsyms_test_func failed:
> >>>>> count=0, addr=0, expect 60ac0
> >>>>>     kallsyms_on_each_match_symbol() for kallsyms_test_func_weak
> >>>>> failed: count=0, addr=0, expect 60ac2
> >>>>>     kallsyms_on_each_match_symbol() for vmalloc failed: count=0,
> >>>>> addr=0, expect c272a
> >>>>>     kallsyms_on_each_match_symbol() for vfree failed: count=0, addr=0,
> >>>>> expect c2142
> >>>>>     abort

> Oh, maybe you can "git reset --hard 30f3bb09778de64" and try again.
> 30f3bb09778de64 kallsyms: Add self-test facility
>
> But the latest kernel is OK on x86. So other patches are unlikely to
> affect this function.

On 30f3bb09778de64 it fails the same way:

    start
    kallsyms_lookup_name() for kallsyms_test_func_static failed:
addr=8dbc, expect 60308
    kallsyms_lookup_name() for kallsyms_test_func failed: addr=8dbc,
expect 60318
    kallsyms_lookup_name() for kallsyms_test_func_weak failed:
addr=8dbc, expect 6031a
    kallsyms_lookup_name() for vmalloc failed: addr=8dbc, expect bdc88
    kallsyms_lookup_name() for vfree failed: addr=8dbc, expect bd6de
    kallsyms_on_each_match_symbol() for kallsyms_test_func_static
failed: count=24890, addr=1ee870, expect 60308
    kallsyms_on_each_match_symbol() for kallsyms_test_func failed:
count=24890, addr=1ee870, expect 60318
    kallsyms_on_each_match_symbol() for kallsyms_test_func_weak
failed: count=24890, addr=1ee870, expect 6031a
    kallsyms_on_each_match_symbol() for vmalloc failed: count=24890,
addr=1ee870, expect bdc88
    kallsyms_on_each_match_symbol() for vfree failed: count=24890,
addr=1ee870, expect bd6de
    abort

> Is m68k big-endian?

Yes it is. And 32-bit values are aligned to only 2 bytes.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
  

On Dez 15 2022, Geert Uytterhoeven wrote:

>     start
>     kallsyms_lookup_name() for kallsyms_test_func_static failed:
> addr=8dbc, expect 60308
>     kallsyms_lookup_name() for kallsyms_test_func failed: addr=8dbc,
> expect 60318
>     kallsyms_lookup_name() for kallsyms_test_func_weak failed:
> addr=8dbc, expect 6031a
>     kallsyms_lookup_name() for vmalloc failed: addr=8dbc, expect bdc88
>     kallsyms_lookup_name() for vfree failed: addr=8dbc, expect bd6de

Curiously addr always comes out the same.  What symbol does it resolve
to?

>     kallsyms_on_each_match_symbol() for kallsyms_test_func_static
> failed: count=24890, addr=1ee870, expect 60308
>     kallsyms_on_each_match_symbol() for kallsyms_test_func failed:
> count=24890, addr=1ee870, expect 60318
>     kallsyms_on_each_match_symbol() for kallsyms_test_func_weak
> failed: count=24890, addr=1ee870, expect 6031a
>     kallsyms_on_each_match_symbol() for vmalloc failed: count=24890,
> addr=1ee870, expect bdc88
>     kallsyms_on_each_match_symbol() for vfree failed: count=24890,
> addr=1ee870, expect bd6de

Here too.
  

Hi Zhen,

On Thu, Dec 15, 2022 at 3:40 PM Leizhen (ThunderTown)
<thunder.leizhen@huawei.com> wrote:
> On 2022/12/15 21:58, Leizhen (ThunderTown) wrote:
> > On 2022/12/15 21:24, Geert Uytterhoeven wrote:
> >> On Thu, Dec 15, 2022 at 1:34 PM Leizhen (ThunderTown)
> >> <thunder.leizhen@huawei.com> wrote:
> >>> On 2022/12/15 17:39, Geert Uytterhoeven wrote:
> >>>> On Thu, Dec 15, 2022 at 10:16 AM Leizhen (ThunderTown)
> >>>> <thunder.leizhen@huawei.com> wrote:
> >>>>> On 2022/12/15 16:50, Geert Uytterhoeven wrote:
> >>>>>> On Tue, Nov 15, 2022 at 9:41 AM Zhen Lei <thunder.leizhen@huawei.com> wrote:
> >>>>>>> Added test cases for basic functions and performance of functions
> >>>>>>> kallsyms_lookup_name(), kallsyms_on_each_symbol() and
> >>>>>>> kallsyms_on_each_match_symbol(). It also calculates the compression rate
> >>>>>>> of the kallsyms compression algorithm for the current symbol set.
> >>>>>>>
> >>>>>>> The basic functions test begins by testing a set of symbols whose address
> >>>>>>> values are known. Then, traverse all symbol addresses and find the
> >>>>>>> corresponding symbol name based on the address. It's impossible to
> >>>>>>> determine whether these addresses are correct, but we can use the above
> >>>>>>> three functions along with the addresses to test each other. Due to the
> >>>>>>> traversal operation of kallsyms_on_each_symbol() is too slow, only 60
> >>>>>>> symbols can be tested in one second, so let it test on average once
> >>>>>>> every 128 symbols. The other two functions validate all symbols.
> >>>>>>>
> >>>>>>> If the basic functions test is passed, print only performance test
> >>>>>>> results. If the test fails, print error information, but do not perform
> >>>>>>> subsequent performance tests.
> >>>>>>>
> >>>>>>> Start self-test automatically after system startup if
> >>>>>>> CONFIG_KALLSYMS_SELFTEST=y.
> >>>>>>>
> >>>>>>> Example of output content: (prefix 'kallsyms_selftest:' is omitted
> >>>>>>>  start
> >>>>>>>   ---------------------------------------------------------
> >>>>>>>  | nr_symbols | compressed size | original size | ratio(%) |
> >>>>>>>  |---------------------------------------------------------|
> >>>>>>>  |     107543 |       1357912   |      2407433  |  56.40   |
> >>>>>>>   ---------------------------------------------------------
> >>>>>>>  kallsyms_lookup_name() looked up 107543 symbols
> >>>>>>>  The time spent on each symbol is (ns): min=630, max=35295, avg=7353
> >>>>>>>  kallsyms_on_each_symbol() traverse all: 11782628 ns
> >>>>>>>  kallsyms_on_each_match_symbol() traverse all: 9261 ns
> >>>>>>>  finish
> >>>>>>>
> >>>>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> >>>>>>
> >>>>>> Thanks for your patch, which is now commit 30f3bb09778de64e ("kallsyms:
> >>>>>> Add self-test facility") in linus/master.
> >>>>>>
> >>>>>> I gave this a try on m68k (atari_defconfig + CONFIG_KALLSYMS_SELFTEST=y),
> >>>>>> but it failed:
> >>>>>>
> >>>>>>     start
> >>>>>>     kallsyms_lookup_name() for kallsyms_test_func_static failed:
> >>>>>> addr=0, expect 60ab0
> >>>>>>     kallsyms_lookup_name() for kallsyms_test_func failed: addr=0, expect 60ac0
> >>>>>>     kallsyms_lookup_name() for kallsyms_test_func_weak failed: addr=0,
> >>>>>> expect 60ac2
> >>>>>>     kallsyms_lookup_name() for vmalloc failed: addr=0, expect c272a
> >>>>>>     kallsyms_lookup_name() for vfree failed: addr=0, expect c2142
> >>>>>>     kallsyms_on_each_match_symbol() for kallsyms_test_func_static
> >>>>>> failed: count=0, addr=0, expect 60ab0
> >>>>>>     kallsyms_on_each_match_symbol() for kallsyms_test_func failed:
> >>>>>> count=0, addr=0, expect 60ac0
> >>>>>>     kallsyms_on_each_match_symbol() for kallsyms_test_func_weak
> >>>>>> failed: count=0, addr=0, expect 60ac2
> >>>>>>     kallsyms_on_each_match_symbol() for vmalloc failed: count=0,
> >>>>>> addr=0, expect c272a
> >>>>>>     kallsyms_on_each_match_symbol() for vfree failed: count=0, addr=0,
> >>>>>> expect c2142
> >>>>>>     abort
> >>>>>>
> >>>>>> Given all addresses are zero, it looks like some required functionality
> >>>>>> or config option is missing.
> >>>>>>
> >>>>>> $ grep SYM .config
> >>>>>> CONFIG_KALLSYMS=y
> >>>>>> CONFIG_KALLSYMS_SELFTEST=y
> >>>>>> CONFIG_KALLSYMS_BASE_RELATIVE=y
> >>>>>> # CONFIG_ASYMMETRIC_KEY_TYPE is not set
> >>>>>> CONFIG_SYMBOLIC_ERRNAME=y
> >>>>>> # CONFIG_STRIP_ASM_SYMS is not set
> >>>>>> CONFIG_KALLSYMS_SELFTEST
> >>>>>>
> >>>>>> Do you have a clue?
> >>>>>
> >>>>> cat /proc/kallsyms | grep kallsyms_test_func
> >>>>> Let's see if the compiler-generated symbols have some special suffixes.
> >>>>
> >>>> Thanks, looks normal to me:
> >>>>
> >>>>     atari:~# cat /proc/kallsyms | grep kallsyms_test_func
> >>>>     00060ab0 t kallsyms_test_func_static
> >>>>     00060ac0 T kallsyms_test_func
> >>>>     00060ac2 W kallsyms_test_func_weak
> >>>>     atari:~#
> >>>
> >>> It's incredible. I don't have a m68k environment and I'm trying to build a qemu
> >>> environment. If you're in a hurry and willing, you can apply the debugging patch
> >>> in the attachment. I'd like to see what's wrong. Use "dmesg | grep tst" to collect
> >>> the output information.
> >>
> >> Still fails:
> >>
> >>     tst: found kallsyms_test_func at index=12845
> >>     tst: [12533] = kallsyms_test_func, seq=17370, offset=191223
> >>     tst: [18800] = kallsyms_test_func, seq=23193, offset=259263
> >>     tst: [21934] = kallsyms_test_func, seq=2527, offset=22331
> >>     tst: [23501] = kallsyms_test_func, seq=11792, offset=126366
> >>     tst: [24284] = kallsyms_test_func, seq=8427, offset=87395
> >>     tst: [24676] = kallsyms_test_func, seq=21896, offset=243536
> >>     tst: [24872] = kallsyms_test_func, seq=22571, offset=251856
> >>     tst: [24970] = kallsyms_test_func, seq=23264, offset=260074
> >>     tst: [25019] = kallsyms_test_func, seq=9003, offset=93752
> >>     tst: [25043] = kallsyms_test_func, seq=14324, offset=155117
> >>     tst: [25055] = kallsyms_test_func, seq=5942, offset=62266
> >>     tst: [25061] = kallsyms_test_func, seq=14347, offset=155467
> >>     tst: [25064] = kallsyms_test_func, seq=14350, offset=155512
> >>     tst: [25066] = kallsyms_test_func, seq=14346, offset=155457
> >>     tst: [25067] = kallsyms_test_func, seq=14354, offset=155565
> >
> > -               pr_warn("tst: [%d] = %s, seq=%d, offset=%d\n", mid, name, seq, off);
> > +               pr_warn("tst: [%d] = %s, seq=%d, offset=%d\n", mid, namebuf, seq, off);
> >
> > Sorry, a variable in the debugging code is incorrectly written. 'name' should
> > be replaced with 'namebuf', then we can see which function the comparison is wrong.
>
> I attached debug patch v2.

On 30f3bb09778de64 with your debug patch v2:

kallsyms_selftest: start
tst: kallsyms_lookup_names() is OK, name=kallsyms_test_func, i=0
kallsyms_selftest: kallsyms_lookup_name() for
kallsyms_test_func_static failed: addr=8dbc, expect 60498
kallsyms_selftest: kallsyms_lookup_name() for kallsyms_test_func
failed: addr=8dbc, expect 604a8
kallsyms_selftest: kallsyms_lookup_name() for kallsyms_test_func_weak
failed: addr=8dbc, expect 604aa
kallsyms_selftest: kallsyms_lookup_name() for vmalloc failed:
addr=8dbc, expect bde18
kallsyms_selftest: kallsyms_lookup_name() for vfree failed: addr=8dbc,
expect bd86e
kallsyms_selftest: kallsyms_on_each_match_symbol() for
kallsyms_test_func_static failed: count=24891, addr=1eea00, expect
60498
kallsyms_selftest: kallsyms_on_each_match_symbol() for
kallsyms_test_func failed: count=24891, addr=1eea00, expect 604a8
kallsyms_selftest: kallsyms_on_each_match_symbol() for
kallsyms_test_func_weak failed: count=24891, addr=1eea00, expect 604aa
kallsyms_selftest: kallsyms_on_each_match_symbol() for vmalloc failed:
count=24891, addr=1eea00, expect bde18
kallsyms_selftest: kallsyms_on_each_match_symbol() for vfree failed:
count=24891, addr=1eea00, expect bd86e
kallsyms_selftest: abort

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
  

On 2022/12/15 22:51, Geert Uytterhoeven wrote:
> Hi Zhen,
> 
> On Thu, Dec 15, 2022 at 3:40 PM Leizhen (ThunderTown)
> <thunder.leizhen@huawei.com> wrote:
>> On 2022/12/15 21:58, Leizhen (ThunderTown) wrote:
>>> On 2022/12/15 21:24, Geert Uytterhoeven wrote:
>>>> On Thu, Dec 15, 2022 at 1:34 PM Leizhen (ThunderTown)
>>>> <thunder.leizhen@huawei.com> wrote:
>>>>> On 2022/12/15 17:39, Geert Uytterhoeven wrote:
>>>>>> On Thu, Dec 15, 2022 at 10:16 AM Leizhen (ThunderTown)
>>>>>> <thunder.leizhen@huawei.com> wrote:
>>>>>>> On 2022/12/15 16:50, Geert Uytterhoeven wrote:
>>>>>>>> On Tue, Nov 15, 2022 at 9:41 AM Zhen Lei <thunder.leizhen@huawei.com> wrote:
>>>>>>>>> Added test cases for basic functions and performance of functions
>>>>>>>>> kallsyms_lookup_name(), kallsyms_on_each_symbol() and
>>>>>>>>> kallsyms_on_each_match_symbol(). It also calculates the compression rate
>>>>>>>>> of the kallsyms compression algorithm for the current symbol set.
>>>>>>>>>
>>>>>>>>> The basic functions test begins by testing a set of symbols whose address
>>>>>>>>> values are known. Then, traverse all symbol addresses and find the
>>>>>>>>> corresponding symbol name based on the address. It's impossible to
>>>>>>>>> determine whether these addresses are correct, but we can use the above
>>>>>>>>> three functions along with the addresses to test each other. Due to the
>>>>>>>>> traversal operation of kallsyms_on_each_symbol() is too slow, only 60
>>>>>>>>> symbols can be tested in one second, so let it test on average once
>>>>>>>>> every 128 symbols. The other two functions validate all symbols.
>>>>>>>>>
>>>>>>>>> If the basic functions test is passed, print only performance test
>>>>>>>>> results. If the test fails, print error information, but do not perform
>>>>>>>>> subsequent performance tests.
>>>>>>>>>
>>>>>>>>> Start self-test automatically after system startup if
>>>>>>>>> CONFIG_KALLSYMS_SELFTEST=y.
>>>>>>>>>
>>>>>>>>> Example of output content: (prefix 'kallsyms_selftest:' is omitted
>>>>>>>>>  start
>>>>>>>>>   ---------------------------------------------------------
>>>>>>>>>  | nr_symbols | compressed size | original size | ratio(%) |
>>>>>>>>>  |---------------------------------------------------------|
>>>>>>>>>  |     107543 |       1357912   |      2407433  |  56.40   |
>>>>>>>>>   ---------------------------------------------------------
>>>>>>>>>  kallsyms_lookup_name() looked up 107543 symbols
>>>>>>>>>  The time spent on each symbol is (ns): min=630, max=35295, avg=7353
>>>>>>>>>  kallsyms_on_each_symbol() traverse all: 11782628 ns
>>>>>>>>>  kallsyms_on_each_match_symbol() traverse all: 9261 ns
>>>>>>>>>  finish
>>>>>>>>>
>>>>>>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>>>>>>
>>>>>>>> Thanks for your patch, which is now commit 30f3bb09778de64e ("kallsyms:
>>>>>>>> Add self-test facility") in linus/master.
>>>>>>>>
>>>>>>>> I gave this a try on m68k (atari_defconfig + CONFIG_KALLSYMS_SELFTEST=y),
>>>>>>>> but it failed:
>>>>>>>>
>>>>>>>>     start
>>>>>>>>     kallsyms_lookup_name() for kallsyms_test_func_static failed:
>>>>>>>> addr=0, expect 60ab0
>>>>>>>>     kallsyms_lookup_name() for kallsyms_test_func failed: addr=0, expect 60ac0
>>>>>>>>     kallsyms_lookup_name() for kallsyms_test_func_weak failed: addr=0,
>>>>>>>> expect 60ac2
>>>>>>>>     kallsyms_lookup_name() for vmalloc failed: addr=0, expect c272a
>>>>>>>>     kallsyms_lookup_name() for vfree failed: addr=0, expect c2142
>>>>>>>>     kallsyms_on_each_match_symbol() for kallsyms_test_func_static
>>>>>>>> failed: count=0, addr=0, expect 60ab0
>>>>>>>>     kallsyms_on_each_match_symbol() for kallsyms_test_func failed:
>>>>>>>> count=0, addr=0, expect 60ac0
>>>>>>>>     kallsyms_on_each_match_symbol() for kallsyms_test_func_weak
>>>>>>>> failed: count=0, addr=0, expect 60ac2
>>>>>>>>     kallsyms_on_each_match_symbol() for vmalloc failed: count=0,
>>>>>>>> addr=0, expect c272a
>>>>>>>>     kallsyms_on_each_match_symbol() for vfree failed: count=0, addr=0,
>>>>>>>> expect c2142
>>>>>>>>     abort
>>>>>>>>
>>>>>>>> Given all addresses are zero, it looks like some required functionality
>>>>>>>> or config option is missing.
>>>>>>>>
>>>>>>>> $ grep SYM .config
>>>>>>>> CONFIG_KALLSYMS=y
>>>>>>>> CONFIG_KALLSYMS_SELFTEST=y
>>>>>>>> CONFIG_KALLSYMS_BASE_RELATIVE=y
>>>>>>>> # CONFIG_ASYMMETRIC_KEY_TYPE is not set
>>>>>>>> CONFIG_SYMBOLIC_ERRNAME=y
>>>>>>>> # CONFIG_STRIP_ASM_SYMS is not set
>>>>>>>> CONFIG_KALLSYMS_SELFTEST
>>>>>>>>
>>>>>>>> Do you have a clue?
>>>>>>>
>>>>>>> cat /proc/kallsyms | grep kallsyms_test_func
>>>>>>> Let's see if the compiler-generated symbols have some special suffixes.
>>>>>>
>>>>>> Thanks, looks normal to me:
>>>>>>
>>>>>>     atari:~# cat /proc/kallsyms | grep kallsyms_test_func
>>>>>>     00060ab0 t kallsyms_test_func_static
>>>>>>     00060ac0 T kallsyms_test_func
>>>>>>     00060ac2 W kallsyms_test_func_weak
>>>>>>     atari:~#
>>>>>
>>>>> It's incredible. I don't have a m68k environment and I'm trying to build a qemu
>>>>> environment. If you're in a hurry and willing, you can apply the debugging patch
>>>>> in the attachment. I'd like to see what's wrong. Use "dmesg | grep tst" to collect
>>>>> the output information.
>>>>
>>>> Still fails:
>>>>
>>>>     tst: found kallsyms_test_func at index=12845
>>>>     tst: [12533] = kallsyms_test_func, seq=17370, offset=191223
>>>>     tst: [18800] = kallsyms_test_func, seq=23193, offset=259263
>>>>     tst: [21934] = kallsyms_test_func, seq=2527, offset=22331
>>>>     tst: [23501] = kallsyms_test_func, seq=11792, offset=126366
>>>>     tst: [24284] = kallsyms_test_func, seq=8427, offset=87395
>>>>     tst: [24676] = kallsyms_test_func, seq=21896, offset=243536
>>>>     tst: [24872] = kallsyms_test_func, seq=22571, offset=251856
>>>>     tst: [24970] = kallsyms_test_func, seq=23264, offset=260074
>>>>     tst: [25019] = kallsyms_test_func, seq=9003, offset=93752
>>>>     tst: [25043] = kallsyms_test_func, seq=14324, offset=155117
>>>>     tst: [25055] = kallsyms_test_func, seq=5942, offset=62266
>>>>     tst: [25061] = kallsyms_test_func, seq=14347, offset=155467
>>>>     tst: [25064] = kallsyms_test_func, seq=14350, offset=155512
>>>>     tst: [25066] = kallsyms_test_func, seq=14346, offset=155457
>>>>     tst: [25067] = kallsyms_test_func, seq=14354, offset=155565
>>>
>>> -               pr_warn("tst: [%d] = %s, seq=%d, offset=%d\n", mid, name, seq, off);
>>> +               pr_warn("tst: [%d] = %s, seq=%d, offset=%d\n", mid, namebuf, seq, off);
>>>
>>> Sorry, a variable in the debugging code is incorrectly written. 'name' should
>>> be replaced with 'namebuf', then we can see which function the comparison is wrong.
>>
>> I attached debug patch v2.
> 
> On 30f3bb09778de64 with your debug patch v2:

I've set up the qemu environment, and I'll try to solve it by tomorrow at the latest.

> 
> kallsyms_selftest: start
> tst: kallsyms_lookup_names() is OK, name=kallsyms_test_func, i=0
> kallsyms_selftest: kallsyms_lookup_name() for
> kallsyms_test_func_static failed: addr=8dbc, expect 60498
> kallsyms_selftest: kallsyms_lookup_name() for kallsyms_test_func
> failed: addr=8dbc, expect 604a8
> kallsyms_selftest: kallsyms_lookup_name() for kallsyms_test_func_weak
> failed: addr=8dbc, expect 604aa
> kallsyms_selftest: kallsyms_lookup_name() for vmalloc failed:
> addr=8dbc, expect bde18
> kallsyms_selftest: kallsyms_lookup_name() for vfree failed: addr=8dbc,
> expect bd86e
> kallsyms_selftest: kallsyms_on_each_match_symbol() for
> kallsyms_test_func_static failed: count=24891, addr=1eea00, expect
> 60498
> kallsyms_selftest: kallsyms_on_each_match_symbol() for
> kallsyms_test_func failed: count=24891, addr=1eea00, expect 604a8
> kallsyms_selftest: kallsyms_on_each_match_symbol() for
> kallsyms_test_func_weak failed: count=24891, addr=1eea00, expect 604aa
> kallsyms_selftest: kallsyms_on_each_match_symbol() for vmalloc failed:
> count=24891, addr=1eea00, expect bde18
> kallsyms_selftest: kallsyms_on_each_match_symbol() for vfree failed:
> count=24891, addr=1eea00, expect bd86e
> kallsyms_selftest: abort
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 
> .
>
  

On 2022/12/16 15:42, Leizhen (ThunderTown) wrote:
> 
> 
> On 2022/12/15 22:51, Geert Uytterhoeven wrote:
>> Hi Zhen,
>>
>>
>> On 30f3bb09778de64 with your debug patch v2:
> 
> I've set up the qemu environment, and I'll try to solve it by tomorrow at the latest.

It seems that the problem is still strcmp(). After I commented strcmp() in
arch/m68k/include/asm/string.h, and force it to use the one in lib/string.c,
it works well.

The output i=0, because compare_symbol_name() always return zero in below loop.
'low' decreases to 0, then the loop ends.
The reason is not clear. I'm still analyzing it.

        low = mid;
        while (low) {
                seq = get_symbol_seq(low - 1);
                off = get_symbol_offset(seq);
                kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf));
                if (compare_symbol_name(name, namebuf))
                        break;
                low--;
        }
        *start = low;


> 
>>
>>
>> Gr{oetje,eeting}s,
>>
>>                         Geert
>>
>> --
>> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>>
>> In personal conversations with technical people, I call myself a hacker. But
>> when I'm talking to journalists I just say "programmer" or something like that.
>>                                 -- Linus Torvalds
>>
>> .
>>
>
  

From: Geert Uytterhoeven 
> Sent: 15 December 2022 13:25
...
> Looks like commit 3bc753c06dd02a35 ("kbuild: treat char as always
> unsigned") is to blame.
> 
> Changing:
> 
>     --- a/arch/m68k/include/asm/string.h
>     +++ b/arch/m68k/include/asm/string.h
>     @@ -42,7 +42,7 @@ static inline char *strncpy(char *dest, const
> char *src, size_t n)
>      #define __HAVE_ARCH_STRCMP
>      static inline int strcmp(const char *cs, const char *ct)
>      {
>     -       char res;
>     +       signed char res;
> 
>             asm ("\n"
>                     "1:     move.b  (%0)+,%2\n"     /* get *cs */
> 
> fixes strcmp, but the test still fails:

Try 'int res;' and an explicit sign extend (I think):
	"3: extb %2\n"

The strcmp() is still wrong if either input string
has characters with the top bit set.
The result needs to be based of the carry flag not
the sign of the byte subtract.

It is too long since I've written m68k asm.
I've checked, all byte operations leave the high 24bits
unchanged.
So it is possible that gcc is making assumptions and
skipping the sign extend under some circumstances.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

Hi Zhen,

On Fri, Dec 16, 2022 at 10:43 AM Leizhen (ThunderTown)
<thunder.leizhen@huawei.com> wrote:
> On 2022/12/16 15:42, Leizhen (ThunderTown) wrote:
> > On 2022/12/15 22:51, Geert Uytterhoeven wrote:
> >> On 30f3bb09778de64 with your debug patch v2:
> > I've set up the qemu environment, and I'll try to solve it by tomorrow at the latest.
>
> It seems that the problem is still strcmp(). After I commented strcmp() in
> arch/m68k/include/asm/string.h, and force it to use the one in lib/string.c,
> it works well.

I can confirm that.

One difference is that the one in lib/string.c always return -1/0/1,
while the m68k version can return other negative or positive numbers.

However, adding:

       if (res < 0) return -1;
       if (res > 0) return 1;

to the m68k version doesn't make a difference.

Renaming the m68k version (changed to -1/0/1) to m68k_strcmp(), and
the generic version to lib_strcmp(), and adding a wrapper that calls
and compares both, shows that both functions do return the same value,
and the test succeeds.

Moving the m68k version inside lib/string.c makes the test pass, too.
So it must be related to the function being inline, and gcc making
(incorrect) assumptions...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
  

On 2022/12/16 18:40, David Laight wrote:
> From: Geert Uytterhoeven 
>> Sent: 15 December 2022 13:25
> ...
>> Looks like commit 3bc753c06dd02a35 ("kbuild: treat char as always
>> unsigned") is to blame.
>>
>> Changing:
>>
>>     --- a/arch/m68k/include/asm/string.h
>>     +++ b/arch/m68k/include/asm/string.h
>>     @@ -42,7 +42,7 @@ static inline char *strncpy(char *dest, const
>> char *src, size_t n)
>>      #define __HAVE_ARCH_STRCMP
>>      static inline int strcmp(const char *cs, const char *ct)
>>      {
>>     -       char res;
>>     +       signed char res;
>>
>>             asm ("\n"
>>                     "1:     move.b  (%0)+,%2\n"     /* get *cs */
>>
>> fixes strcmp, but the test still fails:
> 
> Try 'int res;' and an explicit sign extend (I think):
> 	"3: extb %2\n"

Compilation failed. I tried "return (int)(signed char)res;", it's still failed.

> 
> The strcmp() is still wrong if either input string
> has characters with the top bit set.
> The result needs to be based of the carry flag not
> the sign of the byte subtract.
> 
> It is too long since I've written m68k asm.
> I've checked, all byte operations leave the high 24bits
> unchanged.

Currently, only ASCCIs. So it won't be the reason.

> So it is possible that gcc is making assumptions and
> skipping the sign extend under some circumstances.

Wow, because compare_symbol_name() works properly during the previous binary
search, the compiler must have done something bad. So I add 'volatile' to prevent
compiler optimizations, and it's OK now.

diff --git a/arch/m68k/include/asm/string.h b/arch/m68k/include/asm/string.h
index f759d944c449940..3db81e5a783c72a 100644
--- a/arch/m68k/include/asm/string.h
+++ b/arch/m68k/include/asm/string.h
@@ -42,9 +42,9 @@ static inline char *strncpy(char *dest, const char *src, size_t n)
 #define __HAVE_ARCH_STRCMP
 static inline int strcmp(const char *cs, const char *ct)
 {
-       char res;
+       signed char res;

-       asm ("\n"
+       asm volatile ("\n"
                "1:     move.b  (%0)+,%2\n"     /* get *cs */
                "       cmp.b   (%1)+,%2\n"     /* compare a byte */
                "       jne     2f\n"           /* not equal, break out */

> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
  

On Dez 15 2022, Geert Uytterhoeven wrote:

> Changing:
>
>     --- a/arch/m68k/include/asm/string.h
>     +++ b/arch/m68k/include/asm/string.h
>     @@ -42,7 +42,7 @@ static inline char *strncpy(char *dest, const
> char *src, size_t n)
>      #define __HAVE_ARCH_STRCMP
>      static inline int strcmp(const char *cs, const char *ct)
>      {
>     -       char res;
>     +       signed char res;
>
>             asm ("\n"
>                     "1:     move.b  (%0)+,%2\n"     /* get *cs */

Note that strcmp needs to compute the return value from the difference
of the _unsigned_ characters.  That does not explain the error, though,
since symbol names don't have characters with the high bit set.
  

On Dez 16 2022, Leizhen (ThunderTown) wrote:

> It seems that the problem is still strcmp(). After I commented strcmp() in
> arch/m68k/include/asm/string.h, and force it to use the one in lib/string.c,
> it works well.

Does that help?

diff --git a/arch/m68k/include/asm/string.h b/arch/m68k/include/asm/string.h
index f759d944c449..cdafd67774e0 100644
--- a/arch/m68k/include/asm/string.h
+++ b/arch/m68k/include/asm/string.h
@@ -42,18 +42,19 @@ static inline char *strncpy(char *dest, const char *src, size_t n)
 #define __HAVE_ARCH_STRCMP
 static inline int strcmp(const char *cs, const char *ct)
 {
-	char res;
+	int res = 0, tmp = 0;
 
 	asm ("\n"
 		"1:	move.b	(%0)+,%2\n"	/* get *cs */
-		"	cmp.b	(%1)+,%2\n"	/* compare a byte */
+		"	move.b	(%1)+,%3\n"	/* get *ct */
+		"	cmp.b	%3,%2\n"	/* compare a byte */
 		"	jne	2f\n"		/* not equal, break out */
 		"	tst.b	%2\n"		/* at end of cs? */
 		"	jne	1b\n"		/* no, keep going */
 		"	jra	3f\n"		/* strings are equal */
-		"2:	sub.b	-(%1),%2\n"	/* *cs - *ct */
+		"2:	sub.l	%3,%2\n"	/* *cs - *ct */
 		"3:"
-		: "+a" (cs), "+a" (ct), "=d" (res));
+		: "+a" (cs), "+a" (ct), "+d" (res), "+d" (tmp));
 	return res;
 }
 #endif /* CONFIG_COLDFIRE */
  

On 2022/12/16 19:44, Andreas Schwab wrote:
> On Dez 15 2022, Geert Uytterhoeven wrote:
> 
>> Changing:
>>
>>     --- a/arch/m68k/include/asm/string.h
>>     +++ b/arch/m68k/include/asm/string.h
>>     @@ -42,7 +42,7 @@ static inline char *strncpy(char *dest, const
>> char *src, size_t n)
>>      #define __HAVE_ARCH_STRCMP
>>      static inline int strcmp(const char *cs, const char *ct)
>>      {
>>     -       char res;
>>     +       signed char res;
>>
>>             asm ("\n"
>>                     "1:     move.b  (%0)+,%2\n"     /* get *cs */
> 
> Note that strcmp needs to compute the return value from the difference
> of the _unsigned_ characters.  That does not explain the error, though,
> since symbol names don't have characters with the high bit set.

Why consider the high bit? It is not memcmp(). If the high bit is set,
it is the user's fault.


>
  

On 2022/12/16 19:28, Geert Uytterhoeven wrote:
> Hi Zhen,
> 
> On Fri, Dec 16, 2022 at 10:43 AM Leizhen (ThunderTown)
> <thunder.leizhen@huawei.com> wrote:
>> On 2022/12/16 15:42, Leizhen (ThunderTown) wrote:
>>> On 2022/12/15 22:51, Geert Uytterhoeven wrote:
>>>> On 30f3bb09778de64 with your debug patch v2:
>>> I've set up the qemu environment, and I'll try to solve it by tomorrow at the latest.
>>
>> It seems that the problem is still strcmp(). After I commented strcmp() in
>> arch/m68k/include/asm/string.h, and force it to use the one in lib/string.c,
>> it works well.
> 
> I can confirm that.
> 
> One difference is that the one in lib/string.c always return -1/0/1,
> while the m68k version can return other negative or positive numbers.
> 
> However, adding:
> 
>        if (res < 0) return -1;
>        if (res > 0) return 1;
> 
> to the m68k version doesn't make a difference.
> 
> Renaming the m68k version (changed to -1/0/1) to m68k_strcmp(), and
> the generic version to lib_strcmp(), and adding a wrapper that calls
> and compares both, shows that both functions do return the same value,
> and the test succeeds.
> 
> Moving the m68k version inside lib/string.c makes the test pass, too.
> So it must be related to the function being inline, and gcc making
> (incorrect) assumptions...

Yes, it's the compiler's fault. I just replied David Laight:

I added 'volatile' to prevent compiler optimizations, and it's OK now.

diff --git a/arch/m68k/include/asm/string.h b/arch/m68k/include/asm/string.h
index f759d944c449940..3db81e5a783c72a 100644
--- a/arch/m68k/include/asm/string.h
+++ b/arch/m68k/include/asm/string.h
@@ -42,9 +42,9 @@ static inline char *strncpy(char *dest, const char *src, size_t n)
 #define __HAVE_ARCH_STRCMP
 static inline int strcmp(const char *cs, const char *ct)
 {
-       char res;
+       signed char res;

-       asm ("\n"
+       asm volatile ("\n"
                "1:     move.b  (%0)+,%2\n"     /* get *cs */
                "       cmp.b   (%1)+,%2\n"     /* compare a byte */
                "       jne     2f\n"           /* not equal, break out */

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 
> .
>
  

On Dez 16 2022, Leizhen (ThunderTown) wrote:

> Why consider the high bit? It is not memcmp(). If the high bit is set,
> it is the user's fault.

Nope, the semantics of strcmp are defined on the full CHAR_MIN to
CHAR_MAX range.
  

From: Leizhen (ThunderTown)
> Sent: 16 December 2022 12:02
> 
..
> > Moving the m68k version inside lib/string.c makes the test pass, too.
> > So it must be related to the function being inline, and gcc making
> > (incorrect) assumptions...
> 
> Yes, it's the compiler's fault. I just replied David Laight:
> 
> I added 'volatile' to prevent compiler optimizations, and it's OK now.
> 
> diff --git a/arch/m68k/include/asm/string.h b/arch/m68k/include/asm/string.h
> index f759d944c449940..3db81e5a783c72a 100644
> --- a/arch/m68k/include/asm/string.h
> +++ b/arch/m68k/include/asm/string.h
> @@ -42,9 +42,9 @@ static inline char *strncpy(char *dest, const char *src, size_t n)
>  #define __HAVE_ARCH_STRCMP
>  static inline int strcmp(const char *cs, const char *ct)
>  {
> -       char res;
> +       signed char res;
> 
> -       asm ("\n"
> +       asm volatile ("\n"
>                 "1:     move.b  (%0)+,%2\n"     /* get *cs */
>                 "       cmp.b   (%1)+,%2\n"     /* compare a byte */
>                 "       jne     2f\n"           /* not equal, break out */

Adding 'volatile' there shouldn't make any real difference.

I'd double-check the asm constraints for the two pointers.
They are modified by the asm, but the caller's variables
must not be changed.

I think that means they need to be normal 'input' parameters
and the result must be in different register (early clobber?).
Currently the pointers are "+r" - which I think means they
are input-output and any caller-supplied variable is
likely to get changed.

	David

	

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

On 2022/12/16 19:57, Andreas Schwab wrote:
> On Dez 16 2022, Leizhen (ThunderTown) wrote:
> 
>> It seems that the problem is still strcmp(). After I commented strcmp() in
>> arch/m68k/include/asm/string.h, and force it to use the one in lib/string.c,
>> it works well.
> 
> Does that help?

It still needs to add 'volatile' to prevent compiler optimizations.
Otherwise,
Tested-by: Zhen Lei <thunder.leizhen@huawei.com>

> 
> diff --git a/arch/m68k/include/asm/string.h b/arch/m68k/include/asm/string.h
> index f759d944c449..cdafd67774e0 100644
> --- a/arch/m68k/include/asm/string.h
> +++ b/arch/m68k/include/asm/string.h
> @@ -42,18 +42,19 @@ static inline char *strncpy(char *dest, const char *src, size_t n)
>  #define __HAVE_ARCH_STRCMP
>  static inline int strcmp(const char *cs, const char *ct)
>  {
> -	char res;
> +	int res = 0, tmp = 0;
>  
>  	asm ("\n"

-       asm ("\n"
+       asm volatile ("\n"


>  		"1:	move.b	(%0)+,%2\n"	/* get *cs */
> -		"	cmp.b	(%1)+,%2\n"	/* compare a byte */
> +		"	move.b	(%1)+,%3\n"	/* get *ct */
> +		"	cmp.b	%3,%2\n"	/* compare a byte */
>  		"	jne	2f\n"		/* not equal, break out */
>  		"	tst.b	%2\n"		/* at end of cs? */
>  		"	jne	1b\n"		/* no, keep going */
>  		"	jra	3f\n"		/* strings are equal */
> -		"2:	sub.b	-(%1),%2\n"	/* *cs - *ct */
> +		"2:	sub.l	%3,%2\n"	/* *cs - *ct */
>  		"3:"
> -		: "+a" (cs), "+a" (ct), "=d" (res));
> +		: "+a" (cs), "+a" (ct), "+d" (res), "+d" (tmp));
>  	return res;
>  }
>  #endif /* CONFIG_COLDFIRE */
>
  

On 2022/12/16 20:39, Andreas Schwab wrote:
> On Dez 16 2022, Leizhen (ThunderTown) wrote:
> 
>> Why consider the high bit? It is not memcmp(). If the high bit is set,
>> it is the user's fault.
> 
> Nope, the semantics of strcmp are defined on the full CHAR_MIN to
> CHAR_MAX range.

OK, I got it.

>
  

On Dez 16 2022, Leizhen (ThunderTown) wrote:

> On 2022/12/16 19:57, Andreas Schwab wrote:
>> On Dez 16 2022, Leizhen (ThunderTown) wrote:
>> 
>>> It seems that the problem is still strcmp(). After I commented strcmp() in
>>> arch/m68k/include/asm/string.h, and force it to use the one in lib/string.c,
>>> it works well.
>> 
>> Does that help?
>
> It still needs to add 'volatile' to prevent compiler optimizations.

If that makes a difference, then something else is wrong.
  

From: David Laight
> Sent: 16 December 2022 13:30
> 
> From: Leizhen (ThunderTown)
> > Sent: 16 December 2022 12:02
> >
> ..
> > > Moving the m68k version inside lib/string.c makes the test pass, too.
> > > So it must be related to the function being inline, and gcc making
> > > (incorrect) assumptions...
> >
> > Yes, it's the compiler's fault. I just replied David Laight:
> >
> > I added 'volatile' to prevent compiler optimizations, and it's OK now.
> >
> > diff --git a/arch/m68k/include/asm/string.h b/arch/m68k/include/asm/string.h
> > index f759d944c449940..3db81e5a783c72a 100644
> > --- a/arch/m68k/include/asm/string.h
> > +++ b/arch/m68k/include/asm/string.h
> > @@ -42,9 +42,9 @@ static inline char *strncpy(char *dest, const char *src, size_t n)
> >  #define __HAVE_ARCH_STRCMP
> >  static inline int strcmp(const char *cs, const char *ct)
> >  {
> > -       char res;
> > +       signed char res;
> >
> > -       asm ("\n"
> > +       asm volatile ("\n"
> >                 "1:     move.b  (%0)+,%2\n"     /* get *cs */
> >                 "       cmp.b   (%1)+,%2\n"     /* compare a byte */
> >                 "       jne     2f\n"           /* not equal, break out */
> 
> Adding 'volatile' there shouldn't make any real difference.
> 
> I'd double-check the asm constraints for the two pointers.
> They are modified by the asm, but the caller's variables
> must not be changed.
> 
> I think that means they need to be normal 'input' parameters
> and the result must be in different register (early clobber?).
> Currently the pointers are "+r" - which I think means they
> are input-output and any caller-supplied variable is
> likely to get changed.

Definitely badly broken.
'cs' and 'ct' should be input parameters.
'res' needs to be an early-clobber output parameter "=&r".

Since it is actually a 'static inline' (not just a #define)
then letting cs/ct be changed probably doesn't matter.

But the lack of 'early clobber' will cause grief.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

On Dez 16 2022, David Laight wrote:

> 'cs' and 'ct' should be input parameters.

They are (and output as well).
  

From: Andreas Schwab <schwab@linux-m68k.org>
> Sent: 16 December 2022 15:20
> 
> On Dez 16 2022, David Laight wrote:
> 
> > 'cs' and 'ct' should be input parameters.
> 
> They are (and output as well).

Right, but the 'output' values aren't needed and get
discarded because the asm block in inside a static inline.

It could be a recipe just waiting to go wrong.

But I bet the actual problem is the output register
aliasing one of the inputs and then reading from very
low user addresses - I bet they don't fault on m68k.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

On Dez 16 2022, David Laight wrote:

> Right, but the 'output' values aren't needed and get

Yes, they are, because the inline asm modifies them.
  

From: Andreas Schwab
> Sent: 16 December 2022 15:54
> 
> On Dez 16 2022, David Laight wrote:
> 
> > Right, but the 'output' values aren't needed and get
> 
> Yes, they are, because the inline asm modifies them.

Ok, my brain hurts, too near christmas.

The other issue is a missing "memory" clobber.
Although I'm not sure why that could matter here.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

On Dez 16 2022, David Laight wrote:

> The other issue is a missing "memory" clobber.

strcmp is a pure read-only operation.
  

From: Andreas Schwab
> Sent: 16 December 2022 16:12
> On Dez 16 2022, David Laight wrote:
> 
> > The other issue is a missing "memory" clobber.
> 
> strcmp is a pure read-only operation.

Still needs the memory clobber.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

On Fri, 16 Dec 2022 16:32:08 +0000
David Laight <David.Laight@ACULAB.COM> wrote:

> From: Andreas Schwab
> > Sent: 16 December 2022 16:12
> > On Dez 16 2022, David Laight wrote:
> >   
> > > The other issue is a missing "memory" clobber.  
> > 
> > strcmp is a pure read-only operation.  
> 
> Still needs the memory clobber.

What memory is being modified?

I don't know m68k assembly, but I'm guessing the (%O)+ and the (%1)+ only
modifies the register being used, and no memory is being touched.

-- Steve
  

From: Steven Rostedt 
> Sent: 16 December 2022 16:53
> 
> On Fri, 16 Dec 2022 16:32:08 +0000
> David Laight <David.Laight@ACULAB.COM> wrote:
> 
> > From: Andreas Schwab
> > > Sent: 16 December 2022 16:12
> > > On Dez 16 2022, David Laight wrote:
> > >
> > > > The other issue is a missing "memory" clobber.
> > >
> > > strcmp is a pure read-only operation.
> >
> > Still needs the memory clobber.
> 
> What memory is being modified?
> 
> I don't know m68k assembly, but I'm guessing the (%O)+ and the (%1)+ only
> modifies the register being used, and no memory is being touched.

Memory is being read so a clobber is needed to ensure that all pending
writes have been done.
Although, in this case, the memory being read is effectively
read only so nothing should be pending.

But it really does look like something is 'smelly' with the
constraints.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

On Fri, 16 Dec 2022 16:57:54 +0000
David Laight <David.Laight@ACULAB.COM> wrote:

> > I don't know m68k assembly, but I'm guessing the (%O)+ and the (%1)+ only
> > modifies the register being used, and no memory is being touched.  
> 
> Memory is being read so a clobber is needed to ensure that all pending
> writes have been done.
> Although, in this case, the memory being read is effectively
> read only so nothing should be

I would think that if the memory being read is the address of what the
input parameters being passed into it are, then the compiler should be
smart enough to know that there's a relation. Especially if the input
parameters are pointers.

I assumed that "memory" was for memory unrelated to the input constraints.

-- Steve
  

On Fri, 16 Dec 2022 12:19:47 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> I assumed that "memory" was for memory unrelated to the input constraints.

Well, it looks like you do need a "memory" barrier.

  https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html

"memory"

      The "memory" clobber tells the compiler that the assembly code
      performs memory reads or writes to items other than those listed in
      the input and output operands (for example, accessing the memory
      pointed to by one of the input parameters). To ensure memory contains
      correct values, GCC may need to flush specific register values to
      memory before executing the asm. Further, the compiler does not
      assume that any values read from memory before an asm remain
      unchanged after that asm; it reloads them as needed. Using the
      "memory" clobber effectively forms a read/write memory barrier for
      the compiler.

As the "(for example, accessing the memory pointed to by one of the input
parameters)" is exactly this case.

-- Steve
  

From: Steven Rostedt
> Sent: 16 December 2022 17:38
> 
> On Fri, 16 Dec 2022 12:19:47 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > I assumed that "memory" was for memory unrelated to the input constraints.
> 
> Well, it looks like you do need a "memory" barrier.
> 
>   https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html
> 
> "memory"
> 
>       The "memory" clobber tells the compiler that the assembly code
>       performs memory reads or writes to items other than those listed in
>       the input and output operands (for example, accessing the memory
>       pointed to by one of the input parameters). To ensure memory contains
>       correct values, GCC may need to flush specific register values to
>       memory before executing the asm. Further, the compiler does not
>       assume that any values read from memory before an asm remain
>       unchanged after that asm; it reloads them as needed. Using the
>       "memory" clobber effectively forms a read/write memory barrier for
>       the compiler.
> 
> As the "(for example, accessing the memory pointed to by one of the input
> parameters)" is exactly this case.

Without the memory clobber code like:
int f(const char *s)
{
	char c[4] = "abc";
	return strcmp(s, c);
}
is very like to get optimised so that c[] is never written.

However, in this case, the strings have all existed for ages.
So that won't be the problem.

It might be obvious what is wrong from the asm output.
Although the binary-chop lookup is suspect I'd also check
that the sorted index is plausible - just tracing the first
20 entries might be enough.
No point peering at the search code if the setup is wrong.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

On 2022/12/17 3:27, David Laight wrote:
> From: Steven Rostedt
>> Sent: 16 December 2022 17:38
>>
>> On Fri, 16 Dec 2022 12:19:47 -0500
>> Steven Rostedt <rostedt@goodmis.org> wrote:
>>
>>> I assumed that "memory" was for memory unrelated to the input constraints.
>>
>> Well, it looks like you do need a "memory" barrier.
>>
>>   https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html
>>
>> "memory"
>>
>>       The "memory" clobber tells the compiler that the assembly code
>>       performs memory reads or writes to items other than those listed in
>>       the input and output operands (for example, accessing the memory
>>       pointed to by one of the input parameters). To ensure memory contains
>>       correct values, GCC may need to flush specific register values to
>>       memory before executing the asm. Further, the compiler does not
>>       assume that any values read from memory before an asm remain
>>       unchanged after that asm; it reloads them as needed. Using the
>>       "memory" clobber effectively forms a read/write memory barrier for
>>       the compiler.
>>
>> As the "(for example, accessing the memory pointed to by one of the input
>> parameters)" is exactly this case.
> 
> Without the memory clobber code like:
> int f(const char *s)
> {
> 	char c[4] = "abc";
> 	return strcmp(s, c);
> }
> is very like to get optimised so that c[] is never written.
> 
> However, in this case, the strings have all existed for ages.
> So that won't be the problem.
> 
> It might be obvious what is wrong from the asm output.
> Although the binary-chop lookup is suspect I'd also check
> that the sorted index is plausible - just tracing the first
> 20 entries might be enough.
> No point peering at the search code if the setup is wrong.

6.47.2.1 Volatile
GCC’s optimizers sometimes discard asm statements if they determine there is no need for
the output variables. Also, the optimizers may move code out of loops if they believe that
the code will always return the same result (i.e. none of its input values change between
calls). Using the volatile qualifier disables these optimizations.

So it's quite possible (I didn't disassemble vmlinux, because I didn't learn m68k):

//binary search
while (low <= high) {
    ...
    ret = compare_symbol_name(name, namebuf);   ----> (1)
    if (!ret)
        break;
}

low = mid;
while (low) {
    ...
    if (compare_symbol_name(name, namebuf))     ----> (2)
        break;
    low--;
}

The pointer 'name' and 'namebuf' of (1) and (2) are the same,
so the 'if' statement of (2) maybe omitted by compiler.

By the way, I tried no volatile but with
+               : : "memory");
It also works well.

> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 
> 
> .
>
  

Hi Zhen,

On Sat, Dec 17, 2022 at 8:32 AM Leizhen (ThunderTown)
<thunder.leizhen@huawei.com> wrote:
> On 2022/12/17 3:27, David Laight wrote:
> > From: Steven Rostedt
> >> Sent: 16 December 2022 17:38
> >>
> >> On Fri, 16 Dec 2022 12:19:47 -0500
> >> Steven Rostedt <rostedt@goodmis.org> wrote:
> >>
> >>> I assumed that "memory" was for memory unrelated to the input constraints.
> >>
> >> Well, it looks like you do need a "memory" barrier.
> >>
> >>   https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html
> >>
> >> "memory"
> >>
> >>       The "memory" clobber tells the compiler that the assembly code
> >>       performs memory reads or writes to items other than those listed in
> >>       the input and output operands (for example, accessing the memory
> >>       pointed to by one of the input parameters). To ensure memory contains
> >>       correct values, GCC may need to flush specific register values to
> >>       memory before executing the asm. Further, the compiler does not
> >>       assume that any values read from memory before an asm remain
> >>       unchanged after that asm; it reloads them as needed. Using the
> >>       "memory" clobber effectively forms a read/write memory barrier for
> >>       the compiler.
> >>
> >> As the "(for example, accessing the memory pointed to by one of the input
> >> parameters)" is exactly this case.
> >
> > Without the memory clobber code like:
> > int f(const char *s)
> > {
> >       char c[4] = "abc";
> >       return strcmp(s, c);
> > }
> > is very like to get optimised so that c[] is never written.
> >
> > However, in this case, the strings have all existed for ages.
> > So that won't be the problem.
> >
> > It might be obvious what is wrong from the asm output.
> > Although the binary-chop lookup is suspect I'd also check
> > that the sorted index is plausible - just tracing the first
> > 20 entries might be enough.
> > No point peering at the search code if the setup is wrong.
>
> 6.47.2.1 Volatile
> GCC’s optimizers sometimes discard asm statements if they determine there is no need for
> the output variables. Also, the optimizers may move code out of loops if they believe that
> the code will always return the same result (i.e. none of its input values change between
> calls). Using the volatile qualifier disables these optimizations.
>
> So it's quite possible (I didn't disassemble vmlinux, because I didn't learn m68k):

You don't have to disassemble, "make kernel/kallsyms.s" creates
annotated assembler.

> //binary search
> while (low <= high) {
>     ...
>     ret = compare_symbol_name(name, namebuf);   ----> (1)
>     if (!ret)
>         break;
> }
>
> low = mid;
> while (low) {
>     ...
>     if (compare_symbol_name(name, namebuf))     ----> (2)
>         break;
>     low--;
> }
>
> The pointer 'name' and 'namebuf' of (1) and (2) are the same,
> so the 'if' statement of (2) maybe omitted by compiler.

And that is exactly what is happening: there are 3 calls to strcmp()
with the exact same parameters, and gcc omits two of them, because it
assumes it can predict the outcome, as the parameters haven't changed.

Now, why have we never noticed this before? I guess because it is very
uncommon for a function calling strcmp() multiple times with the exact
same pointer parameters.  Common users change the pointers before
every call, instead of keeping the pointer, but changing the buffers'
contents the pointers point to.

> By the way, I tried no volatile but with
> +               : : "memory");
> It also works well.

Indeed, gcc version 9.4.0 (Ubuntu 9.4.0-1ubuntu1~20.04) generates the
same code for either adding the volatile or the memory clobber.

Note that strcmp() is the only function in arch/m68k/include/asm/string.h
using inline asm without the volatile keyword.  I guess we would
see similar issues with strnlen() (which also doesn't modify memory)
when dropping the volatile.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
  

From: Geert Uytterhoeven
> Sent: 17 December 2022 13:37
...
> > So it's quite possible (I didn't disassemble vmlinux, because I didn't learn m68k):

Pretty easy to read it.

...
> > //binary search
> > while (low <= high) {
> >     ...
> >     ret = compare_symbol_name(name, namebuf);   ----> (1)
> >     if (!ret)
> >         break;
> > }
> >
> > low = mid;
> > while (low) {
> >     ...
> >     if (compare_symbol_name(name, namebuf))     ----> (2)
> >         break;
> >     low--;
> > }
> >
> > The pointer 'name' and 'namebuf' of (1) and (2) are the same,
> > so the 'if' statement of (2) maybe omitted by compiler.
> 
> And that is exactly what is happening: there are 3 calls to strcmp()
> with the exact same parameters, and gcc omits two of them, because it
> assumes it can predict the outcome, as the parameters haven't changed.

Additionally if kallsyms_expand_symbol() is inlined all the writes
it does to namebuf[] can get discarded - because they are used.
Which probably causes the entire function to get optimised away.
More code follows - there could easily be nothing left except
the binary chop itself that ensures the loop terminates.

> Now, why have we never noticed this before? I guess because it is very
> uncommon for a function calling strcmp() multiple times with the exact
> same pointer parameters.  Common users change the pointers before
> every call, instead of keeping the pointer, but changing the buffers'
> contents the pointers point to.
> 
> > By the way, I tried no volatile but with
> > +               : : "memory");
> > It also works well.
> 
> Indeed, gcc version 9.4.0 (Ubuntu 9.4.0-1ubuntu1~20.04) generates the
> same code for either adding the volatile or the memory clobber.
> 
> Note that strcmp() is the only function in arch/m68k/include/asm/string.h
> using inline asm without the volatile keyword.  I guess we would
> see similar issues with strnlen() (which also doesn't modify memory)
> when dropping the volatile.

The 'volatile' ensures that strcmp() doesn't get thrown away.
It doesn't ensure that the writes to namebuf[] aren't optimised away.

Only the memory clobber does that.

So all those asm functions need the memory clobber.
They probably don't need volatile - since the call has
a result.
Indeed since the memory clobber also says 'writes to memory'
it may make the volatile pointless.

I'd also add the 'early clobber' annotation to the result.
It may not matter since the pointers are in-out, but since
the pointer outputs are discarded some 'brain-damaged'
register allocation might assign the same register.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)