Re: [PATCH] perf: Extract is_mapping_symbol() helper for mapping symbol filtering

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Destructive empty map insertion in `machine__process_ksymbol_register()` corrupts the kernel map tree.
- [High] Over-broad filtering unconditionally drops valid user-space and JIT symbols.
--

commit ae672508d7c2692194bb670cadab94e218fa13f7
Author: Rui Qi <qirui.001@bytedance.com>

perf: Extract is_mapping_symbol() helper for mapping symbol filtering

This commit extracts ad-hoc ELF mapping symbol filtering logic into a unified
is_mapping_symbol() helper in symbol.h. It replaces architecture-specific
checks with a single check that ignores any symbol starting with '$', '.L', or
'L0', and applies it across multiple symbol loading paths.

> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -769,6 +769,12 @@ static int machine__process_ksymbol_register(struct machine *machine,
>  		dso = dso__get(map__dso(map));
>  	}
>  
> +	/* Ignore mapping symbols in ksymbol events */
> +	if (is_mapping_symbol(event->ksymbol.name)) {
> +		err = 0;
> +		goto out;
> +	}
> +

Could this early return corrupt the kernel map tree? If a new map was just
allocated and inserted via maps__fixup_overlap_and_insert() earlier in this
function, taking this goto out leaves the empty, unpopulated map in the
rbtree. 

Since maps__fixup_overlap_and_insert() actively splits or deletes
existing overlapping maps, does this permanently punch a hole in the address
space layout?

Would it be safer to perform the is_mapping_symbol() check at the very
beginning of the function before any state mutations occur?

[ ... ]

> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -1592,20 +1592,9 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
>  		if (!is_label && !elf_sym__filter(&sym))
>  			continue;
>  
> -		/* Reject ARM ELF "mapping symbols": these aren't unique and
> -		 * don't identify functions, so will confuse the profile
> -		 * output: */
> -		if (ehdr.e_machine == EM_ARM || ehdr.e_machine == EM_AARCH64) {
> -			if (elf_name[0] == '$' && strchr("adtx", elf_name[1])
> -			    && (elf_name[2] == '\0' || elf_name[2] == '.'))
> -				continue;
> -		}
> -
> -		/* Reject RISCV ELF "mapping symbols" */
> -		if (ehdr.e_machine == EM_RISCV) {
> -			if (elf_name[0] == '$' && strchr("dx", elf_name[1]))
> -				continue;
> -		}
> +		/* Ignore mapping symbols in ELF files */
> +		if (is_mapping_symbol(elf_name))
> +			continue;

The previous logic carefully restricted filtering to exact ARM/RISC-V mapping
prefixes (like '$a', '$d', '$x') on those specific architectures. 

Will unconditionally dropping all symbols starting with '$' across all
architectures silently discard legitimate user-space functions? GCC permits
'$' in identifiers, and it is frequently used in user-space code.

[ ... ]

> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1566,6 +1566,10 @@ static int dso__load_perf_map(const char *map_path, struct dso *dso)
>  		if (len + 2 >= line_len)
>  			continue;
>  
> +		/* Ignore mapping symbols in perf map */
> +		if (is_mapping_symbol(line + len))
> +			continue;
> +

Similarly, does applying this kernel-centric heuristic here discard valid
symbols for user-space JIT environments? The '$' character is a valid and
common prefix for function names in languages like Java and Scala.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260504090609.1801880-1-qirui.001@bytedance.com?part=1