[PATCH] SH/BFD: Fix silent LD failure with non-elf32-sh

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

Hi,

 More breakage from this change:

2010-02-03  Nick Clifton  <nickc@redhat.com>

	* elf-bfd.h (emum elf_object_id): Rename to elf_target_id.  Add
	entries for other architectures.
	(struct elf_link_hash_table): Add hash_table_id field.
	(elf_hash_table_id): New accessor macro.
	* elflink.c (_bfd_elf_link_hash_table_init): Add target_id
	parameter.
	* elf-m10300.c (elf32_mn10300_hash_table): Check table id before
	returning cast pointer.
	(elf32_mn10300_link_hash_table_create): Identify new table as
	containing MN10300 extensions.
	(mn10300_elf_relax_section): Check pointer returned by
	elf32_mn10300_hash_table.
	[...]
	* elf32-sh.c: Likewise, except using SH extensions.

It looks like this target only worked by chance with generic hash table 
options.  Seen in the SH LD testsuite like this:

[...]
Executing on host: sh -c {sh-uclinux-ld   -o tmpdir/sh1.s1 -relax --oformat srec tmpdir/sh1.o 2>&1}  /dev/null ld.tmp (timeout = 3600)
sh-uclinux-ld: warning: cannot find entry symbol start; defaulting to 0000000000001000
sh-uclinux-ld: warning: cannot find entry symbol start; defaulting to 0000000000001000
mv tmpdir/sh1.s1 tmpdir/sh1.s2
mv: cannot stat `tmpdir/sh1.s1': No such file or directory
UNRESOLVED: SH relaxing to S-records

The reason is the output BFD is srec and therefore sh_elf_hash_table() 
returns NULL when called from sh_elf_relocate_section().  The latter fails 
without setting a BFD error code and hence the linker exits 
unsuccessfully, but with no clue as to the cause.

 This change removes the failure making things better for a simple case 
like one in the testsuite, but it looks to me like further development 
will be required to support such invocation correctly.  I have sprinkled 
BFD_ASSERT() clauses across the function to mark places where action might 
be required leaving it up to SH maintainers.  This change has passed 
regression testing for sh-uclinux.  Comments?

 While testing this I noticed a missing newline in log output for the test 
case involved.  This change is included below for brevity, but I'll be 
applying it as obviously correct separately.

2010-08-19  Maciej W. Rozycki  <macro@codesourcery.com>

	bfd/
	* elf32-sh.c (sh_elf_relocate_section): Handle non-ELF output
	BFD.

	ld/testsuite/
	* ld-sh/sh.exp: Add missing newline.

 There may be more targets suffering from breakage that's been revealed by 
the change mentioned at the top (which generally means it was a good one); 
e.g. elf32-avr.c being similar to elf32-sh.c in this area seems a likely 
candidate to me.  This is just a heads-up for the respective maintainers 
-- I won't be looking into it any further.

  Maciej

binutils-fsf-2.20.51-20100819-bfd-sh-relocate-section-0.patch
Index: ld/testsuite/ld-sh/sh.exp
===================================================================
--- ld/testsuite/ld-sh/sh.exp	(revision 296366)
+++ ld/testsuite/ld-sh/sh.exp	(working copy)
@@ -88,7 +88,7 @@ if ![ld_simple_link $ld tmpdir/sh1.s1 $s
 	verbose "$exec_output"
 	unresolved $testsrec
     } else {
-	send_log "$objcopy -O srec tmpdir/sh1 tmpdir/sh1.s1"
+	send_log "$objcopy -O srec tmpdir/sh1 tmpdir/sh1.s1\n"
 	verbose "$objcopy -O srec tmpdir/sh1 tmpdir/sh1.s1"
 	catch "exec $objcopy -O srec tmpdir/sh1 tmpdir/sh1.s1" exec_output
 	if ![string match "" $exec_output] {
Index: bfd/elf32-sh.c
===================================================================
--- bfd/elf32-sh.c	(revision 296366)
+++ bfd/elf32-sh.c	(working copy)
@@ -3929,47 +3929,48 @@ sh_elf_relocate_section (bfd *output_bfd
   Elf_Internal_Shdr *symtab_hdr;
   struct elf_link_hash_entry **sym_hashes;
   Elf_Internal_Rela *rel, *relend;
-  bfd *dynobj;
+  bfd *dynobj = NULL;
   bfd_vma *local_got_offsets;
-  asection *sgot;
-  asection *sgotplt;
-  asection *splt;
-  asection *sreloc;
-  asection *srelgot;
+  asection *sgot = NULL;
+  asection *sgotplt = NULL;
+  asection *splt = NULL;
+  asection *sreloc = NULL;
+  asection *srelgot = NULL;
   bfd_boolean is_vxworks_tls;
   unsigned isec_segment, got_segment, plt_segment, check_segment[2];
+  bfd_boolean fdpic_p = FALSE;
 
   BFD_ASSERT (is_sh_elf (input_bfd));
 
   htab = sh_elf_hash_table (info);
-  if (htab == NULL)
-    return FALSE;
+  if (htab != NULL)
+    {
+      dynobj = htab->root.dynobj;
+      sgot = htab->sgot;
+      sgotplt = htab->sgotplt;
+      splt = htab->splt;
+      fdpic_p = htab->fdpic_p;
+    }
   symtab_hdr = &elf_symtab_hdr (input_bfd);
   sym_hashes = elf_sym_hashes (input_bfd);
-  dynobj = htab->root.dynobj;
   local_got_offsets = elf_local_got_offsets (input_bfd);
 
   isec_segment = sh_elf_osec_to_segment (output_bfd,
 					 input_section->output_section);
-  if (htab->fdpic_p && htab->sgot)
+  if (fdpic_p && sgot)
     got_segment = sh_elf_osec_to_segment (output_bfd,
-					  htab->sgot->output_section);
+					  sgot->output_section);
   else
     got_segment = -1;
-  if (htab->fdpic_p && htab->splt)
+  if (fdpic_p && splt)
     plt_segment = sh_elf_osec_to_segment (output_bfd,
-					  htab->splt->output_section);
+					  splt->output_section);
   else
     plt_segment = -1;
 
-  sgot = htab->sgot;
-  sgotplt = htab->sgotplt;
-  splt = htab->splt;
-  sreloc = NULL;
-  srelgot = NULL;
   /* We have to handle relocations in vxworks .tls_vars sections
      specially, because the dynamic loader is 'weird'.  */
-  is_vxworks_tls = (htab->vxworks_p && info->shared
+  is_vxworks_tls = (htab && htab->vxworks_p && info->shared
 		    && !strcmp (input_section->output_section->name,
 				".tls_vars"));
 
@@ -4147,7 +4148,7 @@ sh_elf_relocate_section (bfd *output_bfd
 	    {
 	      bfd_boolean dyn;
 
-	      dyn = htab->root.dynamic_sections_created;
+	      dyn = htab ? htab->root.dynamic_sections_created : FALSE;
 	      sec = h->root.u.def.section;
 	      /* In these cases, we don't need the relocation value.
 		 We check specially because in some obscure cases
@@ -4461,7 +4462,7 @@ sh_elf_relocate_section (bfd *output_bfd
 		  outrel.r_addend = addend;
 		}
 #endif
-	      else if (htab->fdpic_p
+	      else if (fdpic_p
 		       && (h == NULL
 			   || ((info->symbolic || h->dynindx == -1)
 			       && h->def_regular)))
@@ -4515,12 +4516,14 @@ sh_elf_relocate_section (bfd *output_bfd
 	      if (! relocate)
 		continue;
 	    }
-	  else if (htab->fdpic_p && !info->shared
+	  else if (fdpic_p && !info->shared
 		   && r_type == R_SH_DIR32
 		   && (input_section->flags & SEC_ALLOC) != 0)
 	    {
 	      bfd_vma offset;
 
+	      BFD_ASSERT (htab);
+
 		if (sh_elf_osec_readonly_p (output_bfd,
 					    input_section->output_section))
 		  {
@@ -4569,6 +4572,7 @@ sh_elf_relocate_section (bfd *output_bfd
 	  /* Relocation is to the entry for this symbol in the global
 	     offset table extension for the procedure linkage table.  */
 
+	  BFD_ASSERT (htab);
 	  BFD_ASSERT (sgotplt != NULL);
 	  relocation = (sgotplt->output_offset
 			+ (get_plt_index (htab->plt_info, h->plt.offset)
@@ -4594,6 +4598,7 @@ sh_elf_relocate_section (bfd *output_bfd
 	  /* Relocation is to the entry for this symbol in the global
 	     offset table.  */
 
+	  BFD_ASSERT (htab);
 	  BFD_ASSERT (sgot != NULL);
 	  check_segment[0] = check_segment[1] = -1;
 
@@ -4652,7 +4657,7 @@ sh_elf_relocate_section (bfd *output_bfd
 
 		      /* If we initialize the GOT entry here with a valid
 			 symbol address, also add a fixup.  */
-		      if (htab->fdpic_p && !info->shared
+		      if (fdpic_p && !info->shared
 			  && sh_elf_hash_entry (h)->got_type == GOT_NORMAL
 			  && (ELF_ST_VISIBILITY (h->other) == STV_DEFAULT
 			      || h->root.type != bfd_link_hash_undefweak))
@@ -4713,7 +4718,7 @@ sh_elf_relocate_section (bfd *output_bfd
 		      outrel.r_offset = (sgot->output_section->vma
 					 + sgot->output_offset
 					 + off);
-		      if (htab->fdpic_p)
+		      if (fdpic_p)
 			{
 			  int dynindx
 			    = elf_section_data (sec->output_section)->dynindx;
@@ -4730,7 +4735,7 @@ sh_elf_relocate_section (bfd *output_bfd
 		      loc += srelgot->reloc_count++ * sizeof (Elf32_External_Rela);
 		      bfd_elf32_swap_reloca_out (output_bfd, &outrel, loc);
 		    }
-		  else if (htab->fdpic_p
+		  else if (fdpic_p
 			   && (sh_elf_local_got_type (input_bfd) [r_symndx]
 			       == GOT_NORMAL))
 		    sh_elf_add_rofixup (output_bfd, htab->srofixup,
@@ -4775,6 +4780,7 @@ sh_elf_relocate_section (bfd *output_bfd
 	     we place at the start of the .got.plt section.  This is the same
 	     as the start of the output .got section, unless there are function
 	     descriptors in front of it.  */
+	  BFD_ASSERT (htab);
 	  BFD_ASSERT (sgotplt != NULL);
 	  check_segment[0] = got_segment;
 	  relocation -= sgotplt->output_section->vma + sgotplt->output_offset
@@ -4875,6 +4881,8 @@ sh_elf_relocate_section (bfd *output_bfd
 	    bfd_vma reloc_offset;
 	    int reloc_type = R_SH_FUNCDESC;
 
+	    BFD_ASSERT (htab);
+
 	    check_segment[0] = check_segment[1] = -1;
 
 	    /* FIXME: See what FRV does for global symbols in the
@@ -4892,7 +4900,7 @@ sh_elf_relocate_section (bfd *output_bfd
 	      }
 	    else
 	      {
-		reloc_section = htab->sgot;
+		reloc_section = sgot;
 
 		if (h != NULL)
 		  reloc_offset = h->got.offset;
@@ -5087,6 +5095,7 @@ sh_elf_relocate_section (bfd *output_bfd
 	     executable and --export-dynamic.  If such symbols get
 	     ld.so-allocated descriptors we can not use R_SH_GOTOFFFUNCDESC
 	     for them.  */
+	  BFD_ASSERT (htab);
 
 	  check_segment[0] = check_segment[1] = -1;
 	  relocation = 0;
@@ -5139,8 +5148,8 @@ sh_elf_relocate_section (bfd *output_bfd
 	      relocation = htab->sfuncdesc->output_offset + (offset & ~1);
 	    }
 
-	  relocation -= htab->root.hgot->root.u.def.value
-	    + htab->sgotplt->output_offset;
+	  relocation -= (htab->root.hgot->root.u.def.value
+			 + sgotplt->output_offset);
 #ifdef GOT_BIAS
 	  relocation -= GOT_BIAS;
 #endif
@@ -5175,6 +5184,7 @@ sh_elf_relocate_section (bfd *output_bfd
 
 	case R_SH_TLS_GD_32:
 	case R_SH_TLS_IE_32:
+	  BFD_ASSERT (htab);
 	  check_segment[0] = check_segment[1] = -1;
 	  r_type = sh_elf_optimized_tls_reloc (info, r_type, h == NULL);
 	  got_type = GOT_UNKNOWN;
@@ -5425,6 +5435,7 @@ sh_elf_relocate_section (bfd *output_bfd
 	  goto final_link_relocate;
 
 	case R_SH_TLS_LD_32:
+	  BFD_ASSERT (htab);
 	  check_segment[0] = check_segment[1] = -1;
 	  if (! info->shared)
 	    {
@@ -5558,7 +5569,7 @@ sh_elf_relocate_section (bfd *output_bfd
 	}
 
     relocation_done:
-      if (htab->fdpic_p && check_segment[0] != (unsigned) -1
+      if (fdpic_p && check_segment[0] != (unsigned) -1
 	  && check_segment[0] != check_segment[1])
 	{
 	  /* We don't want duplicate errors for undefined symbols.  */