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

[Michael Matz]

Hey,

On Tue, 7 Dec 2021, 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.

But it does so if _any_ symbol associated with a comdat-grouped section is 
localized.  The thread above discusses the signature symbol only: if it's 
merely a name donor, if its STB_binding should matter or not, how to deal 
with the (probably buggy) situation when the signature symbol is 
STT_SECTION/STB_LOCAL, and what ld -r should be doing.

Specifically, Rolands solution A for the objcopy --localize-hidden step 
was:

>> 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.

So, you would have to amend your change to only be active if the localized 
symbol happens to be the group_signature() of a group.


Ciao,
Michael.