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

["Maciej W. Rozycki" <macro at mips dot com>]

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)]]