[PATCH] VAX/BFD: Fix GOT/PLT handling for non-preemptible symbols

["Maciej W. Rozycki" <macro at linux-mips dot org>]

Hi,

 This change fixes a problem with GOT handling for non-preemptible symbols 
(ones that have a non-default export class aka visibility).  Being 
non-preemptible such symbols do not require a GOT entry, because they 
always resolve locally (there's a small complication around protected 
function symbols where pointer equality is required; as it is handled 
automagically I'm only mentioning it for completeness, the VAX backend 
doesn't have to do anything special here), the VAX architecture provides 
PC-relative addressing for all instructions involving address 
calculations, and the PC-relative addressing mode supports an immediate 
offset that spans the whole address space.

 We currently handle this partially, GOT space is correctly not allocated 
for them, however they are taken into account in GOT offset calculation. 
As a result symbols that actually make it into the GOT may get assigned 
slots beyond the end of the GOT space.  The symptom is an assertion 
failure on R_VAX_GOT32 calculation in elf_vax_relocate_section, at line 
1464:

BFD_ASSERT (off < sgot->size);

-- because off, the offset calculated for the symbol being handled, is 
outside the .got section.  A test case that arranges for this to happen is 
included.

 The change below corrects the problem by removing all the non-default 
export class checks for GOT and PLT relocs scattered throughout the 
backend and relying on elf_vax_instantiate_got_entries setting the 
respective refcounts to -1 for non-preemptible symbols (the 
SYMBOL_REFERENCES_LOCAL macro takes care of both non-preemptible symbols 
and these which have been forced local with a version script).  
Consequently code is removed from elf_vax_relocate_section that allocates 
GOT entries for forced-local symbols, there's no such need and it only 
hits run-time performance.  Complementing code is removed from 
elf_vax_finish_dynamic_symbol that applied R_VAX_RELATIVE relocations to 
such GOT entries.

 The change makes some minor clean-ups as a side effect too.  In 
particular a check for h being NULL is removed from the R_VAX_GOT32 case 
in elf_vax_check_relocs, because we already assert there it's not, and 
contrariwise an assertion that h is NULL is removed from 
elf_vax_relocate_section for R_VAX_GOT32, because we break out of the case 
a few lines above if h is NULL (and similarly for off being unequal to 
-1).  And some conditions are consolidated or moved around for a better 
code structure.

 No regressions for vax-linux or vax-netbsdelf; the test case included 
passes with the change applied.  Questions or comments?  Otherwise OK to 
apply?

2013-05-20  Maciej W. Rozycki  <macro@linux-mips.org>

	bfd/
	* elf32-vax.c (elf_vax_check_relocs) <R_VAX_GOT32, R_VAX_PLT32>:
	Don't check symbol visibility here.  Remove a check already
	asserted for.
	(elf_vax_instantiate_got_entries): Use SYMBOL_REFERENCES_LOCAL
	instead of individual checks.
	(elf_vax_relocate_section) <R_VAX_GOT32, R_VAX_PLT32>: Only
	check the offset to decide if produce a GOT or PLT entry.
	Remove redundant assertions.  Remove code to produce GOT entries
	for local symbols.  Remove a duplicate comment and add a comment 
	on GOT relocations.
	(elf_vax_finish_dynamic_symbol): Remove code to produce RELATIVE 
	dynamic relocs.

	ld/testsuite/
	* ld-vax-elf/got-local-exe.xd: New test.
	* ld-vax-elf/got-local-lib.xd: New test.
	* ld-vax-elf/got-local-aux.s: New test source.
	* ld-vax-elf/got-local-def.s: New test source.
	* ld-vax-elf/got-local-ref.s: New test source.
	* ld-vax-elf/vax-elf.exp: Run the new tests.

  Maciej

binutils-2.23.52-20130506-vax-got-local.patch
Index: binutils/bfd/elf32-vax.c
===================================================================
--- binutils.orig/bfd/elf32-vax.c
+++ binutils/bfd/elf32-vax.c
@@ -595,14 +595,12 @@ elf_vax_check_relocs (bfd *abfd, struct 
 	{
 	case R_VAX_GOT32:
 	  BFD_ASSERT (h != NULL);
-	  if (h->forced_local
-	      || h == elf_hash_table (info)->hgot
-	      || h == elf_hash_table (info)->hplt)
-	    break;
 
 	  /* If this is a local symbol, we resolve it directly without
 	     creating a global offset table entry.  */
-	  if (h == NULL || ELF_ST_VISIBILITY (h->other) != STV_DEFAULT)
+	  if (h->forced_local
+	      || h == elf_hash_table (info)->hgot
+	      || h == elf_hash_table (info)->hplt)
 	    break;
 
 	  /* This symbol requires a global offset table entry.  */
@@ -672,11 +670,11 @@ elf_vax_check_relocs (bfd *abfd, struct 
              never referenced by a dynamic object, in which case we
              don't need to generate a procedure linkage table entry
              after all.  */
+	  BFD_ASSERT (h != NULL);
 
 	  /* If this is a local symbol, we resolve it directly without
 	     creating a procedure linkage table entry.  */
-	  BFD_ASSERT (h != NULL);
-	  if (ELF_ST_VISIBILITY (h->other) != STV_DEFAULT || h->forced_local)
+	  if (h->forced_local)
 	    break;
 
 	  h->needs_plt = 1;
@@ -1321,16 +1319,13 @@ elf_vax_instantiate_got_entries (struct 
   sgot = bfd_get_linker_section (dynobj, ".got");
   srelgot = bfd_get_linker_section (dynobj, ".rela.got");
 
-  if ((info->shared && info->symbolic)
-      || h->forced_local)
+  if (SYMBOL_REFERENCES_LOCAL (info, h))
     {
       h->got.refcount = -1;
       h->plt.refcount = -1;
     }
   else if (h->got.refcount > 0)
     {
-      bfd_boolean dyn;
-
       /* Make sure this symbol is output as a dynamic symbol.  */
       if (h->dynindx == -1)
 	{
@@ -1338,15 +1333,9 @@ elf_vax_instantiate_got_entries (struct 
 	    return FALSE;
 	}
 
-      dyn = elf_hash_table (info)->dynamic_sections_created;
       /* Allocate space in the .got and .rela.got sections.  */
-      if (ELF_ST_VISIBILITY (h->other) == STV_DEFAULT
-	  && (info->shared
-	      || WILL_CALL_FINISH_DYNAMIC_SYMBOL (dyn, 0, h)))
-	{
-	  sgot->size += 4;
-	  srelgot->size += sizeof (Elf32_External_Rela);
-	}
+      sgot->size += 4;
+      srelgot->size += sizeof (Elf32_External_Rela);
     }
 
   return TRUE;
@@ -1471,17 +1460,14 @@ elf_vax_relocate_section (bfd *output_bf
 	case R_VAX_GOT32:
 	  /* Relocation is to the address of the entry for this symbol
 	     in the global offset table.  */
+
+	  /* Resolve a GOTxx reloc against a local symbol directly,
+	     without using the global offset table.  */
 	  if (h == NULL
-	      || ELF_ST_VISIBILITY (h->other) != STV_DEFAULT
-	      || h->got.offset == (bfd_vma) -1
-	      || h->forced_local)
+	      || h->got.offset == (bfd_vma) -1)
 	    break;
 
-	  /* Relocation is the offset of the entry for this symbol in
-	     the global offset table.  */
-
 	  {
-	    bfd_boolean dyn;
 	    bfd_vma off;
 
 	    if (sgot == NULL)
@@ -1490,37 +1476,10 @@ elf_vax_relocate_section (bfd *output_bf
 		BFD_ASSERT (sgot != NULL);
 	      }
 
-	    BFD_ASSERT (h != NULL);
 	    off = h->got.offset;
-	    BFD_ASSERT (off != (bfd_vma) -1);
 	    BFD_ASSERT (off < sgot->size);
 
-	    dyn = elf_hash_table (info)->dynamic_sections_created;
-	    if (! WILL_CALL_FINISH_DYNAMIC_SYMBOL (dyn, info->shared, h)
-		|| (info->shared
-		    && SYMBOL_REFERENCES_LOCAL (info, h)))
-	      {
-		/* The symbol was forced to be local
-		   because of a version file..  We must initialize
-		   this entry in the global offset table.  Since
-		   the offset must always be a multiple of 4, we
-		   use the least significant bit to record whether
-		   we have initialized it already.
-
-		   When doing a dynamic link, we create a .rela.got
-		   relocation entry to initialize the value.  This
-		   is done in the finish_dynamic_symbol routine.  */
-		if ((off & 1) != 0)
-		  off &= ~1;
-		else
-		  {
-		    bfd_put_32 (output_bfd, relocation + rel->r_addend,
-				sgot->contents + off);
-		    h->got.offset |= 1;
-		  }
-	      } else {
-		bfd_put_32 (output_bfd, rel->r_addend, sgot->contents + off);
-	      }
+	    bfd_put_32 (output_bfd, rel->r_addend, sgot->contents + off);
 
 	    relocation = sgot->output_offset + off;
 	    /* The GOT relocation uses the addend.  */
@@ -1547,19 +1506,9 @@ elf_vax_relocate_section (bfd *output_bf
 	  /* Resolve a PLTxx reloc against a local symbol directly,
 	     without using the procedure linkage table.  */
 	  if (h == NULL
-	      || ELF_ST_VISIBILITY (h->other) != STV_DEFAULT
-	      || h->forced_local)
+	      || h->plt.offset == (bfd_vma) -1)
 	    break;
 
-	  if (h->plt.offset == (bfd_vma) -1
-	      || !elf_hash_table (info)->dynamic_sections_created)
-	    {
-	      /* We didn't make a PLT entry for this symbol.  This
-		 happens when statically linking PIC code, or when
-		 using -Bsymbolic.  */
-	      break;
-	    }
-
 	  if (splt == NULL)
 	    {
 	      splt = bfd_get_linker_section (dynobj, ".plt");
@@ -1894,25 +1843,10 @@ elf_vax_finish_dynamic_symbol (bfd *outp
 
       rela.r_offset = (sgot->output_section->vma
 		       + sgot->output_offset
-		       + (h->got.offset &~ 1));
-
-      /* If the symbol was forced to be local because of a version file
-	 locally we just want to emit a RELATIVE reloc.  The entry in
-	 the global offset table will already have been initialized in
-	 the relocate_section function.  */
-      if (info->shared
-	  && h->dynindx == -1
-	  && h->def_regular)
-	{
-	  rela.r_info = ELF32_R_INFO (0, R_VAX_RELATIVE);
-	}
-      else
-	{
-	  rela.r_info = ELF32_R_INFO (h->dynindx, R_VAX_GLOB_DAT);
-	}
+		       + h->got.offset);
+      rela.r_info = ELF32_R_INFO (h->dynindx, R_VAX_GLOB_DAT);
       rela.r_addend = bfd_get_signed_32 (output_bfd,
-					 (sgot->contents
-					  + (h->got.offset & ~1)));
+					 sgot->contents + h->got.offset);
 
       loc = srela->contents;
       loc += srela->reloc_count++ * sizeof (Elf32_External_Rela);
Index: binutils/ld/testsuite/ld-vax-elf/got-local-aux.s
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/got-local-aux.s
@@ -0,0 +1,5 @@
+	.globl	baz
+	.type	baz, @object
+baz:
+	.byte	0
+	.size	baz, . - baz
Index: binutils/ld/testsuite/ld-vax-elf/got-local-def.s
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/got-local-def.s
@@ -0,0 +1,12 @@
+	.data
+	.globl	bar_hidden
+	.type	bar_hidden, @object
+	.hidden	bar_hidden
+bar_hidden:
+	.byte	0
+	.size	bar_hidden, . - bar_hidden
+	.globl	bar_visible
+	.type	bar_visible, @object
+bar_visible:
+	.byte	0
+	.size	bar_visible, . - bar_visible
Index: binutils/ld/testsuite/ld-vax-elf/got-local-exe.xd
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/got-local-exe.xd
@@ -0,0 +1,4 @@
+# Make sure there's only one GOT entry, for the symbol defined externally.
+Hex dump of section '\.got':
+  0x[0-9a-f]+ ........ 00000000 00000000 00000000 .*
+  0x[0-9a-f]+ 00000000                            .*
Index: binutils/ld/testsuite/ld-vax-elf/got-local-lib.xd
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/got-local-lib.xd
@@ -0,0 +1,4 @@
+# Make sure there are only two GOT entries, for the preemptible symbols.
+Hex dump of section '\.got':
+  0x[0-9a-f]+ ........ 00000000 00000000 00000000 .*
+  0x[0-9a-f]+ 00000000 00000000                   .*
Index: binutils/ld/testsuite/ld-vax-elf/got-local-ref.s
===================================================================
--- /dev/null
+++ binutils/ld/testsuite/ld-vax-elf/got-local-ref.s
@@ -0,0 +1,10 @@
+	.text
+	.globl	foo
+	.type	foo, @function
+foo:
+	.word	0
+	movab	bar_hidden, %r0
+	movab	bar_visible, %r1
+	movab	baz, %r2
+	ret
+	.size	foo, . - foo
Index: binutils/ld/testsuite/ld-vax-elf/vax-elf.exp
===================================================================
--- binutils.orig/ld/testsuite/ld-vax-elf/vax-elf.exp
+++ binutils/ld/testsuite/ld-vax-elf/vax-elf.exp
@@ -48,3 +48,37 @@ run_ld_link_tests [list \
 	  { plt-local.s } \
 	  { { objdump -d plt-local.dd } } \
 	  "plt-local"]]
+
+# Global offset table tests.  Make sure hidden symbols do not get GOT
+# assignments.
+run_ld_link_tests [list \
+    [list "GOT test (auxiliary shared library)" \
+	  "-shared" "" \
+	  "-k" \
+	  { got-local-aux.s } \
+	  {} \
+	  "got-local-aux.so"] \
+    [list "GOT test (object 1)" \
+	  "-r" "" \
+	  "-k" \
+	  { got-local-ref.s } \
+	  {} \
+	  "got-local-ref-r.o"] \
+    [list "GOT test (object 2)" \
+	  "-r" "" \
+	  "-k" \
+	  { got-local-def.s } \
+	  {} \
+	  "got-local-def-r.o"] \
+    [list "GOT test (executable)" \
+	  "-e 0 tmpdir/got-local-aux.so tmpdir/got-local-ref-r.o tmpdir/got-local-def-r.o" "" \
+	  "" \
+	  {} \
+	  { { readelf "-x .got" got-local-exe.xd } } \
+	  "got-local-exe"] \
+    [list "GOT test (shared library)" \
+	  "-shared tmpdir/got-local-aux.so tmpdir/got-local-ref-r.o tmpdir/got-local-def-r.o" "" \
+	  "" \
+	  {} \
+	  { { readelf "-x .got" got-local-lib.xd } } \
+	  "got-local-lib.so"]]