[v2] MIPS: Don't move __gnu_lto_slim to .scommon

  The LTO plugin doesn't expect __gnu_lto_slim is marked as
COMMOM or moved .scommon section.

Let's skip __gnu_lto_slim when detect symbols to be moved to
.scommon.

This patch can fix testcase:
	PR ld/15323 (3)
	PR ld/15323 (4)
for MIPS.
---
 bfd/elfxx-mips.c | 4 ++++
 1 file changed, 4 insertions(+)
  

YunQiang Su <yunqiang.su@cipunited.com> 于2023年7月3日周一 18:51写道：
>
> The LTO plugin doesn't expect __gnu_lto_slim is marked as
> COMMOM or moved .scommon section.
>

@Maciej W. Rozycki This is a tiny patch, and I think that it should be
in 2.41 release.
Any idea?

> Let's skip __gnu_lto_slim when detect symbols to be moved to
> .scommon.
>
> This patch can fix testcase:
>         PR ld/15323 (3)
>         PR ld/15323 (4)
> for MIPS.
> ---
>  bfd/elfxx-mips.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c
> index 71f2dc9d779..63742de009c 100644
> --- a/bfd/elfxx-mips.c
> +++ b/bfd/elfxx-mips.c
> @@ -7861,6 +7861,10 @@ _bfd_mips_elf_add_symbol_hook (bfd *abfd, struct bfd_link_info *info,
>    switch (sym->st_shndx)
>      {
>      case SHN_COMMON:
> +      /* __gnu_lto_slim shouldn't mark as COMMON and move to .scommon:
> +       lto plugin doesn't expect so.  */
> +      if (strcmp (*namep, "__gnu_lto_slim") == 0)
> +       break;
>        /* Common symbols less than the GP size are automatically
>          treated as SHN_MIPS_SCOMMON symbols.  */
>        if (sym->st_size > elf_gp_size (abfd)
> --
> 2.30.2
>
  

YunQiang Su <wzssyqa@gmail.com> 于2023年7月5日周三 09:42写道：
>
> YunQiang Su <yunqiang.su@cipunited.com> 于2023年7月3日周一 18:51写道：
> >
> > The LTO plugin doesn't expect __gnu_lto_slim is marked as
> > COMMOM or moved .scommon section.
> >
>
> @Maciej W. Rozycki This is a tiny patch, and I think that it should be
> in 2.41 release.
> Any idea?
>

ping

> > Let's skip __gnu_lto_slim when detect symbols to be moved to
> > .scommon.
> >
> > This patch can fix testcase:
> >         PR ld/15323 (3)
> >         PR ld/15323 (4)
> > for MIPS.
> > ---
> >  bfd/elfxx-mips.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c
> > index 71f2dc9d779..63742de009c 100644
> > --- a/bfd/elfxx-mips.c
> > +++ b/bfd/elfxx-mips.c
> > @@ -7861,6 +7861,10 @@ _bfd_mips_elf_add_symbol_hook (bfd *abfd, struct bfd_link_info *info,
> >    switch (sym->st_shndx)
> >      {
> >      case SHN_COMMON:
> > +      /* __gnu_lto_slim shouldn't mark as COMMON and move to .scommon:
> > +       lto plugin doesn't expect so.  */
> > +      if (strcmp (*namep, "__gnu_lto_slim") == 0)
> > +       break;
> >        /* Common symbols less than the GP size are automatically
> >          treated as SHN_MIPS_SCOMMON symbols.  */
> >        if (sym->st_size > elf_gp_size (abfd)
> > --
> > 2.30.2
> >
>
>
> --
> YunQiang Su
  

On Wed, 5 Jul 2023, YunQiang Su wrote:

> > The LTO plugin doesn't expect __gnu_lto_slim is marked as
> > COMMOM or moved .scommon section.
> >
> 
> @Maciej W. Rozycki This is a tiny patch, and I think that it should be
> in 2.41 release.
> Any idea?

 What produces this symbol and why is it a common symbol in the first 
place if it's not supposed to?

 NB the mail server for your @gmail.com address rejects e-mail coming from 
my system.  Please sort this out with your ISP.

  Maciej
  

Maciej W. Rozycki <macro@orcam.me.uk> 于2023年7月19日周三 22:55写道：
>
> On Wed, 5 Jul 2023, YunQiang Su wrote:
>
> > > The LTO plugin doesn't expect __gnu_lto_slim is marked as
> > > COMMOM or moved .scommon section.
> > >
> >
> > @Maciej W. Rozycki This is a tiny patch, and I think that it should be
> > in 2.41 release.
> > Any idea?
>
>  What produces this symbol and why is it a common symbol in the first
> place if it's not supposed to?
>

The symbol is produced by GCC LTO, and it has SHN_COMMON, while no
SEC_IS_COMMON flag is set.

When gcc -r -flto, this symbol is collected into .scommon and marked
as SEC_IS_COMMON,
which is rejected by linker.

I have no idea why gcc lto uses SHN_COMMON, while it is used by other
architectures.
That's why I block  `__gnu_lto_slim` out .scommon and SEC_IS_COMMON flag.

See bfd/linker.c (_bfd_generic_link_add_one_symbol):
  else if (bfd_is_com_section (section))
    {
      row = COMMON_ROW;
      if (!bfd_link_relocatable (info)
          && name != NULL
          && name[0] == '_'
          && name[1] == '_'
          && strcmp (name + (name[2] == '_'), "__gnu_lto_slim") == 0)
        _bfd_error_handler
          (_("%pB: plugin needed to handle lto object"), abfd);
    }

>  NB the mail server for your @gmail.com address rejects e-mail coming from
> my system.  Please sort this out with your ISP.
>
>   Maciej
  

I think the patch is OK, but the same should be applied to another
place to stop objcopy modifying __gnu_lto_slim.

mips-linux-gnu testcase after running make check:
  binutils/objcopy ld/tmpdir/pr15323a-r.o ld/tmpdir/xxx
then inspect ld/tmpdir/xxx with readelf.

I'm going to apply the following to mainline, and will also apply to
the 2.41 branch tomorrow if no one objects.

	* elfxx-mips.c (_bfd_mips_elf_symbol_processing): Don't treat
	__gnu_lto_slim as SHN_MIPS_SCOMMON.
	(_bfd_mips_elf_add_symbol_hook): Likewise.

diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c
index 71f2dc9d779..1e9851c3190 100644
--- a/bfd/elfxx-mips.c
+++ b/bfd/elfxx-mips.c
@@ -7176,10 +7176,11 @@ _bfd_mips_elf_symbol_processing (bfd *abfd, asymbol *asym)
 
     case SHN_COMMON:
       /* Common symbols less than the GP size are automatically
-	 treated as SHN_MIPS_SCOMMON symbols on IRIX5.  */
+	 treated as SHN_MIPS_SCOMMON symbols, with some exceptions.  */
       if (asym->value > elf_gp_size (abfd)
 	  || ELF_ST_TYPE (elfsym->internal_elf_sym.st_info) == STT_TLS
-	  || IRIX_COMPAT (abfd) == ict_irix6)
+	  || IRIX_COMPAT (abfd) == ict_irix6
+	  || strcmp (asym->name, "__gnu_lto_slim") == 0)
 	break;
       /* Fall through.  */
     case SHN_MIPS_SCOMMON:
@@ -7862,10 +7863,11 @@ _bfd_mips_elf_add_symbol_hook (bfd *abfd, struct bfd_link_info *info,
     {
     case SHN_COMMON:
       /* Common symbols less than the GP size are automatically
-	 treated as SHN_MIPS_SCOMMON symbols.  */
+	 treated as SHN_MIPS_SCOMMON symbols, with some exceptions.  */
       if (sym->st_size > elf_gp_size (abfd)
 	  || ELF_ST_TYPE (sym->st_info) == STT_TLS
-	  || IRIX_COMPAT (abfd) == ict_irix6)
+	  || IRIX_COMPAT (abfd) == ict_irix6
+	  || strcmp (*namep, "__gnu_lto_slim") == 0)
 	break;
       /* Fall through.  */
     case SHN_MIPS_SCOMMON:
  

On Thu, 20 Jul 2023, Alan Modra wrote:

> I think the patch is OK, but the same should be applied to another
> place to stop objcopy modifying __gnu_lto_slim.
> 
> mips-linux-gnu testcase after running make check:
>   binutils/objcopy ld/tmpdir/pr15323a-r.o ld/tmpdir/xxx
> then inspect ld/tmpdir/xxx with readelf.
> 
> I'm going to apply the following to mainline, and will also apply to
> the 2.41 branch tomorrow if no one objects.

 You are the author of this code, so I guess you know what's going on 
here.  I still don't understand the circumstances that cause linker.c to 
reject this symbol if it's in an SHN_MIPS_SCOMMON rather than SHN_COMMON 
section, and the relevant git change descriptions (going back to commit 
b794fc1d1c3a ("Warn for ar/nm/ranlib/ld on lto objects without plugin")) 
do not explain it, so I'd appreciate if you got me enlightened.

  Maciej
  

On Thu, Jul 20, 2023 at 10:34:43AM +0100, Maciej W. Rozycki wrote:
> On Thu, 20 Jul 2023, Alan Modra wrote:
> 
> > I think the patch is OK, but the same should be applied to another
> > place to stop objcopy modifying __gnu_lto_slim.
> > 
> > mips-linux-gnu testcase after running make check:
> >   binutils/objcopy ld/tmpdir/pr15323a-r.o ld/tmpdir/xxx
> > then inspect ld/tmpdir/xxx with readelf.
> > 
> > I'm going to apply the following to mainline, and will also apply to
> > the 2.41 branch tomorrow if no one objects.
> 
>  You are the author of this code, so I guess you know what's going on 
> here.  I still don't understand the circumstances that cause linker.c to 
> reject this symbol if it's in an SHN_MIPS_SCOMMON rather than SHN_COMMON 
> section, and the relevant git change descriptions (going back to commit 
> b794fc1d1c3a ("Warn for ar/nm/ranlib/ld on lto objects without plugin")) 
> do not explain it, so I'd appreciate if you got me enlightened.

It isn't anything in linker.c, I believe the problem occurs in the lto
plugin which processes object files using libiberty/simple-object*.
Code in libiberty/simple-object-elf.c removes SHN_COMMON symbols,
which for most architectures removes __gnu_lto_slim, but not on mips
if the symbol is moved to SHN_MIPS_SCOMMON.  Then the symbol appears
in lto output files, resulting in linker errors like:
/tmp/ccg5JkW9.debug.temp.o: plugin needed to handle lto object

I haven't actually looked at this under gdb to see exactly what is
going on for mips, I'm relying on memory from doing so for powerpc a
while ago.
  

On Thu, 20 Jul 2023, Alan Modra wrote:

> > > I think the patch is OK, but the same should be applied to another
> > > place to stop objcopy modifying __gnu_lto_slim.
> > > 
> > > mips-linux-gnu testcase after running make check:
> > >   binutils/objcopy ld/tmpdir/pr15323a-r.o ld/tmpdir/xxx
> > > then inspect ld/tmpdir/xxx with readelf.
> > > 
> > > I'm going to apply the following to mainline, and will also apply to
> > > the 2.41 branch tomorrow if no one objects.
> > 
> >  You are the author of this code, so I guess you know what's going on 
> > here.  I still don't understand the circumstances that cause linker.c to 
> > reject this symbol if it's in an SHN_MIPS_SCOMMON rather than SHN_COMMON 
> > section, and the relevant git change descriptions (going back to commit 
> > b794fc1d1c3a ("Warn for ar/nm/ranlib/ld on lto objects without plugin")) 
> > do not explain it, so I'd appreciate if you got me enlightened.
> 
> It isn't anything in linker.c, I believe the problem occurs in the lto
> plugin which processes object files using libiberty/simple-object*.
> Code in libiberty/simple-object-elf.c removes SHN_COMMON symbols,
> which for most architectures removes __gnu_lto_slim, but not on mips
> if the symbol is moved to SHN_MIPS_SCOMMON.  Then the symbol appears
> in lto output files, resulting in linker errors like:
> /tmp/ccg5JkW9.debug.temp.o: plugin needed to handle lto object

 It seems to me then we should be fixing libiberty instead, to also remove 
SHN_MIPS_SCOMMON symbols, for whatever intent it's done, shouldn't we?

> I haven't actually looked at this under gdb to see exactly what is
> going on for mips, I'm relying on memory from doing so for powerpc a
> while ago.

 Completely understood.

  Maciej
  

On Thu, Jul 20, 2023 at 03:32:49PM +0100, Maciej W. Rozycki wrote:
> On Thu, 20 Jul 2023, Alan Modra wrote:
> 
> > > > I think the patch is OK, but the same should be applied to another
> > > > place to stop objcopy modifying __gnu_lto_slim.
> > > > 
> > > > mips-linux-gnu testcase after running make check:
> > > >   binutils/objcopy ld/tmpdir/pr15323a-r.o ld/tmpdir/xxx
> > > > then inspect ld/tmpdir/xxx with readelf.
> > > > 
> > > > I'm going to apply the following to mainline, and will also apply to
> > > > the 2.41 branch tomorrow if no one objects.
> > > 
> > >  You are the author of this code, so I guess you know what's going on 
> > > here.  I still don't understand the circumstances that cause linker.c to 
> > > reject this symbol if it's in an SHN_MIPS_SCOMMON rather than SHN_COMMON 
> > > section, and the relevant git change descriptions (going back to commit 
> > > b794fc1d1c3a ("Warn for ar/nm/ranlib/ld on lto objects without plugin")) 
> > > do not explain it, so I'd appreciate if you got me enlightened.
> > 
> > It isn't anything in linker.c, I believe the problem occurs in the lto
> > plugin which processes object files using libiberty/simple-object*.
> > Code in libiberty/simple-object-elf.c removes SHN_COMMON symbols,
> > which for most architectures removes __gnu_lto_slim, but not on mips
> > if the symbol is moved to SHN_MIPS_SCOMMON.  Then the symbol appears
> > in lto output files, resulting in linker errors like:
> > /tmp/ccg5JkW9.debug.temp.o: plugin needed to handle lto object
> 
>  It seems to me then we should be fixing libiberty instead, to also remove 
> SHN_MIPS_SCOMMON symbols, for whatever intent it's done, shouldn't we?

It would be quite reasonable to change the libiberty code to test for
EM_MIPS and SHN_MIPS_SCOMMON, or to discard __gnu_lto_slim by name.
(The only reason that simple_object_elf_copy_lto_debug_sections
removes SHN_COMMON symbols is to remove __gnu_lto_slim.)

However the symbol is a marker only, so I think it is also entirely
reasonable to stop mips ld -r or objcopy from modifying it
unexpectedly.