[PATCH] strip: Don't treat fat IR objects as plugin object
Sam James
sam@gentoo.org
Tue Aug 5 19:53:04 GMT 2025
"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.
I will check it.
More information about the Binutils
mailing list