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

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

[Tom Tromey]

Right now if you bfd_close a member BFD, the pointer to the BFD
remains in the parent archive's cache.  This means that if you try to
re-open that member, BFD will return a stale pointer.

This patch fixes the problem by deleting the entry from the hash.

I could not find a way to look up the member BFD in its parent cache
directly, hence the iteration.

	* archive.c (delete_bfd_from_htab): New function.
	(_bfd_delete_archive_data): Iterate over the parent archive's
	hash table.
---
 bfd/archive.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/bfd/archive.c b/bfd/archive.c
index 8d02257..f84a8fc 100644
--- a/bfd/archive.c
+++ b/bfd/archive.c
@@ -293,6 +293,24 @@ bfd_set_archive_head (bfd *output_archive, bfd *new_head)
   return TRUE;
 }
 
+/* A callback for htab_traverse that finds and clears an entry
+   corresponding to a given BFD.  */
+
+static int
+delete_bfd_from_htab (void **slot, void *arg)
+{
+  struct ar_cache *cache = *slot;
+  bfd *abfd = arg;
+
+  if (cache->arbfd == abfd)
+    {
+      *slot = HTAB_DELETED_ENTRY;
+      return 0;
+    }
+
+  return 1;
+}
+
 /* Free the archive hash table, if it exists.  */
 
 void
@@ -302,6 +320,17 @@ _bfd_delete_archive_data (bfd *abfd)
 
   if (ardata && ardata->cache)
     htab_delete (ardata->cache);
+
+  if (abfd->my_archive)
+    {
+      ardata = bfd_ardata (abfd->my_archive);
+      if (ardata && ardata->cache)
+	{
+	  /* We have to traverse the hash table because there is no
+	     way to find ABFD in it directly.  */
+	  htab_traverse_noresize (ardata->cache, delete_bfd_from_htab, abfd);
+	}
+    }
 }
 
 bfd *
-- 
1.7.7.6

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

[Alan Modra]

On Fri, Aug 03, 2012 at 08:54:40AM -0600, Tom Tromey wrote:
> I could not find a way to look up the member BFD in its parent cache
> directly, hence the iteration.

You've made closing an archive O(n^2) on number of elements.  That I
dislike enough to NAK the patch.

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

Index: bfd/archive.c
===================================================================
RCS file: /cvs/src/src/bfd/archive.c,v
retrieving revision 1.87
diff -u -p -r1.87 archive.c
--- bfd/archive.c	13 Jul 2012 14:22:42 -0000	1.87
+++ bfd/archive.c	3 Aug 2012 14:59:07 -0000
@@ -316,7 +316,8 @@ _bfd_look_for_bfd_in_cache (bfd *arch_bf
 static hashval_t
 hash_file_ptr (const void * p)
 {
-  return (hashval_t) (((struct ar_cache *) p)->ptr);
+  struct ar_cache *ent = (struct ar_cache *) p;
+  return (hashval_t) ent->ptr;
 }
 
 /* Returns non-zero if P1 and P2 are equal.  */
@@ -422,8 +423,6 @@ get_extended_arelt_filename (bfd *arch, 
 	}
       *originp = origin;
     }
-  else
-    *originp = 0;
 
   return bfd_ardata (arch)->extended_names + table_index;
 }
@@ -687,6 +686,7 @@ _bfd_get_elt_at_filepos (bfd *archive, f
     {
       n_nfd->origin = n_nfd->proxy_origin;
       n_nfd->filename = filename;
+      new_areldata->origin = filepos;
     }
 
   n_nfd->arelt_data = new_areldata;
@@ -2682,3 +2682,58 @@ 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)
+{
+  if (bfd_check_format (abfd, 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;
+	}
+    }
+  else if (abfd->my_archive != NULL)
+    {
+      htab_t htab = bfd_ardata (abfd->my_archive)->cache;
+
+      if (htab)
+	{
+	  struct ar_cache ent;
+	  void **slot;
+	  struct areltdata *ared = arch_eltdata (abfd);
+
+	  ent.ptr = ared->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;
+}
Index: bfd/libbfd-in.h
===================================================================
RCS file: /cvs/src/src/bfd/libbfd-in.h,v
retrieving revision 1.99
diff -u -p -r1.99 libbfd-in.h
--- bfd/libbfd-in.h	24 Jul 2012 21:06:58 -0000	1.99
+++ bfd/libbfd-in.h	3 Aug 2012 14:59:10 -0000
@@ -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 *);
Index: bfd/opncls.c
===================================================================
RCS file: /cvs/src/src/bfd/opncls.c,v
retrieving revision 1.69
diff -u -p -r1.69 opncls.c
--- bfd/opncls.c	29 May 2012 14:23:33 -0000	1.69
+++ bfd/opncls.c	3 Aug 2012 14:59:11 -0000
@@ -707,8 +707,6 @@ bfd_boolean
 bfd_close (bfd *abfd)
 {
   bfd_boolean ret;
-  bfd *nbfd;
-  bfd *next;
 
   if (bfd_write_p (abfd))
     {
@@ -716,13 +714,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;
 

-- 
Alan Modra
Australia Development Lab, IBM

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

[Tom Tromey]

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?

Sure, I will take it on.

Tom

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

[Tom Tromey]

>>>>> "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;
 

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

[Alan Modra]

On Wed, Aug 08, 2012 at 11:21:02AM -0600, Tom Tromey wrote:
> Alan, in a private note you mentioned using bfd_read_p in the check in
> _bfd_archive_close_and_cleanup.

I'm reasonably sure that the code as it was doesn't mess with archive
bfds opened for writing, but wanted to make it obvious.  I've also
moved the code setting new areltdata fields (and renamed them) to
_bfd_add_bfd_to_archive_cache to make the code a bit tidier.
Committed.

2012-08-09  Alan Modra  <amodra@gmail.com>
	    Tom Tromey  <tromey@redhat.com>

	* archive.c (SECTION Archives): Update documentation.
	(_bfd_delete_archive_data): Remove.
	(_bfd_add_bfd_to_archive_cache): Set 'parent_cache' and 'key'.
	(archive_close_worker, _bfd_archive_close_and_cleanup): New
	functions.
	* libbfd-in.h (struct areltdata <parent_cache, key>): 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 here.

Index: bfd/archive.c
===================================================================
RCS file: /cvs/src/src/bfd/archive.c,v
retrieving revision 1.88
diff -u -p -r1.88 archive.c
--- bfd/archive.c	7 Aug 2012 13:47:14 -0000	1.88
+++ bfd/archive.c	9 Aug 2012 04:51:31 -0000
@@ -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_archiv
   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)
 {
@@ -375,6 +368,10 @@ _bfd_add_bfd_to_archive_cache (bfd *arch
   cache->arbfd = new_elt;
   *htab_find_slot (hash_table, (const void *) cache, INSERT) = cache;
 
+  /* Provide a means of accessing this from child.  */
+  arch_eltdata (new_elt)->parent_cache = hash_table;
+  arch_eltdata (new_elt)->key = filepos;
+
   return TRUE;
 }
 \f
@@ -2695,3 +2692,58 @@ 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)
+{
+  if (bfd_read_p (abfd) && 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;
+	}
+    }
+  else if (arch_eltdata (abfd) != NULL)
+    {
+      struct areltdata *ared = arch_eltdata (abfd);
+      htab_t htab = (htab_t) ared->parent_cache;
+
+      if (htab)
+	{
+	  struct ar_cache ent;
+	  void **slot;
+
+	  ent.ptr = ared->key;
+	  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;
+}
Index: bfd/libbfd-in.h
===================================================================
RCS file: /cvs/src/src/bfd/libbfd-in.h,v
retrieving revision 1.101
diff -u -p -r1.101 libbfd-in.h
--- bfd/libbfd-in.h	7 Aug 2012 13:47:14 -0000	1.101
+++ bfd/libbfd-in.h	9 Aug 2012 04:51:31 -0000
@@ -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.  */
+  void *parent_cache;		/* Where and how to find this member.  */
+  file_ptr key;
 };
 
 #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 *);
Index: bfd/opncls.c
===================================================================
RCS file: /cvs/src/src/bfd/opncls.c,v
retrieving revision 1.71
diff -u -p -r1.71 opncls.c
--- bfd/opncls.c	7 Aug 2012 13:47:15 -0000	1.71
+++ bfd/opncls.c	9 Aug 2012 04:51:31 -0000
@@ -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;
 

-- 
Alan Modra
Australia Development Lab, IBM

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

[H.J. Lu]

On Thu, Aug 9, 2012 at 3:15 AM, Alan Modra <amodra@gmail.com> wrote:
> On Wed, Aug 08, 2012 at 11:21:02AM -0600, Tom Tromey wrote:
>> Alan, in a private note you mentioned using bfd_read_p in the check in
>> _bfd_archive_close_and_cleanup.
>
> I'm reasonably sure that the code as it was doesn't mess with archive
> bfds opened for writing, but wanted to make it obvious.  I've also
> moved the code setting new areltdata fields (and renamed them) to
> _bfd_add_bfd_to_archive_cache to make the code a bit tidier.
> Committed.
>
> 2012-08-09  Alan Modra  <amodra@gmail.com>
>             Tom Tromey  <tromey@redhat.com>
>
>         * archive.c (SECTION Archives): Update documentation.
>         (_bfd_delete_archive_data): Remove.
>         (_bfd_add_bfd_to_archive_cache): Set 'parent_cache' and 'key'.
>         (archive_close_worker, _bfd_archive_close_and_cleanup): New
>         functions.
>         * libbfd-in.h (struct areltdata <parent_cache, key>): 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 here.
>
>

It breaks strip:

http://sourceware.org/bugzilla/show_bug.cgi?id=14475


-- 
H.J.

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

[Tom Tromey]

HJ> It breaks strip:
HJ> http://sourceware.org/bugzilla/show_bug.cgi?id=14475

Thanks

Here is a patch to fix the problem.

The bug is that bfd_ar_hdr_from_filesystem creates the areltdata on the
archive's objalloc.  But, since this data is attached to the member BFD,
it should really be created on the member's objalloc.

This also fixes another bug I noticed in bfd_ar_hdr_from_filesystem.
This code tries to free 'ared' in one case -- but that is wrong, as this
is not allocated using malloc.

I could see the bug before this patch using valgrind or -lmcheck.  After
the patch the problem is gone.

Ok?

Tom

2012-08-15  Tom Tromey  <tromey@redhat.com>

	PR binutils/14475:
	* archive.c (bfd_ar_hdr_from_filesystem): Allocate areltdata on
	'member' BFD.  Don't try to free 'ared'.

Index: archive.c
===================================================================
RCS file: /cvs/src/src/bfd/archive.c,v
retrieving revision 1.89
diff -u -r1.89 archive.c
--- archive.c	9 Aug 2012 06:25:52 -0000	1.89
+++ archive.c	15 Aug 2012 21:01:51 -0000
@@ -1896,7 +1896,7 @@
     }
 
   amt = sizeof (struct ar_hdr) + sizeof (struct areltdata);
-  ared = (struct areltdata *) bfd_zalloc (abfd, amt);
+  ared = (struct areltdata *) bfd_zalloc (member, amt);
   if (ared == NULL)
     return NULL;
   hdr = (struct ar_hdr *) (((char *) ared) + sizeof (struct areltdata));
@@ -1927,10 +1927,7 @@
   _bfd_ar_spacepad (hdr->ar_mode, sizeof (hdr->ar_mode), "%-8lo",
 		    status.st_mode);
   if (!_bfd_ar_sizepad (hdr->ar_size, sizeof (hdr->ar_size), status.st_size))
-    {
-      free (ared);
-      return NULL;
-    }
+    return NULL;
   memcpy (hdr->ar_fmag, ARFMAG, 2);
   ared->parsed_size = status.st_size;
   ared->arch_header = (char *) hdr;

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

[Alan Modra]

On Wed, Aug 15, 2012 at 03:03:38PM -0600, Tom Tromey wrote:
> 	PR binutils/14475:
> 	* archive.c (bfd_ar_hdr_from_filesystem): Allocate areltdata on
> 	'member' BFD.  Don't try to free 'ared'.

OK, thanks!

-- 
Alan Modra
Australia Development Lab, IBM

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

[H.J. Lu]

On Thu, Aug 16, 2012 at 6:52 AM, Alan Modra <amodra@gmail.com> wrote:
> On Wed, Aug 15, 2012 at 03:03:38PM -0600, Tom Tromey wrote:
>>       PR binutils/14475:
>>       * archive.c (bfd_ar_hdr_from_filesystem): Allocate areltdata on
>>       'member' BFD.  Don't try to free 'ared'.
>
> OK, thanks!
>

Hi Tom,

Your checkin destroys binutils:

http://sourceware.org/bugzilla/show_bug.cgi?id=14475

Can you fix it?

Thanks.


-- 
H.J.

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

[Tom Tromey]

HJ> Your checkin destroys binutils:
HJ> http://sourceware.org/bugzilla/show_bug.cgi?id=14475
HJ> Can you fix it?

Sorry about that.  I see now that I will need a more clever plan to
destroy binutils.

I debugged it.  The immediate problem is that
_bfd_compute_and_write_armap calls bfd_free_cached_info
(aka _bfd_free_cached_info).  This then does:

      objalloc_free ((struct objalloc *) abfd->memory);

Whoops, this also frees the areltdata.

So, I have two possible fixes for it.

I've appended the first possible fix.  It changes _bfd_free_cached_info
not to free all the memory attached to the BFD.

A couple notes here.

First, it seems very wrong to me to clear usrdata in this function.  I
didn't touch this; since presumably clients may be relying on this
clearing in a subtle way (if they allocate the usrdata on the BFD
objalloc, which is perhaps the only sensible approach anyhow).  But, I
think that if the appended patch goes in then this line should be
removed in a follow-up.

Second, the check in _bfd_delete_bfd is perhaps ugly.  Maybe
bfd_hash_table_free should do this check instead.  Let me know what you
think.

I rebuilt ld and binutils with this patch.  Additionally, I hacked the
Makefiles to link all the programs with -lmcheck.  Then I ran the ld and
binutils test suites.  There were no regressions.  I also examined one
particular case from ar.exp using valgrind -- I could reproduce the
problem before the patch, but not after.


Another possible fix for this bug would be to allocate the areltdata
using malloc.  That way it would be immune to the objalloc_free call.
This would require a few more tweaks, like properly freeing it in
_bfd_delete_bfd, etc.

I'm happy to make and test this change if you think it would be better.

Tom

2012-08-16  Tom Tromey  <tromey@redhat.com>

	* opncls.c (_bfd_delete_bfd): Check to see if section htab is
	already freed.
	(_bfd_free_cached_info): Don't free the objalloc.

Index: opncls.c
===================================================================
RCS file: /cvs/src/src/bfd/opncls.c,v
retrieving revision 1.72
diff -u -r1.72 opncls.c
--- opncls.c	9 Aug 2012 06:25:53 -0000	1.72
+++ opncls.c	16 Aug 2012 16:57:36 -0000
@@ -132,14 +132,15 @@
 {
   if (abfd->memory)
     {
-      bfd_hash_table_free (&abfd->section_htab);
+      if (abfd->section_htab.memory != NULL)
+	bfd_hash_table_free (&abfd->section_htab);
       objalloc_free ((struct objalloc *) abfd->memory);
     }
 
   free (abfd);
 }
 
-/* Free objalloc memory.  */
+/* Free some information cached in the BFD.  */
 
 bfd_boolean
 _bfd_free_cached_info (bfd *abfd)
@@ -147,14 +148,12 @@
   if (abfd->memory)
     {
       bfd_hash_table_free (&abfd->section_htab);
-      objalloc_free ((struct objalloc *) abfd->memory);
 
       abfd->sections = NULL;
       abfd->section_last = NULL;
       abfd->outsymbols = NULL;
       abfd->tdata.any = NULL;
       abfd->usrdata = NULL;
-      abfd->memory = NULL;
     }
 
   return TRUE;

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

[H.J. Lu]

On Thu, Aug 16, 2012 at 10:02 AM, Tom Tromey <tromey@redhat.com> wrote:
> HJ> Your checkin destroys binutils:
> HJ> http://sourceware.org/bugzilla/show_bug.cgi?id=14475
> HJ> Can you fix it?
>
> Sorry about that.  I see now that I will need a more clever plan to
> destroy binutils.
>
> I debugged it.  The immediate problem is that
> _bfd_compute_and_write_armap calls bfd_free_cached_info
> (aka _bfd_free_cached_info).  This then does:
>
>       objalloc_free ((struct objalloc *) abfd->memory);
>
> Whoops, this also frees the areltdata.
>
> So, I have two possible fixes for it.
>
> I've appended the first possible fix.  It changes _bfd_free_cached_info
> not to free all the memory attached to the BFD.
>
> A couple notes here.
>
> First, it seems very wrong to me to clear usrdata in this function.  I
> didn't touch this; since presumably clients may be relying on this
> clearing in a subtle way (if they allocate the usrdata on the BFD
> objalloc, which is perhaps the only sensible approach anyhow).  But, I
> think that if the appended patch goes in then this line should be
> removed in a follow-up.
>
> Second, the check in _bfd_delete_bfd is perhaps ugly.  Maybe
> bfd_hash_table_free should do this check instead.  Let me know what you
> think.
>
> I rebuilt ld and binutils with this patch.  Additionally, I hacked the
> Makefiles to link all the programs with -lmcheck.  Then I ran the ld and
> binutils test suites.  There were no regressions.  I also examined one
> particular case from ar.exp using valgrind -- I could reproduce the
> problem before the patch, but not after.
>
>
> Another possible fix for this bug would be to allocate the areltdata
> using malloc.  That way it would be immune to the objalloc_free call.
> This would require a few more tweaks, like properly freeing it in
> _bfd_delete_bfd, etc.
>
> I'm happy to make and test this change if you think it would be better.
>
> Tom
>
> 2012-08-16  Tom Tromey  <tromey@redhat.com>
>
>         * opncls.c (_bfd_delete_bfd): Check to see if section htab is
>         already freed.
>         (_bfd_free_cached_info): Don't free the objalloc.
>
> Index: opncls.c
> ===================================================================
> RCS file: /cvs/src/src/bfd/opncls.c,v
> retrieving revision 1.72
> diff -u -r1.72 opncls.c
> --- opncls.c    9 Aug 2012 06:25:53 -0000       1.72
> +++ opncls.c    16 Aug 2012 16:57:36 -0000
> @@ -132,14 +132,15 @@
>  {
>    if (abfd->memory)
>      {
> -      bfd_hash_table_free (&abfd->section_htab);
> +      if (abfd->section_htab.memory != NULL)
> +       bfd_hash_table_free (&abfd->section_htab);
>        objalloc_free ((struct objalloc *) abfd->memory);
>      }
>
>    free (abfd);
>  }
>
> -/* Free objalloc memory.  */
> +/* Free some information cached in the BFD.  */
>
>  bfd_boolean
>  _bfd_free_cached_info (bfd *abfd)
> @@ -147,14 +148,12 @@
>    if (abfd->memory)
>      {
>        bfd_hash_table_free (&abfd->section_htab);
> -      objalloc_free ((struct objalloc *) abfd->memory);
>
>        abfd->sections = NULL;
>        abfd->section_last = NULL;
>        abfd->outsymbols = NULL;
>        abfd->tdata.any = NULL;
>        abfd->usrdata = NULL;
> -      abfd->memory = NULL;
>      }
>
>    return TRUE;

I think we should step back to take a loot at the original problem:

Right now if you bfd_close a member BFD, the pointer to the BFD
remains in the parent archive's cache.  This means that if you try to
re-open that member, BFD will return a stale pointer.

So far all the checked-in changes have some issues.  The original
codes were written in such a way to properly handle archive operation.
The only missing part was to properly handle the member cache
pointer.  I think we should open a new bug to track the original problem
with a testcase and to see if there is a less intrusive way to fix it instead
making major changes to archive handling codes.

Thanks.

-- 
H.J.

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

[Tom Tromey]

>>>>> "HJ" == H J Lu <hjl.tools@gmail.com> writes:

HJ> The original codes were written in such a way to properly handle
HJ> archive operation.

I don't agree.  The original codes were inconsistent in what they did,
presumably because BFD gave no guidance as to what was correct.

HJ> The only missing part was to properly handle the member cache
HJ> pointer.

BFD also leaked the archive member hash tables.

HJ> I think we should open a new bug to track the original problem with
HJ> a testcase and to see if there is a less intrusive way to fix it
HJ> instead making major changes to archive handling codes.

If you mean reverting the patches, I don't agree with it, but that is up
to you.

Tom

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

[H.J. Lu]

On Thu, Aug 16, 2012 at 11:33 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "HJ" == H J Lu <hjl.tools@gmail.com> writes:
>
> HJ> The original codes were written in such a way to properly handle
> HJ> archive operation.
>
> I don't agree.  The original codes were inconsistent in what they did,
> presumably because BFD gave no guidance as to what was correct.
>
> HJ> The only missing part was to properly handle the member cache
> HJ> pointer.
>
> BFD also leaked the archive member hash tables.

Do you have testcases for those problems?

> HJ> I think we should open a new bug to track the original problem with
> HJ> a testcase and to see if there is a less intrusive way to fix it
> HJ> instead making major changes to archive handling codes.
>
> If you mean reverting the patches, I don't agree with it, but that is up
> to you.
>

I don't think it is a bad idea.  One issue I have with

http://sourceware.org/ml/binutils/2012-08/msg00170.html

is there is no testcase to verify that it fixes any problems.

We can always put them back in after addressing all the issues.

Thanks.

-- 
H.J.

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

[Tom Tromey]

HJ> Do you have testcases for those problems?

Just running valgrind using a test program.

Tom> If you mean reverting the patches, I don't agree with it, but that is up
Tom> to you.

HJ> I don't think it is a bad idea.

Here's a reversion patch.
I couldn't find a ChangeLog for one of the patches, the one Nick
committed.

I'll open some bugs.

If you check this in, please let me know so I can revert the gdb patch.
Thanks.

Tom

2012-08-16  Tom Tromey  <tromey@redhat.com>

	* archive.c, libbfd-in.h, libbfd.h, opncls.c: Revert patches
	relating to archive handling:
	2012-08-16  Tom Tromey  <tromey@redhat.com>
	2012-08-09  Alan Modra  <amodra@gmail.com>
		    Tom Tromey  <tromey@redhat.com>

diff --git a/bfd/archive.c b/bfd/archive.c
index e0cb370..fe57755 100644
--- a/bfd/archive.c
+++ b/bfd/archive.c
@@ -42,17 +42,11 @@ 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
-	<<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.
+	<<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
@@ -368,10 +362,6 @@ _bfd_add_bfd_to_archive_cache (bfd *arch_bfd, file_ptr filepos, bfd *new_elt)
   cache->arbfd = new_elt;
   *htab_find_slot (hash_table, (const void *) cache, INSERT) = cache;
 
-  /* Provide a means of accessing this from child.  */
-  arch_eltdata (new_elt)->parent_cache = hash_table;
-  arch_eltdata (new_elt)->key = filepos;
-
   return TRUE;
 }
 \f
@@ -1896,7 +1886,7 @@ bfd_ar_hdr_from_filesystem (bfd *abfd, const char *filename, bfd *member)
     }
 
   amt = sizeof (struct ar_hdr) + sizeof (struct areltdata);
-  ared = (struct areltdata *) bfd_zalloc (member, amt);
+  ared = (struct areltdata *) bfd_zalloc (abfd, amt);
   if (ared == NULL)
     return NULL;
   hdr = (struct ar_hdr *) (((char *) ared) + sizeof (struct areltdata));
@@ -1927,7 +1917,10 @@ bfd_ar_hdr_from_filesystem (bfd *abfd, const char *filename, bfd *member)
   _bfd_ar_spacepad (hdr->ar_mode, sizeof (hdr->ar_mode), "%-8lo",
 		    status.st_mode);
   if (!_bfd_ar_sizepad (hdr->ar_size, sizeof (hdr->ar_size), status.st_size))
-    return NULL;
+    {
+      free (ared);
+      return NULL;
+    }
   memcpy (hdr->ar_fmag, ARFMAG, 2);
   ared->parsed_size = status.st_size;
   ared->arch_header = (char *) hdr;
@@ -2689,58 +2682,3 @@ 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)
-{
-  if (bfd_read_p (abfd) && 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;
-	}
-    }
-  else if (arch_eltdata (abfd) != NULL)
-    {
-      struct areltdata *ared = arch_eltdata (abfd);
-      htab_t htab = (htab_t) ared->parent_cache;
-
-      if (htab)
-	{
-	  struct ar_cache ent;
-	  void **slot;
-
-	  ent.ptr = ared->key;
-	  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 80cb051..1495825 100644
--- a/bfd/libbfd-in.h
+++ b/bfd/libbfd-in.h
@@ -96,8 +96,6 @@ 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.  */
-  void *parent_cache;		/* Where and how to find this member.  */
-  file_ptr key;
 };
 
 #define arelt_size(bfd) (((struct areltdata *)((bfd)->arelt_data))->parsed_size)
@@ -232,9 +230,7 @@ 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_archive_close_and_cleanup
-extern bfd_boolean _bfd_archive_close_and_cleanup
-  (bfd *);
+#define _bfd_generic_close_and_cleanup bfd_true
 #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 88ff9c6..077f1fb 100644
--- a/bfd/libbfd.h
+++ b/bfd/libbfd.h
@@ -101,8 +101,6 @@ 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.  */
-  void *parent_cache;		/* Where and how to find this member.  */
-  file_ptr key;
 };
 
 #define arelt_size(bfd) (((struct areltdata *)((bfd)->arelt_data))->parsed_size)
@@ -237,9 +235,7 @@ 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_archive_close_and_cleanup
-extern bfd_boolean _bfd_archive_close_and_cleanup
-  (bfd *);
+#define _bfd_generic_close_and_cleanup bfd_true
 #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 b2ed9be..0c02ee4 100644
--- a/bfd/opncls.c
+++ b/bfd/opncls.c
@@ -135,7 +135,6 @@ _bfd_delete_bfd (bfd *abfd)
       bfd_hash_table_free (&abfd->section_htab);
       objalloc_free ((struct objalloc *) abfd->memory);
     }
-
   free (abfd);
 }
 
@@ -708,6 +707,8 @@ bfd_boolean
 bfd_close (bfd *abfd)
 {
   bfd_boolean ret;
+  bfd *nbfd;
+  bfd *next;
 
   if (bfd_write_p (abfd))
     {
@@ -715,6 +716,13 @@ 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;
 

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

[Tom Tromey]

Tom> I'll open some bugs.

Ok, they are 14481 and 14482.

Tom

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

[H.J. Lu]

On Thu, Aug 16, 2012 at 11:48 AM, Tom Tromey <tromey@redhat.com> wrote:
> HJ> Do you have testcases for those problems?
>
> Just running valgrind using a test program.
>
> Tom> If you mean reverting the patches, I don't agree with it, but that is up
> Tom> to you.
>
> HJ> I don't think it is a bad idea.
>
> Here's a reversion patch.
> I couldn't find a ChangeLog for one of the patches, the one Nick
> committed.
>
> I'll open some bugs.
>
> If you check this in, please let me know so I can revert the gdb patch.
> Thanks.
>
> Tom
>
> 2012-08-16  Tom Tromey  <tromey@redhat.com>
>
>         * archive.c, libbfd-in.h, libbfd.h, opncls.c: Revert patches
>         relating to archive handling:
>         2012-08-16  Tom Tromey  <tromey@redhat.com>
>         2012-08-09  Alan Modra  <amodra@gmail.com>
>                     Tom Tromey  <tromey@redhat.com>
>

I checked in a testcase for

http://www.sourceware.org/bugzilla/show_bug.cgi?id=14481

If we revert it now, we get a regression.


-- 
H.J.

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

[H.J. Lu]

On Thu, Aug 16, 2012 at 10:02 AM, Tom Tromey <tromey@redhat.com> wrote:
> HJ> Your checkin destroys binutils:
> HJ> http://sourceware.org/bugzilla/show_bug.cgi?id=14475
> HJ> Can you fix it?
>
> Sorry about that.  I see now that I will need a more clever plan to
> destroy binutils.
>
> I debugged it.  The immediate problem is that
> _bfd_compute_and_write_armap calls bfd_free_cached_info
> (aka _bfd_free_cached_info).  This then does:
>
>       objalloc_free ((struct objalloc *) abfd->memory);
>
> Whoops, this also frees the areltdata.
>
> So, I have two possible fixes for it.
>
> I've appended the first possible fix.  It changes _bfd_free_cached_info
> not to free all the memory attached to the BFD.
>
> A couple notes here.
>
> First, it seems very wrong to me to clear usrdata in this function.  I
> didn't touch this; since presumably clients may be relying on this
> clearing in a subtle way (if they allocate the usrdata on the BFD
> objalloc, which is perhaps the only sensible approach anyhow).  But, I
> think that if the appended patch goes in then this line should be
> removed in a follow-up.
>
> Second, the check in _bfd_delete_bfd is perhaps ugly.  Maybe
> bfd_hash_table_free should do this check instead.  Let me know what you
> think.
>
> I rebuilt ld and binutils with this patch.  Additionally, I hacked the
> Makefiles to link all the programs with -lmcheck.  Then I ran the ld and
> binutils test suites.  There were no regressions.  I also examined one
> particular case from ar.exp using valgrind -- I could reproduce the
> problem before the patch, but not after.
>
>
> Another possible fix for this bug would be to allocate the areltdata
> using malloc.  That way it would be immune to the objalloc_free call.
> This would require a few more tweaks, like properly freeing it in
> _bfd_delete_bfd, etc.
>
> I'm happy to make and test this change if you think it would be better.
>
> Tom
>
> 2012-08-16  Tom Tromey  <tromey@redhat.com>
>
>         * opncls.c (_bfd_delete_bfd): Check to see if section htab is
>         already freed.
>         (_bfd_free_cached_info): Don't free the objalloc.
>
> Index: opncls.c
> ===================================================================
> RCS file: /cvs/src/src/bfd/opncls.c,v
> retrieving revision 1.72
> diff -u -r1.72 opncls.c
> --- opncls.c    9 Aug 2012 06:25:53 -0000       1.72
> +++ opncls.c    16 Aug 2012 16:57:36 -0000
> @@ -132,14 +132,15 @@
>  {
>    if (abfd->memory)
>      {
> -      bfd_hash_table_free (&abfd->section_htab);
> +      if (abfd->section_htab.memory != NULL)
> +       bfd_hash_table_free (&abfd->section_htab);
>        objalloc_free ((struct objalloc *) abfd->memory);
>      }
>
>    free (abfd);
>  }

Why is _bfd_delete_bfd called twice on the same bfd?

> -/* Free objalloc memory.  */
> +/* Free some information cached in the BFD.  */
>
>  bfd_boolean
>  _bfd_free_cached_info (bfd *abfd)
> @@ -147,14 +148,12 @@
>    if (abfd->memory)
>      {
>        bfd_hash_table_free (&abfd->section_htab);
> -      objalloc_free ((struct objalloc *) abfd->memory);
>
>        abfd->sections = NULL;
>        abfd->section_last = NULL;
>        abfd->outsymbols = NULL;
>        abfd->tdata.any = NULL;
>        abfd->usrdata = NULL;
> -      abfd->memory = NULL;
>      }
>
>    return TRUE;



-- 
H.J.

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

[H.J. Lu]

On Thu, Aug 16, 2012 at 10:02 AM, Tom Tromey <tromey@redhat.com> wrote:
> HJ> Your checkin destroys binutils:
> HJ> http://sourceware.org/bugzilla/show_bug.cgi?id=14475
> HJ> Can you fix it?
>
> Sorry about that.  I see now that I will need a more clever plan to
> destroy binutils.
>
> I debugged it.  The immediate problem is that
> _bfd_compute_and_write_armap calls bfd_free_cached_info
> (aka _bfd_free_cached_info).  This then does:
>
>       objalloc_free ((struct objalloc *) abfd->memory);
>
> Whoops, this also frees the areltdata.
>
> So, I have two possible fixes for it.
>
> I've appended the first possible fix.  It changes _bfd_free_cached_info
> not to free all the memory attached to the BFD.
>
> A couple notes here.
>
> First, it seems very wrong to me to clear usrdata in this function.  I
> didn't touch this; since presumably clients may be relying on this
> clearing in a subtle way (if they allocate the usrdata on the BFD
> objalloc, which is perhaps the only sensible approach anyhow).  But, I
> think that if the appended patch goes in then this line should be
> removed in a follow-up.
>
> Second, the check in _bfd_delete_bfd is perhaps ugly.  Maybe
> bfd_hash_table_free should do this check instead.  Let me know what you
> think.
>
> I rebuilt ld and binutils with this patch.  Additionally, I hacked the
> Makefiles to link all the programs with -lmcheck.  Then I ran the ld and
> binutils test suites.  There were no regressions.  I also examined one
> particular case from ar.exp using valgrind -- I could reproduce the
> problem before the patch, but not after.
>
>
> Another possible fix for this bug would be to allocate the areltdata
> using malloc.  That way it would be immune to the objalloc_free call.
> This would require a few more tweaks, like properly freeing it in
> _bfd_delete_bfd, etc.
>
> I'm happy to make and test this change if you think it would be better.
>
> Tom
>
> 2012-08-16  Tom Tromey  <tromey@redhat.com>
>
>         * opncls.c (_bfd_delete_bfd): Check to see if section htab is
>         already freed.
>         (_bfd_free_cached_info): Don't free the objalloc.
>
> Index: opncls.c
> ===================================================================
> RCS file: /cvs/src/src/bfd/opncls.c,v
> retrieving revision 1.72
> diff -u -r1.72 opncls.c
> --- opncls.c    9 Aug 2012 06:25:53 -0000       1.72
> +++ opncls.c    16 Aug 2012 16:57:36 -0000
> @@ -132,14 +132,15 @@
>  {
>    if (abfd->memory)
>      {
> -      bfd_hash_table_free (&abfd->section_htab);
> +      if (abfd->section_htab.memory != NULL)
> +       bfd_hash_table_free (&abfd->section_htab);
>        objalloc_free ((struct objalloc *) abfd->memory);
>      }
>
>    free (abfd);
>  }
>
> -/* Free objalloc memory.  */
> +/* Free some information cached in the BFD.  */
>
>  bfd_boolean
>  _bfd_free_cached_info (bfd *abfd)
> @@ -147,14 +148,12 @@
>    if (abfd->memory)
>      {
>        bfd_hash_table_free (&abfd->section_htab);
> -      objalloc_free ((struct objalloc *) abfd->memory);
>
>        abfd->sections = NULL;
>        abfd->section_last = NULL;
>        abfd->outsymbols = NULL;
>        abfd->tdata.any = NULL;
>        abfd->usrdata = NULL;
> -      abfd->memory = NULL;
>      }
>
>    return TRUE;

I think it is better to use malloc on areltdata so that
we can call objalloc_free to keep memory usage down.

Thanks.

-- 
H.J.

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

[Alan Modra]

On Thu, Aug 16, 2012 at 11:02:18AM -0600, Tom Tromey wrote:
> Another possible fix for this bug would be to allocate the areltdata
> using malloc.  That way it would be immune to the objalloc_free call.
> This would require a few more tweaks, like properly freeing it in
> _bfd_delete_bfd, etc.
> 
> I'm happy to make and test this change if you think it would be better.

Yes, I do think that would be better.  bfd_free_cached_info is called
only in one place, and the whole point of the call is as the comment
says
	  /* Now ask the BFD to free up any cached information, so we
	     don't fill all of memory with symbol tables.  */

If you don't free the bfd memory there isn't much point in having
bfd_free_cached_info!

> 	* opncls.c (_bfd_delete_bfd): Check to see if section htab is
> 	already freed.

This part can be committed now if you like.

-- 
Alan Modra
Australia Development Lab, IBM

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

[H.J. Lu]

On Thu, Aug 16, 2012 at 5:48 PM, Alan Modra <amodra@gmail.com> wrote:
> On Thu, Aug 16, 2012 at 11:02:18AM -0600, Tom Tromey wrote:
>> Another possible fix for this bug would be to allocate the areltdata
>> using malloc.  That way it would be immune to the objalloc_free call.
>> This would require a few more tweaks, like properly freeing it in
>> _bfd_delete_bfd, etc.
>>
>> I'm happy to make and test this change if you think it would be better.
>
> Yes, I do think that would be better.  bfd_free_cached_info is called
> only in one place, and the whole point of the call is as the comment
> says
>           /* Now ask the BFD to free up any cached information, so we
>              don't fill all of memory with symbol tables.  */
>
> If you don't free the bfd memory there isn't much point in having
> bfd_free_cached_info!
>

We run into this problem only when we allocate areltdata from
archive member from filesystem.  When we use xcalloc,
we only support bfd_close archive before bfd_close archive member
if we want to free the memory in bfd_close archive member.

Here is a patch to only use xcalloc in bfd_ar_hdr_from_filesystem.
We can check if parent_cache is NULL before calling free.

-- 
H.J.
--
2012-08-16  H.J. Lu  <hongjiu.lu@intel.com>

	PR binutils/14475
	* archive.c (bfd_ar_hdr_from_filesystem): Allocate areltdata
	with xcalloc.
	(_bfd_archive_close_and_cleanup): Free areltdata if allocated
	with xcalloc.

diff --git a/bfd/archive.c b/bfd/archive.c
index e0cb370..608a7e9 100644
--- a/bfd/archive.c
+++ b/bfd/archive.c
@@ -1896,7 +1896,9 @@ bfd_ar_hdr_from_filesystem (bfd *abfd, const char *filena
me, bfd *member)
     }

   amt = sizeof (struct ar_hdr) + sizeof (struct areltdata);
-  ared = (struct areltdata *) bfd_zalloc (member, amt);
+  /* Use xcalloc instead of bfd_zalloc so that areltdata is available
+     to archive after objalloc_free is called on member memory.  */
+  ared = (struct areltdata *) xcalloc (1, amt);
   if (ared == NULL)
     return NULL;
   hdr = (struct ar_hdr *) (((char *) ared) + sizeof (struct areltdata));
@@ -2741,6 +2743,11 @@ _bfd_archive_close_and_cleanup (bfd *abfd)
 	      htab_clear_slot (htab, slot);
 	    }
 	}
+      else
+	{
+	  /* If HTAB is NULL, free ARED allocated with xcalloc.  */
+	  free (ared);
+	}
     }
   return TRUE;
 }

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

[Alan Modra]

On Thu, Aug 16, 2012 at 06:01:29PM -0700, H.J. Lu wrote:
>    amt = sizeof (struct ar_hdr) + sizeof (struct areltdata);
> -  ared = (struct areltdata *) bfd_zalloc (member, amt);
> +  /* Use xcalloc instead of bfd_zalloc so that areltdata is available
> +     to archive after objalloc_free is called on member memory.  */
> +  ared = (struct areltdata *) xcalloc (1, amt);

xcalloc shouldn't be called from within bfd.  I've already committed a
kneejerk patch to revert Tom's last change, instead using bfd_zmalloc
here.  Tom said he'd look into fixing the leak this causes, so I'm
happy to leave that to him.  :)

-- 
Alan Modra
Australia Development Lab, IBM

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

[Hans-Peter Nilsson]

(Just replying to the last message in the thread)

> xcalloc shouldn't be called from within bfd.  I've already committed a
> kneejerk patch to revert Tom's last change, instead using bfd_zmalloc
> here.  Tom said he'd look into fixing the leak this causes, so I'm
> happy to leave that to him.  :)

The last I see is (2012-08-17-02:39:34 UTC)

bfd:
2012-08-17  Yuri Chornoivan  <yurchor@ukr.net>

	* elf-bfd.h, * elf32-ppc.c, * elf64-ia64-vms.c, * elfnn-ia64.c,
	* elfxx-mips.c, * vms-alpha.c: Typo fixes.

2012-08-17  Alan Modra  <amodra@gmail.com>

	PR binutils/14475:
	* archive.c (bfd_ar_hdr_from_filesystem): Revert last change.
	Instead malloc areltdata.

binutils:
2012-08-17  Yuri Chornoivan  <yurchor@ukr.net>

	* doc/binutils.texi, * objdump.c, * od-xcoff.c: Typo fixes.

2012-08-16  H.J. Lu  <hongjiu.lu@intel.com>

	PR binutils/14481
	* Makefile.am (BFDTEST1_PROG): New.
	(TEST_PROGS): Likewise.
	(bfdtest1_DEPENDENCIES): Likewise.
	(noinst_PROGRAMS): Add $(TEST_PROGS).
	* Makefile.in: Regenerated.

	* bfdtest1.c: New file.

With this I still see FAILS for cris-elf and cris-linux-gnu (but
not for arm-unknown-eabi, mipsisa32r2el-unknown-linux-gnu,
mmix-knuth-mmixware):
Running /tmp/hpautotest-binutils/bsrc/src/binutils/testsuite/binutils-all/ar.exp ...
FAIL: ar long file names (bfdtest1)
FAIL: ar thin archive (bfdtest1)

and in binutils.log:
Executing on host: /tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 tmpdir/artest.a   (timeout = 300)
/tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 exited with status 1
/tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 exited with status 1
FAIL: ar long file names (bfdtest1)
...
Executing on host: /tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 tmpdir/artest.a   (timeout = 300)
/tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 exited with status 1
/tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 exited with status 1
FAIL: ar thin archive (bfdtest1)

Looking closer, it seems bfdtest1 is a new test, and a host
program, which might explain the test-result differences.
Shouldn't bfdtest1 be present and tested for native builds only?

brgds, H-P

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

[Alan Modra]

On Fri, Aug 17, 2012 at 05:53:46AM +0200, Hans-Peter Nilsson wrote:
> Shouldn't bfdtest1 be present and tested for native builds only?

Looks that way.  HJ, please fix.

-- 
Alan Modra
Australia Development Lab, IBM

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

[H.J. Lu]

On Thu, Aug 16, 2012 at 8:53 PM, Hans-Peter Nilsson
<hans-peter.nilsson@axis.com> wrote:
> (Just replying to the last message in the thread)
>
>> xcalloc shouldn't be called from within bfd.  I've already committed a
>> kneejerk patch to revert Tom's last change, instead using bfd_zmalloc
>> here.  Tom said he'd look into fixing the leak this causes, so I'm
>> happy to leave that to him.  :)
>
> The last I see is (2012-08-17-02:39:34 UTC)
>
> bfd:
> 2012-08-17  Yuri Chornoivan  <yurchor@ukr.net>
>
>         * elf-bfd.h, * elf32-ppc.c, * elf64-ia64-vms.c, * elfnn-ia64.c,
>         * elfxx-mips.c, * vms-alpha.c: Typo fixes.
>
> 2012-08-17  Alan Modra  <amodra@gmail.com>
>
>         PR binutils/14475:
>         * archive.c (bfd_ar_hdr_from_filesystem): Revert last change.
>         Instead malloc areltdata.
>
> binutils:
> 2012-08-17  Yuri Chornoivan  <yurchor@ukr.net>
>
>         * doc/binutils.texi, * objdump.c, * od-xcoff.c: Typo fixes.
>
> 2012-08-16  H.J. Lu  <hongjiu.lu@intel.com>
>
>         PR binutils/14481
>         * Makefile.am (BFDTEST1_PROG): New.
>         (TEST_PROGS): Likewise.
>         (bfdtest1_DEPENDENCIES): Likewise.
>         (noinst_PROGRAMS): Add $(TEST_PROGS).
>         * Makefile.in: Regenerated.
>
>         * bfdtest1.c: New file.
>
> With this I still see FAILS for cris-elf and cris-linux-gnu (but
> not for arm-unknown-eabi, mipsisa32r2el-unknown-linux-gnu,
> mmix-knuth-mmixware):
> Running /tmp/hpautotest-binutils/bsrc/src/binutils/testsuite/binutils-all/ar.exp ...
> FAIL: ar long file names (bfdtest1)
> FAIL: ar thin archive (bfdtest1)
>
> and in binutils.log:
> Executing on host: /tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 tmpdir/artest.a   (timeout = 300)
> /tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 exited with status 1
> /tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 exited with status 1
> FAIL: ar long file names (bfdtest1)
> ...
> Executing on host: /tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 tmpdir/artest.a   (timeout = 300)
> /tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 exited with status 1
> /tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 exited with status 1
> FAIL: ar thin archive (bfdtest1)
>
> Looking closer, it seems bfdtest1 is a new test, and a host
> program, which might explain the test-result differences.
> Shouldn't bfdtest1 be present and tested for native builds only?
>

bfdtest1 is built the same as other programs.  valgrind reports:

==3884==
==3884== Invalid read of size 8
==3884==    at 0x403D85: bfd_generic_openr_next_archived_file (archive.c:765)
==3884==    by 0x402CCA: main (bfdtest1.c:60)
==3884==  Address 0x4c35ec0 is 208 bytes inside a block of size 296 free'd
==3884==    at 0x4A079AE: free (vg_replace_malloc.c:427)
==3884==    by 0x40D9B7: bfd_close (opncls.c:726)
==3884==    by 0x402C9D: main (bfdtest1.c:53)
==3884==

My new test is doing its job.  There is a real bug.


-- 
H.J.

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

[H.J. Lu]

On Thu, Aug 16, 2012 at 10:04 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Aug 16, 2012 at 8:53 PM, Hans-Peter Nilsson
> <hans-peter.nilsson@axis.com> wrote:
>> (Just replying to the last message in the thread)
>>
>>> xcalloc shouldn't be called from within bfd.  I've already committed a
>>> kneejerk patch to revert Tom's last change, instead using bfd_zmalloc
>>> here.  Tom said he'd look into fixing the leak this causes, so I'm
>>> happy to leave that to him.  :)
>>
>> The last I see is (2012-08-17-02:39:34 UTC)
>>
>> bfd:
>> 2012-08-17  Yuri Chornoivan  <yurchor@ukr.net>
>>
>>         * elf-bfd.h, * elf32-ppc.c, * elf64-ia64-vms.c, * elfnn-ia64.c,
>>         * elfxx-mips.c, * vms-alpha.c: Typo fixes.
>>
>> 2012-08-17  Alan Modra  <amodra@gmail.com>
>>
>>         PR binutils/14475:
>>         * archive.c (bfd_ar_hdr_from_filesystem): Revert last change.
>>         Instead malloc areltdata.
>>
>> binutils:
>> 2012-08-17  Yuri Chornoivan  <yurchor@ukr.net>
>>
>>         * doc/binutils.texi, * objdump.c, * od-xcoff.c: Typo fixes.
>>
>> 2012-08-16  H.J. Lu  <hongjiu.lu@intel.com>
>>
>>         PR binutils/14481
>>         * Makefile.am (BFDTEST1_PROG): New.
>>         (TEST_PROGS): Likewise.
>>         (bfdtest1_DEPENDENCIES): Likewise.
>>         (noinst_PROGRAMS): Add $(TEST_PROGS).
>>         * Makefile.in: Regenerated.
>>
>>         * bfdtest1.c: New file.
>>
>> With this I still see FAILS for cris-elf and cris-linux-gnu (but
>> not for arm-unknown-eabi, mipsisa32r2el-unknown-linux-gnu,
>> mmix-knuth-mmixware):

This should be a clue.

>> Running /tmp/hpautotest-binutils/bsrc/src/binutils/testsuite/binutils-all/ar.exp ...
>> FAIL: ar long file names (bfdtest1)
>> FAIL: ar thin archive (bfdtest1)

This fixes it.


-- 
H.J.
---
diff --git a/bfd/aoutx.h b/bfd/aoutx.h
index 1e0ad38..8e3f476 100644
--- a/bfd/aoutx.h
+++ b/bfd/aoutx.h
@@ -2846,26 +2846,26 @@ NAME (aout, bfd_free_cached_info) (bfd *abfd)
 {
   asection *o;

-  if (bfd_get_format (abfd) != bfd_object
-      || abfd->tdata.aout_data == NULL)
-    return TRUE;
-
+  if (bfd_get_format (abfd) == bfd_object
+      && abfd->tdata.aout_data != NULL)
+    {
 #define BFCI_FREE(x) if (x != NULL) { free (x); x = NULL; }
-  BFCI_FREE (obj_aout_symbols (abfd));
+      BFCI_FREE (obj_aout_symbols (abfd));
 #ifdef USE_MMAP
-  obj_aout_external_syms (abfd) = 0;
-  bfd_free_window (&obj_aout_sym_window (abfd));
-  bfd_free_window (&obj_aout_string_window (abfd));
-  obj_aout_external_strings (abfd) = 0;
+      obj_aout_external_syms (abfd) = 0;
+      bfd_free_window (&obj_aout_sym_window (abfd));
+      bfd_free_window (&obj_aout_string_window (abfd));
+      obj_aout_external_strings (abfd) = 0;
 #else
-  BFCI_FREE (obj_aout_external_syms (abfd));
-  BFCI_FREE (obj_aout_external_strings (abfd));
+      BFCI_FREE (obj_aout_external_syms (abfd));
+      BFCI_FREE (obj_aout_external_strings (abfd));
 #endif
-  for (o = abfd->sections; o != NULL; o = o->next)
-    BFCI_FREE (o->relocation);
+      for (o = abfd->sections; o != NULL; o = o->next)
+	BFCI_FREE (o->relocation);
 #undef BFCI_FREE
+    }

-  return TRUE;
+  return _bfd_generic_close_and_cleanup (abfd);
 }


 /* a.out link code.  */

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

[H.J. Lu]

On Thu, Aug 16, 2012 at 10:28 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Aug 16, 2012 at 10:04 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Thu, Aug 16, 2012 at 8:53 PM, Hans-Peter Nilsson
>> <hans-peter.nilsson@axis.com> wrote:
>>> (Just replying to the last message in the thread)
>>>
>>>> xcalloc shouldn't be called from within bfd.  I've already committed a
>>>> kneejerk patch to revert Tom's last change, instead using bfd_zmalloc
>>>> here.  Tom said he'd look into fixing the leak this causes, so I'm
>>>> happy to leave that to him.  :)
>>>
>>> The last I see is (2012-08-17-02:39:34 UTC)
>>>
>>> bfd:
>>> 2012-08-17  Yuri Chornoivan  <yurchor@ukr.net>
>>>
>>>         * elf-bfd.h, * elf32-ppc.c, * elf64-ia64-vms.c, * elfnn-ia64.c,
>>>         * elfxx-mips.c, * vms-alpha.c: Typo fixes.
>>>
>>> 2012-08-17  Alan Modra  <amodra@gmail.com>
>>>
>>>         PR binutils/14475:
>>>         * archive.c (bfd_ar_hdr_from_filesystem): Revert last change.
>>>         Instead malloc areltdata.
>>>
>>> binutils:
>>> 2012-08-17  Yuri Chornoivan  <yurchor@ukr.net>
>>>
>>>         * doc/binutils.texi, * objdump.c, * od-xcoff.c: Typo fixes.
>>>
>>> 2012-08-16  H.J. Lu  <hongjiu.lu@intel.com>
>>>
>>>         PR binutils/14481
>>>         * Makefile.am (BFDTEST1_PROG): New.
>>>         (TEST_PROGS): Likewise.
>>>         (bfdtest1_DEPENDENCIES): Likewise.
>>>         (noinst_PROGRAMS): Add $(TEST_PROGS).
>>>         * Makefile.in: Regenerated.
>>>
>>>         * bfdtest1.c: New file.
>>>
>>> With this I still see FAILS for cris-elf and cris-linux-gnu (but
>>> not for arm-unknown-eabi, mipsisa32r2el-unknown-linux-gnu,
>>> mmix-knuth-mmixware):
>
> This should be a clue.
>
>>> Running /tmp/hpautotest-binutils/bsrc/src/binutils/testsuite/binutils-all/ar.exp ...
>>> FAIL: ar long file names (bfdtest1)
>>> FAIL: ar thin archive (bfdtest1)
>
> This fixes it.
>
>
> --
> H.J.
> ---
> diff --git a/bfd/aoutx.h b/bfd/aoutx.h
> index 1e0ad38..8e3f476 100644
> --- a/bfd/aoutx.h
> +++ b/bfd/aoutx.h
> @@ -2846,26 +2846,26 @@ NAME (aout, bfd_free_cached_info) (bfd *abfd)
>  {
>    asection *o;
>
> -  if (bfd_get_format (abfd) != bfd_object
> -      || abfd->tdata.aout_data == NULL)
> -    return TRUE;
> -
> +  if (bfd_get_format (abfd) == bfd_object
> +      && abfd->tdata.aout_data != NULL)
> +    {
>  #define BFCI_FREE(x) if (x != NULL) { free (x); x = NULL; }
> -  BFCI_FREE (obj_aout_symbols (abfd));
> +      BFCI_FREE (obj_aout_symbols (abfd));
>  #ifdef USE_MMAP
> -  obj_aout_external_syms (abfd) = 0;
> -  bfd_free_window (&obj_aout_sym_window (abfd));
> -  bfd_free_window (&obj_aout_string_window (abfd));
> -  obj_aout_external_strings (abfd) = 0;
> +      obj_aout_external_syms (abfd) = 0;
> +      bfd_free_window (&obj_aout_sym_window (abfd));
> +      bfd_free_window (&obj_aout_string_window (abfd));
> +      obj_aout_external_strings (abfd) = 0;
>  #else
> -  BFCI_FREE (obj_aout_external_syms (abfd));
> -  BFCI_FREE (obj_aout_external_strings (abfd));
> +      BFCI_FREE (obj_aout_external_syms (abfd));
> +      BFCI_FREE (obj_aout_external_strings (abfd));
>  #endif
> -  for (o = abfd->sections; o != NULL; o = o->next)
> -    BFCI_FREE (o->relocation);
> +      for (o = abfd->sections; o != NULL; o = o->next)
> +       BFCI_FREE (o->relocation);
>  #undef BFCI_FREE
> +    }
>
> -  return TRUE;
> +  return _bfd_generic_close_and_cleanup (abfd);
>  }
>
>
>  /* a.out link code.  */

Patch is wrong.

-- 
H.J.

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

[H.J. Lu]

On Thu, Aug 16, 2012 at 10:30 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Aug 16, 2012 at 10:28 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Thu, Aug 16, 2012 at 10:04 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Thu, Aug 16, 2012 at 8:53 PM, Hans-Peter Nilsson
>>> <hans-peter.nilsson@axis.com> wrote:
>>>> (Just replying to the last message in the thread)
>>>>
>>>>> xcalloc shouldn't be called from within bfd.  I've already committed a
>>>>> kneejerk patch to revert Tom's last change, instead using bfd_zmalloc
>>>>> here.  Tom said he'd look into fixing the leak this causes, so I'm
>>>>> happy to leave that to him.  :)
>>>>
>>>> The last I see is (2012-08-17-02:39:34 UTC)
>>>>
>>>> bfd:
>>>> 2012-08-17  Yuri Chornoivan  <yurchor@ukr.net>
>>>>
>>>>         * elf-bfd.h, * elf32-ppc.c, * elf64-ia64-vms.c, * elfnn-ia64.c,
>>>>         * elfxx-mips.c, * vms-alpha.c: Typo fixes.
>>>>
>>>> 2012-08-17  Alan Modra  <amodra@gmail.com>
>>>>
>>>>         PR binutils/14475:
>>>>         * archive.c (bfd_ar_hdr_from_filesystem): Revert last change.
>>>>         Instead malloc areltdata.
>>>>
>>>> binutils:
>>>> 2012-08-17  Yuri Chornoivan  <yurchor@ukr.net>
>>>>
>>>>         * doc/binutils.texi, * objdump.c, * od-xcoff.c: Typo fixes.
>>>>
>>>> 2012-08-16  H.J. Lu  <hongjiu.lu@intel.com>
>>>>
>>>>         PR binutils/14481
>>>>         * Makefile.am (BFDTEST1_PROG): New.
>>>>         (TEST_PROGS): Likewise.
>>>>         (bfdtest1_DEPENDENCIES): Likewise.
>>>>         (noinst_PROGRAMS): Add $(TEST_PROGS).
>>>>         * Makefile.in: Regenerated.
>>>>
>>>>         * bfdtest1.c: New file.
>>>>
>>>> With this I still see FAILS for cris-elf and cris-linux-gnu (but
>>>> not for arm-unknown-eabi, mipsisa32r2el-unknown-linux-gnu,
>>>> mmix-knuth-mmixware):
>>
>> This should be a clue.
>>
>>>> Running /tmp/hpautotest-binutils/bsrc/src/binutils/testsuite/binutils-all/ar.exp ...
>>>> FAIL: ar long file names (bfdtest1)
>>>> FAIL: ar thin archive (bfdtest1)
>>
>> This fixes it.
>>
>>

>>  /* a.out link code.  */
>
> Patch is wrong.
>

Here is the correct patch.

-- 
H.J.
--
diff --git a/bfd/aout-target.h b/bfd/aout-target.h
index f6e8bd2..b0edb17 100644
--- a/bfd/aout-target.h
+++ b/bfd/aout-target.h
@@ -577,7 +577,7 @@ MY_bfd_final_link (bfd *abfd, struct bfd_link_info *info)
 #endif

 #ifndef MY_close_and_cleanup
-#define MY_close_and_cleanup MY_bfd_free_cached_info
+#define MY_close_and_cleanup _bfd_generic_close_and_cleanup
 #endif

 #ifndef MY_get_dynamic_symtab_upper_bound

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

[H.J. Lu]

On Thu, Aug 16, 2012 at 10:42 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Aug 16, 2012 at 10:30 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Thu, Aug 16, 2012 at 10:28 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Thu, Aug 16, 2012 at 10:04 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> On Thu, Aug 16, 2012 at 8:53 PM, Hans-Peter Nilsson
>>>> <hans-peter.nilsson@axis.com> wrote:
>>>>> (Just replying to the last message in the thread)
>>>>>
>>>>>> xcalloc shouldn't be called from within bfd.  I've already committed a
>>>>>> kneejerk patch to revert Tom's last change, instead using bfd_zmalloc
>>>>>> here.  Tom said he'd look into fixing the leak this causes, so I'm
>>>>>> happy to leave that to him.  :)
>>>>>
>>>>> The last I see is (2012-08-17-02:39:34 UTC)
>>>>>
>>>>> bfd:
>>>>> 2012-08-17  Yuri Chornoivan  <yurchor@ukr.net>
>>>>>
>>>>>         * elf-bfd.h, * elf32-ppc.c, * elf64-ia64-vms.c, * elfnn-ia64.c,
>>>>>         * elfxx-mips.c, * vms-alpha.c: Typo fixes.
>>>>>
>>>>> 2012-08-17  Alan Modra  <amodra@gmail.com>
>>>>>
>>>>>         PR binutils/14475:
>>>>>         * archive.c (bfd_ar_hdr_from_filesystem): Revert last change.
>>>>>         Instead malloc areltdata.
>>>>>
>>>>> binutils:
>>>>> 2012-08-17  Yuri Chornoivan  <yurchor@ukr.net>
>>>>>
>>>>>         * doc/binutils.texi, * objdump.c, * od-xcoff.c: Typo fixes.
>>>>>
>>>>> 2012-08-16  H.J. Lu  <hongjiu.lu@intel.com>
>>>>>
>>>>>         PR binutils/14481
>>>>>         * Makefile.am (BFDTEST1_PROG): New.
>>>>>         (TEST_PROGS): Likewise.
>>>>>         (bfdtest1_DEPENDENCIES): Likewise.
>>>>>         (noinst_PROGRAMS): Add $(TEST_PROGS).
>>>>>         * Makefile.in: Regenerated.
>>>>>
>>>>>         * bfdtest1.c: New file.
>>>>>
>>>>> With this I still see FAILS for cris-elf and cris-linux-gnu (but
>>>>> not for arm-unknown-eabi, mipsisa32r2el-unknown-linux-gnu,
>>>>> mmix-knuth-mmixware):
>>>
>>> This should be a clue.
>>>
>>>>> Running /tmp/hpautotest-binutils/bsrc/src/binutils/testsuite/binutils-all/ar.exp ...
>>>>> FAIL: ar long file names (bfdtest1)
>>>>> FAIL: ar thin archive (bfdtest1)
>>>
>>> This fixes it.
>>>
>>>
>
>>>  /* a.out link code.  */
>>
>> Patch is wrong.
>>
>
> Here is the correct patch.
>
> --
> H.J.
> --
> diff --git a/bfd/aout-target.h b/bfd/aout-target.h
> index f6e8bd2..b0edb17 100644
> --- a/bfd/aout-target.h
> +++ b/bfd/aout-target.h
> @@ -577,7 +577,7 @@ MY_bfd_final_link (bfd *abfd, struct bfd_link_info *info)
>  #endif
>
>  #ifndef MY_close_and_cleanup
> -#define MY_close_and_cleanup MY_bfd_free_cached_info
> +#define MY_close_and_cleanup _bfd_generic_close_and_cleanup
>  #endif
>
>  #ifndef MY_get_dynamic_symtab_upper_bound

More potential problems:

aout-adobe.c:#define aout_32_close_and_cleanup
aout_32_bfd_free_cached_info
aout-tic30.c:#define MY_close_and_cleanup MY_bfd_free_cached_info
bout.c:#define aout_32_close_and_cleanup
aout_32_bfd_free_cached_info
coff-sh.c:#define coff_small_close_and_cleanup \
elf64-aarch64.c:#define bfd_elf64_close_and_cleanup             \
elf64-ia64-vms.c:#define bfd_elf64_close_and_cleanup elf64_vms_close_and_cleanup
elfxx-target.h:#define	bfd_elfNN_close_and_cleanup _bfd_elf_close_and_cleanup
i386os9k.c:#define aout_32_close_and_cleanup aout_32_bfd_free_cached_info
som.c:#define	som_close_and_cleanup		        som_bfd_free_cached_info
vms-alpha.c:#define alpha_vms_close_and_cleanup	   vms_close_and_cleanup

We need to make sure that all of them call  _bfd_generic_close_and_cleanup
at the end.


H.J.

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

[H.J. Lu]

On Thu, Aug 16, 2012 at 10:45 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Aug 16, 2012 at 10:42 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Thu, Aug 16, 2012 at 10:30 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Thu, Aug 16, 2012 at 10:28 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> On Thu, Aug 16, 2012 at 10:04 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>> On Thu, Aug 16, 2012 at 8:53 PM, Hans-Peter Nilsson
>>>>> <hans-peter.nilsson@axis.com> wrote:
>>>>>> (Just replying to the last message in the thread)
>>>>>>
>>>>>>> xcalloc shouldn't be called from within bfd.  I've already committed a
>>>>>>> kneejerk patch to revert Tom's last change, instead using bfd_zmalloc
>>>>>>> here.  Tom said he'd look into fixing the leak this causes, so I'm
>>>>>>> happy to leave that to him.  :)
>>>>>>
>>>>>> The last I see is (2012-08-17-02:39:34 UTC)
>>>>>>
>>>>>> bfd:
>>>>>> 2012-08-17  Yuri Chornoivan  <yurchor@ukr.net>
>>>>>>
>>>>>>         * elf-bfd.h, * elf32-ppc.c, * elf64-ia64-vms.c, * elfnn-ia64.c,
>>>>>>         * elfxx-mips.c, * vms-alpha.c: Typo fixes.
>>>>>>
>>>>>> 2012-08-17  Alan Modra  <amodra@gmail.com>
>>>>>>
>>>>>>         PR binutils/14475:
>>>>>>         * archive.c (bfd_ar_hdr_from_filesystem): Revert last change.
>>>>>>         Instead malloc areltdata.
>>>>>>
>>>>>> binutils:
>>>>>> 2012-08-17  Yuri Chornoivan  <yurchor@ukr.net>
>>>>>>
>>>>>>         * doc/binutils.texi, * objdump.c, * od-xcoff.c: Typo fixes.
>>>>>>
>>>>>> 2012-08-16  H.J. Lu  <hongjiu.lu@intel.com>
>>>>>>
>>>>>>         PR binutils/14481
>>>>>>         * Makefile.am (BFDTEST1_PROG): New.
>>>>>>         (TEST_PROGS): Likewise.
>>>>>>         (bfdtest1_DEPENDENCIES): Likewise.
>>>>>>         (noinst_PROGRAMS): Add $(TEST_PROGS).
>>>>>>         * Makefile.in: Regenerated.
>>>>>>
>>>>>>         * bfdtest1.c: New file.
>>>>>>
>>>>>> With this I still see FAILS for cris-elf and cris-linux-gnu (but
>>>>>> not for arm-unknown-eabi, mipsisa32r2el-unknown-linux-gnu,
>>>>>> mmix-knuth-mmixware):
>>>>
>>>> This should be a clue.
>>>>
>>>>>> Running /tmp/hpautotest-binutils/bsrc/src/binutils/testsuite/binutils-all/ar.exp ...
>>>>>> FAIL: ar long file names (bfdtest1)
>>>>>> FAIL: ar thin archive (bfdtest1)
>>>>
>>>> This fixes it.
>>>>
>>>>
>>
>>>>  /* a.out link code.  */
>>>
>>> Patch is wrong.
>>>
>>
>> Here is the correct patch.
>>
>> --
>> H.J.
>> --
>> diff --git a/bfd/aout-target.h b/bfd/aout-target.h
>> index f6e8bd2..b0edb17 100644
>> --- a/bfd/aout-target.h
>> +++ b/bfd/aout-target.h
>> @@ -577,7 +577,7 @@ MY_bfd_final_link (bfd *abfd, struct bfd_link_info *info)
>>  #endif
>>
>>  #ifndef MY_close_and_cleanup
>> -#define MY_close_and_cleanup MY_bfd_free_cached_info
>> +#define MY_close_and_cleanup _bfd_generic_close_and_cleanup
>>  #endif
>>
>>  #ifndef MY_get_dynamic_symtab_upper_bound
>
> More potential problems:
>
> aout-adobe.c:#define aout_32_close_and_cleanup
> aout_32_bfd_free_cached_info
> aout-tic30.c:#define MY_close_and_cleanup MY_bfd_free_cached_info
> bout.c:#define aout_32_close_and_cleanup
> aout_32_bfd_free_cached_info
> coff-sh.c:#define coff_small_close_and_cleanup \
> elf64-aarch64.c:#define bfd_elf64_close_and_cleanup             \
> elf64-ia64-vms.c:#define bfd_elf64_close_and_cleanup elf64_vms_close_and_cleanup
> elfxx-target.h:#define  bfd_elfNN_close_and_cleanup _bfd_elf_close_and_cleanup
> i386os9k.c:#define aout_32_close_and_cleanup aout_32_bfd_free_cached_info
> som.c:#define   som_close_and_cleanup                   som_bfd_free_cached_info
> vms-alpha.c:#define alpha_vms_close_and_cleanup    vms_close_and_cleanup
>
> We need to make sure that all of them call  _bfd_generic_close_and_cleanup
> at the end.
>

I took a closer look.  My change may not be correct.
I reopened:

http://sourceware.org/bugzilla/show_bug.cgi?id=14481

I will leave it to target maintainers.


-- 
H.J.

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

[Hans-Peter Nilsson]

> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Fri, 17 Aug 2012 15:05:20 +0200

> I took a closer look.  My change may not be correct.

Any clues to your conclusion, or is it obvious with a gdb walk?

> I reopened:
> 
> http://sourceware.org/bugzilla/show_bug.cgi?id=14481

Your last comment there is a bit misleading; cris-elf is not an
a.out target, it just includes an a.out-supporting BFD
vector...but which is the default BFD vector for all CRIS
targets.  Doh!  (That might be worthwhile changing, but JFTR I
understand changing that would just hiding the bug.)

brgds, H-P

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

[Hans-Peter Nilsson]

> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Fri, 17 Aug 2012 07:04:18 +0200

> On Thu, Aug 16, 2012 at 8:53 PM, Hans-Peter Nilsson
> <hans-peter.nilsson@axis.com> wrote:
> > With this I still see FAILS for cris-elf and cris-linux-gnu (but
> > not for arm-unknown-eabi, mipsisa32r2el-unknown-linux-gnu,
> > mmix-knuth-mmixware):
> > Running /tmp/hpautotest-binutils/bsrc/src/binutils/testsuite/binutils-all/ar.exp ...
> > FAIL: ar long file names (bfdtest1)
> > FAIL: ar thin archive (bfdtest1)
> >
> > and in binutils.log:
> > Executing on host: /tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 tmpdir/artest.a   (timeout = 300)
> > /tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 exited with status 1
> > /tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 exited with status 1
> > FAIL: ar long file names (bfdtest1)
> > ...
> > Executing on host: /tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 tmpdir/artest.a   (timeout = 300)
> > /tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 exited with status 1
> > /tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 exited with status 1
> > FAIL: ar thin archive (bfdtest1)
> >
> > Looking closer, it seems bfdtest1 is a new test, and a host
> > program, which might explain the test-result differences.
> > Shouldn't bfdtest1 be present and tested for native builds only?
> >
> 
> bfdtest1 is built the same as other programs.

Not *test* programs.  It seems bfdtest1 is used as input to ar
in the failing tests.

>  valgrind
> reports:
> 
> ==3884==
> ==3884== Invalid read of size 8
> ==3884==    at 0x403D85: bfd_generic_openr_next_archived_file (archive.c:765)
> ==3884==    by 0x402CCA: main (bfdtest1.c:60)
> ==3884==  Address 0x4c35ec0 is 208 bytes inside a block of size 296 free'd
> ==3884==    at 0x4A079AE: free (vg_replace_malloc.c:427)
> ==3884==    by 0x40D9B7: bfd_close (opncls.c:726)
> ==3884==    by 0x402C9D: main (bfdtest1.c:53)
> ==3884==
> 
> My new test is doing its job.  There is a real bug.

That might be, but I'm not sure random binary files for other
architectures are expected to be valid input to ar.  Is it?
Ungracious failure surely, but I'm inclined to just skip the
test (for my targets surely, for non-native if acceptable).

brgds, H-P

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

[H.J. Lu]

On Fri, Aug 17, 2012 at 8:56 AM, Hans-Peter Nilsson
<hans-peter.nilsson@axis.com> wrote:
>> From: "H.J. Lu" <hjl.tools@gmail.com>
>> Date: Fri, 17 Aug 2012 07:04:18 +0200
>
>> On Thu, Aug 16, 2012 at 8:53 PM, Hans-Peter Nilsson
>> <hans-peter.nilsson@axis.com> wrote:
>> > With this I still see FAILS for cris-elf and cris-linux-gnu (but
>> > not for arm-unknown-eabi, mipsisa32r2el-unknown-linux-gnu,
>> > mmix-knuth-mmixware):
>> > Running /tmp/hpautotest-binutils/bsrc/src/binutils/testsuite/binutils-all/ar.exp ...
>> > FAIL: ar long file names (bfdtest1)
>> > FAIL: ar thin archive (bfdtest1)
>> >
>> > and in binutils.log:
>> > Executing on host: /tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 tmpdir/artest.a   (timeout = 300)
>> > /tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 exited with status 1
>> > /tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 exited with status 1
>> > FAIL: ar long file names (bfdtest1)
>> > ...
>> > Executing on host: /tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 tmpdir/artest.a   (timeout = 300)
>> > /tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 exited with status 1
>> > /tmp/hpautotest-binutils/cris-axis-elf/binutils/bfdtest1 exited with status 1
>> > FAIL: ar thin archive (bfdtest1)
>> >
>> > Looking closer, it seems bfdtest1 is a new test, and a host
>> > program, which might explain the test-result differences.
>> > Shouldn't bfdtest1 be present and tested for native builds only?
>> >
>>
>> bfdtest1 is built the same as other programs.
>
> Not *test* programs.  It seems bfdtest1 is used as input to ar
> in the failing tests.
>
>>  valgrind
>> reports:
>>
>> ==3884==
>> ==3884== Invalid read of size 8
>> ==3884==    at 0x403D85: bfd_generic_openr_next_archived_file (archive.c:765)
>> ==3884==    by 0x402CCA: main (bfdtest1.c:60)
>> ==3884==  Address 0x4c35ec0 is 208 bytes inside a block of size 296 free'd
>> ==3884==    at 0x4A079AE: free (vg_replace_malloc.c:427)
>> ==3884==    by 0x40D9B7: bfd_close (opncls.c:726)
>> ==3884==    by 0x402C9D: main (bfdtest1.c:53)
>> ==3884==
>>
>> My new test is doing its job.  There is a real bug.
>
> That might be, but I'm not sure random binary files for other
> architectures are expected to be valid input to ar.  Is it?
> Ungracious failure surely, but I'm inclined to just skip the
> test (for my targets surely, for non-native if acceptable).

That is not what I saw.  bfdtest1 takes an archive as
input:

/export/build/gnu/binutils-cross/build-cris-elf/binutils/bfdtest1
tmpdir/artest.a
Executing on host:
/export/build/gnu/binutils-cross/build-cris-elf/binutils/bfdtest1
tmpdir/artest.a   (timeout = 300)
spawn -ignore SIGHUP
/export/build/gnu/binutils-cross/build-cris-elf/binutils/bfdtest1
tmpdir/artest.a^M
/export/build/gnu/binutils-cross/build-cris-elf/binutils/bfdtest1
exited with status 1
/export/build/gnu/binutils-cross/build-cris-elf/binutils/bfdtest1
exited with status 1
FAIL: ar long file names (bfdtest1)

It failed because bfdtest1 segfaulted.

-- 
H.J.

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

[Hans-Peter Nilsson]

> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Fri, 17 Aug 2012 18:01:10 +0200

> On Fri, Aug 17, 2012 at 8:56 AM, Hans-Peter Nilsson
> <hans-peter.nilsson@axis.com> wrote:
> > Not *test* programs.  It seems bfdtest1 is used as input to ar
> > in the failing tests.

> That is not what I saw.  bfdtest1 takes an archive as
> input:

You're right, I misread binutils.log.

brgds, H-P

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

[Tom Tromey]

Alan> Tom said he'd look into fixing the leak this causes, so I'm happy
Alan> to leave that to him.  :)

Here's the patch.

I think it would be good for someone to double check it.

I wrote this patch by searching for all the places that could allocate
an areltdata and changing them to use bfd_zmalloc.

Then I examined all the users of bfd->arelt_data and all callers of
bfd_read_ar_hdr (and _bfd_read_ar_hdr) to see how the data was used.
This revealed a number of spots that used bfd_release to free this data.

Finally, I added a free to _bfd_delete_bfd.

I once again built ld and all the binutils programs with -lmcheck, and
then ran the test suites.  These all passed.  I also ran a single 'ar'
test (one that was failing yesterday) plus the new 'bfdtest1' test under
valgrind.  These were also clean.


I found a few oddities in BFD while working on this patch:

* _bfd_get_elt_at_filepos can release new_areldata but still leave a
  stale pointer in n_nfd->arelt_data.  I fixed this.  I am not sure if
  this can ever result in a bug, but I think paranoia is preferable.

* bfd_slurp_bsd_armap_f2 leaks 'mapdata' before the patch -- it frees it
  on the error path but not on the normal path.  I fixed this.

* _bfd_xcoff_read_ar_hdr currently allocates 'ret' with bfd_alloc.  I
  think it should clear it instead; I did this.
  Also, this function already assumes 'ret' is malloced, which is a
  latent bug.


One final note on arelt_data: right now it is a void* in the BFD.  It
seems to me that it would be just as opaque, and more type-safe, to
change the structure name to 'struct bfd_areltdata', then use this name
in the BFD -- but leave the struct type incomplete so that library
clients can't dereference it.

Tom

2012-08-17  Tom Tromey  <tromey@redhat.com>

	* vms-lib.c (_bfd_vms_lib_get_module): Use bfd_zmalloc for
	areltdata.
	* opncls.c (_bfd_delete_bfd): Free arelt_data.
	* mach-o.c (bfd_mach_o_fat_member_init): Use bfd_zmalloc for
	areltdata.
	* ecoff.c (_bfd_ecoff_slurp_armap): Use free for mapdata.
	* coff-rs6000.c (_bfd_xcoff_read_ar_hdr): Use bfd_zmalloc for
	areltdata.
	(xcoff_write_archive_contents_old): Likewise.
	(xcoff_write_archive_contents_big): Likewise.
	* archive64.c (bfd_elf64_archive_slurp_armap): Use free for
	areltdata.
	* archive.c (_bfd_generic_read_ar_hdr_mag): Use bfd_zmalloc and
	free for areltdata.
	(_bfd_get_elt_at_filepos): Likewise.  Clear n_nfd->arelt_data on
	failure.
	(do_slurp_bsd_armap): Use bfd_zmalloc and free for areltdata.
	(do_slurp_coff_armap): Likewise.
	(_bfd_slurp_extended_name_table): Likewise.
	(bfd_slurp_bsd_armap_f2): Likewise.  Don't leak 'mapdata'.

diff --git a/bfd/archive.c b/bfd/archive.c
index 2d67e1f..4abee07 100644
--- a/bfd/archive.c
+++ b/bfd/archive.c
@@ -517,7 +517,7 @@ _bfd_generic_read_ar_hdr_mag (bfd *abfd, const char *mag)
       parsed_size -= namelen;
       extra_size = namelen;
 
-      allocptr = (char *) bfd_zalloc (abfd, allocsize);
+      allocptr = (char *) bfd_zmalloc (allocsize);
       if (allocptr == NULL)
 	return NULL;
       filename = (allocptr
@@ -560,7 +560,7 @@ _bfd_generic_read_ar_hdr_mag (bfd *abfd, const char *mag)
 
   if (!allocptr)
     {
-      allocptr = (char *) bfd_zalloc (abfd, allocsize);
+      allocptr = (char *) bfd_zmalloc (allocsize);
       if (allocptr == NULL)
 	return NULL;
     }
@@ -655,13 +655,13 @@ _bfd_get_elt_at_filepos (bfd *archive, file_ptr filepos)
 	  if (ext_arch == NULL
 	      || ! bfd_check_format (ext_arch, bfd_archive))
 	    {
-	      bfd_release (archive, new_areldata);
+	      free (new_areldata);
 	      return NULL;
 	    }
 	  n_nfd = _bfd_get_elt_at_filepos (ext_arch, new_areldata->origin);
 	  if (n_nfd == NULL)
 	    {
-	      bfd_release (archive, new_areldata);
+	      free (new_areldata);
 	      return NULL;
 	    }
 	  n_nfd->proxy_origin = bfd_tell (archive);
@@ -683,7 +683,7 @@ _bfd_get_elt_at_filepos (bfd *archive, file_ptr filepos)
 
   if (n_nfd == NULL)
     {
-      bfd_release (archive, new_areldata);
+      free (new_areldata);
       return NULL;
     }
 
@@ -707,7 +707,8 @@ _bfd_get_elt_at_filepos (bfd *archive, file_ptr filepos)
   if (_bfd_add_bfd_to_archive_cache (archive, filepos, n_nfd))
     return n_nfd;
 
-  bfd_release (archive, new_areldata);
+  free (new_areldata);
+  n_nfd->arelt_data = NULL;
   return NULL;
 }
 
@@ -894,7 +895,7 @@ do_slurp_bsd_armap (bfd *abfd)
   if (mapdata == NULL)
     return FALSE;
   parsed_size = mapdata->parsed_size;
-  bfd_release (abfd, mapdata);	/* Don't need it any more.  */
+  free (mapdata);
 
   raw_armap = (bfd_byte *) bfd_zalloc (abfd, parsed_size);
   if (raw_armap == NULL)
@@ -970,7 +971,7 @@ do_slurp_coff_armap (bfd *abfd)
   if (mapdata == NULL)
     return FALSE;
   parsed_size = mapdata->parsed_size;
-  bfd_release (abfd, mapdata);	/* Don't need it any more.  */
+  free (mapdata);
 
   if (bfd_bread (int_buf, 4, abfd) != 4)
     {
@@ -1063,7 +1064,7 @@ do_slurp_coff_armap (bfd *abfd)
 	    ardata->first_file_filepos +=
 	      (tmp->parsed_size + sizeof (struct ar_hdr) + 1) & ~(unsigned) 1;
 	  }
-	bfd_release (abfd, tmp);
+	free (tmp);
       }
   }
 
@@ -1180,15 +1181,17 @@ bfd_slurp_bsd_armap_f2 (bfd *abfd)
 
   if (mapdata->parsed_size < HPUX_SYMDEF_COUNT_SIZE + BSD_STRING_COUNT_SIZE)
     {
+      free (mapdata);
     wrong_format:
       bfd_set_error (bfd_error_wrong_format);
     byebye:
-      bfd_release (abfd, mapdata);
       return FALSE;
     }
   left = mapdata->parsed_size - HPUX_SYMDEF_COUNT_SIZE - BSD_STRING_COUNT_SIZE;
 
   amt = mapdata->parsed_size;
+  free (mapdata);
+
   raw_armap = (bfd_byte *) bfd_zalloc (abfd, amt);
   if (raw_armap == NULL)
     goto byebye;
@@ -1290,7 +1293,7 @@ _bfd_slurp_extended_name_table (bfd *abfd)
       if (bfd_ardata (abfd)->extended_names == NULL)
 	{
 	byebye:
-	  bfd_release (abfd, namedata);
+	  free (namedata);
 	  return FALSE;
 	}
 
@@ -1327,8 +1330,7 @@ _bfd_slurp_extended_name_table (bfd *abfd)
       bfd_ardata (abfd)->first_file_filepos +=
 	(bfd_ardata (abfd)->first_file_filepos) % 2;
 
-      /* FIXME, we can't release namedata here because it was allocated
-	 below extended_names on the objalloc...  */
+      free (namedata);
     }
   return TRUE;
 }
diff --git a/bfd/archive64.c b/bfd/archive64.c
index f3a13d3..db4ce2c 100644
--- a/bfd/archive64.c
+++ b/bfd/archive64.c
@@ -77,7 +77,7 @@ bfd_elf64_archive_slurp_armap (bfd *abfd)
   if (mapdata == NULL)
     return FALSE;
   parsed_size = mapdata->parsed_size;
-  bfd_release (abfd, mapdata);
+  free (mapdata);
 
   if (bfd_bread (int_buf, 8, abfd) != 8)
     {
diff --git a/bfd/coff-rs6000.c b/bfd/coff-rs6000.c
index 9326b32..edbef95 100644
--- a/bfd/coff-rs6000.c
+++ b/bfd/coff-rs6000.c
@@ -1496,7 +1496,7 @@ _bfd_xcoff_read_ar_hdr (bfd *abfd)
   struct areltdata *ret;
   bfd_size_type amt = sizeof (struct areltdata);
 
-  ret = (struct areltdata *) bfd_alloc (abfd, amt);
+  ret = (struct areltdata *) bfd_zmalloc (amt);
   if (ret == NULL)
     return NULL;
 
@@ -2113,7 +2113,7 @@ xcoff_write_archive_contents_old (bfd *abfd)
       total_namlen += strlen (normalize_filename (sub)) + 1;
       if (sub->arelt_data == NULL)
 	{
-	  sub->arelt_data = bfd_zalloc (sub, sizeof (struct areltdata));
+	  sub->arelt_data = bfd_zmalloc (sizeof (struct areltdata));
 	  if (sub->arelt_data == NULL)
 	    return FALSE;
 	}
@@ -2329,7 +2329,7 @@ xcoff_write_archive_contents_big (bfd *abfd)
       if (current_bfd->arelt_data == NULL)
 	{
 	  size = sizeof (struct areltdata);
-	  current_bfd->arelt_data = bfd_zalloc (current_bfd, size);
+	  current_bfd->arelt_data = bfd_zmalloc (size);
 	  if (current_bfd->arelt_data == NULL)
 	    return FALSE;
 	}
diff --git a/bfd/ecoff.c b/bfd/ecoff.c
index 3b65c0e..eaf8ada 100644
--- a/bfd/ecoff.c
+++ b/bfd/ecoff.c
@@ -2904,7 +2904,7 @@ _bfd_ecoff_slurp_armap (bfd *abfd)
   if (mapdata == NULL)
     return FALSE;
   parsed_size = mapdata->parsed_size;
-  bfd_release (abfd, (void *) mapdata);
+  free (mapdata);
 
   raw_armap = (char *) bfd_alloc (abfd, parsed_size);
   if (raw_armap == NULL)
diff --git a/bfd/mach-o.c b/bfd/mach-o.c
index 84d5a72..0379f4f 100644
--- a/bfd/mach-o.c
+++ b/bfd/mach-o.c
@@ -4294,7 +4294,7 @@ bfd_mach_o_fat_member_init (bfd *abfd,
       abfd->filename = name;
     }
 
-  areltdata = bfd_zalloc (abfd, sizeof (struct areltdata));
+  areltdata = bfd_zmalloc (sizeof (struct areltdata));
   areltdata->parsed_size = entry->size;
   abfd->arelt_data = areltdata;
   abfd->iostream = NULL;
diff --git a/bfd/opncls.c b/bfd/opncls.c
index b2ed9be..fdccba3 100644
--- a/bfd/opncls.c
+++ b/bfd/opncls.c
@@ -136,6 +136,7 @@ _bfd_delete_bfd (bfd *abfd)
       objalloc_free ((struct objalloc *) abfd->memory);
     }
 
+  free (abfd->arelt_data);
   free (abfd);
 }
 
diff --git a/bfd/vms-lib.c b/bfd/vms-lib.c
index fa23b78..56b80ad 100644
--- a/bfd/vms-lib.c
+++ b/bfd/vms-lib.c
@@ -1,6 +1,6 @@
 /* BFD back-end for VMS archive files.
 
-   Copyright 2010, 2011 Free Software Foundation, Inc.
+   Copyright 2010, 2011, 2012 Free Software Foundation, Inc.
    Written by Tristan Gingold <gingold@adacore.com>, AdaCore.
 
    This file is part of BFD, the Binary File Descriptor library.
@@ -1337,7 +1337,7 @@ _bfd_vms_lib_get_module (bfd *abfd, unsigned int modidx)
       res = _bfd_create_empty_archive_element_shell (abfd);
       if (res == NULL)
         return NULL;
-      arelt = bfd_zalloc (res, sizeof (*arelt));
+      arelt = bfd_zmalloc (sizeof (*arelt));
       if (arelt == NULL)
         return NULL;
       res->arelt_data = arelt;

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

[H.J. Lu]

On Fri, Aug 17, 2012 at 9:09 AM, Tom Tromey <tromey@redhat.com> wrote:
> Alan> Tom said he'd look into fixing the leak this causes, so I'm happy
> Alan> to leave that to him.  :)
>
> Here's the patch.
>
> I think it would be good for someone to double check it.
>
> I wrote this patch by searching for all the places that could allocate
> an areltdata and changing them to use bfd_zmalloc.
>
> Then I examined all the users of bfd->arelt_data and all callers of
> bfd_read_ar_hdr (and _bfd_read_ar_hdr) to see how the data was used.
> This revealed a number of spots that used bfd_release to free this data.
>
> Finally, I added a free to _bfd_delete_bfd.
>
> I once again built ld and all the binutils programs with -lmcheck, and
> then ran the test suites.  These all passed.  I also ran a single 'ar'
> test (one that was failing yesterday) plus the new 'bfdtest1' test under
> valgrind.  These were also clean.
>
>
> I found a few oddities in BFD while working on this patch:
>
> * _bfd_get_elt_at_filepos can release new_areldata but still leave a
>   stale pointer in n_nfd->arelt_data.  I fixed this.  I am not sure if
>   this can ever result in a bug, but I think paranoia is preferable.
>
> * bfd_slurp_bsd_armap_f2 leaks 'mapdata' before the patch -- it frees it
>   on the error path but not on the normal path.  I fixed this.
>
> * _bfd_xcoff_read_ar_hdr currently allocates 'ret' with bfd_alloc.  I
>   think it should clear it instead; I did this.
>   Also, this function already assumes 'ret' is malloced, which is a
>   latent bug.
>
>
> One final note on arelt_data: right now it is a void* in the BFD.  It
> seems to me that it would be just as opaque, and more type-safe, to
> change the structure name to 'struct bfd_areltdata', then use this name
> in the BFD -- but leave the struct type incomplete so that library
> clients can't dereference it.
>
> Tom
>
> 2012-08-17  Tom Tromey  <tromey@redhat.com>
>
>         * vms-lib.c (_bfd_vms_lib_get_module): Use bfd_zmalloc for
>         areltdata.
>         * opncls.c (_bfd_delete_bfd): Free arelt_data.
>         * mach-o.c (bfd_mach_o_fat_member_init): Use bfd_zmalloc for
>         areltdata.
>         * ecoff.c (_bfd_ecoff_slurp_armap): Use free for mapdata.
>         * coff-rs6000.c (_bfd_xcoff_read_ar_hdr): Use bfd_zmalloc for
>         areltdata.
>         (xcoff_write_archive_contents_old): Likewise.
>         (xcoff_write_archive_contents_big): Likewise.
>         * archive64.c (bfd_elf64_archive_slurp_armap): Use free for
>         areltdata.
>         * archive.c (_bfd_generic_read_ar_hdr_mag): Use bfd_zmalloc and
>         free for areltdata.
>         (_bfd_get_elt_at_filepos): Likewise.  Clear n_nfd->arelt_data on
>         failure.
>         (do_slurp_bsd_armap): Use bfd_zmalloc and free for areltdata.
>         (do_slurp_coff_armap): Likewise.
>         (_bfd_slurp_extended_name_table): Likewise.
>         (bfd_slurp_bsd_armap_f2): Likewise.  Don't leak 'mapdata'.

I am not sure if we want to use bfd_zmalloc for all areltdata.  We
use bfd_ar_hdr_from_filesystem since we can't use member
objalloc nor archive objalloc.  In all other places, it is OK to
use archive objalloc for areltdata.

Do you have a testcase to show there is a problem?

Thanks.

-- 
H.J.

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

[Tom Tromey]

>>>>> "HJ" == H J Lu <hjl.tools@gmail.com> writes:

HJ> I am not sure if we want to use bfd_zmalloc for all areltdata.  We
HJ> use bfd_ar_hdr_from_filesystem since we can't use member
HJ> objalloc nor archive objalloc.  In all other places, it is OK to
HJ> use archive objalloc for areltdata.

Aside from the latent bugs.

HJ> Do you have a testcase to show there is a problem?

barimba. valgrind --leak-check=full ./strip-new libutil.a 
[...]
==30145== 696 bytes in 6 blocks are definitely lost in loss record 21 of 21
==30145==    at 0x4A074CD: malloc (vg_replace_malloc.c:236)
==30145==    by 0x432A58: bfd_zmalloc (libbfd.c:319)
==30145==    by 0x428807: bfd_ar_hdr_from_filesystem (archive.c:1899)
==30145==    by 0x428E4D: _bfd_write_archive_contents (archive.c:2131)
==30145==    by 0x4342FE: bfd_close (opncls.c:714)
==30145==    by 0x407991: copy_archive (objcopy.c:2208)
==30145==    by 0x407E54: copy_file (objcopy.c:2318)
==30145==    by 0x404699: main (objcopy.c:3168)

Tom

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

[H.J. Lu]

On Fri, Aug 17, 2012 at 9:50 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "HJ" == H J Lu <hjl.tools@gmail.com> writes:
>
> HJ> I am not sure if we want to use bfd_zmalloc for all areltdata.  We
> HJ> use bfd_ar_hdr_from_filesystem since we can't use member
> HJ> objalloc nor archive objalloc.  In all other places, it is OK to
> HJ> use archive objalloc for areltdata.
>
> Aside from the latent bugs.
>
> HJ> Do you have a testcase to show there is a problem?
>
> barimba. valgrind --leak-check=full ./strip-new libutil.a
> [...]
> ==30145== 696 bytes in 6 blocks are definitely lost in loss record 21 of 21
> ==30145==    at 0x4A074CD: malloc (vg_replace_malloc.c:236)
> ==30145==    by 0x432A58: bfd_zmalloc (libbfd.c:319)
> ==30145==    by 0x428807: bfd_ar_hdr_from_filesystem (archive.c:1899)
> ==30145==    by 0x428E4D: _bfd_write_archive_contents (archive.c:2131)
> ==30145==    by 0x4342FE: bfd_close (opncls.c:714)
> ==30145==    by 0x407991: copy_archive (objcopy.c:2208)
> ==30145==    by 0x407E54: copy_file (objcopy.c:2318)
> ==30145==    by 0x404699: main (objcopy.c:3168)
>

From

http://sourceware.org/ml/binutils/2012-08/msg00309.html

diff --git a/bfd/archive.c b/bfd/archive.c
index 2d67e1f..af6ba24 100644
--- a/bfd/archive.c
+++ b/bfd/archive.c
@@ -2744,6 +2744,11 @@ _bfd_archive_close_and_cleanup (bfd *abfd)
 	      htab_clear_slot (htab, slot);
 	    }
 	}
+      else
+	{
+	  /* If HTAB is NULL, free ARED allocated with bfd_zmalloc.  */
+	  free (ared);
+	}
     }
   return TRUE;
 }

However, it assumes that archive member from filesystem is
closed after archive.  It won't be easy to get around it since
we can't get from archive member from filesystem to
archive.


-- 
H.J.

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

[Tom Tromey]

>>>>> "HJ" == H J Lu <hjl.tools@gmail.com> writes:

HJ> +      else
HJ> +	{
HJ> +	  /* If HTAB is NULL, free ARED allocated with bfd_zmalloc.  */
HJ> +	  free (ared);
HJ> +	}

HJ> However, it assumes that archive member from filesystem is
HJ> closed after archive.  It won't be easy to get around it since
HJ> we can't get from archive member from filesystem to
HJ> archive.

I prefer my approach, because although my patch is more complicated, the
resulting code is simpler.  I think this because, after my patch, an
areltdata is allocated in a single way and also freed in a single way.
This uniformity makes them easier to reason about.

I find the above quite obscure.  It assumes an invariant that is not
obvious and that is also not documented.

If you go with your patch, I'd recommend you also fix the latent bugs I
pointed out.  Some of them would require different fixes than what I
posted.

Tom

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

[H.J. Lu]

On Fri, Aug 17, 2012 at 10:11 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "HJ" == H J Lu <hjl.tools@gmail.com> writes:
>
> HJ> +      else
> HJ> +   {
> HJ> +     /* If HTAB is NULL, free ARED allocated with bfd_zmalloc.  */
> HJ> +     free (ared);
> HJ> +   }
>
> HJ> However, it assumes that archive member from filesystem is
> HJ> closed after archive.  It won't be easy to get around it since
> HJ> we can't get from archive member from filesystem to
> HJ> archive.
>
> I prefer my approach, because although my patch is more complicated, the
> resulting code is simpler.  I think this because, after my patch, an
> areltdata is allocated in a single way and also freed in a single way.
> This uniformity makes them easier to reason about.

Given
  archive = bfd_openr (argv[1], NULL);

  for (last = bfd_openr_next_archived_file (archive, NULL);
       last;
       last = next)
    {
      next = bfd_openr_next_archived_file (archive, last);
      bfd_close (last);
    }

will bfd_close (last) call free (abfd->arelt_data)? If it does,
archive bfd will have bad member arelt_data.

> I find the above quite obscure.  It assumes an invariant that is not
> obvious and that is also not documented.
>
> If you go with your patch, I'd recommend you also fix the latent bugs I
> pointed out.  Some of them would require different fixes than what I
> posted.
>

Those latent bugs should be fixed by separate patches.


-- 
H.J.

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

[Tom Tromey]

>>>>> "HJ" == H J Lu <hjl.tools@gmail.com> writes:

HJ>   archive = bfd_openr (argv[1], NULL);
HJ>   for (last = bfd_openr_next_archived_file (archive, NULL);
HJ>        last;
HJ>        last = next)
HJ>     {
HJ>       next = bfd_openr_next_archived_file (archive, last);
HJ>       bfd_close (last);
HJ>     }

HJ> will bfd_close (last) call free (abfd->arelt_data)?

By 'abfd->arelt_data' I guess you mean 'last->arelt_data'.

HJ> If it does, archive bfd will have bad member arelt_data.

How do you figure?

I tried this with my patch in place using my archive-reading program
(the one I stuck in whatever PR it was), running under valgrind.  It
does the above.  I didn't see any issue.  I don't understand how such a
problem could possibly arise.

Tom

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

[H.J. Lu]

On Fri, Aug 17, 2012 at 11:17 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "HJ" == H J Lu <hjl.tools@gmail.com> writes:
>
> HJ>   archive = bfd_openr (argv[1], NULL);
> HJ>   for (last = bfd_openr_next_archived_file (archive, NULL);
> HJ>        last;
> HJ>        last = next)
> HJ>     {
> HJ>       next = bfd_openr_next_archived_file (archive, last);
> HJ>       bfd_close (last);
> HJ>     }
>
> HJ> will bfd_close (last) call free (abfd->arelt_data)?
>
> By 'abfd->arelt_data' I guess you mean 'last->arelt_data'.
>
> HJ> If it does, archive bfd will have bad member arelt_data.
>
> How do you figure?
>
> I tried this with my patch in place using my archive-reading program
> (the one I stuck in whatever PR it was), running under valgrind.  It
> does the above.  I didn't see any issue.  I don't understand how such a
> problem could possibly arise.
>

Your patch is probably OK.


-- 
H.J.

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

[Alan Modra]

On Fri, Aug 17, 2012 at 10:09:33AM -0600, Tom Tromey wrote:
> I found a few oddities in BFD while working on this patch:

Only a few? :)

> --- a/bfd/archive.c
> +++ b/bfd/archive.c
> @@ -517,7 +517,7 @@ _bfd_generic_read_ar_hdr_mag (bfd *abfd, const char *mag)
>        parsed_size -= namelen;
>        extra_size = namelen;
>  
> -      allocptr = (char *) bfd_zalloc (abfd, allocsize);
> +      allocptr = (char *) bfd_zmalloc (allocsize);
>        if (allocptr == NULL)
>  	return NULL;
>        filename = (allocptr

There's a "return NULL" a few line down from here where you should
free allocptr.

> @@ -655,13 +655,13 @@ _bfd_get_elt_at_filepos (bfd *archive, file_ptr filepos)
>  	  if (ext_arch == NULL
>  	      || ! bfd_check_format (ext_arch, bfd_archive))
>  	    {
> -	      bfd_release (archive, new_areldata);
> +	      free (new_areldata);
>  	      return NULL;
>  	    }

Ditto for a few lines before here.  Other than that, this looks like a
nice cleanup.  Thanks!

-- 
Alan Modra
Australia Development Lab, IBM

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

[Tom Tromey]

Alan> Only a few? :)

:)

[...]
Alan> There's a "return NULL" a few line down from here where you should
Alan> free allocptr.
[...]
Alan> Ditto for a few lines before here.  Other than that, this looks like a
Alan> nice cleanup.  Thanks!

Thanks for the review.  Here is what I am checking in.

Tom

2012-08-20  Tom Tromey  <tromey@redhat.com>

	* vms-lib.c (_bfd_vms_lib_get_module): Use bfd_zmalloc for
	areltdata.
	* opncls.c (_bfd_delete_bfd): Free arelt_data.
	* mach-o.c (bfd_mach_o_fat_member_init): Use bfd_zmalloc for
	areltdata.
	* ecoff.c (_bfd_ecoff_slurp_armap): Use free for mapdata.
	* coff-rs6000.c (_bfd_xcoff_read_ar_hdr): Use bfd_zmalloc for
	areltdata.
	(xcoff_write_archive_contents_old): Likewise.
	(xcoff_write_archive_contents_big): Likewise.
	* archive64.c (bfd_elf64_archive_slurp_armap): Use free for
	areltdata.
	* archive.c (_bfd_generic_read_ar_hdr_mag): Use bfd_zmalloc and
	free for areltdata.
	(_bfd_get_elt_at_filepos): Likewise.  Clear n_nfd->arelt_data on
	failure.
	(do_slurp_bsd_armap): Use bfd_zmalloc and free for areltdata.
	(do_slurp_coff_armap): Likewise.
	(_bfd_slurp_extended_name_table): Likewise.
	(bfd_slurp_bsd_armap_f2): Likewise.  Don't leak 'mapdata'.

Index: archive.c
===================================================================
RCS file: /cvs/src/src/bfd/archive.c,v
retrieving revision 1.91
diff -u -r1.91 archive.c
--- archive.c	17 Aug 2012 01:06:27 -0000	1.91
+++ archive.c	20 Aug 2012 14:31:05 -0000
@@ -517,7 +517,7 @@
       parsed_size -= namelen;
       extra_size = namelen;
 
-      allocptr = (char *) bfd_zalloc (abfd, allocsize);
+      allocptr = (char *) bfd_zmalloc (allocsize);
       if (allocptr == NULL)
 	return NULL;
       filename = (allocptr
@@ -525,6 +525,7 @@
 		  + sizeof (struct ar_hdr));
       if (bfd_bread (filename, namelen, abfd) != namelen)
 	{
+	  free (allocptr);
 	  if (bfd_get_error () != bfd_error_system_call)
 	    bfd_set_error (bfd_error_no_more_archived_files);
 	  return NULL;
@@ -560,7 +561,7 @@
 
   if (!allocptr)
     {
-      allocptr = (char *) bfd_zalloc (abfd, allocsize);
+      allocptr = (char *) bfd_zmalloc (allocsize);
       if (allocptr == NULL)
 	return NULL;
     }
@@ -643,7 +644,10 @@
 	{
 	  filename = _bfd_append_relative_path (archive, filename);
 	  if (filename == NULL)
-	    return NULL;
+	    {
+	      free (new_areldata);
+	      return NULL;
+	    }
 	}
 
       if (new_areldata->origin > 0)
@@ -655,13 +659,13 @@
 	  if (ext_arch == NULL
 	      || ! bfd_check_format (ext_arch, bfd_archive))
 	    {
-	      bfd_release (archive, new_areldata);
+	      free (new_areldata);
 	      return NULL;
 	    }
 	  n_nfd = _bfd_get_elt_at_filepos (ext_arch, new_areldata->origin);
 	  if (n_nfd == NULL)
 	    {
-	      bfd_release (archive, new_areldata);
+	      free (new_areldata);
 	      return NULL;
 	    }
 	  n_nfd->proxy_origin = bfd_tell (archive);
@@ -683,7 +687,7 @@
 
   if (n_nfd == NULL)
     {
-      bfd_release (archive, new_areldata);
+      free (new_areldata);
       return NULL;
     }
 
@@ -707,7 +711,8 @@
   if (_bfd_add_bfd_to_archive_cache (archive, filepos, n_nfd))
     return n_nfd;
 
-  bfd_release (archive, new_areldata);
+  free (new_areldata);
+  n_nfd->arelt_data = NULL;
   return NULL;
 }
 
@@ -894,7 +899,7 @@
   if (mapdata == NULL)
     return FALSE;
   parsed_size = mapdata->parsed_size;
-  bfd_release (abfd, mapdata);	/* Don't need it any more.  */
+  free (mapdata);
 
   raw_armap = (bfd_byte *) bfd_zalloc (abfd, parsed_size);
   if (raw_armap == NULL)
@@ -970,7 +975,7 @@
   if (mapdata == NULL)
     return FALSE;
   parsed_size = mapdata->parsed_size;
-  bfd_release (abfd, mapdata);	/* Don't need it any more.  */
+  free (mapdata);
 
   if (bfd_bread (int_buf, 4, abfd) != 4)
     {
@@ -1063,7 +1068,7 @@
 	    ardata->first_file_filepos +=
 	      (tmp->parsed_size + sizeof (struct ar_hdr) + 1) & ~(unsigned) 1;
 	  }
-	bfd_release (abfd, tmp);
+	free (tmp);
       }
   }
 
@@ -1180,15 +1185,17 @@
 
   if (mapdata->parsed_size < HPUX_SYMDEF_COUNT_SIZE + BSD_STRING_COUNT_SIZE)
     {
+      free (mapdata);
     wrong_format:
       bfd_set_error (bfd_error_wrong_format);
     byebye:
-      bfd_release (abfd, mapdata);
       return FALSE;
     }
   left = mapdata->parsed_size - HPUX_SYMDEF_COUNT_SIZE - BSD_STRING_COUNT_SIZE;
 
   amt = mapdata->parsed_size;
+  free (mapdata);
+
   raw_armap = (bfd_byte *) bfd_zalloc (abfd, amt);
   if (raw_armap == NULL)
     goto byebye;
@@ -1290,7 +1297,7 @@
       if (bfd_ardata (abfd)->extended_names == NULL)
 	{
 	byebye:
-	  bfd_release (abfd, namedata);
+	  free (namedata);
 	  return FALSE;
 	}
 
@@ -1327,8 +1334,7 @@
       bfd_ardata (abfd)->first_file_filepos +=
 	(bfd_ardata (abfd)->first_file_filepos) % 2;
 
-      /* FIXME, we can't release namedata here because it was allocated
-	 below extended_names on the objalloc...  */
+      free (namedata);
     }
   return TRUE;
 }
Index: archive64.c
===================================================================
RCS file: /cvs/src/src/bfd/archive64.c,v
retrieving revision 1.15
diff -u -r1.15 archive64.c
--- archive64.c	20 Jan 2012 14:42:57 -0000	1.15
+++ archive64.c	20 Aug 2012 14:31:05 -0000
@@ -77,7 +77,7 @@
   if (mapdata == NULL)
     return FALSE;
   parsed_size = mapdata->parsed_size;
-  bfd_release (abfd, mapdata);
+  free (mapdata);
 
   if (bfd_bread (int_buf, 8, abfd) != 8)
     {
Index: coff-rs6000.c
===================================================================
RCS file: /cvs/src/src/bfd/coff-rs6000.c,v
retrieving revision 1.106
diff -u -r1.106 coff-rs6000.c
--- coff-rs6000.c	24 Jul 2012 21:06:57 -0000	1.106
+++ coff-rs6000.c	20 Aug 2012 14:31:05 -0000
@@ -1496,7 +1496,7 @@
   struct areltdata *ret;
   bfd_size_type amt = sizeof (struct areltdata);
 
-  ret = (struct areltdata *) bfd_alloc (abfd, amt);
+  ret = (struct areltdata *) bfd_zmalloc (amt);
   if (ret == NULL)
     return NULL;
 
@@ -2113,7 +2113,7 @@
       total_namlen += strlen (normalize_filename (sub)) + 1;
       if (sub->arelt_data == NULL)
 	{
-	  sub->arelt_data = bfd_zalloc (sub, sizeof (struct areltdata));
+	  sub->arelt_data = bfd_zmalloc (sizeof (struct areltdata));
 	  if (sub->arelt_data == NULL)
 	    return FALSE;
 	}
@@ -2329,7 +2329,7 @@
       if (current_bfd->arelt_data == NULL)
 	{
 	  size = sizeof (struct areltdata);
-	  current_bfd->arelt_data = bfd_zalloc (current_bfd, size);
+	  current_bfd->arelt_data = bfd_zmalloc (size);
 	  if (current_bfd->arelt_data == NULL)
 	    return FALSE;
 	}
Index: ecoff.c
===================================================================
RCS file: /cvs/src/src/bfd/ecoff.c,v
retrieving revision 1.76
diff -u -r1.76 ecoff.c
--- ecoff.c	13 Jul 2012 14:22:45 -0000	1.76
+++ ecoff.c	20 Aug 2012 14:31:06 -0000
@@ -2904,7 +2904,7 @@
   if (mapdata == NULL)
     return FALSE;
   parsed_size = mapdata->parsed_size;
-  bfd_release (abfd, (void *) mapdata);
+  free (mapdata);
 
   raw_armap = (char *) bfd_alloc (abfd, parsed_size);
   if (raw_armap == NULL)
Index: mach-o.c
===================================================================
RCS file: /cvs/src/src/bfd/mach-o.c,v
retrieving revision 1.103
diff -u -r1.103 mach-o.c
--- mach-o.c	24 Jul 2012 21:06:58 -0000	1.103
+++ mach-o.c	20 Aug 2012 14:31:06 -0000
@@ -4294,7 +4294,7 @@
       abfd->filename = name;
     }
 
-  areltdata = bfd_zalloc (abfd, sizeof (struct areltdata));
+  areltdata = bfd_zmalloc (sizeof (struct areltdata));
   areltdata->parsed_size = entry->size;
   abfd->arelt_data = areltdata;
   abfd->iostream = NULL;
Index: opncls.c
===================================================================
RCS file: /cvs/src/src/bfd/opncls.c,v
retrieving revision 1.72
diff -u -r1.72 opncls.c
--- opncls.c	9 Aug 2012 06:25:53 -0000	1.72
+++ opncls.c	20 Aug 2012 14:31:06 -0000
@@ -136,6 +136,7 @@
       objalloc_free ((struct objalloc *) abfd->memory);
     }
 
+  free (abfd->arelt_data);
   free (abfd);
 }
 
Index: vms-lib.c
===================================================================
RCS file: /cvs/src/src/bfd/vms-lib.c,v
retrieving revision 1.23
diff -u -r1.23 vms-lib.c
--- vms-lib.c	13 Jul 2012 14:22:50 -0000	1.23
+++ vms-lib.c	20 Aug 2012 14:31:06 -0000
@@ -1,6 +1,6 @@
 /* BFD back-end for VMS archive files.
 
-   Copyright 2010, 2011 Free Software Foundation, Inc.
+   Copyright 2010, 2011, 2012 Free Software Foundation, Inc.
    Written by Tristan Gingold <gingold@adacore.com>, AdaCore.
 
    This file is part of BFD, the Binary File Descriptor library.
@@ -1337,7 +1337,7 @@
       res = _bfd_create_empty_archive_element_shell (abfd);
       if (res == NULL)
         return NULL;
-      arelt = bfd_zalloc (res, sizeof (*arelt));
+      arelt = bfd_zmalloc (sizeof (*arelt));
       if (arelt == NULL)
         return NULL;
       res->arelt_data = arelt;

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

[Alan Modra]

On Fri, Aug 17, 2012 at 10:18:25AM +0930, Alan Modra wrote:
> On Thu, Aug 16, 2012 at 11:02:18AM -0600, Tom Tromey wrote:
> > Another possible fix for this bug would be to allocate the areltdata
> > using malloc.  That way it would be immune to the objalloc_free call.
> > This would require a few more tweaks, like properly freeing it in
> > _bfd_delete_bfd, etc.
> > 
> > I'm happy to make and test this change if you think it would be better.
> 
> Yes, I do think that would be better.  bfd_free_cached_info is called
> only in one place, and the whole point of the call is as the comment
> says
> 	  /* Now ask the BFD to free up any cached information, so we
> 	     don't fill all of memory with symbol tables.  */
> 
> If you don't free the bfd memory there isn't much point in having
> bfd_free_cached_info!
> 
> > 	* opncls.c (_bfd_delete_bfd): Check to see if section htab is
> > 	already freed.
> 
> This part can be committed now if you like.

And in the interests of having a binutils tree that work, I'm
committing this fix now.

	PR binutils/14475:
	* archive.c (bfd_ar_hdr_from_filesystem): Revert last change.
	Instead malloc areltdata.

Index: bfd/archive.c
===================================================================
RCS file: /cvs/src/src/bfd/archive.c,v
retrieving revision 1.90
diff -u -p -r1.90 archive.c
--- bfd/archive.c	16 Aug 2012 14:24:44 -0000	1.90
+++ bfd/archive.c	17 Aug 2012 01:02:13 -0000
@@ -1896,7 +1896,7 @@ bfd_ar_hdr_from_filesystem (bfd *abfd, c
     }
 
   amt = sizeof (struct ar_hdr) + sizeof (struct areltdata);
-  ared = (struct areltdata *) bfd_zalloc (member, amt);
+  ared = (struct areltdata *) bfd_zmalloc (amt);
   if (ared == NULL)
     return NULL;
   hdr = (struct ar_hdr *) (((char *) ared) + sizeof (struct areltdata));
@@ -1927,7 +1927,10 @@ bfd_ar_hdr_from_filesystem (bfd *abfd, c
   _bfd_ar_spacepad (hdr->ar_mode, sizeof (hdr->ar_mode), "%-8lo",
 		    status.st_mode);
   if (!_bfd_ar_sizepad (hdr->ar_size, sizeof (hdr->ar_size), status.st_size))
-    return NULL;
+    {
+      free (ared);
+      return NULL;
+    }
   memcpy (hdr->ar_fmag, ARFMAG, 2);
   ared->parsed_size = status.st_size;
   ared->arch_header = (char *) hdr;

-- 
Alan Modra
Australia Development Lab, IBM

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

[Tom Tromey]

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

>> * opncls.c (_bfd_delete_bfd): Check to see if section htab is
>> already freed.

Alan> This part can be committed now if you like.

The issue was double-freeing of section_htab.  Because _bfd_delete_bfd
checks abfd->memory before freeing the section_htab, and because
_bfd_free_cached_info still calls objalloc_free, there's no need to do
this check.

Tom

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

[H.J. Lu]

On Fri, Aug 17, 2012 at 7:55 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Alan" == Alan Modra <amodra@gmail.com> writes:
>
>>> * opncls.c (_bfd_delete_bfd): Check to see if section htab is
>>> already freed.
>
> Alan> This part can be committed now if you like.
>
> The issue was double-freeing of section_htab.  Because _bfd_delete_bfd
> checks abfd->memory before freeing the section_htab, and because
> _bfd_free_cached_info still calls objalloc_free, there's no need to do
> this check.
>

Agreed.

BTW, we still have a memory leak with bfd_zmalloc.  But I don't know
how serious it is.


--
H.J.

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

[Tom Tromey]

HJ> BTW, we still have a memory leak with bfd_zmalloc.  But I don't know
HJ> how serious it is.

I'm testing a patch.
Writing this patch uncovered various latent bugs as well.

Tom

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

[H.J. Lu]

On Wed, Aug 15, 2012 at 2:03 PM, Tom Tromey <tromey@redhat.com> wrote:
> HJ> It breaks strip:
> HJ> http://sourceware.org/bugzilla/show_bug.cgi?id=14475
>
> Thanks
>
> Here is a patch to fix the problem.
>
> The bug is that bfd_ar_hdr_from_filesystem creates the areltdata on the
> archive's objalloc.  But, since this data is attached to the member BFD,
> it should really be created on the member's objalloc.
>
> This also fixes another bug I noticed in bfd_ar_hdr_from_filesystem.
> This code tries to free 'ared' in one case -- but that is wrong, as this
> is not allocated using malloc.
>
> I could see the bug before this patch using valgrind or -lmcheck.  After
> the patch the problem is gone.
>
> Ok?
>
> Tom
>
> 2012-08-15  Tom Tromey  <tromey@redhat.com>
>
>         PR binutils/14475:
>         * archive.c (bfd_ar_hdr_from_filesystem): Allocate areltdata on
>         'member' BFD.  Don't try to free 'ared'.
>
> Index: archive.c
> ===================================================================
> RCS file: /cvs/src/src/bfd/archive.c,v
> retrieving revision 1.89
> diff -u -r1.89 archive.c
> --- archive.c   9 Aug 2012 06:25:52 -0000       1.89
> +++ archive.c   15 Aug 2012 21:01:51 -0000
> @@ -1896,7 +1896,7 @@
>      }
>
>    amt = sizeof (struct ar_hdr) + sizeof (struct areltdata);
> -  ared = (struct areltdata *) bfd_zalloc (abfd, amt);
> +  ared = (struct areltdata *) bfd_zalloc (member, amt);
>    if (ared == NULL)
>      return NULL;
>    hdr = (struct ar_hdr *) (((char *) ared) + sizeof (struct areltdata));
> @@ -1927,10 +1927,7 @@
>    _bfd_ar_spacepad (hdr->ar_mode, sizeof (hdr->ar_mode), "%-8lo",
>                     status.st_mode);
>    if (!_bfd_ar_sizepad (hdr->ar_size, sizeof (hdr->ar_size), status.st_size))
> -    {
> -      free (ared);
> -      return NULL;
> -    }
> +    return NULL;
>    memcpy (hdr->ar_fmag, ARFMAG, 2);
>    ared->parsed_size = status.st_size;
>    ared->arch_header = (char *) hdr;

I am not sure if it is correct to use the member's objalloc since
areltdata may be allocated from _bfd_generic_read_ar_hdr_mag
which takes archive bfd.

-- 
H.J.