Hi Josh, Thanks for the review!
On Mon, Feb 23, 2026 at 2:56 PM Josh Poimboeuf <[email protected]> wrote: > > On Thu, Feb 19, 2026 at 02:22:37PM -0800, Song Liu wrote: > > correlate_symbols will always try to match full name first. If there is no > > match, try match only demangled_name. > > In commit logs, please add "()" to function names, like > correlate_symbols(). Noted. > > > +++ b/tools/objtool/elf.c > > @@ -323,6 +323,19 @@ struct symbol *find_global_symbol_by_name(const struct > > elf *elf, const char *nam > > return NULL; > > } > > > > +void iterate_global_symbol_by_demangled_name(const struct elf *elf, > > + const char *demangled_name, > > + void (*process)(struct symbol > > *sym, void *data), > > + void *data) > > +{ > > + struct symbol *sym; > > + > > + elf_hash_for_each_possible(symbol_name, sym, name_hash, > > str_hash(demangled_name)) { > > + if (!strcmp(sym->demangled_name, demangled_name) && > > !is_local_sym(sym)) > > + process(sym, data); > > + } > > +} > > + > > I think a saner interface would be something like: > > struct symbol *find_global_demangled_symbol(const struct elf *elf, const char > *demangled_name) > { > struct symbol *ret = NULL; > > elf_hash_for_each_possible(symbol_name, sym, name_hash, > str_hash(demangled_name)) { > if (!is_local_sym(sym) && !strcmp(sym->demangled_name, > demangled_name)) { > if (ret) > return ERR_PTR(-EEXIST); > ret = sym; > } > } > > return ret; > } I had something similar to this initially. However, we need to check sym->twin, and skip symbols that already have correlations. For example, if we have foo.llvm.123 and foo.llvm.456 in the original kernel, and foo.llvm.123 and foo.llvm.789 in the patched kernel, we will match foo.llvm.456 to foo.llvm.789 without ambiguity. Since elf.c doesn't touch sym->twin at all, I think it is cleaner to keep this logic in klp-diff.c. If you think it is OK to have elf.c handle this, we can do something like: struct symbol *find_global_demangled_symbol(const struct elf *elf, const char *demangled_name) { struct symbol *ret = NULL; elf_hash_for_each_possible(symbol_name, sym, name_hash, str_hash(demangled_name)) { if (!is_local_sym(sym) && !strcmp(sym->demangled_name, demangled_name) && !sym->twin) { /* We need to add this */ if (ret) return ERR_PTR(-EEXIST); ret = sym; } } return ret; } I personally like v2 patch better. But I wouldn't mind changing to the above version in v3. > > @@ -453,8 +493,27 @@ static int correlate_symbols(struct elfs *e) > > continue; > > > > sym2 = find_global_symbol_by_name(e->patched, sym1->name); > > + if (sym2 && !sym2->twin) { > > + sym1->twin = sym2; > > + sym2->twin = sym1; > > + } > > + } > > + > > + /* > > + * Correlate globals with demangled_name. > > + * A separate loop is needed because we want to finish all the > > + * full name correlations first. > > + */ > > + for_each_sym(e->orig, sym1) { > > + if (sym1->bind == STB_LOCAL || sym1->twin) > > + continue; > > + > > + if (find_global_symbol_by_demangled_name(e->patched, sym1, > > &sym2)) > > + return -1; > > > > if (sym2 && !sym2->twin) { > > + WARN("correlate %s (original) to %s (patched)", > > + sym1->name, sym2->name); > > Since there's not actually any ambiguity at this point, do we actually > need a warning? I cannot think of a case where this match is ambiguous, so yes, we can remove this warning. Thanks, Song
