Re: [PATCH v3 3/6] perf symbols: Factor out duplicate symbol selection

[Ian Rogers]

On Fri, Sep 25, 2026 at 12:15 PM 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. A loader that selects among aliases before
> allocating symbols cannot use it.
>
> Move the policy into symbol__choose_best(), which compares the size,
> name, type and binding of two candidates described by struct
> symbol_candidate, and pass 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.
>
> Signed-off-by: Alireza Haghdoost <haghdoost@xxxxxxxx>
> ---
>  tools/perf/arch/powerpc/util/sym-handling.c |  6 ++--
>  tools/perf/util/symbol.c                    | 43 +++++++++++++++++++++--------
>  tools/perf/util/symbol.h                    | 14 +++++++++-
>  3 files changed, 47 insertions(+), 16 deletions(-)
>
> diff --git a/tools/perf/arch/powerpc/util/sym-handling.c b/tools/perf/arch/powerpc/util/sym-handling.c
> index 947bfad7aa59..c263cbfefba5 100644
> --- a/tools/perf/arch/powerpc/util/sym-handling.c
> +++ b/tools/perf/arch/powerpc/util/sym-handling.c
> @@ -10,10 +10,10 @@
>  #include "probe-event.h"
>  #include "probe-file.h"
>
> -int arch__choose_best_symbol(struct symbol *syma,
> -                            struct symbol *symb __maybe_unused)
> +int arch__choose_best_symbol(const struct symbol_candidate *syma,
> +                            const struct symbol_candidate *symb __maybe_unused)
>  {
> -       char *sym = syma->name;
> +       const char *sym = syma->name;
>
>  #if !defined(_CALL_ELF) || _CALL_ELF != 2
>         /* Skip over any initial dot */
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 163652f071c6..4b50250d07fa 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -145,8 +145,8 @@ int __weak arch__compare_symbol_names_n(const char *namea, const char *nameb,
>         return strncmp(namea, nameb, n);
>  }
>
> -int __weak arch__choose_best_symbol(struct symbol *syma,
> -                                   struct symbol *symb __maybe_unused)
> +int __weak arch__choose_best_symbol(const struct symbol_candidate *syma,
> +                                   const struct symbol_candidate *symb __maybe_unused)
>  {
>         /* Avoid "SyS" kernel syscall aliases */
>         if (strlen(syma->name) >= 3 && !strncmp(syma->name, "SyS", 3))
> @@ -157,38 +157,39 @@ int __weak arch__choose_best_symbol(struct symbol *syma,
>         return SYMBOL_A;
>  }
>
> -static int choose_best_symbol(struct symbol *syma, struct symbol *symb)
> +int symbol__choose_best(const struct symbol_candidate *syma,
> +                       const struct symbol_candidate *symb)
>  {
>         s64 a;
>         s64 b;
>         size_t na, nb;
>
>         /* Prefer a symbol with non zero length */
> -       a = syma->end - syma->start;
> -       b = symb->end - symb->start;
> +       a = syma->size;
> +       b = symb->size;
>         if ((b == 0) && (a > 0))
>                 return SYMBOL_A;
>         else if ((a == 0) && (b > 0))
>                 return SYMBOL_B;
>
> -       if (symbol__type(syma) != symbol__type(symb)) {
> -               if (symbol__type(syma) == STT_NOTYPE)
> +       if (syma->type != symb->type) {
> +               if (syma->type == STT_NOTYPE)
>                         return SYMBOL_B;
> -               if (symbol__type(symb) == STT_NOTYPE)
> +               if (symb->type == STT_NOTYPE)
>                         return SYMBOL_A;
>         }
>
>         /* Prefer a non weak symbol over a weak one */
> -       a = symbol__binding(syma) == STB_WEAK;
> -       b = symbol__binding(symb) == STB_WEAK;
> +       a = syma->binding == STB_WEAK;
> +       b = symb->binding == STB_WEAK;
>         if (b && !a)
>                 return SYMBOL_A;
>         if (a && !b)
>                 return SYMBOL_B;
>
>         /* Prefer a global symbol over a non global one */
> -       a = symbol__binding(syma) == STB_GLOBAL;
> -       b = symbol__binding(symb) == STB_GLOBAL;
> +       a = syma->binding == STB_GLOBAL;
> +       b = symb->binding == STB_GLOBAL;
>         if (a && !b)
>                 return SYMBOL_A;
>         if (b && !a)
> @@ -213,6 +214,24 @@ static int choose_best_symbol(struct symbol *syma, struct symbol *symb)
>         return arch__choose_best_symbol(syma, symb);
>  }
>
> +static int choose_best_symbol(struct symbol *syma, struct symbol *symb)
> +{
> +       struct symbol_candidate a = {
> +               .size = syma->end - syma->start,
> +               .name = syma->name,
> +               .type = symbol__type(syma),
> +               .binding = symbol__binding(syma),
> +       };
> +       struct symbol_candidate b = {
> +               .size = symb->end - symb->start,
> +               .name = symb->name,
> +               .type = symbol__type(symb),
> +               .binding = symbol__binding(symb),
> +       };
> +
> +       return symbol__choose_best(&a, &b);
> +}
> +
>  void symbols__fixup_duplicate(struct rb_root_cached *symbols)
>  {
>         struct rb_node *nd;
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index 46b1649c64fc..b9fa722a9a14 100644
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
> @@ -299,10 +299,22 @@ const char *arch__normalize_symbol_name(const char *name);
>  #define SYMBOL_A 0
>  #define SYMBOL_B 1
>
> +/* Attributes used to choose between symbols that share a start address. */
> +struct symbol_candidate {
> +       u64             size;
> +       const char      *name;
> +       u8              type;
> +       u8              binding;
> +};
> +
> +int symbol__choose_best(const struct symbol_candidate *a,
> +                       const struct symbol_candidate *b);
> +
>  int arch__compare_symbol_names(const char *namea, const char *nameb);
>  int arch__compare_symbol_names_n(const char *namea, const char *nameb,
>                                  unsigned int n);
> -int arch__choose_best_symbol(struct symbol *syma, struct symbol *symb);
> +int arch__choose_best_symbol(const struct symbol_candidate *a,
> +                            const struct symbol_candidate *b);

Given you can change this function definition and only change 1 C
file, is there any reason not to make the function static in symbol.c
and move the struct symbol_candidate definition also to symbol.c?

Thanks,
Ian

>
>  enum symbol_tag_include {
>         SYMBOL_TAG_INCLUDE__NONE = 0,
>
> --
> Git-157)
>
>