Re: [PATCH v2 05/10] perf symbol: Set dso symtab type for libbfd

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] perf: dso__load() returns 0 on libbfd success causing map__load() to discard first sample
- [Low] perf: unintended fallthrough to dso__load_sym clobbers libbfd symtab type
--

commit c31089cce4c931915ffad7363dd2a84096e2a4cf
Author: Namhyung Kim <namhyung@kernel.org>

perf symbol: Set dso symtab type for libbfd

It missed to set the dso symtab type when the symbols are loaded by
libbfd.  When it happens both syms_ss and runtime_ss may not be set
and won't call dso__load_sym().

> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index fc6e4dff282bc..02b096ccbc60a 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1933,6 +1933,7 @@ int dso__load(struct dso *dso, struct map *map)
>  			nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
>  
>  		if (bfdrc == 0) {
> +			dso__set_symtab_type(dso, symtab_type);
>  			ret = 0;

[Severity: High]
Does returning 0 here cause map__load() to discard the first sample?

Unlike dso__load_sym(), which returns the number of loaded symbols (> 0),
leaving ret = 0 causes dso__load() to return 0. The caller map__load()
strictly checks "if (nr == 0)" and treats it as a failure:

tools/perf/util/map.c:map__load() {
    ...
	} else if (nr == 0) {
    ...
		return -1;
	}

Because dso__load() correctly calls dso__set_loaded(dso) before returning,
subsequent calls to map__load() hit the early success path and return 0.
As a result, does the very first symbol lookup for a libbfd-loaded map
spuriously fail and cause perf to discard the sample?

>  			break;

[Severity: Low]
Does breaking out of the loop here unintentionally clobber the libbfd symtab
type if syms_ss was already set?

If a prior loop iteration populates syms_ss (e.g., from an ELF file) and a
subsequent iteration successfully loads libbfd symbols (bfdrc == 0), the
loop breaks here to return the libbfd symbols.

However, since syms_ss is non-NULL from the prior iteration, the post-loop
cleanup bypass fails:

tools/perf/util/symbol.c:dso__load() {
    ...
	if (!runtime_ss && !syms_ss)
		goto out_free;
    ...
	if (syms_ss)
		ret = dso__load_sym(dso, map, syms_ss, runtime_ss, kmod);
    ...
}

Because execution falls through to dso__load_sym(), does it unconditionally
call dso__set_symtab_type(dso, syms_ss->type) and silently overwrite the
newly added symtab_type, mixing the symbols?

>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006234315.920817-1-namhyung@kernel.org?part=5