Re: [PATCH 05/13] libdwfl [5/13]: introduce Dwfl_Process_Tracker
Hi Serhei,
On Sun, 2025-03-16 at 19:14 -0400, Serhei Makarov wrote:
> New data structure to coordinate caching Elf data among multiple Dwfl
> structs attached to different processes. Meant to reduce the overhead
> for profilers that use elfutils for unwinding.
>
> The caching is well-justified, as the prior approach (e.g. in
> eu-stacktrace, sysprof-live-unwinder) of creating a separate Dwfl per
> process was wildly redundant, opening ~hundreds of file descriptors
> just for each common library such as libc.so and leaking the data.
>
> Initial patch just introduces the struct, to be filled in by the rest
> of the patch series.
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?
If so we might be able to simplify the interface a little and make it
slightly more intuitive to use.
Cheers,
Mark
> * libdwfl/libdwfl.h (Dwfl_Process_Tracker): New struct.
> (dwfl_process_tracker_begin): New function.
> (dwfl_begin_with_tracker): New function.
> (dwfl_process_tracker_end): New function.
> * libdw/libdw.map: Add new functions.
> * libdwfl/libdwflP.h (struct Dwfl_Process_Tracker): New struct.
> (struct Dwfl): Add 'tracker' field.
> * libdwfl/Makefile.am (libdwfl_a_SOURCES): Add dwfl_process_tracker.c.
> * libdwfl/dwfl_process_tracker.c: New file.
> (dwfl_process_tracker_begin): Initialize the tracker.
> (dwfl_begin_with_tracker): Initialize Dwfl * with specified tracker.
> (dwfl_process_tracker_end): Deallocate the tracker.
> ---
> libdw/libdw.map| 3 ++
> libdwfl/Makefile.am| 3 +-
> libdwfl/dwfl_process_tracker.c | 66 ++
> libdwfl/libdwfl.h | 17 -
> libdwfl/libdwflP.h | 9 -
> 5 files changed, 95 insertions(+), 3 deletions(-)
> create mode 100644 libdwfl/dwfl_process_tracker.c
>
> diff --git a/libdw/libdw.map b/libdw/libdw.map
> index 3218fd9c..ad3ec590 100644
> --- a/libdw/libdw.map
> +++ b/libdw/libdw.map
> @@ -390,4 +390,7 @@ ELFUTILS_0.193 {
>global:
> dwfl_set_initial_registers_thread;
> dwfl_perf_sample_preferred_regs_mask;
> +dwfl_process_tracker_begin;
> +dwfl_begin_with_tracker;
> +dwfl_process_tracker_end;
> } ELFUTILS_0.192;
> diff --git a/libdwfl/Makefile.am b/libdwfl/Makefile.am
> index 37c57bee..b41122e3 100644
> --- a/libdwfl/Makefile.am
> +++ b/libdwfl/Makefile.am
> @@ -71,7 +71,8 @@ libdwfl_a_SOURCES = dwfl_begin.c dwfl_end.c dwfl_error.c
> dwfl_version.c \
> link_map.c core-file.c open.c image-header.c \
> dwfl_frame.c frame_unwind.c dwfl_frame_pc.c \
> linux-pid-attach.c linux-core-attach.c dwfl_frame_regs.c \
> - dwfl_perf_frame.c \
> + dwfl_process_tracker.c \
> +dwfl_perf_frame.c \
> gzip.c debuginfod-client.c
>
> if BZLIB
> diff --git a/libdwfl/dwfl_process_tracker.c b/libdwfl/dwfl_process_tracker.c
> new file mode 100644
> index ..c42d8ad2
> --- /dev/null
> +++ b/libdwfl/dwfl_process_tracker.c
> @@ -0,0 +1,66 @@
> +/* Track multiple Dwfl structs for multiple processes.
> + Copyright (C) 2025, Red Hat, Inc.
> + This file is part of elfutils.
> +
> + This file is free software; you can redistribute it and/or modify
> + it under the terms of either
> +
> + * the GNU Lesser General Public License as published by the Free
> + Software Foundation; either version 3 of the License, or (at
> + your option) any later version
> +
> + or
> +
> + * the GNU General Public License as published by the Free
> + Software Foundation; either version 2 of the License, or (at
> + your option) any later version
> +
> + or both in parallel, as here.
> +
> + elfutils is distributed in the hope that it will be useful, but
> + WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + General Public License for more details.
> +
> + You should have received copies of the GNU General Public License and
> + the GNU Lesser General Public License along with this program. If
> + not, see
Re: [PATCH 02/13] libdwfl [2/13]: expose setfunc callback for libdwfl+libebl clients
Hi Serhei, On Sun, Mar 16, 2025 at 07:13:00PM -0400, Serhei Makarov wrote: > Renaming to dwfl_set_initial_registers_thread. > > This callback was private to one file, but now that other > tools (eu-stacktrace, sysprof via the dwfl_perf_sample_getframes) are > invoking the ebl set_initial_registers_sample api, we need to expose > it. Otherwise, clients would need to reimplement this code > identically, including the undocumented/unexplained use of -2 for > aarch64 insn_mask. > > TODO(REVIEW): Should this be in libdwflP.h since libebl is private? > On the other hand, dwfl_thread_state_register_pc et al. are also > public in spite of being used primarily by the previously-internal > code in dwfl_set_initial_registers_thread. You are mostly right, pid_thread_state_registers_cb is basically a convenience wrapper around the already public dwfl_thread_state_register_pc and dwfl_thread_state_registers. But as implemented there are two things that make it an internal (libdwflP.h) interface. It uses a void *arg which really should be a Dwfl_Thread *. If we make it public it should have a normal Dwfl_Thread * (as first) argument. Secondly there is an assert in the call which should be a normal sanity check that return an error when violated if anybody can call it. Also as you already point out it handles a magic, not publicly documented, -2 regnum. We should document it when made a public function. I am not against making it a public libdwfl function with the above changes, but I think it is simpler/less work to just make it an internal libdwflP.h helper function. Cheers, Mark
Re: [PATCH 04/13] libdwfl [4/13]: add dwfl_perf_sample_preferred_regs_mask
On Wed, Mar 19, 2025, at 5:36 PM, Mark Wielaard wrote: > Hi Serhei, > > On Sun, Mar 16, 2025 at 07:14:11PM -0400, Serhei Makarov wrote: >> Since libebl is a private interface, subsequent patches in the series >> introduce another api wrapping the libebl perf register handling. In >> this patch, add an interface to access the preferred set of registers >> that libdwfl would like to see to allow proper unwinding of stack >> sample data. >> >> * libdwfl/libdwfl.h (dwfl_perf_sample_preferred_regs_mask): >> New function. >> * libdwfl/dwfl_perf_frame.c: New file. >> (dwfl_perf_sample_preferred_regs_mask): New function. >> * libdw/libdw.map: Add dwfl_perf_sample_preferred_regs_mask. >> * libdwfl/Makefile.am: Add dwfl_perf_frame.c. > > I probably need to read the rest of the patch series first. On its own > I am not a fan, it feels very much an implementation detail that > should be abstracted away. Yeah, the trouble is that it's an implementation detail that needs to be agreed on between elfutils (which does unwinding) and the profiler (which calls perf_event_open and provides regs_mask). So there are two ways to do that. Either the profiler decides which regs_mask to use, or the profiler asks elfutils for a regs_mask. The function in this patch is meant to support the latter scenario, which gives the profiler confidence that it's picking a set of registers elfutils can work with.
Re: [PATCH] Add 'Key to Flags' to eu-readelf output [bz 29571]
> On Mon, 2025-03-17 at 17:39 +1000, Samuel Zeter wrote: > > When printing section headers, also include a key to what each flag > > is at the end of the section header output. > > Did you run make check after your patch? It seems various tests fail > because they aren't expecting the new flag info. I am getting: Oops. I'll fix this up. Regards, Sam
Re: [PATCH 1/1] debuginfod: add --http-addr option
Hi Mark,
> Am 19.03.2025 um 13:35 schrieb Mark Wielaard :
>
> 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 ?
that’s interesting, initially I wanted to use ‘--addr=‘ because of the already
available
‘-—port=‘ option. This would be more consistent but less descriptive.
If this is an option, I would prefer anything like
http-addr/listen-addr/bind-addr.
> 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) ?
There might be use cases but I’m not aware of any and for my use case,
debuginfod behind a reverse proxy, a listen on a localhost address is
sufficient. Either IPv4 or IPv6 would work, more important is the fact, that
the listen is not exposed and there is no need for a ruleset on a firewall.
The change uses the MHD_OPTION_SOCK_ADDR of libmicrohttpd, which can be used
for a single sockaddr.
I guess multiple listen socket must be handled in the application and
connections have to be send to libmicrohttpd with MHD_add_connection.
From https://www.gnu.org/software/libmicrohttpd/manual/libmicrohttpd.html
MHD_USE_NO_LISTEN_SOCKETRun the HTTP server without any listen socket. This
option only makes sense if MHD_add_connection is going to be used exclusively
to connect HTTP clients to the HTTP server. This option is incompatible with
using a thread pool; if it is used, MHD_OPTION_THREAD_POOL_SIZE is ignored.
This is a different concept and out of scope of this change.
The initial intention was a bind to 127.0.0.1 or ::1 and therefore
‘—-listen-local4’ and ‘—-listen-local6’ is sufficient. I would prefer these
explicit options over a generic ‘—-listen-local’ because a reverse proxy setup
for a local service should have a proxy config like 'proxy_pass
http://127.0.0.1:8002;’ and from a security perspective the configuration
should be as strict as possible. If you want to setup the service on 127.0.0.1
this should be possible without any need for customisation of the name
resolution.
Binding any other local address does not need additional code and therefore I
decided to provide the generic option ‘—-http-addr=‘ which supports any active
IPv4/IPv6 address. I see no need for this generic version in my setup, but it’s
available and might be interesting for other setups.
>
> 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).
In summary, I would use the generic option, as you’ve suggested with,
'—-listen-address=_IPv4/IPv6_ADDRESS_’ because it covers all possible addresses.
The more restrictive option for localhost addresses could be '—-listen-local4’
to bind to 127.0.0.1 or '—-listen-local6’ for a bind to ::1.
What do you think about that?
Thanks,
Michael
>
> 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
