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

Message ID 20180429065756.bvponxkyyqtmdrbg@debian64.galaxy.int
State New
Headers show
Series
  • [v4] BFD: Prevent writing the MIPS _gp_disp symbol into symbol tables
Related show

Commit Message

Simon Atanasyan April 29, 2018, 6:57 a.m.
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 LD test suite on a mipsel-linux board.

References:

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

2018-02-28  Simon Atanasyan  <simon@atanasyan.com>

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/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.
	* 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.

Comments

Maciej W. Rozycki April 30, 2018, 12:21 p.m. | #1
Hi Simon,

 Thank you for the update.  I still have a couple of minor nits about your
new test case, as follows.

> 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..b96355b269

> --- /dev/null

> +++ b/ld/testsuite/ld-mips-elf/gp-disp-sym.d

> @@ -0,0 +1,12 @@

> +#name: Do not include _gp_disp in symbol tables


 You need to make it clear that this is a MIPS-specific test case, so that 
people who look through test results know it right away.  Also I prefer 
names (noun phrases) rather than complete phrases for test case names.

> +#as: -mips32 -32


 As I noted before `-mips32' needs to be dropped.  Also I missed the need 
to pass `$abi_asflags(o32)' in test invocation (so that the endianness 
matches one specified for LD with `$abi_ldflags(o32)'), which will include 
`-32' already, meaning that this option can go altogether.

> +#ld: -shared

> +#objdump: -tT

> +#target: [check_shared_lib_support]

> +

> +#failif

> +#...

> +.*_gp_disp

> +#...

> +

> +#pass


 There's no need for `#...' ahead of `#pass' as the latter will already 
cause anything that appears after `.*_gp_disp' to be discarded.  Sorry to 
be unclear in the previous round.

> diff --git a/ld/testsuite/ld-mips-elf/mips-elf.exp b/ld/testsuite/ld-mips-elf/mips-elf.exp

> index 95d677e577..565fee74ec 100644

> --- a/ld/testsuite/ld-mips-elf/mips-elf.exp

> +++ b/ld/testsuite/ld-mips-elf/mips-elf.exp

> @@ -1255,3 +1255,6 @@ run_dump_test "mips-abiflags-1"

>  run_dump_test "mips-abiflags-1r"

>  run_dump_test "mips-abiflags-2"

>  run_dump_test "mips-abiflags-2r"

> +

> +# Test that _gp_disp symbol is not present in symbol tables.

> +run_dump_test "gp-disp-sym" [list [list ld $abi_ldflags(o32)]]


 As I noted above `$abi_asflags(o32)' have to be passed here as well.  
Ideally that would happen automagically, but that would require a larger 
MIPS test framework rewrite.

 To speed up upstreaming your change here's an update patch I have made to 
address my concerns that I will fold into your change and push in the next 
couple of days.  Please let me know if you disagree with any of the 
changes I propose.

  Maciej

---
 ld/testsuite/ld-mips-elf/gp-disp-sym.d |    5 +----
 ld/testsuite/ld-mips-elf/mips-elf.exp  |    3 ++-
 2 files changed, 3 insertions(+), 5 deletions(-)

binutils-mips-gp-disp-remove-update.diff
Index: binutils/ld/testsuite/ld-mips-elf/gp-disp-sym.d
===================================================================
--- binutils.orig/ld/testsuite/ld-mips-elf/gp-disp-sym.d	2018-04-30 09:37:34.000000000 +0100
+++ binutils/ld/testsuite/ld-mips-elf/gp-disp-sym.d	2018-04-30 09:37:01.303274000 +0100
@@ -1,5 +1,4 @@
-#name: Do not include _gp_disp in symbol tables
-#as: -mips32 -32
+#name: MIPS _gp_disp removal from symbol tables
 #ld: -shared
 #objdump: -tT
 #target: [check_shared_lib_support]
@@ -7,6 +6,4 @@
 #failif
 #...
 .*_gp_disp
-#...
-
 #pass
Index: binutils/ld/testsuite/ld-mips-elf/mips-elf.exp
===================================================================
--- binutils.orig/ld/testsuite/ld-mips-elf/mips-elf.exp	2018-04-30 09:37:34.000000000 +0100
+++ binutils/ld/testsuite/ld-mips-elf/mips-elf.exp	2018-04-30 09:37:38.714999847 +0100
@@ -1257,4 +1257,5 @@ run_dump_test "mips-abiflags-2"
 run_dump_test "mips-abiflags-2r"
 
 # Test that _gp_disp symbol is not present in symbol tables.
-run_dump_test "gp-disp-sym" [list [list ld $abi_ldflags(o32)]]
+run_dump_test "gp-disp-sym" [list [list as $abi_asflags(o32)] \
+				  [list ld $abi_ldflags(o32)]]
Joseph Myers April 30, 2018, 3:21 p.m. | #2
Does this affect the glibc testsuite (which currently has _gp_disp in all 
the *.abilist expectations for 32-bit MIPS)?  If it does, do you have a 
suggestion for how to keep it passing with both old and new linkers (e.g. 
ignoring absolute symbols in the ABI tests)?

-- 
Joseph S. Myers
joseph@codesourcery.com
Maciej W. Rozycki April 30, 2018, 4:26 p.m. | #3
Hi Joseph,

> Does this affect the glibc testsuite (which currently has _gp_disp in all 

> the *.abilist expectations for 32-bit MIPS)?  If it does, do you have a 

> suggestion for how to keep it passing with both old and new linkers (e.g. 

> ignoring absolute symbols in the ABI tests)?


 Thanks for raising this concern.

 The presence of a dynamic `_gp_disp' symbol is I think a grey area of the 
MIPS psABI.  This is what it has to say about this symbol[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."

By inference this means no dynamic references are allowed to `_gp_disp', 
because the R_MIPS_HI16 and R_MIPS_LO16 relocations must fully resolve at 
static link time.  Therefore while the presence of `_gp_disp' in the 
dynamic symbol table is not explicitly disallowed by the MIPS psABI, it 
serves no run-time purpose either, and certainly none defined the the ABI.  
Consequently I think the symbol should be considered not a part of the 
ABI.

 Therefore I think we should selectively ignore it.  What you write 
implies we have no predefined way of doing that, and it looks to me like 
we ought to define a way to feed `scripts/abilist.awk' a (carefully 
selected) list of symbols to ignore in processing.

 Alternatively (and that looks to me like a better way to deal with this 
symbol, as it will actually fix non-compliant dynamic binaries produced by 
unfixed binutils) we could run `strip --strip-symbol=_gp_disp' after link.  
I'm not sure how feasible implementing this solution would be though and 
then this should probably be wired into the link stage somehow for o32 
MIPS targets only, that is with `sysdeps/mips/mips32/configure.ac', so as 
not to burden irrelevant configurations.

References:

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

  Maciej
Joseph Myers April 30, 2018, 7:38 p.m. | #4
On Mon, 30 Apr 2018, Maciej W. Rozycki wrote:

>  Therefore I think we should selectively ignore it.  What you write 

> implies we have no predefined way of doing that, and it looks to me like 

> we ought to define a way to feed `scripts/abilist.awk' a (carefully 

> selected) list of symbols to ignore in processing.


But do we need such a list, or would just ignoring all absolute symbols 
suffice?

At present, abilist.awk shows such symbols with a type of "A".  Apart from 
symbol version names, _gp_disp seems to be the only such symbol.  Is there 
any need to have the "A" lines for each symbol version in the abilist 
files?

-- 
Joseph S. Myers
joseph@codesourcery.com
Maciej W. Rozycki May 1, 2018, 11:54 a.m. | #5
On Mon, 30 Apr 2018, Joseph Myers wrote:

> >  Therefore I think we should selectively ignore it.  What you write 

> > implies we have no predefined way of doing that, and it looks to me like 

> > we ought to define a way to feed `scripts/abilist.awk' a (carefully 

> > selected) list of symbols to ignore in processing.

> 

> But do we need such a list, or would just ignoring all absolute symbols 

> suffice?

> 

> At present, abilist.awk shows such symbols with a type of "A".  Apart from 

> symbol version names, _gp_disp seems to be the only such symbol.  Is there 

> any need to have the "A" lines for each symbol version in the abilist 

> files?


 I don't know.  Perhaps someone else knows.

 As I noted stripping the problematic symbol is IMO the best solution.  
Using `$CC -print-prog-name=strip' to discover where the right `strip' 
binary is should be a straightforward way to arrange that in a platform 
`configure' fragment.  Then we can dump `_gp_disp' from .abilist files.

  Maciej
Simon Atanasyan May 2, 2018, 6:49 a.m. | #6
Hi Maciej,

I'm sorry for delay with the reply.

On Mon, Apr 30, 2018 at 3:21 PM, Maciej W. Rozycki <macro@mips.com> wrote:
>  Thank you for the update.  I still have a couple of minor nits about your

> new test case, as follows.


[...]

>  To speed up upstreaming your change here's an update patch I have made to

> address my concerns that I will fold into your change and push in the next

> couple of days.  Please let me know if you disagree with any of the

> changes I propose.


I agree with all the changes.  The patch becomes really better.  Thank you
for all notes and suggestions.  Feel free to push it upstream.

-- 
Simon Atanasyan
Maciej W. Rozycki May 2, 2018, 3:48 p.m. | #7
On Tue, 1 May 2018, Joseph Myers wrote:

> Does strip even work to remove symbols from the dynamic symbol table (as 

> opposed to the static symbol table)?


 Indeed, `strip' seems unable to remove dynamic symbol table entries, so I 
need to withdraw my alternative proposal.

 As to filtering out all absolute symbols as I noted I don't know what 
impact it may have across current targets and whether it may cause hassle 
in the future, so I have no opinion either.  If you think it's safe, then 
I won't object.

  Maciej
Maciej W. Rozycki May 3, 2018, 4:21 p.m. | #8
Hi Simon,

> >  To speed up upstreaming your change here's an update patch I have made to

> > address my concerns that I will fold into your change and push in the next

> > couple of days.  Please let me know if you disagree with any of the

> > changes I propose.

> 

> I agree with all the changes.  The patch becomes really better.  Thank you

> for all notes and suggestions.  Feel free to push it upstream.


 I have committed your change now with the update included, and having 
fixed the description in a couple of places to follow the requirement to 
have two spaces between a full stop and the next sentence.

 Thank you for your contribution and your patience through the review.

  Maciej

Patch

diff --git a/bfd/elf32-mips.c b/bfd/elf32-mips.c
index 87147b5c4f..23a5712d46 100644
--- a/bfd/elf32-mips.c
+++ b/bfd/elf32-mips.c
@@ -2420,6 +2420,17 @@  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)
+{
+  if (strcmp (h->root.root.string, "_gp_disp") == 0)
+    _bfd_elf_link_hash_hide_symbol (info, h, TRUE);
+  return TRUE;
+}
 
 /* Depending on the target vector we generate some version of Irix
    executables or "normal" MIPS ELF ABI executables.  */
@@ -2523,6 +2534,7 @@  static const struct ecoff_debug_swap mips_elf32_ecoff_debug_swap = {
 #define elf_backend_gc_mark_hook	_bfd_mips_elf_gc_mark_hook
 #define elf_backend_copy_indirect_symbol \
 					_bfd_mips_elf_copy_indirect_symbol
+#define elf_backend_fixup_symbol	elf32_mips_fixup_symbol
 #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
diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c
index ce645817bb..e349e8aa77 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;
 	}
@@ -10976,12 +10970,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..b96355b269
--- /dev/null
+++ b/ld/testsuite/ld-mips-elf/gp-disp-sym.d
@@ -0,0 +1,12 @@ 
+#name: Do not include _gp_disp in symbol tables
+#as: -mips32 -32
+#ld: -shared
+#objdump: -tT
+#target: [check_shared_lib_support]
+
+#failif
+#...
+.*_gp_disp
+#...
+
+#pass
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 95d677e577..565fee74ec 100644
--- a/ld/testsuite/ld-mips-elf/mips-elf.exp
+++ b/ld/testsuite/ld-mips-elf/mips-elf.exp
@@ -1255,3 +1255,6 @@  run_dump_test "mips-abiflags-1"
 run_dump_test "mips-abiflags-1r"
 run_dump_test "mips-abiflags-2"
 run_dump_test "mips-abiflags-2r"
+
+# Test that _gp_disp symbol is not present in symbol tables.
+run_dump_test "gp-disp-sym" [list [list ld $abi_ldflags(o32)]]
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  ................