Re: Runtime regression for gdb.base/reread.exp & co. [Re: [RFA] Remove target_section.bfd]

[Doug Evans <dje at google dot com>]

Jan Kratochvil writes:
 > On Thu, 18 Jul 2013 18:24:34 +0200, Doug Evans wrote:
 > > Blech, I can't recreate this on an ubuntu or fc17 system.
 > 
 > Tried it on Fedora 17 x86_64 and it is really not reproducible (PASSes) with:
 > 	CFLAGS=-g ./configure
 > 
 > but the regression is reproducible with:
 > 	CFLAGS=-g ./configure --enable-libmcheck

Ah, thanks.

The problem is in this function:

void
remove_target_sections (void *key, bfd *abfd)
{
  struct target_section *src, *dest;
  struct target_section_table *table = current_target_sections;

  dest = table->sections;
  for (src = table->sections; src < table->sections_end; src++)
    if (src->key != key || src->the_bfd_section->owner != abfd)
      {
	/* Keep this section.  */
	if (dest < src)
	  *dest = *src;
	dest++;
      }

When exec_close calls this function the bfd has already been closed.
Therefore src->the_bfd_section->owner now contains garbage and so
"src->the_bfd_section->owner != abfd" succeeds and we end up keeping the
now deleted section.

I think yet more cleanup could be applied here, but I'd like to reach
a stopping point we can agree on (and I'm not yet willing to revert
my patch - though I will if it comes to that of course).
There's just so much cleanup that is needed I'd like to make
some forward progress ...

struct target_section has member "key" that is used to help distinguish
sections from different origins (generally, executable and shlibs).

The change was added here:
http://sourceware.org/ml/gdb-patches/2012-07/msg00742.html
Tom: Can you remember the reproducer for the issue fixed by this patch?

The "constructor", if you will, is exec.c:add_to_section_table but
it sets the key to NULL.  It is only when exec.c:add_target_sections is
called that key is set to non-NULL, and add_target_sections is *only*
called for executables and shlibs.
[Seems like another cleanup is to initialize "key" in the constructor,
but I'd like to leave that for another day.  I can look into that
if you want, however.]

The only place the key is used is in remove_target_sections,
which is also only called for executables and shlibs.
So remove_target_sections will never see a non-NULL key.
Furthermore, it seems reasonable to me to require that when we're
removing sections in remove_target_sections for an object
(executable or shlib) it's reasonable from an API point of view
to just remove all of them. [And if that doesn't work my first thought
is that yet more cleanup is required ...]

Note that all calls to remove_target_sections pass the bfd from "key":

exec.c:

  bfd *abfd = exec_bfd;
  ...
  remove_target_sections (&exec_bfd, abfd);

solib.c:

  remove_target_sections (gdb, gdb->abfd);
  remove_target_sections (so, so->abfd);
  remove_target_sections (so, so->abfd);

Thus I think(!) removing the bfd argument to remove_target_sections
is reasonable and safe:
[I'm showing the pre-target-section.bfd removal version of the code here,
since this patch could, I think, be applied even if that patch got reverted.]

-    if (src->key != key || src->bfd != abfd)
+    if (src->key != key)

It's also a bit cleaner: add_target_sections doesn't take a bfd argument.

Could be wrong of course, let me know your thoughts.

The name "key" is now a bit less clear than it could be,
I like "owner" better.

Another change in this patch worth pointing out is in solib.c:clear_solib:

-      if (so->abfd)
-	remove_target_sections (so, so->abfd);
+      remove_target_sections (so);

There's no need for the test AFAICT: If the so doesn't have a bfd then
it won't be in the target section table.  [In which case we'll be calling
remove_target_sections unnecessarily, but the speedup is a micro-optimization.
Otherwise the reader has to reason about why the test is there: Is it more
than just a (micro-)optimization?]

So how about this patch?
Regression tested on amd64-linux with --enable-libmcheck,
and verified it fixes the reread.exp regression.

2013-07-18  Doug Evans  <dje@google.com>

	* exec.h (remove_target_sections): Delete arg abfd.
	* exec.c (remove_target_sections): Delete arg abfd.
	(exec_close): Update call to remove_target_sections.
	* solib.c (update_solib_list): Ditto.
	(reload_shared_libraries_1): Ditto.
	(clear_solib): Ditto, and unconditionally call remove_target_sections.

Index: exec.c
===================================================================
RCS file: /cvs/src/src/gdb/exec.c,v
retrieving revision 1.126
diff -u -p -r1.126 exec.c
--- exec.c	16 Jul 2013 20:41:55 -0000	1.126
+++ exec.c	19 Jul 2013 00:11:29 -0000
@@ -101,7 +101,7 @@ exec_close (void)
       exec_bfd = NULL;
       exec_bfd_mtime = 0;
 
-      remove_target_sections (&exec_bfd, abfd);
+      remove_target_sections (&exec_bfd);
     }
 }
 
@@ -339,7 +339,7 @@ add_to_section_table (bfd *abfd, struct 
   if (!(aflag & SEC_ALLOC))
     return;
 
-  (*table_pp)->key = NULL;
+  (*table_pp)->owner = NULL;
   (*table_pp)->the_bfd_section = asect;
   (*table_pp)->addr = bfd_section_vma (abfd, asect);
   (*table_pp)->endaddr = (*table_pp)->addr + bfd_section_size (abfd, asect);
@@ -397,7 +397,7 @@ build_section_table (struct bfd *some_bf
    current set of target sections.  */
 
 void
-add_target_sections (void *key,
+add_target_sections (void *owner,
 		     struct target_section *sections,
 		     struct target_section *sections_end)
 {
@@ -414,7 +414,7 @@ add_target_sections (void *key,
       for (i = 0; i < count; ++i)
 	{
 	  table->sections[space + i] = sections[i];
-	  table->sections[space + i].key = key;
+	  table->sections[space + i].owner = owner;
 	}
 
       /* If these are the first file sections we can provide memory
@@ -427,17 +427,20 @@ add_target_sections (void *key,
     }
 }
 
-/* Remove all target sections taken from ABFD.  */
+/* Remove all target sections owned by OWNER.
+   OWNER must be the same value passed to add_target_sections.  */
 
 void
-remove_target_sections (void *key, bfd *abfd)
+remove_target_sections (void *owner)
 {
   struct target_section *src, *dest;
   struct target_section_table *table = current_target_sections;
 
+  gdb_assert (owner != NULL);
+
   dest = table->sections;
   for (src = table->sections; src < table->sections_end; src++)
-    if (src->key != key || src->the_bfd_section->owner != abfd)
+    if (src->owner != owner)
       {
 	/* Keep this section.  */
 	if (dest < src)
Index: exec.h
===================================================================
RCS file: /cvs/src/src/gdb/exec.h,v
retrieving revision 1.20
diff -u -p -r1.20 exec.h
--- exec.h	1 Jan 2013 06:32:42 -0000	1.20
+++ exec.h	19 Jul 2013 00:11:29 -0000
@@ -81,14 +81,14 @@ extern int section_table_xfer_memory_par
 /* Set the loaded address of a section.  */
 extern void exec_set_section_address (const char *, int, CORE_ADDR);
 
-/* Remove all target sections taken from ABFD.  */
+/* Remove all target sections owned by OWNER.  */
 
-extern void remove_target_sections (void *key, bfd *abfd);
+extern void remove_target_sections (void *owner);
 
 /* Add the sections array defined by [SECTIONS..SECTIONS_END[ to the
    current set of target sections.  */
 
-extern void add_target_sections (void *key,
+extern void add_target_sections (void *owner,
 				 struct target_section *sections,
 				 struct target_section *sections_end);
 
Index: solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.176
diff -u -p -r1.176 solib.c
--- solib.c	4 Jun 2013 13:17:06 -0000	1.176
+++ solib.c	19 Jul 2013 00:11:29 -0000
@@ -770,7 +770,7 @@ update_solib_list (int from_tty, struct 
 
 	  /* Some targets' section tables might be referring to
 	     sections from so->abfd; remove them.  */
-	  remove_target_sections (gdb, gdb->abfd);
+	  remove_target_sections (gdb);
 
 	  free_so (gdb);
 	  gdb = *gdb_link;
@@ -1151,8 +1151,7 @@ clear_solib (void)
 
       so_list_head = so->next;
       observer_notify_solib_unloaded (so);
-      if (so->abfd)
-	remove_target_sections (so, so->abfd);
+      remove_target_sections (so);
       free_so (so);
     }
 
@@ -1276,7 +1275,7 @@ reload_shared_libraries_1 (int from_tty)
 	  if (so->objfile && ! (so->objfile->flags & OBJF_USERLOADED)
 	      && !solib_used (so))
 	    free_objfile (so->objfile);
-	  remove_target_sections (so, so->abfd);
+	  remove_target_sections (so);
 	  clear_so (so);
 	}
 
Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/target.h,v
retrieving revision 1.265
diff -u -p -r1.265 target.h
--- target.h	16 Jul 2013 20:41:55 -0000	1.265
+++ target.h	19 Jul 2013 00:11:29 -0000
@@ -1893,11 +1893,12 @@ struct target_section
 
     struct bfd_section *the_bfd_section;
 
-    /* A given BFD may appear multiple times in the target section
-       list, so each BFD is associated with a given key.  The key is
-       just some convenient pointer that can be used to differentiate
-       the BFDs.  These are managed only by convention.  */
-    void *key;
+    /* The "owner" of the section.
+       It can be any unique value.  It is set by add_target_sections
+       and used by remove_target_sections.
+       For example, for executables it is a pointer to exec_bfd and
+       for shlibs it is the so_list pointer.  */
+    void *owner;
   };
 
 /* Holds an array of target sections.  Defined by [SECTIONS..SECTIONS_END[.  */