Re: [PATCH v2 3/6] objcopy: Remove SHT_LLVM_ADDRSIG sections by default.

[Tatsuyuki Ishi <ishitatsuyuki@gmail.com>]

Hi Jan, thanks again for your review.

> > --- a/binutils/objcopy.c
> > +++ b/binutils/objcopy.c
> > @@ -1316,10 +1316,20 @@ is_mergeable_note_section (bfd * abfd, asection * sec)
> >    return false;
> >  }
> >
> > +static bool
> > +is_addrsig_section (bfd *abfd, asection *sec)
> > +{
> > +  if (bfd_get_flavour (abfd) == bfd_target_elf_flavour
> > +      && elf_section_data (sec)->this_hdr.sh_type == SHT_LLVM_ADDRSIG)
> > +    return true;
> > +
> > +  return false;
> > +}
>
> Not sure if this would conflict with unwritten style expectations,
> but when I see such I always wonder: Why not a simple return
> statement, without explicit use of true/false (or 1/0)?

I just blindly copied the style of is_mergeable_note_section above,
but I think your suggestion also makes sense. If there's no objections
I will turn that into a direct return in the next revision.

> > @@ -1348,6 +1358,20 @@ is_strip_section_1 (bfd *abfd ATTRIBUTE_UNUSED, asection *sec)
> >       return true;
> >      }
> >
> > +  /* addrsig needs to be updated if the symtab is altered, but we don't know
> > +     how to read nor write it, so just throw it away to not confuse
> > +     downstream tools.  */
> > +  if (is_addrsig_section (abfd, sec))
> > +    {
> > +      non_fatal (_ ("warning: removing section %s to prevent corrupt addrsig "
> > +                    "information"),
> > +                 bfd_section_name (sec));
> > +      non_fatal (_ ("pass --remove-section=%1$s to suppress warning, or "
> > +                    "--keep-section=%1$s to override"),
> > +                 bfd_section_name (sec));
> > +      return true;
> > +    }
>
> Isn't this too aggressive? The comment says "if the symtab is altered",
> but the code looks to request stripping unconditionally.

Sorry, the way the comment is written is indeed quite confusing, so I
can update that. But there's probably not too much value in checking
whether symtab is altered here, since it has been thrown away
(unconditionally) by LLD from the sh_link heuristic in prior versions
of binutils anyway.

Tatsuyuki