[Bug find-debuginfo/30505] New: unchecked "gdb-add-index" invocation

[Bug find-debuginfo/30505] New: unchecked "gdb-add-index" invocation

[lersek at redhat dot com]

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

            Bug ID: 30505
           Summary: unchecked "gdb-add-index" invocation
           Product: debugedit
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: find-debuginfo
          Assignee: unassigned at sourceware dot org
          Reporter: lersek at redhat dot com
                CC: debugedit at sourceware dot org
  Target Milestone: ---

As of commit 5bade25a11b5, "scripts/find-debuginfo.in" contains:

>|   # Add .gdb_index if requested.
>|   if $include_gdb_index; then
>|     if type gdb-add-index >/dev/null 2>&1; then
>|       gdb-add-index "$f"
>|     else
>|       echo >&2 "*** ERROR: GDB index requested, but no gdb-add-index installed"
>|       exit 2
>|     fi
>|   fi

If the objcopy invocation underlying "gdb-add-index" fails, then
"gdb-add-index" propagates that issue through its exit status; however,
"find-debuginfo" ignores it (see above).

This allows RPM builds to succeed if debuginfo extraction is started
with "-i" (i.e., GDB index is being requested), gdb-add-index is
available, but gdb-add-index fails for some reason.

One example is a shared library object that contains Perl bindings for a
C-language library. Perl's ExtUtils::Install module installs artifacts
in read-only mode. On a read-only .so, objcopy fails, hence
gdb-add-index fails. Yet the RPM build succeeds, resulting in such
debuginfo (*.so.debug) files that lack the .gdb_index section. The
expected behavior is that the RPM build fail, similarly to how it
currently fails if "-i" is passed to "find-debuginfo", but
"gdb-add-index" is unavailable (see the"exit 2" statement in the quote).

Installing files in read-only mode is arguably a bug in
ExtUtils::Install itself
<https://rt.cpan.org/Public/Bug/Display.html?id=40976>; however, that
does not mean "find-debuginfo" should ignore "gdb-add-index" failures.

I figure something like the following might work:

>| diff --git a/scripts/find-debuginfo.in b/scripts/find-debuginfo.in
>| index 7dec3c3bfdc5..e0d931175cd9 100755
>| --- a/scripts/find-debuginfo.in
>| +++ b/scripts/find-debuginfo.in
>| @@ -470,6 +470,10 @@ do_file()
>|    if $include_gdb_index; then
>|      if type gdb-add-index >/dev/null 2>&1; then
>|        gdb-add-index "$f"
>| +      exit_status=$?
>| +      if test $exit_status -ne 0; then
>| +        exit $exit_status
>| +      fi
>|      else
>|        echo >&2 "*** ERROR: GDB index requested, but no gdb-add-index installed"
>|        exit 2

However, "find-debuginfo" seems to have an (undocumented) convention
where it uses only exit statuses 1 and 2. In that case, propagating the
exit status from gdb-add-index may not be right. Then one alternative
would be:

>| diff --git a/scripts/find-debuginfo.in b/scripts/find-debuginfo.in
>| index 7dec3c3bfdc5..99cb3b906a00 100755
>| --- a/scripts/find-debuginfo.in
>| +++ b/scripts/find-debuginfo.in
>| @@ -469,7 +469,7 @@ do_file()
>|    # Add .gdb_index if requested.
>|    if $include_gdb_index; then
>|      if type gdb-add-index >/dev/null 2>&1; then
>| -      gdb-add-index "$f"
>| +      gdb-add-index "$f" || exit 1
>|      else
>|        echo >&2 "*** ERROR: GDB index requested, but no gdb-add-index installed"
>|        exit 2

Note that the problem (not checking the exit status of gdb-add-index)
dates back to the original introduction of gdb-add-index to
find-debuginfo:

  https://github.com/rpm-software-management/rpm/commit/9570a7f6af15
  https://github.com/rpm-software-management/rpm/commit/41c4dcf507e2
  https://github.com/rpm-software-management/rpm/commit/67d3df338875
  https://github.com/rpm-software-management/rpm/commit/04b0805a756c

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

[Bug find-debuginfo/30505] unchecked "gdb-add-index" invocation

[mark at klomp dot org]

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

Mark Wielaard <mark at klomp dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mark at klomp dot org

--- Comment #1 from Mark Wielaard <mark at klomp dot org> ---
Thanks for reporting this (and providing different patches to fix it).

I think originally we ignored errors like these (same for add_minidebug)
because these are "just nice to have extras". But given that people explicitly
opt in to these "extras" (or at least not opt-out) we should probably do as you
suggest and explicitly report errors.

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

[Bug find-debuginfo/30505] unchecked "gdb-add-index" invocation

[mark at klomp dot org]

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

Mark Wielaard <mark at klomp dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://bugzilla.redhat.com
                   |                            |/show_bug.cgi?id=1793746

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

[Bug find-debuginfo/30505] unchecked "gdb-add-index" invocation

[keiths at redhat dot com]

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

Keith Seitz <keiths at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |keiths at redhat dot com

--- Comment #2 from Keith Seitz <keiths at redhat dot com> ---
I've developed a more thorough patch for this.

Even with the patch herein applied, find-debuginfo.sh
will never exit with a non-zero status.

I've submitted a patch (which includes something for this
bug):

https://sourceware.org/pipermail/debugedit/2024-August/000290.html

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

[Bug find-debuginfo/30505] unchecked "gdb-add-index" invocation

[mark at klomp dot org]

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

--- Comment #3 from Mark Wielaard <mark at klomp dot org> ---
commit dfe1f7ff30f4e0be538835fca1e6348723ea7aa7
Author: Keith Seitz <keiths@redhat.com>
Date:   Fri Aug 16 11:54:20 2024 -0700

    find-debuginfo.sh: Exit with real exit status in parallel jobs

    Currently, when the script is executed in parallel (-jN), the
    resulting exit status will always be 0.

    The script execs an appropriate number of clones of itself, calling
    run_job to run the actual workload. This then calls do_file(), saving
    the exit status into "res.$jobid".

    In do_file(), though, if an error occurs, exit is called. This causes
    the entire exec'd shell to exit with status 0 (since there are almost
    always echo calls as the last executed statement). The real exit
    status is therefor never written to the "res.$jobid" files by run_job().

    The simple solution is to use 'return' instead of 'exit'. A number
    of minor adjustments are also made to propagate this properly so that
    it is reported as the correct exit status.

    While at it, I've incorporated a patch for find-debuginfo/30505.
    Using this patch and another patch to the RPM package (submitted as
    github issue #3215), failures of gdb-add-index.sh will now properly fail
    the build instead of being swallowed. It should be much easier for
    developers to figure out why their builds have failed should gdb crash.

    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30505
    Signed-off-by: Keith Seitz <keiths@redhat.com>

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

[Bug find-debuginfo/30505] unchecked "debugedit" "gdb-add-index" "strip_to_debug" "add_minidebug" invocation

[mark at klomp dot org]

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

Mark Wielaard <mark at klomp dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|unchecked "gdb-add-index"   |unchecked "debugedit"
                   |invocation                  |"gdb-add-index"
                   |                            |"strip_to_debug"
                   |                            |"add_minidebug" invocation

--- Comment #4 from Mark Wielaard <mark at klomp dot org> ---
Although the above patch now propagates the errors from gdb-add-index, there
are still some other places that "hide" errors. The main debugedit invocation,
strip_to_debug, add_minidebug.

So keeping this bug open for now to track we handle those too.

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