[PATCH] PR ld/20828: Fix linker script symbols wrongly forced local with section GC

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

Fix a generic ELF linker regression introduced with a chain of changes 
made to unused input section garbage collection:

- commit 1a766c6843ce ("Also hide symbols without PLT nor GOT 
  references."), 
  <https://sourceware.org/ml/binutils/2011-09/msg00076.html>,

- commit 1d5316ab67e1 ("PR ld/13177: garbage collector retains zombie 
  references to external libraries"), 
  <https://sourceware.org/ml/binutils/2011-10/msg00161.html>,

- commit 6673f753c019 ("Fix PR 12772, garbage collection of dynamic 
  syms"), <https://sourceware.org/ml/binutils/2011-12/msg00077.html>,

causing the garbage collection of unused symbols present in a DSO 
involved in a link to make identically named symbols ordinarily defined 
(i.e. not hidden or PROVIDEd) by a linker script local, even though the 
latter symbols are supposed to be global as if no DSO defined them as 
well.

This is because linker script assignments are processed very late as 
`lang_process' proceeds, down in the call to `ldemul_before_allocation', 
which is made after the call to `lang_gc_sections' to do input section 
garbage collecting.  Consequently if unused, then any such DSO-defined 
symbol has already been garbage-collected and internally marked local. 
It would ordinarily be removed from dynamic symbol table output, however 
a linker script assignment correctly replaces its original definition 
with the new one and enters it into the dynamic symbol table produced as 
it is supposed to be exported.  The original local marking is however 
retained making the symbol local in the dynamic symbol table and 
therefore not available externally.  This also causes a sorting problem 
with the MIPS target, which does not expect non-section local dynamic 
symbols to be output and produces an invalid binary.

Fix the problem then, by removing the `forced_local' marking for the 
offending case and add suitable test cases.  First to verify that unused 
symbols ordinarily defined with linker script assignments remain 
exported in the context of input section garbage collection whether or 
not a DSO defining identically named symbols is present in the link.  
Second that a linker version script still correctly retains or removes 
such symbols as requested.

	bfd/
	PR ld/20828
	* elflink.c (bfd_elf_record_link_assignment): Clear any 
	`forced_local' marking for DSO symbols that are not being 
	provided.

	ld/
	PR ld/20828
	* testsuite/ld-elf/pr20828-1.sd: New test.
	* testsuite/ld-elf/pr20828-2a.sd: New test.
	* testsuite/ld-elf/pr20828-2b.sd: New test.
	* testsuite/ld-elf/pr20828.ld: New test linker script.
	* testsuite/ld-elf/pr20828.ver: New test version script.
	* testsuite/ld-elf/pr20828.s: New test source.
	* testsuite/ld-elf/shared.exp: Run the new test.
---
 I do hope the patch description clearly indicates the bug conditions; if 
you have any questions or concerns, or you think additional details need 
to be included, then please shout.

 I have some further notes on the test cases, which do not belong in the 
patch description.

 First, I have chosen to place them in ld-elf/shared.exp rather than 
ld-elf/elf.exp as these are strictly shared-library target tests and 
therefore the former script seems more appropriate to me, especially with 
its recent clean-ups in place, than the latter one, where we have already 
accumulated a couple of shared library tests.  Perhaps they could actually 
be moved over too.

 Second, there are no regressions with this change in place across my 
usual set of targets, counting 164 right now, and the new test case scores 
progressions across 53 of them.  Detailed target lists are available upon 
request.

 There are however 4 targets that still score failures with the fix in 
place and the actual results make me believe that these are unrelated 
issues with individual backends.  I encourage target maintainers to look 
into these issues to decide whether they want to have them fixed or to 
have the test cases excluded.  These targets and issues are as follows:

- `bfin-elf', `mn10300-elf', `sh64-elf':

  FAIL: PR ld/20828 dynamic symbols with section GC (version script)

  -- both `_fdata' and `_edata' are made local and removed from the 
  dynamic symbol table in a link with the `pr20828.ver' version script,
  where `_edata' is supported to be exported, making it a bug in version 
  script handling, but these are bare metal ELF targets so the bug could 
  instead be papered over by claiming no shared library support and 
  excluding these targets from the `check_shared_lib_support' list,

- `score-elf':

  FAIL: PR ld/20828 dynamic symbols with section GC (version script)

  -- conversely, both `_fdata' and `_edata' are made global in a link with 
  the `pr20828.ver' version script, where `_fdata' is supported to be 
  local and removed from the dynamic symbol table, making it a bug in 
  version script handling too, but again this is a bare metal ELF target, 
  so the observation above applies.

NB there appears to be no previous plain linker test case in our test 
suite covering linker version script, so many targets may have been given 
no testing of this feature and the two former failures above may well be 
genuine issues worth looking into.  The only test cases I could find are 
either compiler driven and therefore never run for some targets, or 
target-specific.

 I think these test result anomalies can be handled separately, and 
therefore is it OK to apply this change to master and 2.28?

  Maciej

binutils-bfd-elf-link-assignment-forced-local.diff
Index: binutils/bfd/elflink.c
===================================================================
--- binutils.orig/bfd/elflink.c	2017-01-16 19:07:44.570301078 +0000
+++ binutils/bfd/elflink.c	2017-01-16 19:16:34.662138245 +0000
@@ -671,12 +671,17 @@ bfd_elf_record_link_assignment (bfd *out
 
   /* If this symbol is not being provided by the linker script, and it is
      currently defined by a dynamic object, but not by a regular object,
-     then clear out any version information because the symbol will not be
-     associated with the dynamic object any more.  */
+     then undo any forced local marking that may have been set by input
+     section garbage collection and clear out any version information
+     because the symbol will not be associated with the dynamic object
+     any more.  */
   if (!provide
       && h->def_dynamic
       && !h->def_regular)
-    h->verinfo.verdef = NULL;
+    {
+      h->forced_local = 0;
+      h->verinfo.verdef = NULL;
+    }
 
   h->def_regular = 1;
 
Index: binutils/ld/testsuite/ld-elf/pr20828-1.sd
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils/ld/testsuite/ld-elf/pr20828-1.sd	2017-01-16 19:16:45.652286278 +0000
@@ -0,0 +1,12 @@
+# Make sure symbols are global rather than local in the dynamic symbol table,
+# e.g.:
+#    Num:    Value  Size Type    Bind   Vis      Ndx Name
+#      1: 00000000     0 NOTYPE  GLOBAL DEFAULT    1 _fdata
+#      2: 00000000     0 NOTYPE  GLOBAL DEFAULT    1 _edata
+# vs:
+#      1: 00000000     0 NOTYPE  LOCAL  DEFAULT    1 _fdata
+#      2: 00000000     0 NOTYPE  LOCAL  DEFAULT    1 _edata
+#...
+ *[0-9]+: +[0-9a-f]+ +0 +NOTYPE +GLOBAL +DEFAULT +[0-9]+ +_fdata
+ *[0-9]+: +[0-9a-f]+ +0 +NOTYPE +GLOBAL +DEFAULT +[0-9]+ +_edata
+#pass
Index: binutils/ld/testsuite/ld-elf/pr20828-2a.sd
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils/ld/testsuite/ld-elf/pr20828-2a.sd	2017-01-16 19:16:45.671636162 +0000
@@ -0,0 +1,9 @@
+# Make sure `_edata' is global rather than local in the dynamic symbol table,
+# e.g.:
+#    Num:    Value  Size Type    Bind   Vis      Ndx Name
+#      1: 00000000     0 NOTYPE  GLOBAL DEFAULT    1 _edata
+# vs:
+#      1: 00000000     0 NOTYPE  LOCAL  DEFAULT    1 _edata
+#...
+ *[0-9]+: +[0-9a-f]+ +0 +NOTYPE +GLOBAL +DEFAULT +[0-9]+ +_edata
+#pass
Index: binutils/ld/testsuite/ld-elf/pr20828-2b.sd
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils/ld/testsuite/ld-elf/pr20828-2b.sd	2017-01-16 19:16:45.686849968 +0000
@@ -0,0 +1,7 @@
+# Make sure no `_fdata' is present in the dynamic symbol table, e.g.:
+#    Num:    Value  Size Type    Bind   Vis      Ndx Name
+#      1: 00000000     0 NOTYPE  LOCAL  DEFAULT    1 _fdata
+#failif
+#...
+.+ +_fdata
+#pass
Index: binutils/ld/testsuite/ld-elf/pr20828.ld
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils/ld/testsuite/ld-elf/pr20828.ld	2017-01-16 19:16:45.696030026 +0000
@@ -0,0 +1,19 @@
+SECTIONS
+{
+  .data :
+  {
+    _fdata = .;
+    *(.data)
+    _edata = .;
+  }
+  .dynamic : { *(.dynamic) }
+  .hash : { *(.hash) }
+  .dynsym : { *(.dynsym) }
+  .dynstr : { *(.dynstr) }
+  .shstrtab : { *(.shstrtab) }
+  .symtab : { *(.symtab) }
+  .strtab : { *(.strtab) }
+  .got.plt : { *(.got.plt) }
+  .got : { *(.got) }
+  /DISCARD/ : { *(*) }
+}
Index: binutils/ld/testsuite/ld-elf/pr20828.s
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils/ld/testsuite/ld-elf/pr20828.s	2017-01-16 19:16:45.709258749 +0000
@@ -0,0 +1,2 @@
+	.data
+	.byte	0
Index: binutils/ld/testsuite/ld-elf/pr20828.ver
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils/ld/testsuite/ld-elf/pr20828.ver	2017-01-16 19:16:45.722480544 +0000
@@ -0,0 +1 @@
+{ global: _edata; local: *; };
Index: binutils/ld/testsuite/ld-elf/shared.exp
===================================================================
--- binutils.orig/ld/testsuite/ld-elf/shared.exp	2017-01-16 19:13:15.079854497 +0000
+++ binutils/ld/testsuite/ld-elf/shared.exp	2017-01-16 20:00:54.268456951 +0000
@@ -31,6 +31,53 @@ if ![check_shared_lib_support] {
     return
 }
 
+# This target requires extra GAS options when building code for shared
+# libraries.
+set AFLAGS_PIC ""
+if [istarget "tic6x-*-*"] {
+    append AFLAGS_PIC " -mpic -mpid=near"
+}
+# This target requires a non-default emulation for successful shared
+# library/executable builds.
+set LFLAGS ""
+if [istarget "tic6x-*-*"] {
+    append LFLAGS " -melf32_tic6x_le"
+}
+
+# PR ld/20828 check for correct dynamic symbol table entries where:
+# - symbols have been defined with a linker script,
+# - the same symbols have been seen in shared library used in the link,
+# - the shared library symbols have been swept in section garbage collection.
+# Verify that the symbols are global rather than local and that a version
+# script adjusts them accordingly.
+if { [check_gc_sections_available] } {
+    run_ld_link_tests [list \
+	[list \
+	    "PR ld/20828 dynamic symbols with section GC\
+	      (auxiliary shared library)" \
+	     "$LFLAGS -shared --gc-sections -T pr20828.ld" "" "$AFLAGS_PIC" \
+	     {pr20828.s} \
+	     {{readelf --dyn-syms pr20828-1.sd}} \
+	     "libpr20828.so"] \
+	[list \
+	    "PR ld/20828 dynamic symbols with section GC (plain)" \
+	     "$LFLAGS -shared --gc-sections -T pr20828.ld" \
+	     "tmpdir/libpr20828.so" "$AFLAGS_PIC" \
+	     {pr20828.s} \
+	     {{readelf --dyn-syms pr20828-1.sd}} \
+	     "pr20828-1.so"] \
+	[list \
+	    "PR ld/20828 dynamic symbols with section GC (version script)" \
+	     "$LFLAGS -shared --gc-sections -T pr20828.ld\
+	      --version-script=pr20828.ver" \
+	     "tmpdir/libpr20828.so" \
+	     "$AFLAGS_PIC" \
+	     {pr20828.s} \
+	     {{readelf --dyn-syms pr20828-2a.sd} \
+	      {readelf --dyn-syms pr20828-2b.sd}} \
+	     "pr20828-2.so"]]
+}
+
 # Check to see if the C compiler works
 if { [which $CC] == 0 } {
     return