On Thu, Apr 24, 2025 at 5:49 PM Serhei Makarov <ser...@serhei.io> wrote:
>
> Changes for v4:
>
> - Separate out libdwfl_stacktrace, as requested.
>
> * * *
>
> Initial minimal change to ensure dwflst_tracker_find_pid is
> tested. For now, we keep the additional dwfltab implementation in
> stacktrace.c, since it's being used to track statistics.
>
> In future follow-ups, it will be good to switch to storing
> eu-stacktrace statistics e.g. in dwfl->process->callbacks_arg. This
> requires some additional design to keep the statistics from being lost
> when a pid is reused and the corresponding processtracker table entry
> is replaced.
>
> * src/stacktrace.c (sysprof_init_dwfl): New function.
> (sysprof_find_dwfl): Rename the existing sysprof_init_dwfl.
> Also use dwflst_tracker_find_pid with callback.
> (sysprof_unwind_cb): Rename the existing sysprof_init_dwfl.
> ---
> src/stacktrace.c | 63 +++++++++++++++++++++++++++---------------------
> 1 file changed, 36 insertions(+), 27 deletions(-)
>
> diff --git a/src/stacktrace.c b/src/stacktrace.c
> index c0c9929d..dd58ef3b 100644
> --- a/src/stacktrace.c
> +++ b/src/stacktrace.c
> @@ -96,7 +96,7 @@
> #include ELFUTILS_HEADER(ebl)
> /* #include ELFUTILS_HEADER(dwfl) */
> #include "../libdwfl/libdwflP.h"
> -/* XXX: Private header needed for find_procfile, sysprof_init_dwfl,
> +/* XXX: Private header needed for find_procfile, sysprof_find_dwfl,
> sample_set_initial_registers. */
> #include ELFUTILS_HEADER(dwfl_stacktrace)
>
> @@ -1014,7 +1014,34 @@ find_procfile (Dwfl *dwfl, pid_t *pid, Elf **elf, int
> *elf_fd)
> }
>
> Dwfl *
> -sysprof_init_dwfl (struct sysprof_unwind_info *sui,
> +sysprof_init_dwfl (Dwflst_Process_Tracker *cb_tracker,
Just a nit but I would include _cb or _callback in the name to make it
more clear this function is a callback. Otherwise LGTM.
> + pid_t pid,
> + void *arg __attribute__ ((unused)))
> +{
> + Dwfl *dwfl = dwflst_tracker_dwfl_begin (cb_tracker);
> +
> + int err = dwfl_linux_proc_report (dwfl, pid);
> + if (err < 0)
> + {
> + if (show_failures)
> + fprintf(stderr, "dwfl_linux_proc_report pid %lld: %s",
> + (long long) pid, dwfl_errmsg (-1));
> + return NULL;
> + }
> + err = dwfl_report_end (dwfl, NULL, NULL);
> + if (err != 0)
> + {
> + if (show_failures)
> + fprintf(stderr, "dwfl_report_end pid %lld: %s",
> + (long long) pid, dwfl_errmsg (-1));
> + return NULL;
> + }
> +
> + return dwfl;
> +}
> +
> +Dwfl *
> +sysprof_find_dwfl (struct sysprof_unwind_info *sui,
> SysprofCaptureStackUser *ev,
> SysprofCaptureUserRegs *regs)
> {
> @@ -1027,42 +1054,24 @@ sysprof_init_dwfl (struct sysprof_unwind_info *sui,
> if (regs->n_regs < EXPECTED_REGS) /* XXX expecting everything except FLAGS
> */
> {
> if (show_failures)
> - fprintf(stderr, N_("sysprof_init_dwfl: n_regs=%d, expected %d\n"),
> + fprintf(stderr, N_("sysprof_find_dwfl: n_regs=%d, expected %d\n"),
> regs->n_regs, EXPECTED_REGS);
> return NULL;
> }
>
> - Dwfl *dwfl = pid_find_dwfl(pid);
> + Dwfl *dwfl = dwflst_tracker_find_pid (tracker, pid, sysprof_init_dwfl,
> NULL);
> struct __sample_arg *sample_arg;
> bool cached = false;
> - if (dwfl != NULL)
> + if (dwfl != NULL && dwfl->process != NULL)
> {
> sample_arg = dwfl->process->callbacks_arg; /* XXX requires libdwflP.h
> */
> cached = true;
> goto reuse;
> }
> - dwfl = dwflst_tracker_dwfl_begin (tracker);
> -
> - int err = dwfl_linux_proc_report (dwfl, pid);
> - if (err < 0)
> - {
> - if (show_failures)
> - fprintf(stderr, "dwfl_linux_proc_report pid %lld: %s",
> - (long long) pid, dwfl_errmsg (-1));
> - return NULL;
> - }
> - err = dwfl_report_end (dwfl, NULL, NULL);
> - if (err != 0)
> - {
> - if (show_failures)
> - fprintf(stderr, "dwfl_report_end pid %lld: %s",
> - (long long) pid, dwfl_errmsg (-1));
> - return NULL;
> - }
>
> Elf *elf = NULL;
> int elf_fd = -1;
> - err = find_procfile (dwfl, &pid, &elf, &elf_fd);
> + int err = find_procfile (dwfl, &pid, &elf, &elf_fd);
> if (err < 0)
> {
> if (show_failures)
> @@ -1099,7 +1108,7 @@ sysprof_init_dwfl (struct sysprof_unwind_info *sui,
>
> if (show_frames) {
> bool is_abi32 = (sample_arg->abi == PERF_SAMPLE_REGS_ABI_32);
> - fprintf(stderr, "sysprof_init_dwfl pid %lld%s: size=%ld%s pc=%lx
> sp=%lx+(%lx)\n",
> + fprintf(stderr, "sysprof_find_dwfl pid %lld%s: size=%ld%s pc=%lx
> sp=%lx+(%lx)\n",
> (long long) pid, cached ? " (cached)" : "",
> sample_arg->size, is_abi32 ? " (32-bit)" : "",
> sample_arg->pc, sample_arg->base_addr,
> @@ -1260,7 +1269,7 @@ sysprof_unwind_cb (SysprofCaptureFrame *frame, void
> *arg)
> SysprofCaptureUserRegs *regs = (SysprofCaptureUserRegs *)tail_ptr;
> if (show_frames)
> fprintf(stderr, "\n"); /* extra newline for padding */
> - Dwfl *dwfl = sysprof_init_dwfl (sui, ev, regs);
> + Dwfl *dwfl = sysprof_find_dwfl (sui, ev, regs);
> if (dwfl == NULL)
> {
> if (show_summary)
> @@ -1270,7 +1279,7 @@ sysprof_unwind_cb (SysprofCaptureFrame *frame, void
> *arg)
> dwfl_ent->lost_samples++;
> }
> if (show_failures)
> - fprintf(stderr, "sysprof_init_dwfl pid %lld (%s) (failed)\n",
> + fprintf(stderr, "sysprof_find_dwfl pid %lld (%s) (failed)\n",
> (long long)frame->pid, comm);
> return SYSPROF_CB_OK;
> }
> --
> 2.47.0
>
Aaron
