[v5,03/12] libebl [3/12]: eu-stacktrace: use new register handling api

  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(-)
  

On Thu, Apr 24, 2025 at 5:48 PM Serhei Makarov <serhei@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
  

On Fri, Apr 25, 2025, at 1:54 AM, Aaron Merey wrote:
> 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.
I'm inclined to keep as-is. It's a relevant-enough bit of design history
that keeping it in the git log might be helpful.