PR25593, --as-needed breaks DT_NEEDED order with linker plugin

PR25593, --as-needed breaks DT_NEEDED order with linker plugin

[Alan Modra]

This patch delays setting up DT_NEEDED dynamic tags until all object
files and libraries have been opened and their symbols processed,
rather than adding the tags while processing symbols.  Tags are
ordered according to the position of the associated library on the
command line and linker scripts.  It is still possible with
--as-needed libs that are mentioned more than once for tags to be
ordered according to which mention was needed.  For example with
"--as-needed a.so b.so c.so b.so" when b.so is not needed by a.so or
any other prior object file but is needed by c.so, the order of tags
will be "A C B".

bfd/
	PR 25593
	* elf-bfd.h (struct elf_link_hash_table): Rename "loaded" to
	"dyn_loaded".
	(bfd_elf_add_dt_needed_tag): Declare.
	* elf-strtab.c (_bfd_elf_strtab_restore): Handle NULL buf.
	* elflink.c (bfd_elf_add_dt_needed_tag): Make global and rename
	from elf_add_dt_needed_tag.  Remove soname and doit param.
	(elf_link_add_object_symbols): Don't use elf_add_dt_needed_tag
	to see whether as-needed lib is already loaded, use dyn_loaded
	list instead.  When saving and restoring around as-needed lib
	handle possibility that dynstr has not been initialised.  Don't
	add DT_NEEDED tags here.  Limit dyn_loaded list to dynamic libs.
	Mark libs loaded via DT_NEEDED entries of other libs with
	DYN_NO_NEEDED if they should not be mentioned in DT_NEEDED of
	the output.
	(elf_link_check_versioned_symbol): Remove now unneccesary
	DYNAMIC check when traversing dyn_loaded list.
ld/
	PR 25593
	* ldelf.c (ldelf_try_needed): Add DT_NEEDED lib to input_bfds.
	(ldelf_after_open): Save state of input_bfds list before loading
	DT_NEEDED libs.  Traverse input_bfds list adding DT_NEEDED tags.
	Restore input_bfds list.
	* testsuite/ld-cris/gotplt1.d: Adjust for changed .dynstr order.

diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index 7d36e23ea1..b930761363 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -637,8 +637,8 @@ struct elf_link_hash_table
   asection *tls_sec;
   bfd_size_type tls_size;
 
-  /* A linked list of BFD's loaded in the link.  */
-  struct elf_link_loaded_list *loaded;
+  /* A linked list of dynamic BFD's loaded in the link.  */
+  struct elf_link_loaded_list *dyn_loaded;
 
   /* Short-cuts to get to dynamic linker sections.  */
   asection *sgot;
@@ -2510,6 +2510,8 @@ extern bfd_boolean bfd_elf_link_add_symbols
   (bfd *, struct bfd_link_info *);
 extern bfd_boolean _bfd_elf_add_dynamic_entry
   (struct bfd_link_info *, bfd_vma, bfd_vma);
+extern int bfd_elf_add_dt_needed_tag
+  (bfd *, struct bfd_link_info *);
 extern bfd_boolean _bfd_elf_link_check_relocs
   (bfd *, struct bfd_link_info *);
 
diff --git a/bfd/elf-strtab.c b/bfd/elf-strtab.c
index d3e50c76cf..c397180dcc 100644
--- a/bfd/elf-strtab.c
+++ b/bfd/elf-strtab.c
@@ -245,13 +245,16 @@ _bfd_elf_strtab_save (struct elf_strtab_hash *tab)
 void
 _bfd_elf_strtab_restore (struct elf_strtab_hash *tab, void *buf)
 {
-  size_t idx, curr_size = tab->size;
+  size_t idx, curr_size = tab->size, save_size;
   struct strtab_save *save = (struct strtab_save *) buf;
 
   BFD_ASSERT (tab->sec_size == 0);
-  BFD_ASSERT (save->size <= curr_size);
-  tab->size = save->size;
-  for (idx = 1; idx < save->size; ++idx)
+  save_size = 1;
+  if (save != NULL)
+    save_size = save->size;
+  BFD_ASSERT (save_size <= curr_size);
+  tab->size = save_size;
+  for (idx = 1; idx < save_size; ++idx)
     tab->array[idx]->refcount = save->refcount[idx];
 
   for (; idx < curr_size; ++idx)
diff --git a/bfd/elflink.c b/bfd/elflink.c
index 8e7ae2a160..6f03c5c09f 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -3501,23 +3501,21 @@ _bfd_elf_add_dynamic_entry (struct bfd_link_info *info,
   return TRUE;
 }
 
-/* Add a DT_NEEDED entry for this dynamic object if DO_IT is true,
-   otherwise just check whether one already exists.  Returns -1 on error,
+/* Add a DT_NEEDED entry for this dynamic object.  Returns -1 on error,
    1 if a DT_NEEDED tag already exists, and 0 on success.  */
 
-static int
-elf_add_dt_needed_tag (bfd *abfd,
-		       struct bfd_link_info *info,
-		       const char *soname,
-		       bfd_boolean do_it)
+int
+bfd_elf_add_dt_needed_tag (bfd *abfd, struct bfd_link_info *info)
 {
   struct elf_link_hash_table *hash_table;
   size_t strindex;
+  const char *soname;
 
   if (!_bfd_elf_link_create_dynstrtab (abfd, info))
     return -1;
 
   hash_table = elf_hash_table (info);
+  soname = elf_dt_name (abfd);
   strindex = _bfd_elf_strtab_add (hash_table->dynstr, soname, FALSE);
   if (strindex == (size_t) -1)
     return -1;
@@ -3547,17 +3545,11 @@ elf_add_dt_needed_tag (bfd *abfd,
 	  }
     }
 
-  if (do_it)
-    {
-      if (!_bfd_elf_link_create_dynamic_sections (hash_table->dynobj, info))
-	return -1;
+  if (!_bfd_elf_link_create_dynamic_sections (hash_table->dynobj, info))
+    return -1;
 
-      if (!_bfd_elf_add_dynamic_entry (info, DT_NEEDED, strindex))
-	return -1;
-    }
-  else
-    /* We were just checking for existence of the tag.  */
-    _bfd_elf_strtab_delref (hash_table->dynstr, strindex);
+  if (!_bfd_elf_add_dynamic_entry (info, DT_NEEDED, strindex))
+    return -1;
 
   return 0;
 }
@@ -4069,7 +4061,7 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
       char *audit = NULL;
       struct bfd_link_needed_list *rpath = NULL, *runpath = NULL;
       const Elf_Internal_Phdr *phdr;
-      int ret;
+      struct elf_link_loaded_list *loaded_lib;
 
       /* ld --just-symbols and dynamic objects don't mix very well.
 	 ld shouldn't allow it.  */
@@ -4258,15 +4250,22 @@ error_free_dyn:
 	 will need to know it.  */
       elf_dt_name (abfd) = soname;
 
-      ret = elf_add_dt_needed_tag (abfd, info, soname, add_needed);
-      if (ret < 0)
-	goto error_return;
-
       /* If we have already included this dynamic object in the
 	 link, just ignore it.  There is no reason to include a
 	 particular dynamic object more than once.  */
-      if (ret > 0)
-	return TRUE;
+      for (loaded_lib = htab->dyn_loaded;
+	   loaded_lib != NULL;
+	   loaded_lib = loaded_lib->next)
+	{
+	  if (strcmp (elf_dt_name (loaded_lib->abfd), soname) == 0)
+	    return TRUE;
+	}
+
+      /* Create dynamic sections for backends that require that be done
+	 before setup_gnu_properties.  */
+      if (add_needed
+	  && !_bfd_elf_link_create_dynamic_sections (abfd, info))
+	return FALSE;
 
       /* Save the DT_AUDIT entry for the linker emulation code. */
       elf_dt_audit (abfd) = audit;
@@ -4389,9 +4388,13 @@ error_free_dyn:
       old_table = htab->root.table.table;
       old_size = htab->root.table.size;
       old_count = htab->root.table.count;
-      old_strtab = _bfd_elf_strtab_save (htab->dynstr);
-      if (old_strtab == NULL)
-	goto error_free_vers;
+      old_strtab = NULL;
+      if (htab->dynstr != NULL)
+	{
+	  old_strtab = _bfd_elf_strtab_save (htab->dynstr);
+	  if (old_strtab == NULL)
+	    goto error_free_vers;
+	}
 
       for (i = 0; i < htab->root.table.size; i++)
 	{
@@ -5102,7 +5105,6 @@ error_free_dyn:
 		      && !on_needed_list (elf_dt_name (abfd),
 					  htab->needed, NULL))))
 	    {
-	      int ret;
 	      const char *soname = elf_dt_name (abfd);
 
 	      info->callbacks->minfo ("%!", soname, old_bfd,
@@ -5127,12 +5129,11 @@ error_free_dyn:
 	      elf_dyn_lib_class (abfd) = (enum dynamic_lib_link_class)
 		(elf_dyn_lib_class (abfd) & ~DYN_AS_NEEDED);
 
+	      /* Create dynamic sections for backends that require
+		 that be done before setup_gnu_properties.  */
+	      if (!_bfd_elf_link_create_dynamic_sections (abfd, info))
+		return FALSE;
 	      add_needed = TRUE;
-	      ret = elf_add_dt_needed_tag (abfd, info, soname, add_needed);
-	      if (ret < 0)
-		goto error_free_vers;
-
-	      BFD_ASSERT (ret == 0);
 	    }
 	}
     }
@@ -5222,7 +5223,8 @@ error_free_dyn:
       memcpy (htab->root.table.table, old_tab, tabsize);
       htab->root.undefs = old_undefs;
       htab->root.undefs_tail = old_undefs_tail;
-      _bfd_elf_strtab_restore (htab->dynstr, old_strtab);
+      if (htab->dynstr != NULL)
+	_bfd_elf_strtab_restore (htab->dynstr, old_strtab);
       free (old_strtab);
       old_strtab = NULL;
       for (i = 0; i < htab->root.table.size; i++)
@@ -5550,7 +5552,7 @@ error_free_dyn:
 	}
     }
 
-  if (is_elf_hash_table (htab) && add_needed)
+  if (dynamic && add_needed)
     {
       /* Add this bfd to the loaded list.  */
       struct elf_link_loaded_list *n;
@@ -5559,9 +5561,12 @@ error_free_dyn:
       if (n == NULL)
 	goto error_return;
       n->abfd = abfd;
-      n->next = htab->loaded;
-      htab->loaded = n;
+      n->next = htab->dyn_loaded;
+      htab->dyn_loaded = n;
     }
+  if (dynamic && !add_needed
+      && (elf_dyn_lib_class (abfd) & DYN_DT_NEEDED) != 0)
+    elf_dyn_lib_class (abfd) |= DYN_NO_NEEDED;
 
   return TRUE;
 
@@ -9689,7 +9694,7 @@ elf_link_check_versioned_symbol (struct bfd_link_info *info,
     }
   BFD_ASSERT (abfd != NULL);
 
-  for (loaded = elf_hash_table (info)->loaded;
+  for (loaded = elf_hash_table (info)->dyn_loaded;
        loaded != NULL;
        loaded = loaded->next)
     {
@@ -9709,7 +9714,6 @@ elf_link_check_versioned_symbol (struct bfd_link_info *info,
 
       /* We check each DSO for a possible hidden versioned definition.  */
       if (input == abfd
-	  || (input->flags & DYNAMIC) == 0
 	  || elf_dynversym (input) == 0)
 	continue;
 
diff --git a/ld/ldelf.c b/ld/ldelf.c
index 3ac3bb4e0a..b055929d02 100644
--- a/ld/ldelf.c
+++ b/ld/ldelf.c
@@ -375,6 +375,9 @@ ldelf_try_needed (struct dt_needed *needed, int force, int is_linux)
 
   bfd_elf_set_dyn_lib_class (abfd, (enum dynamic_lib_link_class) link_class);
 
+  *link_info.input_bfds_tail = abfd;
+  link_info.input_bfds_tail = &abfd->link.next;
+
   /* Add this file into the symbol table.  */
   if (! bfd_link_add_symbols (abfd, &link_info))
     einfo (_("%F%P: %pB: error adding symbols: %E\n"), abfd);
@@ -992,6 +995,7 @@ ldelf_after_open (int use_libpath, int native, int is_linux, int is_freebsd,
   struct elf_link_hash_table *htab;
   asection *s;
   bfd *abfd;
+  bfd **save_input_bfd_tail;
 
   after_open_default ();
 
@@ -1134,6 +1138,7 @@ ldelf_after_open (int use_libpath, int native, int is_linux, int is_freebsd,
      special action by the person doing the link.  Note that the
      needed list can actually grow while we are stepping through this
      loop.  */
+  save_input_bfd_tail = link_info.input_bfds_tail;
   needed = bfd_elf_get_needed_list (link_info.output_bfd, &link_info);
   for (l = needed; l != NULL; l = l->next)
     {
@@ -1290,6 +1295,20 @@ ldelf_after_open (int use_libpath, int native, int is_linux, int is_freebsd,
 	     l->name, l->by);
     }
 
+  for (abfd = link_info.input_bfds; abfd; abfd = abfd->link.next)
+    if (bfd_get_format (abfd) == bfd_object
+	&& ((abfd->flags) & DYNAMIC) != 0
+	&& bfd_get_flavour (abfd) == bfd_target_elf_flavour
+	&& (elf_dyn_lib_class (abfd) & (DYN_AS_NEEDED | DYN_NO_NEEDED)) == 0
+	&& elf_dt_name (abfd) != NULL)
+      {
+	if (bfd_elf_add_dt_needed_tag (abfd, &link_info) < 0)
+	  einfo (_("%F%P: failed to add DT_NEEDED dynamic tag\n"));
+      }
+
+  link_info.input_bfds_tail = save_input_bfd_tail;
+  *save_input_bfd_tail = NULL;
+
   if (link_info.eh_frame_hdr_type == COMPACT_EH_HDR)
     if (!bfd_elf_parse_eh_frame_entries (NULL, &link_info))
       einfo (_("%F%P: failed to parse EH frame entries\n"));
diff --git a/ld/testsuite/ld-cris/gotplt1.d b/ld/testsuite/ld-cris/gotplt1.d
index 28724d7004..defba8ad6a 100644
--- a/ld/testsuite/ld-cris/gotplt1.d
+++ b/ld/testsuite/ld-cris/gotplt1.d
@@ -34,7 +34,7 @@ Contents of section \.text:
  80178 6f0d1000 0000611a 6f2e5401 08000000  .*
  80188 6f3e70df ffff0000                    .*
 Contents of section \.dynamic:
- 82190 01000000 01000000 04000000 e4000800  .*
+ 82190 01000000 07000000 04000000 e4000800  .*
  821a0 05000000 18010800 06000000 f8000800  .*
  821b0 0a000000 1a000000 0b000000 10000000  .*
  821c0 15000000 00000000 03000000 18220800  .*

-- 
Alan Modra
Australia Development Lab, IBM

[PATCH] ld: Add PR ld/25593 tests

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 2170 bytes --]

On Tue, Feb 25, 2020 at 4:05 PM Alan Modra <amodra@gmail.com> wrote:
>
> This patch delays setting up DT_NEEDED dynamic tags until all object
> files and libraries have been opened and their symbols processed,
> rather than adding the tags while processing symbols.  Tags are
> ordered according to the position of the associated library on the
> command line and linker scripts.  It is still possible with
> --as-needed libs that are mentioned more than once for tags to be
> ordered according to which mention was needed.  For example with
> "--as-needed a.so b.so c.so b.so" when b.so is not needed by a.so or
> any other prior object file but is needed by c.so, the order of tags
> will be "A C B".
>
> bfd/
>         PR 25593
>         * elf-bfd.h (struct elf_link_hash_table): Rename "loaded" to
>         "dyn_loaded".
>         (bfd_elf_add_dt_needed_tag): Declare.
>         * elf-strtab.c (_bfd_elf_strtab_restore): Handle NULL buf.
>         * elflink.c (bfd_elf_add_dt_needed_tag): Make global and rename
>         from elf_add_dt_needed_tag.  Remove soname and doit param.
>         (elf_link_add_object_symbols): Don't use elf_add_dt_needed_tag
>         to see whether as-needed lib is already loaded, use dyn_loaded
>         list instead.  When saving and restoring around as-needed lib
>         handle possibility that dynstr has not been initialised.  Don't
>         add DT_NEEDED tags here.  Limit dyn_loaded list to dynamic libs.
>         Mark libs loaded via DT_NEEDED entries of other libs with
>         DYN_NO_NEEDED if they should not be mentioned in DT_NEEDED of
>         the output.
>         (elf_link_check_versioned_symbol): Remove now unneccesary
>         DYNAMIC check when traversing dyn_loaded list.
> ld/
>         PR 25593
>         * ldelf.c (ldelf_try_needed): Add DT_NEEDED lib to input_bfds.
>         (ldelf_after_open): Save state of input_bfds list before loading
>         DT_NEEDED libs.  Traverse input_bfds list adding DT_NEEDED tags.
>         Restore input_bfds list.
>         * testsuite/ld-cris/gotplt1.d: Adjust for changed .dynstr order.
>

Here is a patch to add some tests.   OK for master branch.

-- 
H.J.

[-- Attachment #2: 0001-ld-Add-PR-ld-25593-tests.patch --]
[-- Type: application/x-patch, Size: 4844 bytes --]

Re: [PATCH] ld: Add PR ld/25593 tests

[Alan Modra]

On Tue, Feb 25, 2020 at 06:27:24PM -0800, H.J. Lu wrote:
> Here is a patch to add some tests.   OK for master branch.

OK, thanks.

-- 
Alan Modra
Australia Development Lab, IBM