| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAP-5=fX+zw-CUEYEtrPA6vYUKqpp4LZp-p==QsbZaNwLcX0Whw@mail.gmail.com>
Date: Tue, 13 Jan 2026 16:45:50 -0800
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, James Clark <james.clark@...aro.org>,
Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>,
Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...nel.org>,
LKML <linux-kernel@...r.kernel.org>, linux-perf-users@...r.kernel.org
Subject: Re: [PATCH] perf tools: Get debug info of DSO properly
On Tue, Jan 13, 2026 at 3:37 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> The dso__debuginfo() just used the path name to open the file but it may
> be outdated. It should check build-ID and use the file in the build-ID
> cache if available rather than just using the path name.
>
> Let's factor out dso__get_filename() to avoid code duplicate.
This change lgtm:
Reviewed-by: Ian Rogers <irogers@...gle.com>
I should have recognized this earlier, we seem to be getting multiple
copies of the dwfl and I wonder repeatedly parsing the debug info has
costs:
1) for arch powerpc there is a dwfl used in
tools/perf/arch/powerpc/util/skip-callchain-idx.c
2) there is a dwfl for the libdw addr2line
3) there is a dwfl in debuginfo
4) there is a dwfl in unwind_info in unwind-libdw
So 1 and 2 both associate the dwfl with the dso and lazily add it, 1
leaks the dwfl whereas 2 ties it to the lifetime of the dso.
For 3 there are individual debuginfo uses as well as a debuginfo cache
in probe-event.
For 4 the lifetime is an unwind of a stacktrace by unwind__get_entries.
1 and 4 load an ELF with a path using dwfl_report_elf and specifying
a base load address.
2 and 3 use dwfl_report_offline with no load address.
My understanding is that if you have the Dwfl the Dwfl_module and
Dwarf are cheap to compute. The module is cached in the Dwfl and with
a single Dwfl/Dwfl_module per dso then there should only ever be 1
value:
https://github.com/cuviper/elfutils/blob/master/libdwfl/libdwflP.h#L131
And the Dwarf is held in the module:
https://github.com/cuviper/elfutils/blob/master/libdwfl/libdwflP.h#L195
This makes me think the debuginfo may be holding onto more values than
it needs to and perhaps we can simplify it.
I think ideally we'd have a single Dwfl but that means the
Dwfl_modules will need different addresses to avoid overlapping. It
doesn't seem to be possible to relocate a module once it is loaded
(e.g with dwfl_module_relocate_address -
https://github.com/cuviper/elfutils/blob/master/libdwfl/libdwfl.h#L397)
and so we'd need some way to come up with an address to keep the
modules separate (we couldn't just use the virtual address for a
thread as the module address may differ per thread in different
processes).
So I wonder if we should have a single global Dwfl, and have each dso
have a made up load address to get the Dwfl_Module from it - the dso
could hold the Dwfl_Module* or the made up load address. 1 and 4 can
use their current base load address and adjust it by the made up load
address that would be associated with the dso. 2 and 3 can just use
the made up load address with the dso to get the module and possibly
with (2) remove some of the bias logic. This should speed up a bunch
of things by avoiding unnecessary/repeated dwfl_report_elf and
dwfl_report_offline calls, it would also cut down on duplication.
Thanks,
Ian
> Fixes: 53a61a6ca279165d ("perf annotate: Add dso__debuginfo() helper")
> Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> ---
> tools/perf/util/dso.c | 63 ++++++++++++++++++++++++++++++++-----------
> tools/perf/util/dso.h | 11 ++------
> 2 files changed, 50 insertions(+), 24 deletions(-)
>
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 06980844c014ec9b..922fcc2f798baa57 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -112,7 +112,7 @@ bool dso__is_object_file(const struct dso *dso)
>
> int dso__read_binary_type_filename(const struct dso *dso,
> enum dso_binary_type type,
> - char *root_dir, char *filename, size_t size)
> + const char *root_dir, char *filename, size_t size)
> {
> char build_id_hex[SBUILD_ID_SIZE];
> int ret = 0;
> @@ -564,20 +564,15 @@ char *dso__filename_with_chroot(const struct dso *dso, const char *filename)
> return filename_with_chroot(nsinfo__pid(dso__nsinfo_const(dso)), filename);
> }
>
> -static int __open_dso(struct dso *dso, struct machine *machine)
> - EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)
> +static char *dso__get_filename(struct dso *dso, const char *root_dir,
> + bool *decomp)
> {
> - int fd = -EINVAL;
> - char *root_dir = (char *)"";
> char *name = malloc(PATH_MAX);
> - bool decomp = false;
>
> - if (!name)
> - return -ENOMEM;
> + *decomp = false;
>
> - mutex_lock(dso__lock(dso));
> - if (machine)
> - root_dir = machine->root_dir;
> + if (name == NULL)
> + return NULL;
>
> if (dso__read_binary_type_filename(dso, dso__binary_type(dso),
> root_dir, name, PATH_MAX))
> @@ -602,20 +597,38 @@ static int __open_dso(struct dso *dso, struct machine *machine)
> size_t len = sizeof(newpath);
>
> if (dso__decompress_kmodule_path(dso, name, newpath, len) < 0) {
> - fd = -(*dso__load_errno(dso));
> + errno = *dso__load_errno(dso);
> goto out;
> }
>
> - decomp = true;
> + *decomp = true;
> strcpy(name, newpath);
> }
> + return name;
> +
> +out:
> + free(name);
> + return NULL;
> +}
>
> - fd = do_open(name);
> +static int __open_dso(struct dso *dso, struct machine *machine)
> + EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)
> +{
> + int fd = -EINVAL;
> + char *name;
> + bool decomp = false;
> +
> + mutex_lock(dso__lock(dso));
> +
> + name = dso__get_filename(dso, machine ? machine->root_dir : "", &decomp);
> + if (name)
> + fd = do_open(name);
> + else
> + fd = -errno;
>
> if (decomp)
> unlink(name);
>
> -out:
> mutex_unlock(dso__lock(dso));
> free(name);
> return fd;
> @@ -1912,3 +1925,23 @@ const u8 *dso__read_symbol(struct dso *dso, const char *symfs_filename,
> return __dso__read_symbol(dso, symfs_filename, start, len,
> out_buf, out_buf_len, is_64bit);
> }
> +
> +struct debuginfo *dso__debuginfo(struct dso *dso)
> +{
> + char *name;
> + bool decomp = false;
> + struct debuginfo *dinfo = NULL;
> +
> + mutex_lock(dso__lock(dso));
> +
> + name = dso__get_filename(dso, "", &decomp);
> + if (name)
> + dinfo = debuginfo__new(name);
> +
> + if (decomp)
> + unlink(name);
> +
> + mutex_unlock(dso__lock(dso));
> + free(name);
> + return dinfo;
> +}
> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> index 4aee23775054cf83..3dd7d0fc00797f37 100644
> --- a/tools/perf/util/dso.h
> +++ b/tools/perf/util/dso.h
> @@ -777,7 +777,7 @@ int dso__kernel_module_get_build_id(struct dso *dso, const char *root_dir);
>
> char dso__symtab_origin(const struct dso *dso);
> int dso__read_binary_type_filename(const struct dso *dso, enum dso_binary_type type,
> - char *root_dir, char *filename, size_t size);
> + const char *root_dir, char *filename, size_t size);
> bool is_kernel_module(const char *pathname, int cpumode);
> bool dso__needs_decompress(struct dso *dso);
> int dso__decompress_kmodule_fd(struct dso *dso, const char *name);
> @@ -926,14 +926,7 @@ u64 dso__findnew_global_type(struct dso *dso, u64 addr, u64 offset);
> bool perf_pid_map_tid(const char *dso_name, int *tid);
> bool is_perf_pid_map_name(const char *dso_name);
>
> -/*
> - * In the future, we may get debuginfo using build-ID (w/o path).
> - * Add this helper is for the smooth conversion.
> - */
> -static inline struct debuginfo *dso__debuginfo(struct dso *dso)
> -{
> - return debuginfo__new(dso__long_name(dso));
> -}
> +struct debuginfo *dso__debuginfo(struct dso *dso);
>
> const u8 *dso__read_symbol(struct dso *dso, const char *symfs_filename,
> const struct map *map, const struct symbol *sym,
> --
> 2.52.0.457.g6b5491de43-goog
>
Powered by blists - more mailing lists