This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] BFD: Prevent writing the MIPS _gp_disp symbol into symbol tables
- From: Simon Atanasyan <simon at atanasyan dot com>
- To: "Maciej W. Rozycki" <macro at mips dot com>
- Cc: binutils at sourceware dot org
- Date: Thu, 8 Mar 2018 18:45:29 +0300
- Subject: Re: [PATCH] BFD: Prevent writing the MIPS _gp_disp symbol into symbol tables
- Authentication-results: sourceware.org; auth=none
- References: <20180128102923.vo4pmswxmhiopvbt@debian64.galaxy.int> <alpine.DEB.2.00.1802062000360.3553@tp.orcam.me.uk> <CAGyS+DQ7LncsG_jsNCFgj+LcfLZyYATjPeG0s43FO3NkPFHOpQ@mail.gmail.com> <alpine.DEB.2.00.1802230847370.3553@tp.orcam.me.uk>
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.