PR22458, failure to choose a matching ELF target

Message ID 20180516121542.GJ23663@bubble.grove.modra.org
State New
Headers show
Series
  • PR22458, failure to choose a matching ELF target
Related show

Commit Message

Alan Modra May 16, 2018, 12:15 p.m.
https://sourceware.org/ml/binutils/2013-05/msg00271.html was supposed
to banish "file format is ambiguous" errors for ELF.  It didn't,
because the code supposedly detecting formats that implement
match_priority didn't work.  That was due to not placing all matching
targets into the vector of matching targets.  ELF objects should all
match the generic ELF target (priority 2), plus one or more machine
specific targets (priority 1), and perhaps a single machine specific
target with OS/ABI set (priority 0, best match).  So the armel object
in the testcase actually matches elf32-littlearm,
elf32-littlearm-symbian, and elf32-littlearm-vxworks (all priority 1),
and elf32-little (priority 2).  As the PR reported, elf32-little
wasn't seen as matching.  Fixing that part of the problem wasn't too
difficult but matching the generic ELF target as well as the ARM ELF
targets resulted in ARM testsuite failures.

These proved to be the annoying reordering of stubs that occurs from
time to time due to the stub names containing the section id.
Matching another target causes more sections to be created in
elf_object_p.  If section ids change, stub names change, which results
in different hashing and can therefore result in different hash table
traversal and stub creation order.  That particular problem is fixed
by resetting section_id to the initial state before attempting each
target match, and taking a snapshot of its value after a successful
match.

	PR 22458
	* format.c (struct bfd_preserve): Add section_id.
	(bfd_preserve_save, bfd_preserve_restore): Save and restore
	_bfd_section_id.
	(bfd_reinit): Set _bfd_section_id.
	(bfd_check_format_matches): Put all matches of any priority into
	matching_vector.  Save initial section id and start each attempted
	match at that section id.
	* libbfd-in.h (_bfd_section_id): Declare.
	* section.c (_bfd_section_id): Rename from section_id and make
	global.  Adjust uses.
	(bfd_get_next_section_id): Delete.
	* elf64-ppc.c (ppc64_elf_setup_section_lists): Replace use of
	bfd_get_section_id with _bfd_section_id.
	* libbfd.h: Regenerate.
	* bfd-in2.h: Regenerate.


-- 
Alan Modra
Australia Development Lab, IBM

Comments

H.J. Lu May 16, 2018, 1:45 p.m. | #1
On Wed, May 16, 2018 at 5:15 AM, Alan Modra <amodra@gmail.com> wrote:
> https://sourceware.org/ml/binutils/2013-05/msg00271.html was supposed

> to banish "file format is ambiguous" errors for ELF.  It didn't,

> because the code supposedly detecting formats that implement

> match_priority didn't work.  That was due to not placing all matching

> targets into the vector of matching targets.  ELF objects should all

> match the generic ELF target (priority 2), plus one or more machine

> specific targets (priority 1), and perhaps a single machine specific

> target with OS/ABI set (priority 0, best match).  So the armel object

> in the testcase actually matches elf32-littlearm,

> elf32-littlearm-symbian, and elf32-littlearm-vxworks (all priority 1),

> and elf32-little (priority 2).  As the PR reported, elf32-little

> wasn't seen as matching.  Fixing that part of the problem wasn't too

> difficult but matching the generic ELF target as well as the ARM ELF

> targets resulted in ARM testsuite failures.

>

> These proved to be the annoying reordering of stubs that occurs from

> time to time due to the stub names containing the section id.

> Matching another target causes more sections to be created in

> elf_object_p.  If section ids change, stub names change, which results

> in different hashing and can therefore result in different hash table

> traversal and stub creation order.  That particular problem is fixed

> by resetting section_id to the initial state before attempting each

> target match, and taking a snapshot of its value after a successful

> match.

>

>         PR 22458

>         * format.c (struct bfd_preserve): Add section_id.

>         (bfd_preserve_save, bfd_preserve_restore): Save and restore

>         _bfd_section_id.

>         (bfd_reinit): Set _bfd_section_id.

>         (bfd_check_format_matches): Put all matches of any priority into

>         matching_vector.  Save initial section id and start each attempted

>         match at that section id.

>         * libbfd-in.h (_bfd_section_id): Declare.

>         * section.c (_bfd_section_id): Rename from section_id and make

>         global.  Adjust uses.

>         (bfd_get_next_section_id): Delete.

>         * elf64-ppc.c (ppc64_elf_setup_section_lists): Replace use of

>         bfd_get_section_id with _bfd_section_id.

>         * libbfd.h: Regenerate.

>         * bfd-in2.h: Regenerate.

>


> diff --git a/bfd/libbfd-in.h b/bfd/libbfd-in.h

> index 0c80f4d598..d8b445f894 100644

> --- a/bfd/libbfd-in.h

> +++ b/bfd/libbfd-in.h

> @@ -53,6 +53,9 @@ struct section_hash_entry

>    asection section;

>  };

>

> +/* Unique section id.  */

> +extern unsigned int _bfd_section_id;

> +


Should it be marked as hidden?

-- 
H.J.
Alan Modra May 16, 2018, 11:57 p.m. | #2
On Wed, May 16, 2018 at 06:45:57AM -0700, H.J. Lu wrote:
> On Wed, May 16, 2018 at 5:15 AM, Alan Modra <amodra@gmail.com> wrote:

> > --- a/bfd/libbfd-in.h

> > +++ b/bfd/libbfd-in.h

> > @@ -53,6 +53,9 @@ struct section_hash_entry

> >    asection section;

> >  };

> >

> > +/* Unique section id.  */

> > +extern unsigned int _bfd_section_id;

> > +

> 

> Should it be marked as hidden?


True.  Every variable or function declaration in that file should be
hidden.  Looks like we could do with an ATTRIBUTE_HIDDEN macro.

Also, opcodes/nfp-dis.c should not be including libbfd.h.

-- 
Alan Modra
Australia Development Lab, IBM
Alan Modra May 18, 2018, 3:44 a.m. | #3
On Thu, May 17, 2018 at 09:27:57AM +0930, Alan Modra wrote:
> Also, opcodes/nfp-dis.c should not be including libbfd.h.


Fixed like this.

	* nfp-dis.c: Don't #include libbfd.h.
	(init_nfp3200_priv): Use bfd_get_section_contents.
	(nit_nfp6000_mecsr_sec): Likewise.

diff --git a/opcodes/nfp-dis.c b/opcodes/nfp-dis.c
index 9dda34b173..e501b14959 100644
--- a/opcodes/nfp-dis.c
+++ b/opcodes/nfp-dis.c
@@ -34,7 +34,6 @@
 #include "elf-bfd.h"
 #include "bfd.h"
 #include "bfd_stdint.h"
-#include "libbfd.h"
 
 #define _NFP_ERR_STOP -1
 #define _NFP_ERR_CONT -8
@@ -2573,9 +2572,8 @@ init_nfp3200_priv (nfp_priv_data * priv, struct disassemble_info *dinfo)
 
       mecfg = &priv->mecfgs[isl][menum][1];
 
-      if (!_bfd_generic_get_section_contents (dinfo->section->owner,
-					      sec->bfd_section, buffer,
-					      roff, sizeof (buffer)))
+      if (!bfd_get_section_contents (dinfo->section->owner, sec->bfd_section,
+				     buffer, roff, sizeof (buffer)))
 	return FALSE;
 
       mecfg_ent.ctx_enables = bfd_getl32 (buffer + offsetof (Elf_Nfp_MeConfig,
@@ -2622,9 +2620,8 @@ init_nfp6000_mecsr_sec (nfp_priv_data * priv, Elf_Internal_Shdr * sec,
       uint32_t csr_off;
       nfp_priv_mecfg *mecfg;
 
-      if (!_bfd_generic_get_section_contents (dinfo->section->owner,
-					      sec->bfd_section, buffer,
-					      ireg_off, sizeof (buffer)))
+      if (!bfd_get_section_contents (dinfo->section->owner, sec->bfd_section,
+				     buffer, ireg_off, sizeof (buffer)))
 	return FALSE;
 
       ireg.cpp_offset_lo = bfd_getl32 (buffer

-- 
Alan Modra
Australia Development Lab, IBM

Patch

diff --git a/bfd/elf64-ppc.c b/bfd/elf64-ppc.c
index 09377d102c..b166558945 100644
--- a/bfd/elf64-ppc.c
+++ b/bfd/elf64-ppc.c
@@ -11760,7 +11760,7 @@  ppc64_elf_setup_section_lists (struct bfd_link_info *info)
   if (htab == NULL)
     return -1;
 
-  htab->sec_info_arr_size = bfd_get_next_section_id ();
+  htab->sec_info_arr_size = _bfd_section_id;
   amt = sizeof (*htab->sec_info) * (htab->sec_info_arr_size);
   htab->sec_info = bfd_zmalloc (amt);
   if (htab->sec_info == NULL)
diff --git a/bfd/format.c b/bfd/format.c
index 64d33f7f98..c4afd97d08 100644
--- a/bfd/format.c
+++ b/bfd/format.c
@@ -103,6 +103,7 @@  struct bfd_preserve
   struct bfd_section *sections;
   struct bfd_section *section_last;
   unsigned int section_count;
+  unsigned int section_id;
   struct bfd_hash_table section_htab;
   const struct bfd_build_id *build_id;
 };
@@ -125,6 +126,7 @@  bfd_preserve_save (bfd *abfd, struct bfd_preserve *preserve)
   preserve->sections = abfd->sections;
   preserve->section_last = abfd->section_last;
   preserve->section_count = abfd->section_count;
+  preserve->section_id = _bfd_section_id;
   preserve->section_htab = abfd->section_htab;
   preserve->marker = bfd_alloc (abfd, 1);
   preserve->build_id = abfd->build_id;
@@ -138,12 +140,13 @@  bfd_preserve_save (bfd *abfd, struct bfd_preserve *preserve)
 /* Clear out a subset of BFD state.  */
 
 static void
-bfd_reinit (bfd *abfd)
+bfd_reinit (bfd *abfd, unsigned int section_id)
 {
   abfd->tdata.any = NULL;
   abfd->arch_info = &bfd_default_arch_struct;
   abfd->flags &= BFD_FLAGS_SAVED;
   bfd_section_list_clear (abfd);
+  _bfd_section_id = section_id;
 }
 
 /* Restores bfd state saved by bfd_preserve_save.  */
@@ -160,6 +163,7 @@  bfd_preserve_restore (bfd *abfd, struct bfd_preserve *preserve)
   abfd->sections = preserve->sections;
   abfd->section_last = preserve->section_last;
   abfd->section_count = preserve->section_count;
+  _bfd_section_id = preserve->section_id;
   abfd->build_id = preserve->build_id;
 
   /* bfd_release frees all memory more recently bfd_alloc'd than
@@ -214,6 +218,7 @@  bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
   const bfd_target *save_targ, *right_targ, *ar_right_targ, *match_targ;
   int match_count, best_count, best_match;
   int ar_match_index;
+  unsigned int initial_section_id = _bfd_section_id;
   struct bfd_preserve preserve;
 
   if (matching != NULL)
@@ -287,14 +292,13 @@  bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
 
       /* Don't check the default target twice.  */
       if (*target == &binary_vec
-	  || (!abfd->target_defaulted && *target == save_targ)
-	  || (*target)->match_priority > best_match)
+	  || (!abfd->target_defaulted && *target == save_targ))
 	continue;
 
       /* If we already tried a match, the bfd is modified and may
 	 have sections attached, which will confuse the next
 	 _bfd_check_format call.  */
-      bfd_reinit (abfd);
+      bfd_reinit (abfd, initial_section_id);
 
       /* Change BFD's target temporarily.  */
       abfd->xvec = *target;
@@ -329,9 +333,6 @@  bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
 	      || (bfd_has_map (abfd)
 		  && bfd_get_error () != bfd_error_wrong_object_format))
 	    {
-	      /* This format checks out as ok!  */
-	      right_targ = temp;
-
 	      /* If this is the default target, accept it, even if
 		 other targets might match.  People who want those
 		 other targets have to set the GNUTARGET variable.  */
@@ -347,7 +348,12 @@  bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
 		  best_match = match_priority;
 		  best_count = 0;
 		}
-	      best_count++;
+	      if (match_priority <= best_match)
+		{
+		  /* This format checks out as ok!  */
+		  right_targ = temp;
+		  best_count++;
+		}
 	    }
 	  else
 	    {
@@ -446,7 +452,7 @@  bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
 	 state (except possibly for XVEC).  */
       if (match_targ != right_targ)
 	{
-	  bfd_reinit (abfd);
+	  bfd_reinit (abfd, initial_section_id);
 	  if (bfd_seek (abfd, (file_ptr) 0, SEEK_SET) != 0)
 	    goto err_ret;
 	  match_targ = BFD_SEND_FMT (abfd, _bfd_check_format, (abfd));
diff --git a/bfd/libbfd-in.h b/bfd/libbfd-in.h
index 0c80f4d598..d8b445f894 100644
--- a/bfd/libbfd-in.h
+++ b/bfd/libbfd-in.h
@@ -53,6 +53,9 @@  struct section_hash_entry
   asection section;
 };
 
+/* Unique section id.  */
+extern unsigned int _bfd_section_id;
+
 /* tdata for an archive.  For an input archive, cache
    needs to be free()'d.  For an output archive, symdefs do.  */
 
diff --git a/bfd/section.c b/bfd/section.c
index 752f291cb1..7ee3f6915b 100644
--- a/bfd/section.c
+++ b/bfd/section.c
@@ -820,21 +820,21 @@  _bfd_generic_new_section_hook (bfd *abfd, asection *newsect)
   return TRUE;
 }
 
-static unsigned int section_id = 0x10;  /* id 0 to 3 used by STD_SECTION.  */
+unsigned int _bfd_section_id = 0x10;  /* id 0 to 3 used by STD_SECTION.  */
 
 /* Initializes a new section.  NEWSECT->NAME is already set.  */
 
 static asection *
 bfd_section_init (bfd *abfd, asection *newsect)
 {
-  newsect->id = section_id;
+  newsect->id = _bfd_section_id;
   newsect->index = abfd->section_count;
   newsect->owner = abfd;
 
   if (! BFD_SEND (abfd, _new_section_hook, (abfd, newsect)))
     return NULL;
 
-  section_id++;
+  _bfd_section_id++;
   abfd->section_count++;
   bfd_section_list_append (abfd, newsect);
   return newsect;
@@ -1284,23 +1284,6 @@  bfd_make_section (bfd *abfd, const char *name)
   return bfd_make_section_with_flags (abfd, name, 0);
 }
 
-/*
-FUNCTION
-	bfd_get_next_section_id
-
-SYNOPSIS
-	int bfd_get_next_section_id (void);
-
-DESCRIPTION
-	Returns the id that the next section created will have.
-*/
-
-int
-bfd_get_next_section_id (void)
-{
-  return section_id;
-}
-
 /*
 FUNCTION
 	bfd_set_section_flags