rfc patch PR32760: find-debuginfo for static libraries
Mark Wielaard
mark@klomp.org
Wed Jun 18 12:41:23 GMT 2025
Hi Frank,
On Fri, 2025-06-13 at 16:35 -0400, Frank Ch. Eigler via Debugedit
wrote:
> > > [...]
> > > find-debuginfo looks for archive ".a" files in the build tree, and
> > > runs debugedit only to enumerate and rewrite source paths. This
> > > allows ultimate users of the .a files to receive debuginfo, with
> > > usable (and debuginfod-resolvable) source file paths.
> >
> > Is the thing between brackets (and debuginfod-resolvable) really true?
>
> Absolutely!
>
> > But how does the debuginfod-resolvable part then work? There is no
> > reference to where these objects and debuginfo came from in the
> > binary/packages that incorporate them is there?
>
> Except the source file full path (canonicalized during rewriting).
> That normally includes substrings like the rpm n-v-r, which makes them
> unique enough to locate in the n-static-debugsource-v-r.rpm .
O, wow. Cute! So that makes the sources debuginfod-resolvable since it
unpacks and indexes them. How does it get the .debug file reference? Or
don't you need that because the .a keeps the debuginfo and links it
into the app (so it gets placed in the app.debug)? What about the dwz
multi file or .debug_index?
... time passes /me actually reads the patch again ...
+ # NB: no need to strip or dwz-compress or gdbindex or
+ # ELFBINSFILE-collect objects / archives. These operations only
+ # make sense on the final binaries that the static archives are
+ # linked into.
Right. OK.
> > > +# Handle ELF archives
> > > +do_ar_file()
> > > +{
> > > + local nlinks="$1" inum="$2" f="$3" id link linked
> > > + local tmpa="$temp/output.a"
> > > + local res=0
> >
> > Question about the use of $temp here with a fixed file name. Isn't
> > $temp also a "fixed" output dir script wide? If so and if do_ar_file
> > can be called in parallel, it seems the tmpa file can clash with other
> > do_ar_file runs.
>
> Sorry, I updated the patch on the try-branch after sending it here,
> adding the "$inum" into the string.
>
> + local tmpa="$temp/$inum-output.a"
Yes, that should work.
> > [...]
> > > + local tmpdir="$temp/archive-member" # super short lived
> >
> > Same question for tmpdir as tmpa above, can this clash on concurrent
> > runs?
>
> Yeah, previously fixed the same way ($inum).
Ack, thanks.
> > > + # extract the file by offset
> > > + (cd "$tmpdir"; dd status=noxfer if="$f" of="$member_dn$member_bn" bs=1 skip="$offset" count="$size")
> > > + if [ $? -ne 0 ]; then
> > > + res=1
> > > + fi
> >
> > Maybe add a comment here why you cannot just use ar to extract it by
> > name.
>
> Sure.
Thanks, so ar could in theory but the bookkeeping to get the numbers
right for each name is just way too much work. Odd that ar doesn't have
an extract by offset.
> > > +# expect original source tree refs
> > > +AT_CHECK([$READELF --debug-dump=line subdir_build/dupes.a | grep `pwd`], [0], [stdout], [ignore])
> > > +
> > > +AT_CHECK([which find-debuginfo], [0], [stdout], [ignore])
> >
> > What is this checking?
>
> That the script is properly installed. Kinda trivial, maybe
> pointless, also harmless. :)
Not completely harmless, we just removed depending on which.
(Which [pun intended] in itself might be pointless...)
But I think that is precisely what AT_TESTED([find-debuginfo]) already
does (and more):
— Macro: AT_TESTED (executables)
Log the file name and answer to --version of each program in space-
separated list executables. Several invocations register new
executables, in other words, don't fear registering one program several
times.
Autotest test suites rely on PATH to find the tested program. This
avoids the need to generate absolute names of the various tools, and
makes it possible to test installed programs. Therefore, knowing which
programs are being exercised is crucial to understanding problems in
the test suite itself, or its occasional misuses. It is a good idea to
also subscribe foreign programs you depend upon, to avoid incompatible
diagnostics.
> Patch follows (also in git at users/fche/try-pr32760v10)
> ------------------------------------------------------------------------
>
> commit 5b58b4e62613d7c448e8f4fe1fc3e327aebf72fb (HEAD -> main, origin/remotes/origin/users/fche/try-pr32760v10)
> Author: Frank Ch. Eigler <fche@redhat.com>
> Date: Thu Mar 6 14:45:36 2025 -0500
>
> PR32760: find-debuginfo: handle static libraries
>
> Formerly, static .a libraries with debuginfo were ignored by
> find-debuginfo. If unstripped, then the raw build-directory paths in
> them would leak into the downstream package. New code in
> find-debuginfo looks for archive ".a" files in the build tree, and
> runs debugedit only to enumerate and rewrite source paths. This
> allows ultimate users of the .a files to receive debuginfo, with
> usable (and debuginfod-resolvable) source file paths.
>
> This works by having find-debuginfo find .a files, extracting all
> the .o files one at a time, running debugedit, then repacking the
> files back into the .a. do_file() delegates to do_ar_file(),
> doing rather less work than for .so/exec files.
Maybe add the explanation that the archive (members) themselves aren't
stripped because we want the debuginfo itself to be linked into the
final executable? (For people like me who are dense and have to get
explained how this really works :)
> New autotest case covers duplicate-member-name functionality,
> including with cruelly whitespace named members.
>
> Signed-off-by: Frank Ch. Eigler <fche@redhat.com>
>
> diff --git a/Makefile.am b/Makefile.am
> index 562ffa46fa30..99555bdad153 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -36,6 +36,7 @@ do_subst = ($(SED) -e 's,[@]PACKAGE[@],$(PACKAGE),g' \
> -e 's,[@]READELF[@],$(READELF),g' \
> -e 's,[@]OBJCOPY[@],$(OBJCOPY),g' \
> -e 's,[@]NM[@],$(NM),g' \
> + -e 's,[@]AR[@],$(AR),g' \
> -e 's,[@]DWZ_J[@],$(DWZ_J),g')
Nice.
> find-debuginfo: $(top_srcdir)/scripts/find-debuginfo.in Makefile
> diff --git a/configure.ac b/configure.ac
> index ad4d70c63995..47771d4ca807 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -47,6 +47,7 @@ AC_CHECK_TOOL([LD], [ld])
> AC_CHECK_TOOL([READELF], [readelf])
> AC_CHECK_TOOL([OBJCOPY], [objcopy])
> AC_CHECK_TOOL([NM], [nm])
> +AC_CHECK_TOOL([AR], [ar])
> AM_MISSING_PROG(HELP2MAN, help2man)
Ack. For the above to work.
> # Whether dwz support -j.
> diff --git a/scripts/find-debuginfo.in b/scripts/find-debuginfo.in
> index 4cc49f2f8cd0..38d460ccabb6 100755
> --- a/scripts/find-debuginfo.in
> +++ b/scripts/find-debuginfo.in
> @@ -5,6 +5,7 @@
> # for inclusion in package file lists.
>
> # Copyright (C) 2002-2021 rpm and debugedit contributors
> +# Copyright (C) 2025 Red Hat Inc.
> #
> # This program is free software; you can redistribute it and/or modify
> # it under the terms of the GNU General Public License as published by
> @@ -28,7 +29,7 @@ automagically generates debug info and file lists
> Options:
> [--strict-build-id] [-g] [-r] [-m] [-i] [-n] [-q] [-v]
> [--keep-section SECTION] [--remove-section SECTION]
> -[--g-libs]
> +[--g-libs] [--no-ar-files]
> [-j N] [--jobs N]
> [-o debugfiles.list]
> [-S debugsourcefiles.list]
> @@ -95,6 +96,11 @@ will be called /usr/debug/src/<BASE>. This makes sure the debug source
> dirs are unique between package version, release and achitecture (Use
> --unique-debug-src-base "%{name}-%{VERSION}-%{RELEASE}.%{_arch}")
>
> +If --no-ar-files is given, then static libraries will be ignored. Otherwise,
> +they receive only with source-path rewriting and collection. They are
> +not stripped, since they have no persistent build-ids to accommodate
> +eventual reunification.
Right! There it is documented. Thanks.
> The -q or --quiet flag silences all non-error output from the script.
> The -v or --verbose flag add more output for all files processed.
> When neither -q or -v is given then only output for each pass is given.
> @@ -115,6 +121,7 @@ PATH=${install_dir}:$PATH
> READELF=${READELF:=@READELF@}
> OBJCOPY=${OBJCOPY:=@OBJCOPY@}
> NM=${NM:=@NM@}
> +AR=${AR:=@AR@}
>
> # With -g arg, pass it to strip on libraries or executables.
> strip_g=false
> @@ -167,6 +174,9 @@ quiet=false
> # add more non-error output
> verbose=false
>
> +# process static libraries
> +process_ar=true
> +
> BUILDDIR=.
> out=debugfiles.list
> srcout=
> @@ -260,6 +270,9 @@ while [ $# -gt 0 ]; do
> srcout=$2
> shift
> ;;
> + --no-ar-files)
> + process_ar=false
> + ;;
> -q|--quiet)
> quiet=true
> verbose=false
Ack. process_ar handling. Defaulting to true.
> @@ -443,11 +456,19 @@ trap 'rm -rf "$temp"' EXIT
> # Build a list of unstripped ELF files and their hardlinks
> touch "$temp/primary"
> touch "$temp/linked"
> -find "$RPM_BUILD_ROOT" ! -path "${debugdir}/*.debug" -type f \
> - \( -perm -0100 -or -perm -0010 -or -perm -0001 \) \
> - -print | LC_ALL=C sort |
> -file -N -f - | sed -n -e 's/^\(.*\):[ ]*.*ELF.*, not stripped.*/\1/p' |
> -xargs --no-run-if-empty stat -c '%h %D_%i %n' |
> +(
> + find "$RPM_BUILD_ROOT" ! -path "${debugdir}/*.debug" -type f \( -perm -0100 -or -perm -0010 -or -perm -0001 \) -print |
> + file -N -f - |
> + sed -n -e 's/^\(.*\):[ ]*.*ELF.*, not stripped.*/\1/p';
> +
> + # plus static libraries
> + $process_ar && find "$RPM_BUILD_ROOT" -type f -name '*.a' -print |
> + file -N -f - |
> + sed -n -e 's/^\(.*\):[ ]*current ar archive.*/\1/p';
> + true
> +) |
> + env LC_ALL=C sort |
> + xargs --no-run-if-empty stat -c '%h %D_%i %n' |
> while read nlinks inum f; do
> if [ $nlinks -gt 1 ]; then
> var=seen_$inum
Getting a little dizzy from the file output processing, but yeah, this
should work...
> @@ -461,14 +482,123 @@ while read nlinks inum f; do
> echo "$nlinks $inum $f" >>"$temp/primary"
> done
>
> +
> +# Handle ELF archives
> +do_ar_file()
> +{
> + local nlinks="$1" inum="$2" f="$3" id link linked
> + local tmpa="$temp/$inum-output.a"
> + local res=0
> +
> + # See also cpio SOURCEFILE copy. Directories must match up.
> + debug_base_name="$RPM_BUILD_DIR"
> + debug_dest_name="/usr/src/debug"
> + if [ ! -z "$unique_debug_src_base" ]; then
> + debug_base_name="$BUILDDIR"
> + debug_dest_name="/usr/src/debug/${unique_debug_src_base}"
> + fi
> +
> + $verbose && echo "extracting debug info from $f"
It isn't "extracting debug info", maybe "processing ar archive $f"?
> + # Extract members from archive, one at a time. There may be
> + # duplicate names, so we can't just extract the entire archive in
> + # one go (overwriting each other). Instead, we pick off member
> + # files one-by-one, into synthetic subdirectories, adding processed
> + # versions to a new archive, in order.
> +
> + # Create empty output .a; mktemp would create a 0-byte file, which ar rv doesn't like
> + ${AR} r "$tmpa" # no members
> +
> + ${AR} tvO "$f" | while read line; do
> + local pattern='^[rwx-]+ [0-9]+/[0-9]+ +([0-9]+) (.................) (.*) (0x[0-9a-f]+)$'
> + if [[ $line =~ $pattern ]]; then
> + local size=${BASH_REMATCH[1]}
> + local date=${BASH_REMATCH[2]}
> + local member=${BASH_REMATCH[3]}
> + local offset
> + (( offset=${BASH_REMATCH[4]} )) # convert from hexadecimal
> + $verbose && echo "considering ${f#$RPM_BUILD_DIR/} ${member} size ${size} at ${offset}"
> + local tmpdir="$temp/$inum-archive-member" # super short lived
> + local member_dn=$(dirname "$member")
> + if [ "$member_dn" = "." ]; then
> + member_dn="" # empty
> + else
> + member_dn="${member_dn}/" # or suffixed with /
> + fi
> + local member_bn=$(basename "$member")
> +
> + # (re)create a directory to hold the (pathname-inclusive) member
> + mkdir -p "$tmpdir/$member_dn"
Right, all this is now based on $inum in paths, so no worry about
conflicting files in parallel mode.
>
> + # extract the file by offset, because extracting it by name
> + # is hard, in case the same name exists multiple times. A
> + # distinct instance-number would have to be given to ar ("N ###"),
> + # kept on a per-name basis.
> + (cd "$tmpdir"; dd status=none if="$f" of="$member_dn$member_bn" bs=1 skip="$offset" count="$size")
> + if [ $? -ne 0 ]; then
> + res=1
> + fi
> + # preserve timestamp from original file, though debugedit may lose it
> + touch -d "$date" "$tmpdir/$member_dn$member_bn"
Is this a bug in debugedit? Could you file a bug for it then?
debugedit probably should preserve the st_atim/mtim and futimens them
back if it doesn't do so already.
> + if file "$tmpdir/$member_dn$member_bn" |
> + grep -qE 'ELF.*, not stripped'; then
> + debugedit -b "$debug_base_name" -d "$debug_dest_name" \
> + -l "$SOURCEFILE" "$tmpdir/$member_dn$member_bn"
> + if [ $? -ne 0 ]; then
> + res=1
> + $verbose && echo "failed processing ELF object ${member}"
> + else
> + $verbose && echo "processed ELF object ${member}"
> + fi
> + else
> + $verbose && echo "skipped ${member}"
> + fi
Maybe add "skipped ${member} (no debuginfo)" so people know why?
> +
> + # add the file; qP mode, so strict append, no dupe elimination, path preserved
> + (cd "$tmpdir"; ${AR} qP "$tmpa" "$member_dn$member_bn")
> +
> + # remove the entire temporary directory, in case another
> + # member object comes later with a conflicting name
> + rm -rf "$tmpdir"
> + else
> + $verbose && echo "skipping archive $f with unparseable contents"
> + res=1
> + fi
> + done
> + if [ $res -eq 0 ]; then
> + # replace original archive with new version
> + ${AR} sP "$tmpa" # ranlib, preserve paths
> + mv "$tmpa" "$f"
> + fi
> + rm -f "$tmpa"
> +
> + $verbose && echo found $(tr -dc '\0' < "$SOURCEFILE" | wc -c) source files
> +
> + # NB: no need to strip or dwz-compress or gdbindex or
> + # ELFBINSFILE-collect objects / archives. These operations only
> + # make sense on the final binaries that the static archives are
> + # linked into.
> +
> + return $res
> +}
Nice.
> # Strip ELF binaries
> do_file()
> {
> local nlinks=$1 inum=$2 f=$3 id link linked
>
> + # reject files already located under /usr/lib/debug, presumably processed
> get_debugfn "$f"
> [ -f "${debugfn}" ] && return 0
>
> + local ar_re="^.*\.a$"
> + if [[ $f =~ $ar_re ]]; then # treat as static archive
> + do_ar_file "$1" "$2" "$3"
> + return
> + fi
Do you also need the "file" /current ar archive/ trick here?
Or is it totally unlikely that an .a file is an ELF (not stripped)
file?
> $verbose && echo "extracting debug info from $f"
> # See also cpio SOURCEFILE copy. Directories must match up.
> debug_base_name="$RPM_BUILD_DIR"
> @@ -490,6 +620,8 @@ do_file()
> $strict && return 2
> fi
>
> + $verbose && echo found $(tr -dc '\0' < "$SOURCEFILE" | wc -c) source files
> +
> # debugedit makes sure to to get write permission to the file and
> # restores original state after modifications. Other utilities
> # might not.
Thanks, this will be helpful because later on rpm might error out when
SOURCEFILE turns out to be empty.
> @@ -835,3 +967,4 @@ if ((nout > 0)); then
> fi
>
> $quiet || echo "find-debuginfo: done" 2>&1
> +exit 0
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index b1d39f4ac1fc..b6c39a9b1b93 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -32,12 +32,14 @@ $(srcdir)/package.m4: $(top_srcdir)/configure.ac
> # The tests
> TESTSUITE_AT = \
> testsuite.at \
> - debugedit.at
> + debugedit.at \
> + find-debuginfo.at
>
> # Some source files that are needed by the tests
> EXTRA_DIST += data/SOURCES/foo.c \
> data/SOURCES/bar.c \
> data/SOURCES/baz.c \
> + data/SOURCES/dupe.c \
> data/SOURCES/foobar.h
>
> TESTSUITE = $(srcdir)/testsuite
Ack.
> diff --git a/tests/atlocal.in b/tests/atlocal.in
> index 9da28786ba26..2f2ff4a80646 100644
> --- a/tests/atlocal.in
> +++ b/tests/atlocal.in
> @@ -8,6 +8,7 @@ PATH=@abs_builddir@:@abs_top_builddir@:$top_srcdir:$srcdir:$PATH
> CC="@CC@"
> CFLAGS=""
> LD="@LD@"
> +AR="@AR@"
> LDFLAGS=""
> READELF="@READELF@"
> READELF_VERSION_OK="@READELF_VERSION_OK@"
> diff --git a/tests/data/SOURCES/dupe.c b/tests/data/SOURCES/dupe.c
> new file mode 100644
> index 000000000000..8491786395fb
> --- /dev/null
> +++ b/tests/data/SOURCES/dupe.c
> @@ -0,0 +1,4 @@
> +int FOO (void);
> +int FOO (void) {
> + return 0;
> +}
OK.
You
> diff --git a/tests/find-debuginfo.at b/tests/find-debuginfo.at
> new file mode 100644
> index 000000000000..cd41c3c378c5
> --- /dev/null
> +++ b/tests/find-debuginfo.at
> @@ -0,0 +1,81 @@
> +# debugedit.at: Tests for the find-debuginfo tool
> +#
> +# Copyright (C) 2025 Red Hat Inc.
> +#
> +# This program 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 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program 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 see <http://www.gnu.org/licenses/>.
> +
> +# Recommended usage for verbose logging to the console:
> +# make check TESTSUITEFLAGS=-v
> +
> +AT_BANNER([find-debuginfo])
> +AT_TESTED([find-debuginfo])
> +
> +m4_define([BUILDO],[[
> +mkdir -p "$1"
> +cp "${abs_srcdir}"/data/SOURCES/dupe.c "$1"
> +$CC $CFLAGS -g3 -c "`pwd`/$1/dupe.c" -DFOO=$2 -o "$1/dupe.o"
> +echo built "$1/dupe.o"
> +]])
Nice helper
> +AT_SETUP([find-debuginfo help])
> +AT_KEYWORDS([find-debuginfo] [find-debuginfo])
> +AT_CHECK([find-debuginfo --help],[0],[ignore],[ignore])
> +AT_CLEANUP
Making sure --help works. OK.
> +AT_SETUP([find-debuginfo on .a])
> +# build a little .a
> +export HOME=${PWD}
> +BUILDO([subdir_foob],[foo])
> +touch -t 197001010101 subdir_foob/dupe.o # set distinct timestamps
> +BUILDO([subdir_barb],[bar])
> +touch -t 197001020101 subdir_barb/dupe.o
> +BUILDO([subdir_zo b],[jam])
> +touch -t 197001030101 subdir_zo?b/dupe.o
> +BUILDO([subdir_club],[pix])
> +touch -t 197001040101 subdir_club/dupe.o
> +mkdir -p subdir_build
> +$AR rv subdir_build/dupes.a subdir_foob/dupe.o subdir_barb/dupe.o # without P, these duplicate members "/dupe.o"
> +$AR qP subdir_build/dupes.a subdir_zo?b/dupe.o subdir_club/dupe.o # with P, these are subdirectoried members
> +# NB: find-debugedit can't handle file names with embedded whitespace; find | xargs used here and there
find-debug$(sed s/edit/info/)
https://sourceware.org/bugzilla/show_bug.cgi?id=32862
> +
> +# list old .a contents
> +AT_CHECK([$AR tPv subdir_build/dupes.a],[0],[stdout],[ignore])
> +AT_CHECK([test `wc -l < stdout` -eq 4],[0],[ignore],[ignore])
> +
> +# expect original source tree refs
> +AT_CHECK([$READELF --debug-dump=line subdir_build/dupes.a | grep `pwd`], [0], [stdout], [ignore])
> +
> +# (which is not everpresent)
> +AT_CHECK([type -p find-debuginfo], [0], [stdout], [ignore])
I think this isn't needed, see AT_TESTED above.
Also type -p is bourne shell only (which this is, so fine)
command -v is POSIX and should work everywhere.
> +# run conversion
> +AT_CHECK([[env RPM_BUILD_DIR=${PWD} RPM_BUILD_ROOT=${PWD} RPM_PACKAGE_NAME=pkg RPM_PACKAGE_VERSION=ver RPM_PACKAGE_RELEASE=rel RPM_ARCH=arch \
> + bash -x `type -p find-debuginfo` -S sourcefiles.list -v ${PWD}/subdir_build]], [0], [stdout], [ignore])
Can't you just use find-debuginfo directly?
> +# list new .a contents
> +AT_CHECK([$AR tPv subdir_build/dupes.a],[0],[stdout],[ignore])
> +AT_CHECK([test `wc -l < stdout` -eq 4],[0],[ignore],[ignore])
This is probably enough. But you could check for exact output with
something like:
AT_DATA([expout],
[dupe.o
dupe.o
dupe.o
dupe.o
])
AT_CHECK([$AR tPv subdir_build/dupes.a],[0],[expout],[])
> +
> +# expect non-empty source file list
> +AT_CHECK([grep . ${PWD}/subdir_build/sourcefiles.list | tr '\0' '\n'], [0], [stdout], [ignore])
> +
> +# expect proper source tree
> +AT_CHECK([$READELF --debug-dump=line subdir_build/dupes.a | grep /usr/src/debug], [0], [stdout], [ignore])
> +
> +# no traces of old source tree
> +AT_CHECK([$READELF --debug-dump=line subdir_build/dupes.a | grep `pwd`], [1], [stdout], [stderr])
> +
> +AT_CLEANUP
Nice tests.
> diff --git a/tests/testsuite.at b/tests/testsuite.at
> index baf7ca2c81c6..f26cfbe1c6f1 100644
> --- a/tests/testsuite.at
> +++ b/tests/testsuite.at
> @@ -8,5 +8,5 @@ m4_define([AT_SKIP_TEST],[exit 77])
> AT_INIT
>
> AT_TESTED([debugedit])
> -
> m4_include([debugedit.at])
> +m4_include([find-debuginfo.at])
OK.
So the above are all just nitpicks, feel free to make changes or ignore
them and commit. I'll deal with the new tests/find-debuginfo.at file
merge conflicts with my patch.
Have you done some other testing? Could you find some larger
static/devel packages in fedora and rebuild them with this new ar
processing code?
Thanks,
Mark
More information about the Debugedit
mailing list