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

classic Classic list List threaded Threaded
3 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