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

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Simon Atanasyan
BFD: Prevent writing the MIPS _gp_disp symbol into symbol tables

The _gp_disp is a magic symbol, always implicitly defined by the linker.
It does not make a sense to write it into symbol tables for output files.
Moreover, now if the linker gets a version script, the _gp_disp symbol
gets zero version definition index. This symbol is global while the zero
index means unversioned local symbol. That confuses some tools like for
example the LLD linker when they get such files as inputs.

This patch fixes the problems - it prevents writing the _gp_disp symbol
in regular and dynamic symbol tables.

2018-01-25  Simon Atanasyan  <[hidden email]>

bfd/

        * elf32-mips.c: (elf32_mips_fixup_symbol ): New function.
        (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.
        * ld-mips-elf/mips16-pic-2.nd: Likewise.
        * ld-mips-elf/pic-and-nonpic-3a.dd: Likewise.
        * ld-mips-elf/tlslib-o32-hidden.got: Likewise.
        * ld-mips-elf/tlslib-o32-ver.got: Likewise.
        * ld-mips-elf/tlslib-o32.got: Likewise.

diff --git a/bfd/elf32-mips.c b/bfd/elf32-mips.c
index fa0cc15aba..5f5b08136f 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 magic _gp_disp symbol from the dynamic symbol table.  */
+
+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);
+    }
+  return TRUE;
+}
 
 /* Depending on the target vector we generate some version of Irix
    executables or "normal" MIPS ELF ABI executables.  */
@@ -2598,6 +2614,9 @@ static const struct ecoff_debug_swap mips_elf32_ecoff_debug_swap = {
 #undef elf_backend_write_core_note
 #define elf_backend_write_core_note elf32_mips_write_core_note
 
+#undef elf_backend_fixup_symbol
+#define elf_backend_fixup_symbol elf32_mips_fixup_symbol
+
 /* Include the target file again for this target.  */
 #include "elf32-target.h"
 
@@ -2620,6 +2639,7 @@ static const struct ecoff_debug_swap mips_elf32_ecoff_debug_swap = {
 #define elf32_bed elf32_fbsd_tradbed
 
 #undef elf_backend_write_core_note
+#undef elf_backend_fixup_symbol
 
 #include "elf32-target.h"
 /* Implement elf_backend_final_write_processing for VxWorks.  */
diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c
index 285401367d..f387ca47db 100644
--- a/bfd/elfxx-mips.c
+++ b/bfd/elfxx-mips.c
@@ -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;
+
   /* If we see a common symbol, which implies a relocatable link, then
      if a symbol was small common in an input file, mark it as small
      common in the output file.  */
diff --git a/ld/testsuite/ld-mips-elf/mips16-pic-2.ad b/ld/testsuite/ld-mips-elf/mips16-pic-2.ad
index 52d3ea4c3b..689f0c2557 100644
--- a/ld/testsuite/ld-mips-elf/mips16-pic-2.ad
+++ b/ld/testsuite/ld-mips-elf/mips16-pic-2.ad
@@ -1,6 +1,6 @@
 # [MIPS_GOTSYM, MIPS_SYMTABNO) covers used4...used7.
 #...
- .* \(MIPS_SYMTABNO\) * 8
+ .* \(MIPS_SYMTABNO\) * 7
 #...
- .* \(MIPS_GOTSYM\) * 0x4
+ .* \(MIPS_GOTSYM\) * 0x3
 #pass
diff --git a/ld/testsuite/ld-mips-elf/mips16-pic-2.nd b/ld/testsuite/ld-mips-elf/mips16-pic-2.nd
index bc2cd38ee9..a2a579412f 100644
--- a/ld/testsuite/ld-mips-elf/mips16-pic-2.nd
+++ b/ld/testsuite/ld-mips-elf/mips16-pic-2.nd
@@ -1,9 +1,9 @@
 # used8 should come before MIPS_GOTSYM.
 #...
- +3: 000405bc +36 +FUNC +GLOBAL +DEFAULT .* used8
- +4: 00040574 +36 +FUNC +GLOBAL +DEFAULT .* used6
- +5: 00040598 +36 +FUNC +GLOBAL +DEFAULT .* used7
- +6: 00040550 +36 +FUNC +GLOBAL +DEFAULT .* used5
- +7: 0004052c +36 +FUNC +GLOBAL +DEFAULT .* used4
+ +2: 000405bc +36 +FUNC +GLOBAL +DEFAULT .* used8
+ +3: 00040574 +36 +FUNC +GLOBAL +DEFAULT .* used6
+ +4: 00040598 +36 +FUNC +GLOBAL +DEFAULT .* used7
+ +5: 00040550 +36 +FUNC +GLOBAL +DEFAULT .* used5
+ +6: 0004052c +36 +FUNC +GLOBAL +DEFAULT .* used4
 
 #pass
diff --git a/ld/testsuite/ld-mips-elf/pic-and-nonpic-3a.dd b/ld/testsuite/ld-mips-elf/pic-and-nonpic-3a.dd
index 3dcfe12cfc..b286f131f4 100644
--- a/ld/testsuite/ld-mips-elf/pic-and-nonpic-3a.dd
+++ b/ld/testsuite/ld-mips-elf/pic-and-nonpic-3a.dd
@@ -35,5 +35,5 @@ Disassembly of section \.MIPS\.stubs:
  c00: 8f998010 lw t9,-32752\(gp\)
  c04: 03e07825 move t7,ra
  c08: 0320f809 jalr t9
- c0c: 24180005 li t8,5
+ c0c: 24180004 li t8,4
  \.\.\.
diff --git a/ld/testsuite/ld-mips-elf/tlslib-o32-hidden.got b/ld/testsuite/ld-mips-elf/tlslib-o32-hidden.got
index 563d8bb082..a746031f7e 100644
--- a/ld/testsuite/ld-mips-elf/tlslib-o32-hidden.got
+++ b/ld/testsuite/ld-mips-elf/tlslib-o32-hidden.got
@@ -4,11 +4,11 @@
 DYNAMIC RELOCATION RECORDS
 OFFSET   TYPE              VALUE
 00000000 R_MIPS_NONE       \*ABS\*
-000403bc R_MIPS_TLS_TPREL32  \*ABS\*
-000403c0 R_MIPS_TLS_DTPMOD32  \*ABS\*
-000403c8 R_MIPS_TLS_DTPMOD32  \*ABS\*
+0004039c R_MIPS_TLS_TPREL32  \*ABS\*
+000403a0 R_MIPS_TLS_DTPMOD32  \*ABS\*
+000403a8 R_MIPS_TLS_DTPMOD32  \*ABS\*
 
 
 Contents of section .got:
- 403b0 00000000 80000000 00000380 00000008  ................
- 403c0 00000000 ffff8004 00000000 00000000  ................
+ 40390 00000000 80000000 00000360 00000008  ................
+ 403a0 00000000 ffff8004 00000000 00000000  ................
diff --git a/ld/testsuite/ld-mips-elf/tlslib-o32-ver.got b/ld/testsuite/ld-mips-elf/tlslib-o32-ver.got
index e675f9f64a..17a6385e8e 100644
--- a/ld/testsuite/ld-mips-elf/tlslib-o32-ver.got
+++ b/ld/testsuite/ld-mips-elf/tlslib-o32-ver.got
@@ -4,12 +4,12 @@
 DYNAMIC RELOCATION RECORDS
 OFFSET   TYPE              VALUE
 00000000 R_MIPS_NONE       \*ABS\*
-000404d8 R_MIPS_TLS_DTPMOD32  \*ABS\*
-000404d0 R_MIPS_TLS_DTPMOD32  tlsvar_gd@@VER_1
-000404d4 R_MIPS_TLS_DTPREL32  tlsvar_gd@@VER_1
-000404cc R_MIPS_TLS_TPREL32  tlsvar_ie@@VER_1
+000404b8 R_MIPS_TLS_DTPMOD32  \*ABS\*
+000404b0 R_MIPS_TLS_DTPMOD32  tlsvar_gd@@VER_1
+000404b4 R_MIPS_TLS_DTPREL32  tlsvar_gd@@VER_1
+000404ac R_MIPS_TLS_TPREL32  tlsvar_ie@@VER_1
 
 
 Contents of section .got:
- 404c0 00000000 80000000 00000490 00000000  ................
- 404d0 00000000 00000000 00000000 00000000  ................
+ 404a0 00000000 80000000 00000470 00000000  ................
+ 404b0 00000000 00000000 00000000 00000000  ................
diff --git a/ld/testsuite/ld-mips-elf/tlslib-o32.got b/ld/testsuite/ld-mips-elf/tlslib-o32.got
index ad90fb019e..a389c30146 100644
--- a/ld/testsuite/ld-mips-elf/tlslib-o32.got
+++ b/ld/testsuite/ld-mips-elf/tlslib-o32.got
@@ -4,12 +4,12 @@ tmpdir/tlslib-o32.so:     file format elf32-tradbigmips
 DYNAMIC RELOCATION RECORDS
 OFFSET   TYPE              VALUE
 00000000 R_MIPS_NONE       \*ABS\*
-00040448 R_MIPS_TLS_DTPMOD32  \*ABS\*
-00040440 R_MIPS_TLS_DTPMOD32  tlsvar_gd
-00040444 R_MIPS_TLS_DTPREL32  tlsvar_gd
-0004043c R_MIPS_TLS_TPREL32  tlsvar_ie
+00040428 R_MIPS_TLS_DTPMOD32  \*ABS\*
+00040420 R_MIPS_TLS_DTPMOD32  tlsvar_gd
+00040424 R_MIPS_TLS_DTPREL32  tlsvar_gd
+0004041c R_MIPS_TLS_TPREL32  tlsvar_ie
 
 
 Contents of section .got:
- 40430 00000000 80000000 00000400 00000000  ................
- 40440 00000000 00000000 00000000 00000000  ................
+ 40410 00000000 80000000 000003e0 00000000  ................
+ 40420 00000000 00000000 00000000 00000000  ................
Reply | Threaded
Open this post in threaded view
|

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

Maciej W. Rozycki-2
Hi Simon,

 Thank you for your submission.  Before I actually have a look at your
implementation I have a couple of questions about the problem you have
reported.

> The _gp_disp is a magic symbol, always implicitly defined by the linker.
> It does not make a sense to write it into symbol tables for output files.
> Moreover, now if the linker gets a version script, the _gp_disp symbol
> gets zero version definition index. This symbol is global while the zero
> index means unversioned local symbol. That confuses some tools like for
> example the LLD linker when they get such files as inputs.

 Hmm, LLD being confused may well be a bug in that program.

 The thing is we have been doing this since forever and there were no
issues so far, so obviously anything you observe must be a corner case.  
E.g. I've had a quick look at a shared library I built back in 2001 and it
does have `_gp_disp' in its dynamic symbol table, as an absolute symbol
set to the canonical gp value.  And it is no different with binaries built
nowadays.  So before we move forward, can you please post an actual test
case which reproduces the problem?

 Also this is your first submission to binutils and it is substantial
enough for you to have a copyright assignment or a similar arrangement in
place with FSF before it can be accepted for inclusion.  Do you have one
already?  Please let me know if you need further guidance with that and I
will help you.

 Please ask if you have any questions.

  Maciej
Reply | Threaded
Open this post in threaded view
|

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

Simon Atanasyan
Hi Maciej,

On Tue, Feb 6, 2018 at 11:32 PM, Maciej W. Rozycki <[hidden email]> wrote:

>> The _gp_disp is a magic symbol, always implicitly defined by the linker.
>> It does not make a sense to write it into symbol tables for output files.
>> Moreover, now if the linker gets a version script, the _gp_disp symbol
>> gets zero version definition index. This symbol is global while the zero
>> index means unversioned local symbol. That confuses some tools like for
>> example the LLD linker when they get such files as inputs.
>
>  Hmm, LLD being confused may well be a bug in that program.
>
>  The thing is we have been doing this since forever and there were no
> issues so far, so obviously anything you observe must be a corner case.
> E.g. I've had a quick look at a shared library I built back in 2001 and it
> does have `_gp_disp' in its dynamic symbol table, as an absolute symbol
> set to the canonical gp value.  And it is no different with binaries built
> nowadays.  So before we move forward, can you please post an actual test
> case which reproduces the problem?

Here is a reproduction script for the problem:

$ cat test.s
  .global foo
  .text
foo:
  lui    $t0, %hi(_gp_disp)
  addi   $t0, $t0, %lo(_gp_disp)

$ cat test.ver
LLD_1.0.0 { global: foo; };

$ mips-mti-linux-gnu-as test.s -o test.o
$ mips-mti-linux-gnu-ld -shared test.o -o libtest.so --version-script test.ver
$ mips-mti-linux-gnu-readelf -sV libtest.so

Symbol table '.dynsym' contains 11 entries:
   Num:    Value  Size Type    Bind   Vis      Ndx Name
     0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND
     1: 00000370     0 SECTION LOCAL  DEFAULT    9
     2: 00010380     0 NOTYPE  GLOBAL DEFAULT   10 _fdata
     3: 00018370     0 SECTION GLOBAL DEFAULT  ABS _gp_disp
[...]

Version symbols section '.gnu.version' contains 11 entries:
 Addr: 0000000000000318  Offset: 0x000318  Link: 5 (.dynsym)
  000:   0 (*local*)       0 (*local*)       1 (*global*)      0 (*local*)
[...]

Please note that the .gnu.version's entry related to the _gp_disp
symbol contains zero. Zero means "The symbol is local, not available
outside the object", while _gp_disp has GLOBAL binding. LLD does not
like that difference.

Probably the better solution would be to fix _gp_disp entry in the
.gnu.version and write 1 there. From the other side, I thought that
removing the _gp_disp from symbols table was not too bad idea because
this symbol is useless in a linked file.

I can implement both fixes or one of them.

>  Also this is your first submission to binutils and it is substantial
> enough for you to have a copyright assignment or a similar arrangement in
> place with FSF before it can be accepted for inclusion.  Do you have one
> already?  Please let me know if you need further guidance with that and I
> will help you.

No, I do not have a copyright assignment, so please guide me on this way.

Thanks in advance.

--
Simon Atanasyan
Reply | Threaded
Open this post in threaded view
|

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

Maciej W. Rozycki-2
Hi Simon,

 Apologies for the delay, I have been unloading the pile of various issues
discovered in the course of verifying the recent binutils 2.30 release.

> >  The thing is we have been doing this since forever and there were no
> > issues so far, so obviously anything you observe must be a corner case.
> > E.g. I've had a quick look at a shared library I built back in 2001 and it
> > does have `_gp_disp' in its dynamic symbol table, as an absolute symbol
> > set to the canonical gp value.  And it is no different with binaries built
> > nowadays.  So before we move forward, can you please post an actual test
> > case which reproduces the problem?
>
> Here is a reproduction script for the problem:
>
> $ cat test.s
>   .global foo
>   .text
> foo:
>   lui    $t0, %hi(_gp_disp)
>   addi   $t0, $t0, %lo(_gp_disp)
>
> $ cat test.ver
> LLD_1.0.0 { global: foo; };
>
> $ mips-mti-linux-gnu-as test.s -o test.o
> $ mips-mti-linux-gnu-ld -shared test.o -o libtest.so --version-script test.ver
> $ mips-mti-linux-gnu-readelf -sV libtest.so
>
> Symbol table '.dynsym' contains 11 entries:
>    Num:    Value  Size Type    Bind   Vis      Ndx Name
>      0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND
>      1: 00000370     0 SECTION LOCAL  DEFAULT    9
>      2: 00010380     0 NOTYPE  GLOBAL DEFAULT   10 _fdata
>      3: 00018370     0 SECTION GLOBAL DEFAULT  ABS _gp_disp
> [...]
>
> Version symbols section '.gnu.version' contains 11 entries:
>  Addr: 0000000000000318  Offset: 0x000318  Link: 5 (.dynsym)
>   000:   0 (*local*)       0 (*local*)       1 (*global*)      0 (*local*)
> [...]
>
> Please note that the .gnu.version's entry related to the _gp_disp
> symbol contains zero. Zero means "The symbol is local, not available
> outside the object", while _gp_disp has GLOBAL binding. LLD does not
> like that difference.

 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?

> Probably the better solution would be to fix _gp_disp entry in the
> .gnu.version and write 1 there. From the other side, I thought that
> removing the _gp_disp from symbols table was not too bad idea because
> this symbol is useless in a linked file.
>
> I can implement both fixes or one of them.

 The MIPS psABI is clear on `_gp_disp' use[1]:

"The symbol name _gp_disp is reserved.  Only R_MIPS_HI16 and R_MIPS_LO16
relocations are permitted with _gp_disp.  These relocation entries must
appear consecutively in the relocation section and they must reference
consecutive relocation area addresses."

so obviously a dynamic symbol is useless as you cannot refer to one with
R_MIPS_HI16 or R_MIPS_LO16 and you cannot refer to a symbol other than
with a relocation.

 I was concerned about possible IRIX use as IRIX seems to often diverge
slightly from the published psABI.  However as it turns out the issue of
`_gp_disp' being unnecessarily exported has been already discussed:
<https://sourceware.org/ml/binutils/2005-07/msg00496.html> and that has
cleared my concern.  A suggestion has also been made in the course of that
discussion to discard this symbol in the final link.

 I also considered whether the symbol could be used for anything directly
by the user in GDB.  E.g. in theory, observing the symbol being absolute:

(gdb) print &_gp - &_gp_disp

should calculate the base address.  However `_gp_disp' due to being
defined, oddly enough, as a section symbol, is not exported as a
user-accessible expression by GDB.  So even such a marginal use case is
not possible.

 So I think your change is good to go in once the technical details,
discussed below, have been sorted out and you'll have confirmed that the
copyright assignment has as well.  Please note that may have to work it
around in LLD as well, to cope with existing binaries.

 For the record this special handling for `_gp_disp' has been added with:

commit 5b3b9ff61d3a2a6cdd90fcec1a61d38699f3c608
Author: Ian Lance Taylor <[hidden email]>
Date:   Thu Jan 11 21:06:42 1996 +0000

        * elf32-mips.c: Extensive changes for a start at dynamic linking
        support, from Kazumoto Kojima <[hidden email]>.

which for practical purposes means it's been there since forever.

 Detailed change review follows.

> BFD: Prevent writing the MIPS _gp_disp symbol into symbol tables

 Don't repeat the change heading in the description.

> The _gp_disp is a magic symbol, always implicitly defined by the linker.
> It does not make a sense to write it into symbol tables for output files.
> Moreover, now if the linker gets a version script, the _gp_disp symbol
> gets zero version definition index. This symbol is global while the zero
> index means unversioned local symbol. That confuses some tools like for
> example the LLD linker when they get such files as inputs.

 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.

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

"Please put two spaces after the end of a sentence in your comments, so
that the Emacs sentence commands will work."

> 2018-01-25  Simon Atanasyan  <[hidden email]>

 You have:

From: Simon Atanasyan <[hidden email]>

in the headers however, so which authorship attribution is right for this
submission?

 Whatever is used in the `From:' mail header will be used for authorship
attribution in the commit.  If you want to use a different one, then
please use the first line of the change description (e-mail body) to
specify the correct attribution.  The syntax is the same as with the
`From:' mail header, e.g.:

From: Simon Atanasyan <[hidden email]>

GIT handles this automatically.  If unsure, then try feeding your composed
e-mail to upstream GIT head with `git am' and see if the result is as you
expect before posting.  The address you are going to record the authorship
with has to match the copyright assignment.

> bfd/
>
> * elf32-mips.c: (elf32_mips_fixup_symbol ): New function.

 No space before the closing parenthesis.

> (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:

ld/
        * testsuite/ld-mips-elf/mips16-pic-2.ad: [...]

Likewise throughout.

 Also his will land in a separate ChangeLog file, so please be more
specific in referring to the BFD change, e.g.:

        * testsuite/ld-mips-elf/mips16-pic-2.ad: Update for _gp_disp
        symbol removal.

will do.

> diff --git a/bfd/elf32-mips.c b/bfd/elf32-mips.c
> index fa0cc15aba..5f5b08136f 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 magic _gp_disp symbol from the dynamic symbol table.  */
> +
> +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.

> +    {
> +      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.

> @@ -2598,6 +2614,9 @@ static const struct ecoff_debug_swap
> mips_elf32_ecoff_debug_swap = {
>  #undef elf_backend_write_core_note
>  #define elf_backend_write_core_note elf32_mips_write_core_note
>  
> +#undef elf_backend_fixup_symbol
> +#define elf_backend_fixup_symbol elf32_mips_fixup_symbol

 Please use a tab to align the RHS of the definition to column 40, as with
other definitions.

 Also (assuming that we do want to keep it), as per the discussion above,
we want it across all targets, so group the definition with ones ahead of
the first "elf32-target.h" inclusion.  Do not separate the definition from
the other ones with a new line; group them all together as elsewhere in
this file.

> @@ -2620,6 +2639,7 @@ static const struct ecoff_debug_swap
> mips_elf32_ecoff_debug_swap = {
>  #define elf32_bed elf32_fbsd_tradbed
>  
>  #undef elf_backend_write_core_note
> +#undef elf_backend_fixup_symbol

 This has to go naturally then.

> diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c
> index 285401367d..f387ca47db 100644
> --- a/bfd/elfxx-mips.c
> +++ b/bfd/elfxx-mips.c
> @@ -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.

 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.

 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.

 Please resubmit with the issues discussed above addressed.

  Maciej

References:

[1] "SYSTEM V APPLICATION BINARY INTERFACE, MIPS RISC Processor
    Supplement, 3rd Edition", Section "Relocation Types", pp. 4-20

[2] "The GNU Coding Standards", Section 5.2 "Commenting Your Work"
    <https://www.gnu.org/prep/standards/standards.html#Comments>
Reply | Threaded
Open this post in threaded view
|

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

Simon Atanasyan
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  <[hidden email]>
>
>  You have:
>
> From: Simon Atanasyan <[hidden email]>
>
> in the headers however, so which authorship attribution is right for this
> submission?

I will use [hidden email]. 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.
Reply | Threaded
Open this post in threaded view
|

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

Simon Atanasyan
In reply to this post by Maciej W. Rozycki-2
The _gp_disp is a magic symbol, always implicitly defined by the linker.
It does not make a sense to write it into symbol tables for output files.
Moreover, now if the linker gets a version script, the _gp_disp symbol
gets zero version definition index.  The zero index means[1]:

"The symbol is local, not available outside the object."

But the _gp_disp symbol has GLOBAL binding.  That confuses some tools like
for example the LLD linker when they get such files as inputs.

This patch fixes the problem - it prevents writing the _gp_disp symbol
in regular and dynamic symbol tables.

This was tested by running ls test suite on mipsel-linux board.

References:

[1] "Linux Standard Base Specification", Section "10.7.2 Symbol
    Version Table", p. 32

2018-02-28  Simon Atanasyan  <[hidden email]>

bfd/

        * elf32-mips.c: (elf32_mips_fixup_symbol): New function.
        (elf_backend_fixup_symbol): New macro.
        * elfxx-mips.c: (mips_elf_output_extsym): Discard _gp_disp
        handling.
        (_bfd_mips_elf_finish_dynamic_symbol): Likewise.

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.
        * 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.
        * testsuite/ld-mips-elf/mips16-pic-2.ad: Update for _gp_disp
        symbol removal.
        * testsuite/ld-mips-elf/mips16-pic-2.nd: Likewise.
        * testsuite/ld-mips-elf/pic-and-nonpic-3a.dd: Likewise.
        * testsuite/ld-mips-elf/tlslib-o32-hidden.got: Likewise.
        * testsuite/ld-mips-elf/tlslib-o32-ver.got: Likewise.
        * testsuite/ld-mips-elf/tlslib-o32.got: Likewise.

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);
+    }
+  return TRUE;
+}
 
 /* Depending on the target vector we generate some version of Irix
    executables or "normal" MIPS ELF ABI executables.  */
@@ -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
 
 #define elf_backend_got_header_size (4 * MIPS_RESERVED_GOTNO)
 #define elf_backend_want_dynrelro 1
diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c
index 285401367d..03f997b288 100644
--- a/bfd/elfxx-mips.c
+++ b/bfd/elfxx-mips.c
@@ -2903,12 +2903,6 @@ mips_elf_output_extsym (struct mips_elf_link_hash_entry *h, void *data)
       h->esym.asym.value =
  mips_elf_hash_table (einfo->info)->procedure_count;
     }
-  else if (strcmp (name, "_gp_disp") == 0 && ! NEWABI_P (einfo->abfd))
-    {
-      h->esym.asym.sc = scAbs;
-      h->esym.asym.st = stLabel;
-      h->esym.asym.value = elf_gp (einfo->abfd);
-    }
   else
     h->esym.asym.sc = scUndefined;
  }
@@ -10953,12 +10947,6 @@ _bfd_mips_elf_finish_dynamic_symbol (bfd *output_bfd,
       sym->st_info = ELF_ST_INFO (STB_GLOBAL, STT_SECTION);
       sym->st_value = 1;
     }
-  else if (strcmp (name, "_gp_disp") == 0 && ! NEWABI_P (output_bfd))
-    {
-      sym->st_shndx = SHN_ABS;
-      sym->st_info = ELF_ST_INFO (STB_GLOBAL, STT_SECTION);
-      sym->st_value = elf_gp (output_bfd);
-    }
   else if (SGI_COMPAT (output_bfd))
     {
       if (strcmp (name, mips_elf_dynsym_rtproc_names[0]) == 0
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
+#objdump: -tT
+
+#failif
+#...
+.*_gp_disp
+#...
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
+run_dump_test "gp-disp-sym"
+
 # Export class call relocation tests.
 set abis [concat o32 [expr {$has_newabi ? "n32 n64" : ""}]]
 foreach { abi } $abis {
diff --git a/ld/testsuite/ld-mips-elf/mips16-pic-2.ad b/ld/testsuite/ld-mips-elf/mips16-pic-2.ad
index 52d3ea4c3b..689f0c2557 100644
--- a/ld/testsuite/ld-mips-elf/mips16-pic-2.ad
+++ b/ld/testsuite/ld-mips-elf/mips16-pic-2.ad
@@ -1,6 +1,6 @@
 # [MIPS_GOTSYM, MIPS_SYMTABNO) covers used4...used7.
 #...
- .* \(MIPS_SYMTABNO\) * 8
+ .* \(MIPS_SYMTABNO\) * 7
 #...
- .* \(MIPS_GOTSYM\) * 0x4
+ .* \(MIPS_GOTSYM\) * 0x3
 #pass
diff --git a/ld/testsuite/ld-mips-elf/mips16-pic-2.nd b/ld/testsuite/ld-mips-elf/mips16-pic-2.nd
index bc2cd38ee9..a2a579412f 100644
--- a/ld/testsuite/ld-mips-elf/mips16-pic-2.nd
+++ b/ld/testsuite/ld-mips-elf/mips16-pic-2.nd
@@ -1,9 +1,9 @@
 # used8 should come before MIPS_GOTSYM.
 #...
- +3: 000405bc +36 +FUNC +GLOBAL +DEFAULT .* used8
- +4: 00040574 +36 +FUNC +GLOBAL +DEFAULT .* used6
- +5: 00040598 +36 +FUNC +GLOBAL +DEFAULT .* used7
- +6: 00040550 +36 +FUNC +GLOBAL +DEFAULT .* used5
- +7: 0004052c +36 +FUNC +GLOBAL +DEFAULT .* used4
+ +2: 000405bc +36 +FUNC +GLOBAL +DEFAULT .* used8
+ +3: 00040574 +36 +FUNC +GLOBAL +DEFAULT .* used6
+ +4: 00040598 +36 +FUNC +GLOBAL +DEFAULT .* used7
+ +5: 00040550 +36 +FUNC +GLOBAL +DEFAULT .* used5
+ +6: 0004052c +36 +FUNC +GLOBAL +DEFAULT .* used4
 
 #pass
diff --git a/ld/testsuite/ld-mips-elf/pic-and-nonpic-3a.dd b/ld/testsuite/ld-mips-elf/pic-and-nonpic-3a.dd
index 3dcfe12cfc..b286f131f4 100644
--- a/ld/testsuite/ld-mips-elf/pic-and-nonpic-3a.dd
+++ b/ld/testsuite/ld-mips-elf/pic-and-nonpic-3a.dd
@@ -35,5 +35,5 @@ Disassembly of section \.MIPS\.stubs:
  c00: 8f998010 lw t9,-32752\(gp\)
  c04: 03e07825 move t7,ra
  c08: 0320f809 jalr t9
- c0c: 24180005 li t8,5
+ c0c: 24180004 li t8,4
  \.\.\.
diff --git a/ld/testsuite/ld-mips-elf/tlslib-o32-hidden.got b/ld/testsuite/ld-mips-elf/tlslib-o32-hidden.got
index 563d8bb082..a746031f7e 100644
--- a/ld/testsuite/ld-mips-elf/tlslib-o32-hidden.got
+++ b/ld/testsuite/ld-mips-elf/tlslib-o32-hidden.got
@@ -4,11 +4,11 @@
 DYNAMIC RELOCATION RECORDS
 OFFSET   TYPE              VALUE
 00000000 R_MIPS_NONE       \*ABS\*
-000403bc R_MIPS_TLS_TPREL32  \*ABS\*
-000403c0 R_MIPS_TLS_DTPMOD32  \*ABS\*
-000403c8 R_MIPS_TLS_DTPMOD32  \*ABS\*
+0004039c R_MIPS_TLS_TPREL32  \*ABS\*
+000403a0 R_MIPS_TLS_DTPMOD32  \*ABS\*
+000403a8 R_MIPS_TLS_DTPMOD32  \*ABS\*
 
 
 Contents of section .got:
- 403b0 00000000 80000000 00000380 00000008  ................
- 403c0 00000000 ffff8004 00000000 00000000  ................
+ 40390 00000000 80000000 00000360 00000008  ................
+ 403a0 00000000 ffff8004 00000000 00000000  ................
diff --git a/ld/testsuite/ld-mips-elf/tlslib-o32-ver.got b/ld/testsuite/ld-mips-elf/tlslib-o32-ver.got
index e675f9f64a..17a6385e8e 100644
--- a/ld/testsuite/ld-mips-elf/tlslib-o32-ver.got
+++ b/ld/testsuite/ld-mips-elf/tlslib-o32-ver.got
@@ -4,12 +4,12 @@
 DYNAMIC RELOCATION RECORDS
 OFFSET   TYPE              VALUE
 00000000 R_MIPS_NONE       \*ABS\*
-000404d8 R_MIPS_TLS_DTPMOD32  \*ABS\*
-000404d0 R_MIPS_TLS_DTPMOD32  tlsvar_gd@@VER_1
-000404d4 R_MIPS_TLS_DTPREL32  tlsvar_gd@@VER_1
-000404cc R_MIPS_TLS_TPREL32  tlsvar_ie@@VER_1
+000404b8 R_MIPS_TLS_DTPMOD32  \*ABS\*
+000404b0 R_MIPS_TLS_DTPMOD32  tlsvar_gd@@VER_1
+000404b4 R_MIPS_TLS_DTPREL32  tlsvar_gd@@VER_1
+000404ac R_MIPS_TLS_TPREL32  tlsvar_ie@@VER_1
 
 
 Contents of section .got:
- 404c0 00000000 80000000 00000490 00000000  ................
- 404d0 00000000 00000000 00000000 00000000  ................
+ 404a0 00000000 80000000 00000470 00000000  ................
+ 404b0 00000000 00000000 00000000 00000000  ................
diff --git a/ld/testsuite/ld-mips-elf/tlslib-o32.got b/ld/testsuite/ld-mips-elf/tlslib-o32.got
index ad90fb019e..a389c30146 100644
--- a/ld/testsuite/ld-mips-elf/tlslib-o32.got
+++ b/ld/testsuite/ld-mips-elf/tlslib-o32.got
@@ -4,12 +4,12 @@ tmpdir/tlslib-o32.so:     file format elf32-tradbigmips
 DYNAMIC RELOCATION RECORDS
 OFFSET   TYPE              VALUE
 00000000 R_MIPS_NONE       \*ABS\*
-00040448 R_MIPS_TLS_DTPMOD32  \*ABS\*
-00040440 R_MIPS_TLS_DTPMOD32  tlsvar_gd
-00040444 R_MIPS_TLS_DTPREL32  tlsvar_gd
-0004043c R_MIPS_TLS_TPREL32  tlsvar_ie
+00040428 R_MIPS_TLS_DTPMOD32  \*ABS\*
+00040420 R_MIPS_TLS_DTPMOD32  tlsvar_gd
+00040424 R_MIPS_TLS_DTPREL32  tlsvar_gd
+0004041c R_MIPS_TLS_TPREL32  tlsvar_ie
 
 
 Contents of section .got:
- 40430 00000000 80000000 00000400 00000000  ................
- 40440 00000000 00000000 00000000 00000000  ................
+ 40410 00000000 80000000 000003e0 00000000  ................
+ 40420 00000000 00000000 00000000 00000000  ................
Reply | Threaded
Open this post in threaded view
|

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

Maciej W. Rozycki-2
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