rfc patch PR32760: find-debuginfo for static libraries

[Mark Wielaard]

Hi Frank,

On Thu, 2025-03-06 at 14:53 -0500, Frank Ch. Eigler wrote:
> This patch appears to make unstripped static libraries work, in the
> sense of PR32760, as mentioned in RHBZ1395280.  I'm trying to figure
> out how to test it automatically; there are no current tests for the
> script in the debugedit testsuite.

Indeed :{ There are only minimal checks in the debugedit repo. help2man
is called to create a man page (which works, the find-debuginfo.1 man
page does contain a description of -a with this patch). make distcheck
will check that find-debuginfo supports --version and --help)

The main issue is that find-debuginfo still has some dependencies on
being called from rpm.
https://sourceware.org/bugzilla/show_bug.cgi?id=27637
There is a patch to fix that, which is still waiting for review:
https://inbox.sourceware.org/debugedit/20220611212217.4297-1-adrianvovk@gmail.com/

Fedora contains some tests by basically wrapping the invocation into a
full rpmbuild:
https://src.fedoraproject.org/tests/debugedit-gating/tree/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.
>     
>     This works by having find-debuginfo find .a files, extracting all
>     the .o files, individually 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.
>     
>     Signed-off-by: Frank Ch. Eigler <fche@redhat.com>
> 
> diff --git a/scripts/find-debuginfo.in b/scripts/find-debuginfo.in
> index 4cc49f2f8cd0..494d68e05797 100755
> --- a/scripts/find-debuginfo.in
> +++ b/scripts/find-debuginfo.in
> @@ -4,7 +4,7 @@
>  # find-debuginfo - automagically generate debug info and file list
>  # for inclusion in package file lists.
>  
> -# Copyright (C) 2002-2021 rpm and debugedit contributors
> +# Copyright (C) 2002-2025 rpm and debugedit contributors

Although technically correct it isn't super informative. So feel free
to add your own (or your company) copyright statement here.

>  #
>  # 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 +28,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 +95,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.
> +

Is this the right default? I would have expected -a to enable
processing of archives, not disable it.

>  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.
> @@ -167,6 +172,9 @@ quiet=false
>  # add more non-error output
>  verbose=false
>  
> +# process static libraries
> +process_ar=true
> +
>  BUILDDIR=.
>  out=debugfiles.list
>  srcout=
> @@ -260,6 +268,9 @@ while [ $# -gt 0 ]; do
>      srcout=$2
>      shift
>      ;;
> +  -a)
> +    process_ar=false
> +    ;;
>    -q|--quiet)
>      quiet=true
>      verbose=false

So I would have switched these around, init to false, set to true when
-a is seen.

> @@ -443,10 +454,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

How stable is the file output for archives? We have found
bugs/regressions in the file output in the past. We really should use
something like eu-classify --elf-archive
https://bugzilla.redhat.com/show_bug.cgi?id=1609013
(but that is for another time if you know file works fine over some
versions as used by distros).

Also by convention find-debuginfo only processes files with an
executable bit set. Should the same hold for .a archives?

> @@ -461,11 +476,54 @@ 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
> +
> +  artemp="$temp/ar.$inum"
> +  mkdir -p "$artemp"
> +  (cd "$artemp"; ar x "$f")
> +
> +  # 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
> +
> +  find "$artemp" -type f -type f -print | file -N -f - |
> +      sed -n -e 's/^\(.*\):[ 	]*.*ELF.*, not stripped.*/\1/p' |
> +      while read objfile; do
> +          $verbose && echo "processing ${f#$RPM_BUILD_DIR/} ${objfile#$artemp/}"
> +          # nb: debugedit -l writes to $SOURCEFILES in O_APPEND mode, so can be reused.
> +          debugedit -b "$debug_base_name" -d "$debug_dest_name" \
> +		    -l "$SOURCEFILE" "$objfile"
> +          # replace old object with new in archive, nb: relative path names
> +          (cd "$artemp"; ar r "$f" ${objfile#$artemp/}) 
> +      done
> +
> +  $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.
> +}

Would we need to make ar overridable with an environment variable AR,
like we do for READELF, OBJCOPY and NM to make cross-arch processing
possible? See commit cc57e72dcf3018ddb06f4e503c969e904b9a4d27 ("find-
debuginfo: Allow overriding binutils tools").

> +
> +
>  # Strip ELF binaries
>  do_file()
>  {
>    local nlinks=$1 inum=$2 f=$3 id link linked
>  
> +  if expr "$f" : "^.*\.a$" >/dev/null; then # treat as static archive
> +      do_ar_file "$1" "$2" "$3"
> +      return
> +  fi
> +

No error processing? The rest of do_file returns an error when
processing fails. So this really should at least return 0. But better
to also add error processing in do_ar_file and propagate the error
here.

Thanks,

Mark