On Thu, Apr 24, 2025 at 5:48 PM Serhei Makarov <ser...@serhei.io> wrote:
>
> Changes for v4:
>
> - Since __libdwfl_set_initial_registers_thread is now private to
> libdwfl, the modified code in this patch has been disabled.
>
> * * *
>
> Dummy commit to show how the sample_set_initial_registers callback in
> eu-stacktrace would use the proper libebl
> ebl_set_initial_registers_sample function (if it were public).
>
> * src/Makefile.am (stacktrace_LDADD): Add libebl.
> * src/stacktrace.c (sample_registers_cb): New function,
> though identical to pid_thread_state_registers_cb.
> (sample_set_initial_registers): (XXX Invoke
> ebl_set_initial_registers_sample instead of containing
> platform-specific code directly. This is now commented out.
> Patch12 in the series replaces with code in
> libdwfl_stacktrace/dwflst_perf_frame.c.)
> ---
> src/Makefile.am | 4 ++--
> src/stacktrace.c | 34 +++++++++++++++++++++++++++++-----
> 2 files changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index ed245fc1..6d713e88 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -1,6 +1,6 @@
> ## Process this file with automake to create Makefile.in
> ##
> -## Copyright (C) 1996-2014, 2016, 2024 Red Hat, Inc.
> +## Copyright (C) 1996-2014, 2016, 2024-2025 Red Hat, Inc.
> ## This file is part of elfutils.
> ##
> ## This file is free software; you can redistribute it and/or modify
> @@ -105,7 +105,7 @@ ar_LDADD = libar.a $(libelf) $(libeu) $(argp_LDADD)
> $(obstack_LIBS)
> unstrip_LDADD = $(libebl) $(libelf) $(libdw) $(libeu) $(argp_LDADD)
> stack_LDADD = $(libebl) $(libelf) $(libdw) $(libeu) $(argp_LDADD)
> $(demanglelib)
> if ENABLE_STACKTRACE
> -stacktrace_LDADD = $(libelf) $(libdw) $(libeu) $(argp_LDADD)
> +stacktrace_LDADD = $(libebl) $(libelf) $(libdw) $(libeu) $(argp_LDADD)
> endif
> elfcompress_LDADD = $(libebl) $(libelf) $(libdw) $(libeu) $(argp_LDADD)
> elfclassify_LDADD = $(libelf) $(libdw) $(libeu) $(argp_LDADD)
> diff --git a/src/stacktrace.c b/src/stacktrace.c
> index d8699ce5..3f5950fb 100644
> --- a/src/stacktrace.c
> +++ b/src/stacktrace.c
> @@ -1,5 +1,5 @@
> /* Process a stream of stack samples into stack traces.
> - Copyright (C) 2023-2024 Red Hat, Inc.
> + Copyright (C) 2023-2025 Red Hat, Inc.
> This file is part of elfutils.
>
> This file is free software; you can redistribute it and/or modify
> @@ -93,9 +93,11 @@
> * Includes: libdwfl data structures *
> *************************************/
>
> +#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_init_dwfl,
> + sample_set_initial_registers. */
>
> /*************************************
> * Includes: sysprof data structures *
> @@ -574,10 +576,31 @@ sample_memory_read (Dwfl *dwfl, Dwarf_Addr addr,
> Dwarf_Word *result, void *arg)
> return true;
> }
>
> -/* TODO: Need to generalize this code beyond x86 architectures. */
> static bool
> -sample_set_initial_registers (Dwfl_Thread *thread, void *thread_arg)
> +sample_set_initial_registers (Dwfl_Thread *thread, void *arg)
> {
> +#if 0
> + /* TODO: __libdwfl_set_initial_registers_thread not exported from libdwfl,
> + after it was decided to be unsuitable for a public API.
> +
> + A subsequent patch in the series removes sample_set_initial_registers,
> + replacing it with code in libdwfl_stacktrace/dwflst_perf_frame.c.
> + Keeping this code commented-out for the record, cf how we would
> + implement if the set_initial_registers utility func was public.
> +
> + To *actually* make this work, would need to copy the
> set_initial_registers
> + implementation into stacktrace.c; not worth doing since the later patch
> + overrides this code. */
> + struct __sample_arg *sample_arg = (struct __sample_arg *)arg;
> + dwfl_thread_state_register_pc (thread, sample_arg->pc);
> + Dwfl_Process *process = thread->process;
> + Ebl *ebl = process->ebl;
> + /* XXX Sysprof provides exactly the required registers for unwinding: */
> + uint64_t regs_mask = ebl_perf_frame_regs_mask (ebl);
> + return ebl_set_initial_registers_sample
> + (ebl, sample_arg->regs, sample_arg->n_regs, regs_mask, sample_arg->abi,
> + __libdwfl_set_initial_registers_thread, thread);
> +#else
> /* The following facts are needed to translate x86 registers correctly:
> - perf register order seen in linux
> arch/x86/include/uapi/asm/perf_regs.h
> The registers array is built in the same order as the enum!
> @@ -592,7 +615,7 @@ sample_set_initial_registers (Dwfl_Thread *thread, void
> *thread_arg)
> For comparison, you can study
> codereview.qt-project.org/gitweb?p=qt-creator/perfparser.git;a=blob;f=app/perfregisterinfo.cpp;hb=HEAD
> and follow the code which uses those tables of magic numbers.
> But it's better to follow original sources of truth for this. */
> - struct __sample_arg *sample_arg = (struct __sample_arg *)thread_arg;
> + struct __sample_arg *sample_arg = (struct __sample_arg *)arg;
> bool is_abi32 = (sample_arg->abi == PERF_SAMPLE_REGS_ABI_32);
> static const int regs_i386[] = {0, 2, 3, 1, 7/*sp*/, 6, 4, 5, 8/*ip*/};
> static const int regs_x86_64[] = {0, 3, 2, 1, 4, 5, 6, 7/*sp*/, 9, 10, 11,
> 12, 13, 14, 15, 16, 8/*ip*/};
> @@ -610,6 +633,7 @@ sample_set_initial_registers (Dwfl_Thread *thread, void
> *thread_arg)
> dwfl_thread_state_registers (thread, i, 1, &sample_arg->regs[j]);
> }
> return true;
> +#endif
> }
>
> static void
> --
> 2.47.0
>
This commit could probably be merged with 2 or 12 with the
sample_set_initial_registers design decisions explained in the commit
message. But this patch as-is is harmless so if you prefer to keep it
this way that's ok with me.
Aaron
