Re: [PATCH] BFD: Prevent writing the MIPS _gp_disp symbol into symbol tables

[Simon Atanasyan <simon at atanasyan dot com>]

Hi Maciej,

Thanks for so detailed review.

On Tue, Feb 27, 2018 at 01:52:02PM +0000, Maciej W. Rozycki wrote:
>  Thank you for the test case.  I think it will be good if it is included 
> with the commit as a part of the LD test suite.  I think the test should 
> negatively match the presence of `_gp_disp' in the output of `readelf -s'.  
> 
>  Will you be able to work on this as an exercise or shall I look into it 
> myself?

I included the new test case into this patch. I will send the updated
patch as a separate message.

> > BFD: Prevent writing the MIPS _gp_disp symbol into symbol tables
> 
>  Don't repeat the change heading in the description.

OK

>  Please include a quote from the relevant part of the ELF gABI in your 
> updated description to back up the zero index interpretation, so that 
> people do not have to chase it.

OK

>  Also please use two spaces after full stops as per the GNU Coding 
> Standards[2]:

OK

> > 2018-01-25  Simon Atanasyan  <simon.atanasyan@mips.com>
> 
>  You have:
> 
> From: Simon Atanasyan <simon@atanasyan.com>
> 
> in the headers however, so which authorship attribution is right for this 
> submission?

I will use simon@atanasyan.com. The same address is mentioned in my
copyright assignment.

> > bfd/
> > 
> > 	* elf32-mips.c: (elf32_mips_fixup_symbol ): New function.
> 
>  No space before the closing parenthesis.

Fixed.

> > 	(elf_backend_fixup_symbol): New macro.
> > 	* elfxx-mips.c: (_bfd_mips_elf_link_output_symbol_hook): Discard
> > 	the _gp_disp symbol in case of O32 ABI.
> > 
> > testsuite/ld
> > 
> > 	* ld-mips-elf/mips16-pic-2.ad: Update test case expectations.
> 
>  We don't use separate ChangeLog files for test suites anymore (and BTW 
> the path is ld/testsuite/), so please update accordingly:

Fixed.

> > +static bfd_boolean
> > +elf32_mips_fixup_symbol (struct bfd_link_info *info,
> > +			    struct elf_link_hash_entry *h)
> > +{
> > +  const char *name = h->root.root.string;
> 
>  Please add a new line after the variable declaration block.
> 
> > +  if (h->dynindx != -1 && strcmp(name, "_gp_disp") == 0)
> 
>  Space before the opening parenthesis in a function call.

Fixed.

> > +    {
> > +      h->dynindx = -1;
> > +      _bfd_elf_strtab_delref (elf_hash_table (info)->dynstr,
> > +			      h->dynstr_index);
> > +    }
> > +  return TRUE;
> > +}
> 
>  Why do you actually need this part in addition to the other one?  It 
> looks to me like the change to the `elf_backend_link_output_symbol_hook' 
> handler should do by itself what you require.

In fact, _bfd_mips_elf_link_output_symbol_hook does not need to be
changed. If I add a fix to the _bfd_mips_elf_link_output_symbol_hook
only, linker stops to put the _gp_disp symbol into the .symbols, but
continues to write an entry to the .dynsyms. If I add elf32_mips_fixup_symbol
routine and keep _bfd_mips_elf_link_output_symbol_hook unchanged, linker
stops to write _gp_disp symbol into the both .symbols and .dynsym tables.

> > @@ -7741,9 +7741,16 @@ _bfd_mips_elf_add_symbol_hook (bfd *abfd, struct
> > bfd_link_info *info,
> >  int
> >  _bfd_mips_elf_link_output_symbol_hook
> >    (struct bfd_link_info *info ATTRIBUTE_UNUSED,
> > -   const char *name ATTRIBUTE_UNUSED, Elf_Internal_Sym *sym,
> > -   asection *input_sec, struct elf_link_hash_entry *h ATTRIBUTE_UNUSED)
> > +   const char *name, Elf_Internal_Sym *sym, asection *input_sec,
> > +   struct elf_link_hash_entry *h ATTRIBUTE_UNUSED)
> >  {
> > +  /* Discard the _gp_disp symbol.  */
> > +  if (! NEWABI_P (info->output_bfd)
> > +      && ! bfd_link_relocatable (info)
> > +      && name
> > +      && strcmp (name, "_gp_disp") == 0)
> > +    return 2;
> 
>  Please don't update this handler like this.  Instead define a new handler 
> in bfd/elf32-mips.c and tail-call `_bfd_mips_elf_link_output_symbol_hook' 
> after the newly added case has been processed.

Fixed.

>  Also since we will not be outputting `_gp_disp' anymore, please discard 
> code in `mips_elf_output_extsym' and `_bfd_mips_elf_finish_dynamic_symbol' 
> that arranges for this symbol to become section/absolute in the output 
> symbol table as this will now become dead code.

Fixed.

>  Also you didn't write how your change was tested.  Please always include 
> such information with patch submissions.  This will typically require 
> running regression tests at the very least.

The patch was tested by running ls test suite on mipsel-linux board I
will mention it in the patch description.