Re: [PATCH v2 5/6] Remove usage of "unlocked" variant of stdio print functions

[Mark Wielaard]

Hi Michael,

On Wed, 2024-10-16 at 19:53 +, Michael Pratt wrote:
> These "unlocked" Linux Standard Base variants of standard functions
> are not available on some systems that are still capable
> of building Linux and ELFs.
> 
> The difference is negligible for simple printing to stdout.
> 
> POSIX also states for the similar putc_unlocked():
> 
>   These functions can safely be used in a multi-threaded program
>   if and only if they are called while the invoking thread owns
>   the (FILE *) object, as is the case after a successful call
>   to the flockfile() or ftrylockfile() functions.
> 
> ...
> 
>   These unlocked versions can be safely used
>   only within explicitly locked program regions,
>   using exported locking primitives.
> 
> and these precautions were never done.
> 
> Use the standard forms of these print functions.
> 
> There is inconsistent use of fputc_unlocked() with putc_unlocked(),
> so consistently use the safer fputc() instead.

Looks good. Pushed.

Thanks,

Mark

[PATCH v2 3/5] libdwfl: add unwind origin diagnostics

[Serhei Makarov]

Track the method used to unwind each Dwfl_Frame using an enum field
unwound_source; provide access to the field. Then use that in
eu-stacktrace to display the unwind methods used for each process.

This is an important diagnostic to verify how many processes are
adequately covered by the eh_frame unwinder.

* libdw/libdw.map (ELFUTILS_0.192): Add dwfl_frame_unwound_source,
  dwfl_unwound_source_str.
* libdwfl/libdwfl.h (Dwfl_Unwound_Source): New enum.
  (dwfl_frame_unwound_source)
  (dwfl_unwound_source_str): New functions.
* libdwfl/dwfl_frame.c (dwfl_frame_unwound_source)
  (dwfl_unwound_source_str): New functions.
* libdwfl/libdwflP.h: Add INTDECL for dwfl_frame_unwound_source,
  dwfl_unwound_source_str.
  (struct Dwfl_Frame): Add unwound_source field.
* libdwfl/frame_unwind.c (__libdwfl_frame_unwind): Set
  state->unwound_source depending on the unwind method used.
* src/stacktrace.c (struct sysprof_unwind_info): Add last_pid
  field to provide access to the current sample's dwfltab entry.
  (sysprof_unwind_frame_cb): Add unwound_source to the data displayed
  with the show_frames option.
  (sysprof_unwind_cb): Set last_pid when processing a sample.
  (main): Add unwind_source to the data displayed in the final summary
  table.

Signed-off-by: Serhei Makarov 
---
 libdw/libdw.map|  2 ++
 libdwfl/dwfl_frame.c   | 31 ++-
 libdwfl/frame_unwind.c | 14 +++---
 libdwfl/libdwfl.h  | 22 +-
 libdwfl/libdwflP.h |  5 -
 src/stacktrace.c   | 33 -
 6 files changed, 96 insertions(+), 11 deletions(-)

diff --git a/libdw/libdw.map b/libdw/libdw.map
index 552588a9..bc53385f 100644
--- a/libdw/libdw.map
+++ b/libdw/libdw.map
@@ -382,4 +382,6 @@ ELFUTILS_0.191 {
 ELFUTILS_0.192 {
   global:
 dwfl_set_sysroot;
+dwfl_frame_unwound_source;
+dwfl_unwound_source_str;
 } ELFUTILS_0.191;
diff --git a/libdwfl/dwfl_frame.c b/libdwfl/dwfl_frame.c
index 8af8843f..2e6c6de8 100644
--- a/libdwfl/dwfl_frame.c
+++ b/libdwfl/dwfl_frame.c
@@ -1,5 +1,5 @@
 /* Get Dwarf Frame state for target PID or core file.
-   Copyright (C) 2013, 2014 Red Hat, Inc.
+   Copyright (C) 2013, 2014, 2024 Red Hat, Inc.
This file is part of elfutils.
 
This file is free software; you can redistribute it and/or modify
@@ -98,6 +98,7 @@ state_alloc (Dwfl_Thread *thread)
   state->signal_frame = false;
   state->initial_frame = true;
   state->pc_state = DWFL_FRAME_STATE_ERROR;
+  state->unwound_source = DWFL_UNWOUND_INITIAL_FRAME;
   memset (state->regs_set, 0, sizeof (state->regs_set));
   thread->unwound = state;
   state->unwound = NULL;
@@ -248,6 +249,34 @@ dwfl_frame_thread (Dwfl_Frame *state)
 }
 INTDEF(dwfl_frame_thread)
 
+Dwfl_Unwound_Source
+dwfl_frame_unwound_source (Dwfl_Frame *state)
+{
+  return state->unwound_source;
+}
+INTDEF(dwfl_frame_unwound_source)
+
+const char *
+dwfl_unwound_source_str (Dwfl_Unwound_Source unwound_source)
+{
+  switch (unwound_source)
+{
+case DWFL_UNWOUND_NONE:
+  return "none";
+case DWFL_UNWOUND_INITIAL_FRAME:
+  return "initial";
+case DWFL_UNWOUND_EH_CFI:
+  return "eh_frame";
+case DWFL_UNWOUND_DWARF_CFI:
+  return "dwarf";
+case DWFL_UNWOUND_EBL:
+  return "ebl";
+default:
+  return "unknown";
+}
+}
+INTDEF(dwfl_unwound_source_str)
+
 int
 dwfl_getthreads (Dwfl *dwfl, int (*callback) (Dwfl_Thread *thread, void *arg),
 void *arg)
diff --git a/libdwfl/frame_unwind.c b/libdwfl/frame_unwind.c
index ab444d25..de65e09c 100644
--- a/libdwfl/frame_unwind.c
+++ b/libdwfl/frame_unwind.c
@@ -1,5 +1,5 @@
 /* Get previous frame state for an existing frame state.
-   Copyright (C) 2013, 2014, 2016 Red Hat, Inc.
+   Copyright (C) 2013, 2014, 2016, 2024 Red Hat, Inc.
This file is part of elfutils.
 
This file is free software; you can redistribute it and/or modify
@@ -515,6 +515,7 @@ new_unwound (Dwfl_Frame *state)
   unwound->signal_frame = false;
   unwound->initial_frame = false;
   unwound->pc_state = DWFL_FRAME_STATE_ERROR;
+  unwound->unwound_source = DWFL_UNWOUND_NONE;
   memset (unwound->regs_set, 0, sizeof (unwound->regs_set));
   return unwound;
 }
@@ -742,14 +743,20 @@ __libdwfl_frame_unwind (Dwfl_Frame *state)
{
  handle_cfi (state, pc - bias, cfi_eh, bias);
  if (state->unwound)
-   return;
+   {
+ state->unwound->unwound_source = DWFL_UNWOUND_EH_CFI;
+ return;
+   }
}
   Dwarf_CFI *cfi_dwarf = INTUSE(dwfl_module_dwarf_cfi) (mod, &bias);
   if (cfi_dwarf)
{
  handle_cfi (state, pc - bias, cfi_dwarf, bias);
  if (state->unwound)
-   return;
+   {
+ state->unwound->unwound_source = DWFL_UNWOUND_DWARF_CFI;
+ return;
+   }
}
 }
   assert (state->unwound == NULL);
@@ -774,6 +781,7 @@ __libdwfl_frame_unwind (Dwfl_Frame *state)
   // __libdwfl_

[PATCH v2 4/5] src: add unwind origin diagnostics to eu-stack

[Serhei Makarov]

Since we obtain diagnostics about unwind method, another logical place
to show them is in eu-stack.

* src/stack.c (show_unwound_source): New global variable.
  (struct frame): Add unwound_source field.
  (frame_callback): Copy over unwound_source from Dwfl_Frame.
  (print_frame): Take unwound_source string and print it.
  (print_inline_frames): Take unwound_source argument and pass it on,
  except for subsequent frames where we pass the string "inline".
  (print_frames): Take unwound_source field from struct frame and pass
  it on.
  (parse_opt): Add --cfi-type,-c option to set show_unwound_source.
  (main): Ditto.
* tests/run-stack-i-test.sh: Add testcase for --cfi-type.

Signed-off-by: Serhei Makarov 
---
 src/stack.c   | 30 +++---
 tests/run-stack-i-test.sh | 15 ++-
 2 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/src/stack.c b/src/stack.c
index eb87f5f1..a06cb077 100644
--- a/src/stack.c
+++ b/src/stack.c
@@ -1,5 +1,5 @@
 /* Unwinding of frames like gstack/pstack.
-   Copyright (C) 2013-2014 Red Hat, Inc.
+   Copyright (C) 2013-2014, 2024 Red Hat, Inc.
This file is part of elfutils.
 
This file is free software; you can redistribute it and/or modify
@@ -44,6 +44,7 @@ ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT;
 static bool show_activation = false;
 static bool show_module = false;
 static bool show_build_id = false;
+static bool show_unwound_source = false;
 static bool show_source = false;
 static bool show_one_tid = false;
 static bool show_quiet = false;
@@ -58,6 +59,7 @@ struct frame
 {
   Dwarf_Addr pc;
   bool isactivation;
+  Dwfl_Unwound_Source unwound_source;
 };
 
 struct frames
@@ -180,6 +182,7 @@ frame_callback (Dwfl_Frame *state, void *arg)
 {
   struct frames *frames = (struct frames *) arg;
   int nr = frames->frames;
+  frames->frame[nr].unwound_source = dwfl_frame_unwound_source (state);
   if (! dwfl_frame_pc (state, &frames->frame[nr].pc,
   &frames->frame[nr].isactivation))
 return -1;
@@ -221,7 +224,7 @@ static void
 print_frame (int nr, Dwarf_Addr pc, bool isactivation,
 Dwarf_Addr pc_adjusted, Dwfl_Module *mod,
 const char *symname, Dwarf_Die *cudie,
-Dwarf_Die *die)
+Dwarf_Die *die, const char *unwound_source)
 {
   int width = get_addr_width (mod);
   printf ("#%-2u 0x%0*" PRIx64, nr, width, (uint64_t) pc);
@@ -271,6 +274,11 @@ print_frame (int nr, Dwarf_Addr pc, bool isactivation,
}
 }
 
+  if (show_unwound_source)
+{
+  printf (" [%s]", unwound_source);
+}
+
   if (show_source)
 {
   int line, col;
@@ -323,7 +331,8 @@ print_frame (int nr, Dwarf_Addr pc, bool isactivation,
 static void
 print_inline_frames (int *nr, Dwarf_Addr pc, bool isactivation,
 Dwarf_Addr pc_adjusted, Dwfl_Module *mod,
-const char *symname, Dwarf_Die *cudie, Dwarf_Die *die)
+const char *symname, Dwarf_Die *cudie, Dwarf_Die *die,
+Dwfl_Unwound_Source unwound_source)
 {
   Dwarf_Die *scopes = NULL;
   int nscopes = dwarf_getscopes_die (die, &scopes);
@@ -333,7 +342,7 @@ print_inline_frames (int *nr, Dwarf_Addr pc, bool 
isactivation,
 the name.  This is the actual source location where it
 happened.  */
   print_frame ((*nr)++, pc, isactivation, pc_adjusted, mod, symname,
-  NULL, NULL);
+  NULL, NULL, dwfl_unwound_source_str(unwound_source));
 
   /* last_scope is the source location where the next frame/function
 call was done. */
@@ -349,7 +358,7 @@ print_inline_frames (int *nr, Dwarf_Addr pc, bool 
isactivation,
 
  symname = die_name (scope);
  print_frame ((*nr)++, pc, isactivation, pc_adjusted, mod, symname,
-  cudie, last_scope);
+  cudie, last_scope, "inline");
 
  /* Found the "top-level" in which everything was inlined?  */
  if (tag == DW_TAG_subprogram)
@@ -375,6 +384,7 @@ print_frames (struct frames *frames, pid_t tid, int 
dwflerr, const char *what)
   Dwarf_Addr pc = frames->frame[nr].pc;
   bool isactivation = frames->frame[nr].isactivation;
   Dwarf_Addr pc_adjusted = pc - (isactivation ? 0 : 1);
+  Dwfl_Unwound_Source unwound_source = frames->frame[nr].unwound_source;
 
   /* Get PC->SYMNAME.  */
   Dwfl_Module *mod = dwfl_addrmodule (dwfl, pc_adjusted);
@@ -417,10 +427,10 @@ print_frames (struct frames *frames, pid_t tid, int 
dwflerr, const char *what)
 
   if (show_inlines && die != NULL)
print_inline_frames (&frame_nr, pc, isactivation, pc_adjusted, mod,
-symname, cudie, die);
+symname, cudie, die, unwound_source);
   else
print_frame (frame_nr++, pc, isactivation, pc_adjusted, mod, symname,
-NULL, NULL);
+NULL, NULL, dwfl_unwound_source_str(unwound_s

[PATCH v2 5/5] NEWS: add entry for eu-stacktrace

[Serhei Makarov]

Signed-off-by: Serhei Makarov 
---
 NEWS | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/NEWS b/NEWS
index 410bccc1..01909c0c 100644
--- a/NEWS
+++ b/NEWS
@@ -8,6 +8,13 @@ debuginfod: Add per-file signature verification for integrity
 
 debuginfod: New API for metadata queries: file name -> buildid.
 
+stacktrace: Experimental new tool that can process a stream of stack
+samples from the Sysprof profiler and unwind them into call
+chains. Enable on x86 with --enable-stacktrace. See
+README.eu-stacktrace in the development branch for detailed
+usage instructions:
+
https://sourceware.org/cgit/elfutils/tree/README.eu-stacktrace?h=users/serhei/eu-stacktrace
+
 Version 0.191 "Bug fixes in C major"
 
 libdw: dwarf_addrdie now supports binaries lacking a .debug_aranges
-- 
2.45.1

[PATCH v2 1/5] src: add eu-stacktrace tool

[Serhei Makarov]

eu-stacktrace is a utility to process a stream of raw stack
samples (such as those obtained from the Linux kernel's
PERF_SAMPLE_STACK facility) into a stream of stack traces (such as
those obtained from PERF_SAMPLE_CALLCHAIN), freeing other profiling
utilities from having to implement their own backtracing logic.

eu-stacktrace accepts data from a profiling tool via a pipe or
fifo. The initial version of the tool works on x86 architectures and
accepts data from Sysprof [1]. For future work, it will make sense
to expand support to other profilers, in particular perf tool.

Further patches in this series provide configury, docs, and improved
diagnostics for tracking the method used to unwind each frame in the
stack trace.

[1]: The following patched version of Sysprof (upstream submission ETA
~very_soon) can produce data with stack samples:

https://git.sr.ht/~serhei/sysprof-experiments/log/serhei/samples-via-fifo

Invoking the patched sysprof with eu-stacktrace:

$ sudo sysprof-cli --use-stacktrace
$ sudo sysprof-cli --use-stacktrace --stacktrace-path=/path/to/eu-stacktrace

Invoking the patched sysprof and eu-stacktrace manually through a fifo:

$ mkfifo /tmp/test.fifo
$ sudo eu-stacktrace --input /tmp/test.fifo --output test.syscap &
$ sysprof-cli --sample-method=stack --use-fifo=/tmp/test.fifo test.syscap

Note that sysprof polkit actions must be installed systemwide
(e.g. installing the system sysprof package will provide these).
Otherwise, "Action org.gnome.sysprof3.profile is not registered"
error will result.

* src/stacktrace.c: Add new tool.

Signed-off-by: Serhei Makarov 
---
 src/stacktrace.c | 1571 ++
 1 file changed, 1571 insertions(+)
 create mode 100644 src/stacktrace.c

diff --git a/src/stacktrace.c b/src/stacktrace.c
new file mode 100644
index ..3e13e26c
--- /dev/null
+++ b/src/stacktrace.c
@@ -0,0 +1,1571 @@
+/* Process a stream of stack samples into stack traces.
+   Copyright (C) 2023-2024 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 the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   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 a copy of the GNU General Public License
+   along with this program.  If not, see .
+
+   This file incorporates work covered by the following copyright and
+   permission notice:
+
+   Copyright 2016-2019 Christian Hergert 
+
+   Redistribution and use in source and binary forms, with or without
+   modification, are permitted provided that the following conditions are
+   met:
+
+   1. Redistributions of source code must retain the above copyright notice,
+  this list of conditions and the following disclaimer.
+
+   2. Redistributions in binary form must reproduce the above copyright
+  notice, this list of conditions and the following disclaimer in the
+  documentation and/or other materials provided with the distribution.
+
+   Subject to the terms and conditions of this license, each copyright holder
+   and contributor hereby grants to those receiving rights under this license
+   a perpetual, worldwide, non-exclusive, no-charge, royalty-free,
+   irrevocable (except for failure to satisfy the conditions of this license)
+   patent license to make, have made, use, offer to sell, sell, import, and
+   otherwise transfer this software, where such license applies only to those
+   patent claims, already acquired or hereafter acquired, licensable by such
+   copyright holder or contributor that are necessarily infringed by:
+
+   (a) their Contribution(s) (the licensed copyrights of copyright holders
+   and non-copyrightable additions of contributors, in source or binary
+   form) alone; or
+
+   (b) combination of their Contribution(s) with the work of authorship to
+   which such Contribution(s) was added by such copyright holder or
+   contributor, if, at the time the Contribution is added, such addition
+   causes such combination to be necessarily infringed. The patent license
+   shall not apply to any other combinations which include the
+   Contribution.
+
+   Except as expressly stated above, no rights or licenses from any copyright
+   holder or contributor is granted under this license, whether expressly, by
+   implication, estoppel or otherwise.
+
+   DISCLAIMER
+
+   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
+   IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+   THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+   

[PATCH v2 2/5] configure.ac: eu-stacktrace initial version (x86/sysprof only)

[Serhei Makarov]

Due to the x86-specific code in the initial version the configury has
significant restrictions. If --enable-stacktrace is not explicitly
provided, then eu-stacktrace will be disabled by default.

The way we test for x86 is a bit unusual. What we actually care about
is that the register file provided by perf_events on the system is an
x86 register file; this is done by checking that  is
Linux kernel arch/x86/include/uapi/asm/perf_regs.h.

Once eu-stacktrace is properly portable across architectures,
these grody checks can be simplified. Enablement of the feature
by default depends on a released Sysprof version we can point to
for the patches.

* configure.ac: Add configure checks and conditionals for stacktrace tool.
* src/Makefile.am: Add stacktrace tool conditional on ENABLE_STACKTRACE.

Signed-off-by: Serhei Makarov 
---
 configure.ac| 56 -
 src/Makefile.am |  9 +++-
 2 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 8f5901a2..05a95f50 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1,7 +1,7 @@
 dnl Process this file with autoconf to produce a configure script.
 dnl Configure input file for elfutils. -*-autoconf-*-
 dnl
-dnl Copyright (C) 1996-2019 Red Hat, Inc.
+dnl Copyright (C) 1996-2019, 2024 Red Hat, Inc.
 dnl Copyright (C) 2022, 2023 Mark J. Wielaard 
 dnl
 dnl This file is part of elfutils.
@@ -918,6 +918,59 @@ AC_ARG_ENABLE(debuginfod-ima-cert-path,
 AC_SUBST(DEBUGINFOD_IMA_CERT_PATH, $default_debuginfod_ima_cert_path)
 AC_CONFIG_FILES([config/profile.sh config/profile.csh config/profile.fish])
 
+# XXX Currently, eu-stacktrace can only work with sysprof/x86, hence:
+AC_ARG_ENABLE([stacktrace],AS_HELP_STRING([--enable-stacktrace], [Enable 
eu-stacktrace]))
+# check for x86, or more precisely _ASM_X86_PERF_REGS_H
+AS_IF([test "x$enable_stacktrace" = "xyes"], [
+   enable_stacktrace=no
+   AC_LANG([C])
+   AC_CACHE_CHECK([for _ASM_X86_PERF_REGS_H], ac_cv_has_asm_x86_perf_regs_h,
+  [AC_COMPILE_IFELSE([AC_LANG_SOURCE([
+#include 
+#ifndef _ASM_X86_PERF_REGS_H
+#error "_ASM_X86_PERF_REGS_H not found"
+#endif
+])], ac_cv_has_asm_x86_perf_regs_h=yes, ac_cv_has_asm_x86_perf_regs_h=no)])
+   AS_IF([test "x$ac_cv_has_asm_x86_perf_regs_h" = xyes], [
+  enable_stacktrace=yes
+   ])
+   if test "x$enable_stacktrace" = "xno"; then
+  AC_MSG_ERROR([${program_prefix}stacktrace currently only supports x86, 
use --disable-stacktrace to disable.])
+   fi
+])
+# check for sysprof headers:
+AS_IF([test "x$enable_stacktrace" = "xyes"], [
+   enable_stacktrace=no
+   AC_CACHE_CHECK([for sysprof-6/sysprof-capture-types.h], 
ac_cv_has_sysprof_6_headers,
+  [AC_COMPILE_IFELSE([AC_LANG_SOURCE([[#include 
]])],
+  ac_cv_has_sysprof_6_headers=yes, ac_cv_has_sysprof_6_headers=no)])
+   AS_IF([test "x$ac_cv_has_sysprof_6_headers" = xyes], [
+  AC_DEFINE(HAVE_SYSPROF_6_HEADERS)
+  enable_stacktrace=yes
+   ])
+   AH_TEMPLATE([HAVE_SYSPROF_6_HEADERS], [Define to 1 if 
`sysprof-6/sysprof-capture-types.h` is provided by the system, 0 otherwise.])
+   AC_CACHE_CHECK([for sysprof-4/sysprof-capture-types.h], 
ac_cv_has_sysprof_4_headers,
+  [AC_COMPILE_IFELSE([AC_LANG_SOURCE([[#include 
]])],
+  ac_cv_has_sysprof_4_headers=yes, ac_cv_has_sysprof_4_headers=no)])
+   AS_IF([test "x$ac_cv_has_sysprof_4_headers" = xyes], [
+  AC_DEFINE(HAVE_SYSPROF_4_HEADERS)
+  enable_stacktrace=yes
+   ])
+   AH_TEMPLATE([HAVE_SYSPROF_4_HEADERS], [Define to 1 if 
`sysprof-4/sysprof-capture-types.h` is provided by the system, 0 otherwise.])
+   if test "x$enable_stacktrace" = "xno"; then
+  AC_MSG_ERROR([sysprof headers for ${program_prefix}stacktrace not found, 
use --disable-stacktrace to disable.])
+   fi
+],[
+   # If eu-stacktrace is disabled, also disable the automake conditionals:
+   ac_cv_has_sysprof_6_headers=no
+   ac_cv_has_sysprof_4_headers=no
+   # And make sure the feature listing shows 'no':
+   enable_stacktrace=no
+])
+AM_CONDITIONAL([HAVE_SYSPROF_6_HEADERS],[test "x$ac_cv_has_sysprof_6_headers" 
= xyes])
+AM_CONDITIONAL([HAVE_SYSPROF_4_HEADERS],[test "x$ac_cv_has_sysprof_4_headers" 
= xyes])
+AM_CONDITIONAL([ENABLE_STACKTRACE],[test "x$enable_stacktrace" = "xyes"])
+
 AC_OUTPUT
 
 AC_MSG_NOTICE([
@@ -957,6 +1010,7 @@ AC_MSG_NOTICE([
 Default DEBUGINFOD_URLS: ${default_debuginfod_urls}
 Debuginfod RPM sig checking: ${enable_debuginfod_ima_verification} 
 Default DEBUGINFOD_IMA_CERT_PATH   : ${default_debuginfod_ima_cert_path}
+${program_prefix}stacktrace support  : ${enable_stacktrace}
 
   EXTRA TEST FEATURES (used with make check)
 have bunzip2 installed (required)  : ${HAVE_BUNZIP2}
diff --git a/src/Makefile.am b/src/Makefile.am
index e0267d96..6bdf2dfb 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, 20

Re: [PATCH v2 4/5] src: add unwind origin diagnostics to eu-stack

[Mark Wielaard]

Hi Serhei,

On Thu, Oct 17, 2024 at 02:54:34PM -0400, Serhei Makarov wrote:
> Since we obtain diagnostics about unwind method, another logical place
> to show them is in eu-stack.
> 
> * src/stack.c (show_unwound_source): New global variable.
>   (struct frame): Add unwound_source field.
>   (frame_callback): Copy over unwound_source from Dwfl_Frame.
>   (print_frame): Take unwound_source string and print it.
>   (print_inline_frames): Take unwound_source argument and pass it on,
>   except for subsequent frames where we pass the string "inline".
>   (print_frames): Take unwound_source field from struct frame and pass
>   it on.
>   (parse_opt): Add --cfi-type,-c option to set show_unwound_source.
>   (main): Ditto.
> * tests/run-stack-i-test.sh: Add testcase for --cfi-type.

I like this version.

Thanks,

Mark

Re: [PATCH v2 2/5] configure.ac: eu-stacktrace initial version (x86/sysprof only)

[Aaron Merey]

On Thu, Oct 17, 2024 at 2:51 PM Serhei Makarov  wrote:
>
> Due to the x86-specific code in the initial version the configury has
> significant restrictions. If --enable-stacktrace is not explicitly
> provided, then eu-stacktrace will be disabled by default.
>
> The way we test for x86 is a bit unusual. What we actually care about
> is that the register file provided by perf_events on the system is an
> x86 register file; this is done by checking that  is
> Linux kernel arch/x86/include/uapi/asm/perf_regs.h.
>
> Once eu-stacktrace is properly portable across architectures,
> these grody checks can be simplified. Enablement of the feature
> by default depends on a released Sysprof version we can point to
> for the patches.
>
> * configure.ac: Add configure checks and conditionals for stacktrace tool.
> * src/Makefile.am: Add stacktrace tool conditional on ENABLE_STACKTRACE.
>
> Signed-off-by: Serhei Makarov 

LGTM, please merge.

Thanks,
Aaron

Re: [PATCH v2 3/5] libdwfl: add unwind origin diagnostics

[Aaron Merey]

On Thu, Oct 17, 2024 at 2:53 PM Serhei Makarov  wrote:
>
> Track the method used to unwind each Dwfl_Frame using an enum field
> unwound_source; provide access to the field. Then use that in
> eu-stacktrace to display the unwind methods used for each process.
>
> This is an important diagnostic to verify how many processes are
> adequately covered by the eh_frame unwinder.
>
> * libdw/libdw.map (ELFUTILS_0.192): Add dwfl_frame_unwound_source,
>   dwfl_unwound_source_str.
> * libdwfl/libdwfl.h (Dwfl_Unwound_Source): New enum.
>   (dwfl_frame_unwound_source)
>   (dwfl_unwound_source_str): New functions.
> * libdwfl/dwfl_frame.c (dwfl_frame_unwound_source)
>   (dwfl_unwound_source_str): New functions.
> * libdwfl/libdwflP.h: Add INTDECL for dwfl_frame_unwound_source,
>   dwfl_unwound_source_str.
>   (struct Dwfl_Frame): Add unwound_source field.
> * libdwfl/frame_unwind.c (__libdwfl_frame_unwind): Set
>   state->unwound_source depending on the unwind method used.
> * src/stacktrace.c (struct sysprof_unwind_info): Add last_pid
>   field to provide access to the current sample's dwfltab entry.
>   (sysprof_unwind_frame_cb): Add unwound_source to the data displayed
>   with the show_frames option.
>   (sysprof_unwind_cb): Set last_pid when processing a sample.
>   (main): Add unwind_source to the data displayed in the final summary
>   table.
>
> Signed-off-by: Serhei Makarov 

LGTM, please merge.

Thanks,
Aaron

Re: [PATCH v2 5/5] NEWS: add entry for eu-stacktrace

[Aaron Merey]

On Thu, Oct 17, 2024 at 2:56 PM Serhei Makarov  wrote:
>
> Signed-off-by: Serhei Makarov 
> ---
>  NEWS | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/NEWS b/NEWS
> index 410bccc1..01909c0c 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -8,6 +8,13 @@ debuginfod: Add per-file signature verification for integrity
>
>  debuginfod: New API for metadata queries: file name -> buildid.
>
> +stacktrace: Experimental new tool that can process a stream of stack
> +samples from the Sysprof profiler and unwind them into call
> +chains. Enable on x86 with --enable-stacktrace. See
> +README.eu-stacktrace in the development branch for detailed
> +usage instructions:
> +
> https://sourceware.org/cgit/elfutils/tree/README.eu-stacktrace?h=users/serhei/eu-stacktrace

LGTM, please merge.

Thanks,
Aaron

Re: [PATCH v2 4/5] src: add unwind origin diagnostics to eu-stack

[Aaron Merey]

On Thu, Oct 17, 2024 at 3:41 PM Mark Wielaard  wrote:
>
> Hi Serhei,
>
> On Thu, Oct 17, 2024 at 02:54:34PM -0400, Serhei Makarov wrote:
> > Since we obtain diagnostics about unwind method, another logical place
> > to show them is in eu-stack.
> >
> > * src/stack.c (show_unwound_source): New global variable.
> >   (struct frame): Add unwound_source field.
> >   (frame_callback): Copy over unwound_source from Dwfl_Frame.
> >   (print_frame): Take unwound_source string and print it.
> >   (print_inline_frames): Take unwound_source argument and pass it on,
> >   except for subsequent frames where we pass the string "inline".
> >   (print_frames): Take unwound_source field from struct frame and pass
> >   it on.
> >   (parse_opt): Add --cfi-type,-c option to set show_unwound_source.
> >   (main): Ditto.
> > * tests/run-stack-i-test.sh: Add testcase for --cfi-type.
>
> I like this version.
>

Same here, this is now ready to merge.

Thanks,
Aaron

Re: [PATCH v2 1/5] src: add eu-stacktrace tool

[Aaron Merey]

On Thu, Oct 17, 2024 at 2:49 PM Serhei Makarov  wrote:
>
> eu-stacktrace is a utility to process a stream of raw stack
> samples (such as those obtained from the Linux kernel's
> PERF_SAMPLE_STACK facility) into a stream of stack traces (such as
> those obtained from PERF_SAMPLE_CALLCHAIN), freeing other profiling
> utilities from having to implement their own backtracing logic.
>
> eu-stacktrace accepts data from a profiling tool via a pipe or
> fifo. The initial version of the tool works on x86 architectures and
> accepts data from Sysprof [1]. For future work, it will make sense
> to expand support to other profilers, in particular perf tool.
>
> Further patches in this series provide configury, docs, and improved
> diagnostics for tracking the method used to unwind each frame in the
> stack trace.
>
> [1]: The following patched version of Sysprof (upstream submission ETA
> ~very_soon) can produce data with stack samples:
>
> https://git.sr.ht/~serhei/sysprof-experiments/log/serhei/samples-via-fifo
>
> Invoking the patched sysprof with eu-stacktrace:
>
> $ sudo sysprof-cli --use-stacktrace
> $ sudo sysprof-cli --use-stacktrace --stacktrace-path=/path/to/eu-stacktrace
>
> Invoking the patched sysprof and eu-stacktrace manually through a fifo:
>
> $ mkfifo /tmp/test.fifo
> $ sudo eu-stacktrace --input /tmp/test.fifo --output test.syscap &
> $ sysprof-cli --sample-method=stack --use-fifo=/tmp/test.fifo test.syscap
>
> Note that sysprof polkit actions must be installed systemwide
> (e.g. installing the system sysprof package will provide these).
> Otherwise, "Action org.gnome.sysprof3.profile is not registered"
> error will result.
>
> * src/stacktrace.c: Add new tool.
>
> Signed-off-by: Serhei Makarov 

LGTM. eu-stacktrace is disabled by default even when sysprof headers
are found and when enabled it builds with no errors. Please go ahead
and merge.

Thanks,
Aaron

[PATCH v2] libdwelf: add dwelf_elf_remove_debug_relocs

[Aaron Merey]

From: Di Chen 

Provide a public function for removing debug section relocations.

eu-strip previously contained the code to remove debug section
relocations.  This patch moves that code into
dwelf_elf_remove_debug_relocs.

https://sourceware.org/bugzilla/show_bug.cgi?id=31447

Signed-off-by: Di Chen 
Signed-off-by: Aaron Merey 
---

I'd like to be able to call __libelf_seterrno from 
dwelf_elf_remove_debug_relocations.c, but I keep getting the following
linker error:

> /usr/local/bin/ld: 
> ../libdwelf/libdwelf_pic.a(dwelf_elf_remove_debug_relocations.os): in 
> function `relocate':
> /home/amerey/elfutils/libdwelf/dwelf_elf_remove_debug_relocations.c:113:(.text+0x28e):
>  undefined reference to `__libelf_seterrno'

I tried to modifying libdwelf/Makefile.am but wasn't able to solve this.
__libdw_seterrno works without this issue and I'm not sure what's
missing from the libelf linking.

I've left the calls to __libelf_seterrno commented out for now.

v1: https://sourceware.org/pipermail/elfutils-devel/2024q3/007388.html

v2 changes:
Renamed dwelf_relocation_debug_sections to
dwelf_elf_remove_debug_relocations. Renamed various files to match this
new function name.

Removed all asserts and error exits from
dwelf_elf_remove_debug_relocations.c

 libdw/libdw.map   |   1 +
 libdwelf/Makefile.am  |   5 +-
 libdwelf/dwelf_elf_remove_debug_relocations.c | 400 ++
 libdwelf/libdwelf.h   |   4 +
 src/strip.c   | 354 +---
 tests/Makefile.am |   5 +-
 tests/remove-relocs.c |  38 ++
 tests/run-remove-relocs.sh|  54 +++
 tests/testfile-remove-relocs.bz2  | Bin 0 -> 1088 bytes
 9 files changed, 513 insertions(+), 348 deletions(-)
 create mode 100644 libdwelf/dwelf_elf_remove_debug_relocations.c
 create mode 100644 tests/remove-relocs.c
 create mode 100755 tests/run-remove-relocs.sh
 create mode 100644 tests/testfile-remove-relocs.bz2

diff --git a/libdw/libdw.map b/libdw/libdw.map
index bc53385f..b282e886 100644
--- a/libdw/libdw.map
+++ b/libdw/libdw.map
@@ -384,4 +384,5 @@ ELFUTILS_0.192 {
 dwfl_set_sysroot;
 dwfl_frame_unwound_source;
 dwfl_unwound_source_str;
+dwelf_elf_remove_debug_relocations;
 } ELFUTILS_0.191;
diff --git a/libdwelf/Makefile.am b/libdwelf/Makefile.am
index a35a2873..6090fd64 100644
--- a/libdwelf/Makefile.am
+++ b/libdwelf/Makefile.am
@@ -42,13 +42,14 @@ noinst_HEADERS = libdwelfP.h
 libdwelf_a_SOURCES = dwelf_elf_gnu_debuglink.c dwelf_dwarf_gnu_debugaltlink.c \
 dwelf_elf_gnu_build_id.c dwelf_scn_gnu_compressed_size.c \
 dwelf_strtab.c dwelf_elf_begin.c \
-dwelf_elf_e_machine_string.c
+dwelf_elf_e_machine_string.c \
+dwelf_elf_remove_debug_relocations.c
 
 libdwelf = $(libdw)
 
 libdw = ../libdw/libdw.so
 libelf = ../libelf/libelf.so
-libebl = ../libebl/libebl.a
+libebl = ../libebl/libebl.a ../backends/libebl_backends.a
 libeu = ../lib/libeu.a
 
 libdwelf_pic_a_SOURCES =
diff --git a/libdwelf/dwelf_elf_remove_debug_relocations.c 
b/libdwelf/dwelf_elf_remove_debug_relocations.c
new file mode 100644
index ..cbb25fb4
--- /dev/null
+++ b/libdwelf/dwelf_elf_remove_debug_relocations.c
@@ -0,0 +1,400 @@
+/* Remove relocations from debug sections.
+   Copyright (C) 2014 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 .  */
+
+#ifdef HAVE_CONFIG_H
+# include 
+#endif
+
+#include "libdwelfP.h"
+#include "libelfP.h"
+#include "libebl.h"
+
+typedef uint8_t GElf_Byte;
+
+const char *
+secndx_name (Elf *elf, size_t ndx)
+{
+  size_t shstrndx;
+  GElf_Shdr mem;
+  Elf_Scn *sec = elf_getscn (elf, ndx);
+  GElf_Shdr *shdr = gelf_getshdr (sec, &mem);
+  if (shdr == NULL || elf_getshdrstrndx (elf, &shstrndx) < 0)
+return "???";
+  return elf_strptr (elf, shstrndx, shdr->sh_name) ?: "???";
+}
+
+
+Elf_Data *
+get_xndxdata (Elf *elf, Elf_Scn *symscn

Re: [PATCH] libelf: elf_compress doesn't handle multiple elf_newdata chunks correctly

[Aaron Merey]

On Thu, Oct 17, 2024 at 11:59 AM Mark Wielaard  wrote:
>
> elf_compress would compress all (new) data chunks, but didn't reset
> the section data_list. This would cause extra data to be returned
> after decompression or create bad compressed data. Add a new testcase
> for this and explicitly zap the scn->data_list before resetting the
> elf section raw data after (de)compression.
>
>  * libelf/elf_compress.c (__libelf_reset_rawdata): Cleanup
>  scn->data_list.
>  * tests/newzdata.c: New testcase.
>  * tests/Makefile.am (check_PROGRAMS): Add newzdata.
>  (TESTS): Likewise.
>  (newzdata_LDADD): New variable.
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=32102
>
> Signed-off-by: Mark Wielaard 

LGTM. The new testcases pass on the trybots. And when I revert the
elf_compress.c changes and run the new tests they fail as expected, so
the elf_compress.c changes do in fact fix PR32102.

Aaron

[PATCH] doc: Add libelf man page

[Aaron Merey]

Signed-off-by: Aaron Merey 
---
 config/elfutils.spec.in |   1 +
 doc/Makefile.am |   3 +-
 doc/libelf.3| 363 
 3 files changed, 366 insertions(+), 1 deletion(-)
 create mode 100644 doc/libelf.3

diff --git a/config/elfutils.spec.in b/config/elfutils.spec.in
index 0d1ec11b..dcaa85b3 100644
--- a/config/elfutils.spec.in
+++ b/config/elfutils.spec.in
@@ -304,6 +304,7 @@ fi
 %{_mandir}/man3/elf_*.3*
 %{_mandir}/man3/elf32_*.3*
 %{_mandir}/man3/elf64_*.3*
+%{_mandir}/man3/libelf.3
 
 %files libelf-devel-static
 %{_libdir}/libelf.a
diff --git a/doc/Makefile.am b/doc/Makefile.am
index ceb6fd2d..c95369e9 100644
--- a/doc/Makefile.am
+++ b/doc/Makefile.am
@@ -57,7 +57,8 @@ notrans_dist_man3_MANS= elf_update.3 \
elf32_fsize.3 \
elf64_fsize.3 \
elf32_checksum.3 \
-   elf64_checksum.3
+   elf64_checksum.3 \
+   libelf.3
 
 # libdebuginfod man pages (also notrans)
 # Note we include them even when not building them because we want
diff --git a/doc/libelf.3 b/doc/libelf.3
new file mode 100644
index ..e6fb1a1b
--- /dev/null
+++ b/doc/libelf.3
@@ -0,0 +1,363 @@
+.TH LIBELF 3 2024-10-18 "Libelf" "Libelf Programmer's Manual"
+
+.SH NAME
+libelf \- a library for accessing and manipulating ELF (Executable and Linkable
+Format) files
+.SH LIBRARY
+Elfutils library (\fBlibelf\fP, \fBlibelf.so\fP, \fB-lelf\fP)
+.SH SYNOPSIS
+.nf
+.B #include 
+
+.SH DESCRIPTION
+The \fBlibelf\fP library provides an API for reading, writing, and manipulating
+ELF (Executable and Linkable Format) files. ELF is a standard format for object
+files, shared libraries, core dumps, and executables.  See
+.BR elf (5)
+for more information regarding ELF.
+
+\fBlibelf\fP provides routines for working with ELF object file headers,
+sections, symbol tables, relocation entries, and other key components.
+
+The core of the library is based on
+.I Elf
+file descriptors representing ELF files, which can be read from, written to,
+or updated in-place. The \fBelf_begin\fP function initializes access to an
+ELF object, while additional functions like \fBelf_getscn\fP, 
\fBelf_getdata\fP,
+and \fBelf_ndxscn\fP provide access to specific parts of the ELF file.
+
+.SH FILE VS MEMORY REPRESENTATION
+
+The \fBlibelf\fP library distinguishes between the file representation of an
+ELF file and its memory representation.
+
+.PP
+File Representation refers to the format in which an ELF file is stored on 
disk.
+The fields in the file may use specific sizes, alignment, and byte ordering
+(endianness) that could be different from the native format used by the host
+system.
+
+.PP
+Memory Representation refers to the way the ELF data is organized when loaded
+into an application's memory. In memory, the data structures are typically
+converted into the native format of the host system (e.g., the system's
+endianness, word size, and alignment).
+
+.PP
+\fBlibelf\fP provides the following functions to translate ELF data between
+file and memory representations:
+.BR elf32_xlatetom ,
+.BR elf64_xlatetom ,
+.BR elf32_xlatetof ,
+and
+.BR elf64_xlatetof .
+
+See
+.BR elf32_xlatetom (3)
+for more information.
+
+.SH ELF VERSION
+
+To account for the possibility of multiple versions of the ELF specification,
+the ELF version number must be specified with the \fBelf_version\fP function
+before any other \fBlibelf\fP functions. This function sets \fBlibelf\fP's ELF
+version to the specified value.  At this time the only supported ELF version is
+\fBEV_CURRENT\fP.
+
+.SH DESCRIPTORS
+.I Elf
+descriptors the central \fBlibelf\fP object for accessing and manipulating
+ELF files.  They are created with the
+.BR elf_begin ,
+.BR elf_clone ,
+and
+.B elf_memory
+functions and closed with the
+.B elf_end
+function.
+
+\fBlibelf\fP also provides
+.I Elf_Scn
+and
+.I Elf_Data
+descriptors for ELF sections and section contents, respectively.  Members
+of the
+.I Elf_Data
+struct are described below.
+Members of the
+.I Elf
+and
+.I Elf_Scn
+structs are hidden from applications.
+
+These descriptors can be acquired and modified using various
+functions provided by \fBlibelf\fP.  See
+.B libelf.h
+for a complete list.
+
+.SH ERROR HANDLING
+If a \fBlibelf\fP function encounters an error it will set an internal
+error code that can be retrieved with
+.BR elf_errno .
+Each thread maintains its own separate error code.  The meaning of
+each error code can be determined with
+.BR elf_errmsg,
+which returns a string describing the error.
+
+.SH MEMORY MANAGEMENT
+\fBlibelf\fP manages all of the memory it allocates and frees it with
+.BR elf_end .
+The application must not call
+.B free
+on any memory allocated by \fBlibelf\fP.
+
+.SH NAMESPACE
+\fBlibelf\fP uses the following prefix format. See \fBlibelf.h\fP for more
+information.
+
+.RS
+.TP
+.PD 0
+.TP
+.B elf_
+Functions usable with both 32-bit and 64

Re: [PATCH 2/4] eu-stacktrace: configury for initial version (x86/sysprof only)

[Aaron Merey]

On Thu, Oct 17, 2024 at 11:28 AM Serhei Makarov  wrote:
>
> On Wed, Oct 16, 2024, at 11:38 PM, Aaron Merey wrote:
> > Using less wide indentation throughout would help make the configure.ac
> > changes more readable.  Otherwise this patch LGTM.
> Ok, my code imitated the offset for the debuginfod configury immediately 
> above,
> but I'll update the patch to use 3 spaces instead (seen elsewhere in the 
> file).
> Or, do you have a different preference for the indentation?

You're right we are inconsistent with indentation. Because this patch
adds particularly long lines, I'd use 3 spaces here.

Aaron

Re: [PATCH 1/4] eu-stacktrace: add eu-stacktrace tool

[Serhei Makarov]

On Wed, Oct 16, 2024, at 11:37 PM, Aaron Merey wrote:
> Please add an eu-stacktrace entry to NEWS.
Ok, will do.

> I tried to run this but I kept getting:
> Failed to complete recording: GDBus.Error[...]: Action 
> org.gnome.sysprof3.profile is not registered
Right. To have sysprof work requires the sysprof polkit action to be installed.
Discussed it on IRC, not sure I've figured out yet where your setup is different
from mine and Frank's. I think I'll post PATCH v2 but it might be the case that
we'll need to add more clarifications to the build instructions.

> I ran eu-stacktrace under `valgrind --leak-check=full`.  Memory
> allocated at stacktrace.c:1542 and 1543 is leaked.
Ok, added the free() towards the end of main().

> I got a "conflicting types" error when trying to compile this due to
> SysprofCaptureStackUser and SysprofCaptureUserRegs being defined both
> here and in sysprof-capture-types.h.
>
> Should these structs instead be defined when HAVE_SYSPROF_HEADERS is 0?
> I was able to build eu-stacktrace by making that change.
Ok, this one I think I've resolved.

The idea here was to be able to build eu-stacktrace if 
 points to a system sysprof package in 
/usr/include rather than the
patched sysprof.

The #ifndef SYSPROF_CAPTURE_FRAME_STACK_USER was supposed to guard
against duplicate declarations (this will be defined in the patched
sysprof-capture-types header). Frank pointed out this won't work
since that symbol is from an enum. Amending to use a macro in the check:

#if SYSPROF_CAPTURE_FRAME_LAST < 19

This should become unnecessary once there's a released version of
sysprof with the declarations. Then we'll just require sysprof version N.M.etc

> Also the patches that modify other parts of elfutils aren't risky IMO.
>
> I'm comfortable with this being merged for the release as long as any
> build errors are sorted out and we emphasize in the usage text and
> NEWS that eu-stacktrace is an experimental feature with a possibly
> unstable ABI.
I think the key point is that for now eu-stacktrace has to be explicitly
enabled in the configury. If the user doesn't ask for it, it won't be built.
We'll revisit the status of the feature when there's a released Sysprof
version with the stack sampling included.

Would the following wording for NEWS work?

stacktrace: Experimental new tool that can process a stream of stack
samples from the Sysprof profiler and unwind them into call
chains. Enable on x86 with --enable-stacktrace. Requires a
matching set of patches for Sysprof (available at

https://git.sr.ht/~serhei/sysprof-experiments/log/serhei/samples-via-fifo).

-- 
All the best,
Serhei
http://serhei.io

[Bug libelf/32102] elf_compress doesn't handle multiple Elf_Data chunks created with elf_newdata correctly

[mark at klomp dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=32102

Mark Wielaard  changed:

   What|Removed |Added

   Assignee|unassigned at sourceware dot org   |mark at klomp dot org
 Status|NEW |ASSIGNED

--- Comment #1 from Mark Wielaard  ---
https://inbox.sourceware.org/elfutils-devel/20241017155901.750192-1-m...@klomp.org/

-- 
You are receiving this mail because:
You are on the CC list for the bug.

Re: [PATCH 1/4] eu-stacktrace: add eu-stacktrace tool

[Aaron Merey]

On Thu, Oct 17, 2024 at 11:50 AM Serhei Makarov  wrote:
>
> > I'm comfortable with this being merged for the release as long as any
> > build errors are sorted out and we emphasize in the usage text and
> > NEWS that eu-stacktrace is an experimental feature with a possibly
> > unstable ABI.
> I think the key point is that for now eu-stacktrace has to be explicitly
> enabled in the configury. If the user doesn't ask for it, it won't be built.
> We'll revisit the status of the feature when there's a released Sysprof
> version with the stack sampling included.

I'm seeing eu-stacktrace built by default on x86 with no
--enable-stacktrace given to configure.

$ ./configure
[...]
eu-stacktrace support  : yes

> Would the following wording for NEWS work?
>
> stacktrace: Experimental new tool that can process a stream of stack
> samples from the Sysprof profiler and unwind them into call
> chains. Enable on x86 with --enable-stacktrace. Requires a
> matching set of patches for Sysprof (available at
> 
> https://git.sr.ht/~serhei/sysprof-experiments/log/serhei/samples-via-fifo).

LGTM. Can we change the eu-stacktrace usage text to mention that this
is an experimental tool? And can "see README.eu-stacktrace in the
source branch." include the URL?

Thanks,
Aaron

Re: [PATCH 4/4] eu-stacktrace: add unwind origin diagnostics to eu-stack

[Mark Wielaard]

Hi Serhei,

On Tue, 2024-10-15 at 11:26 -0400, Serhei Makarov wrote:
> Since we obtain diagnostics about unwind method, another logical place
> to show them is in eu-stack.
> 
> This requires an additional pseudo-unwind type DWFL_UNWOUND_INLINE to
> clarify what happens with entries for inlined functions (which
> eu-stacktrace does not display) based on the same Dwfl_Frame.
> 
> * libdwfl/libdwfl.h (Dwfl_Unwound_Source): Add DWFL_UNWOUND_INLINE.
> * libdwfl/dwfl_frame.c (dwfl_unwound_source_str):
>   Handle DWFL_UNWOUND_INLINE.

This part looks a little iffy IMHO.
It feels like a layering violation.
The meaning of DWFL_UNWOUND_INLINE is also different from the others.
There is no worst/best ordering here.
They are not connected to frames.

Is there a way to move the printing of this into stack.c?
For example make print_frame take a decorator argument that is a const
char * instead of an unwound_source argument.

Thanks,

Mark

Re: [PATCH 3/4] eu-stacktrace: add unwind origin diagnostics

[Serhei Makarov]

On Wed, Oct 16, 2024, at 11:39 PM, Aaron Merey wrote:
> Hi Serhei,
>
> On Tue, Oct 15, 2024 at 11:28 AM Serhei Makarov  wrote:
>> 
>> Track the method used to unwind each Dwfl_Frame using an enum field
>> unwound_source; provide access to the field. Then use that in
>> eu-stacktrace to display the unwind methods used for each process.
>> 
>> This is an important diagnostic to verify how many processes are
>> adequately covered by the eh_frame unwinder.
>> 
>> * libdw/libdw.map (ELFUTILS_0.192): Add dwfl_frame_unwound_source,
>>   dwfl_unwound_source_str.
>> * libdwfl/libdwfl.h (Dwfl_Unwound_Source): New enum.
>>   (dwfl_frame_unwound_source)
>>   (dwfl_unwound_source_str): New functions.
>> * libdwfl/dwfl_frame.c (dwfl_frame_unwound_source)
>>   (dwfl_unwound_source_str): New functions.
>> * libdwfl/dwflP.h: Add INTDECL for dwfl_frame_unwound_source,
>
> Should be libdwflP.h.
Fixed.

> It might be worth adding a "Keep this the last entry." comment right
> above DWFL_UNWOUND_NUM.  This is done for libelf.h enums.
Fixed.

>
> Also is DWFL_UNWOUND_UNKNOWN needed? It isn't used anywhere.
Removed. An earlier version of the code required it, but this one no longer 
does.

> What does worst_unwound mean? It looks like the enum values are ordered.
> A comment that describes this ordering would be helpful.
Added comments at the fprintf that displays (last %s, worst %s)
and at the enum definition (also important):

"This identifies the method used to unwind a particular Dwfl_Frame.
The enum order matches the order of preference for unwinding
(i.e. eh_frame is preferred to dwarf is preferred to ebl)."

Re: [PATCH 2/4] eu-stacktrace: configury for initial version (x86/sysprof only)

[Serhei Makarov]

On Wed, Oct 16, 2024, at 11:38 PM, Aaron Merey wrote:
> Using less wide indentation throughout would help make the configure.ac
> changes more readable.  Otherwise this patch LGTM.
Ok, my code imitated the offset for the debuginfod configury immediately above,
but I'll update the patch to use 3 spaces instead (seen elsewhere in the file).
Or, do you have a different preference for the indentation?

[PATCH] libelf: elf_compress doesn't handle multiple elf_newdata chunks correctly

[Mark Wielaard]

elf_compress would compress all (new) data chunks, but didn't reset
the section data_list. This would cause extra data to be returned
after decompression or create bad compressed data. Add a new testcase
for this and explicitly zap the scn->data_list before resetting the
elf section raw data after (de)compression.

 * libelf/elf_compress.c (__libelf_reset_rawdata): Cleanup
 scn->data_list.
 * tests/newzdata.c: New testcase.
 * tests/Makefile.am (check_PROGRAMS): Add newzdata.
 (TESTS): Likewise.
 (newzdata_LDADD): New variable.

https://sourceware.org/bugzilla/show_bug.cgi?id=32102

Signed-off-by: Mark Wielaard 
---
 libelf/elf_compress.c |  10 +
 tests/Makefile.am |   7 +-
 tests/newzdata.c  | 500 ++
 3 files changed, 515 insertions(+), 2 deletions(-)
 create mode 100644 tests/newzdata.c

diff --git a/libelf/elf_compress.c b/libelf/elf_compress.c
index 0ad6a32a7b2f..2cd32a6f345b 100644
--- a/libelf/elf_compress.c
+++ b/libelf/elf_compress.c
@@ -519,7 +519,17 @@ __libelf_reset_rawdata (Elf_Scn *scn, void *buf, size_t 
size, size_t align,
   scn->rawdata.d.d_align = align;
   scn->rawdata.d.d_type = type;
 
+  /* Remove the old data.  */
+  Elf_Data_List *runp = scn->data_list.next;
+  while (runp != NULL)
+{
+  Elf_Data_List *oldp = runp;
+  runp = runp->next;
+  if ((oldp->flags & ELF_F_MALLOCED) != 0)
+   free (oldp);
+}
   /* Existing existing data is no longer valid.  */
+  scn->data_list.next = NULL;
   scn->data_list_rear = NULL;
   if (scn->data_base != scn->rawdata_base)
 free (scn->data_base);
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 424c184bc200..ff07f7dc3f0b 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -52,7 +52,8 @@ check_PROGRAMS = arextract arsymtest newfile saridx scnnames 
sectiondump \
  backtrace-data backtrace-dwarf debuglink debugaltlink \
  buildid deleted deleted-lib.so aggregate_size peel_type \
  vdsosyms \
- getsrc_die strptr newdata elfstrtab dwfl-proc-attach \
+ getsrc_die strptr newdata newzdata \
+ elfstrtab dwfl-proc-attach \
  elfshphehdr elfstrmerge dwelfgnucompressed elfgetchdr \
  elfgetzdata elfputzdata zstrptr emptyfile vendorelf \
  fillfile dwarf_default_lower_bound dwarf-die-addr-die \
@@ -183,7 +184,8 @@ TESTS = run-arextract.sh run-arsymtest.sh run-ar.sh newfile 
test-nlist \
run-readelf-dwz-multi.sh run-allfcts-multi.sh run-deleted.sh \
run-linkmap-cut.sh run-aggregate-size.sh run-peel-type.sh \
vdsosyms run-readelf-A.sh \
-   run-getsrc-die.sh run-strptr.sh newdata elfstrtab dwfl-proc-attach \
+   run-getsrc-die.sh run-strptr.sh newdata newzdata \
+   elfstrtab dwfl-proc-attach \
elfshphehdr run-lfs-symbols.sh run-dwelfgnucompressed.sh \
run-elfgetchdr.sh \
run-elfgetzdata.sh run-elfputzdata.sh run-zstrptr.sh \
@@ -824,6 +826,7 @@ vdsosyms_LDADD = $(libeu) $(libdw) $(libelf)
 getsrc_die_LDADD = $(libeu) $(libdw) $(libelf)
 strptr_LDADD = $(libelf)
 newdata_LDADD = $(libelf)
+newzdata_LDADD = $(libelf)
 elfstrtab_LDADD = $(libelf)
 dwfl_proc_attach_LDADD = $(libeu) $(libdw)
 dwfl_proc_attach_LDFLAGS = -pthread -rdynamic $(AM_LDFLAGS)
diff --git a/tests/newzdata.c b/tests/newzdata.c
new file mode 100644
index ..7e6abed2fd04
--- /dev/null
+++ b/tests/newzdata.c
@@ -0,0 +1,500 @@
+/* Test program for elf_newdata function with compressed elf sections.
+   Copyright (C) 2015 Red Hat, Inc.
+   Copyright (C) 2024 Mark J. Wielaard 
+   This file is part of elfutils.
+
+   This file is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   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 a copy of the GNU General Public License
+   along with this program.  If not, see .  */
+
+#ifdef HAVE_CONFIG_H
+# include 
+#endif
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "system.h"
+
+#include ELFUTILS_HEADER(elf)
+#include 
+
+// Random data string (16 bytes, 15 chars plus zero-terminator).
+static char *DATA = "123456789ABCDEF";
+static size_t DATA_LEN = 16;
+
+static void
+add_section_data (Elf *elf, char *buf, size_t len)
+{
+  printf ("Adding %zd bytes.\n", len);
+
+  Elf_Scn *scn = elf_getscn (elf, 1);
+  if (scn == NULL)
+{
+  printf ("couldn't get data section: %s\n", elf_errmsg (-1));
+  exit (1);
+}
+
+  Elf_Data *data 

Re: [PATCH 1/4] eu-stacktrace: add eu-stacktrace tool

[Serhei Makarov]

On Thu, Oct 17, 2024, at 12:18 PM, Aaron Merey wrote:
> I'm seeing eu-stacktrace built by default on x86 with no
> --enable-stacktrace given to configure.
Now I'm waffling back and forth on what to do.

Mark wants to enable eu-stacktrace build on one of the buildbots.
The current behaviour (which does build eu-stacktrace
IF x86 asm/perf_regs.h AND /usr/include/sysprof-* are found)
would be fine for that.
Otherwise the ./configure command for that buildbot needs to be changed.

> LGTM. Can we change the eu-stacktrace usage text to mention that this
> is an experimental tool? And can "see README.eu-stacktrace in the
> source branch." include the URL?
Yes to both, fixing now.

Could also change the NEWS to

stacktrace: Experimental new tool that can process a stream of stack
samples from the Sysprof profiler and unwind them into call
chains. Enable on x86 with --enable-stacktrace. See
README.eu-stacktrace in the development branch for detailed
usage instructions:

https://sourceware.org/cgit/elfutils/tree/README.eu-stacktrace?h=users/serhei/eu-stacktrace

Re: [PATCH 4/4] eu-stacktrace: add unwind origin diagnostics to eu-stack

[Serhei Makarov]

On Thu, Oct 17, 2024, at 1:00 PM, Mark Wielaard wrote:
> This part looks a little iffy IMHO.
> It feels like a layering violation.
> The meaning of DWFL_UNWOUND_INLINE is also different from the others.
> There is no worst/best ordering here.
> They are not connected to frames.
>
> Is there a way to move the printing of this into stack.c?
> For example make print_frame take a decorator argument that is a const
> char * instead of an unwound_source argument.

Ok, just to be clear that I understand what you want.

The unwound_source arguments (in print_frame+print_inline_frames)
changed to const char *. The print_frames() function
invokes unwound_source_str() to supply that argument.

struct frame still stores Dwfl_Unwound_Source, not char *.

In print_inline_frames the print_frame invocation is changed to
> print_frame (..., i > 1 ? "inline" : unwound_source)

DWFL_UNWOUND_INLINE removed from the enum,
making the enum a straightforward sequence of unwind methods.

I can do that.

Re: [PATCH 4/4] eu-stacktrace: add unwind origin diagnostics to eu-stack

[Mark Wielaard]

Hi Serhei,

On Thu, 2024-10-17 at 13:13 -0400, Serhei Makarov wrote:
> On Thu, Oct 17, 2024, at 1:00 PM, Mark Wielaard wrote:
> > This part looks a little iffy IMHO.
> > It feels like a layering violation.
> > The meaning of DWFL_UNWOUND_INLINE is also different from the others.
> > There is no worst/best ordering here.
> > They are not connected to frames.
> > 
> > Is there a way to move the printing of this into stack.c?
> > For example make print_frame take a decorator argument that is a const
> > char * instead of an unwound_source argument.
> 
> Ok, just to be clear that I understand what you want.
> 
> The unwound_source arguments (in print_frame+print_inline_frames)
> changed to const char *. The print_frames() function
> invokes unwound_source_str() to supply that argument.

I think (but haven't tried) only print_frame has to have a char *
argument, print_inline_frames and print_frames can still pass around an
Dwfl_Unwound_Source. Both print_frames and print_inline_frames can then
call print_frame with the string argument from unwound_source_str (or
the static string "inline" in the print_inline_frames case for the
"bottom" frame).

> struct frame still stores Dwfl_Unwound_Source, not char *.

Yep.

> In print_inline_frames the print_frame invocation is changed to
> > print_frame (..., i > 1 ? "inline" : unwound_source)

The second call, yes. But with "inline" :
unwound_source_str(unwound_source). The first call to print_frame would
just use unwound_source_str(unwound_source) directly.

> DWFL_UNWOUND_INLINE removed from the enum,
> making the enum a straightforward sequence of unwind methods.
> 
> I can do that.

Great.

Thanks,

Mark

Re: [PATCH 1/4] eu-stacktrace: add eu-stacktrace tool

[Aaron Merey]

On Thu, Oct 17, 2024 at 1:09 PM Serhei Makarov  wrote:
> On Thu, Oct 17, 2024, at 12:18 PM, Aaron Merey wrote:
> > I'm seeing eu-stacktrace built by default on x86 with no
> > --enable-stacktrace given to configure.
> Now I'm waffling back and forth on what to do.
>
> Mark wants to enable eu-stacktrace build on one of the buildbots.
> The current behaviour (which does build eu-stacktrace
> IF x86 asm/perf_regs.h AND /usr/include/sysprof-* are found)
> would be fine for that.
> Otherwise the ./configure command for that buildbot needs to be changed.

IMO let's disable by default for now since build issues/errors will be
less likely. But if others prefer it enabled by default on x86 I'm not
opposed to that.

>
> > LGTM. Can we change the eu-stacktrace usage text to mention that this
> > is an experimental tool? And can "see README.eu-stacktrace in the
> > source branch." include the URL?
> Yes to both, fixing now.
>
> Could also change the NEWS to
>
> stacktrace: Experimental new tool that can process a stream of stack
> samples from the Sysprof profiler and unwind them into call
> chains. Enable on x86 with --enable-stacktrace. See
> README.eu-stacktrace in the development branch for detailed
> usage instructions:
> 
> https://sourceware.org/cgit/elfutils/tree/README.eu-stacktrace?h=users/serhei/eu-stacktrace

Good idea to include the URL there too.

Thanks,
Aaron

Re: [PATCH 4/4] eu-stacktrace: add unwind origin diagnostics to eu-stack

[Serhei Makarov]

On Thu, Oct 17, 2024, at 1:22 PM, Mark Wielaard wrote:
> I think (but haven't tried) only print_frame has to have a char *
> argument, print_inline_frames and print_frames can still pass around an
> Dwfl_Unwound_Source. Both print_frames and print_inline_frames can then
> call print_frame with the string argument from unwound_source_str (or
> the static string "inline" in the print_inline_frames case for the
> "bottom" frame).
I can also do that. The asymmetry between print_frames/print_inline_frames is a 
bit weird.

One more needed simplification I noticed:

i > 1 ? "inline" : ...
can just be replaced with "inline"

The prior lines have a guard that skips the scope if dwarf_tag(scope) != 
DW_TAG_inlined_subroutine, hence anything printed through that invocation is 
already known to be an inlined invocation.