Re: Change bfd_link_hash_entry.type to a bitfield?

[Alan Modra <amodra at gmail dot com>]

On Thu, Apr 21, 2011 at 12:54:57PM +0100, Richard Sandiford wrote:
> > On Wed, Apr 20, 2011 at 6:57 PM, Alan Modra <amodra@gmail.com> wrote:
> >> How do people feel about poking some flags into bfd_link_hash_entry?
> >> This can be done without increasing the size of the struct, since
> >> "type" is an unsigned int but only needs 3 bits. ?The negative side is
> >> that hosts without efficient byte read access will see a linker
> >> slowdown. ?"type" is the most used field of bfd_link_hash_entry.
> >> On the positive side, this would allow the linker lto plugin code to
> >> lose a potentially very large symbol hash table. ?Also, other fields
> >> from elf_link_hash_entry and coff_link_hash_entry could move into
> >> bfd_link_hash_entry, reducing the size of the linker hash table.
> >>
> >
> "H.J. Lu" <hjl.tools@gmail.com> writes:
> > I like it,
> 
> +1.  How about defining ENUM_BITFIELD in ansidecl.h, so that other
> headers could use it too?

That would be a good idea.  Maybe for a later patch..

This version fixes PR 12696 as well.  Committed.

include/
	PR ld/12365
	PR ld/12696
	* bfdlink.h (ENUM_BITFIELD): Define.
	(struct bfd_link_hash_entry): Make "type" a bitfield.  Add "non_ir_ref".
	(struct bfd_link_callbacks <notice>): Pass bfd_link_hash_entry pointer
	rather than "name".
bfd/
	PR ld/12365
	PR ld/12696
	* coff-aux.c (coff_m68k_aux_link_add_one_symbol): Update "notice" call.
	* linker.c (_bfd_link_hash_newfunc): Clear bitfields.
	(_bfd_generic_link_add_one_symbol): Update "notice" call.
	* elflink.c (_bfd_elf_merge_symbol): Don't skip weak redefs when
	it is a redef of an IR symbol in a real BFD.
ld/
	PR ld/12365
	PR ld/12696
	* ldmain.c (notice): Delete "name" param, add "h".
	* plugin.c (plugin_notice): Likewise.  Set non_ir_ref.  Handle
	redefinitions of IR symbols in real BFDs.
	(plugin_multiple_definition, plugin_multiple_common): Delete.
	(non_ironly_hash, init_non_ironly_hash): Delete.
	(is_visible_from_outside): Traverse entry_symbol chain.
	(get_symbols): Use non_ir_ref flag rather than hash lookup.

Index: include/bfdlink.h
===================================================================
RCS file: /cvs/src/src/include/bfdlink.h,v
retrieving revision 1.83
diff -u -p -r1.83 bfdlink.h
--- include/bfdlink.h	20 Apr 2011 00:11:29 -0000	1.83
+++ include/bfdlink.h	24 Apr 2011 07:35:37 -0000
@@ -24,6 +24,12 @@
 #ifndef BFDLINK_H
 #define BFDLINK_H
 
+#if (__GNUC__ * 1000 + __GNUC_MINOR__ > 2000)
+#define ENUM_BITFIELD(TYPE) __extension__ enum TYPE
+#else
+#define ENUM_BITFIELD(TYPE) unsigned int
+#endif
+
 /* Which symbols to strip during a link.  */
 enum bfd_link_strip
 {
@@ -91,7 +97,9 @@ struct bfd_link_hash_entry
   struct bfd_hash_entry root;
 
   /* Type of this entry.  */
-  enum bfd_link_hash_type type;
+  ENUM_BITFIELD (bfd_link_hash_type) type : 8;
+
+  unsigned int non_ir_ref : 1;
 
   /* A union of information depending upon the type.  */
   union
@@ -570,11 +578,11 @@ struct bfd_link_callbacks
     (struct bfd_link_info *, const char *name,
      bfd *abfd, asection *section, bfd_vma address);
   /* A function which is called when a symbol in notice_hash is
-     defined or referenced.  NAME is the symbol.  ABFD, SECTION and
-     ADDRESS are the value of the symbol.  If SECTION is
+     defined or referenced.  H is the symbol.  ABFD, SECTION and
+     ADDRESS are the (new) value of the symbol.  If SECTION is
      bfd_und_section, this is a reference.  */
   bfd_boolean (*notice)
-    (struct bfd_link_info *, const char *name,
+    (struct bfd_link_info *, struct bfd_link_hash_entry *h,
      bfd *abfd, asection *section, bfd_vma address);
   /* Error or warning link info message.  */
   void (*einfo)
Index: bfd/elflink.c
===================================================================
RCS file: /cvs/src/src/bfd/elflink.c,v
retrieving revision 1.401
diff -u -p -r1.401 elflink.c
--- bfd/elflink.c	20 Apr 2011 07:17:01 -0000	1.401
+++ bfd/elflink.c	24 Apr 2011 07:35:20 -0000
@@ -1427,7 +1427,10 @@ _bfd_elf_merge_symbol (bfd *abfd,
   /* Skip weak definitions of symbols that are already defined.  */
   if (newdef && olddef && newweak)
     {
-      *skip = TRUE;
+      /* Don't skip new non-IR weak syms.  */
+      if (!((oldbfd->flags & BFD_PLUGIN) != 0
+	    && (abfd->flags & BFD_PLUGIN) == 0))
+	*skip = TRUE;
 
       /* Merge st_other.  If the symbol already has a dynamic index,
 	 but visibility says it should not be visible, turn it into a
Index: bfd/coff-aux.c
===================================================================
RCS file: /cvs/src/src/bfd/coff-aux.c,v
retrieving revision 1.10
diff -u -p -r1.10 coff-aux.c
--- bfd/coff-aux.c	2 Sep 2009 07:18:36 -0000	1.10
+++ bfd/coff-aux.c	24 Apr 2011 07:35:10 -0000
@@ -105,7 +105,7 @@ coff_m68k_aux_link_add_one_symbol (info,
 	  && (bfd_hash_lookup (info->notice_hash, name, FALSE, FALSE)
 	      != (struct bfd_hash_entry *) NULL))
 	{
-	  if (! (*info->callbacks->notice) (info, name, abfd, section, value))
+	  if (! (*info->callbacks->notice) (info, h, abfd, section, value))
 	    return FALSE;
 	}
 
Index: bfd/linker.c
===================================================================
RCS file: /cvs/src/src/bfd/linker.c,v
retrieving revision 1.80
diff -u -p -r1.80 linker.c
--- bfd/linker.c	20 Apr 2011 00:22:08 -0000	1.80
+++ bfd/linker.c	24 Apr 2011 07:35:23 -0000
@@ -465,10 +465,8 @@ _bfd_link_hash_newfunc (struct bfd_hash_
       struct bfd_link_hash_entry *h = (struct bfd_link_hash_entry *) entry;
 
       /* Initialize the local fields.  */
-      h->type = bfd_link_hash_new;
-      memset (&h->u.undef.next, 0,
-	      (sizeof (struct bfd_link_hash_entry)
-	       - offsetof (struct bfd_link_hash_entry, u.undef.next)));
+      memset ((char *) &h->root + sizeof (h->root), 0,
+	      sizeof (*h) - sizeof (h->root));
     }
 
   return entry;
@@ -1609,8 +1607,7 @@ _bfd_generic_link_add_one_symbol (struct
       || (info->notice_hash != NULL
 	  && bfd_hash_lookup (info->notice_hash, name, FALSE, FALSE) != NULL))
     {
-      if (! (*info->callbacks->notice) (info, h->root.string, abfd, section,
-					value))
+      if (! (*info->callbacks->notice) (info, h, abfd, section, value))
 	return FALSE;
     }
 
Index: ld/ldmain.c
===================================================================
RCS file: /cvs/src/src/ld/ldmain.c,v
retrieving revision 1.153
diff -u -p -r1.153 ldmain.c
--- ld/ldmain.c	20 Apr 2011 00:22:08 -0000	1.153
+++ ld/ldmain.c	24 Apr 2011 07:35:38 -0000
@@ -150,7 +150,8 @@ static bfd_boolean reloc_dangerous
 static bfd_boolean unattached_reloc
   (struct bfd_link_info *, const char *, bfd *, asection *, bfd_vma);
 static bfd_boolean notice
-  (struct bfd_link_info *, const char *, bfd *, asection *, bfd_vma);
+  (struct bfd_link_info *, struct bfd_link_hash_entry *,
+   bfd *, asection *, bfd_vma);
 
 static struct bfd_link_callbacks link_callbacks =
 {
@@ -1479,18 +1480,21 @@ unattached_reloc (struct bfd_link_info *
 
 static bfd_boolean
 notice (struct bfd_link_info *info,
-	const char *name,
+	struct bfd_link_hash_entry *h,
 	bfd *abfd,
 	asection *section,
 	bfd_vma value)
 {
-  if (name == NULL)
+  const char *name;
+
+  if (h == NULL)
     {
       if (command_line.cref || nocrossref_list != NULL)
 	return handle_asneeded_cref (abfd, (enum notice_asneeded_action) value);
       return TRUE;
     }
 
+  name = h->root.string;
   if (info->notice_hash != NULL
       && bfd_hash_lookup (info->notice_hash, name, FALSE, FALSE) != NULL)
     {
Index: ld/plugin.c
===================================================================
RCS file: /cvs/src/src/ld/plugin.c,v
retrieving revision 1.32
diff -u -p -r1.32 plugin.c
--- ld/plugin.c	20 Apr 2011 00:22:08 -0000	1.32
+++ ld/plugin.c	24 Apr 2011 07:35:38 -0000
@@ -90,13 +90,6 @@ static plugin_t *called_plugin = NULL;
 /* Last plugin to cause an error, if any.  */
 static const char *error_plugin = NULL;
 
-/* A hash table that records symbols referenced by non-IR files.  Used
-   at get_symbols time to determine whether any prevailing defs from
-   IR files are referenced only from other IR files, so tthat we can
-   we can distinguish the LDPR_PREVAILING_DEF and LDPR_PREVAILING_DEF_IRONLY
-   cases when establishing symbol resolutions.  */
-static struct bfd_hash_table *non_ironly_hash = NULL;
-
 /* State of linker "notice" interface before we poked at it.  */
 static bfd_boolean orig_notice_all;
 
@@ -133,18 +126,8 @@ static const size_t tv_header_size = ARR
 
 /* Forward references.  */
 static bfd_boolean plugin_notice (struct bfd_link_info *info,
-				  const char *name, bfd *abfd,
+				  struct bfd_link_hash_entry *h, bfd *abfd,
 				  asection *section, bfd_vma value);
-static bfd_boolean plugin_multiple_definition (struct bfd_link_info *info,
-					       struct bfd_link_hash_entry *h,
-					       bfd *nbfd,
-					       asection *nsec,
-					       bfd_vma nval);
-static bfd_boolean plugin_multiple_common (struct bfd_link_info *info,
-					   struct bfd_link_hash_entry *h,
-					   bfd *nbfd,
-					   enum bfd_link_hash_type ntype,
-					   bfd_vma nsize);
 
 #if !defined (HAVE_DLFCN_H) && defined (HAVE_WINDOWS_H)
 
@@ -438,6 +421,8 @@ static inline bfd_boolean
 is_visible_from_outside (struct ld_plugin_symbol *lsym, asection *section,
 			 struct bfd_link_hash_entry *blhe)
 {
+  struct bfd_sym_chain *sym;
+
   /* Section's owner may be NULL if it is the absolute
      section, fortunately is_ir_dummy_bfd handles that.  */
   if (!is_ir_dummy_bfd (section->owner))
@@ -466,6 +451,12 @@ is_visible_from_outside (struct ld_plugi
       return (lsym->visibility == LDPV_DEFAULT
 	      || lsym->visibility == LDPV_PROTECTED);
     }
+
+  for (sym = &entry_symbol; sym != NULL; sym = sym->next)
+    if (sym->name
+	&& strcmp (sym->name, blhe->root.string) == 0)
+      return TRUE;
+
   return FALSE;
 }
 
@@ -520,9 +511,8 @@ get_symbols (const void *handle, int nsy
 	 even potentially-referenced, perhaps in a future final link if
 	 this is a partial one, perhaps dynamically at load-time if the
 	 symbol is externally visible.  */
-      ironly = (!is_visible_from_outside (&syms[n], owner_sec, blhe)
-		&& !bfd_hash_lookup (non_ironly_hash, syms[n].name,
-				     FALSE, FALSE));
+      ironly = !(blhe->non_ir_ref
+		 || is_visible_from_outside (&syms[n], owner_sec, blhe));
 
       /* If it was originally undefined or common, then it has been
 	 resolved; determine how.  */
@@ -740,27 +730,6 @@ plugin_active_plugins_p (void)
   return plugins_list != NULL;
 }
 
-/* Init the non_ironly hash table.  */
-static void
-init_non_ironly_hash (void)
-{
-  struct bfd_sym_chain *sym;
-
-  non_ironly_hash
-    = (struct bfd_hash_table *) xmalloc (sizeof (struct bfd_hash_table));
-  if (!bfd_hash_table_init_n (non_ironly_hash,
-			      bfd_hash_newfunc,
-			      sizeof (struct bfd_hash_entry),
-			      61))
-    einfo (_("%P%F: bfd_hash_table_init failed: %E\n"));
-
-  for (sym = &entry_symbol; sym != NULL; sym = sym->next)
-    if (sym->name
-	&& !bfd_hash_lookup (non_ironly_hash, sym->name, TRUE, TRUE))
-      einfo (_("%P%X: hash table failure adding symbol %s\n"),
-	     sym->name);
-}
-
 /* Load up and initialise all plugins after argument parsing.  */
 int
 plugin_load_plugins (void)
@@ -814,7 +783,6 @@ plugin_load_plugins (void)
   plugin_callbacks.notice = &plugin_notice;
   link_info.notice_all = TRUE;
   link_info.callbacks = &plugin_callbacks;
-  init_non_ironly_hash ();
 
   return 0;
 }
@@ -888,9 +856,6 @@ plugin_call_all_symbols_read (void)
   /* Disable any further file-claiming.  */
   no_more_claiming = TRUE;
 
-  plugin_callbacks.multiple_definition = &plugin_multiple_definition;
-  plugin_callbacks.multiple_common = &plugin_multiple_common;
-
   while (curplug)
     {
       if (curplug->all_symbols_read_handler)
@@ -934,88 +899,47 @@ plugin_call_cleanup (void)
 
 /* To determine which symbols should be resolved LDPR_PREVAILING_DEF
    and which LDPR_PREVAILING_DEF_IRONLY, we notice all the symbols as
-   the linker adds them to the linker hash table.  If we see a symbol
-   being referenced from a non-IR file, we add it to the non_ironly hash
-   table.  If we can't find it there at get_symbols time, we know that
-   it was referenced only by IR files.  We have to notice_all symbols,
-   because we won't necessarily know until later which ones will be
-   contributed by IR files.  */
+   the linker adds them to the linker hash table.  Mark those
+   referenced from a non-IR file with non_ir_ref.  We have to
+   notice_all symbols, because we won't necessarily know until later
+   which ones will be contributed by IR files.  */
 static bfd_boolean
 plugin_notice (struct bfd_link_info *info,
-	       const char *name,
+	       struct bfd_link_hash_entry *h,
 	       bfd *abfd,
 	       asection *section,
 	       bfd_vma value)
 {
-  if (name != NULL)
+  if (h != NULL)
     {
       /* No further processing if this def/ref is from an IR dummy BFD.  */
       if (is_ir_dummy_bfd (abfd))
 	return TRUE;
 
-      /* We only care about refs, not defs, indicated by section
-	 pointing to the undefined section (according to the bfd
-	 linker notice callback interface definition).  */
+      /* If this is a ref, set non_ir_ref.  */
       if (bfd_is_und_section (section))
-	{
-	  /* This is a ref from a non-IR file, so note the ref'd
-	     symbol in the non-IR-only hash.  */
-	  if (!bfd_hash_lookup (non_ironly_hash, name, TRUE, TRUE))
-	    einfo (_("%P%X: %s: hash table failure adding symbol %s\n"),
-		   abfd->filename, name);
-	}
+	h->non_ir_ref = TRUE;
+
+      /* Otherwise, it must be a new def.  Ensure any symbol defined
+	 in an IR dummy BFD takes on a new value from a real BFD.
+	 Weak symbols are not normally overridden by a new weak
+	 definition, and strong symbols will normally cause multiple
+	 definition errors.  Avoid this by making the symbol appear
+	 to be undefined.  */
+      else if (((h->type == bfd_link_hash_defweak
+		 || h->type == bfd_link_hash_defined)
+		&& is_ir_dummy_bfd (h->u.def.section->owner))
+	       || (h->type == bfd_link_hash_common
+		   && is_ir_dummy_bfd (h->u.c.p->section->owner)))
+	h->type = bfd_link_hash_undefweak;
     }
 
   /* Continue with cref/nocrossref/trace-sym processing.  */
-  if (name == NULL
+  if (h == NULL
       || orig_notice_all
       || (info->notice_hash != NULL
-	  && bfd_hash_lookup (info->notice_hash, name, FALSE, FALSE) != NULL))
-    return (*orig_callbacks->notice) (info, name, abfd, section, value);
+	  && bfd_hash_lookup (info->notice_hash, h->root.string,
+			      FALSE, FALSE) != NULL))
+    return (*orig_callbacks->notice) (info, h, abfd, section, value);
   return TRUE;
 }
-
-/* When we add new object files to the link at all symbols read time,
-   these contain the real code and symbols generated from the IR files,
-   and so duplicate all the definitions already supplied by the dummy
-   IR-only BFDs that we created at claim files time.  We use the linker's
-   multiple-definitions callback hook to fix up the clash, discarding
-   the symbol from the IR-only BFD in favour of the symbol from the
-   real BFD.  We return true if this was not-really-a-clash because
-   we've fixed it up, or anyway if --allow-multiple-definition was in
-   effect (before we disabled it to ensure we got called back).  */
-static bfd_boolean
-plugin_multiple_definition (struct bfd_link_info *info,
-			    struct bfd_link_hash_entry *h,
-			    bfd *nbfd, asection *nsec, bfd_vma nval)
-{
-  if (h->type == bfd_link_hash_defined
-      && is_ir_dummy_bfd (h->u.def.section->owner))
-    {
-      /* Replace it with new details.  */
-      h->u.def.section = nsec;
-      h->u.def.value = nval;
-      return TRUE;
-    }
-
-  return (*orig_callbacks->multiple_definition) (info, h, nbfd, nsec, nval);
-}
-
-static bfd_boolean
-plugin_multiple_common (struct bfd_link_info *info,
-			struct bfd_link_hash_entry *h,
-			bfd *nbfd, enum bfd_link_hash_type ntype, bfd_vma nsize)
-{
-  if (h->type == bfd_link_hash_common
-      && is_ir_dummy_bfd (h->u.c.p->section->owner)
-      && ntype == bfd_link_hash_common
-      && !is_ir_dummy_bfd (nbfd))
-    {
-      /* Arrange to have it replaced.  */
-      ASSERT (nsize != 0);
-      h->u.c.size = 0;
-      return TRUE;
-    }
-
-  return (*orig_callbacks->multiple_common) (info, h, nbfd, ntype, nsize);
-}

-- 
Alan Modra
Australia Development Lab, IBM