rfc patch PR32760: find-debuginfo for static libraries
Mark Wielaard
mark@klomp.org
Thu May 15 15:27:57 GMT 2025
Hi Frank,
Sorry for the late reply. Please feel free to ping aggressively. I
admit to often work from "top of INBOX".
On Sat, 2025-03-22 at 05:29 -0400, Frank Ch. Eigler via Debugedit
wrote:
> Patch v2 adds handling of duplicate member file names, with cunning
> use of dd (!) to extract files one by one, error handling, $AR
> parametrization, and a test case.
It is almost as if we need a better ar processing tool...
> commit 412455a95e2f2dd673bd0eddc1990555de310519 (HEAD -> main)
> 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.
Is the thing between brackets (and debuginfod-resolvable) really true?
It is certainly a good improvement to have the source path "correct" in
the .a archive. The debuginfo and paths should then follow into the
(static) binaries that are created by linking the object in the ar
file. So I am all for this improvement.
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?
> 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.
>
> New autotest case covers duplicate-member-name functionality,
> including with cruelly whitespace named members.
Thanks for including such a testcase.
>
> 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')
>
> 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)
>
> # Whether dwz support -j.
> diff --git a/scripts/find-debuginfo.in b/scripts/find-debuginfo.in
> index 4cc49f2f8cd0..edcad7d2c596 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] [-a]
> [-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 -a 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.
Can I convince you to use a long option like --no-ar-files ?
Having -a meaning no ar file processing seems a little cryptic.
We already have too many slightly cryptic since char flags.
> 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@}
Great.
> # 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
> ;;
> + -a)
> + process_ar=false
> + ;;
> -q|--quiet)
> quiet=true
> verbose=false
> @@ -443,10 +456,14 @@ 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' |
> +(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'
> +) | 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
Still slightly worried about the parsing of "file" here, but it is what
it is for now.
> @@ -461,11 +478,115 @@ 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/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.
> + # 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
OK.
> + # 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
> + echo "$line"
Probably want to hide this behind $verbose && ...
> + 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}"
Wow, I learn something new about bash scripting every time :)
> + local tmpdir="$temp/archive-member" # super short lived
Same question for tmpdir as tmpa above, can this clash on concurrent
runs?
> + 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"
> +
> + # 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.
> + # preserve timestamp from original file, though debugedit may lose it
> + touch -d "$date" "$tmpdir/$member_dn$member_bn"
> +
> + 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
This does seem to match the way file is used earlier in the script.
debugedit is called without any build-id processing because these
object files don't have any. OK.
Use of $SOURCEFILE here seems correct since it run_job defines a local
version which I assume is what is used here instead of the global one.
> +
> + # 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
OK.
> + if [ $res -eq 0 ]; then
> + # replace original archive with new version
> + ${AR} sP "$tmpa" # ranlib, preserve paths
> + mv "$tmpa" "$f"
> + fi
> + rm -f "$tmpa"
OK.
> + $verbose && echo found $(tr -dc '\0' < "$SOURCEFILE" | wc -c) source files
Nice, but shouldn't that be prefixed with the ar file name?
Should the same be done for none-ar-file processing?
> + # 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
> +}
OK.
> # Strip ELF binaries
> do_file()
> {
> local nlinks=$1 inum=$2 f=$3 id link linked
>
> + local ar_re="^.*\.a$"
> + if [[ $f =~ $ar_re ]]; then # treat as static archive
> + do_ar_file "$1" "$2" "$3"
> + return
> + fi
> +
> get_debugfn "$f"
> [ -f "${debugfn}" ] && return 0
>
I think the check for ar files should go after this check whether the
file is (already) under /usr/lib/debug. The contract seems to be that
we don't process files already there.
>
> @@ -835,3 +956,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
OK.
> 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@"
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;
> +}
SHOUTING foo now? :)
> diff --git a/tests/find-debuginfo.at b/tests/find-debuginfo.at
> new file mode 100644
> index 000000000000..e1f9e2cf7748
> --- /dev/null
> +++ b/tests/find-debuginfo.at
> @@ -0,0 +1,77 @@
> +# 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/>.
> +
> +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"
> +]])
> +
> +
> +AT_SETUP([find-debuginfo help])
> +AT_KEYWORDS([find-debuginfo] [find-debuginfo])
> +AT_CHECK([find-debuginfo --help],[0],[ignore],[ignore])
> +AT_CLEANUP
> +
> +
> +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
BTW. This is now 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])
> +
> +AT_CHECK([which find-debuginfo], [0], [stdout], [ignore])
What is this checking?
> +# 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 \
> + sh -x `which find-debuginfo` -S sourcefiles.list -v ${PWD}/subdir_build]], [0], [stdout], [ignore])
> +
> +# 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])
> +
> +# 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
Very nice.
> 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.
Thanks,
Mark
More information about the Debugedit
mailing list