[PATCH] strip: Don't treat fat IR objects as plugin object
H.J. Lu
hjl.tools@gmail.com
Tue Aug 5 20:08:01 GMT 2025
On Tue, Aug 5, 2025 at 12:53 PM Sam James <sam@gentoo.org> wrote:
>
> "H.J. Lu" <hjl.tools@gmail.com> writes:
>
> > On Tue, Aug 5, 2025 at 8:06 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >>
> >> On Tue, Aug 5, 2025 at 7:42 AM Alan Modra <amodra@gmail.com> wrote:
> >> >
> >> > On Mon, Aug 04, 2025 at 07:59:55PM -0700, H.J. Lu wrote:
> >> > > (bfd_check_format_matches_lto): Add a bool argument to indicate
> >> > > called from strip.
> >> >
> >> > This seems like a poor idea to me. I'd rather not see special cases
> >> > like this in bfd_check_format. I think it would be better to keep the
> >> > logic in objcopy.c, even if it means closing then reopening a bfd with
> >> > plugins disabled via bfd_plugin_no in bfd->plugin_format.
> >> >
> >> > The following is something I worked on today, but haven't yet
> >> > committed.
> >> >
> >> > From 8dd8dafe1603e2fe2cc968424a7634283da778bd Mon Sep 17 00:00:00 2001
> >> > From: Alan Modra <amodra@gmail.com>
> >> > Date: Tue, 5 Aug 2025 21:18:44 +0930
> >> > Subject: Remove bfd_check_format_lto
> >> >
> >> > Tidy changes to bfd_check_format_matches made by commit 9b854f169df9
> >> > which added a bfd_plugin_specified_p test and commit f752be8f916e
> >> > which added an lto_sections_removed arg. Both of these changes are
> >> > unnecessary if plugin_format is set to bfd_plugin_no before calling
> >> > bfd_check_format. bfd_plugin_no will prevent the plugin object_p
> >> > function from returning a match (and in the first case from a segfault
> >> > when loading plugins while a plugin is running). The plugin object_p
> >> > function already protected itself from recursive calls by setting
> >> > bfd_plugin_no before loading a plugin, but commit 9b854f169df9 opened
> >> > new bfds so they were unprotected.
> >> >
> >> > It isn't strictly necessary to test for bfd_plugin_no in
> >> > bfd_check_format_matches but I kept the check to minimise functional
> >> > changes. Close inspection of the patch will notice I've added an
> >> > is_linker_input test too. That also isn't strictly necessary, I
> >> > think, but the match_count test was for the linker. See commit
> >> > 999d6dff80fa.
> >> >
> >> > PR 12291
> >> > PR 12430
> >> > PR 13298
> >> > PR 33198
> >> > bfd/
> >> > * format.c (bfd_check_format_lto): Revert to bfd_check_format.
> >> > (bfd_check_format_matches_lto): Revert to bfd_check_format_matches.
> >> > Correct comments. Manage both the lto_sections_removed and
> >> > bfd_plugin_specified_p cases by testing for bfd_plugin_no.
> >> > * plugin.c (bfd_plugin_get_symbols_in_object_only): Set
> >> > plugin_format to bfd_plugin_no before checking new bfds.
> >> > * bfd-in2.h: Regenerate.
> >> > binutils/
> >> > * objcopy.c (copy_archive): Replace bfd_check_format_lto calls
> >> > with bfd_check_format using plugin_format set to bfd_plugin_no.
> >> > (check_format_object): New function.
> >> > (copy_file): Use it.
> >> >
> >> > diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
> >> > index 2ff3e930bfa..b013ef954da 100644
> >> > --- a/bfd/bfd-in2.h
> >> > +++ b/bfd/bfd-in2.h
> >> > @@ -2903,15 +2903,8 @@ bool generic_core_file_matches_executable_p
> >> > (bfd *core_bfd, bfd *exec_bfd);
> >> >
> >> > /* Extracted from format.c. */
> >> > -bool bfd_check_format_lto (bfd *abfd, bfd_format format,
> >> > - bool lto_sections_removed);
> >> > -
> >> > bool bfd_check_format (bfd *abfd, bfd_format format);
> >> >
> >> > -bool bfd_check_format_matches_lto
> >> > - (bfd *abfd, bfd_format format, char ***matching,
> >> > - bool lto_sections_removed);
> >> > -
> >> > bool bfd_check_format_matches
> >> > (bfd *abfd, bfd_format format, char ***matching);
> >> >
> >> > diff --git a/bfd/format.c b/bfd/format.c
> >> > index f3a0774af08..dc05880873c 100644
> >> > --- a/bfd/format.c
> >> > +++ b/bfd/format.c
> >> > @@ -56,19 +56,16 @@ extern const size_t _bfd_target_vector_entries;
> >> >
> >> > /*
> >> > FUNCTION
> >> > - bfd_check_format_lto
> >> > + bfd_check_format
> >> >
> >> > SYNOPSIS
> >> > - bool bfd_check_format_lto (bfd *abfd, bfd_format format,
> >> > - bool lto_sections_removed);
> >> > + bool bfd_check_format (bfd *abfd, bfd_format format);
> >> >
> >> > DESCRIPTION
> >> > Verify if the file attached to the BFD @var{abfd} is compatible
> >> > with the format @var{format} (i.e., one of <<bfd_object>>,
> >> > <<bfd_archive>> or <<bfd_core>>).
> >> >
> >> > - If LTO_SECTION_REMOVED is true, ignore plugin target.
> >> > -
> >> > If the BFD has been set to a specific target before the
> >> > call, only the named target and format combination is
> >> > checked. If the target has not been set, or has been set to
> >> > @@ -102,31 +99,10 @@ DESCRIPTION
> >> > itself.
> >> > */
> >> >
> >> > -bool
> >> > -bfd_check_format_lto (bfd *abfd, bfd_format format,
> >> > - bool lto_sections_removed)
> >> > -{
> >> > - return bfd_check_format_matches_lto (abfd, format, NULL,
> >> > - lto_sections_removed);
> >> > -}
> >> > -
> >> > -
> >> > -/*
> >> > -FUNCTION
> >> > - bfd_check_format
> >> > -
> >> > -SYNOPSIS
> >> > - bool bfd_check_format (bfd *abfd, bfd_format format);
> >> > -
> >> > -DESCRIPTION
> >> > - Similar to bfd_check_format_plugin, except plugin target isn't
> >> > - ignored.
> >> > -*/
> >> > -
> >> > bool
> >> > bfd_check_format (bfd *abfd, bfd_format format)
> >> > {
> >> > - return bfd_check_format_matches_lto (abfd, format, NULL, false);
> >> > + return bfd_check_format_matches (abfd, format, NULL);
> >> > }
> >> >
> >> > struct bfd_preserve
> >> > @@ -431,36 +407,32 @@ bfd_set_lto_type (bfd *abfd ATTRIBUTE_UNUSED)
> >> >
> >> > /*
> >> > FUNCTION
> >> > - bfd_check_format_matches_lto
> >> > + bfd_check_format_matches
> >> >
> >> > SYNOPSIS
> >> > - bool bfd_check_format_matches_lto
> >> > - (bfd *abfd, bfd_format format, char ***matching,
> >> > - bool lto_sections_removed);
> >> > + bool bfd_check_format_matches
> >> > + (bfd *abfd, bfd_format format, char ***matching);
> >> >
> >> > DESCRIPTION
> >> > Like <<bfd_check_format>>, except when it returns FALSE with
> >> > - <<bfd_errno>> set to <<bfd_error_file_ambiguously_recognized>>. In that
> >> > - case, if @var{matching} is not NULL, it will be filled in with
> >> > - a NULL-terminated list of the names of the formats that matched,
> >> > - allocated with <<malloc>>.
> >> > + <<bfd_errno>> set to <<bfd_error_file_ambiguously_recognized>>.
> >> > + In that case, if @var{matching} is not NULL, it will be filled
> >> > + in with a NULL-terminated list of the names of the formats
> >> > + that matched, allocated with <<malloc>>.
> >> > Then the user may choose a format and try again.
> >> >
> >> > When done with the list that @var{matching} points to, the caller
> >> > should free it.
> >> > -
> >> > - If LTO_SECTION_REMOVED is true, ignore plugin target.
> >> > */
> >> >
> >> > bool
> >> > -bfd_check_format_matches_lto (bfd *abfd, bfd_format format,
> >> > - char ***matching,
> >> > - bool lto_sections_removed ATTRIBUTE_UNUSED)
> >> > +bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
> >> > {
> >> > extern const bfd_target binary_vec;
> >> > const bfd_target * const *target;
> >> > const bfd_target **matching_vector = NULL;
> >> > const bfd_target *save_targ, *right_targ, *ar_right_targ, *match_targ;
> >> > + const bfd_target *fail_targ;
> >> > int match_count, best_count, best_match;
> >> > int ar_match_index;
> >> > unsigned int initial_section_id = _bfd_section_id;
> >> > @@ -524,13 +496,14 @@ bfd_check_format_matches_lto (bfd *abfd, bfd_format format,
> >> > if (!bfd_preserve_save (abfd, &preserve, NULL))
> >> > goto err_ret;
> >> >
> >> > - /* If the target type was explicitly specified, just check that target.
> >> > - If LTO_SECTION_REMOVED is true, don't match the plugin target. */
> >> > + /* If the target type was explicitly specified, just check that target. */
> >> > + fail_targ = NULL;
> >> > if (!abfd->target_defaulted
> >> > #if BFD_SUPPORTS_PLUGINS
> >> > - && (!lto_sections_removed || !bfd_plugin_target_p (abfd->xvec))
> >> > + && !(abfd->plugin_format == bfd_plugin_no
> >> > + && bfd_plugin_target_p (save_targ))
> >> > #endif
> >> > - )
> >> > + )
> >> > {
> >> > if (bfd_seek (abfd, 0, SEEK_SET) != 0) /* rewind! */
> >> > goto err_ret;
> >> > @@ -554,10 +527,10 @@ bfd_check_format_matches_lto (bfd *abfd, bfd_format format,
> >> > target. */
> >> > if (format == bfd_archive && save_targ == &binary_vec)
> >> > goto err_unrecog;
> >> > + fail_targ = save_targ;
> >> > }
> >> >
> >> > - /* Since the target type was defaulted, check them all in the hope
> >> > - that one will be uniquely recognized. */
> >> > + /* Check all targets in the hope that one will be recognized. */
> >> > right_targ = NULL;
> >> > ar_right_targ = NULL;
> >> > match_targ = NULL;
> >> > @@ -571,25 +544,25 @@ bfd_check_format_matches_lto (bfd *abfd, bfd_format format,
> >> > void **high_water;
> >> >
> >> > /* The binary target matches anything, so don't return it when
> >> > - searching. Don't match the plugin target if we have another
> >> > - alternative since we want to properly set the input format
> >> > - before allowing a plugin to claim the file. Also, don't
> >> > - check the default target twice. If LTO_SECTION_REMOVED is
> >> > - true, don't match the plugin target. */
> >> > + searching. Also, don't check the current target twice when
> >> > + it has failed already.
> >> > + Don't match the plugin target during linking if we have
> >> > + another alternative since we want to properly set the input
> >> > + format before allowing a plugin to claim the file.
> >> > + Also as an optimisation don't match the plugin target when
> >> > + abfd->plugin_format is set to bfd_plugin_no. (This occurs
> >> > + when LTO sections have been stripped or when we have a
> >> > + recursive call here from the plugin object_p via
> >> > + bfd_plugin_get_symbols_in_object_only.) */
> >> > if (*target == &binary_vec
> >> > + || *target == fail_targ
> >> > #if BFD_SUPPORTS_PLUGINS
> >> > - || ((lto_sections_removed || match_count != 0)
> >> > + || (((abfd->is_linker_input && match_count != 0)
> >> > + || abfd->plugin_format == bfd_plugin_no)
> >> > && bfd_plugin_target_p (*target))
> >> > #endif
> >> > - || (!abfd->target_defaulted && *target == save_targ))
> >> > - continue;
> >> > -
> >> > -#if BFD_SUPPORTS_PLUGINS
> >> > - /* If the plugin target is explicitly specified when a BFD file
> >> > - is opened, don't check it twice. */
> >> > - if (bfd_plugin_specified_p () && bfd_plugin_target_p (*target))
> >> > + )
> >> > continue;
> >> > -#endif
> >> >
> >> > /* If we already tried a match, the bfd is modified and may
> >> > have sections attached, which will confuse the next
> >> > @@ -831,32 +804,6 @@ bfd_check_format_matches_lto (bfd *abfd, bfd_format format,
> >> > return false;
> >> > }
> >> >
> >> > -/*
> >> > -FUNCTION
> >> > - bfd_check_format_matches
> >> > -
> >> > -SYNOPSIS
> >> > - bool bfd_check_format_matches
> >> > - (bfd *abfd, bfd_format format, char ***matching);
> >> > -
> >> > -DESCRIPTION
> >> > - Like <<bfd_check_format>>, except when it returns FALSE with
> >> > - <<bfd_errno>> set to <<bfd_error_file_ambiguously_recognized>>. In that
> >> > - case, if @var{matching} is not NULL, it will be filled in with
> >> > - a NULL-terminated list of the names of the formats that matched,
> >> > - allocated with <<malloc>>.
> >> > - Then the user may choose a format and try again.
> >> > -
> >> > - When done with the list that @var{matching} points to, the caller
> >> > - should free it.
> >> > -*/
> >> > -
> >> > -bool
> >> > -bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
> >> > -{
> >> > - return bfd_check_format_matches_lto (abfd, format, matching, false);
> >> > -}
> >> > -
> >> > /*
> >> > FUNCTION
> >> > bfd_set_format
> >> > diff --git a/bfd/plugin.c b/bfd/plugin.c
> >> > index 1c72b748a8f..f78974861c6 100644
> >> > --- a/bfd/plugin.c
> >> > +++ b/bfd/plugin.c
> >> > @@ -196,7 +196,8 @@ bfd_plugin_get_symbols_in_object_only (bfd *abfd)
> >> > abfd->filename, bfd_errmsg (bfd_get_error ()));
> >> > return;
> >> > }
> >> > - else if (!bfd_check_format (nbfd, bfd_object))
> >> > + nbfd->plugin_format = bfd_plugin_no;
> >> > + if (!bfd_check_format (nbfd, bfd_object))
> >> > {
> >> > /* There is no object only section if it isn't a bfd_object
> >> > file. */
> >> > @@ -239,6 +240,7 @@ bfd_plugin_get_symbols_in_object_only (bfd *abfd)
> >> >
> >> > /* Open the file containing object only section. */
> >> > nbfd = bfd_openr (object_only_file, NULL);
> >> > + nbfd->plugin_format = bfd_plugin_no;
> >> > if (!bfd_check_format (nbfd, bfd_object))
> >> > {
> >> > (*_bfd_error_handler)
> >> > diff --git a/binutils/objcopy.c b/binutils/objcopy.c
> >> > index 5774711abe6..8bd523aba13 100644
> >> > --- a/binutils/objcopy.c
> >> > +++ b/binutils/objcopy.c
> >> > @@ -170,6 +170,8 @@ static bool sections_removed;
> >> > #if BFD_SUPPORTS_PLUGINS
> >> > /* TRUE if all GCC LTO sections are to be removed. */
> >> > static bool lto_sections_removed;
> >> > +#else
> >> > +#define lto_sections_removed false
> >> > #endif
> >> >
> >> > /* TRUE if only some sections are to be copied. */
> >> > @@ -3745,11 +3747,10 @@ copy_archive (bfd *ibfd, bfd *obfd, const char *output_target,
> >> >
> >> > #if BFD_SUPPORTS_PLUGINS
> >> > /* Ignore plugin target if all LTO sections should be removed. */
> >> > - ok_object = bfd_check_format_lto (this_element, bfd_object,
> >> > - lto_sections_removed);
> >> > -#else
> >> > - ok_object = bfd_check_format (this_element, bfd_object);
> >> > + if (lto_sections_removed)
> >> > + this_element->plugin_format = bfd_plugin_no;
> >> > #endif
> >> > + ok_object = bfd_check_format (this_element, bfd_object);
> >> >
> >> > /* PR binutils/3110: Cope with archives
> >> > containing multiple target types. */
> >> > @@ -3863,6 +3864,25 @@ copy_archive (bfd *ibfd, bfd *obfd, const char *output_target,
> >> > return ok;
> >> > }
> >> >
> >> > +static bool
> >> > +check_format_object (bfd *ibfd, char ***obj_matching,
> >> > + bool no_plugins ATTRIBUTE_UNUSED)
> >> > +{
> >> > +#if BFD_SUPPORTS_PLUGINS
> >> > + /* Ignore plugin target first if all LTO sections should be
> >> > + removed. Try with plugin target next if ignoring plugin
> >> > + target fails to match the format. */
> >> > + if (no_plugins && ibfd->plugin_format == bfd_plugin_unknown)
> >> > + {
> >> > + ibfd->plugin_format = bfd_plugin_no;
> >> > + if (bfd_check_format_matches (ibfd, bfd_object, obj_matching))
> >> > + return true;
> >> > + ibfd->plugin_format = bfd_plugin_unknown;
> >> > + }
> >> > +#endif
> >> > + return bfd_check_format_matches (ibfd, bfd_object, obj_matching);
> >> > +}
> >> > +
> >> > /* The top-level control. */
> >> >
> >> > static void
> >> > @@ -3988,21 +4008,7 @@ copy_file (const char *input_filename, const char *output_filename, int ofd,
> >> > input_arch, target_defaulted))
> >> > status = 1;
> >> > }
> >> > - else if (
> >> > -#if BFD_SUPPORTS_PLUGINS
> >> > - /* Ignore plugin target first if all LTO sections should be
> >> > - removed. Try with plugin target next if ignoring plugin
> >> > - target fails to match the format. */
> >> > - bfd_check_format_matches_lto (ibfd, bfd_object, &obj_matching,
> >> > - lto_sections_removed)
> >> > - || (lto_sections_removed
> >> > - && bfd_check_format_matches_lto (ibfd, bfd_object,
> >> > - &obj_matching, false))
> >> > -#else
> >> > - bfd_check_format_matches_lto (ibfd, bfd_object, &obj_matching,
> >> > - false)
> >> > -#endif
> >> > - )
> >> > + else if (check_format_object (ibfd, &obj_matching, lto_sections_removed))
> >> > {
> >> > bfd *obfd;
> >> > do_copy:
> >> >
> >>
> >> This failed PR 33246 tests in my patch:
> >>
> >> FAIL: pr33246-fat with --strip-debug --enable-deterministic-archives
> >> (strip tmpdir/pr33246-fat.o)
> >> FAIL: pr33246-llvm-fat with --strip-debug
> >> --enable-deterministic-archives (strip tmpdir/pr33246-llvm-fat.o)
> >>
> >
> > This works on top of yours.
>
> Actually, could you give me one with the testcases included? (Just a
> hack is fine, it doesn't need to be properly formatted or anything.)
This is the combined patch.
--
H.J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-strip-Don-t-treat-fat-IR-objects-as-plugin-object.patch
Type: text/x-patch
Size: 24452 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20250805/fe91d956/attachment-0001.bin>
More information about the Binutils
mailing list