Re: [PATCH v4 3/5] perf symbols: Factor out duplicate symbol selection

[Alireza Haghdoost]

On Fri, Oct 2, 2026 at 3:16 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> On Fri, Oct 2, 2026 at 11:46 AM Alireza Haghdoost via B4 Relay
> <devnull+haghdoost.uber.com@xxxxxxxxxx> wrote:
> >
> > From: Alireza Haghdoost <haghdoost@xxxxxxxx>
> >
> > symbols__fixup_duplicate() chooses between symbols with the same start
> > address through choose_best_symbol(), which needs fully constructed
> > struct symbol objects. The lazy symbol loader added later in this series
> > selects among aliases from its index entries, before any struct symbol
> > exists, so it cannot use it.
> >
> > This patch moves the policy into symbol__choose_best(), which compares
> > the size, name, type and binding of two candidates described by struct
> > symbol_candidate, and passes the same description to the
> > arch__choose_best_symbol() hook. choose_best_symbol() becomes a wrapper
> > that describes two struct symbols. No functional change intended.
> >
> > symbol__choose_best() is not static so that the lazy loader can call it.
> > struct symbol_candidate stays in symbol.h because powerpc overrides the
> > weak arch__choose_best_symbol(), which takes it.
>
> Could we have symbol__choose_best() that takes struct symbol
> arguments? The struct symbol_candidate doesn't appear to add anything
> other than a cache of symbol values, and these could be as well cached
> in local variables.
>
> Thanks,
> Ian
>

The lazy caller is the reason it cannot take struct symbol. In patch 4/5,
sym_idx__dedup_aliases() (tools/perf/util/symbol-elf.c:1765-1808) selects
aliases while building the compact index, before any struct symbol exists.
Creating temporary symbols would require allocations and name copies because
struct symbol embeds its name.

struct symbol_candidate is a non-owning view of the four attributes shared
by the eager struct-symbol path and the lazy index path, including the
powerpc hook. Without it, the common helper would need eight scalar
arguments. Would a different name for the helper or struct make that intent
clearer?

Thanks,
Alireza