Re: [PATCH v3] kallsyms: add names of built-in modules

Eugene Loh
2019-12-10T17:46:29Z

Thank you for the careful review. Sorry for the delayed response. I
will post a v4 momentarily. Further comments below.

On 11/22/2019 02:00 AM, Masahiro Yamada wrote:

> On Wed, Nov 20, 2019 at 2:02 PM <eugene.loh@oracle.com> wrote:
>> From: Eugene Loh <eugene.loh@oracle.com>
>>
>> /proc/kallsyms is very useful for tracers and other tools that need
>> to map kernel symbols to addresses.
>>
>> It would be useful if there were a mapping between kernel symbol and
>> module name that only changed when the kernel source code is changed.
> Unfortunately, this is not necessarily true.
> Some objects could be linked into multiple modules.

Good point but, as Nick pointed out, at least we can solve the common
case. I have added a remark to that effect to the commit message.

>> .gitignore | 1 +
>> Documentation/dontdiff | 1 +
>> Makefile | 41 ++-
>> kernel/kallsyms.c | 12 +-
>> scripts/Makefile.modbuiltin | 20 +-
>> scripts/kallsyms.c | 515 +++++++++++++++++++++++++++++++++++-
>> scripts/link-vmlinux.sh | 17 ++
>> scripts/namespace.pl | 5 +
>> 8 files changed, 589 insertions(+), 23 deletions(-)
> This diff-stat is unfortunate.
> scripts/kallsyms.c increased 65% for parsing
> .tmp_vmlinux.ranges and modules_think.builtin
>
> I tend to suspect the design mistake...

I have cleaned the changes up some and moved one portion, that can be
used for other purposes, into a separate file. So the delta on
scripts/kallsyms.c has been decreased some.

>> diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
>> @@ -69,10 +76,116 @@ static unsigned char best_table[256][2];
>> +static struct obj2mod_elem *obj2mod[OBJ2MOD_N];
>> +
>> +static void obj2mod_init(void)
>> +{
>> + memset(obj2mod, 0, sizeof(obj2mod));
>> +}
> Unneeded.
>
> The .bss section is automatically zero-cleared by
> operating system. obj2mod is already zero-filled.

Thanks. Change made.

>> +static void obj2mod_put(char *obj, int mod)
> you can add 'const' to the 'char *'.
> Same for obj2mod_get().

Thanks. Change made.

>> +static int addrmap_compare(const void *keyp, const void *rangep)
>> +{
>> + unsigned long long addr = *((const unsigned long long *)keyp);
>> + const struct addrmap_entry *range = (const struct addrmap_entry *)rangep;
> Cast is uneeded since rangep is an opaque pointer.

Thanks. Change made.

>> @@ -125,6 +252,16 @@ static int read_symbol(FILE *in, struct sym_entry *s)
>> + /* skip the .init.scratch section */
>> + if (strcmp(sym, "__init_scratch_end") == 0) {
>> + init_scratch = 0;
>> + goto read_another;
>> + }
>> + if (strcmp(sym, "__init_scratch_begin") == 0)
>> + init_scratch = 1;
>> + if (init_scratch)
>> + goto read_another;
> How is this hunk related?
> I do not understand it from the commit log.

I removed this section.

>> @@ -154,6 +291,14 @@ static int read_symbol(FILE *in, struct sym_entry *s)
>> else if (!strncmp(sym, ".LASANPC", 8))
>> return -1;
>>
>> + /* look up the builtin module this is part of (if any) */
>> + range = (struct addrmap_entry *) bsearch(&s->addr,
> Unneeded cast because bsearch() returns an opaque pointer.

Thanks. Change made.

>> @@ -454,6 +601,19 @@ static void write_src(void)
>> for (i = 0; i < 256; i++)
>> printf("\t.short\t%d\n", best_idx[i]);
>> printf("\n");
>> +
>> + output_label("kallsyms_modules");
>> + for (i = 0; i < builtin_module_len; i++)
>> + printf("\t.asciz\t\"%s\"\n", builtin_modules[i]);
>> + printf("\n");
> Output strings in plain text?
> Did you consider the possibility for compression?

As Nick pointed out, these names (one per module) only add a little
extra to the size.

>> int main(int argc, char **argv)
>> {
>> + const char *modules_builtin = "modules_thick.builtin";
>> +
>> if (argc >= 2) {
>> int i;
>> for (i = 1; i < argc; i++) {
>> - if(strcmp(argv[i], "--all-symbols") == 0)
>> + if (strcmp(argv[i], "--all-symbols") == 0)
>> all_symbols = 1;
>> else if (strcmp(argv[i], "--absolute-percpu") == 0)
>> absolute_percpu = 1;
>> else if (strcmp(argv[i], "--base-relative") == 0)
>> base_relative = 1;
>> + else if (strncmp(argv[i], "--builtin=", 10) == 0)
>> + modules_builtin = &argv[i][10];
>
> ".tmp_vmlinux.ranges" is hard-coded, but
> "modules_think.builtin" can be changed via option. Heh.

Good point. I removed the unneeded option.

>> diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
>> @@ -138,6 +140,19 @@ kallsyms()
>> + # read the linker map to identify ranges of addresses:
>> + # - for each *.o file, report address, size, pathname
>> + # - most such lines will have four fields
>> + # - but sometimes there is a line break after the first field
>> + # - start reading at "Linker script and memory map"
> Searching for "Linker script and memory map" will probably bring
> portability issue.
>
> llvm folks will be unhappy with it.

Actually, LLVM emits the same string.