Home Original page

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

Maciej W. Rozycki macro@mips.com
Mon Apr 9 11:50:00 GMT 2018 
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
More information about the Binutils mailing list