Re: [PATCH 3/4] perf script: Add --lazy-load-symbols for lazy symbol loading

[Alireza Haghdoost]

> > +static void dso__clip_ondemand_symbols_at(struct dso *dso, u64 addr)
> > +{
> > +     struct dso_ondemand *od = dso__ondemand(dso);
> > +     struct sym_idx *sym;
> > +     u32 lo = 0, hi, mid;
> > +
> > +     if (!od)
> > +             return;
> > +
> > +     hi = od->nr_sorted;
> > +     while (lo < hi) {
> > +             mid = (lo + hi) / 2;
> > +             if (od->sorted[mid].start < addr)
> > +                     lo = mid + 1;
> > +             else
> > +                     hi = mid;
> > +     }
>
> Why not use bsearch()?
>

This search is a lower-bound (bisect_left) operation. I could not find
an ISO C or glibc API that provides that operation.
In patch series v2, I will refactor this search into a small
sym_idx__lower_bound() helper. The clipping helper (symbol-elf.c:626)
and the exact-start IRELATIVE lookup (symbol-elf.c:2176) will use it,
while ordinary non-overlapping interval lookups will use bsearch().


> >  int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss)
> >  {
> >       uint32_t idx;
> > @@ -623,6 +650,8 @@ int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss)
> >       if (!elf_section_by_name(elf, &ehdr, &shdr_plt, ".plt", NULL))
> >               return 0;
> >
> > +     dso__clip_ondemand_symbols_at(dso, shdr_plt.sh_offset);
>
> Why is this needed?
>

Lazy address lookup uses the index, not the rb-tree. Without clipping
the index at .plt, a preceding fill-forward range can claim PLT IPs
before dso__synthesize_plt_symbols() inserts the PLT symbols. This is
the same truncation the eager path already does on the rb-tree
(symbol-elf.c:680). I will add that explanation at the call site in
v2.

> > +
> > +     /* Alias dedup: keep only the best symbol for each start address */
> > +     if (!symbol_conf.allow_aliases) {
>
> Probably better to factor out the dedup logic into a function.
>

I will factor out the duplicate-selection policy into
symbol__choose_best() and use it in both the eager and lazy paths. The
surrounding dedup loops remain separate because the eager path uses
rb-tree struct symbol objects while the lazy path uses compact index
entries before materialization.

> > +             u32 out = 0;
> > +
> > +             for (i = 0; i < count; i++) {
> > +                     u32 best = i;
> > +                     const char *na = NULL, *nb;
> > +                     char *da = NULL, *db;
> > +
> > +                     /* name_off is the strtab index (st_name) */
> > +                     na = elf_strptr(elf, shdr.sh_link,
> > +                                     od->sorted[best].name_off);
> > +                     if (na) {
> > +                             da = dso__demangle_sym(dso, 0, na);
> > +                             if (da)
> > +                                     na = da;
> > +                     }
> > +
> > +                     /* Find the best among all entries with this start */
> > +                     for (j = i + 1; j < count &&
> > +                          od->sorted[j].start == od->sorted[i].start; j++) {
> > +                             nb = elf_strptr(elf, shdr.sh_link,
> > +                                             od->sorted[j].name_off);
> > +                             if (!na || !nb)
> > +                                     continue;
> > +
> > +                             /* Demangle for comparison, matching eager path */
> > +                             db = dso__demangle_sym(dso, 0, nb);
> > +                             if (db)
> > +                                     nb = db;
> > +
> > +                             /* od->sorted[].end holds st_size at this point */
> > +                             if (choose_best_symbol_raw(
> > +                                         od->sorted[best].end,
> > +                                         od->sorted[best].type,
> > +                                         od->sorted[best].binding, na,
> > +                                         od->sorted[j].end,
> > +                                         od->sorted[j].type,
> > +                                         od->sorted[j].binding, nb) == SYMBOL_B) {
> > +                                     best = j;
> > +                                     free(da);
> > +                                     da = db;
> > +                                     na = nb;
> > +                             } else {
> > +                                     free(db);
> > +                             }
> > +                     }
> > +
> > +                     free(da);
> > +                     od->sorted[out++] = od->sorted[best];
> > +                     i = j - 1; /* skip past all aliases of this start */
> > +             }
> > +
> > +             if (out < count) {
> > +                     struct sym_idx *shrunk;
> > +
> > +                     shrunk = realloc(od->sorted, out * sizeof(*od->sorted));
> > +                     if (shrunk) {
> > +                             od->sorted = shrunk;
> > +                             od->nr_alloc = out;
> > +                     }
> > +             }
> > +             count = out;
> > +     }
> > +
> > +     /* Convert st_size to end addresses */
> > +     for (i = 0; i < count; i++) {
> > +             u64 size = od->sorted[i].end; /* was st_size */
> > +
> > +             if (size > 0)
> > +                     od->sorted[i].end = od->sorted[i].start + size;
> > +             else if (i + 1 < count)
> > +                     od->sorted[i].end = od->sorted[i + 1].start;
> > +             else
> > +                     /* Match symbols__fixup_end's last-symbol formula. */
> > +                     od->sorted[i].end = roundup(od->sorted[i].start, 4096) + 4096;
> > +     }
> > +
> > +     /*
> > +      * Keep a private fd open for pread() of symbol names.  Dup with
> > +      * O_CLOEXEC so children don't inherit it, and so that
> > +      * symsrc__destroy() can close the original regardless of whether
> > +      * it is a real file or a temporary debugdata extraction.
> > +      *
> > +      * If the dup fails (e.g. fd exhaustion), decline by returning 0
> > +      * without setting the index: the caller falls back to the eager
> > +      * loader so the DSO still gets symbols rather than going symbol-less.
> > +      */
> > +     od->fd = fcntl(syms_ss->fd, F_DUPFD_CLOEXEC, 0);
> > +     if (od->fd < 0) {
> > +             free(od->sorted);
> > +             free(od);
> > +             return 0;
> > +     }
> > +     od->strtab_offset = strtab_offset;
> > +     od->strtab_size = strshdr.sh_size;
> > +     od->nr_sorted = count;
> > +
> > +     symbol__account_bytes(od->nr_alloc * sizeof(*od->sorted));
> > +
> > +     dso__set_ondemand(dso, od);
> > +
> > +     pr_debug("%s: on-demand index: %u symbols\n",
> > +              dso__long_name(dso), count);
> > +
> > +     return 1;
> > +}
> > +
> > +/*
> > + * Read a NUL-terminated symbol name from the file's string table at
> > + * file offset @off.  The fast path uses a stack buffer; if the NUL is
> > + * not found within it (names can exceed 1 KiB for template-heavy C++
> > + * mangled names), grow a heap buffer geometrically from 4 KiB, doubling
> > + * until the terminator is found or the strtab is exhausted.  This keeps
> > + * a single long name cheap even when it sits near the start of a large
> > + * (tens of MB) strtab, while bounding a corrupt/missing terminator by
> > + * the remaining strtab size.
> > + *
> > + * Returns a pointer to the name (either @buf or a heap allocation) and
> > + * sets *@to_free to the buffer that must be free()d (NULL for @buf).
> > + * Returns NULL on read error or if no NUL terminator exists within the
> > + * strtab bounds.
> > + */
>
> We have dso-cache APIs to read file data (dso__data_read_offset) and it
> manages file descriptors so you don't need to worry about FD exhaustion.
>

Thanks, I will use dso__data_read_offset() in v2 and remove the
per-index retained fd. I will configure the data-cache reads with the
same symbol-file path and namespace used to build the index, so
split-debuginfo string-table offsets are not read from the runtime
image.


> > +struct symbol *dso__find_symbol_ondemand(struct dso *dso, u64 addr)
> > +{
> > +     struct dso_ondemand *od = dso__ondemand(dso);
> > +     u32 lo, hi, mid;
> > +     const char *name;
> > +     char namebuf[1024];
> > +     char *name_heap = NULL;
> > +     char *demangled;
> > +     struct symbol *s = NULL;
> > +
> > +     if (!od || !od->sorted || od->fd < 0)
> > +             return NULL;
> > +
> > +     lo = 0;
> > +     hi = od->nr_sorted;
> > +     while (lo < hi) {
> > +             mid = (lo + hi) / 2;
> > +             if (addr < od->sorted[mid].start)
> > +                     hi = mid;
> > +             else if (addr >= od->sorted[mid].end)
> > +                     lo = mid + 1;
> > +             else
> > +                     goto found;
> > +     }
>
> bsearch()?
>

Yes. In v2, the default deduplicated index has non-overlapping ranges,
so this lookup will use bsearch() with an interval comparator.
With --allow-aliases, two symbols can cover the same address.
bsearch() then cannot reliably choose which alias to return, so that
uncommon case keeps the explicit search.