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: "Maciej W. Rozycki" <macro at mips dot com>
- To: Simon Atanasyan <simon at atanasyan dot com>
- Cc: <binutils at sourceware dot org>
- Date: Mon, 9 Apr 2018 12:49:46 +0100
- Subject: Re: [PATCH] BFD: Prevent writing the MIPS _gp_disp symbol into symbol tables
- 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> <20180308155804.g43752hn2r7ly2vx@debian64.galaxy.int>
Hi Simon,
Apologies for the delay, I have been catching up with issues after a
time off.
Please always indicate the change version in the subject with revised
submissions, i.e. [PATCH v2], [PATCH v3], etc. See:
<https://sourceware.org/gdb/wiki/ContributionChecklist> for full details.
Also from your other mail:
> 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.
I've looked into it a bit deeper and realised that
`->elf_backend_link_output_symbol_hook' is called too late to let it have
an effect on the dynamic symbol table. So let's take the
`elf_backend_fixup_symbol' route indeed.
On Thu, 8 Mar 2018, Simon Atanasyan wrote:
> This was tested by running ls test suite on mipsel-linux board.
"LD test suite".
> ld/
>
> * testsuite/ld-mips-elf/mips-elf.exp: Add new gp-disp-sym test
> case to check that _gp_disp symbol is not included into symbol
> tables.
Under the GNU Coding Standard ChangeLog entries are supposed to tell what
changes are made and not why. So: "Add new gp-disp-sym test case."...
> * testsuite/ld-mips-elf/gp-disp-sym.s: Source code for
> the new test.
> * testsuite/ld-mips-elf/gp-disp-sym.d: Expectations for
> the new test.
... however it is the .d file which is the test case and `mips-elf.exp'
only runs it, so how about making this all shorter:
* testsuite/ld-mips-elf/gp-disp-sym.d: New test.
* testsuite/ld-mips-elf/gp-disp-sym.s: New test source.
* testsuite/ld-mips-elf/mips-elf.exp: Run the new test.
?
> diff --git a/bfd/elf32-mips.c b/bfd/elf32-mips.c
> index fa0cc15aba..e6fb9871f5 100644
> --- a/bfd/elf32-mips.c
> +++ b/bfd/elf32-mips.c
> @@ -2414,6 +2414,22 @@ elf32_mips_write_core_note (bfd *abfd, char *buf, int *bufsiz, int note_type,
> }
> }
> }
> +
> +/* Remove the magic _gp_disp symbol from the symbol tables. */
> +
> +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;
> +
> + if (h->dynindx != -1 && strcmp (name, "_gp_disp") == 0)
> + {
> + h->dynindx = -1;
> + _bfd_elf_strtab_delref (elf_hash_table (info)->dynstr, h->dynstr_index);
> + }
I think calling `_bfd_elf_link_hash_hide_symbol' will do and will reduce
code duplication. Also `name' is used only once, so you can get rid of
the extra variable (unless you had a particular reason to add it -- did
you?).
> @@ -2520,6 +2536,7 @@ static const struct ecoff_debug_swap mips_elf32_ecoff_debug_swap = {
> #define elf_backend_grok_prstatus elf32_mips_grok_prstatus
> #define elf_backend_grok_psinfo elf32_mips_grok_psinfo
> #define elf_backend_ecoff_debug_swap &mips_elf32_ecoff_debug_swap
> +#define elf_backend_fixup_symbol elf32_mips_fixup_symbol
Please place it according to the order of `struct elf_backend_data'
members, that is between `elf_backend_copy_indirect_symbol' and
`elf_backend_grok_prstatus'.
> diff --git a/ld/testsuite/ld-mips-elf/gp-disp-sym.d b/ld/testsuite/ld-mips-elf/gp-disp-sym.d
> new file mode 100644
> index 0000000000..9450ce1d72
> --- /dev/null
> +++ b/ld/testsuite/ld-mips-elf/gp-disp-sym.d
> @@ -0,0 +1,9 @@
> +#source: gp-disp-sym.s
> +#as: -EB -mips32 -32
> +#ld: -melf32btsmip -shared
Please avoid explicit endianness/ISA flags for GAS/LD, to keep coverage
as wide as possible. Not all MIPS targets support the `elf32btsmip'
emulation too (e.g. `mips-elf') and its use makes the test fail with them
unnecessarily. Instead pass `[list [list ld $abi_ldflags(o32)]]' on test
invocation.
Also `source' is not needed, because the name corresponds to the name of
the .d file. Please add a `name' tag though as MIPS tests use it.
> +#objdump: -tT
> +
> +#failif
> +#...
> +.*_gp_disp
> +#...
`#pass' here, 'cause it's the end.
> diff --git a/ld/testsuite/ld-mips-elf/gp-disp-sym.s b/ld/testsuite/ld-mips-elf/gp-disp-sym.s
> new file mode 100644
> index 0000000000..c6380ba1fb
> --- /dev/null
> +++ b/ld/testsuite/ld-mips-elf/gp-disp-sym.s
> @@ -0,0 +1,5 @@
> + .global foo
> + .text
> +foo:
> + lui $t0, %hi(_gp_disp)
> + addi $t0, $t0, %lo(_gp_disp)
> diff --git a/ld/testsuite/ld-mips-elf/mips-elf.exp b/ld/testsuite/ld-mips-elf/mips-elf.exp
> index 13dbbc64f9..dcf114d79d 100644
> --- a/ld/testsuite/ld-mips-elf/mips-elf.exp
> +++ b/ld/testsuite/ld-mips-elf/mips-elf.exp
> @@ -1004,6 +1004,9 @@ if { $linux_gnu } {
> # MIPS16 and microMIPS interlinking test.
> run_dump_test "mips16-and-micromips"
>
> +# Test that _gp_disp symbol is not present in EXE or DSO
A full stop is required at the end of a sentence by the GNU Coding
Standard.
Contrary to the comment you are testing a DSO only -- did you mean to
make a second test?
> +run_dump_test "gp-disp-sym"
> +
> # Export class call relocation tests.
> set abis [concat o32 [expr {$has_newabi ? "n32 n64" : ""}]]
> foreach { abi } $abis {
Please place it at the end of the file, there's nothing related to
interlinking or export class tests that would justify grouping with either
of these test.
Please resubmit with these issues addressed.
Maciej