[RFC] Fix the support of larger symbol name

[fbq]

In the initial support, we enlarge the max length of symbol name to be 511 (by setting KSYM_NAME_LEN), however we still use a 500 bytes buffer to read the symbol name in read_symbol(), which makes 1) the support for symbol name with length 500~511 is missing and 2) we are unable to tell whether a symbol name is too long .

Fix this by enlarging the buffer size. A selftest is also added in this PR, if the fix is dropped, the selftest can trigger a warning that shows the problem. Note that the selftest is still a POC, because the longest symbol is always compiled when KALLSYMS is enabled, which we probably don't want it.

[ojeda]

Yay, hardcoded constants!

When I initially added the big symbol support, I grepped for 255, 256 etc., but I missed this one using 499 and 500.

Note that the selftest is still a POC, because the longest symbol is always compiled when KALLSYMS is enabled, which we probably don't want it.

Could we put it under DEBUG_KERNEL or perhaps DEBUG_MISC?

[ojeda]

By the way, it was quite clever what you did to be able to use further powers of 2 (minus 1) in the future, although hopefully we will not need to jump another entire factor of 2 (!) and we can just e.g. add 100 more or something. I tried to come with something easier to read assuming we don't need to further increase in powers of 2, e.g. using a close value like 500 and then add the remainder:

#define TIMES_10_(x) x##x##x##x##x##x##x##x##x##x
#define TIMES_10(x)  TIMES_10_(x)

#define ADD_11_(x)   x ## abcde ## abcde ## a
#define ADD_11(x)    ADD_11_(x)

#define LONGEST_NAME ADD_11(TIMES_10(TIMES_10(abcde)))

But really, why don't we just include the symbol:

int
#include "longest_symbol"
;

We can keep the file static, or we can even generate it on-the-fly reading KSYM_NAME_LEN in a C file -- and then we don't need the macros nor to change them if the length changes.

[fbq]

Yay, hardcoded constants!

When I initially added the big symbol support, I grepped for 255, 256 etc., but I missed this one using 499 and 500.

Right, the current code style is horrible, I'm also considering sending the patch 1 directly to kernel community, so that we fix it there ;-)

Note that the selftest is still a POC, because the longest symbol is always compiled when KALLSYMS is enabled, which we probably don't want it.

Could we put it under DEBUG_KERNEL or perhaps DEBUG_MISC?

DEBUG_KERNEL may be too much, since the symbol is only used to test scripts/kallsyms.c, I think a separate kconfig will be better. Something like KALLSYMS_TEST maybe.

[fbq]

But really, why don't we just include the symbol:

int
#include "longest_symbol"
;

We can keep the file static, or we can even generate it on-the-fly reading KSYM_NAME_LEN in a C file -- and then we don't need the macros nor to change them if the length changes.

Good idea! Let me try that. And if I get it work, I probably submit patches directly to LKML, in that case I will copy you guys and close this PR.

[ojeda]

Right, the current code style is horrible, I'm also considering sending the patch 1 directly to kernel community, so that we fix it there ;-)

Please do so! Actually, we were told to send these patches (i.e. things that are improvements but "unrelated" to Rust, including the "big symbol support") independently, to start having the pieces needed in mainline already.

So if you want to do it, go ahead, otherwise I can do it too (with Co-developed-by or Suggested-by etc.).

DEBUG_KERNEL may be too much, since the symbol is only used to test scripts/kallsyms.c, I think a separate kconfig will be better. Something like KALLSYMS_TEST maybe.

That's fine too. I thought an entire Kconfig for that line would be too much, but perhaps it is a good idea since that way we already have it for possible future checks that we may add here and there.

Good idea! Let me try that. And if I get it work, I probably submit patches directly to LKML, in that case I will copy you guys and close this PR.

+1

(well, we should apply the patches here anyway, to avoid waiting to solve it -- then it will disappear in the diff when we merge mainline again into here).

[fbq]

Right, the current code style is horrible, I'm also considering sending the patch 1 directly to kernel community, so that we fix it there ;-)

Please do so! Actually, we were told to send these patches (i.e. things that are improvements but "unrelated" to Rust, including the "big symbol support") independently, to start having the pieces needed in mainline already.

So if you want to do it, go ahead, otherwise I can do it too (with Co-developed-by or Suggested-by etc.).

Feel free to do that ;-) Actually I think it's better that you do the upstream code of the kallsyms.c "fix" (i.e. patch 1 in this PR), because then you will know when it gets merged and adopt that in the sync with mainline. (no, not because I'm lazy :-))

DEBUG_KERNEL may be too much, since the symbol is only used to test scripts/kallsyms.c, I think a separate kconfig will be better. Something like KALLSYMS_TEST maybe.

That's fine too. I thought an entire Kconfig for that line would be too much, but perhaps it is a good idea since that way we already have it for possible future checks that we may add here and there.

Good idea! Let me try that. And if I get it work, I probably submit patches directly to LKML, in that case I will copy you guys and close this PR.

+1

(well, we should apply the patches here anyway, to avoid waiting to solve it -- then it will disappear in the diff when we merge mainline again into here).

Agreed. The remaining question is do we want to merge this PR as it is, or do we want to have a better test on longest name support. If it's the latter, maybe we only take the fix (i.e patch 1) in Rust-for-linux, and create a new issue tracking the upstream process of the longest name test?

[ojeda]

Yeah, I would put here the fix (patch 1) already, no reason not to, as well as the selftest too later on if we finish it.

So if you want, please drop the commit 2 for the moment, and we put commit 1 in.

[fbq]

Yeah, I would put here the fix (patch 1) already, no reason not to, as well as the selftest too later on if we finish it.

So if you want, please drop the commit 2 for the moment, and we put commit 1 in.

Done and create #504