* [PATCH] VAX/BFD: Omit PLT slots for local symbols
@ 2012-05-07 23:30 Maciej W. Rozycki
2012-05-08 4:12 ` Hans-Peter Nilsson
0 siblings, 1 reply; 6+ messages in thread
From: Maciej W. Rozycki @ 2012-05-07 23:30 UTC (permalink / raw)
To: binutils
Hi,
The change below fixes a problem with the creation of the PLT on the VAX
target, where entries are made where they should not for local symbols,
leading to unneeded or even empty slots. The conditions for the latters
are a bit obscure, for such a slot to be created a dynamic symbol has to
be preempted in the link with a static hidden symbol referred to by a PLT
relocation (i.e. from a PIC relocatable). This is covered by the test
case provided. Executables with empty PLT slots are invalid and rejected
by the dynamic linker.
There is some code duplication in the area affected, so I have merged the
two pieces (that deal with local and garbage-collected symbols,
respectively) into one, similarly to how some other targets do it. Also
the attempt to make a symbol whose h->dynindx is -1 dynamic is incorrect
here -- such symbols will have qualified as local in SYMBOL_CALLS_LOCAL
and shouldn't be produced.
No regressions with this change. OK to apply?
2012-05-08 Maciej W. Rozycki <macro@linux-mips.org>
bfd/
* elf32-vax.c (elf_vax_adjust_dynamic_symbol): Don't allocate
PLT slots for local symbols.
ld/testsuite/
* ld-vax-elf/plt-local-lib.dd: New test.
* ld-vax-elf/plt-local-lib.ld: New test linker script.
* ld-vax-elf/plt-local-lib.s: New test source.
* ld-vax-elf/plt-local.dd: New test.
* ld-vax-elf/plt-local.ld:New test linker script.
* ld-vax-elf/plt-local.s: New test source.
* ld-vax-elf/plt-local-hidden-pic.s: New test source.
* ld-vax-elf/plt-local-rehidden-pic.s: New test source.
* ld-vax-elf/vax-elf.exp: New test script.
Maciej
binutils-vax-plt-local.patch
Index: binutils/bfd/elf32-vax.c
===================================================================
--- binutils.orig/bfd/elf32-vax.c
+++ binutils/bfd/elf32-vax.c
@@ -934,39 +934,21 @@ elf_vax_adjust_dynamic_symbol (info, h)
if (h->type == STT_FUNC
|| h->needs_plt)
{
- if (! info->shared
- && !h->def_dynamic
- && !h->ref_dynamic
- /* We must always create the plt entry if it was referenced
- by a PLTxxO relocation. In this case we already recorded
- it as a dynamic symbol. */
- && h->dynindx == -1)
+ if (h->plt.refcount <= 0
+ || SYMBOL_CALLS_LOCAL (info, h)
+ || (ELF_ST_VISIBILITY (h->other) != STV_DEFAULT
+ && h->root.type == bfd_link_hash_undefweak))
{
/* This case can occur if we saw a PLTxx reloc in an input
file, but the symbol was never referred to by a dynamic
- object. In such a case, we don't actually need to build
- a procedure linkage table, and we can just do a PCxx
- reloc instead. */
- BFD_ASSERT (h->needs_plt);
+ object, or if all references were garbage collected. In
+ such a case, we don't actually need to build a procedure
+ linkage table, and we can just do a PCxx reloc instead. */
h->plt.offset = (bfd_vma) -1;
- return TRUE;
- }
-
- /* GC may have rendered this entry unused. */
- if (h->plt.refcount <= 0)
- {
h->needs_plt = 0;
- h->plt.offset = (bfd_vma) -1;
return TRUE;
}
- /* Make sure this symbol is output as a dynamic symbol. */
- if (h->dynindx == -1)
- {
- if (! bfd_elf_link_record_dynamic_symbol (info, h))
- return FALSE;
- }
-
s = bfd_get_section_by_name (dynobj, ".plt");
BFD_ASSERT (s != NULL);
Index: binutils/ld/testsuite/ld-vax-elf/plt-local-hidden-pic.s
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/plt-local-hidden-pic.s
@@ -0,0 +1,14 @@
+ .text
+
+ .hidden foo_hidden
+ .globl foo_hidden
+ .type foo_hidden, @function
+foo_hidden:
+ .word 0
+ calls $0, foo_extern
+ calls $0, foo_global
+ calls $0, foo_local
+ calls $0, foo_hidden
+ calls $0, foo_rehidden
+ ret
+ .size foo_hidden, . - foo_hidden
Index: binutils/ld/testsuite/ld-vax-elf/plt-local-lib.dd
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/plt-local-lib.dd
@@ -0,0 +1,91 @@
+.*: +file format .*vax.*
+
+Disassembly of section \.plt:
+
+00001000 <foo_local@plt-0xc>:
+ 1000: dd ef 76 20 pushl 307c <_GLOBAL_OFFSET_TABLE_\+0x4>
+ 1004: 00 00
+ 1006: 17 ff 74 20 jmp \*3080 <_GLOBAL_OFFSET_TABLE_\+0x8>
+ 100a: 00 00
+
+0000100c <foo_local@plt>:
+ 100c: fc 0f \.word 0x0ffc # Entry mask: < r11 r10 r9 r8 r7 r6 r5 r4 r3 r2 >
+ 100e: 16 ef ec ff jsb 1000 <foo_local@plt-0xc>
+ 1012: ff ff
+ 1014: 00 00 00 00 \.long 0x00000000
+
+00001018 <foo_extern@plt>:
+ 1018: fc 0f \.word 0x0ffc # Entry mask: < r11 r10 r9 r8 r7 r6 r5 r4 r3 r2 >
+ 101a: 16 ef e0 ff jsb 1000 <foo_local@plt-0xc>
+ 101e: ff ff
+ 1020: 0c 00 00 00 \.long 0x0000000c
+
+00001024 <foo_rehidden@plt>:
+ 1024: fc 0f \.word 0x0ffc # Entry mask: < r11 r10 r9 r8 r7 r6 r5 r4 r3 r2 >
+ 1026: 16 ef d4 ff jsb 1000 <foo_local@plt-0xc>
+ 102a: ff ff
+ 102c: 18 00 00 00 \.long 0x00000018
+
+00001030 <foo_global@plt>:
+ 1030: fc 0f \.word 0x0ffc # Entry mask: < r11 r10 r9 r8 r7 r6 r5 r4 r3 r2 >
+ 1032: 16 ef c8 ff jsb 1000 <foo_local@plt-0xc>
+ 1036: ff ff
+ 1038: 24 00 00 00 \.long 0x00000024
+
+Disassembly of section \.text:
+
+00002000 <foo_extern>:
+ 2000: 00 00 \.word 0x0000 # Entry mask: < >
+ 2002: fb 00 ff 7f calls \$0x0,\*3088 <_GLOBAL_OFFSET_TABLE_\+0x10>
+ 2006: 10 00 00
+ 2009: fb 00 ff 80 calls \$0x0,\*3090 <_GLOBAL_OFFSET_TABLE_\+0x18>
+ 200d: 10 00 00
+ 2010: fb 00 ff 6d calls \$0x0,\*3084 <_GLOBAL_OFFSET_TABLE_\+0xc>
+ 2014: 10 00 00
+ 2017: fb 00 ef 2e calls \$0x0,204c <foo_hidden>
+ 201b: 00 00 00
+ 201e: fb 00 ff 67 calls \$0x0,\*308c <_GLOBAL_OFFSET_TABLE_\+0x14>
+ 2022: 10 00 00
+ 2025: 04 ret
+
+00002026 <foo_local>:
+ 2026: 00 00 \.word 0x0000 # Entry mask: < >
+ 2028: fb 00 ff 59 calls \$0x0,\*3088 <_GLOBAL_OFFSET_TABLE_\+0x10>
+ 202c: 10 00 00
+ 202f: fb 00 ff 5a calls \$0x0,\*3090 <_GLOBAL_OFFSET_TABLE_\+0x18>
+ 2033: 10 00 00
+ 2036: fb 00 ff 47 calls \$0x0,\*3084 <_GLOBAL_OFFSET_TABLE_\+0xc>
+ 203a: 10 00 00
+ 203d: fb 00 ef 08 calls \$0x0,204c <foo_hidden>
+ 2041: 00 00 00
+ 2044: fb 00 ff 41 calls \$0x0,\*308c <_GLOBAL_OFFSET_TABLE_\+0x14>
+ 2048: 10 00 00
+ 204b: 04 ret
+
+0000204c <foo_hidden>:
+ 204c: 00 00 \.word 0x0000 # Entry mask: < >
+ 204e: fb 00 ff 33 calls \$0x0,\*3088 <_GLOBAL_OFFSET_TABLE_\+0x10>
+ 2052: 10 00 00
+ 2055: fb 00 ff 34 calls \$0x0,\*3090 <_GLOBAL_OFFSET_TABLE_\+0x18>
+ 2059: 10 00 00
+ 205c: fb 00 ff 21 calls \$0x0,\*3084 <_GLOBAL_OFFSET_TABLE_\+0xc>
+ 2060: 10 00 00
+ 2063: fb 00 ef e2 calls \$0x0,204c <foo_hidden>
+ 2067: ff ff ff
+ 206a: fb 00 ff 1b calls \$0x0,\*308c <_GLOBAL_OFFSET_TABLE_\+0x14>
+ 206e: 10 00 00
+ 2071: 04 ret
+
+00002072 <foo_rehidden>:
+ 2072: 00 00 \.word 0x0000 # Entry mask: < >
+ 2074: fb 00 ff 0d calls \$0x0,\*3088 <_GLOBAL_OFFSET_TABLE_\+0x10>
+ 2078: 10 00 00
+ 207b: fb 00 ff 0e calls \$0x0,\*3090 <_GLOBAL_OFFSET_TABLE_\+0x18>
+ 207f: 10 00 00
+ 2082: fb 00 ff fb calls \$0x0,\*3084 <_GLOBAL_OFFSET_TABLE_\+0xc>
+ 2086: 0f 00 00
+ 2089: fb 00 ef bc calls \$0x0,204c <foo_hidden>
+ 208d: ff ff ff
+ 2090: fb 00 ff f5 calls \$0x0,\*308c <_GLOBAL_OFFSET_TABLE_\+0x14>
+ 2094: 0f 00 00
+ 2097: 04 ret
Index: binutils/ld/testsuite/ld-vax-elf/plt-local-lib.ld
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/plt-local-lib.ld
@@ -0,0 +1,18 @@
+SECTIONS
+{
+ . = 0;
+ .hash : { *(.hash) }
+ .dynsym : { *(.dynsym) }
+ .dynstr : { *(.dynstr) }
+ .rela.plt : { *(.rela.plt) }
+
+ . = 0x1000;
+ .plt : { *(.plt) }
+
+ . = 0x2000;
+ .text : { *(.text) }
+
+ . = 0x3000;
+ .dynamic : { *(.dynamic) }
+ .got : { *(.got.plt) }
+}
Index: binutils/ld/testsuite/ld-vax-elf/plt-local-lib.s
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/plt-local-lib.s
@@ -0,0 +1,50 @@
+ .text
+
+ .globl foo_extern
+ .type foo_extern, @function
+foo_extern:
+ .word 0
+ calls $0, foo_extern
+ calls $0, foo_global
+ calls $0, foo_local
+ calls $0, foo_hidden
+ calls $0, foo_rehidden
+ ret
+ .size foo_extern, . - foo_extern
+
+ .globl foo_local
+ .type foo_local, @function
+foo_local:
+ .word 0
+ calls $0, foo_extern
+ calls $0, foo_global
+ calls $0, foo_local
+ calls $0, foo_hidden
+ calls $0, foo_rehidden
+ ret
+ .size foo_local, . - foo_local
+
+ .hidden foo_hidden
+ .globl foo_hidden
+ .type foo_hidden, @function
+foo_hidden:
+ .word 0
+ calls $0, foo_extern
+ calls $0, foo_global
+ calls $0, foo_local
+ calls $0, foo_hidden
+ calls $0, foo_rehidden
+ ret
+ .size foo_hidden, . - foo_hidden
+
+ .globl foo_rehidden
+ .type foo_rehidden, @function
+foo_rehidden:
+ .word 0
+ calls $0, foo_extern
+ calls $0, foo_global
+ calls $0, foo_local
+ calls $0, foo_hidden
+ calls $0, foo_rehidden
+ ret
+ .size foo_rehidden, . - foo_rehidden
Index: binutils/ld/testsuite/ld-vax-elf/plt-local-rehidden-pic.s
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/plt-local-rehidden-pic.s
@@ -0,0 +1,14 @@
+ .text
+
+ .hidden foo_rehidden
+ .globl foo_rehidden
+ .type foo_rehidden, @function
+foo_rehidden:
+ .word 0
+ calls $0, foo_extern
+ calls $0, foo_global
+ calls $0, foo_local
+ calls $0, foo_hidden
+ calls $0, foo_rehidden
+ ret
+ .size foo_rehidden, . - foo_rehidden
Index: binutils/ld/testsuite/ld-vax-elf/plt-local.dd
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/plt-local.dd
@@ -0,0 +1,73 @@
+.*: +file format .*vax.*
+
+Disassembly of section \.plt:
+
+00001000 <foo_extern@plt-0xc>:
+ 1000: dd ef 86 20 pushl 308c <_GLOBAL_OFFSET_TABLE_\+0x4>
+ 1004: 00 00
+ 1006: 17 ff 84 20 jmp \*3090 <_GLOBAL_OFFSET_TABLE_\+0x8>
+ 100a: 00 00
+
+0000100c <foo_extern@plt>:
+ 100c: fc 0f \.word 0x0ffc # Entry mask: < r11 r10 r9 r8 r7 r6 r5 r4 r3 r2 >
+ 100e: 16 ef ec ff jsb 1000 <foo_extern@plt-0xc>
+ 1012: ff ff
+ 1014: 00 00 00 00 \.long 0x00000000
+
+Disassembly of section \.text:
+
+00002000 <foo_hidden>:
+ 2000: 00 00 \.word 0x0000 # Entry mask: < >
+ 2002: fb 00 ff 8b calls \$0x0,\*3094 <_GLOBAL_OFFSET_TABLE_\+0xc>
+ 2006: 10 00 00
+ 2009: fb 00 ef 3c calls \$0x0,204c <foo_global>
+ 200d: 00 00 00
+ 2010: fb 00 ef 5b calls \$0x0,2072 <foo_local>
+ 2014: 00 00 00
+ 2017: fb 00 ef e2 calls \$0x0,2000 <foo_hidden>
+ 201b: ff ff ff
+ 201e: fb 00 ef 01 calls \$0x0,2026 <foo_rehidden>
+ 2022: 00 00 00
+ 2025: 04 ret
+
+00002026 <foo_rehidden>:
+ 2026: 00 00 \.word 0x0000 # Entry mask: < >
+ 2028: fb 00 ff 65 calls \$0x0,\*3094 <_GLOBAL_OFFSET_TABLE_\+0xc>
+ 202c: 10 00 00
+ 202f: fb 00 ef 16 calls \$0x0,204c <foo_global>
+ 2033: 00 00 00
+ 2036: fb 00 ef 35 calls \$0x0,2072 <foo_local>
+ 203a: 00 00 00
+ 203d: fb 00 ef bc calls \$0x0,2000 <foo_hidden>
+ 2041: ff ff ff
+ 2044: fb 00 ef db calls \$0x0,2026 <foo_rehidden>
+ 2048: ff ff ff
+ 204b: 04 ret
+
+0000204c <foo_global>:
+ 204c: 00 00 \.word 0x0000 # Entry mask: < >
+ 204e: fb 00 ff 3f calls \$0x0,\*3094 <_GLOBAL_OFFSET_TABLE_\+0xc>
+ 2052: 10 00 00
+ 2055: fb 00 ef f0 calls \$0x0,204c <foo_global>
+ 2059: ff ff ff
+ 205c: fb 00 ef 0f calls \$0x0,2072 <foo_local>
+ 2060: 00 00 00
+ 2063: fb 00 ef 96 calls \$0x0,2000 <foo_hidden>
+ 2067: ff ff ff
+ 206a: fb 00 ef b5 calls \$0x0,2026 <foo_rehidden>
+ 206e: ff ff ff
+ 2071: 04 ret
+
+00002072 <foo_local>:
+ 2072: 00 00 \.word 0x0000 # Entry mask: < >
+ 2074: fb 00 ff 19 calls \$0x0,\*3094 <_GLOBAL_OFFSET_TABLE_\+0xc>
+ 2078: 10 00 00
+ 207b: fb 00 ef ca calls \$0x0,204c <foo_global>
+ 207f: ff ff ff
+ 2082: fb 00 ef e9 calls \$0x0,2072 <foo_local>
+ 2086: ff ff ff
+ 2089: fb 00 ef 70 calls \$0x0,2000 <foo_hidden>
+ 208d: ff ff ff
+ 2090: fb 00 ef 8f calls \$0x0,2026 <foo_rehidden>
+ 2094: ff ff ff
+ 2097: 04 ret
Index: binutils/ld/testsuite/ld-vax-elf/plt-local.ld
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/plt-local.ld
@@ -0,0 +1,34 @@
+ENTRY (foo_global)
+SECTIONS
+{
+ . = 0;
+ .interp : { *(.interp) }
+ .hash : { *(.hash) }
+ .dynsym : { *(.dynsym) }
+ .dynstr : { *(.dynstr) }
+ .gnu.version : { *(.gnu.version) }
+ .gnu.version_d : { *(.gnu.version_d) }
+ .rela.plt : { *(.rela.plt) }
+
+ . = 0x1000;
+ .plt : { *(.plt) }
+
+ . = 0x2000;
+ .text : { *(.text) }
+
+ . = 0x3000;
+ .dynamic : { *(.dynamic) }
+ .got : { *(.got.plt) }
+};
+VERSION
+{
+ {
+ global:
+ foo_extern;
+ foo_global;
+ foo_hidden;
+ foo_rehidden;
+ local:
+ foo_local;
+ };
+}
Index: binutils/ld/testsuite/ld-vax-elf/plt-local.s
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/plt-local.s
@@ -0,0 +1,25 @@
+ .text
+
+ .globl foo_global
+ .type foo_global, @function
+foo_global:
+ .word 0
+ calls $0, foo_extern
+ calls $0, foo_global
+ calls $0, foo_local
+ calls $0, foo_hidden
+ calls $0, foo_rehidden
+ ret
+ .size foo_global, . - foo_global
+
+ .globl foo_local
+ .type foo_local, @function
+foo_local:
+ .word 0
+ calls $0, foo_extern
+ calls $0, foo_global
+ calls $0, foo_local
+ calls $0, foo_hidden
+ calls $0, foo_rehidden
+ ret
+ .size foo_local, . - foo_local
Index: binutils/ld/testsuite/ld-vax-elf/vax-elf.exp
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/vax-elf.exp
@@ -0,0 +1,50 @@
+# Expect script for VAX ELF linker tests
+# Copyright 2012 Free Software Foundation, Inc.
+#
+# This file is part of the GNU Binutils.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
+# MA 02110-1301, USA.
+#
+
+if { ![istarget vax-*-*] || ![is_elf_format] } {
+ return
+}
+
+run_ld_link_tests [list \
+ [list "PLT test (shared library)" \
+ "-shared -T plt-local-lib.ld" \
+ "-k" \
+ { plt-local-lib.s } \
+ { { objdump -d plt-local-lib.dd } } \
+ "plt-local-lib.so"] \
+ [list "PLT test (object 1)" \
+ "-r" \
+ "-k" \
+ { plt-local-hidden-pic.s } \
+ {} \
+ "plt-local-hidden-pic-r.o"] \
+ [list "PLT test (object 2)" \
+ "-r" \
+ "-k" \
+ { plt-local-rehidden-pic.s } \
+ {} \
+ "plt-local-rehidden-pic-r.o"] \
+ [list "PLT test (executable)" \
+ "-T plt-local.ld tmpdir/plt-local-hidden-pic-r.o tmpdir/plt-local-rehidden-pic-r.o tmpdir/plt-local-lib.so" \
+ "" \
+ { plt-local.s } \
+ { { objdump -d plt-local.dd } } \
+ "plt-local"]]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] VAX/BFD: Omit PLT slots for local symbols
2012-05-07 23:30 [PATCH] VAX/BFD: Omit PLT slots for local symbols Maciej W. Rozycki
@ 2012-05-08 4:12 ` Hans-Peter Nilsson
2012-07-11 0:04 ` Maciej W. Rozycki
0 siblings, 1 reply; 6+ messages in thread
From: Hans-Peter Nilsson @ 2012-05-08 4:12 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: binutils
(Not a maintainer-status review.)
On Tue, 8 May 2012, Maciej W. Rozycki wrote:
> ld/testsuite/
> * ld-vax-elf/plt-local-lib.dd: New test.
> * ld-vax-elf/plt-local-lib.ld: New test linker script.
> * ld-vax-elf/plt-local-lib.s: New test source.
> * ld-vax-elf/plt-local.dd: New test.
> * ld-vax-elf/plt-local.ld:New test linker script.
> * ld-vax-elf/plt-local.s: New test source.
> * ld-vax-elf/plt-local-hidden-pic.s: New test source.
> * ld-vax-elf/plt-local-rehidden-pic.s: New test source.
> * ld-vax-elf/vax-elf.exp: New test script.
> Index: binutils/ld/testsuite/ld-vax-elf/vax-elf.exp
> ===================================================================
> --- /dev/null
> +++ binutils/ld/testsuite/ld-vax-elf/vax-elf.exp
> @@ -0,0 +1,50 @@
> +# Expect script for VAX ELF linker tests
...
> +
> +run_ld_link_tests [list \
> + [list "PLT test (shared library)" \
> + "-shared -T plt-local-lib.ld" \
> + "-k" \
> + { plt-local-lib.s } \
> + { { objdump -d plt-local-lib.dd } } \
> + "plt-local-lib.so"] \
> + [list "PLT test (object 1)" \
> + "-r" \
... (pruned for brevity)
Ouch, another one of those tables-of-lists-of-tables and in a
brand-new .exp.
Let me suggest instead using a mechanism such as run_dump_test
with depended parts (DSO's linked against) sorted before others.
This'd give easy drop-in of new tests without having to add to a
separate table entry somewhere. See ld-cris/cris.exp. Similar
new test: ld-arm/gc-hidden-1.d (unfortunately there's no
file-name iterator in arm-elf.exp, entries are still added
manually).
brgds, H-P
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] VAX/BFD: Omit PLT slots for local symbols
2012-05-08 4:12 ` Hans-Peter Nilsson
@ 2012-07-11 0:04 ` Maciej W. Rozycki
2012-07-11 3:29 ` Hans-Peter Nilsson
2012-07-15 21:57 ` Jan-Benedict Glaw
0 siblings, 2 replies; 6+ messages in thread
From: Maciej W. Rozycki @ 2012-07-11 0:04 UTC (permalink / raw)
To: Hans-Peter Nilsson; +Cc: Jan-Benedict Glaw, binutils
On Tue, 8 May 2012, Hans-Peter Nilsson wrote:
> > ld/testsuite/
> > * ld-vax-elf/plt-local-lib.dd: New test.
> > * ld-vax-elf/plt-local-lib.ld: New test linker script.
> > * ld-vax-elf/plt-local-lib.s: New test source.
> > * ld-vax-elf/plt-local.dd: New test.
> > * ld-vax-elf/plt-local.ld:New test linker script.
> > * ld-vax-elf/plt-local.s: New test source.
> > * ld-vax-elf/plt-local-hidden-pic.s: New test source.
> > * ld-vax-elf/plt-local-rehidden-pic.s: New test source.
> > * ld-vax-elf/vax-elf.exp: New test script.
>
> > Index: binutils/ld/testsuite/ld-vax-elf/vax-elf.exp
> > ===================================================================
> > --- /dev/null
> > +++ binutils/ld/testsuite/ld-vax-elf/vax-elf.exp
> > @@ -0,0 +1,50 @@
> > +# Expect script for VAX ELF linker tests
>
> ...
> > +
> > +run_ld_link_tests [list \
> > + [list "PLT test (shared library)" \
> > + "-shared -T plt-local-lib.ld" \
> > + "-k" \
> > + { plt-local-lib.s } \
> > + { { objdump -d plt-local-lib.dd } } \
> > + "plt-local-lib.so"] \
> > + [list "PLT test (object 1)" \
> > + "-r" \
>
> ... (pruned for brevity)
>
> Ouch, another one of those tables-of-lists-of-tables and in a
> brand-new .exp.
>
> Let me suggest instead using a mechanism such as run_dump_test
> with depended parts (DSO's linked against) sorted before others.
> This'd give easy drop-in of new tests without having to add to a
> separate table entry somewhere. See ld-cris/cris.exp. Similar
> new test: ld-arm/gc-hidden-1.d (unfortunately there's no
> file-name iterator in arm-elf.exp, entries are still added
> manually).
So I have found some time to look into your suggestion, and frankly, I am
not happy with it at all. In my opinion your approach merely avoids the
shortcomings of the LD test framework in a different way. If I could
honestly say it is a solution, then I would certainly be eager to adopt
it.
I find it to be a workaround because:
1. The dependency between the final executable and any shared libraries is
not expressed anywhere except from the linker flags in the ld:
specifier. Of course keeping the dependency in a single place is good,
but the way it works in your scheme it doesn't actually cause the
framework to take it into account in any way, it just depends on
building all the shared libraries first.
2. Test cases are not grouped together anymore, if for example a test case
consists of a bunch of shared libraries and a bunch of executables
linked against them, then the shared libraries are tested at a
different stage than the executables. That may make following the
dependencies a bit more difficult.
3. Test cases are run implicitly -- you see it as an advantage, I do not.
I find such test suites harder to follow and prefer being able to add
any extra commentary or other arrangements in .exp files (cf. the MIPS
test suite and the "foreach { abi flag emul } $abis { ... }" loop
there).
4. It resorts to some fragile file shuffling code so that proper .so
objects are created.
On the contrary my proposal makes it clearly a self-contained test case,
it is easily readable and the only drawback is it uses an unnecessary
incremental link because run_ld_link_tests does not support stopping after
assembly for intermediate steps. So yes, it has shortcomings, but your
proposal has no clear advantage to me I am afraid.
NB the first shared-library dump is not strictly necessary and does not
check anything related to this bug, I just added it to have extra
coverage, because -- as you must have undoubtedly noticed -- current one
for the VAX ELF target is particularly poor. Your proposal does not
address cases where a dump of any intermediate shared libraries is not
required and in my version you can just remove the "objdump" clause.
So I think the test case is as good as it could be as it stands.
That said however, I do not object discussing any improvement to the LD
test framework. Including in particular a way to pull shared library
dependencies automatically from .d files. I have a vague shape of how
this might look like in my mind, but let's leave it to sharpen for now; I
don't think any prospect of a test suite improvement should be holding
this obvious bug fix.
So taking the above consideration into account would anyone with the
power to approve this submission please review my proposal and say whether
it is OK or not? I have repeated it below to save you digging through
mailing list archives. Thanks.
> The change below fixes a problem with the creation of the PLT on the VAX
> target, where entries are made where they should not for local symbols,
> leading to unneeded or even empty slots. The conditions for the latters
> are a bit obscure, for such a slot to be created a dynamic symbol has to
> be preempted in the link with a static hidden symbol referred to by a PLT
> relocation (i.e. from a PIC relocatable). This is covered by the test
> case provided. Executables with empty PLT slots are invalid and rejected
> by the dynamic linker.
>
> There is some code duplication in the area affected, so I have merged the
> two pieces (that deal with local and garbage-collected symbols,
> respectively) into one, similarly to how some other targets do it. Also
> the attempt to make a symbol whose h->dynindx is -1 dynamic is incorrect
> here -- such symbols will have qualified as local in SYMBOL_CALLS_LOCAL
> and shouldn't be produced.
>
> No regressions with this change. OK to apply?
2012-07-11 Maciej W. Rozycki <macro@linux-mips.org>
bfd/
* elf32-vax.c (elf_vax_adjust_dynamic_symbol): Don't allocate
PLT slots for local symbols.
ld/testsuite/
* ld-vax-elf/plt-local-lib.dd: New test.
* ld-vax-elf/plt-local-lib.ld: New test linker script.
* ld-vax-elf/plt-local-lib.s: New test source.
* ld-vax-elf/plt-local.dd: New test.
* ld-vax-elf/plt-local.ld:New test linker script.
* ld-vax-elf/plt-local.s: New test source.
* ld-vax-elf/plt-local-hidden-pic.s: New test source.
* ld-vax-elf/plt-local-rehidden-pic.s: New test source.
* ld-vax-elf/vax-elf.exp: New test script.
Maciej
binutils-vax-plt-local.patch
Index: binutils/bfd/elf32-vax.c
===================================================================
--- binutils.orig/bfd/elf32-vax.c
+++ binutils/bfd/elf32-vax.c
@@ -934,39 +934,21 @@ elf_vax_adjust_dynamic_symbol (info, h)
if (h->type == STT_FUNC
|| h->needs_plt)
{
- if (! info->shared
- && !h->def_dynamic
- && !h->ref_dynamic
- /* We must always create the plt entry if it was referenced
- by a PLTxxO relocation. In this case we already recorded
- it as a dynamic symbol. */
- && h->dynindx == -1)
+ if (h->plt.refcount <= 0
+ || SYMBOL_CALLS_LOCAL (info, h)
+ || (ELF_ST_VISIBILITY (h->other) != STV_DEFAULT
+ && h->root.type == bfd_link_hash_undefweak))
{
/* This case can occur if we saw a PLTxx reloc in an input
file, but the symbol was never referred to by a dynamic
- object. In such a case, we don't actually need to build
- a procedure linkage table, and we can just do a PCxx
- reloc instead. */
- BFD_ASSERT (h->needs_plt);
+ object, or if all references were garbage collected. In
+ such a case, we don't actually need to build a procedure
+ linkage table, and we can just do a PCxx reloc instead. */
h->plt.offset = (bfd_vma) -1;
- return TRUE;
- }
-
- /* GC may have rendered this entry unused. */
- if (h->plt.refcount <= 0)
- {
h->needs_plt = 0;
- h->plt.offset = (bfd_vma) -1;
return TRUE;
}
- /* Make sure this symbol is output as a dynamic symbol. */
- if (h->dynindx == -1)
- {
- if (! bfd_elf_link_record_dynamic_symbol (info, h))
- return FALSE;
- }
-
s = bfd_get_section_by_name (dynobj, ".plt");
BFD_ASSERT (s != NULL);
Index: binutils/ld/testsuite/ld-vax-elf/plt-local-hidden-pic.s
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/plt-local-hidden-pic.s
@@ -0,0 +1,14 @@
+ .text
+
+ .hidden foo_hidden
+ .globl foo_hidden
+ .type foo_hidden, @function
+foo_hidden:
+ .word 0
+ calls $0, foo_extern
+ calls $0, foo_global
+ calls $0, foo_local
+ calls $0, foo_hidden
+ calls $0, foo_rehidden
+ ret
+ .size foo_hidden, . - foo_hidden
Index: binutils/ld/testsuite/ld-vax-elf/plt-local-lib.dd
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/plt-local-lib.dd
@@ -0,0 +1,91 @@
+.*: +file format .*vax.*
+
+Disassembly of section \.plt:
+
+00001000 <foo_local@plt-0xc>:
+ 1000: dd ef 76 20 pushl 307c <_GLOBAL_OFFSET_TABLE_\+0x4>
+ 1004: 00 00
+ 1006: 17 ff 74 20 jmp \*3080 <_GLOBAL_OFFSET_TABLE_\+0x8>
+ 100a: 00 00
+
+0000100c <foo_local@plt>:
+ 100c: fc 0f \.word 0x0ffc # Entry mask: < r11 r10 r9 r8 r7 r6 r5 r4 r3 r2 >
+ 100e: 16 ef ec ff jsb 1000 <foo_local@plt-0xc>
+ 1012: ff ff
+ 1014: 00 00 00 00 \.long 0x00000000
+
+00001018 <foo_extern@plt>:
+ 1018: fc 0f \.word 0x0ffc # Entry mask: < r11 r10 r9 r8 r7 r6 r5 r4 r3 r2 >
+ 101a: 16 ef e0 ff jsb 1000 <foo_local@plt-0xc>
+ 101e: ff ff
+ 1020: 0c 00 00 00 \.long 0x0000000c
+
+00001024 <foo_rehidden@plt>:
+ 1024: fc 0f \.word 0x0ffc # Entry mask: < r11 r10 r9 r8 r7 r6 r5 r4 r3 r2 >
+ 1026: 16 ef d4 ff jsb 1000 <foo_local@plt-0xc>
+ 102a: ff ff
+ 102c: 18 00 00 00 \.long 0x00000018
+
+00001030 <foo_global@plt>:
+ 1030: fc 0f \.word 0x0ffc # Entry mask: < r11 r10 r9 r8 r7 r6 r5 r4 r3 r2 >
+ 1032: 16 ef c8 ff jsb 1000 <foo_local@plt-0xc>
+ 1036: ff ff
+ 1038: 24 00 00 00 \.long 0x00000024
+
+Disassembly of section \.text:
+
+00002000 <foo_extern>:
+ 2000: 00 00 \.word 0x0000 # Entry mask: < >
+ 2002: fb 00 ff 7f calls \$0x0,\*3088 <_GLOBAL_OFFSET_TABLE_\+0x10>
+ 2006: 10 00 00
+ 2009: fb 00 ff 80 calls \$0x0,\*3090 <_GLOBAL_OFFSET_TABLE_\+0x18>
+ 200d: 10 00 00
+ 2010: fb 00 ff 6d calls \$0x0,\*3084 <_GLOBAL_OFFSET_TABLE_\+0xc>
+ 2014: 10 00 00
+ 2017: fb 00 ef 2e calls \$0x0,204c <foo_hidden>
+ 201b: 00 00 00
+ 201e: fb 00 ff 67 calls \$0x0,\*308c <_GLOBAL_OFFSET_TABLE_\+0x14>
+ 2022: 10 00 00
+ 2025: 04 ret
+
+00002026 <foo_local>:
+ 2026: 00 00 \.word 0x0000 # Entry mask: < >
+ 2028: fb 00 ff 59 calls \$0x0,\*3088 <_GLOBAL_OFFSET_TABLE_\+0x10>
+ 202c: 10 00 00
+ 202f: fb 00 ff 5a calls \$0x0,\*3090 <_GLOBAL_OFFSET_TABLE_\+0x18>
+ 2033: 10 00 00
+ 2036: fb 00 ff 47 calls \$0x0,\*3084 <_GLOBAL_OFFSET_TABLE_\+0xc>
+ 203a: 10 00 00
+ 203d: fb 00 ef 08 calls \$0x0,204c <foo_hidden>
+ 2041: 00 00 00
+ 2044: fb 00 ff 41 calls \$0x0,\*308c <_GLOBAL_OFFSET_TABLE_\+0x14>
+ 2048: 10 00 00
+ 204b: 04 ret
+
+0000204c <foo_hidden>:
+ 204c: 00 00 \.word 0x0000 # Entry mask: < >
+ 204e: fb 00 ff 33 calls \$0x0,\*3088 <_GLOBAL_OFFSET_TABLE_\+0x10>
+ 2052: 10 00 00
+ 2055: fb 00 ff 34 calls \$0x0,\*3090 <_GLOBAL_OFFSET_TABLE_\+0x18>
+ 2059: 10 00 00
+ 205c: fb 00 ff 21 calls \$0x0,\*3084 <_GLOBAL_OFFSET_TABLE_\+0xc>
+ 2060: 10 00 00
+ 2063: fb 00 ef e2 calls \$0x0,204c <foo_hidden>
+ 2067: ff ff ff
+ 206a: fb 00 ff 1b calls \$0x0,\*308c <_GLOBAL_OFFSET_TABLE_\+0x14>
+ 206e: 10 00 00
+ 2071: 04 ret
+
+00002072 <foo_rehidden>:
+ 2072: 00 00 \.word 0x0000 # Entry mask: < >
+ 2074: fb 00 ff 0d calls \$0x0,\*3088 <_GLOBAL_OFFSET_TABLE_\+0x10>
+ 2078: 10 00 00
+ 207b: fb 00 ff 0e calls \$0x0,\*3090 <_GLOBAL_OFFSET_TABLE_\+0x18>
+ 207f: 10 00 00
+ 2082: fb 00 ff fb calls \$0x0,\*3084 <_GLOBAL_OFFSET_TABLE_\+0xc>
+ 2086: 0f 00 00
+ 2089: fb 00 ef bc calls \$0x0,204c <foo_hidden>
+ 208d: ff ff ff
+ 2090: fb 00 ff f5 calls \$0x0,\*308c <_GLOBAL_OFFSET_TABLE_\+0x14>
+ 2094: 0f 00 00
+ 2097: 04 ret
Index: binutils/ld/testsuite/ld-vax-elf/plt-local-lib.ld
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/plt-local-lib.ld
@@ -0,0 +1,18 @@
+SECTIONS
+{
+ . = 0;
+ .hash : { *(.hash) }
+ .dynsym : { *(.dynsym) }
+ .dynstr : { *(.dynstr) }
+ .rela.plt : { *(.rela.plt) }
+
+ . = 0x1000;
+ .plt : { *(.plt) }
+
+ . = 0x2000;
+ .text : { *(.text) }
+
+ . = 0x3000;
+ .dynamic : { *(.dynamic) }
+ .got : { *(.got.plt) }
+}
Index: binutils/ld/testsuite/ld-vax-elf/plt-local-lib.s
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/plt-local-lib.s
@@ -0,0 +1,50 @@
+ .text
+
+ .globl foo_extern
+ .type foo_extern, @function
+foo_extern:
+ .word 0
+ calls $0, foo_extern
+ calls $0, foo_global
+ calls $0, foo_local
+ calls $0, foo_hidden
+ calls $0, foo_rehidden
+ ret
+ .size foo_extern, . - foo_extern
+
+ .globl foo_local
+ .type foo_local, @function
+foo_local:
+ .word 0
+ calls $0, foo_extern
+ calls $0, foo_global
+ calls $0, foo_local
+ calls $0, foo_hidden
+ calls $0, foo_rehidden
+ ret
+ .size foo_local, . - foo_local
+
+ .hidden foo_hidden
+ .globl foo_hidden
+ .type foo_hidden, @function
+foo_hidden:
+ .word 0
+ calls $0, foo_extern
+ calls $0, foo_global
+ calls $0, foo_local
+ calls $0, foo_hidden
+ calls $0, foo_rehidden
+ ret
+ .size foo_hidden, . - foo_hidden
+
+ .globl foo_rehidden
+ .type foo_rehidden, @function
+foo_rehidden:
+ .word 0
+ calls $0, foo_extern
+ calls $0, foo_global
+ calls $0, foo_local
+ calls $0, foo_hidden
+ calls $0, foo_rehidden
+ ret
+ .size foo_rehidden, . - foo_rehidden
Index: binutils/ld/testsuite/ld-vax-elf/plt-local-rehidden-pic.s
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/plt-local-rehidden-pic.s
@@ -0,0 +1,14 @@
+ .text
+
+ .hidden foo_rehidden
+ .globl foo_rehidden
+ .type foo_rehidden, @function
+foo_rehidden:
+ .word 0
+ calls $0, foo_extern
+ calls $0, foo_global
+ calls $0, foo_local
+ calls $0, foo_hidden
+ calls $0, foo_rehidden
+ ret
+ .size foo_rehidden, . - foo_rehidden
Index: binutils/ld/testsuite/ld-vax-elf/plt-local.dd
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/plt-local.dd
@@ -0,0 +1,73 @@
+.*: +file format .*vax.*
+
+Disassembly of section \.plt:
+
+00001000 <foo_extern@plt-0xc>:
+ 1000: dd ef 86 20 pushl 308c <_GLOBAL_OFFSET_TABLE_\+0x4>
+ 1004: 00 00
+ 1006: 17 ff 84 20 jmp \*3090 <_GLOBAL_OFFSET_TABLE_\+0x8>
+ 100a: 00 00
+
+0000100c <foo_extern@plt>:
+ 100c: fc 0f \.word 0x0ffc # Entry mask: < r11 r10 r9 r8 r7 r6 r5 r4 r3 r2 >
+ 100e: 16 ef ec ff jsb 1000 <foo_extern@plt-0xc>
+ 1012: ff ff
+ 1014: 00 00 00 00 \.long 0x00000000
+
+Disassembly of section \.text:
+
+00002000 <foo_hidden>:
+ 2000: 00 00 \.word 0x0000 # Entry mask: < >
+ 2002: fb 00 ff 8b calls \$0x0,\*3094 <_GLOBAL_OFFSET_TABLE_\+0xc>
+ 2006: 10 00 00
+ 2009: fb 00 ef 3c calls \$0x0,204c <foo_global>
+ 200d: 00 00 00
+ 2010: fb 00 ef 5b calls \$0x0,2072 <foo_local>
+ 2014: 00 00 00
+ 2017: fb 00 ef e2 calls \$0x0,2000 <foo_hidden>
+ 201b: ff ff ff
+ 201e: fb 00 ef 01 calls \$0x0,2026 <foo_rehidden>
+ 2022: 00 00 00
+ 2025: 04 ret
+
+00002026 <foo_rehidden>:
+ 2026: 00 00 \.word 0x0000 # Entry mask: < >
+ 2028: fb 00 ff 65 calls \$0x0,\*3094 <_GLOBAL_OFFSET_TABLE_\+0xc>
+ 202c: 10 00 00
+ 202f: fb 00 ef 16 calls \$0x0,204c <foo_global>
+ 2033: 00 00 00
+ 2036: fb 00 ef 35 calls \$0x0,2072 <foo_local>
+ 203a: 00 00 00
+ 203d: fb 00 ef bc calls \$0x0,2000 <foo_hidden>
+ 2041: ff ff ff
+ 2044: fb 00 ef db calls \$0x0,2026 <foo_rehidden>
+ 2048: ff ff ff
+ 204b: 04 ret
+
+0000204c <foo_global>:
+ 204c: 00 00 \.word 0x0000 # Entry mask: < >
+ 204e: fb 00 ff 3f calls \$0x0,\*3094 <_GLOBAL_OFFSET_TABLE_\+0xc>
+ 2052: 10 00 00
+ 2055: fb 00 ef f0 calls \$0x0,204c <foo_global>
+ 2059: ff ff ff
+ 205c: fb 00 ef 0f calls \$0x0,2072 <foo_local>
+ 2060: 00 00 00
+ 2063: fb 00 ef 96 calls \$0x0,2000 <foo_hidden>
+ 2067: ff ff ff
+ 206a: fb 00 ef b5 calls \$0x0,2026 <foo_rehidden>
+ 206e: ff ff ff
+ 2071: 04 ret
+
+00002072 <foo_local>:
+ 2072: 00 00 \.word 0x0000 # Entry mask: < >
+ 2074: fb 00 ff 19 calls \$0x0,\*3094 <_GLOBAL_OFFSET_TABLE_\+0xc>
+ 2078: 10 00 00
+ 207b: fb 00 ef ca calls \$0x0,204c <foo_global>
+ 207f: ff ff ff
+ 2082: fb 00 ef e9 calls \$0x0,2072 <foo_local>
+ 2086: ff ff ff
+ 2089: fb 00 ef 70 calls \$0x0,2000 <foo_hidden>
+ 208d: ff ff ff
+ 2090: fb 00 ef 8f calls \$0x0,2026 <foo_rehidden>
+ 2094: ff ff ff
+ 2097: 04 ret
Index: binutils/ld/testsuite/ld-vax-elf/plt-local.ld
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/plt-local.ld
@@ -0,0 +1,34 @@
+ENTRY (foo_global)
+SECTIONS
+{
+ . = 0;
+ .interp : { *(.interp) }
+ .hash : { *(.hash) }
+ .dynsym : { *(.dynsym) }
+ .dynstr : { *(.dynstr) }
+ .gnu.version : { *(.gnu.version) }
+ .gnu.version_d : { *(.gnu.version_d) }
+ .rela.plt : { *(.rela.plt) }
+
+ . = 0x1000;
+ .plt : { *(.plt) }
+
+ . = 0x2000;
+ .text : { *(.text) }
+
+ . = 0x3000;
+ .dynamic : { *(.dynamic) }
+ .got : { *(.got.plt) }
+};
+VERSION
+{
+ {
+ global:
+ foo_extern;
+ foo_global;
+ foo_hidden;
+ foo_rehidden;
+ local:
+ foo_local;
+ };
+}
Index: binutils/ld/testsuite/ld-vax-elf/plt-local.s
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/plt-local.s
@@ -0,0 +1,25 @@
+ .text
+
+ .globl foo_global
+ .type foo_global, @function
+foo_global:
+ .word 0
+ calls $0, foo_extern
+ calls $0, foo_global
+ calls $0, foo_local
+ calls $0, foo_hidden
+ calls $0, foo_rehidden
+ ret
+ .size foo_global, . - foo_global
+
+ .globl foo_local
+ .type foo_local, @function
+foo_local:
+ .word 0
+ calls $0, foo_extern
+ calls $0, foo_global
+ calls $0, foo_local
+ calls $0, foo_hidden
+ calls $0, foo_rehidden
+ ret
+ .size foo_local, . - foo_local
Index: binutils/ld/testsuite/ld-vax-elf/vax-elf.exp
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/vax-elf.exp
@@ -0,0 +1,50 @@
+# Expect script for VAX ELF linker tests
+# Copyright 2012 Free Software Foundation, Inc.
+#
+# This file is part of the GNU Binutils.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
+# MA 02110-1301, USA.
+#
+
+if { ![istarget vax-*-*] || ![is_elf_format] } {
+ return
+}
+
+run_ld_link_tests [list \
+ [list "PLT test (shared library)" \
+ "-shared -T plt-local-lib.ld" \
+ "-k" \
+ { plt-local-lib.s } \
+ { { objdump -d plt-local-lib.dd } } \
+ "plt-local-lib.so"] \
+ [list "PLT test (object 1)" \
+ "-r" \
+ "-k" \
+ { plt-local-hidden-pic.s } \
+ {} \
+ "plt-local-hidden-pic-r.o"] \
+ [list "PLT test (object 2)" \
+ "-r" \
+ "-k" \
+ { plt-local-rehidden-pic.s } \
+ {} \
+ "plt-local-rehidden-pic-r.o"] \
+ [list "PLT test (executable)" \
+ "-T plt-local.ld tmpdir/plt-local-hidden-pic-r.o tmpdir/plt-local-rehidden-pic-r.o tmpdir/plt-local-lib.so" \
+ "" \
+ { plt-local.s } \
+ { { objdump -d plt-local.dd } } \
+ "plt-local"]]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] VAX/BFD: Omit PLT slots for local symbols
2012-07-11 0:04 ` Maciej W. Rozycki
@ 2012-07-11 3:29 ` Hans-Peter Nilsson
2012-07-15 21:57 ` Jan-Benedict Glaw
1 sibling, 0 replies; 6+ messages in thread
From: Hans-Peter Nilsson @ 2012-07-11 3:29 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Jan-Benedict Glaw, binutils
On Wed, 11 Jul 2012, Maciej W. Rozycki wrote:
> On Tue, 8 May 2012, Hans-Peter Nilsson wrote:
>
> > > ld/testsuite/
> > > * ld-vax-elf/plt-local-lib.dd: New test.
> > > * ld-vax-elf/plt-local-lib.ld: New test linker script.
> > > * ld-vax-elf/plt-local-lib.s: New test source.
> > > * ld-vax-elf/plt-local.dd: New test.
> > > * ld-vax-elf/plt-local.ld:New test linker script.
> > > * ld-vax-elf/plt-local.s: New test source.
> > > * ld-vax-elf/plt-local-hidden-pic.s: New test source.
> > > * ld-vax-elf/plt-local-rehidden-pic.s: New test source.
> > > * ld-vax-elf/vax-elf.exp: New test script.
> >
> > > Index: binutils/ld/testsuite/ld-vax-elf/vax-elf.exp
> > > ===================================================================
> > > --- /dev/null
> > > +++ binutils/ld/testsuite/ld-vax-elf/vax-elf.exp
> > > @@ -0,0 +1,50 @@
> > > +# Expect script for VAX ELF linker tests
> >
> > ...
> > > +
> > > +run_ld_link_tests [list \
> > > + [list "PLT test (shared library)" \
> > > + "-shared -T plt-local-lib.ld" \
> > > + "-k" \
> > > + { plt-local-lib.s } \
> > > + { { objdump -d plt-local-lib.dd } } \
> > > + "plt-local-lib.so"] \
> > > + [list "PLT test (object 1)" \
> > > + "-r" \
> >
> > ... (pruned for brevity)
> >
> > Ouch, another one of those tables-of-lists-of-tables and in a
> > brand-new .exp.
> >
> > Let me suggest instead using a mechanism such as run_dump_test
> > with depended parts (DSO's linked against) sorted before others.
> > This'd give easy drop-in of new tests without having to add to a
> > separate table entry somewhere. See ld-cris/cris.exp. Similar
> > new test: ld-arm/gc-hidden-1.d (unfortunately there's no
> > file-name iterator in arm-elf.exp, entries are still added
> > manually).
>
> So I have found some time to look into your suggestion, and frankly, I am
> not happy with it at all. In my opinion your approach merely avoids the
> shortcomings of the LD test framework in a different way. If I could
> honestly say it is a solution, then I would certainly be eager to adopt
> it.
>
> I find it to be a workaround because:
>
> 1. The dependency between the final executable and any shared libraries is
> not expressed anywhere except from the linker flags in the ld:
> specifier. Of course keeping the dependency in a single place is good,
> but the way it works in your scheme it doesn't actually cause the
> framework to take it into account in any way, it just depends on
> building all the shared libraries first.
That's the only drawback, IMO, and no worse than for tables.
Just add (framework for a) "dependency: <renamed-output-file>
<name-of-.d-file-for-previous-test>" and presto, it's explicit.
It's possible without changes to the tests themselves, in theory
at least, if run_dump_test sneaks a peek at the options in "ld:"
and "ld_after_inputfiles:" and arranges the order and naming of
the output files (mapping between .d and .so). But, I'd prefer
the more explicit keyword.
> 2. Test cases are not grouped together anymore, if for example a test case
> consists of a bunch of shared libraries and a bunch of executables
> linked against them, then the shared libraries are tested at a
> different stage than the executables. That may make following the
> dependencies a bit more difficult.
(This seems to me the same point as above.) My take is that the
grouping is cluttering: I see them as different test-cases, so
they *should* be separate. The order is no better or more
explicit with the ordered-in-the-table-approach. The nacl-tests
made that clear; there were implicit orders in the table that
were hard enough to follow, that when the tests were rearranged
for one reason, the implicit order was broken.
> 3. Test cases are run implicitly -- you see it as an advantage, I do not.
> I find such test suites harder to follow and prefer being able to add
> any extra commentary or other arrangements in .exp files (cf. the MIPS
> test suite and the "foreach { abi flag emul } $abis { ... }" loop
> there).
It seems we are just of different opinions here with no factual
component. Comments can be added nearby the options in both
cases. The "foreach-cases" are IMHO just harder to follow, but
you can do that equally simple with the run_dump_test-based
test-cases if you really want.
> 4. It resorts to some fragile file shuffling code so that proper .so
> objects are created.
(Still the same point as 1 and 2!) Fragility is certainly in
the order in the table too, n.b. recent nacl-tests mentioned
above (broken-test-case complaints in the list archives). At
least it's more explicit with the "fragile file shuffling code".
> On the contrary my proposal makes it clearly a self-contained test case,
No self-containedness as far as I see it: you have to edit more
than one file for each new test; the new test and the framework
bits and flags to run it. This is noticeable when you review a
patch adding more than one test-case in that you have to
double-check that the patch adds the framework bits for all
tests.
> it is easily readable and the only drawback is it uses an unnecessary
> incremental link because run_ld_link_tests does not support stopping after
> assembly for intermediate steps.
A drawback, there.
> So yes, it has shortcomings, but your
> proposal has no clear advantage to me I am afraid.
Really? For one, there is no risk of forgetting to update the
framework .exp file to actually run the test with the looped *.d
approach. (Yes, I remember at least one such case in the past.)
Another is that you have the test-case (the expected output or
error text), and the flags to build it all in one single place.
And that same place for comments too.
A third is that with the tabled run_dump_test approach, there
are more options already present than for run_ld_link_tests.
Not a big thing, for example xfail is easy to add, seven lines.
Reading the ld-lib.exp source, I also see a fourth: a bug making
the machinery that enables you to run a subset of the tests
ineffective; for run_ld_link_tests; the $testname has to match.
(For run_dump_test cases, looped or explicit, this is always the
filename.)
A fifth follows; the testname isn't optional. I prefer seeing
"FAIL: name-of-.d-file/file-with-expected-output" to "FAIL:
elaborate name". It might be a matter of taste, but at least I
have the option for run_dump_tests (looped-implicit or
explicit).
> NB the first shared-library dump is not strictly necessary and does not
> check anything related to this bug, I just added it to have extra
> coverage, because -- as you must have undoubtedly noticed -- current one
> for the VAX ELF target is particularly poor. Your proposal does not
> address cases where a dump of any intermediate shared libraries is not
> required and in my version you can just remove the "objdump" clause.
But without the objdump or equivalent analyzer, you don't really
have the extra coverage which you apparently prefer. ;)
I always prefer it, so I don't see any inconvenience in the
extra line or three. Patches for a keyword skipping the
analyzer are likely welcome; a two- or three-liner to
ld-lib.exp. In the meantime, the more cryptic "PROG: :" and
"#pass" should work.
> So I think the test case is as good as it could be as it stands.
Sure, I definitely have no intention to block test-cases, I was
just suggesting using a different and IMO preferable framework,
at the discretion of the author; you, and any approver.
> That said however, I do not object discussing any improvement to the LD
> test framework. Including in particular a way to pull shared library
> dependencies automatically from .d files. I have a vague shape of how
> this might look like in my mind,
See above "dependency: <renamed-output-file>
<name-of-.d-file-for-previous-test>" for run_dump_test-based
tests. Doesn't carry over to tabled run_ld_link_tests, but
that's IMHO another flaw in that machinery. :)
brgds, H-P
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] VAX/BFD: Omit PLT slots for local symbols
2012-07-11 0:04 ` Maciej W. Rozycki
2012-07-11 3:29 ` Hans-Peter Nilsson
@ 2012-07-15 21:57 ` Jan-Benedict Glaw
2012-08-06 10:14 ` Maciej W. Rozycki
1 sibling, 1 reply; 6+ messages in thread
From: Jan-Benedict Glaw @ 2012-07-15 21:57 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Hans-Peter Nilsson, binutils
[-- Attachment #1: Type: text/plain, Size: 1248 bytes --]
On Wed, 2012-07-11 01:04:04 +0100, Maciej W. Rozycki <macro@linux-mips.org> wrote:
[...]
> 2012-07-11 Maciej W. Rozycki <macro@linux-mips.org>
>
> bfd/
> * elf32-vax.c (elf_vax_adjust_dynamic_symbol): Don't allocate
> PLT slots for local symbols.
>
> ld/testsuite/
> * ld-vax-elf/plt-local-lib.dd: New test.
> * ld-vax-elf/plt-local-lib.ld: New test linker script.
> * ld-vax-elf/plt-local-lib.s: New test source.
> * ld-vax-elf/plt-local.dd: New test.
> * ld-vax-elf/plt-local.ld:New test linker script.
> * ld-vax-elf/plt-local.s: New test source.
> * ld-vax-elf/plt-local-hidden-pic.s: New test source.
> * ld-vax-elf/plt-local-rehidden-pic.s: New test source.
> * ld-vax-elf/vax-elf.exp: New test script.
Even if the test cases wiring-up can be done in one way or another and
both have their pros and cons, lets stop the discussion here. First of
all, I'm quite happy that there are actually new test cases. Editing
another list to let it run is a second step, but ... I think we can
manage that.
Please apply.
MfG, JBG
--
Jan-Benedict Glaw jbglaw@lug-owl.de +49-172-7608481
Signature of: http://www.eyrie.org/~eagle/faqs/questions.html
the second :
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] VAX/BFD: Omit PLT slots for local symbols
2012-07-15 21:57 ` Jan-Benedict Glaw
@ 2012-08-06 10:14 ` Maciej W. Rozycki
0 siblings, 0 replies; 6+ messages in thread
From: Maciej W. Rozycki @ 2012-08-06 10:14 UTC (permalink / raw)
To: Jan-Benedict Glaw; +Cc: Hans-Peter Nilsson, binutils
On Sun, 15 Jul 2012, Jan-Benedict Glaw wrote:
> > bfd/
> > * elf32-vax.c (elf_vax_adjust_dynamic_symbol): Don't allocate
> > PLT slots for local symbols.
> >
> > ld/testsuite/
> > * ld-vax-elf/plt-local-lib.dd: New test.
> > * ld-vax-elf/plt-local-lib.ld: New test linker script.
> > * ld-vax-elf/plt-local-lib.s: New test source.
> > * ld-vax-elf/plt-local.dd: New test.
> > * ld-vax-elf/plt-local.ld:New test linker script.
> > * ld-vax-elf/plt-local.s: New test source.
> > * ld-vax-elf/plt-local-hidden-pic.s: New test source.
> > * ld-vax-elf/plt-local-rehidden-pic.s: New test source.
> > * ld-vax-elf/vax-elf.exp: New test script.
>
> Even if the test cases wiring-up can be done in one way or another and
> both have their pros and cons, lets stop the discussion here. First of
> all, I'm quite happy that there are actually new test cases. Editing
> another list to let it run is a second step, but ... I think we can
> manage that.
Nothing's going to be cast in stone here, I'll be happy to update or even
turn this test case (and any following) upside down entirely once we've
come up with a better solution.
> Please apply.
Thanks for your review, I have trivially refreshed this change and
regression retested it with no problems spotted. Applied now. Apologies
for the latency.
Maciej
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-08-05 22:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-07 23:30 [PATCH] VAX/BFD: Omit PLT slots for local symbols Maciej W. Rozycki
2012-05-08 4:12 ` Hans-Peter Nilsson
2012-07-11 0:04 ` Maciej W. Rozycki
2012-07-11 3:29 ` Hans-Peter Nilsson
2012-07-15 21:57 ` Jan-Benedict Glaw
2012-08-06 10:14 ` Maciej W. Rozycki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).