Re: Add section caches to coff_data_type

Re: Add section caches to coff_data_type

[Alan Modra]

Commit 0e759f232b6d regressed these tests:
rs6000-aix7.2  +FAIL: Garbage collection test 1 (32-bit)
rs6000-aix7.2  +FAIL: Garbage collection test 1 (64-bit)
rs6000-aix7.2  +FAIL: Glink test 1 (32-bit)
rs6000-aix7.2  +FAIL: Glink test 1 (64-bit)

Investigation showed segfaults in coff_section_from_bfd_index called
by xcoff_write_global_symbol due to the hash table pointer being
NULL.  Well, yes, the hash table isn't initialised for the output bfd.
mkobject_hook is the wrong place to do that.

	* coffcode.h: Revert 0e759f232b6d changes.
	* peicode.h: Likewise.
	* coff-x86_64.c (htab_hash_section_index, htab_eq_section_index):
	Moved here from coffcode.h.
	(coff_amd64_rtype_to_howto): Create section_by_index htab.
	* coffgen.c (htab_hash_section_target_index),
	(htab_eq_section_target_index): Moved here from coffcode.h.
	(coff_section_from_bfd_index): Create section_by_target_index
	htab.  Stash newly created sections in htab.

diff --git a/bfd/coff-x86_64.c b/bfd/coff-x86_64.c
index 1f8c8d515c4..f5bd5a27c40 100644
--- a/bfd/coff-x86_64.c
+++ b/bfd/coff-x86_64.c
@@ -656,6 +656,20 @@ coff_pe_amd64_relocate_section (bfd *output_bfd,
 
 #define coff_relocate_section coff_pe_amd64_relocate_section
 
+static hashval_t
+htab_hash_section_index (const void * entry)
+{
+  const struct bfd_section * sec = entry;
+  return sec->index;
+}
+
+static int
+htab_eq_section_index (const void * e1, const void * e2)
+{
+  const struct bfd_section * sec1 = e1;
+  const struct bfd_section * sec2 = e2;
+  return sec1->index == sec2->index;
+}
 #endif /* COFF_WITH_PE */
 
 /* Convert an rtype to howto for the COFF backend linker.  */
@@ -756,11 +770,17 @@ coff_amd64_rtype_to_howto (bfd *abfd ATTRIBUTE_UNUSED,
 	  htab_t table = coff_data (abfd)->section_by_index;
 	  asection *s;
 
+	  if (!table)
+	    {
+	      table = htab_create (10, htab_hash_section_index,
+				   htab_eq_section_index, NULL);
+	      if (table == NULL)
+		return NULL;
+	      coff_data (abfd)->section_by_index = table;
+	    }
+
 	  if (htab_elements (table) == 0)
 	    {
-	      /* Sigh - if we do not have a section index then the only way
-		 to get the section to offset against is to find it the hard
-		 way.  */
 	      for (s = abfd->sections; s != NULL; s = s->next)
 		{
 		  void ** slot = htab_find_slot (table, s, INSERT);
diff --git a/bfd/coffcode.h b/bfd/coffcode.h
index ab6f797b324..777515e82e1 100644
--- a/bfd/coffcode.h
+++ b/bfd/coffcode.h
@@ -731,36 +731,6 @@ sec_to_styp_flags (const char *sec_name, flagword sec_flags)
 
 #ifndef COFF_WITH_PE
 
-static hashval_t
-htab_hash_section_index (const void * entry)
-{
-  const struct bfd_section * sec = entry;
-  return sec->index;
-}
-
-static int
-htab_eq_section_index (const void * e1, const void * e2)
-{
-  const struct bfd_section * sec1 = e1;
-  const struct bfd_section * sec2 = e2;
-  return sec1->index == sec2->index;
-}
-
-static hashval_t
-htab_hash_section_target_index (const void * entry)
-{
-  const struct bfd_section * sec = entry;
-  return sec->target_index;
-}
-
-static int
-htab_eq_section_target_index (const void * e1, const void * e2)
-{
-  const struct bfd_section * sec1 = e1;
-  const struct bfd_section * sec2 = e2;
-  return sec1->target_index == sec2->target_index;
-}
-
 static bool
 styp_to_sec_flags (bfd *abfd,
 		   void * hdr,
@@ -2185,11 +2155,6 @@ coff_mkobject_hook (bfd * abfd,
     abfd->flags |= HAS_DEBUG;
 #endif
 
-  coff->section_by_index
-    = htab_create (10, htab_hash_section_index, htab_eq_section_index, NULL);
-  coff->section_by_target_index = htab_create
-    (10, htab_hash_section_target_index, htab_eq_section_target_index, NULL);
-
   return coff;
 }
 #endif
diff --git a/bfd/coffgen.c b/bfd/coffgen.c
index 03b64ac6762..d4c14fba1d9 100644
--- a/bfd/coffgen.c
+++ b/bfd/coffgen.c
@@ -356,6 +356,21 @@ coff_object_p (bfd *abfd)
 			      : (struct internal_aouthdr *) NULL));
 }
 
+static hashval_t
+htab_hash_section_target_index (const void * entry)
+{
+  const struct bfd_section * sec = entry;
+  return sec->target_index;
+}
+
+static int
+htab_eq_section_target_index (const void * e1, const void * e2)
+{
+  const struct bfd_section * sec1 = e1;
+  const struct bfd_section * sec2 = e2;
+  return sec1->target_index == sec2->target_index;
+}
+
 /* Get the BFD section from a COFF symbol section number.  */
 
 asection *
@@ -371,17 +386,23 @@ coff_section_from_bfd_index (bfd *abfd, int section_index)
   struct bfd_section *answer;
   htab_t table = coff_data (abfd)->section_by_target_index;
 
-  if (htab_elements (table) == 0)
+  if (!table)
     {
-      answer = abfd->sections;
+      table = htab_create (10, htab_hash_section_target_index,
+			   htab_eq_section_target_index, NULL);
+      if (table == NULL)
+	return bfd_und_section_ptr;
+      coff_data (abfd)->section_by_target_index = table;
+    }
 
-      while (answer)
+  if (htab_elements (table) == 0)
+    {
+      for (answer = abfd->sections; answer; answer = answer->next)
 	{
 	  void **slot = htab_find_slot (table, answer, INSERT);
 	  if (slot == NULL)
 	    return bfd_und_section_ptr;
 	  *slot = answer;
-	  answer = answer->next;
 	}
     }
 
@@ -392,14 +413,16 @@ coff_section_from_bfd_index (bfd *abfd, int section_index)
   if (answer != NULL)
     return answer;
 
-  answer = abfd->sections;
-
-  while (answer)
-    {
-      if (answer->target_index == section_index)
+  /* Cover the unlikely case of sections added after the first call to
+     this function.  */
+  for (answer = abfd->sections; answer; answer = answer->next)
+    if (answer->target_index == section_index)
+      {
+	void **slot = htab_find_slot (table, answer, INSERT);
+	if (slot != NULL)
+	  *slot = answer;
 	return answer;
-      answer = answer->next;
-    }
+      }
 
   /* We should not reach this point, but the SCO 3.2v4 /lib/libc_s.a
      has a bad symbol table in biglitpow.o.  */
diff --git a/bfd/peicode.h b/bfd/peicode.h
index 436ff54fbea..e2e2be65b5d 100644
--- a/bfd/peicode.h
+++ b/bfd/peicode.h
@@ -255,36 +255,6 @@ coff_swap_scnhdr_in (bfd * abfd, void * ext, void * in)
 #endif
 }
 
-static hashval_t
-htab_hash_section_index (const void * entry)
-{
-  const struct bfd_section * sec = entry;
-  return sec->index;
-}
-
-static int
-htab_eq_section_index (const void * e1, const void * e2)
-{
-  const struct bfd_section * sec1 = e1;
-  const struct bfd_section * sec2 = e2;
-  return sec1->index == sec2->index;
-}
-
-static hashval_t
-htab_hash_section_target_index (const void * entry)
-{
-  const struct bfd_section * sec = entry;
-  return sec->target_index;
-}
-
-static int
-htab_eq_section_target_index (const void * e1, const void * e2)
-{
-  const struct bfd_section * sec1 = e1;
-  const struct bfd_section * sec2 = e2;
-  return sec1->target_index == sec2->target_index;
-}
-
 static bool
 pe_mkobject (bfd * abfd)
 {
@@ -382,11 +352,6 @@ pe_mkobject_hook (bfd * abfd,
   memcpy (pe->dos_message, internal_f->pe.dos_message,
 	  sizeof (pe->dos_message));
 
-  pe->coff.section_by_index
-    = htab_create (10, htab_hash_section_index, htab_eq_section_index, NULL);
-  pe->coff.section_by_target_index = htab_create
-    (10, htab_hash_section_target_index, htab_eq_section_target_index, NULL);
-
   return (void *) pe;
 }
 

-- 
Alan Modra
Australia Development Lab, IBM

Re: Add section caches to coff_data_type

[Alan Modra]

On Thu, May 18, 2023 at 12:24:27PM +0930, Alan Modra wrote:
> NULL.  Well, yes, the hash table isn't initialised for the output bfd.
> mkobject_hook is the wrong place to do that.

One other reason why it's the wrong place to call htab_create is that
any memory malloc'd in an object_p function should result in the
object_p returning a bfd_cleanup function that frees the memory.
bfd_alloc memory is fine in object_p, as alloc'd memory handled by
bfd_check_format_matches.

Another thing, section target_index is renumbered in
coff_compute_section_file_positions and _bfd_xcoff_bfd_final_link.  I
don't know that there is currently any way that the output bfd
section_by_target_index could be populated before this point but
clear them out so no one need worry about it.

	* coffcode.h (coff_compute_section_file_positions): Clear
	section_by_target_index hash table when changing target_index.
	(_bfd_xcoff_bfd_final_link): Likewise.

diff --git a/bfd/coffcode.h b/bfd/coffcode.h
index 777515e82e1..2d429828822 100644
--- a/bfd/coffcode.h
+++ b/bfd/coffcode.h
@@ -3067,6 +3067,9 @@ coff_compute_section_file_positions (bfd * abfd)
       sofar += bfd_coff_scnhsz (abfd);
 #endif
 
+  if (coff_data (abfd)->section_by_target_index)
+    htab_empty (coff_data (abfd)->section_by_target_index);
+
 #ifdef COFF_IMAGE_WITH_PE
   {
     /* PE requires the sections to be in memory order when listed in
diff --git a/bfd/xcofflink.c b/bfd/xcofflink.c
index b57ed21afb9..a27721301dd 100644
--- a/bfd/xcofflink.c
+++ b/bfd/xcofflink.c
@@ -7076,6 +7076,8 @@ _bfd_xcoff_bfd_final_link (bfd *abfd, struct bfd_link_info *info)
 
 	  /* Reset the section indices after inserting the new
 	     sections.  */
+	  if (xcoff_data (abfd)->coff.section_by_target_index)
+	    htab_empty (xcoff_data (abfd)->coff.section_by_target_index);
 	  indx = 0;
 	  for (o = abfd->sections; o != NULL; o = o->next)
 	    {

-- 
Alan Modra
Australia Development Lab, IBM