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

[Namhyung Kim <namhyung@kernel.org>]

On Tue, Oct 06, 2026 at 11:57:34PM +0000, sashiko-bot@kernel.org wrote:
> 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?

This is a pre-existing bug.  And I think it should return a positive
number for success.

> 
> >  			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?

Correct, but this is also a separate issue.

Thanks,
Namhyung