* [PATCH] PR ld/21334: Always assign dynsym indices even in static binaries
@ 2017-04-11 23:16 Maciej W. Rozycki
2017-04-20 16:03 ` [PING][PATCH] " Maciej W. Rozycki
0 siblings, 1 reply; 4+ messages in thread
From: Maciej W. Rozycki @ 2017-04-11 23:16 UTC (permalink / raw)
To: binutils; +Cc: James Cowgill
Complement commit e17b0c351f0b ("MIPS/BFD: Respect the ELF gABI dynamic
symbol table sort requirement") and correct an inconsistency in dynamic
symbol accounting data causing an assertion failure in the MIPS backend:
ld: BFD (GNU Binutils) 2.28.51.20170330 assertion fail ../../binutils-gdb/bfd/elfxx-mips.c:3860
in the course of making a GOT entry in a static binary to satisfy a GOT
relocation present in input, due to the local dynamic symbol count not
having been established.
To do so move the call to `_bfd_elf_link_renumber_dynsyms' outside a
check for dynamic sections to have been created, so that the function
always assigns dynsym indices and initializes associated data, even for
static binaries.
bfd/
PR ld/21334
* elflink.c (bfd_elf_size_dynsym_hash_dynstr): Always call
`_bfd_elf_link_renumber_dynsyms'.
ld/
PR ld/21334
* testsuite/ld-mips-elf/pr21334.dd: New test.
* testsuite/ld-mips-elf/pr21334.gd: New test.
* testsuite/ld-mips-elf/pr21334.ld: New test linker script.
* testsuite/ld-mips-elf/pr21334.s: New test source.
* testsuite/ld-mips-elf/mips-elf.exp: Run the new tests.
---
No regressions against the usual targets.
Calling `_bfd_elf_link_renumber_dynsyms' unconditionally has also been
verified to some extent to be safe by Debian across their targets, as a
side effect of `_bfd_elf_link_renumber_dynsyms' always being called when
section GC has been enabled with `--gc-sections', as it has been the case
with some of their packages, which according to James's statement posted
with the PR includes KDE/Qt and some other packages.
OK for master and 2.28?
NB one of the test cases added relies on MIPS static GOT dump support
in `readelf' just posted, so firstly the order of patches will have to
be maintained and secondly I'll remove it from the 2.28 backport.
Maciej
binutils-bfd-elf-always-dynsym-renumber.diff
Index: binutils/bfd/elflink.c
===================================================================
--- binutils.orig/bfd/elflink.c 2017-04-10 23:11:21.572119928 +0100
+++ binutils/bfd/elflink.c 2017-04-11 02:45:59.328420966 +0100
@@ -6778,6 +6778,8 @@ bfd_boolean
bfd_elf_size_dynsym_hash_dynstr (bfd *output_bfd, struct bfd_link_info *info)
{
const struct elf_backend_data *bed;
+ unsigned long section_sym_count;
+ bfd_size_type dynsymcount;
if (!is_elf_hash_table (info->hash))
return TRUE;
@@ -6785,24 +6787,29 @@ bfd_elf_size_dynsym_hash_dynstr (bfd *ou
bed = get_elf_backend_data (output_bfd);
(*bed->elf_backend_init_index_section) (output_bfd, info);
+ /* Assign dynsym indices. In a shared library we generate a section
+ symbol for each output section, which come first. Next come all
+ of the back-end allocated local dynamic syms, followed by the rest
+ of the global symbols.
+
+ We always do this step even if dynamic sections have not been made,
+ because backends may need symbol accounting data produced here,
+ e.g. the MIPS backend uses dynamic symbol counts to lay out GOT,
+ which will be produced in the presence of GOT relocations even in
+ static binaries (holding fixed data in that case, to satisfy those
+ relocations). */
+
+ dynsymcount = _bfd_elf_link_renumber_dynsyms (output_bfd, info,
+ §ion_sym_count);
+
if (elf_hash_table (info)->dynamic_sections_created)
{
bfd *dynobj;
asection *s;
- bfd_size_type dynsymcount;
- unsigned long section_sym_count;
unsigned int dtagcount;
dynobj = elf_hash_table (info)->dynobj;
- /* Assign dynsym indicies. In a shared library we generate a
- section symbol for each output section, which come first.
- Next come all of the back-end allocated local dynamic syms,
- followed by the rest of the global symbols. */
-
- dynsymcount = _bfd_elf_link_renumber_dynsyms (output_bfd, info,
- §ion_sym_count);
-
/* Work out the size of the symbol version section. */
s = bfd_get_linker_section (dynobj, ".gnu.version");
BFD_ASSERT (s != NULL);
Index: binutils/ld/testsuite/ld-mips-elf/mips-elf.exp
===================================================================
--- binutils.orig/ld/testsuite/ld-mips-elf/mips-elf.exp 2017-04-10 23:11:20.401635333 +0100
+++ binutils/ld/testsuite/ld-mips-elf/mips-elf.exp 2017-04-11 02:47:43.596728893 +0100
@@ -1130,3 +1130,13 @@ if { $linux_gnu } {
"export-$class-ref"]]
}
}
+
+# PR ld/21334 GOT relocation in static binary test.
+run_ld_link_tests [list \
+ [list \
+ "PR ld/21233 MIPS GOT16 relocation in static binary" \
+ "$abi_ldflags(o32) -e foo -T pr21334.ld" "" "$abi_asflags(o32)" \
+ {pr21334.s} \
+ {{objdump {-d --prefix-addresses} pr21334.dd} \
+ {readelf -A pr21334.gd}} \
+ "pr21334"]]
Index: binutils/ld/testsuite/ld-mips-elf/pr21334.dd
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils/ld/testsuite/ld-mips-elf/pr21334.dd 2017-04-11 02:47:43.603859887 +0100
@@ -0,0 +1,10 @@
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+[0-9a-f]+ <[^>]*> lui gp,0x1
+[0-9a-f]+ <[^>]*> addiu gp,gp,-32736
+[0-9a-f]+ <[^>]*> addu gp,gp,t9
+[0-9a-f]+ <[^>]*> lw v0,-32744\(gp\)
+[0-9a-f]+ <[^>]*> jr ra
+[0-9a-f]+ <[^>]*> addiu v0,v0,4
+ \.\.\.
Index: binutils/ld/testsuite/ld-mips-elf/pr21334.gd
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils/ld/testsuite/ld-mips-elf/pr21334.gd 2017-04-11 02:47:43.629240183 +0100
@@ -0,0 +1,11 @@
+Static GOT:
+ Canonical gp value: 00008020
+
+ Reserved entries:
+ Address Access Value
+ 00000030 -32752\(gp\) 00000000
+ 00000034 -32748\(gp\) 80000000
+
+ Local entries:
+ Address Access Value
+ 00000038 -32744\(gp\) 00000000
Index: binutils/ld/testsuite/ld-mips-elf/pr21334.ld
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils/ld/testsuite/ld-mips-elf/pr21334.ld 2017-04-11 02:47:43.646400289 +0100
@@ -0,0 +1,9 @@
+SECTIONS
+{
+ .text : { *(.text) }
+ HIDDEN (_gp = ALIGN (16) + 0x7ff0);
+ .got : { *(.got) }
+ .symtab : { *(.symtab) }
+ .strtab : { *(.strtab) }
+ /DISCARD/ : { *(*) }
+}
Index: binutils/ld/testsuite/ld-mips-elf/pr21334.s
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils/ld/testsuite/ld-mips-elf/pr21334.s 2017-04-11 02:47:43.663515875 +0100
@@ -0,0 +1,20 @@
+ .abicalls
+ .text
+ .set noreorder
+ .globl foo
+ .ent foo
+foo:
+ .frame $sp, 0, $31
+ .mask 0x00000000, 0
+ .fmask 0x00000000, 0
+ .cpload $25
+ lw $2, %got(bar)($28)
+ jr $31
+ addiu $2, $2, 4
+ .end foo
+ .weak bar
+ .hidden bar
+
+# Force some (non-delay-slot) zero bytes, to make 'objdump' print ...
+ .align 4, 0
+ .space 16
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PING][PATCH] PR ld/21334: Always assign dynsym indices even in static binaries
2017-04-11 23:16 [PATCH] PR ld/21334: Always assign dynsym indices even in static binaries Maciej W. Rozycki
@ 2017-04-20 16:03 ` Maciej W. Rozycki
2017-04-21 10:14 ` Alan Modra
0 siblings, 1 reply; 4+ messages in thread
From: Maciej W. Rozycki @ 2017-04-20 16:03 UTC (permalink / raw)
To: binutils; +Cc: James Cowgill
On Wed, 12 Apr 2017, Maciej W. Rozycki wrote:
> bfd/
> PR ld/21334
> * elflink.c (bfd_elf_size_dynsym_hash_dynstr): Always call
> `_bfd_elf_link_renumber_dynsyms'.
>
> ld/
> PR ld/21334
> * testsuite/ld-mips-elf/pr21334.dd: New test.
> * testsuite/ld-mips-elf/pr21334.gd: New test.
> * testsuite/ld-mips-elf/pr21334.ld: New test linker script.
> * testsuite/ld-mips-elf/pr21334.s: New test source.
> * testsuite/ld-mips-elf/mips-elf.exp: Run the new tests.
Can I ask for
<https://www.sourceware.org/ml/binutils/2017-04/msg00127.html> to be
reviewed?
Maciej
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PING][PATCH] PR ld/21334: Always assign dynsym indices even in static binaries
2017-04-20 16:03 ` [PING][PATCH] " Maciej W. Rozycki
@ 2017-04-21 10:14 ` Alan Modra
2017-04-24 15:21 ` Maciej W. Rozycki
0 siblings, 1 reply; 4+ messages in thread
From: Alan Modra @ 2017-04-21 10:14 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: binutils, James Cowgill
On Thu, Apr 20, 2017 at 05:02:46PM +0100, Maciej W. Rozycki wrote:
> Can I ask for
> <https://www.sourceware.org/ml/binutils/2017-04/msg00127.html> to be
> reviewed?
I don't even recall seeing the patch, sorry.. I suggest instead that
you renumber dynsyms for mips when !dynamic_sections_created inside a
mips elf_backend_init_index_section. That way the great majority of
targets that do not need this for static executables will not incur
two extra passes over the symbol table.
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PING][PATCH] PR ld/21334: Always assign dynsym indices even in static binaries
2017-04-21 10:14 ` Alan Modra
@ 2017-04-24 15:21 ` Maciej W. Rozycki
0 siblings, 0 replies; 4+ messages in thread
From: Maciej W. Rozycki @ 2017-04-24 15:21 UTC (permalink / raw)
To: Alan Modra; +Cc: binutils, James Cowgill
On Fri, 21 Apr 2017, Alan Modra wrote:
> > Can I ask for
> > <https://www.sourceware.org/ml/binutils/2017-04/msg00127.html> to be
> > reviewed?
>
> I don't even recall seeing the patch, sorry..
No worries!
> I suggest instead that
> you renumber dynsyms for mips when !dynamic_sections_created inside a
> mips elf_backend_init_index_section. That way the great majority of
> targets that do not need this for static executables will not incur
> two extra passes over the symbol table.
Good point about the wasted passes, thanks.
I have looked into alternatives and concluded I could not agree about
`elf_backend_init_index_section' being a good place to add such code to.
While it appears right in terms of execution flow, the intent of this hook
is I think too distant from what `_bfd_elf_link_renumber_dynsyms' does,
and consequently placing the call there would make code hard to understand
and maintain long-term. I might be able to remember that myself, but
anyone coming after me would not have that knowledge.
So instead I looked into putting the call in `mips_before_allocation',
right after `gld${EMULATION_NAME}_before_allocation', which is where the
call to `bfd_elf_size_dynsym_hash_dynstr' is made from. I thought this
would be more appropriate. But then there is that other call made from
`bfd_elf_size_dynamic_sections' after section GC. The complementing MIPS
call for static binaries would have to be made from
`_bfd_mips_elf_always_size_sections', again making the whole arrangement
messy.
So let's do it properly and have `bfd_elf_size_dynamic_sections' calls
left in their current single places, but with an additional backend flag
to control whether they need to be always executed. This has the extra
advantage of keeping the function local to `elflink.c'. I'll be posting a
replacement mini patch series shortly then, including a preparatory fix
for the missing conditional in the `bfd_elf_size_dynamic_sections' call in
section GC, as observed in the course of PR ld/21334 investigation and
noted in Bugzilla.
For the record, below is the alternative I considered and have discarded
as too hackish.
Maciej
binutils-mips-ld-always-dynsym-renumber.diff
Index: binutils/bfd/elf-bfd.h
===================================================================
--- binutils.orig/bfd/elf-bfd.h 2017-04-22 22:57:25.253629828 +0100
+++ binutils/bfd/elf-bfd.h 2017-04-22 23:32:51.062487621 +0100
@@ -2208,6 +2208,8 @@ extern bfd_boolean _bfd_elf_link_create_
(bfd *, struct bfd_link_info *);
extern bfd_boolean _bfd_elf_link_omit_section_dynsym
(bfd *, struct bfd_link_info *, asection *);
+extern unsigned long _bfd_elf_link_renumber_dynsyms
+ (bfd *, struct bfd_link_info *, unsigned long *);
extern bfd_boolean _bfd_elf_create_dynamic_sections
(bfd *, struct bfd_link_info *);
extern bfd_boolean _bfd_elf_create_got_section
Index: binutils/bfd/elflink.c
===================================================================
--- binutils.orig/bfd/elflink.c 2017-04-22 23:31:17.306090155 +0100
+++ binutils/bfd/elflink.c 2017-04-22 23:32:51.090814194 +0100
@@ -907,7 +907,7 @@ _bfd_elf_link_omit_section_dynsym (bfd *
allocated local dynamic syms, followed by the rest of the global
symbols. */
-static unsigned long
+unsigned long
_bfd_elf_link_renumber_dynsyms (bfd *output_bfd,
struct bfd_link_info *info,
unsigned long *section_sym_count)
Index: binutils/bfd/elfxx-mips.c
===================================================================
--- binutils.orig/bfd/elfxx-mips.c 2017-04-22 22:57:23.000000000 +0100
+++ binutils/bfd/elfxx-mips.c 2017-04-22 23:46:58.332082902 +0100
@@ -9267,10 +9267,25 @@ _bfd_mips_elf_always_size_sections (bfd
asection *sect;
struct mips_elf_link_hash_table *htab;
struct mips_htab_traverse_info hti;
+ unsigned long section_sym_count;
htab = mips_elf_hash_table (info);
BFD_ASSERT (htab != NULL);
+ /* In the case of linking a static binary if symbols have been removed
+ as a result of section garbage, then reassign dynsym indices. This
+ would have been done in our caller `bfd_elf_size_dynamic_sections',
+ however most targets do not need this step for static binaries and
+ therefore the call to `_bfd_elf_link_renumber_dynsyms' is not made
+ there in that case.
+
+ We need dynamic symbol counts to lay out GOT, which will be
+ produced in the presence of GOT relocations even in static
+ binaries (holding fixed data in that case, to satisfy those
+ relocations), so we do this here instead. */
+ if (!htab->root.dynamic_sections_created && info->gc_sections)
+ _bfd_elf_link_renumber_dynsyms (output_bfd, info, §ion_sym_count);
+
/* The .reginfo section has a fixed size. */
sect = bfd_get_section_by_name (output_bfd, ".reginfo");
if (sect != NULL)
Index: binutils/ld/emultempl/mipself.em
===================================================================
--- binutils.orig/ld/emultempl/mipself.em 2017-04-22 22:57:25.303526473 +0100
+++ binutils/ld/emultempl/mipself.em 2017-04-22 23:32:51.127204842 +0100
@@ -214,6 +214,7 @@ mips_create_output_section_statements (v
static void
mips_before_allocation (void)
{
+ unsigned long section_sym_count;
flagword flags;
flags = elf_elfheader (link_info.output_bfd)->e_flags;
@@ -223,6 +224,22 @@ mips_before_allocation (void)
_bfd_mips_elf_use_plts_and_copy_relocs (&link_info);
gld${EMULATION_NAME}_before_allocation ();
+
+ /* Assign dynsym indices in the case of linking a static binary.
+ This would normally be done in \`bfd_elf_size_dynsym_hash_dynstr'
+ called from \`gld${EMULATION_NAME}_before_allocation' above,
+ however most targets do not need this step for static binaries
+ and therefore the call to \`_bfd_elf_link_renumber_dynsyms' is
+ not made there in that case.
+
+ We need dynamic symbol counts to lay out GOT, which will be
+ produced in the presence of GOT relocations even in static
+ binaries (holding fixed data in that case, to satisfy those
+ relocations), so we do this here instead. */
+ if (is_elf_hash_table (link_info.hash)
+ && !elf_hash_table (&link_info)->dynamic_sections_created)
+ _bfd_elf_link_renumber_dynsyms (link_info.output_bfd, &link_info,
+ §ion_sym_count);
}
/* Avoid processing the fake stub_file in vercheck, stat_needed and
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-04-24 15:21 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-11 23:16 [PATCH] PR ld/21334: Always assign dynsym indices even in static binaries Maciej W. Rozycki
2017-04-20 16:03 ` [PING][PATCH] " Maciej W. Rozycki
2017-04-21 10:14 ` Alan Modra
2017-04-24 15:21 ` 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).