Re: [PATCH 05/13] libdwfl [5/13]: introduce Dwfl_Process_Tracker
On Thu, Mar 20, 2025, at 8:01 AM, Mark Wielaard wrote:
> Hi Serhei,
>
> On Sun, 2025-03-16 at 19:14 -0400, Serhei Makarov wrote:
> I am wondering whether this isn't too abstract. libdwfl Dwfl's are
> already super abstract so they can be used for running processes and
> core dump and an offline or running kernel. Which is super powerful,
> but also slightly confusing at times.
>
> Can a Dwfl_Process_Tracker really take any callbacks and any Dwfl? Or
> would it make sense to limit it (as its name already implies) to Dwfls
> that report through dwfl_linux_proc_report with a given pid?
It's important to note that Dwfl_Process_Tracker collects multiple Dwfls that
report about processes with different pid's, for the system-wide profiling use
case.
That gives a way for these Dwfls to share an Elf data cache,
and query whether a Dwfl for a given pid has already been created.
It looks like I should adjust the comments to make that immediately obvious.
That's ok; for anything in the public headers, a lot of bikeshedding
is appropriate.
I don't see why it's necessarily tied to dwfl_linux_proc_report.
Any method of providing information about a live process would
work, including the profiler doing something custom with
the lower-level dwfl_report_... public functions. That might be
one way for sysprof to handle containers, for example,
without requiring us to reimplement their functionality
for peeking into container filesystems.
{One potentially confusing point about this api IMO comes later
in the patch series, where dwfl_perf_sample_getframes calls
dwfl_attach_state() if the Dwfl is unattached. I'm open to handling
it slightly differently, perhaps by splitting out the attach operation
into a separate function the profiler would need to call first.
I'll reiterate this in the discussion on patch12.}
[RFC v2] sketch of an unwinder cache interface for elfutils libdwfl
On Tue, Dec 10, 2024, at 4:42 PM, Serhei Makarov wrote: > This email sketches an 'unwinder cache' interface for libdwfl, derived > from recent eu-stacktrace code [1] and based on Christian Hergert's > adaptation of eu-stacktrace as sysprof-live-unwinder [2]. The intent is > to remove the need for a lot of boilerplate code that would be > identical among profiling tools that use libdwfl to unwind perf_events > stack samples. But since this becomes a library design, it's in need of > feedback and bikeshedding. In advance of finishing up the Dwfl_Process_Tracker patchset (initial version currently under review), wanted to post an update summarizing the current api design. api summary === (1) Create a Dwfl_Process_Tracker that caches data from multiple Dwfls related to a system-wide profiling session: typedef struct Dwfl_Process_Tracker Dwfl_Process_Tracker; Dwfl_Process_Tracker *dwfl_process_tracker_begin (const Dwfl_Callbacks *callbacks); Dwfl *dwfl_begin_with_tracker (Dwfl_Process_Tracker *tracker); void dwfl_process_tracker_end (Dwfl_Process_Tracker *tracker); (2) Given a pid, check if a Dwfl has already been created for it and return that; if it hasn't been created, invoke the callback (which is expected to use dwfl_begin_with_tracker to create an appropriate Dwfl). Dwfl *dwfl_process_tracker_find_pid (Dwfl_Process_Tracker *tracker, pid_t pid, Dwfl *(*callback) (Dwfl_Process_Tracker *tracker, pid_t pid, void *arg), void *arg); (3) This is a variant of the dwfl_linux_proc_find_elf callback which first checks the Dwfl_Process_Tracker associated with the Dwfl, and reuses any previously-loaded Elf data; otherwise, it creates a new Elf using dwfl_linux_proc_find_elf. int dwfl_process_tracker_find_elf (Dwfl_Module *mod, void **userdata, const char *module_name, Dwarf_Addr base, char **file_name, Elf **) -> same arguments as dwfl_linux_proc_find_elf (4) Obtain a perf_events regs_mask suitable for unwinding on the specified architecture. Then, use the elfutils unwinder to unwind the stack sample data returned from perf_events: uint64_t dwfl_perf_sample_preferred_regs_mask (GElf_Half machine); int dwfl_perf_sample_getframes (Dwfl *dwfl, Elf *elf, pid_t pid, pid_t tid, const Dwarf_Word *regs, uint32_t n_regs, uint64_t perf_regs_mask, uint32_t abi, int (*callback) (Dwfl_Frame *state, void *arg), void *arg); NOTE -- based on patch-review feedback, I'm considering to add another api function that must be called on an unattached Dwfl prior to using dwfl_perf_sample_getframes: int dwfl_sample_attach (Dwfl *dwfl); Otherwise, the flow with dwfl_perf_sample_getframes() is too confusing; currently it calls dwfl_attach_state on an unattached Dwfl (with its own callbacks arg, hidden to the library user), but it can also be called on a Dwfl that was attached by a previous call to dwfl_perf_sample_getframes(). This 'just works' when used in the profiler code, but is too fiddly to explain what is and isn't permissible to do.
Re: [PATCH 2/3] src/readelf.c: Support concurrency for -w, --debug-dump
Hi - > Due to significant lock contention, running eu-readelf when configured > with --enable-thread-safety causes a major performance degradation. > Currently, concurrency may double the runtime of eu-readelf. > However I think this work should still be merged. --enable-thread-safety > is still marked as experimental and these patches facilitate testing > future improvements to lock contention. Experimental but being enabled in RHEL/Fedora, right? Have you considered making this patch set eu-readelf default to traditional serial mode? That way, no performance regression for normal users, but all the code is compiled in for experiments. - FChE
[PATCH 3/5] doc: Add elf_getbase.3
Signed-off-by: Aaron Merey
---
doc/Makefile.am | 3 ++-
doc/elf_getbase.3 | 55 +++
2 files changed, 57 insertions(+), 1 deletion(-)
create mode 100644 doc/elf_getbase.3
diff --git a/doc/Makefile.am b/doc/Makefile.am
index 0dec1fc7..56c56b7b 100644
--- a/doc/Makefile.am
+++ b/doc/Makefile.am
@@ -60,7 +60,8 @@ notrans_dist_man3_MANS= elf_update.3 \
elf64_checksum.3 \
libelf.3 \
elf_end \
- elf_fill
+ elf_fill \
+ elf_getbase
# libdebuginfod man pages (also notrans)
# Note we include them even when not building them because we want
diff --git a/doc/elf_getbase.3 b/doc/elf_getbase.3
new file mode 100644
index ..1934e1ac
--- /dev/null
+++ b/doc/elf_getbase.3
@@ -0,0 +1,55 @@
+.TH ELF_GETBASE 3 2025-03-31 "Libelf" "Libelf Programmer's Manual"
+
+.SH NAME
+elf_getbase \- Retrieve the base offset for an ELF object file.
+
+.SH SYNOPSIS
+.B #include
+
+.BI "int64_t elf_getbase(Elf *" elf ");"
+
+.SH DESCRIPTION
+.B elf_getbase
+returns the file offset of the first byte of the ELF descriptor
+.IR elf .
+If
+.I elf
+is a member of an archive and has ELF kind
+.BR ELF_K_AR ,
+the base offset is the offset of the ELF object within the archive.
+For other ELF object types the base offset is 0.
+
+.SH PARAMETERS
+.TP
+.I elf
+The ELF descriptor.
+
+.SH RETURN VALUE
+Return the base offset of
+.IR elf .
+If
+.I elf
+is NULL, return -1.
+
+.SH SEE ALSO
+.BR libelf (3),
+.BR elf (5)
+
+.SH ATTRIBUTES
+For an explanation of the terms used in this section, see
+.BR attributes (7).
+
+.TS
+allbox;
+lbx lb lb
+l l l.
+Interface Attribute Value
+T{
+.na
+.nh
+.BR elf_getbase ()
+T} Thread safety MT-Safe
+.TE
+
+.SH REPORTING BUGS
+Report bugs to or
https://sourceware.org/bugzilla/.
--
2.49.0
Re: [PATCH 1/1] debuginfod: add --http-addr option
Hi Michael,
On Thu, 2025-03-13 at 12:05 +0100, Michael Trapp wrote:
> Use MHD_OPTION_SOCK_ADDR to bind the http listen socket to a single address.
> The address can be any IPv4 or IPv6 address configured on the system:
> --http-addr=127.0.0.1
> --http-addr=::1
> --http-addr='ANY_ACTIVE_LOCAL_IP_ADDRESS'
> As debuginfod does not include any security features, a listen on the
> localhost address is sufficient for a HTTP/HTTPS reverse-proxy setup.
I like the idea behind this patch. But is the option name clear enough?
Should it maybe be --bind-address= or --listen-address= simply --addr=
like it is --port=?
Or even combine it as --listen=addr:port ?
Does it make sense to allow multiple local addresses to bind to? Like
both 127.0.01 and ::1 ? I guess maybe not because if you are using this
you probably are behind a proxy anyway. So maybe it could simply be --
listen-local and debuginfod figures out whether that is ipv4
(127.0.0.1) and/or ipv6 (::1) ?
I don't know what other programs do, can we follow some semi-standard?
The code itself does look ok, although I think it could be simplified a
little if we go for something like --listen-local only (assuming that
makes sense).
Cheers,
Mark
> ---
> debuginfod/debuginfod.cxx | 115 ++
> doc/debuginfod.8 | 5 ++
> 2 files changed, 84 insertions(+), 36 deletions(-)
>
> diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
> index 0edd57cb..30916093 100644
> --- a/debuginfod/debuginfod.cxx
> +++ b/debuginfod/debuginfod.cxx
> @@ -81,6 +81,7 @@ extern "C" {
> #include
> #include
> #include
> +#include
>
>
> /* If fts.h is included before config.h, its indirect inclusions may not
> @@ -481,6 +482,8 @@ static const struct argp_option options[] =
> #define ARGP_KEY_METADATA_MAXTIME 0x100C
> { "metadata-maxtime", ARGP_KEY_METADATA_MAXTIME, "SECONDS", 0,
> "Number of seconds to limit metadata query run time, 0=unlimited.", 0 },
> +#define ARGP_KEY_HTTP_ADDR 0x100D
> + { "http-addr", ARGP_KEY_HTTP_ADDR, "ADDR", 0, "HTTP address to listen
> on.", 0 },
> { NULL, 0, NULL, 0, NULL, 0 },
>};
>
> @@ -512,6 +515,8 @@ static volatile sig_atomic_t sigusr1 = 0;
> static volatile sig_atomic_t forced_groom_count = 0;
> static volatile sig_atomic_t sigusr2 = 0;
> static unsigned http_port = 8002;
> +static struct sockaddr_in6 http_sockaddr;
> +static string addr_info = "";
> static bool webapi_cors = false;
> static unsigned rescan_s = 300;
> static unsigned groom_s = 86400;
> @@ -753,6 +758,16 @@ parse_opt (int key, char *arg,
>requires_koji_sigcache_mapping = true;
>break;
> #endif
> +case ARGP_KEY_HTTP_ADDR:
> + if (inet_pton(AF_INET, arg,
> &(((sockaddr_in*)&http_sockaddr)->sin_addr)) == 1)
> + http_sockaddr.sin6_family = AF_INET;
> + else
> + if (inet_pton(AF_INET6, arg, &http_sockaddr.sin6_addr) == 1)
> + http_sockaddr.sin6_family = AF_INET6;
> + else
> + argp_failure(state, 1, EINVAL, "HTTP address");
> + addr_info = arg;
> + break;
>// case 'h': argp_state_help (state, stderr,
> ARGP_HELP_LONG|ARGP_HELP_EXIT_OK);
> default: return ARGP_ERR_UNKNOWN;
> }
> @@ -5596,6 +5611,8 @@ main (int argc, char *argv[])
>fdcache_prefetch = 64; // guesstimate storage is this much less costly
> than re-decompression
>
>/* Parse and process arguments. */
> + memset(&http_sockaddr, 0, sizeof(http_sockaddr));
> + http_sockaddr.sin6_family = AF_UNSPEC;
>int remaining;
>(void) argp_parse (&argp, argc, argv, ARGP_IN_ORDER, &remaining, NULL);
>if (remaining != argc)
> @@ -5702,50 +5719,75 @@ main (int argc, char *argv[])
> #endif
> | MHD_USE_DEBUG); /* report errors to stderr */
>
> - // Start httpd server threads. Use a single dual-homed pool.
> - MHD_Daemon *d46 = MHD_start_daemon (mhd_flags, http_port,
> - NULL, NULL, /* default accept policy */
> - handler_cb, NULL, /* handler callback */
> - MHD_OPTION_EXTERNAL_LOGGER,
> - error_cb, NULL,
> - MHD_OPTION_THREAD_POOL_SIZE,
> - (int)connection_pool,
> - MHD_OPTION_END);
> -
> - MHD_Daemon *d4 = NULL;
> - if (d46 == NULL)
> + MHD_Daemon *dsa = NULL,
> + *d4 = NULL,
> + *d46 = NULL;
> +
> + if (http_sockaddr.sin6_family != AF_UNSPEC)
> {
> - // Cannot use dual_stack, use ipv4 only
> - mhd_flags &= ~(MHD_USE_DUAL_STACK);
> - d4 = MHD_start_daemon (mhd_flags, http_port,
> - NULL, NULL, /* default accept policy */
> + if (http_sockaddr.sin6_family == AF_INET)
> + ((sockaddr_in*)&http_sockaddr)->sin_port = htons(http_port);
> + if (http_sockaddr.sin6_fami
