[RFA:] fix linker deallocation bug: link_info.hash deallocated too soon

[Hans-Peter Nilsson <hp at bitrange dot com>]

This bug is fallout from the free-sprinkling-spree of December 2012.
The bug is premature freeing of the link_info.hash table.  See the
comments in the patch.  It affects only "weird" non-ELF backends.

A BFD target is allowed to hold on to stuff, e.g. symbols until its
_bfd_elf_write_object_contents method, called at time of bfd_close.
The "mmo" target does this, as the position for symbols depend on the
size of all other file contents including escape codes (the "mmo"
format is stream- or record-like, like ihex, not mapped like ELF and
without record length information, hence escape codes).  ELF targets
don't see the bug as they don't output "regular" symbols through
_bfd_elf_write_object_contents.  There is no alternative BFD backend
function where contents could be emitted, and frankly I'd rather fix
BFD-usage bugs in the linker than change the BFD API or silently use
another BFD backend function in a sequence not required by the BFD
documentation.

I noticed this through massive regressions for mmix-knuth-mmixware on
gcc test-results, where the bug caused invalid symbols (empty ones and
duplicate ones) for which the mmo symbol-writer code aborted.  With a
binutils test-run doctored to call valgrind, valgrind somewhat
surprisingly still showed nothing for the ordinary tests!  Apparently
--wrap is required to expose the problem, and --wrap is only tested
for ELF targets in the linker test-suite (and only for native targets
and requires a compiler, so those test-cases aren't easily adopted to
a wider audience).

I added four test-cases alas MMIX-specific, as I had trouble coming up
with a generic clean test-case.  (N.B.: only ld-mmix/wrap1 trigs the
bug; there's also a elf64mmix one and two dummies; variants that just
produce the same binary without --wrap).  Valgrind is still required
to spot the bug.  Only a truly big test-case (at least 6647 of the gcc
test-cases) requires memory allocation in the mmo target such that the
symbol table info is corrupted making the bug visible without
valgrind-like tools - and still not enough to crash the linker.  Maybe
a plugin is required too, but an initial wild-goose chase found only
that it was not the cause of the bug (I should have used valgrind
at once as that pointed out the bug, but plugins was a "usual suspect").

At first I tried plainly moving the bfd_link_hash_table_free call into
ld_cleanup, but valgrind alerted me that bfd_link_hash_table_free
needs to deference the (deallocated) output_bfd.  The hash_table_free
function is "virtual"; it needs to dereference the BFD xvec "vtable".

Alternative solution 1: add two new BFD functions that together
perform bfd_close; i.e. split out the bfd deallocation such that
link_info.output_bfd is still valid as an parameter to BFD functions.
I'm on the fence on this one, but it'd still require moving the
link_info.hash deallocation to later - and the below is the "cleanest"
move as opposed to moving it to ld_cleanup mentioned above, which
would move it further away from the context where it was created.

Alternative solution 2: don't call bfd_link_hash_table_free at all.
But that'd be counter to adding it in the first place and would
require adding a comment in lang_finish so it won't happen again.  Too
much trouble! :-P  Oh, and we wouldn't get a clean no-leak bill from
whatever leak checkers people like to run on binutils.

Tested mmix-knuth-mmixware and cris-elf.
(Also ran the gcc test-suite for mmix-knuth-mmixware which now shows
sane results; as sane as with a binutils before December 2012.)

Ok to commit?

ld:
	* ldlang.c (lang_finish_atexit): New function.
	(output_bfd_hash_table_free_fn): New variable.
	(lang_finish): Arrange to call lang_finish_atexit at exit-time
	to free link_info.hash.

ld/testsuite:
	* ld-mmix/wrap1.d, ld-mmix/wrap1a.s, ld-mmix/wrap1b.s,
	ld-mmix/wrap1c.s, ld-mmix/wrap2.d, ld-mmix/wrap3.d,
	ld-mmix/wrap3a.s, ld-mmix/wrap3b.s, ld-mmix/wrap4.d: New
	tests.

diff --git a/ld/ldlang.c b/ld/ldlang.c
index d147ee0..c612294 100644
--- a/ld/ldlang.c
+++ b/ld/ldlang.c
@@ -1234,10 +1234,32 @@ lang_init (void)
   asneeded_list_tail = &asneeded_list_head;
 }

+static void (*output_bfd_hash_table_free_fn) (struct bfd_link_hash_table *);
+
+/* Helper for lang_finish: complete deferred deallocation.  */
+
+static void
+lang_finish_atexit (void)
+{
+  (*output_bfd_hash_table_free_fn) (link_info.hash);
+}
+
 void
 lang_finish (void)
 {
-  bfd_link_hash_table_free (link_info.output_bfd, link_info.hash);
+  /* We want to please memory leak checkers by deleting
+     link_info.hash.  We can't do it now, as a bfd target may hold
+     references to symbols in this table and use them when their
+     _bfd_write_contents function is invoked, as part of bfd_close on
+     the output_bfd.  But, output_bfd is deallocated at bfd_close, so
+     we can't refer to output_bfd after that time, and dereferencing
+     it is needed to call "bfd_link_hash_table_free".  Smash this
+     dependency deadlock and grab the function pointer; arrange to
+     call it on link_info.hash before exiting.  */
+  output_bfd_hash_table_free_fn
+    = link_info.output_bfd->xvec->_bfd_link_hash_table_free;
+  xatexit (lang_finish_atexit);
+
   bfd_hash_table_free (&lang_definedness_table);
   output_section_statement_table_free ();
 }

diff --git a/ld/testsuite/ld-mmix/wrap1.d b/ld/testsuite/ld-mmix/wrap1.d
new file mode 100644
index 0000000..02d7bef
--- /dev/null
+++ b/ld/testsuite/ld-mmix/wrap1.d
@@ -0,0 +1,21 @@
+#source: start.s
+#source: wrap1a.s
+#source: wrap1b.s
+#source: wrap1c.s
+#ld: -m mmo --wrap deal
+#as: -no-expand
+#objdump: -d
+
+.*:     file format mmo
+
+Disassembly of section \.text:
+
+0+ <(_start|Main)>:
+   0:	e3fd0001 	setl \$253,0x1
+   4:	f2000001 	pushj \$0,8 <__wrap_deal>
+
+0+8 <__wrap_deal>:
+   8:	f0000001 	jmp c <deal>
+
+0+c <deal>:
+   c:	fd000000 	swym 0,0,0
diff --git a/ld/testsuite/ld-mmix/wrap1a.s b/ld/testsuite/ld-mmix/wrap1a.s
new file mode 100644
index 0000000..88a5cd2
--- /dev/null
+++ b/ld/testsuite/ld-mmix/wrap1a.s
@@ -0,0 +1,2 @@
+ .text
+ pushj $0,deal
diff --git a/ld/testsuite/ld-mmix/wrap1b.s b/ld/testsuite/ld-mmix/wrap1b.s
new file mode 100644
index 0000000..367aea0
--- /dev/null
+++ b/ld/testsuite/ld-mmix/wrap1b.s
@@ -0,0 +1,4 @@
+ .text
+ .globl __wrap_deal
+__wrap_deal:
+ jmp __real_deal
diff --git a/ld/testsuite/ld-mmix/wrap1c.s b/ld/testsuite/ld-mmix/wrap1c.s
new file mode 100644
index 0000000..a7678d4
--- /dev/null
+++ b/ld/testsuite/ld-mmix/wrap1c.s
@@ -0,0 +1,4 @@
+ .text
+ .globl deal
+deal:
+ swym 0
diff --git a/ld/testsuite/ld-mmix/wrap2.d b/ld/testsuite/ld-mmix/wrap2.d
new file mode 100644
index 0000000..49b4d3b
--- /dev/null
+++ b/ld/testsuite/ld-mmix/wrap2.d
@@ -0,0 +1,21 @@
+#source: start.s
+#source: wrap1a.s
+#source: wrap1b.s
+#source: wrap1c.s
+#ld: -m elf64mmix --wrap deal
+#as: -no-expand
+#objdump: -d
+
+.*:     file format elf64-mmix
+
+Disassembly of section \.text:
+
+0+ <(_start|Main)>:
+   0:	e3fd0001 	setl \$253,0x1
+   4:	f2000001 	pushj \$0,8 <__wrap_deal>
+
+0+8 <__wrap_deal>:
+   8:	f0000001 	jmp c <deal>
+
+0+c <deal>:
+   c:	fd000000 	swym 0,0,0
diff --git a/ld/testsuite/ld-mmix/wrap3.d b/ld/testsuite/ld-mmix/wrap3.d
new file mode 100644
index 0000000..80b20f1
--- /dev/null
+++ b/ld/testsuite/ld-mmix/wrap3.d
@@ -0,0 +1,21 @@
+#source: start.s
+#source: wrap3a.s
+#source: wrap3b.s
+#source: wrap1c.s
+#ld: -m mmo
+#as: -no-expand
+#objdump: -d
+
+.*:     file format mmo
+
+Disassembly of section \.text:
+
+0+ <(_start|Main)>:
+   0:	e3fd0001 	setl \$253,0x1
+   4:	f2000001 	pushj \$0,8 <__wrap_deal>
+
+0+8 <__wrap_deal>:
+   8:	f0000001 	jmp c <deal>
+
+0+c <deal>:
+   c:	fd000000 	swym 0,0,0
diff --git a/ld/testsuite/ld-mmix/wrap3a.s b/ld/testsuite/ld-mmix/wrap3a.s
new file mode 100644
index 0000000..7192a93
--- /dev/null
+++ b/ld/testsuite/ld-mmix/wrap3a.s
@@ -0,0 +1,2 @@
+ .text
+ pushj $0,__wrap_deal
diff --git a/ld/testsuite/ld-mmix/wrap3b.s b/ld/testsuite/ld-mmix/wrap3b.s
new file mode 100644
index 0000000..6a8a606
--- /dev/null
+++ b/ld/testsuite/ld-mmix/wrap3b.s
@@ -0,0 +1,4 @@
+ .text
+ .globl __wrap_deal
+__wrap_deal:
+ jmp deal
diff --git a/ld/testsuite/ld-mmix/wrap4.d b/ld/testsuite/ld-mmix/wrap4.d
new file mode 100644
index 0000000..a64578d
--- /dev/null
+++ b/ld/testsuite/ld-mmix/wrap4.d
@@ -0,0 +1,21 @@
+#source: start.s
+#source: wrap3a.s
+#source: wrap3b.s
+#source: wrap1c.s
+#ld: -m elf64mmix
+#as: -no-expand
+#objdump: -d
+
+.*:     file format elf64-mmix
+
+Disassembly of section \.text:
+
+0+ <(_start|Main)>:
+   0:	e3fd0001 	setl \$253,0x1
+   4:	f2000001 	pushj \$0,8 <__wrap_deal>
+
+0+8 <__wrap_deal>:
+   8:	f0000001 	jmp c <deal>
+
+0+c <deal>:
+   c:	fd000000 	swym 0,0,0

brgds, H-P