Re: [PATCH v3 next 12/13] scripts/kallsyms: Reuse tokens that have no references

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 1592 bytes --]

> scripts/kallsyms: Reuse tokens that have no references

TL;DR: Nits only: sym_arr_free_all() in scripts/kallsyms.c has a misleading
name, since it only zeroes counts, and returns its argument needlessly. The
review suggests a void reset helper instead.

> diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
> --- a/scripts/kallsyms.c
> +++ b/scripts/kallsyms.c

[ ... ]

> @@ -720,6 +733,19 @@ static void token_add_symbol(unsigned int token_idx, unsigned int sym_idx)
>  	sym_arr_add(arr, sym_idx);
>  }
>  
> +static unsigned int sym_arr_free_all(unsigned int ch)
> +{
> +	unsigned int i;
> +
> +	/* Just zero the counts, the entries are about to be reused. */
> +	for (i = 0; i < 256; i++) {
> +		token_syms[token_index(i, ch)].cnt = 0;
> +		token_syms[token_index(ch, i)].cnt = 0;
> +	}
> +
> +	return ch;
> +}

This isn't a bug, but would a name like token_syms_reset_char() with a void
return be clearer here?

sym_arr_free_all() sits next to sym_arr_free(), which really frees a struct
sym_arr, but this function frees nothing and only clears ->cnt for the
token_syms[] pairs containing ch.

It also returns its argument unchanged just so the caller can write:

	to_process[idx++] = sym_arr_free_all(token_first(best));

Would a plain to_process[idx++] = token_first(best) at the call site, after a
separate reset call, read more directly?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/43116767394