[0/17] RFC: share minimal symbols across objfiles

[0/17] RFC: share minimal symbols across objfiles

[Tom Tromey]

This patch series changes gdb to allow some sharing of data across
objfiles.  In particular this series adds sharing of minimal symbols.

The big idea here is that most data in an objfile is actually
independent of the inferior.  So, if multiple inferiors refer to the
data, we should be able to read it just once and then share.

This was discussed earlier:

    http://sourceware.org/ml/gdb-patches/2009-06/msg00668.html


I wanted to post this patch series to get some comments on it.  Although
it does pass regression testing, it is not ready to check in yet.  It
has a few hacks and a few FIXMEs, and one un-finished project.

One major problem with it right now is that because it duplicates the
demangled_names_hash, it probably requires more memory at runtime for
the single-inferior case than an unmodified gdb.  I haven't tried to
measure this.

The other major problem with this patch is that it makes it clear how
the BFD cache is fundamentally confused.

The BFD cache maintains a limited number of open file descriptors for
BFD objects.  (Right now this limit is hard coded as 10, which is
absurdly low.)  BFD will automatically close file descriptors and will
reopen the underlying file when needed.

However, GDB has its own logic for reopening files -- it notices when
the mtime has changed and will try to reread symbols at this point.

Now consider the case where you have two inferiors accessing the same
underlying file.  Suppose further that one inferior is running, and the
underlying file is changed and the user re-runs the second inferior.  If
the cache decides to reopen BFDs during this process, the first inferior
could try to access invalid data (depending on when the various debug
sections are mapped.)

Also there are comments in gdb indicating that the BFDs must be closed,
because on some operating systems keeping them open will mean that the
file cannot be exec'd.

In sum, it is a mess and I am not sure what to do about it.  Mapping all
the needed debug sections is possible, but slow, and in the
multi-inferior case that seems to be a recipe for running out of memory.
(But we can run out of memory anyway and probably need some approach for
fixing this...)


In contrast to my previous big patch, I split this one up as much as I
thought reasonably possible.  I haven't regtested all the patches
individually, but I will probably do that at some point.

Tom

Re: [0/17] RFC: share minimal symbols across objfiles

[Joel Brobecker]

> The other major problem with this patch is that it makes it clear how
> the BFD cache is fundamentally confused.
[...]
> In sum, it is a mess and I am not sure what to do about it.  Mapping all
> the needed debug sections is possible, but slow, and in the
> multi-inferior case that seems to be a recipe for running out of memory.
> (But we can run out of memory anyway and probably need some approach for
> fixing this...)

I don't see a way out of this without involving BFD. If we could
register for notifications for a given bfd, I think it would allow
us to stay informed of what BFD has, so far, been doing behind
the scene...

-- 
Joel

Re: [0/17] RFC: share minimal symbols across objfiles

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Tom> The other major problem with this patch is that it makes it clear how
Tom> the BFD cache is fundamentally confused.
[...]
Tom> In sum, it is a mess and I am not sure what to do about it.  Mapping all
Tom> the needed debug sections is possible, but slow, and in the
Tom> multi-inferior case that seems to be a recipe for running out of memory.
Tom> (But we can run out of memory anyway and probably need some approach for
Tom> fixing this...)

Joel> I don't see a way out of this without involving BFD. If we could
Joel> register for notifications for a given bfd, I think it would allow
Joel> us to stay informed of what BFD has, so far, been doing behind
Joel> the scene...

I think the problem is deeper than just BFD, though I agree we probably
need to fix up BFD a bit somehow.  (Actually, I tend to think we should
just not use the BFD cache.)

We want to keep the fd open on the BFD, so that we can lazily map
sections.  If we close the fd, we might get different contents when we
reopen it.  This is actually a latent bug in gdb right now.

However, according to a comment in fork-child., we have to close the fd
in order to exec on some targets:

  /* On some systems an exec will fail if the executable is open.  */
  close_exec_file ();

This is the basic contradiction.


One idea I had is that, in this situation, we could eagerly map all the
sections we think we might ever want.

Another idea is to notice that the mtime has changed on reopen and
prevent us from mapping any more sections from that BFD.

It would be helpful to know which systems have this issue.  Maybe we
could make it a gdbarch parameter, so that we don't need workarounds on
more capable systems.

Tom

Re: [0/17] RFC: share minimal symbols across objfiles

[Joel Brobecker]

> It would be helpful to know which systems have this issue.  Maybe we
> could make it a gdbarch parameter, so that we don't need workarounds on
> more capable systems.

Just a wild guess: Windows, and hppa-hpux. I don't remember of any other
filesystems that lock their files. Oddly, I remember seeing some
messages on GNU/Linux machines which oddly looked like the kind of
message you would get from a locking filesystem. But I don't remember
the details, and it was racy anyway.  I'm not completely sure about
hppa-hpux, because I think you can still run a program that's locked
by someone else, especially just for reading.  The only thing that
you can't do, IIRC, is delete (but you can move out of the way!).
Unfortunately, I can't check anymore, because I no longer have access
to hppa-hpux machines.

I have been thinking of deprecating hppa-hpux, and maybe all of hppa
if no one steps up. That might help a bit. I would love to deprecate
Windows, but I won't even dare dream about it. Besides, it turns out
that there are much much worse platforms than Windows, even if it
often drives me crazy.

If we can't determine where and how this is needed (maybe there's some
archeological info to be dug out of CVS?), we might want to consider
the idea of doing without it. And wait to see if it breaks someone.
I know that Jerome and I have occasionaly looked at this sort of
issue, but haven't always been succesful at making a proper patch.
I can try digging things up, but it's getting late, so maybe tomorrow.

-- 
Joel

[commit] delete corefile.c:close_exec_file (was: "Re: [0/17] RFC: share minimal symbols across objfiles")

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]

> However, according to a comment in fork-child., we have to close the fd
> in order to exec on some targets:
> 
>   /* On some systems an exec will fail if the executable is open.  */
>   close_exec_file ();
> 
> This is the basic contradiction.

I reasearched this one, and I'm afraid that it predates CVS.

But the good news is in corefile.c:

    | /* The exec file must be closed before running an inferior.
    |    If it is needed again after the inferior dies, it must
    |    be reopened.  */
    | 
    | void
    | close_exec_file (void)
    | {
    | #if 0                           /* FIXME */
    |   if (exec_bfd)
    |     bfd_tempclose (exec_bfd);
    | #endif
    | }

Ha ha ha!

The code has been commented out side Jul 07, 1999 (the revision history
says: "import gdb-1999-07-07 post reformat").

Attached is the patch that I just checked in as obvious.  Does it
simplify a bit your work?

gdb/ChangeLog:

        * corefile.c (close_exec_file): Delete.
        (reopen_exec_file): Remove commented out code that seems related
        to close_exec_file, which is being deleted here.
        * inferior.h (close_exec_file): Delete.
        * fork-child.c (fork_inferior): Remove call to fork_inferior.

Tested by rebuilding GDB on x86_64-linux.  Testsuite running, but
how can it regress?

-- 
Joel

[-- Attachment #2: 0001-Get-rid-of-corefile.c-close_exec_file.patch --]
[-- Type: text/x-diff, Size: 3231 bytes --]

From 36ac6184edccab0330e7d87a23f02e97f22b8ae9 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Wed, 4 Jan 2012 19:07:24 +0400
Subject: [PATCH] Get rid of corefile.c:close_exec_file

The body of this function has been commented out since Jul 1999, and
thus seems unnecessary. While at it, remove some commented out code
that seems to be related to the function being deleted.

gdb/ChangeLog:

	* corefile.c (close_exec_file): Delete.
	(reopen_exec_file): Remove commented out code that seems related
	to close_exec_file, which is being deleted here.
	* inferior.h (close_exec_file): Delete.
	* fork-child.c (fork_inferior): Remove call to fork_inferior.
---
 gdb/ChangeLog    |    8 ++++++++
 gdb/corefile.c   |   18 ------------------
 gdb/fork-child.c |    3 ---
 gdb/inferior.h   |    2 --
 4 files changed, 8 insertions(+), 23 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 8bdb758..445e931 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,13 @@
 2012-01-04  Joel Brobecker  <brobecker@adacore.com>
 
+	* corefile.c (close_exec_file): Delete.
+	(reopen_exec_file): Remove commented out code that seems related
+	to close_exec_file, which is being deleted here.
+	* inferior.h (close_exec_file): Delete.
+	* fork-child.c (fork_inferior): Remove call to fork_inferior.
+
+2012-01-04  Joel Brobecker  <brobecker@adacore.com>
+
 	* ada-lang.c: #include "cli/cli-utils.h".
 	(get_selections): Use skip_spaces.
 	(ada_get_next_arg): Use skip_spaces and skip_to_space.
diff --git a/gdb/corefile.c b/gdb/corefile.c
index c07447b..1741e9c 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -132,26 +132,9 @@ specify_exec_file_hook (void (*hook) (char *))
     deprecated_exec_file_display_hook = hook;
 }
 
-/* The exec file must be closed before running an inferior.
-   If it is needed again after the inferior dies, it must
-   be reopened.  */
-
-void
-close_exec_file (void)
-{
-#if 0				/* FIXME */
-  if (exec_bfd)
-    bfd_tempclose (exec_bfd);
-#endif
-}
-
 void
 reopen_exec_file (void)
 {
-#if 0				/* FIXME */
-  if (exec_bfd)
-    bfd_reopen (exec_bfd);
-#else
   char *filename;
   int res;
   struct stat st;
@@ -175,7 +158,6 @@ reopen_exec_file (void)
     bfd_cache_close_all ();
 
   do_cleanups (cleanups);
-#endif
 }
 \f
 /* If we have both a core file and an exec file,
diff --git a/gdb/fork-child.c b/gdb/fork-child.c
index cba91f9..3b8da49 100644
--- a/gdb/fork-child.c
+++ b/gdb/fork-child.c
@@ -272,9 +272,6 @@ fork_inferior (char *exec_file_arg, char *allargs, char **env,
       argv[3] = (char *) 0;
     }
 
-  /* On some systems an exec will fail if the executable is open.  */
-  close_exec_file ();
-
   /* Retain a copy of our environment variables, since the child will
      replace the value of environ and if we're vforked, we have to
      restore it.  */
diff --git a/gdb/inferior.h b/gdb/inferior.h
index f9b3656..f05789f 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -154,8 +154,6 @@ extern void fetch_inferior_event (void *);
 
 extern void init_wait_for_inferior (void);
 
-extern void close_exec_file (void);
-
 extern void reopen_exec_file (void);
 
 /* The `resume' routine should only be called in special circumstances.
-- 
1.7.1

Re: [commit] delete corefile.c:close_exec_file

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> Ha ha ha!

Indeed :)

Joel> Attached is the patch that I just checked in as obvious.  Does it
Joel> simplify a bit your work?

Yes, I think it does, but there are still a lot of bfd_cache_close_all
calls to contend with.  Maybe these hid the fact that close_exec_file
did nothing.

I imagine at least some of these calls are needed on some platforms.
I haven't tried to test this yet.

Some of the calls seem a bit weird.  E.g., the one in
symbol_file_add_with_addrs_or_offsets, from this thread:

    http://sourceware.org/ml/gdb-patches/2004-07/msg00439.html

I'm still thinking about what options we have.

Tom

Re: [commit] delete corefile.c:close_exec_file

[Joel Brobecker]

> Yes, I think it does, but there are still a lot of bfd_cache_close_all
> calls to contend with.  Maybe these hid the fact that close_exec_file
> did nothing.

Ah, yes, of course. These, I know we're responsible for some of them.
And there is at least one that we tried to contribute, wasn't quite
right, and then fell on the side. I think. But we should be able to
find the history of each one.

> I imagine at least some of these calls are needed on some platforms.
> I haven't tried to test this yet.

Yes, I am pretty sure they are, unfortunately.

> Some of the calls seem a bit weird.  E.g., the one in
> symbol_file_add_with_addrs_or_offsets, from this thread:
> 
>     http://sourceware.org/ml/gdb-patches/2004-07/msg00439.html

I reconnected the thread in the archives (a real shame that
the threading does not work across months...):

    [RFA] win32: bfd_cache_close after kill
    http://www.sourceware.org/ml/gdb-patches/2004-05/msg00678.html
    http://www.sourceware.org/ml/gdb-patches/2004-05/msg00756.html
    http://www.sourceware.org/ml/gdb-patches/2004-06/msg00008.html
    http://www.sourceware.org/ml/gdb-patches/2004-06/msg00512.html

In the process of doing so, I found:

    [commit] Close the executable after it exits
    http://sourceware.org/ml/gdb-patches/2008-04/msg00706.html
    (from DanielJ)

    [patch] release handle on object files after program exits
    http://sourceware.org/ml/gdb-patches/2009-04/msg00074.html
    (from yours truly)

I think it should be reasonably easy to find the history of each
of them should we need to. Based on the few messages I glanced at,
it seems that they deal with unlocking the exe after program exits
or gets killed by GDB. The main platform involved is Windows, with
HP-UX being mentioned as well...

-- 
Joel

Re: [0/17] RFC: share minimal symbols across objfiles

[Joel Brobecker]

> It would be helpful to know which systems have this issue.  Maybe we
> could make it a gdbarch parameter, so that we don't need workarounds on
> more capable systems.

Is exec-bfd the only objfile with the issue? To me, the answer seems
an obvious NOT, but you never know. Going back to this question,
I'm just going to repeat as a reminder that the only systems I remember
having file-locking problems that forced us to close fds are pa-hpux,
and Windows in general. The sort of issue I remember seeing was trying
to update the executable while the program is opened by GDB and yet
not running. I still haven't gotten to te bottom of some failures see
in GDB on GNU/Linux systems as well, with symptoms very similar to
a file-locking issue. I started looking at them when I had a free
second or two, but I could never make sense out of them in the short
amount of time I could dedicate.

The gdbarch parameter seems risky to me, it's often more a property
of the filesystem than a property of the platform...

Having spent a fair amount of time on many of these exotic platforms,
I would be happy to try helping a bit.

-- 
Joel

Re: [0/17] RFC: share minimal symbols across objfiles

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> This patch series changes gdb to allow some sharing of data across
Tom> objfiles.  In particular this series adds sharing of minimal symbols.

Tom> The big idea here is that most data in an objfile is actually
Tom> independent of the inferior.  So, if multiple inferiors refer to the
Tom> data, we should be able to read it just once and then share.

I recently thought of a problem with this patch series.

If an inferior uses dlmopen to load a .so multiple times, then the
patched gdb will fail.  The problem is that the series (in particular
patch #14) assumes that a given minimal symbol appears a single time in
the inferior; but with dlmopen it may appear multiple times.

I'm still not sure of the best way to fix this problem.  One idea is to
have the various searching functions return a {minsym, objfile} pair,
rather than just a pointer to the minimal symbol.  This would be a
pretty big (if mostly mechanical) patch, though.

Another approach I've considered a little is having gdb create relocated
minimal symbols on the fly, and cache them per-objfile.  Then the lookup
functions can always return these cached, relocated minsyms.  This uses
a bit more memory (but presumably not too bad -- most symbols aren't
used); but it is also obviously just a hack to work around a design
flaw.  On the whole I think the first idea is cleaner.

Maybe there is some other approach that would work.

I think I'd like to resurrect parts of this series (the BFD refcounting
and filename changes, and registry.h) and put them in, since they are
reasonable (IMO :) cleanups in their own right.

Tom