Need some help with bfd_elf_link_record_dynamic_symbol

[Bernd Schmidt <bernds_cb1 at t-online dot de>]

I'm debugging a problem on the Blackfin, and while I have a patch, I 
don't know enough of the bfd internals to be sure it's correct.  So, I'm 
asking for help.

When the linker sees a reference to an undefined hidden symbol in one 
file, and there is a definition of that symbol in one of the following 
files, an unneeded reference to this hidden symbol is created in .dynsym.

test.c:
extern int foo_hidden(void) __attribute__((visibility("hidden")));
int main() { return foo_hidden(); }

foo.c:
static int foo_static(void) { return 2; }
__attribute__((visibility("hidden")))
int foo_hidden(void) { return 1 + foo_static(); }

Linking these together with
  bfin-linux-uclibc-gcc test.c foo.c

produces the incorrect output; reversing the order of input files fixes 
the problem.

I've tracked this down to a piece of code in elf32-bfin.c, function 
bfinfdpic_check_relocs:
              if (h->dynindx == -1)
                switch (ELF_ST_VISIBILITY (h->other))
                  {
                  case STV_INTERNAL:
                  case STV_HIDDEN:
                    break;
                  default:
                    bfd_elf_link_record_dynamic_symbol (info, h);
                    break;
                  }

The problem is that this apparently happens too early, in check_relocs, 
before the definition of the symbol is seen.

This code is similar to the following idiom
          /* Make sure this symbol is output as a dynamic symbol.  */
          if (h->dynindx == -1 && !h->forced_local)
            {
              if (!bfd_elf_link_record_dynamic_symbol (info, h))
                return FALSE;
            }

which occurs frequently in other ports: elf32-i386.c has four copies of 
this, under varying conditions, in its allocate_dynrelocs function. 
This is where I have to fold and ask for help: I don't understand the 
code well enough to be certain under which conditions this code fragment 
should be run.  The patch below, which adds a dummy allocate_dynrelocs 
function to the Blackfin port and removes the offending code from 
check_relocs, seems to eliminate the problem, but I don't know if this 
is correct.

Why isn't this kind of thing done in the target-independent code, 
anyway?  It seems wasteful to have near-identical but subtly out-of-sync 
code in all the backends.


Bernd
--
This footer brought to you by insane German lawmakers.
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 40368
Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif

Index: elf32-bfin.c
===================================================================
--- elf32-bfin.c	(revision 2365)
+++ elf32-bfin.c	(working copy)
@@ -4061,22 +4064,59 @@ _bfinfdpic_resolve_final_relocs_info (vo
   return 1;
 }
 
+/* Allocate space in .plt, .got and associated reloc sections for
+   dynamic relocs.  */
+
+static bfd_boolean
+allocate_dynrelocs (struct elf_link_hash_entry *h, void *inf)
+{
+  struct bfd_link_info *info;
+  struct elf_link_hash_table *htab;
+
+  if (h->root.type == bfd_link_hash_indirect)
+    return TRUE;
+
+  if (h->root.type == bfd_link_hash_warning)
+    /* When warning symbols are created, they **replace** the "real"
+       entry in the hash table, thus we never get to see the real
+       symbol in a hash traversal.  So look at it now.  */
+    h = (struct elf_link_hash_entry *) h->root.u.i.link;
+
+  info = (struct bfd_link_info *) inf;
+  htab = elf_hash_table (info);
+
+  if (htab->dynamic_sections_created)
+    {
+      /* Make sure this symbol is output as a dynamic symbol.
+	 Undefined weak syms won't yet be marked as dynamic.  */
+      if (h->dynindx == -1
+	  && !h->forced_local)
+	{
+	  if (! bfd_elf_link_record_dynamic_symbol (info, h))
+	    return FALSE;
+	}
+    }
+  return TRUE;
+}
+
 /* Set the sizes of the dynamic sections.  */
 
 static bfd_boolean
 elf32_bfinfdpic_size_dynamic_sections (bfd *output_bfd,
 				      struct bfd_link_info *info)
 {
+  struct elf_link_hash_table *htab;
   bfd *dynobj;
   asection *s;
   struct _bfinfdpic_dynamic_got_plt_info gpinfo;
   bfd_signed_vma odd;
   bfd_vma limit;
 
-  dynobj = elf_hash_table (info)->dynobj;
+  htab = elf_hash_table (info);
+  dynobj = htab->dynobj;
   BFD_ASSERT (dynobj != NULL);
 
-  if (elf_hash_table (info)->dynamic_sections_created)
+  if (htab->dynamic_sections_created)
     {
       /* Set the contents of the .interp section to the interpreter.  */
       if (info->executable)
@@ -4088,6 +4284,10 @@ elf32_bfinfdpic_size_dynamic_sections (b
 	}
     }
 
+  /* Allocate global sym .plt and .got entries, and space for global
+     sym dynamic relocs.  */
+  elf_link_hash_traverse (htab, allocate_dynrelocs, (PTR) info);
+
   memset (&gpinfo, 0, sizeof (gpinfo));
   gpinfo.g.info = info;
 
@@ -4722,16 +4920,6 @@ bfinfdpic_check_relocs (bfd *abfd, struc
 	    }
 	  if (h != NULL)
 	    {
-	      if (h->dynindx == -1)
-		switch (ELF_ST_VISIBILITY (h->other))
-		  {
-		  case STV_INTERNAL:
-		  case STV_HIDDEN:
-		    break;
-		  default:
-		    bfd_elf_link_record_dynamic_symbol (info, h);
-		    break;
-		  }
 	      picrel
 		= bfinfdpic_relocs_info_for_global (bfinfdpic_relocs_info (info),
 						   abfd, h,