This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] PR ld/20828: Move symbol version processing ahead of GC symbol sweep
On Wed, 15 Feb 2017, Alan Modra wrote:
> > ... I can't reproduce this...
>
> valgrind reports uninitialized writes, so some luck is involved in
> getting the test to fail.
>
> If I put MALLOC_PERTURB_=1 in my environment then the test passes but
> readelf -s --wide tmpdir/rdynamic-1 shows:
>
> Symbol table '.dynsym' contains 18 entries:
> Num: Value Size Type Bind Vis Ndx Name
> 0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND
> 1: 0000000000000000 0 FUNC GLOBAL DEFAULT UND __libc_start_main@GLIBC_2.2.5 (2)
> 2: 0000000000000000 0 NOTYPE WEAK DEFAULT UND __gmon_start__
> 3: 0000000000601024 0 NOTYPE GLOBAL DEFAULT 25 _edata
> 4: 0000000000601020 0 NOTYPE GLOBAL DEFAULT 25 __data_start
> 5: fefefefefefefefe 0xfefefefefefefefe <processor specific>: 14 <processor specific>: 15 HIDDEN [<other>: fc] bad section index[65278] <corrupt>
> 6: 0000000000601028 0 NOTYPE GLOBAL DEFAULT 26 _end
> 7: 0000000000601020 0 NOTYPE WEAK DEFAULT 25 data_start
> 8: 00000000004007e0 4 OBJECT GLOBAL DEFAULT 16 _IO_stdin_used
> 9: 0000000000400760 101 FUNC GLOBAL DEFAULT 14 __libc_csu_init
> 10: 0000000000400650 42 FUNC GLOBAL DEFAULT 14 _start
> 11: 0000000000601024 0 NOTYPE GLOBAL DEFAULT 26 __bss_start
> 12: 0000000000400640 3 FUNC GLOBAL DEFAULT 14 main
> 13: fefefefefefefefe 0xfefefefefefefefe <processor specific>: 14 <processor specific>: 15 HIDDEN [<other>: fc] bad section index[65278] <corrupt>
> 14: 00000000004005f0 0 FUNC GLOBAL DEFAULT 11 _init
> 15: 00000000004007d0 2 FUNC GLOBAL DEFAULT 14 __libc_csu_fini
> 16: 00000000004007d4 0 FUNC GLOBAL DEFAULT 15 _fini
> 17: 0000000000400750 2 FUNC GLOBAL DEFAULT 14 rdynamic
This must be something specific to your environment (or mine), which is
an obvious shortcoming (or, depending on how you look at it, a benefit) of
compiled tests -- in that results will inevitably depend on external
components such as target libc included in the build of individual test
cases (the shortcoming is the lack of universal reproducibility, while the
benefit is the diversity of test environments giving a higher chance to
hit an issue if one is there).
Or to put it short I still cannot reproduce it either way, and the dynsym
table always looks the same here, regardless of whether my original patch
has been applied or not, and if so, then if any of the updates has, as
follows:
Symbol table '.dynsym' contains 16 entries:
Num: Value Size Type Bind Vis Ndx Name
0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND
1: 0000000000000000 0 FUNC GLOBAL DEFAULT UND __libc_start_main@GLIBC_2.2.5 (2)
2: 0000000000000000 0 NOTYPE WEAK DEFAULT UND __gmon_start__
3: 0000000000601024 0 NOTYPE GLOBAL DEFAULT 25 _edata
4: 0000000000601020 0 NOTYPE GLOBAL DEFAULT 25 __data_start
5: 0000000000601028 0 NOTYPE GLOBAL DEFAULT 26 _end
6: 0000000000601020 0 NOTYPE WEAK DEFAULT 25 data_start
7: 0000000000400750 4 OBJECT GLOBAL DEFAULT 16 _IO_stdin_used
8: 00000000004006d0 101 FUNC GLOBAL DEFAULT 14 __libc_csu_init
9: 00000000004005d3 0 FUNC GLOBAL DEFAULT 14 _start
10: 0000000000601024 0 NOTYPE GLOBAL DEFAULT 26 __bss_start
11: 00000000004005d0 3 FUNC GLOBAL DEFAULT 14 main
12: 0000000000400580 0 FUNC GLOBAL DEFAULT 11 _init
13: 0000000000400740 2 FUNC GLOBAL DEFAULT 14 __libc_csu_fini
14: 0000000000400744 0 FUNC GLOBAL DEFAULT 15 _fini
15: 00000000004006c0 2 FUNC GLOBAL DEFAULT 14 rdynamic
That said however all 3 regressions have the use of `-export-dynamic' in
common, so I have used the other two I could get to trigger here as
reference tests and tracked the issue down to a piece of code in
`bfd_elf_size_dynamic_sections' that I previously missed, which handles
the option and symbols that would otherwise remain local to the dynamic
symbol table. These symbols obviously have to be passed through version
processing, so they have to be exported before linker versioning is
applied.
By the same reasoning I have shown in the previous message these symbols
will not be garbage-collected, or they wouldn't have been candidates for
exporting in the first place -- as indicated in `_bfd_elf_export_symbol'
they need to be either locally defined or locally referenced. So it is
safe AFAICT to have the symbol sweep pass executed after the forciful
symbol export loop, and therefore the loop can be moved ahead of version
processing and hence also the symbol sweep pass, while still satisfying
the requirements of both.
So here's an updated update, which removes the:
FAIL: vers4a
FAIL: vers9
regressions for me, and hopefully also your:
FAIL: Build rdynamic-1
(please verify that for me).
For the purpose of further review and for the avoidance of doubt as to
what my actual proposed change is right now, I'll post v2 of the original
change with all the amendments applied. It'll include a further test case
reduced from `vers4a' too, which does not rely on the presence of a
compiler (and which I yet need to actually push through my usual testing).
Thanks for your input!
Maciej
binutils-bfd-elf-size-dynamic-sections-version-update-3.diff
Index: binutils/bfd/elflink.c
===================================================================
--- binutils.orig/bfd/elflink.c 2017-02-17 23:27:33.275425647 +0000
+++ binutils/bfd/elflink.c 2017-02-17 23:46:30.128370517 +0000
@@ -5938,13 +5938,28 @@ bfd_elf_size_dynamic_sections (bfd *outp
if (dynobj != NULL && elf_hash_table (info)->dynamic_sections_created)
{
struct bfd_elf_version_tree *verdefs;
- unsigned long section_sym_count;
struct elf_info_failed asvinfo;
struct bfd_elf_version_tree *t;
struct bfd_elf_version_expr *d;
+ struct elf_info_failed eif;
bfd_boolean all_defined;
asection *s;
+ eif.info = info;
+ eif.failed = FALSE;
+
+ /* If we are supposed to export all symbols into the dynamic symbol
+ table (this is not the normal case), then do so. */
+ if (info->export_dynamic
+ || (bfd_link_executable (info) && info->dynamic))
+ {
+ elf_link_hash_traverse (elf_hash_table (info),
+ _bfd_elf_export_symbol,
+ &eif);
+ if (eif.failed)
+ return FALSE;
+ }
+
/* Make all global versions with definition. */
for (t = info->version_info; t != NULL; t = t->next)
for (d = t->globals.list; d != NULL; d = d->next)
@@ -6368,15 +6383,6 @@ bfd_elf_size_dynamic_sections (bfd *outp
elf_tdata (output_bfd)->cverrefs = crefs;
}
}
-
- if ((elf_tdata (output_bfd)->cverrefs == 0
- && elf_tdata (output_bfd)->cverdefs == 0)
- || _bfd_elf_link_renumber_dynsyms (output_bfd, info,
- §ion_sym_count) == 0)
- {
- s = bfd_get_linker_section (dynobj, ".gnu.version");
- s->flags |= SEC_EXCLUDE;
- }
}
bed = get_elf_backend_data (output_bfd);
@@ -6545,18 +6551,6 @@ bfd_elf_size_dynamic_sections (bfd *outp
eif.info = info;
eif.failed = FALSE;
- /* If we are supposed to export all symbols into the dynamic symbol
- table (this is not the normal case), then do so. */
- if (info->export_dynamic
- || (bfd_link_executable (info) && info->dynamic))
- {
- elf_link_hash_traverse (elf_hash_table (info),
- _bfd_elf_export_symbol,
- &eif);
- if (eif.failed)
- return FALSE;
- }
-
/* Find all symbols which were defined in a dynamic object and make
the backend pick a reasonable value for them. */
elf_link_hash_traverse (elf_hash_table (info),
@@ -6676,6 +6670,8 @@ bfd_elf_size_dynamic_sections (bfd *outp
if (dynobj != NULL && elf_hash_table (info)->dynamic_sections_created)
{
+ unsigned long section_sym_count;
+
if (elf_tdata (output_bfd)->cverdefs)
{
unsigned int crefs = elf_tdata (output_bfd)->cverdefs;
@@ -6714,6 +6710,17 @@ bfd_elf_size_dynamic_sections (bfd *outp
|| !_bfd_elf_add_dynamic_entry (info, DT_VERNEEDNUM, crefs))
return FALSE;
}
+
+ if ((elf_tdata (output_bfd)->cverrefs == 0
+ && elf_tdata (output_bfd)->cverdefs == 0)
+ || _bfd_elf_link_renumber_dynsyms (output_bfd, info,
+ §ion_sym_count) == 0)
+ {
+ asection *s;
+
+ s = bfd_get_linker_section (dynobj, ".gnu.version");
+ s->flags |= SEC_EXCLUDE;
+ }
}
return TRUE;
}