Re: [PATCH v3 1/3] module/kallsyms: Rename is_mapping_symbol() to is_local_mapping_symbol()

[Petr Pavlu <petr.pavlu@suse.com>]

On 8/12/26 5:28 AM, Tiezhu Yang wrote:
> The helper function is_mapping_symbol() historically checks for both
> local labels prefixed with ".L" or "L0" and mapping symbols prefixed
> with "$".
> 
> Rename it to is_local_mapping_symbol() to better reflect this actual
> behavior and scope, preventing conceptual confusion.
> 
> No functional change.
> 
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>  include/linux/module_symbol.h | 4 ++--
>  kernel/module/kallsyms.c      | 2 +-
>  scripts/faddr2line            | 2 +-
>  scripts/mod/modpost.h         | 2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/module_symbol.h b/include/linux/module_symbol.h
> index 574609aced99..ca76ed5cb489 100644
> --- a/include/linux/module_symbol.h
> +++ b/include/linux/module_symbol.h
> @@ -7,8 +7,8 @@ enum ksym_flags {
>  	KSYM_FLAG_GPL_ONLY	= 1 << 0,
>  };
>  
> -/* This ignores the intensely annoying "mapping symbols" found in ELF files. */
> -static inline bool is_mapping_symbol(const char *str)
> +/* This ignores the intensely annoying "local or mapping symbols" found in ELF files. */
> +static inline bool is_local_mapping_symbol(const char *str)

I think the updated comment and the function name are confusing. They
read as if they are about local symbols in general (STB_LOCAL), but they
should refer only to local label symbols (.L<xyz>, ...). The function
would need to be called is_local_label_or_mapping_symbol() but that is
too long.

If you do want to rename this function, one option is to be consistent
with perf, which implements the same filter and calls it
is_ignored_kernel_symbol().

The comment could also be clearer, for example:

/*
 * Ignore local labels (.L*, L0*) and mapping symbols ($*). These symbols are
 * not useful for the kernel, for example, they should not appear in kallsyms.
 */

>  {
>  	if (str[0] == '.' && str[1] == 'L')
>  		return true;
> diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
> index f23126d804b2..a595f8cd29b2 100644
> --- a/kernel/module/kallsyms.c
> +++ b/kernel/module/kallsyms.c
> @@ -294,7 +294,7 @@ static const char *find_kallsyms_symbol(struct module *mod,
>  		 * and inserted at a whim.
>  		 */
>  		if (*kallsyms_symbol_name(kallsyms, i) == '\0' ||
> -		    is_mapping_symbol(kallsyms_symbol_name(kallsyms, i)))
> +		    is_local_mapping_symbol(kallsyms_symbol_name(kallsyms, i)))
>  			continue;
>  
>  		if (thisval <= addr && thisval > bestval) {
> diff --git a/scripts/faddr2line b/scripts/faddr2line
> index 622875396bcf..bda4ea20e39e 100755
> --- a/scripts/faddr2line
> +++ b/scripts/faddr2line
> @@ -243,7 +243,7 @@ __faddr2line() {
>  			local cur_sym_elf_size=${fields[2]}
>  			local cur_sym_name=${fields[7]:-}
>  
> -			# is_mapping_symbol(cur_sym_name)
> +			# is_local_mapping_symbol(cur_sym_name)
>  			if [[ ${cur_sym_name} =~ ^(\.L|L0|\$) ]]; then
>  				continue
>  			fi
> diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> index 2aecb8f25c87..4173fff2788a 100644
> --- a/scripts/mod/modpost.h
> +++ b/scripts/mod/modpost.h
> @@ -198,7 +198,7 @@ static inline bool is_valid_name(struct elf_info *elf, Elf_Sym *sym)
>  
>  	if (!name || !strlen(name))
>  		return false;
> -	return !is_mapping_symbol(name);
> +	return !is_local_mapping_symbol(name);
>  }
>  
>  /* symsearch.c */

Another comment-only reference to is_mapping_symbol() that should be
updated is in tools/perf/util/symbol.h.

-- 
Thanks,
Petr