Re: [PATCH 2/9 v2] doc: Add elf32_fsize.3 and elf64_fsize.3

[Mark Wielaard]

Hi Aaron,

On Wed, 2024-10-02 at 22:26 -0400, Aaron Merey wrote:
> Signed-off-by: Aaron Merey 
> 
> ---
> v2 changes:
> Reword description.
> 
> State EV_CURRENT is the only valid version.
> 
> Mention possibility of integer overflow.
> 
> On Tue, Aug 27, 2024 at 12:32 PM Mark Wielaard  wrote:
> > 
> > Maybe give at least some examples of Elf_Type and which data structure
> > they represent? ELF_T_ADDR (32 bit address), ELF_T_EHDR (Elf32_Ehdr),
> > etc.
> > 
> > And mention that elf_getdata will set the Elf_Data d_type to the
> > Elf_Type of the section?
> 
> I added "See libelf(3) for more information regarding Elf_Type" to
> the description.  This man page doesn't exist yet but I will add it
> and include a list of the different Elf_Type and its relationship to
> Elf_Data.

Nice. This version of the elf(32|64)_fsize man page looks good to me.

Thanks,

Mark

>  doc/elf32_fsize.3 | 78 +++
>  doc/elf64_fsize.3 |  1 +
>  2 files changed, 79 insertions(+)
>  create mode 100644 doc/elf32_fsize.3
>  create mode 100644 doc/elf64_fsize.3
> 
> diff --git a/doc/elf32_fsize.3 b/doc/elf32_fsize.3
> new file mode 100644
> index ..a0aac70e
> --- /dev/null
> +++ b/doc/elf32_fsize.3
> @@ -0,0 +1,78 @@
> +.TH ELF32_FSIZE 3 2024-08-14 "Libelf" "Libelf Programmer's Manual"
> +
> +.SH NAME
> +elf32_fsize, elf64_fsize \- calculate the file size of an ELF data structure
> +
> +.SH SYNOPSIS
> +.nf
> +.B #include 
> +
> +.BI "size_t elf32_fsize(Elf_Type " type ", size_t " count ", unsigned int " 
> version ");"
> +.BI "size_t elf64_fsize(Elf_Type " type ", size_t " count ", unsigned int " 
> version ");"
> +
> +.SH DESCRIPTION
> +Given an
> +.B Elf_Type
> +representation of a core ELF structure as well as the number of items, return
> +the number of bytes needed for the on-disk representation in a 32-bit or 
> 64-bit
> +ELF file.  The on-disk and in-memory representations of
> +.B Elf_Type
> +are assumed to be the same. See
> +.BR libelf (3)
> +for more information regarding
> +.BR Elf_Type .
> +
> +.SH PARAMETERS
> +.TP
> +.I type
> +The ELF data structure type for which the file size is to be calculated.
> +
> +.TP
> +.I count
> +The number of elements of the specified type.
> +
> +.TP
> +.I version
> +The ELF version. This should be set to
> +.BR EV_CURRENT ,
> +which is the only valid value.
> +
> +.SH RETURN VALUE
> +The size in bytes of the specified count and type of data structure.
> +If version is not set to
> +.B EV_CURRENT
> +or
> +.I type
> +is not a valid
> +.BR Elf_Type ,
> +return 0 and set a libelf error code. Integer overflow can occur if
> +the size of
> +.I type
> +multiplied by
> +.I count
> +is greater than
> +.BR SIZE_MAX .
> +
> +.SH SEE ALSO
> +.BR elf_errno (3),
> +.BR libelf (3),
> +.BR elf (5)
> +
> +.SH ATTRIBUTES
> +For an explanation of the terms used in this section, see
> +.BR attributes (7).
> +.TS
> +allbox;
> +lbx lb lb
> +l l l.
> +InterfaceAttribute   Value
> +T{
> +.na
> +.nh
> +.BR elf32_fsize (),
> +.BR elf64_fsize ()
> +T}   Thread safety   MT-Safe
> +.TE
> +
> +.SH REPORTING BUGS
> +Report bugs to  or 
> https://sourceware.org/bugzilla/.
> diff --git a/doc/elf64_fsize.3 b/doc/elf64_fsize.3
> new file mode 100644
> index ..178152ec
> --- /dev/null
> +++ b/doc/elf64_fsize.3
> @@ -0,0 +1 @@
> +.so man3/elf32_fsize.3

Re: [PATCH 3/9 v2] doc: Add elf32_getchdr.3 and elf64_getchdr.3

[Mark Wielaard]

Hi Aaron,

On Wed, 2024-10-02 at 22:26 -0400, Aaron Merey wrote:
> Signed-off-by: Aaron Merey 
> ---
> 
> v2 changes:
> 
> Mention that SHF_COMPRESSED must be set.
> 
> Add elf_compress (3) to SEE ALSO.
> 
> Remove "This elfutils libelf function may not be
> found in other libelf implementations".
> 
> On Tue, Aug 27, 2024 at 1:23 PM Mark Wielaard  wrote:
> > 
> > It should also mention what the Elf32_Chdr structure looks
> > like and what the meaning the fields have. What the legal values of
> > ch_type are, that ch_size is the uncompressed section data size, and
> > that ch_addralign is the alignment of the uncompressed data.
> 
> I will include this information in the upcoming libelf man page.

OK. Then this version of the man page looks good.

Thanks,

Mark

>  doc/elf32_getchdr.3 | 60 +
>  doc/elf64_getchdr.3 |  1 +
>  2 files changed, 61 insertions(+)
>  create mode 100644 doc/elf32_getchdr.3
>  create mode 100644 doc/elf64_getchdr.3
> 
> diff --git a/doc/elf32_getchdr.3 b/doc/elf32_getchdr.3
> new file mode 100644
> index ..f7f35c96
> --- /dev/null
> +++ b/doc/elf32_getchdr.3
> @@ -0,0 +1,60 @@
> +.TH ELF32_GETCHDR 3 2024-08-14 "Libelf" "Libelf Programmer's Manual"
> +
> +.SH NAME
> +elf32_getchdr, elf64_getchdr \- retrieve the compression header for a
> +section from a 32-bit or 64-bit ELF object file.
> +
> +.SH SYNOPSIS
> +.nf
> +.B #include 
> +
> +.BI "Elf32_Chdr *elf32_getchdr(Elf_Scn *" scn ");"
> +.BI "Elf64_Chdr *elf64_getchdr(Elf_Scn *" scn ");"
> +
> +.SH DESCRIPTION
> +Retrieve the compression header for a section with compressed data.
> +Sections with compressed data are indicated with the
> +.B SHF_COMPRESSED
> +flag.  See
> +.BR libelf (3)
> +for more information regarding the compression header.
> +
> +.SH PARAMETERS
> +.TP
> +.I scn
> +Section whose compression header will be retrieved. The section's
> +.B SHF_COMPRESSED
> +flag must be set.
> +
> +.SH RETURN VALUE
> +On success, return a pointer to the compression header. On failure,
> +return NULL and set a libelf error code.
> +
> +.SH SEE ALSO
> +.BR elf_compress (3),
> +.BR elf_errno (3),
> +.BR libelf (3),
> +.BR elf (5)
> +
> +.SH ATTRIBUTES
> +For an explanation of the terms used in this section, see
> +.BR attributes (7).
> +.TS
> +allbox;
> +lbx lb lb
> +l l l.
> +InterfaceAttribute   Value
> +T{
> +.na
> +.nh
> +.BR elf32_getchdr (),
> +.BR elf64_getchdr ()
> +T}   Thread safety   MT-Safe
> +.TE
> +
> +.SH REPORTING BUGS
> +Report bugs to  or 
> https://sourceware.org/bugzilla/.
> +
> +.SH HISTORY
> +.B elf32_getchdr
> +first appeared in elfutils 0.165.
> diff --git a/doc/elf64_getchdr.3 b/doc/elf64_getchdr.3
> new file mode 100644
> index ..fa49616b
> --- /dev/null
> +++ b/doc/elf64_getchdr.3
> @@ -0,0 +1 @@
> +.so man3/elf32_getchdr.3

Re: [PATCH 1/6] lib: Add missing config.h include to next_prime.c

[Michael Pratt]

Hi Mark,

On Monday, October 14th, 2024 at 16:38, Mark Wielaard  wrote:

> 
> 
> There isn't much code in this file. What kind of build issue are you
> seeing without this? Does config.h contain some construct that affects
> the build of this file?


Like I mentioned in the cover letter, we incorporate gnulib to the build,
so the build simply fails due to their strict requirements
of including config.h before including any other header that gnulib provides.

I'm not familiar with the exact reason for this requirement,
but I'm sure it could potentially cause a problem if one overrides it.

--
MCP

Re: [PATCH 1/9 v2] doc: Add elf32_checksum.3 and elf64_checksum.3

[Mark Wielaard]

Hi Aaron,

On Wed, 2024-10-02 at 22:26 -0400, Aaron Merey wrote:
> Signed-off-by: Aaron Merey 
> 
> ---
> v2 changes:
> elf64_checksum.3 now refers to elf32_checksum.3.
> 
> Explained which sections are used to calculate the checksum.
> 
> Mentioned DT_CHECKSUM.
> 
> Mentioned that the checksum is a 4-byte value that might be
> zero-extended.

Looks good. Please check one sentence below.

>  doc/elf32_checksum.3 | 60 
>  doc/elf64_checksum.3 |  1 +
>  2 files changed, 61 insertions(+)
>  create mode 100644 doc/elf32_checksum.3
>  create mode 100644 doc/elf64_checksum.3
> 
> diff --git a/doc/elf32_checksum.3 b/doc/elf32_checksum.3
> new file mode 100644
> index ..ab707f15
> --- /dev/null
> +++ b/doc/elf32_checksum.3
> @@ -0,0 +1,60 @@
> +.TH ELF32_CHECKSUM 3 2024-08-14 "Libelf" "Libelf Programmer's Manual"
> +
> +.SH NAME
> +elf32_checksum, elf64_checksum \- compute the checksum for a 32-bit or 64-bit
> +ELF object file
> +
> +.SH SYNOPSIS
> +.nf
> +.B #include 
> +
> +.BI "long int elf32_checksum(Elf *" elf ");"
> +.BI "long int elf64_checksum(Elf *" elf ");"
> +
> +.SH DESCRIPTION
> +Compute a checksum for the ELF object file referred to by
> +.IR elf .
> +The checksum is computed from permanent parts of the ELF file and
> +the result is repeatable.  To be repeatable, strippable sections are
> +not included in computing the checksum.
> +.B SHT_NOBITS
> +sections are also not included when computing the checksum.  The checksum
> +can be used as a value for
> +.BR DT_CHECKSUM .
> +
> +.SH PARAMETERS
> +.TP
> +.I elf
> +The ELF object file for which the checksum is to be computed.
> +
> +.SH RETURN VALUE
> +On success, return the computed checksum. If an error occurs, return -1
> +and set a libelf error code.  The checksum is always calculated a 4-byte
> +value.
> 

Should that be "claculated as a 4-byte value." ?

>  If
> +.I long int
> +is larger than 4 bytes, then the checksum will be extended by padding
> +with zeros.
> +
> +.SH SEE ALSO
> +.BR elf_errno (3),
> +.BR libelf (3),
> +.BR elf (5)
> +
> +.SH ATTRIBUTES
> +For an explanation of the terms used in this section, see
> +.BR attributes (7).
> +.TS
> +allbox;
> +lbx lb lb
> +l l l.
> +InterfaceAttribute   Value
> +T{
> +.na
> +.nh
> +.BR elf32_checksum (),
> +.BR elf64_checksum ()
> +T}   Thread safety   MT-Safe
> +.TE
> +
> +.SH REPORTING BUGS
> +Report bugs to  or 
> https://sourceware.org/bugzilla/.
> diff --git a/doc/elf64_checksum.3 b/doc/elf64_checksum.3
> new file mode 100644
> index ..16d3cc24
> --- /dev/null
> +++ b/doc/elf64_checksum.3
> @@ -0,0 +1 @@
> +.so man3/elf32_checksum.3

Re: [PATCH 5/9 v2] doc: Add elf32_getshdr.3 and elf64_getshdr.3

[Mark Wielaard]

Hi Aaron,

On Wed, 2024-10-02 at 22:26 -0400, Aaron Merey wrote:
> Signed-off-by: Aaron Merey 
> ---
> 
> v2 changes:
> Mention elf_flagshdr.
> 
> Mention NULL is returned if scn is NULL.

This version looks good to me.

Thanks,

Mark

Re: [PATCH 7/9 v2] doc: Add elf32_newphdr.3 and elf64_newphdr.3

[Mark Wielaard]

Hi Aaron,

On Wed, 2024-10-02 at 22:26 -0400, Aaron Merey wrote:
> v2 changes:
> Mention new program header table is zero'ed out and that
> NULL is returned if elf is NULL.

Looks good.

Thanks,

Mark

Re: [PATCH 4/9 v2] doc: Add elf32_getphdr.3 and elf64_getphdr.3

[Mark Wielaard]

Hi Aaron,

On Wed, 2024-10-02 at 22:26 -0400, Aaron Merey wrote:
> Signed-off-by: Aaron Merey 
> ---
> v2 changes:
> Improved description.
> 
> Mention that NULL is returned if there is no program header.
> 
> Add elf_getphdrnum and elf32_newphdr to SEE ALSO.

This version looks good to me.

Thanks,

Mark

Re: [PATCH 6/9 v2] doc: Add elf32_newehdr.3 and elf64_newehdr.3

[Mark Wielaard]

Hi Aaron,

On Wed, 2024-10-02 at 22:26 -0400, Aaron Merey wrote:
> v2 changes:
> Mention that the existing header will be returned if one is
> already present.
> 
> Mention that an ELF header must be present before callling
> elf_newscn or elf_newphdr

Looks good to me.

Thanks,

Mark

Re: [PATCH] PR32218: debuginfod-client: support very long source file

[Mark Wielaard]

Hi Frank,

On Thu, 2024-10-10 at 17:24 -0400, Frank Ch. Eigler wrote:
> From be79d138989b9968f9c687ef62cc91b5b93e32b5 Mon Sep 17 00:00:00 2001
> From: "Frank Ch. Eigler" 
> Date: Thu, 10 Oct 2024 16:30:19 -0400
> Subject: [PATCH] PR32218: debuginfod-client: support very long source file
>  names
> 
> debuginfod clients & servers may sometimes encounter very long source
> file names.  Previously, the client would synthesize a path name like
>$CACHEDIR/$BUILDID/source-$PATHNAME
> where $PATHNAME was a funky ##-encoded version of the entire source
> path name.  This can get too long to store as a single component
> of a file system pathname (e.g. linux/limits.h NAME_MAX), resulting
> on client-side errors even after a successful download.

Maybe add the bugzilla URL as an example of this.
I see it is in the subject, but it is nice to just click on something.

> New code switches encoding of the $PATHNAME part to use less escaping,
> and a merciless truncation to the tail part of the filename.  (We keep
> the tail rather than the head, so that the extension is preserved,
> which makes some consumers happier.)  To limit collision damage from
> truncation, we add also insert a goofy hash (4-byte elf_hash) of the
> name into the path name.

Is this 4-byte hash enough to prevent accidental collisions?

>   The result is a relatively short name:
> 
>$CACHEDIR/$BUILDID/source-$HASH-$NAMETAIL
> 
> This is a transparent change to clients, who are not to make any
> assumptions about cache file naming structure.

How does it interact with existing source file cache files?

>   However, one existing
> test did make such assumptions, so is fixed with some globness.  A new
> test is also added, using a pre-baked tarball with a very long srcfile
> name.  Some auxiliary makefiles are tweaked to satisfy the new
> libdebuginfod->libelf link order dependency.

Are you sure you want to use elf_hash? It returns an (arch dependent)
unsigned long int but the actual implementation only produces an
unsigned int. To simplify things and not need extra build dependencies
you might want to just include a hand-coded string hash function like
djb2? Which is really just 4 lines of code and probably a better hash
for strings than elf_hash.

> Signed-off-by: Frank Ch. Eigler 
> ---
>  debuginfod/Makefile.am|   4 +-
>  debuginfod/debuginfod-client.c|  76 +++---
>  src/Makefile.am   |   2 +-
>  tests/Makefile.am |   7 +-
>  .../bighello-sources/bighello.c   |   7 ++
>  .../bighello-sources/bighello.h   |   1 +
>  .../moremoremoremoremoremoremoremore  |   1 +
>  tests/debuginfod-tars/bighello.tar| Bin 0 -> 51200 bytes
>  tests/run-debuginfod-longsource.sh|  69 
>  tests/run-debuginfod-section.sh   |  16 ++--
>  10 files changed, 144 insertions(+), 39 deletions(-)
>  create mode 100644 tests/debuginfod-tars/bighello-sources/bighello.c
>  create mode 100644 tests/debuginfod-tars/bighello-sources/bighello.h
>  create mode 12 
> tests/debuginfod-tars/bighello-sources/moremoremoremoremoremoremoremore
>  create mode 100644 tests/debuginfod-tars/bighello.tar
>  create mode 100755 tests/run-debuginfod-longsource.sh
> 
> diff --git a/debuginfod/Makefile.am b/debuginfod/Makefile.am
> index 5ad4e188c4c3..dbba2a437c80 100644
> --- a/debuginfod/Makefile.am
> +++ b/debuginfod/Makefile.am
> @@ -70,10 +70,10 @@ bin_PROGRAMS += debuginfod-find
>  endif
>  
>  debuginfod_SOURCES = debuginfod.cxx
> -debuginfod_LDADD = $(libdw) $(libelf) $(libeu) $(libdebuginfod) 
> $(argp_LDADD) $(fts_LIBS) $(libmicrohttpd_LIBS) $(sqlite3_LIBS) 
> $(libarchive_LIBS) $(rpm_LIBS) $(jsonc_LIBS) $(libcurl_LIBS) $(lzma_LIBS) 
> -lpthread -ldl
> +debuginfod_LDADD = $(libdw) $(libdebuginfod) $(libelf) $(libeu) 
> $(argp_LDADD) $(fts_LIBS) $(libmicrohttpd_LIBS) $(sqlite3_LIBS) 
> $(libarchive_LIBS) $(rpm_LIBS) $(jsonc_LIBS) $(libcurl_LIBS) $(lzma_LIBS) 
> -lpthread -ldl
>  
>  debuginfod_find_SOURCES = debuginfod-find.c
> -debuginfod_find_LDADD = $(libdw) $(libelf) $(libeu) $(libdebuginfod) 
> $(argp_LDADD) $(fts_LIBS) $(jsonc_LIBS)
> +debuginfod_find_LDADD = $(libdw) $(libdebuginfod) $(libelf) $(libeu) 
> $(argp_LDADD) $(fts_LIBS) $(jsonc_LIBS)
>  
>  if LIBDEBUGINFOD
>  noinst_LIBRARIES = libdebuginfod.a

Move libelf after libdebuginfod. OK (but see above, isn't it simpler to
just include a small string hash function right in the code itself?)

> diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
> index 24ede19af385..2f56f6f4d784 100644
> --- a/debuginfod/debuginfod-client.c
> +++ b/debuginfod/debuginfod-client.c
> @@ -1201,29 +1201,49 @@ perform_queries(CURLM *curlm, CURL **target_handle, 
> struct handle_data *data, de
>  /* Copy SRC to DEST, s,/,#,g */
>  
>  static void
> -path_escape (const char *src, char *dest)
> +path_escape (const char *src, char *des

[PATCH 1/4] eu-stacktrace: 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).

* src/stacktrace.c: Add new tool.

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

diff --git a/src/stacktrace.c b/src/stacktrace.c
new file mode 100644
index ..ebd914e5
--- /dev/null
+++ b/src/stacktrace.c
@@ -0,0 +1,1566 @@
+/* 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
+   PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDERS OR
+   CONTRIBUTORS 

[PATCH 2/4] eu-stacktrace: configury for 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 when
x86 architecture or sysprof headers are absent.

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.

* 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| 66 -
 src/Makefile.am |  9 ++-
 2 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 8f5901a2..1af1c708 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,69 @@ 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]),
+stacktrace_option_given=yes,
+stacktrace_option_given=no)
+# check for x86, or more precisely _ASM_X86_PERF_REGS_H
+AS_IF([test "x$enable_stacktrace" != "xno"], [
+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
+if test "x$stacktrace_option_given" = "xyes"; then
+AC_MSG_ERROR([${program_prefix}stacktrace 
currently only supports x86, use --disable-stacktrace to disable.])
+else
+AC_MSG_WARN([${program_prefix}stacktrace 
currently only supports x86, disabling by default on other architectures.])
+fi
+fi
+])
+# check for sysprof headers:
+AS_IF([test "x$enable_stacktrace" != "xno"], [
+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
+if test "x$stacktrace_option_given" = "xyes"; then
+AC_MSG_ERROR([sysprof headers for 
${program_prefix}stacktrace not found, use --disable-stacktrace to disable.])
+else
+AC_MSG_WARN([sysprof headers for 
${program_prefix}stacktrace not found, disabling by default.])
+fi
+fi
+],[
+# If eu-stacktrace is disabled, also disable the automake conditionals:
+ac_cv_has_sysprof_6_headers=no
+ac_cv_has_sysprof_4_

[PATCH 3/4] eu-stacktrace: 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/dwflP.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  | 20 +++-
 libdwfl/libdwflP.h |  5 -
 src/stacktrace.c   | 31 ++-
 6 files changed, 92 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_seterrn

[PATCH 4/4] eu-stacktrace: 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.

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.
* 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 argument and print it.
  (print_inline_frames): Take unwound_source argument and pass it on,
  except for subsequent frames where DWFL_UNWOUND_INLINE is assumed.
  (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 
---
 libdwfl/dwfl_frame.c  |  2 ++
 libdwfl/libdwfl.h |  1 +
 src/stack.c   | 31 ---
 tests/run-stack-i-test.sh | 15 ++-
 4 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/libdwfl/dwfl_frame.c b/libdwfl/dwfl_frame.c
index 2e6c6de8..3659420a 100644
--- a/libdwfl/dwfl_frame.c
+++ b/libdwfl/dwfl_frame.c
@@ -265,6 +265,8 @@ dwfl_unwound_source_str (Dwfl_Unwound_Source unwound_source)
   return "none";
 case DWFL_UNWOUND_INITIAL_FRAME:
   return "initial";
+case DWFL_UNWOUND_INLINE:
+  return "inline";
 case DWFL_UNWOUND_EH_CFI:
   return "eh_frame";
 case DWFL_UNWOUND_DWARF_CFI:
diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
index ffd951db..21cb7d8d 100644
--- a/libdwfl/libdwfl.h
+++ b/libdwfl/libdwfl.h
@@ -53,6 +53,7 @@ typedef struct Dwfl_Frame Dwfl_Frame;
 typedef enum {
 DWFL_UNWOUND_NONE = 0,
 DWFL_UNWOUND_INITIAL_FRAME,
+DWFL_UNWOUND_INLINE, /* XXX used for src/stack.c print_inline_frames() */
 DWFL_UNWOUND_EH_CFI,
 DWFL_UNWOUND_DWARF_CFI,
 DWFL_UNWOUND_EBL,
diff --git a/src/stack.c b/src/stack.c
index eb87f5f1..baee9cae 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, Dwfl_Unwound_Source 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]", dwfl_unwound_source_str (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, unwound_source);
 
   /* last_scope is the source location where the next frame/function
 call was done. */
@@ -349,7 +358,8 @@ print_inline_frames (int *nr, Dwarf_Addr pc, bool 
isac

Re: [PATCH 6/6] Use file stream or format variants of stdio print functions

[Michael Pratt]

Hi again,


On Monday, October 14th, 2024 at 17:30, Mark Wielaard  wrote:

> 
> 
> If we are trying to be more consistent then I think I would prefer we
> use putchar, puts, and printf, instead of adding stdout to all these
> calls.


I don't really have a preference, you could even throw out this last patch.
I just thought it would be nice to remove use of one of the functions
from the whole project to reduce needless variations and save the need to link
more functions that do the exact same thing and waste binary space.

Also, that searching "puts" results in "outputs" and "inputs"...

It would be nice to have patch 5 before release,
for patch 6 doesn't matter if or when to me.

Let me know if you want a v2 of either patch 5 or 6.

--
MCP

Re: [PATCH 8/9 v2] doc: Add elf{32, 64}_xlatetof.3 and elf{32, 64}_xlatetom.3

[Mark Wielaard]

Hi Aaron,

On Wed, Oct 02, 2024 at 10:26:09PM -0400, Aaron Merey wrote:
> v2 changes:
> Merge xlatetof and xlatetom man pages.
> 
> Added additional details suggested in Mark's review
> https://sourceware.org/pipermail/elfutils-devel/2024q3/007377.html

I like this version. One small wording question below.

> diff --git a/doc/elf32_xlatetof.3 b/doc/elf32_xlatetof.3
> new file mode 100644
> index ..47ecda27
> --- /dev/null
> +++ b/doc/elf32_xlatetof.3
> @@ -0,0 +1 @@
> +.so man3/elf32_xlatetom.3
> diff --git a/doc/elf32_xlatetom.3 b/doc/elf32_xlatetom.3
> new file mode 100644
> index ..ec2024fd
> --- /dev/null
> +++ b/doc/elf32_xlatetom.3
> @@ -0,0 +1,130 @@
> +.TH ELF32_XLATETOM 3 2024-08-14 "Libelf" "Libelf Programmer's Manual"
> +
> +.SH NAME
> +.nf
> +elf32_xlatetom, elf64_xlatetom \- translate 32-bit or 64-bit ELF data from 
> file
> +representation to memory representation
> +
> +elf32_xlatetof, elf64_xlatetof \- translate 32-bit or 64-bit ELF data from 
> memory
> +representation to file representation
> +
> +.SH SYNOPSIS
> +.nf
> +.B #include 
> +
> +.BI "int elf32_xlatetom(Elf_Data *" dst ", const Elf_Data *" src ", unsigned 
> int " encoding ");"
> +.BI "int elf64_xlatetom(Elf_Data *" dst ", const Elf_Data *" src ", unsigned 
> int " encoding ");"
> +
> +.BI "int elf32_xlatetof(Elf_Data *" dst ", const Elf_Data *" src ", unsigned 
> int " encoding ");"
> +.BI "int elf64_xlatetof(Elf_Data *" dst ", const Elf_Data *" src ", unsigned 
> int " encoding ");"
> +
> +.SH DESCRIPTION
> +Translate ELF data from file representation to memory representation or
> +vice versa.  File and memory representations of ELF data can differ in
> +terms of endianness.  Data in file representation normally comes from
> +.B elf_rawdata
> +while data in memory representation normally comes from
> +.BR elf_getdata .
> +When there is no difference between file and memory representations,
> +these functions simply copy the ELF data from
> +.I src
> +to
> +.IR dst .
> +Otherwise the encoding with swap between
> +.B ELFDATA2LSB
> +(two's complement little-endian) and
> +.B ELFDATA2MSB
> +(two's complement big-endian).

Should that be "will swap" ?

> The encoding of an ELF file is specified
> +in the
> +.B Elf32_Ehdr
> +or
> +.B Elf64_Ehdr e_ident[EI_DATA]
> +member.  To know the memory encoding for a program you can
> +.B #include 
> +and check BYTE_ORDER == LITTLE_ENDIAN (corresponding to
> +.BR ELFDATA2LSB )
> +or BYTE_ORDER == BIG_ENDIAN (corresponding to
> +.BR ELFDATA2MSB ).
> +
> +.SH PARAMETERS
> +.TP
> +.I dst
> +Destination where the translated data will be stored.
> +The
> +.B d_size
> +of
> +.I dst
> +should be at least as big as the
> +.B d_size
> +of
> +.IR src .
> +
> +.TP
> +.I src
> +Source data. For the
> +.B xlatetom
> +functions, the source data should be in file representation.
> +For the
> +.B xlatetof
> +functions, the source data should be in memory representation.
> +
> +.TP
> +.I encoding
> +Specifies an encoding.  Can be either
> +.B ELFDATA2LSB
> +(two's complement little-endian) or
> +.B ELFDATA2MSB
> +(two's complement big-endian).  For the
> +.B xlatetom
> +functions, this specifies the encoding of
> +.IR src .
> +For the
> +.B xlatetof
> +functions, this specifies the encoding of
> +.IR dst .
> +
> +.SH RETURN VALUE
> +On success, return
> +.IR dst ,
> +which will contain the translated data.  If there is no difference
> +between the file and memory representations,
> +.I dst
> +will contain a copy of the source data.  The
> +.B d_type
> +and
> +.B d_size
> +of
> +.I dst
> +will be set to those of
> +.IR src .
> +
> + If an error occurs, return
> +NULL and set a libelf error code.
> +
> +.SH SEE ALSO
> +.BR elf_errno (3),
> +.BR elf_getdata (3),
> +.BR elf_rawdata (3),
> +.BR libelf (3),
> +.BR elf (5)
> +
> +.SH ATTRIBUTES
> +For an explanation of the terms used in this section, see
> +.BR attributes (7).
> +.TS
> +allbox;
> +lbx lb lb
> +l l l.
> +InterfaceAttribute   Value
> +T{
> +.na
> +.nh
> +.BR elf32_xlatetom (),
> +.BR elf64_xlatetom (),
> +.BR elf32_xlatetof (),
> +.BR elf64_xlatetof ()
> +T}   Thread safety   MT-Safe
> +.TE
> +
> +.SH REPORTING BUGS
> +Report bugs to  or 
> https://sourceware.org/bugzilla/.
> diff --git a/doc/elf64_xlatetof.3 b/doc/elf64_xlatetof.3
> new file mode 100644
> index ..47ecda27
> --- /dev/null
> +++ b/doc/elf64_xlatetof.3
> @@ -0,0 +1 @@
> +.so man3/elf32_xlatetom.3
> diff --git a/doc/elf64_xlatetom.3 b/doc/elf64_xlatetom.3
> new file mode 100644
> index ..47ecda27
> --- /dev/null
> +++ b/doc/elf64_xlatetom.3
> @@ -0,0 +1 @@
> +.so man3/elf32_xlatetom.3
> -- 
> 2.46.2
> 

Re: Preparing for the 0.192 release on Friday October 18

[Serhei Makarov]

On Thu, Oct 3, 2024, at 12:22 PM, Aaron Merey wrote:
> If your patch is still under review [1] or you have other patches you'd
> like merged before the release, please let us know.
I posted a cleaned-up patch series for eu-stacktrace this morning [2].
Hoping that can make it in.

[2]: https://patchwork.sourceware.org/project/elfutils/list/?series=39563

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

Re: [PATCH 9/9] doc/Makefile.am: Add man pages

[Mark Wielaard]

Hi Aaron,

On Wed, Oct 02, 2024 at 10:26:10PM -0400, Aaron Merey wrote:
> Add the following man pages to notrans_dist_man3_MANS:
> 
> elf32_xlatetom.3 elf64_xlatetom.3 elf32_xlatetof.3 elf64_xlatetof.3
> elf32_newphdr.3 elf64_newphdr.3 elf32_newehdr.3 elf64_newehdr.3
> elf32_getshdr.3 elf64_getshdr.3 elf32_getphdr.3 elf64_getphdr.3
> elf32_getchdr.3 elf64_getchdr.3 elf32_fsize.3 elf64_fsize.3
> elf32_checksum.3 elf64_checksum.3

Looks good.
Maybe could have been added when the individual files were added.
But it should be fine to add them all at the end.

Cheers,

Mark

Re: [PATCH] PR32218: debuginfod-client: support very long source file

[Frank Ch. Eigler]

Hi -

> Hi Frank,

Thanks for the review.

> Maybe add the bugzilla URL as an example of this.
> I see it is in the subject, but it is nice to just click on something.

Done.

> [...]
> Is this 4-byte hash enough to prevent accidental collisions?

Yeah, it only needs to be uniqueish among source files of the same
binary, and with the same #-escaped tail string.

> > This is a transparent change to clients, who are not to make any
> > assumptions about cache file naming structure.
> 
> How does it interact with existing source file cache files?

Existing files will be ignored.  All files will be periodically
garbage collected.

> [...]
> Are you sure you want to use elf_hash? It returns an (arch dependent)
> unsigned long int but the actual implementation only produces an
> unsigned int. To simplify things and not need extra build dependencies
> you might want to just include a hand-coded string hash function like
> djb2? Which is really just 4 lines of code and probably a better hash
> for strings than elf_hash.

Good idea, done.


> Move libelf after libdebuginfod. OK (but see above, isn't it simpler to
> just include a small string hash function right in the code itself?)

Yeah, nuked these.

> [...]
> What do these "Reduce" comments mean?
> So we hope to get at least the original input path length plus 10 (8
> char hash plus dash plus zero), but we are fine with less.

New comments elaborate on this.

> [...]
> Yeah, this is 4bytes everywhere, still would love to see it being an
> int32_t if that is what you want.

Well, it's only in the sprintf as a type, and only in one place; it's
dresses in disguise as a "10" elsewhere.

> [...]
> OK, so this always adds (or overwrites) a 8 char hash plus dash '-' in
> front. Have you considered to only add it if the src doesn't fit the
> dest_len?

Yes, new comments explain why.

> [...]
> sizeof(suffix) == PATH_MAX
> What is the relation to NAME_MAX?

Nothing, just PATH_MAX >> NAME_MAX.

> [...]
> The comment confuses me a little. is .foo and/or .bar the section name?

Yes, changed to a more obvious example.

> > -  if (section != NULL)
> > +  if (suffix[0] != '\0') /* section, source queries */
> >  xalloc_str (target_cache_path, "%s/%s-%s", target_cache_dir, type, 
> > suffix);
> >else
> > -xalloc_str (target_cache_path, "%s/%s%s", target_cache_dir, type, 
> > suffix);
> > +xalloc_str (target_cache_path, "%s/%s", target_cache_dir, type);
> >xalloc_str (target_cache_tmppath, "%s.XX", target_cache_path);
> 
> Maybe related to my confusion about the comment above. Why this change?

Simplifies code.  Suffix from section as well as source queries is
handled along the same path.

> [...]
> > +# for test case debugging, uncomment:
> > +set -x
> 
> Missing # ? Or use it just unconditionally?

I *love* set -x unconditionally, so tweaked the comment.
(IMO test-subr.sh should set this for all our tests.)


patch v2:

>From 4023ad1a81f31ae404c2959bad752d05ad2bb3b9 Mon Sep 17 00:00:00 2001
From: "Frank Ch. Eigler" 
Date: Thu, 10 Oct 2024 16:30:19 -0400
Subject: [PATCH] PR32218: debuginfod-client: support very long source file
 names

debuginfod clients & servers may sometimes encounter very long source
file names.  Previously, the client would synthesize a path name like
   $CACHEDIR/$BUILDID/source-$PATHNAME
where $PATHNAME was a funky ##-encoded version of the entire source
path name.  See https://sourceware.org/PR32218 for a horror case.
This can get too long to store as a single component of a file system
pathname (e.g. linux/limits.h NAME_MAX), resulting on client-side
errors even after a successful download.

New code switches encoding of the $PATHNAME part to use less escaping,
and a merciless truncation to the tail part of the filename.  (We keep
the tail rather than the head, so that the extension is preserved,
which makes some consumers happier.)  To limit collision damage from
truncation, we add also insert a goofy hash (4-byte DJBX33A) of the
name into the path name.  The result is a relatively short name:

   $CACHEDIR/$BUILDID/source-$HASH-$NAMETAIL

This is a transparent change to clients, who are not to make any
assumptions about cache file naming structure.  However, one existing
test did make such assumptions, so is fixed with some globness.  A new
test is also added, using a pre-baked tarball with a very long srcfile
name.

Signed-off-by: Frank Ch. Eigler 
---
 debuginfod/debuginfod-client.c| 106 --
 tests/Makefile.am |   7 +-
 .../bighello-sources/bighello.c   |   7 ++
 .../bighello-sources/bighello.h   |   1 +
 .../moremoremoremoremoremoremoremore  |   1 +
 tests/debuginfod-tars/bighello.tar| Bin 0 -> 51200 bytes
 tests/run-debuginfod-longsource.sh|  69 
 tests/run-debuginfod-section.sh   |  16 +--
 8 files changed, 161 insertions(+), 46 deletions(-)
 create mode 100644 tests/debuginfod-tars/b

Re: [PATCH 6/6] Use file stream or format variants of stdio print functions

[Mark Wielaard]

Hi Michael,

On Tue, Oct 15, 2024 at 03:10:16PM +, Michael Pratt wrote:
> > If we are trying to be more consistent then I think I would prefer we
> > use putchar, puts, and printf, instead of adding stdout to all these
> > calls.
> 
> I don't really have a preference, you could even throw out this last patch.
> I just thought it would be nice to remove use of one of the functions
> from the whole project to reduce needless variations and save the need to link
> more functions that do the exact same thing and waste binary space.
> 
> Also, that searching "puts" results in "outputs" and "inputs"...
> 
> It would be nice to have patch 5 before release,
> for patch 6 doesn't matter if or when to me.
> 
> Let me know if you want a v2 of either patch 5 or 6.

Lets postpone the consistency/renaming patch to after the release. If
you could simplify patch 5 by just changing the putprint_unlocked
functions to their putprint variant that would be appreciated.

Thanks,

Mark