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

Doug Evans
2013-07-19T00:20:15Z

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[. */