Re: [PATCH 41/48] objtool/klp: Rewrite symbol correlation algorithm

[Song Liu]

On Thu, Apr 30, 2026 at 3:53 PM Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
>
> On Tue, Apr 28, 2026 at 09:50:46PM +0100, Song Liu wrote:
> > On Tue, Apr 28, 2026 at 5:23 PM Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> > [...]
> > > > Also a few nitpicks below.
> > > >
> > > > > Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> > > > > ---
> > > > [...]
> > > > > +static struct symbol *find_twin(struct elfs *e, struct symbol *sym1)
> > > > > +{
> > > > > +       struct symbol *name_last = NULL, *scope_last = NULL,
> > > > > +                     *file_last = NULL, *csum_last = NULL;
> > > > > +       unsigned int name_orig = 0, name_patched = 0;
> > > > > +       unsigned int scope_orig = 0, scope_patched = 0;
> > > > > +       unsigned int file_orig = 0, file_patched = 0;
> > > > > +       unsigned int csum_orig = 0, csum_patched = 0;
> > > > > +       struct symbol *sym2, *match = NULL;
> > > > > +
> > > > > +       /* Count orig candidates */
> > > > > +       for_each_sym_by_demangled_name(e->orig, sym1->demangled_name, sym2) {
> > > > > +               if (sym2->twin || sym1->type != sym2->type || dont_correlate(sym2) ||
> > > > > +                   (!maybe_same_file(sym1, sym2)))
> > > > >                         continue;
> > > > >
> > > > > -               count++;
> > > > > -               result = sym2;
> > > > > +               /* Level 1: name match (widest filter)  */
> > > > > +               name_orig++;
> > > > > +
> > > > > +               /* Level 2: scope (scope changes allowed) */
> > > > > +               if (is_tu_local_sym(sym1) != is_tu_local_sym(sym2))
> > > >
> > > > is_tu_local_sym(sym1) is called many times, shall we add a variable
> > > > for it?
> > >
> > > Unless it's actually a performance issue I'd prefer not to add yet
> > > another bit to struct symbol.
> >
> > We don't need to add it to struct symbol, a local variable for sym1
> > will be good. No need for sym2.
> >
> > IOW, we have something like
> >
> > bool sym1_is_local = is_tu_local_sym(sym1);
> > ...
> > if (sym1_is_local != is_tu_local_sym(sym2))
>
> I'd rather keep it the way it is for readability, the compiler should be
> able to recognize the sym1 value doesn't change and calculate its value
> once.

Agreed for readability.

We can always optimize it in case this becomes a real slowdown.

Thanks,
Song