Re: [PATCH 3/5] remove deleted BFDs from the archive cache

[Tom Tromey <tromey@redhat.com>]

>>>>> "Alan" == Alan Modra <amodra@gmail.com> writes:

Alan> Here's something I hacked together to go a little further than your
Alan> patch and actually close archive member bfds when the archive is
Alan> closed.  You'll see I solved the problem of looking up the member BFD
Alan> by saving the filepos in arelt_data.  What I haven't done is make this
Alan> all work with thin archives, but that shouldn't be too hard.  
Alan> Just a matter of adding a field to struct areltdata instead of
Alan> reusing "origin", and solving the fact that my_archive isn't set for
Alan> thin archives, I think.  Care to run with this?

Here's a revised patch.

This patch rolls my various remaining patches into one.  I think it
remains pretty clear now.

It adds a couple of new fields to areltdata -- the origin for the
purposes of the cache, and the containing thin archive.  I looked a
little at reusing my_archive for thin archives, but this seemed complex.

I tested this under valgrind using a little program that opens an
archive and iterates over the contents, closing each member in turn.
Before the patch, I could provoke either memory leaks or crashes; but
now it is all clean.  I ran the test program on itself (to test the
non-archive case), an ordinary archive, a thin archive, and a thin
archive containing a nested archive.  I can send the test program if you
want.

Alan, in a private note you mentioned using bfd_read_p in the check in
_bfd_archive_close_and_cleanup.  I didn't understand this, so I left it out;
but I can certainly add it if you think it necessary.

Ok?

Tom

2012-08-08  Alan Modra  <amodra@gmail.com>
    	    Tom Tromey  <tromey@redhat.com>
    
    	* archive.c (SECTION Archives): Update documentation.
    	(_bfd_delete_archive_data): Remove.
    	(_bfd_get_elt_at_filepos): Set 'thin_archive' and
    	'cache_origin' fields on new_areldata.
    	(archive_close_worker, _bfd_archive_close_and_cleanup): New
    	functions.
    	* libbfd-in.h (struct areltdata) <cache_origin, thin_archive>:
    	New fields.
    	(_bfd_delete_archive_data): Don't declare.
    	(_bfd_archive_close_and_cleanup): Declare.
    	(_bfd_generic_close_and_cleanup): Redefine.
    	* libbfd.h: Rebuild.
    	* opncls.c (_bfd_delete_bfd): Don't call
    	_bfd_delete_archive_data.
    	(bfd_close): Don't close nested thin archives.

diff --git a/bfd/archive.c b/bfd/archive.c
index 1148c11..1b19f00 100644
--- a/bfd/archive.c
+++ b/bfd/archive.c
@@ -42,11 +42,17 @@ DESCRIPTION
 	have to read the entire archive if you don't want
 	to!  Read it until you find what you want.
 
+	A BFD returned by <<bfd_openr_next_archived_file>> can be
+	closed manually with <<bfd_close>>.  If you do not close it,
+	then a second iteration through the members of an archive may
+	return the same BFD.  If you close the archive BFD, then all
+	the member BFDs will automatically be closed as well.
+
 	Archive contents of output BFDs are chained through the
-	<<next>> pointer in a BFD.  The first one is findable through
-	the <<archive_head>> slot of the archive.  Set it with
-	<<bfd_set_archive_head>> (q.v.).  A given BFD may be in only one
-	open output archive at a time.
+	<<archive_next>> pointer in a BFD.  The first one is findable
+	through the <<archive_head>> slot of the archive.  Set it with
+	<<bfd_set_archive_head>> (q.v.).  A given BFD may be in only
+	one open output archive at a time.
 
 	As expected, the BFD archive code is more general than the
 	archive code of any given environment.  BFD archives may
@@ -293,19 +299,6 @@ bfd_set_archive_head (bfd *output_archive, bfd *new_head)
   return TRUE;
 }
 
-/* Free the archive hash table, if it exists.  */
-
-void
-_bfd_delete_archive_data (bfd *abfd)
-{
-  struct artdata *ardata = bfd_ardata (abfd);
-
-  BFD_ASSERT (abfd->format == bfd_archive);
-
-  if (ardata && ardata->cache)
-    htab_delete (ardata->cache);
-}
-
 bfd *
 _bfd_look_for_bfd_in_cache (bfd *arch_bfd, file_ptr filepos)
 {
@@ -678,6 +671,7 @@ _bfd_get_elt_at_filepos (bfd *archive, file_ptr filepos)
       n_nfd = bfd_openr (filename, target);
       if (n_nfd == NULL)
 	bfd_set_error (bfd_error_malformed_archive);
+      new_areldata->thin_archive = archive;
     }
   else
     {
@@ -702,6 +696,7 @@ _bfd_get_elt_at_filepos (bfd *archive, file_ptr filepos)
       n_nfd->filename = filename;
     }
 
+  new_areldata->cache_origin = filepos;
   n_nfd->arelt_data = new_areldata;
 
   /* Copy BFD_COMPRESS and BFD_DECOMPRESS flags.  */
@@ -2695,3 +2690,68 @@ coff_write_armap (bfd *arch,
 
   return TRUE;
 }
+
+static int
+archive_close_worker (void **slot, void *inf ATTRIBUTE_UNUSED)
+{
+  struct ar_cache *ent = (struct ar_cache *) *slot;
+
+  bfd_close_all_done (ent->arbfd);
+  return 1;
+}
+
+bfd_boolean
+_bfd_archive_close_and_cleanup (bfd *abfd)
+{
+  bfd *archive;
+
+  if (abfd->format == bfd_archive)
+    {
+      bfd *nbfd;
+      bfd *next;
+      htab_t htab;
+
+      /* Close nested archives (if this bfd is a thin archive).  */
+      for (nbfd = abfd->nested_archives; nbfd; nbfd = next)
+	{
+	  next = nbfd->archive_next;
+	  bfd_close (nbfd);
+	}
+
+      htab = bfd_ardata (abfd)->cache;
+      if (htab)
+	{
+	  htab_traverse_noresize (htab, archive_close_worker, NULL);
+	  htab_delete (htab);
+	  bfd_ardata (abfd)->cache = NULL;
+	}
+    }
+
+  if (abfd->my_archive != NULL)
+    archive = abfd->my_archive;
+  else if (arch_eltdata (abfd) != NULL
+	   && arch_eltdata (abfd)->thin_archive != NULL)
+    archive = arch_eltdata (abfd)->thin_archive;
+  else
+    archive = NULL;
+  if (archive != NULL)
+    {
+      htab_t htab = bfd_ardata (archive)->cache;
+
+      if (htab)
+	{
+	  struct ar_cache ent;
+	  void **slot;
+	  struct areltdata *ared = arch_eltdata (abfd);
+
+	  ent.ptr = ared->cache_origin;
+	  slot = htab_find_slot (htab, &ent, NO_INSERT);
+	  if (slot != NULL)
+	    {
+	      BFD_ASSERT (((struct ar_cache *) *slot)->arbfd == abfd);
+	      htab_clear_slot (htab, slot);
+	    }
+	}
+    }
+  return TRUE;
+}
diff --git a/bfd/libbfd-in.h b/bfd/libbfd-in.h
index 8cdb1c6..a557e36 100644
--- a/bfd/libbfd-in.h
+++ b/bfd/libbfd-in.h
@@ -96,6 +96,8 @@ struct areltdata
   bfd_size_type extra_size;	/* BSD4.4: extra bytes after the header.  */
   char *filename;		/* Null-terminated.  */
   file_ptr origin;		/* For element of a thin archive.  */
+  file_ptr cache_origin;	/* Origin as recorded in the archive cache.  */
+  struct bfd *thin_archive;     /* The containing thin archive.  */
 };
 
 #define arelt_size(bfd) (((struct areltdata *)((bfd)->arelt_data))->parsed_size)
@@ -130,8 +132,6 @@ extern void bfd_release
 
 bfd * _bfd_create_empty_archive_element_shell
   (bfd *obfd);
-void _bfd_delete_archive_data
-  (bfd *abfd);
 bfd * _bfd_look_for_bfd_in_cache
   (bfd *, file_ptr);
 bfd_boolean _bfd_add_bfd_to_archive_cache
@@ -232,7 +232,9 @@ int bfd_generic_stat_arch_elt
 /* Generic routines to use for BFD_JUMP_TABLE_GENERIC.  Use
    BFD_JUMP_TABLE_GENERIC (_bfd_generic).  */
 
-#define _bfd_generic_close_and_cleanup bfd_true
+#define _bfd_generic_close_and_cleanup _bfd_archive_close_and_cleanup
+extern bfd_boolean _bfd_archive_close_and_cleanup
+  (bfd *);
 #define _bfd_generic_bfd_free_cached_info bfd_true
 extern bfd_boolean _bfd_generic_new_section_hook
   (bfd *, asection *);
diff --git a/bfd/libbfd.h b/bfd/libbfd.h
index a1e544f..6b7055a 100644
--- a/bfd/libbfd.h
+++ b/bfd/libbfd.h
@@ -101,6 +101,8 @@ struct areltdata
   bfd_size_type extra_size;	/* BSD4.4: extra bytes after the header.  */
   char *filename;		/* Null-terminated.  */
   file_ptr origin;		/* For element of a thin archive.  */
+  file_ptr cache_origin;	/* Origin as recorded in the archive cache.  */
+  struct bfd *thin_archive;     /* The containing thin archive.  */
 };
 
 #define arelt_size(bfd) (((struct areltdata *)((bfd)->arelt_data))->parsed_size)
@@ -135,8 +137,6 @@ extern void bfd_release
 
 bfd * _bfd_create_empty_archive_element_shell
   (bfd *obfd);
-void _bfd_delete_archive_data
-  (bfd *abfd);
 bfd * _bfd_look_for_bfd_in_cache
   (bfd *, file_ptr);
 bfd_boolean _bfd_add_bfd_to_archive_cache
@@ -237,7 +237,9 @@ int bfd_generic_stat_arch_elt
 /* Generic routines to use for BFD_JUMP_TABLE_GENERIC.  Use
    BFD_JUMP_TABLE_GENERIC (_bfd_generic).  */
 
-#define _bfd_generic_close_and_cleanup bfd_true
+#define _bfd_generic_close_and_cleanup _bfd_archive_close_and_cleanup
+extern bfd_boolean _bfd_archive_close_and_cleanup
+  (bfd *);
 #define _bfd_generic_bfd_free_cached_info bfd_true
 extern bfd_boolean _bfd_generic_new_section_hook
   (bfd *, asection *);
diff --git a/bfd/opncls.c b/bfd/opncls.c
index e538981..b2ed9be 100644
--- a/bfd/opncls.c
+++ b/bfd/opncls.c
@@ -130,9 +130,6 @@ _bfd_new_bfd_contained_in (bfd *obfd)
 static void
 _bfd_delete_bfd (bfd *abfd)
 {
-  if (abfd->format == bfd_archive)
-    _bfd_delete_archive_data (abfd);
-
   if (abfd->memory)
     {
       bfd_hash_table_free (&abfd->section_htab);
@@ -711,8 +708,6 @@ bfd_boolean
 bfd_close (bfd *abfd)
 {
   bfd_boolean ret;
-  bfd *nbfd;
-  bfd *next;
 
   if (bfd_write_p (abfd))
     {
@@ -720,13 +715,6 @@ bfd_close (bfd *abfd)
 	return FALSE;
     }
 
-  /* Close nested archives (if this bfd is a thin archive).  */
-  for (nbfd = abfd->nested_archives; nbfd; nbfd = next)
-    {
-      next = nbfd->archive_next;
-      bfd_close (nbfd);
-    }
-
   if (! BFD_SEND (abfd, _close_and_cleanup, (abfd)))
     return FALSE;