[binutils-gdb] gdb: move all bfd_cache_close_all calls in gdb_bfd.c

[binutils-gdb] gdb: move all bfd_cache_close_all calls in gdb_bfd.c

[Andrew Burgess]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=96619f154a3b9bbd8e3dbd5b4408fa4f27d67f17

commit 96619f154a3b9bbd8e3dbd5b4408fa4f27d67f17
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Tue Oct 24 17:54:51 2023 +0100

    gdb: move all bfd_cache_close_all calls in gdb_bfd.c
    
    In the following commit I ran into a problem.  The next commit aims to
    improve GDB's handling of the main executable being a file on a remote
    target (i.e. one with a 'target:' prefix).
    
    To do this I have replaced a system 'stat' call with a bfd_stat call.
    
    However, doing this caused a regression in gdb.base/attach.exp.
    
    The problem is that the bfd library caches open FILE* handles for bfd
    objects that it has accessed, which is great for short-lived, non
    interactive programs (e.g. the assembler, or objcopy, etc), however,
    for GDB this caching causes us a problem.
    
    If we open the main executable as a bfd then the bfd library will
    cache the open FILE*.  If some time passes, maybe just sat at the GDB
    prompt, or with the inferior running, and then later we use bfd_stat
    to check if the underlying, on-disk file has changed, then the bfd
    library will actually use fstat on the underlying file descriptor.
    This is of course slightly different than using system stat on with
    the on-disk file name.
    
    If the on-disk file has changed then system stat will give results for
    the current on-disk file.  But, if the bfd cache is still holding open
    the file descriptor for the original on-disk file (from before the
    change) then fstat will return a result based on the original file,
    and so show no change as having happened.
    
    This is a known problem in GDB, and so far this has been solved by
    scattering bfd_cache_close_all() calls throughout GDB.  But, as I
    said, in the next commit I've made a change and run into a
    problem (gdb.base/attach.exp) where we are apparently missing a
    bfd_cache_close_all() call.
    
    Now I could solve this problem by adding a bfd_cache_close_all() call
    before the bfd_stat call that I plan to add in the next commit, that
    would for sure solve the problem, but feels a little crude.
    
    Better I think would be to track down where the bfd is being opened
    and add a corresponding bfd_cache_close_all() call elsewhere in GDB
    once we've finished doing whatever it is that caused us to open the
    bfd in the first place.
    
    This second solution felt like the better choice, so I tracked the
    problem down to elf_locate_base and fixed that.  But that just exposed
    another problem in gdb_bfd_map_section which was also re-opening the
    bfd, so I fixed this (with another bfd_cache_close_all() call), and
    that exposed another issue in gdbarch_lookup_osabi... and at this
    point I wondered if I was approaching this problem the wrong way...
    
    .... And so, I wonder, is there a _better_ way to handle these
    bfd_cache_close_all() calls?
    
    I see two problems with the current approach:
    
      1. It's fragile.  Folk aren't always aware that they need to clear
      the bfd cache, and this feels like something that is easy to
      overlook in review.  So adding new code to GDB can innocently touch
      a bfd, which populates the cache, which will then be a bug that can
      lie hidden until an on-disk file just happens to change at the wrong
      time ... and GDB fails to spot the change.  Additionally,
    
      2. It's in efficient.  The caching is intended to stop the bfd
      library from continually having to re-open the on-disk file.  If we
      have a function that touches a bfd then often that function is the
      obvious place to call bfd_cache_close_all.  But if a single GDB
      command calls multiple functions, each of which touch the bfd, then
      we will end up opening and closing the same on-disk file multiple
      times.  It feels like we would be better postponing the
      bfd_cache_close_all call until some later point, then we can benefit
      from the bfd cache.
    
    So, in this commit I propose a new approach.  We now clear the bfd
    cache in two places:
    
      (a) Just before we display a GDB prompt.  We display a prompt after
      completing a command, and GDB is about to enter an idle state
      waiting for further input from the user (or in async mode, for an
      inferior event).  If while we are in this idle state the user
      changes the on-disk file(s) then we would like GDB to notice this
      the next time it leaves its idle state, e.g. the next time the user
      executes a command, or when an inferior event arrives,
    
      (b) When we resume the inferior.  In synchronous mode, resuming the
      inferior is another time when GDB is blocked and sitting idle, but
      in this case we don't display a prompt.  As with (a) above, when an
      inferior event arrives we want GDB to notice any changes to on-disk
      files.
    
    It turns out that there are existing observers for both of these
    cases (before_prompt and target_resumed respectively), so my initial
    thought was that I should attach to these observers in gdb_bfd.c, and
    in both cases call bfd_cache_close_all().
    
    And this does indeed solve the gdb.base/attach.exp problem that I see
    with the following commit.
    
    However, I see a problem with this solution.
    
    Both of the observers I'm using are exposed through the Python API as
    events that a user can hook into.  The user can potentially run any
    GDB command (using gdb.execute), so Python code might end up causing
    some bfds to be reopened, and inserted into the cache.
    
    To solve this one solution would be to add a bfd_cache_close_all()
    call into gdbpy_enter::~gdbpy_enter().  Unfortunately, there's no
    similar enter/exit object for Guile, though right now Guile doesn't
    offer the same event API, so maybe we could just ignore that
    problem... but this doesn't feel great.
    
    So instead, I think a better solution might be to not use observers
    for the bfd_cache_close_all() calls.  Instead, I'll call
    bfd_cache_close_all() directly from core GDB after we've notified the
    before_prompt and target_resumed observers, this was we can be sure
    that the cache is cleared after the observers have run, and before GDB
    enters an idle state.
    
    This commit also removes all of the other bfd_cache_close_all() calls
    from GDB.  My claim is that these are no longer needed.
    
    Approved-By: Tom Tromey <tom@tromey.com>

Diff:
---
 gdb/corefile.c  |  5 -----
 gdb/event-top.c | 20 +++++++++++++++++---
 gdb/exec.c      |  2 --
 gdb/infcmd.c    |  1 -
 gdb/inferior.c  |  2 --
 gdb/symfile.c   |  1 -
 gdb/target.c    |  5 -----
 gdb/thread.c    |  5 +++++
 8 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/gdb/corefile.c b/gdb/corefile.c
index c27061a3ae3..19a96bc6f86 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -120,11 +120,6 @@ reopen_exec_file (void)
       && current_program_space->ebfd_mtime
       && current_program_space->ebfd_mtime != st.st_mtime)
     exec_file_attach (filename.c_str (), 0);
-  else
-    /* If we accessed the file since last opening it, close it now;
-       this stops GDB from holding the executable open after it
-       exits.  */
-    bfd_cache_close_all ();
 }
 \f
 /* If we have both a core file and an exec file,
diff --git a/gdb/event-top.c b/gdb/event-top.c
index 9886ca46e7b..6ce53704539 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -462,6 +462,22 @@ display_gdb_prompt (const char *new_prompt)
     }
 }
 
+/* Notify the 'before_prompt' observer, and run any additional actions
+   that must be done before we display the prompt.  */
+static void
+notify_before_prompt (const char *prompt)
+{
+  /* Give observers a chance of changing the prompt.  E.g., the python
+     `gdb.prompt_hook' is installed as an observer.  */
+  gdb::observers::before_prompt.notify (prompt);
+
+  /* As we are about to display the prompt, and so GDB might be sitting
+     idle for some time, close all the cached BFDs.  This ensures that
+     when we next start running a user command all BFDs will be reopened
+     as needed, and as a result, we will see any on-disk changes.  */
+  bfd_cache_close_all ();
+}
+
 /* Return the top level prompt, as specified by "set prompt", possibly
    overridden by the python gdb.prompt_hook hook, and then composed
    with the prompt prefix and suffix (annotations).  */
@@ -469,9 +485,7 @@ display_gdb_prompt (const char *new_prompt)
 static std::string
 top_level_prompt (void)
 {
-  /* Give observers a chance of changing the prompt.  E.g., the python
-     `gdb.prompt_hook' is installed as an observer.  */
-  gdb::observers::before_prompt.notify (get_prompt ().c_str ());
+  notify_before_prompt (get_prompt ().c_str ());
 
   const std::string &prompt = get_prompt ();
 
diff --git a/gdb/exec.c b/gdb/exec.c
index 5956012338f..59965b84d55 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -500,8 +500,6 @@ exec_file_attach (const char *filename, int from_tty)
 	(*deprecated_exec_file_display_hook) (filename);
     }
 
-  bfd_cache_close_all ();
-
   /* Are are loading the same executable?  */
   bfd *prev_bfd = exec_bfd_holder.get ();
   bfd *curr_bfd = current_program_space->exec_bfd ();
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 8890a16eade..2fc0eb138a6 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2499,7 +2499,6 @@ kill_command (const char *arg, int from_tty)
   int infnum = current_inferior ()->num;
 
   target_kill ();
-  bfd_cache_close_all ();
 
   update_previous_thread ();
 
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 1778723863e..927c5f16ae2 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -714,8 +714,6 @@ kill_inferior_command (const char *args, int from_tty)
 
       target_kill ();
     }
-
-  bfd_cache_close_all ();
 }
 
 /* See inferior.h.  */
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 7f7ac337806..bbac6ad9df1 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1124,7 +1124,6 @@ symbol_file_add_with_addrs (const gdb_bfd_ref_ptr &abfd, const char *name,
 
   gdb::observers::new_objfile.notify (objfile);
 
-  bfd_cache_close_all ();
   return objfile;
 }
 
diff --git a/gdb/target.c b/gdb/target.c
index 8e5c934b457..92aa1dd882e 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2737,11 +2737,6 @@ target_mourn_inferior (ptid_t ptid)
 {
   gdb_assert (ptid.pid () == inferior_ptid.pid ());
   current_inferior ()->top_target ()->mourn_inferior ();
-
-  /* We no longer need to keep handles on any of the object files.
-     Make sure to release them to avoid unnecessarily locking any
-     of them while we're not actually debugging.  */
-  bfd_cache_close_all ();
 }
 
 /* Look for a target which can describe architectural features, starting
diff --git a/gdb/thread.c b/gdb/thread.c
index 47cc5c9cd14..810fdae18a8 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -894,6 +894,11 @@ notify_target_resumed (ptid_t ptid)
 {
   interps_notify_target_resumed (ptid);
   gdb::observers::target_resumed.notify (ptid);
+
+  /* We are about to resume the inferior.  Close all cached BFDs so that
+     when the inferior next stops, and GDB regains control, we will spot
+     any on-disk changes to the BFDs we are using.  */
+  bfd_cache_close_all ();
 }
 
 /* See gdbthread.h.  */