[PATCH] elf: Drop GRP_COMDAT when localizing a global symbol

[H.J. Lu]

On Tue, Dec 7, 2021 at 5:11 AM Alan Modra <amodra@gmail.com> wrote:
>
> On Tue, Dec 07, 2021 at 04:46:29AM -0800, H.J. Lu wrote:
> > On Tue, Dec 7, 2021 at 12:00 AM Alan Modra <amodra@gmail.com> wrote:
> > >
> > > On Mon, Dec 06, 2021 at 08:54:42AM -0800, H.J. Lu via Binutils wrote:
> > > > --- a/binutils/objcopy.c
> > > > +++ b/binutils/objcopy.c
> > > > @@ -1696,6 +1696,21 @@ filter_symbols (bfd *abfd, bfd *obfd, asymbol **osyms,
> > > >                     && ! is_specified_symbol (name, keepglobal_specific_htab))
> > > >                 || (localize_hidden && is_hidden_symbol (sym))))
> > > >           {
> > > > +           if (bfd_get_flavour (abfd) == bfd_target_elf_flavour)
> > > > +             {
> > > > +               sec_ptr s = sym->section;
> > > > +               if (s != NULL
> > > > +                   && elf_section_data (s) != NULL
> > > > +                   && elf_group_name (s) != NULL)
> > > > +                 {
> > > > +                   /* When a global symbol in a COMDAT group was
> > > > +                      changed to local, drop GRP_COMDAT on the
> > > > +                      group.  */
> > > > +                   s = elf_sec_group (s);
> > > > +                   if (s != NULL && s->output_section != NULL)
> > > > +                     s->output_section->flags &= ~SEC_LINK_ONCE;
> > > > +                 }
> > > > +             }
> > > >             sym->flags &= ~ (BSF_GLOBAL | BSF_WEAK);
> > > >             sym->flags |= BSF_LOCAL;
> > > >           }
> > >
> > > I don't think this is quite correct.  Surely you only want to change
> > > the group semantics if the group symbol changes?  This patch would
> > > change group semantics if *any* symbol defined in the group is
> > > localised.
> >
> > This is done on purpose.  When any global symbol in a comdat group
> > is localized, this comdat group is no longer interchangeable with other
> > comdat groups of the same group signature.
>
> The ELF gABI says of GRP_COMDAT that "duplication is defined as having
> the same group signature".  There is no other requirement.  Things
> that you might imagine are necessary to make groups work in practice

It is not my imagination:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59842

> are not relevant.  objcopy is a tool that can destroy object files.
>
> Besides that, someone may be using objcopy on multiple object files
> localising one symbol for every object, leaving all the comdat groups
> with a given signature the same.
>
> Please read https://groups.google.com/g/generic-abi/c/2X6mR-s2zoc
> again.  The only symbol change that should affect the comdat status of
> the group is the group symbol.

>From Roland in the URL above

A. In step #2, merge COMDAT groups so all the desired merging from the
step #1 translation units is complete before step #3.
    In step #3, have objcopy notice that it's localizing a symbol
that's used as a COMDAT signature symbol and clear the GRP_COMDAT bit
for that group.
    Now step #4 will not be doing any merging of the #1->#2 inputs but
will do all appropriate merging among the other inputs to step #4 (and
any non-localized symbols left after step #3).

>From Ali:

To sum up, it sounds to me like objcopy failing to remove the
GRP_COMDAT as part of step 3 in this process was the bug, and
that your solution A is the right fix.

My patch drops GRP_COMDAT.

-- 
H.J.