Re: [PATCH] Implement objcopy/strip --keep-section=<sectionpattern>

[Alan Modra <amodra at gmail dot com>]

On Thu, Oct 31, 2019 at 09:16:03PM -0700, Fāng-ruì Sòng wrote:
> On Thu, Oct 31, 2019 at 7:00 PM Alan Modra <amodra@gmail.com> wrote:
> >
> > On Thu, Oct 31, 2019 at 05:06:58PM -0700, Fāng-ruì Sòng via binutils wrote:
> > > +     * objcopy.c (enum option_values): Add OPTION_KEEP_SECTION.
> > > +     (SECTION_CONTEXT_KEEP): New macro.
> >
> > This needs to be more detailed.  You haven't mentioned that you are
> > changing the other macros, nor have you mentioned the functions you
> > are changing in objcopy.c.  The NEWS, binutils.texi, and testsuite
> > changes also need entries in the ChangeLog.
> > For the macro changes,
> >         (SECTION_CONTEXT_KEEP): Define.  Adjust other SECTION_CONTEXT macros.
> > would be sufficient.
> 
> Thanks. Changed in the updated patch.

Still not complete.

> > > @@ -1367,6 +1373,10 @@ is_strip_section_1 (bfd *abfd ATTRIBUTE_UNUSED, asection *sec)
> > >  static bfd_boolean
> > >  is_strip_section (bfd *abfd ATTRIBUTE_UNUSED, asection *sec)
> > >  {
> > > +  if (find_section_list (bfd_section_name (sec), FALSE, SECTION_CONTEXT_KEEP)
> > > +      != NULL)
> > > +    return FALSE;
> > > +
> > >    if (is_strip_section_1 (abfd, sec))
> > >      return TRUE;
> > >
> >
> > I believe this change should be made in is_strip_section_1 instead.
> > Otherwise, with a proper ChangeLog entry the patch looks good to
> > commit.
> 
> is_strip_section_1 returns either TRUE (strip the section) or FALSE
> (fall through to other conditions). We cannot put the --keep-section
> logic there.

Those "other conditions" are precisely why you should put the new
--keep-section logic in is_strip_section_1.  Really.  SEC_GROUP is
special.

-- 
Alan Modra
Australia Development Lab, IBM