[PATCH 0/2] BFD cache management And Exec file with target: prefix

[PATCH 0/2] BFD cache management And Exec file with target: prefix

[Andrew Burgess]

This started with patch #2, which improves GDB's handling of the main
executable having a target: prefix, but that ran into an issue with
the bfd cache.

So patch #1 aims to address this cache issue.

WARNING: There is still an open question for patch #1, I'd appreciate
peoples thoughts on the right approach.

---

Andrew Burgess (2):
  gdb: move all bfd_cache_close_all calls in gdb_bfd.c
  gdb: fix reopen_exec_file for files with target: prefix

 gdb/corefile.c                                | 22 +++---
 gdb/exec.c                                    |  2 -
 gdb/gdb_bfd.c                                 | 20 ++++++
 gdb/infcmd.c                                  |  1 -
 gdb/inferior.c                                |  2 -
 gdb/symfile.c                                 |  1 -
 gdb/target.c                                  |  5 --
 gdb/testsuite/gdb.server/target-exec-file.exp | 70 +++++++++++++++++++
 8 files changed, 100 insertions(+), 23 deletions(-)


base-commit: a0094f1a70e1d5a7a8124e7c988fc1ddd1886f19
-- 
2.25.4

[PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c

[Andrew Burgess]

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 replace 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.

Nicely, there are existing observers for both of these
cases (before_prompt and target_resumed respectively), so, in
gdb_bfd.c I've hooked into both of these and arranged to clear the bfd
cache.

With this commit in place the next commit no longer shows any issues
with gdb.base/attach.exp.

One possible problem that I see with this commit is: what if some
other user of one of the observers I've hooked into causes a bfd to be
opened after my new observers have run?  If this happened then we
would proceed with a populated bfd cache, and this might causes
problems.

Right now, the only other users of the observers that I'm connecting
too are the extension languages, specifically, Python.  I suspect it
must be possible for Python to carry out actions that can cause the
bfd cache to be populated, so maybe there is a risk here.

To counter this risk, we could move the bfd_cache_close_all() calls
out of observers, and move them into GDB core close too, but after the
two observers in questions are actually called, so into
top_level_prompt for the before_prompt observer and into
notify_target_resumed for the target_resumed observer.

Another choice would be to add a bfd_cache_close_all() into
gdbpy_enter::~gdbpy_enter, so whenever we call into a Python
extension, we always clear the bfd cache once we're done.

For now I haven't made either of these changes, maybe I'm worrying
about nothing?  I'd appreciate peoples thoughts.

This commit also removes all of the other bfd_cache_close_all() calls
from GDB.  My claim is that these are no longer needed.
---
 gdb/corefile.c |  5 -----
 gdb/exec.c     |  2 --
 gdb/gdb_bfd.c  | 20 ++++++++++++++++++++
 gdb/infcmd.c   |  1 -
 gdb/inferior.c |  2 --
 gdb/symfile.c  |  1 -
 gdb/target.c   |  5 -----
 7 files changed, 20 insertions(+), 16 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/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/gdb_bfd.c b/gdb/gdb_bfd.c
index 56a4c5ecc91..9b7a06da231 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -33,6 +33,7 @@
 #include "gdbsupport/fileio.h"
 #include "inferior.h"
 #include "cli/cli-style.h"
+#include "observable.h"
 #include <unordered_map>
 
 /* An object of this type is stored in the section's user data when
@@ -1171,10 +1172,29 @@ gdb_bfd_error_handler (const char *fmt, va_list ap)
   (*default_bfd_error_handler) (fmt, ap);
 }
 
+/* A before_prompt observer.  */
+
+static void
+gdb_bfd_before_prompt (const char * /* ignored */)
+{
+  bfd_cache_close_all ();
+}
+
+/* A target_resumed observer.  */
+
+static void
+gdb_bfd_target_resumed (ptid_t /* ignored */)
+{
+  bfd_cache_close_all ();
+}
+
 void _initialize_gdb_bfd ();
 void
 _initialize_gdb_bfd ()
 {
+  gdb::observers::target_resumed.attach (gdb_bfd_target_resumed, "gdb-bfd");
+  gdb::observers::before_prompt.attach (gdb_bfd_before_prompt, "gdb-bfd");
+
   all_bfds = htab_create_alloc (10, htab_hash_pointer, htab_eq_pointer,
 				NULL, xcalloc, xfree);
 
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index cf8cd527955..5153843dde8 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2498,7 +2498,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 eebc5ea44b9..24570372316 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 f688ff33e3b..aeb53dd91d0 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2729,11 +2729,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
-- 
2.25.4

[PATCH 2/2] gdb: fix reopen_exec_file for files with target: prefix

[Andrew Burgess]

Following on from this commit:

  commit f2c4f78c813a9cef38b7e9c9ad18822fb9e19345
  Date:   Thu Sep 21 16:35:30 2023 +0100

      gdb: fix reread_symbols when an objfile has target: prefix

In this commit I update reopen_exec_file to correctly handle
executables with a target: prefix.  Before this commit we used the
system 'stat' call, which obviously isn't going to work for files with
a target: prefix (files located on a possibly remote target machine).

By switching to bfd_stat we will use remote fileio to stat the remote
files, which means we should now correctly detect changes in a remote
executable.

The program_space::ebfd_mtime variable, with which we compare the
result of bfd_stat is set with a call to bfd_get_mtime, which in turn
calls bfd_stat, so comparing to the result of calling bfd_stat makes
sense (I think).

As I discussed in the commit f2c4f78c813a, if a BFD is an in-memory
BFD, then calling bfd_stat will always return 0, while bfd_get_mtime
will always return the time at which the BFD was created.  As a result
comparing the results will always show the file having changed.

I don't believe that GDB can set the main executable to an in-memory
BFD object, so, in this commit, I simply assert that the executable is
not in-memory.  If this ever changes then we would need to decide how
to handle this case -- always reload, or never reload.  The assert
doesn't appear to trigger for our current test suite.
---
 gdb/corefile.c                                | 17 +++--
 gdb/testsuite/gdb.server/target-exec-file.exp | 70 +++++++++++++++++++
 2 files changed, 80 insertions(+), 7 deletions(-)

diff --git a/gdb/corefile.c b/gdb/corefile.c
index 19a96bc6f86..cfbb852b3c2 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -105,21 +105,24 @@ specify_exec_file_hook (void (*hook) (const char *))
 void
 reopen_exec_file (void)
 {
-  int res;
-  struct stat st;
-
   /* Don't do anything if there isn't an exec file.  */
   if (current_program_space->exec_bfd () == NULL)
     return;
 
+  bfd *exec_bfd = current_program_space->exec_bfd ();
+
+  /* The main executable can't be an in-memory BFD object.  If it was then
+     the use of bfd_stat below would not work as expected.  */
+  gdb_assert ((exec_bfd->flags & BFD_IN_MEMORY) == 0);
+
   /* If the timestamp of the exec file has changed, reopen it.  */
-  std::string filename = bfd_get_filename (current_program_space->exec_bfd ());
-  res = stat (filename.c_str (), &st);
+  struct stat st;
+  int res = bfd_stat (exec_bfd, &st);
 
   if (res == 0
-      && current_program_space->ebfd_mtime
+      && current_program_space->ebfd_mtime != 0
       && current_program_space->ebfd_mtime != st.st_mtime)
-    exec_file_attach (filename.c_str (), 0);
+    exec_file_attach (bfd_get_filename (exec_bfd), 0);
 }
 \f
 /* If we have both a core file and an exec file,
diff --git a/gdb/testsuite/gdb.server/target-exec-file.exp b/gdb/testsuite/gdb.server/target-exec-file.exp
index 9260df8b88d..40863538785 100644
--- a/gdb/testsuite/gdb.server/target-exec-file.exp
+++ b/gdb/testsuite/gdb.server/target-exec-file.exp
@@ -52,6 +52,19 @@ set target_exec [gdb_remote_download target $binfile]
 # prompt us if this is the right thing to do.
 gdb_test_no_output "set confirm off"
 
+if { [allow_python_tests] } {
+    # Register an event handler for the executable changed event.
+    # This handler just copies the event into a global Python object.
+    gdb_test_multiline "Add connection_removed event" \
+	"python" "" \
+	"global_exec_changed_event = None" "" \
+	"def executable_changed(event):" "" \
+	"   global global_exec_changed_event" "" \
+	"   global_exec_changed_event = event" "" \
+	"gdb.events.executable_changed.connect (executable_changed)" "" \
+	"end" ""
+}
+
 # Start gdbserver, but always in extended-remote mode, and then
 # connect to it from GDB.
 set res [gdbserver_start "--multi" $target_exec]
@@ -59,6 +72,22 @@ set gdbserver_protocol "extended-remote"
 set gdbserver_gdbport [lindex $res 1]
 gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport
 
+if { [allow_python_tests] } {
+    # When connecting to a remote target, if the user has not told GDB
+    # which executable to use, then GDB will figure out an executable
+    # from the remote target.
+    #
+    # As a result we expect to have seen an executable changed event.
+    with_test_prefix "after connecting" {
+	gdb_test "python print(global_exec_changed_event)" \
+	    "<gdb.ExecutableChangedEvent object at $hex>"
+	gdb_test "python print(global_exec_changed_event.progspace.executable_filename)" \
+	    [string_to_regexp target:$target_exec]
+	gdb_test "python print(global_exec_changed_event.reload)" "False"
+	gdb_test_no_output "python global_exec_changed_event = None"
+    }
+}
+
 # Issue a 'file' command and parse the output.  We look for a couple
 # of specific things to ensure that we are correctly reading the exec
 # from the remote target.
@@ -104,6 +133,20 @@ gdb_assert { $saw_read_of_remote_exec } \
 gdb_assert { $saw_read_of_syms_from_exec } \
     "symbols were read from remote exec file"
 
+if { [allow_python_tests] } {
+    # The 'file' command forces GDB to always load the executable,
+    # even if the same filename is used.  In this case, as the
+    # filename is the same, this will show as a reload event.
+    with_test_prefix "after 'file' command" {
+	gdb_test "python print(global_exec_changed_event)" \
+	    "<gdb.ExecutableChangedEvent object at $hex>"
+	gdb_test "python print(global_exec_changed_event.progspace.executable_filename)" \
+	    [string_to_regexp target:$target_exec]
+	gdb_test "python print(global_exec_changed_event.reload)" "True"
+	gdb_test_no_output "python global_exec_changed_event = None"
+    }
+}
+
 # Start the inferior (with the 'start' command), use TESTNAME for any
 # pass/fail calls.  EXPECT_REREAD should be true or false and
 # indicates if we expect to too a line like:
@@ -155,10 +198,24 @@ proc start_inferior { testname expect_reread } {
 # see the symbols re-read now.
 start_inferior "start inferior the first time" false
 
+if { [allow_python_tests] } {
+    # The executable hasn't changed.
+    with_test_prefix "after starting inferior for the first time" {
+	gdb_test "python print(global_exec_changed_event)" "None"
+    }
+}
+
 # Re-start the inferior.  The executable is unchanged so we should not
 # see the symbol file being re-read.
 start_inferior "start inferior a second time" false
 
+if { [allow_python_tests] } {
+    # The executable still hasn't changed.
+    with_test_prefix "after starting inferior for the second time" {
+	gdb_test "python print(global_exec_changed_event)" "None"
+    }
+}
+
 # Delay for a short while so, when we touch the exec, we know the
 # timestamp will change.
 sleep 1
@@ -172,3 +229,16 @@ if { $status != 0 } {
 # Start the inferior again, we expect to see the symbols being re-read
 # from the remote file.
 start_inferior "start inferior a third time" true
+
+if { [allow_python_tests] } {
+    # The executable has now changed on disk.  This will be a reload
+    # event.
+    with_test_prefix "after starting inferior for the third time" {
+	gdb_test "python print(global_exec_changed_event)" \
+	    "<gdb.ExecutableChangedEvent object at $hex>"
+	gdb_test "python print(global_exec_changed_event.progspace.executable_filename)" \
+	    [string_to_regexp target:$target_exec]
+	gdb_test "python print(global_exec_changed_event.reload)" "True"
+	gdb_test_no_output "python global_exec_changed_event = None"
+    }
+}
-- 
2.25.4

Re: [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c

[Lancelot SIX]

On Wed, Oct 25, 2023 at 04:08:17PM +0100, Andrew Burgess wrote:
> 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 replace 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.
> 
> Nicely, there are existing observers for both of these
> cases (before_prompt and target_resumed respectively), so, in
> gdb_bfd.c I've hooked into both of these and arranged to clear the bfd
> cache.
> 
> With this commit in place the next commit no longer shows any issues
> with gdb.base/attach.exp.
> 
> One possible problem that I see with this commit is: what if some
> other user of one of the observers I've hooked into causes a bfd to be
> opened after my new observers have run?  If this happened then we
> would proceed with a populated bfd cache, and this might causes
> problems.
> 
> Right now, the only other users of the observers that I'm connecting
> too are the extension languages, specifically, Python.  I suspect it
> must be possible for Python to carry out actions that can cause the
> bfd cache to be populated, so maybe there is a risk here.
> 
> To counter this risk, we could move the bfd_cache_close_all() calls
> out of observers, and move them into GDB core close too, but after the
> two observers in questions are actually called, so into
> top_level_prompt for the before_prompt observer and into
> notify_target_resumed for the target_resumed observer.
> 
> Another choice would be to add a bfd_cache_close_all() into
> gdbpy_enter::~gdbpy_enter, so whenever we call into a Python
> extension, we always clear the bfd cache once we're done.
> 
> For now I haven't made either of these changes, maybe I'm worrying
> about nothing?  I'd appreciate peoples thoughts.

Hi Andrew,

I think there are a couple of places in GDB where observers are not
notified directly, but via a notify_ helper function.  Those functions
ensure that some step are take before/after the observers listeners
execute.

Instances of this are notify_about_to_proceed, notify_about_to_proceed,
notify_normal_stop or notify_user_selected_context_changed (in
infrunc.c).

For this problem, could you replace
"gdb::observers::target_resumed.notify (ptid);" and
"gdb::observers::before_prompt.notify (get_prompt ().c_str ())" calls
with such helper function?  This way, you could ensure that listeners
are all executed before the "bfd_cache_close_all ()" call.

This approach does require that future change do not insert new direct
notify calls to the observers, but at least should solve the problem of
observers being notified in an arbitrary order.

Best,
Lancelot.

> 
> This commit also removes all of the other bfd_cache_close_all() calls
> from GDB.  My claim is that these are no longer needed.
> ---
>  gdb/corefile.c |  5 -----
>  gdb/exec.c     |  2 --
>  gdb/gdb_bfd.c  | 20 ++++++++++++++++++++
>  gdb/infcmd.c   |  1 -
>  gdb/inferior.c |  2 --
>  gdb/symfile.c  |  1 -
>  gdb/target.c   |  5 -----
>  7 files changed, 20 insertions(+), 16 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/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/gdb_bfd.c b/gdb/gdb_bfd.c
> index 56a4c5ecc91..9b7a06da231 100644
> --- a/gdb/gdb_bfd.c
> +++ b/gdb/gdb_bfd.c
> @@ -33,6 +33,7 @@
>  #include "gdbsupport/fileio.h"
>  #include "inferior.h"
>  #include "cli/cli-style.h"
> +#include "observable.h"
>  #include <unordered_map>
>  
>  /* An object of this type is stored in the section's user data when
> @@ -1171,10 +1172,29 @@ gdb_bfd_error_handler (const char *fmt, va_list ap)
>    (*default_bfd_error_handler) (fmt, ap);
>  }
>  
> +/* A before_prompt observer.  */
> +
> +static void
> +gdb_bfd_before_prompt (const char * /* ignored */)
> +{
> +  bfd_cache_close_all ();
> +}
> +
> +/* A target_resumed observer.  */
> +
> +static void
> +gdb_bfd_target_resumed (ptid_t /* ignored */)
> +{
> +  bfd_cache_close_all ();
> +}
> +
>  void _initialize_gdb_bfd ();
>  void
>  _initialize_gdb_bfd ()
>  {
> +  gdb::observers::target_resumed.attach (gdb_bfd_target_resumed, "gdb-bfd");
> +  gdb::observers::before_prompt.attach (gdb_bfd_before_prompt, "gdb-bfd");
> +
>    all_bfds = htab_create_alloc (10, htab_hash_pointer, htab_eq_pointer,
>  				NULL, xcalloc, xfree);
>  
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index cf8cd527955..5153843dde8 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -2498,7 +2498,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 eebc5ea44b9..24570372316 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 f688ff33e3b..aeb53dd91d0 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -2729,11 +2729,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
> -- 
> 2.25.4
> 

Re: [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c

[Andrew Burgess]

Lancelot SIX <lsix@lancelotsix.com> writes:

> On Wed, Oct 25, 2023 at 04:08:17PM +0100, Andrew Burgess wrote:
>> 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 replace 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.
>> 
>> Nicely, there are existing observers for both of these
>> cases (before_prompt and target_resumed respectively), so, in
>> gdb_bfd.c I've hooked into both of these and arranged to clear the bfd
>> cache.
>> 
>> With this commit in place the next commit no longer shows any issues
>> with gdb.base/attach.exp.
>> 
>> One possible problem that I see with this commit is: what if some
>> other user of one of the observers I've hooked into causes a bfd to be
>> opened after my new observers have run?  If this happened then we
>> would proceed with a populated bfd cache, and this might causes
>> problems.
>> 
>> Right now, the only other users of the observers that I'm connecting
>> too are the extension languages, specifically, Python.  I suspect it
>> must be possible for Python to carry out actions that can cause the
>> bfd cache to be populated, so maybe there is a risk here.
>> 
>> To counter this risk, we could move the bfd_cache_close_all() calls
>> out of observers, and move them into GDB core close too, but after the
>> two observers in questions are actually called, so into
>> top_level_prompt for the before_prompt observer and into
>> notify_target_resumed for the target_resumed observer.
>> 
>> Another choice would be to add a bfd_cache_close_all() into
>> gdbpy_enter::~gdbpy_enter, so whenever we call into a Python
>> extension, we always clear the bfd cache once we're done.
>> 
>> For now I haven't made either of these changes, maybe I'm worrying
>> about nothing?  I'd appreciate peoples thoughts.
>
> Hi Andrew,
>
> I think there are a couple of places in GDB where observers are not
> notified directly, but via a notify_ helper function.  Those functions
> ensure that some step are take before/after the observers listeners
> execute.
>
> Instances of this are notify_about_to_proceed, notify_about_to_proceed,
> notify_normal_stop or notify_user_selected_context_changed (in
> infrunc.c).
>
> For this problem, could you replace
> "gdb::observers::target_resumed.notify (ptid);" and
> "gdb::observers::before_prompt.notify (get_prompt ().c_str ())" calls
> with such helper function?  This way, you could ensure that listeners
> are all executed before the "bfd_cache_close_all ()" call.
>
> This approach does require that future change do not insert new direct
> notify calls to the observers, but at least should solve the problem of
> observers being notified in an arbitrary order.

Thanks.  I think this is probably the best approach.  I'll re-roll this
patch along these lines.

Thanks,
Andrew


>
> Best,
> Lancelot.
>
>> 
>> This commit also removes all of the other bfd_cache_close_all() calls
>> from GDB.  My claim is that these are no longer needed.
>> ---
>>  gdb/corefile.c |  5 -----
>>  gdb/exec.c     |  2 --
>>  gdb/gdb_bfd.c  | 20 ++++++++++++++++++++
>>  gdb/infcmd.c   |  1 -
>>  gdb/inferior.c |  2 --
>>  gdb/symfile.c  |  1 -
>>  gdb/target.c   |  5 -----
>>  7 files changed, 20 insertions(+), 16 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/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/gdb_bfd.c b/gdb/gdb_bfd.c
>> index 56a4c5ecc91..9b7a06da231 100644
>> --- a/gdb/gdb_bfd.c
>> +++ b/gdb/gdb_bfd.c
>> @@ -33,6 +33,7 @@
>>  #include "gdbsupport/fileio.h"
>>  #include "inferior.h"
>>  #include "cli/cli-style.h"
>> +#include "observable.h"
>>  #include <unordered_map>
>>  
>>  /* An object of this type is stored in the section's user data when
>> @@ -1171,10 +1172,29 @@ gdb_bfd_error_handler (const char *fmt, va_list ap)
>>    (*default_bfd_error_handler) (fmt, ap);
>>  }
>>  
>> +/* A before_prompt observer.  */
>> +
>> +static void
>> +gdb_bfd_before_prompt (const char * /* ignored */)
>> +{
>> +  bfd_cache_close_all ();
>> +}
>> +
>> +/* A target_resumed observer.  */
>> +
>> +static void
>> +gdb_bfd_target_resumed (ptid_t /* ignored */)
>> +{
>> +  bfd_cache_close_all ();
>> +}
>> +
>>  void _initialize_gdb_bfd ();
>>  void
>>  _initialize_gdb_bfd ()
>>  {
>> +  gdb::observers::target_resumed.attach (gdb_bfd_target_resumed, "gdb-bfd");
>> +  gdb::observers::before_prompt.attach (gdb_bfd_before_prompt, "gdb-bfd");
>> +
>>    all_bfds = htab_create_alloc (10, htab_hash_pointer, htab_eq_pointer,
>>  				NULL, xcalloc, xfree);
>>  
>> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
>> index cf8cd527955..5153843dde8 100644
>> --- a/gdb/infcmd.c
>> +++ b/gdb/infcmd.c
>> @@ -2498,7 +2498,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 eebc5ea44b9..24570372316 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 f688ff33e3b..aeb53dd91d0 100644
>> --- a/gdb/target.c
>> +++ b/gdb/target.c
>> @@ -2729,11 +2729,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
>> -- 
>> 2.25.4
>> 

Re: [PATCH 2/2] gdb: fix reopen_exec_file for files with target: prefix

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> @@ -105,21 +105,24 @@ specify_exec_file_hook (void (*hook) (const char *))
Andrew>  void
Andrew>  reopen_exec_file (void)
Andrew>  {
Andrew> -  int res;
Andrew> -  struct stat st;
Andrew> -
Andrew>    /* Don't do anything if there isn't an exec file.  */
Andrew>    if (current_program_space->exec_bfd () == NULL)
Andrew>      return;
 
Andrew> +  bfd *exec_bfd = current_program_space->exec_bfd ();

IMO you might as well hoist this a few lines up and use the local in
the NULL check as well.

Otherwise I think this looks good.  Thank you for doing this.

Tom

Re: [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> Better I think would be to track down where the bfd is being opened
Andrew> and add a corresponding bfd_cache_close_all() call elsewhere in GDB
Andrew> once we've finished doing whatever it is that caused us to open the
Andrew> bfd in the first place.

I had contemplated this solution as well, though using bfd_cache_close
on the specific BFD, instead.

Like you point out, though, it seems finicky to get right.  We could
move up the call chain and add the close calls at some higher level, but
even that seems hard to be assured of.

Andrew>   (a) Just before we display a GDB prompt.  We display a prompt after
Andrew>   completing a command, and GDB is about to enter an idle state
Andrew>   waiting for further input from the user (or in async mode, for an
Andrew>   inferior event).  If while we are in this idle state the user
Andrew>   changes the on-disk file(s) then we would like GDB to notice this
Andrew>   the next time it leaves its idle state, e.g. the next time the user
Andrew>   executes a command, or when an inferior event arrives,

Andrew>   (b) When we resume the inferior.  In synchronous mode, resuming the
Andrew>   inferior is another time when GDB is blocked and sitting idle, but
Andrew>   in this case we don't display a prompt.  As with (a) above, when an
Andrew>   inferior event arrives we want GDB to notice any changes to on-disk
Andrew>   files.

I've been working on moving DWARF reading to the background.  For the
most part, your plan won't be an issue -- gdb pre-reads the relevant
section data on the main thread and doesn't require the BFD to be open
when scanning.

However, there is one case where this will change: opening dwo files.
Now, the way the background reading is implemented, the worst case here
is some inefficiency: the main thread might close all the BFDs, and then
the background reader might immediately reopen one.

Maybe this isn't something to worry about?  I could make the background
reader also call bfd_cache_close for the DWO BFDs, to ensure they are
closed.  (This only really matters for the "rebuild" case, not any sort
of exec case, because DWOs aren't really part of the inferior.)

Andrew> Right now, the only other users of the observers that I'm connecting
Andrew> too are the extension languages, specifically, Python.  I suspect it
Andrew> must be possible for Python to carry out actions that can cause the
Andrew> bfd cache to be populated, so maybe there is a risk here.

Andrew> To counter this risk, we could move the bfd_cache_close_all() calls
Andrew> out of observers, and move them into GDB core close too, but after the
Andrew> two observers in questions are actually called, so into
Andrew> top_level_prompt for the before_prompt observer and into
Andrew> notify_target_resumed for the target_resumed observer.

Andrew> Another choice would be to add a bfd_cache_close_all() into
Andrew> gdbpy_enter::~gdbpy_enter, so whenever we call into a Python
Andrew> extension, we always clear the bfd cache once we're done.

Andrew> For now I haven't made either of these changes, maybe I'm worrying
Andrew> about nothing?  I'd appreciate peoples thoughts.

Do we know of specific Python APIs that could cause problems?

I guess anything that reads memory if trust-readonly-sections is
enabled?  That's the only one I could think of offhand.

Maybe we need a combo approach where some calls call bfd_cache_close at
the end.

Tom

Re: [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> Better I think would be to track down where the bfd is being opened
> Andrew> and add a corresponding bfd_cache_close_all() call elsewhere in GDB
> Andrew> once we've finished doing whatever it is that caused us to open the
> Andrew> bfd in the first place.
>
> I had contemplated this solution as well, though using bfd_cache_close
> on the specific BFD, instead.
>
> Like you point out, though, it seems finicky to get right.  We could
> move up the call chain and add the close calls at some higher level, but
> even that seems hard to be assured of.
>
> Andrew>   (a) Just before we display a GDB prompt.  We display a prompt after
> Andrew>   completing a command, and GDB is about to enter an idle state
> Andrew>   waiting for further input from the user (or in async mode, for an
> Andrew>   inferior event).  If while we are in this idle state the user
> Andrew>   changes the on-disk file(s) then we would like GDB to notice this
> Andrew>   the next time it leaves its idle state, e.g. the next time the user
> Andrew>   executes a command, or when an inferior event arrives,
>
> Andrew>   (b) When we resume the inferior.  In synchronous mode, resuming the
> Andrew>   inferior is another time when GDB is blocked and sitting idle, but
> Andrew>   in this case we don't display a prompt.  As with (a) above, when an
> Andrew>   inferior event arrives we want GDB to notice any changes to on-disk
> Andrew>   files.
>
> I've been working on moving DWARF reading to the background.  For the
> most part, your plan won't be an issue -- gdb pre-reads the relevant
> section data on the main thread and doesn't require the BFD to be open
> when scanning.

I did remember you mentioning your background debug parsing work, but I
couldn't find any emails about it, but maybe we talked in IRC...

> However, there is one case where this will change: opening dwo files.
> Now, the way the background reading is implemented, the worst case here
> is some inefficiency: the main thread might close all the BFDs, and then
> the background reader might immediately reopen one.

Right now I don't see bfd/cache.c as thread safe, so are you planning to
make it thread safe?  Or place restrictions within GDB to prevent
multiple threads accessing BFDs at the same time (e.g. background
reading can only happen when GDB itself is known to be idle)?

> Maybe this isn't something to worry about?  I could make the background
> reader also call bfd_cache_close for the DWO BFDs, to ensure they are
> closed.  (This only really matters for the "rebuild" case, not any sort
> of exec case, because DWOs aren't really part of the inferior.)

I figured all I'm really doing is moving the bfd_cache_close_all()
calls, we already had them randomly scattered throughout GDB anyway, so
any background reader you implement would already need to deal with the
cache being cleared under its feet (or prevent that from happening when
its running), so I didn't think I'd made things harder for you ... just
moved the problem around a bit.

Also, I figured it's not that hard to implement a
gdb_bfd_cache_close_all() wrapper which counts the number of threads
that are actively using the cache, and only calls bfd_cache_close_all()
if the "other" thread isn't currently running.  Of course, that assumes
that the cache itself is somehow thread-safe.

Another option is to only perform background reading while GDB is
in-active, in which case, my current bfd_cache_close_all() calls are
actually the points at which your backgound threads can safely start
working without worrying about GDB also accessing the bfd cache --
though, you'd still need a mechanism to stop the background reader once
GDB becomes active again.

The other option I considered, is that you could add a mechanism to
allow BFDs to be opened without going through the BFD cache.  In this
case, I think your thread safety issues go away, and also, you no longer
care about my bfd_cache_close_all() calls.  The BFD cache exists to
handle systems that only allow a small number of open files (they quote
20 as being a limit on some systems), so avoiding the cache might run
into an open FD limit issue ... but I'm not sure if this low limit
problem is really a thing on systems that are going to be running
multiple threads?

>
> Andrew> Right now, the only other users of the observers that I'm connecting
> Andrew> too are the extension languages, specifically, Python.  I suspect it
> Andrew> must be possible for Python to carry out actions that can cause the
> Andrew> bfd cache to be populated, so maybe there is a risk here.
>
> Andrew> To counter this risk, we could move the bfd_cache_close_all() calls
> Andrew> out of observers, and move them into GDB core close too, but after the
> Andrew> two observers in questions are actually called, so into
> Andrew> top_level_prompt for the before_prompt observer and into
> Andrew> notify_target_resumed for the target_resumed observer.
>
> Andrew> Another choice would be to add a bfd_cache_close_all() into
> Andrew> gdbpy_enter::~gdbpy_enter, so whenever we call into a Python
> Andrew> extension, we always clear the bfd cache once we're done.
>
> Andrew> For now I haven't made either of these changes, maybe I'm worrying
> Andrew> about nothing?  I'd appreciate peoples thoughts.
>
> Do we know of specific Python APIs that could cause problems?

Well the generic (maybe unhelpful) answer is gdb.execute().

>
> I guess anything that reads memory if trust-readonly-sections is
> enabled?  That's the only one I could think of offhand.
>
> Maybe we need a combo approach where some calls call bfd_cache_close at
> the end.

I'd really prefer to avoid relying on folk to remember that they need to
call bfd_cache_close_all() in certain places.  I think its far too easy
to forget, and these are bugs that are only going to crop up once in a
while .... A user is running a particular Python extension, and performs
a particular action, just at the same time as they recompile their test
binary ... and suddenly GDB "misses" that the test executable changed.
Bugs like this can be a nightmare to track down.

Right now, I'm thinking I'll switch away from using the observers, and
just ensure that bfd_cache_close_all() is called after all the observers
have run.

Thanks,
Andrew

[PATCH 0/2] BFD cache management And Exec file with target: prefix

[Andrew Burgess]

This started with patch #2, which improves GDB's handling of the main
executable having a target: prefix, but that ran into an issue with
the bfd cache.

So patch #1 aims to address this cache issue.

Changes in V2:

  - No longer using observers to call bfd_cache_close_all() (see V1
    discussion for reasons), instead I'm calling directly from core
    GDB after the before_prompt and target_resumed observers have been
    notified.

---

Andrew Burgess (2):
  gdb: move all bfd_cache_close_all calls in gdb_bfd.c
  gdb: fix reopen_exec_file for files with target: prefix

 gdb/corefile.c                                | 22 +++---
 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/testsuite/gdb.server/target-exec-file.exp | 70 +++++++++++++++++++
 gdb/thread.c                                  |  5 ++
 9 files changed, 102 insertions(+), 26 deletions(-)


base-commit: 2029e13917d53d2289d3ebb390c4f40bd2112d21
-- 
2.25.4

[PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c

[Andrew Burgess]

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.
---
 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 3d6fa896a9c..b8378f4e653 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 cf8cd527955..5153843dde8 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2498,7 +2498,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 eebc5ea44b9..24570372316 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 f688ff33e3b..aeb53dd91d0 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2729,11 +2729,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 c8145da59bc..a1e003b082e 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -876,6 +876,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.  */
-- 
2.25.4

[PATCH 2/2] gdb: fix reopen_exec_file for files with target: prefix

[Andrew Burgess]

Following on from this commit:

  commit f2c4f78c813a9cef38b7e9c9ad18822fb9e19345
  Date:   Thu Sep 21 16:35:30 2023 +0100

      gdb: fix reread_symbols when an objfile has target: prefix

In this commit I update reopen_exec_file to correctly handle
executables with a target: prefix.  Before this commit we used the
system 'stat' call, which obviously isn't going to work for files with
a target: prefix (files located on a possibly remote target machine).

By switching to bfd_stat we will use remote fileio to stat the remote
files, which means we should now correctly detect changes in a remote
executable.

The program_space::ebfd_mtime variable, with which we compare the
result of bfd_stat is set with a call to bfd_get_mtime, which in turn
calls bfd_stat, so comparing to the result of calling bfd_stat makes
sense (I think).

As I discussed in the commit f2c4f78c813a, if a BFD is an in-memory
BFD, then calling bfd_stat will always return 0, while bfd_get_mtime
will always return the time at which the BFD was created.  As a result
comparing the results will always show the file having changed.

I don't believe that GDB can set the main executable to an in-memory
BFD object, so, in this commit, I simply assert that the executable is
not in-memory.  If this ever changes then we would need to decide how
to handle this case -- always reload, or never reload.  The assert
doesn't appear to trigger for our current test suite.
---
 gdb/corefile.c                                | 17 +++--
 gdb/testsuite/gdb.server/target-exec-file.exp | 70 +++++++++++++++++++
 2 files changed, 80 insertions(+), 7 deletions(-)

diff --git a/gdb/corefile.c b/gdb/corefile.c
index 19a96bc6f86..b9c204d18dc 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -105,21 +105,24 @@ specify_exec_file_hook (void (*hook) (const char *))
 void
 reopen_exec_file (void)
 {
-  int res;
-  struct stat st;
+  bfd *exec_bfd = current_program_space->exec_bfd ();
 
   /* Don't do anything if there isn't an exec file.  */
-  if (current_program_space->exec_bfd () == NULL)
+  if (exec_bfd == nullptr)
     return;
 
+  /* The main executable can't be an in-memory BFD object.  If it was then
+     the use of bfd_stat below would not work as expected.  */
+  gdb_assert ((exec_bfd->flags & BFD_IN_MEMORY) == 0);
+
   /* If the timestamp of the exec file has changed, reopen it.  */
-  std::string filename = bfd_get_filename (current_program_space->exec_bfd ());
-  res = stat (filename.c_str (), &st);
+  struct stat st;
+  int res = bfd_stat (exec_bfd, &st);
 
   if (res == 0
-      && current_program_space->ebfd_mtime
+      && current_program_space->ebfd_mtime != 0
       && current_program_space->ebfd_mtime != st.st_mtime)
-    exec_file_attach (filename.c_str (), 0);
+    exec_file_attach (bfd_get_filename (exec_bfd), 0);
 }
 \f
 /* If we have both a core file and an exec file,
diff --git a/gdb/testsuite/gdb.server/target-exec-file.exp b/gdb/testsuite/gdb.server/target-exec-file.exp
index 9260df8b88d..40863538785 100644
--- a/gdb/testsuite/gdb.server/target-exec-file.exp
+++ b/gdb/testsuite/gdb.server/target-exec-file.exp
@@ -52,6 +52,19 @@ set target_exec [gdb_remote_download target $binfile]
 # prompt us if this is the right thing to do.
 gdb_test_no_output "set confirm off"
 
+if { [allow_python_tests] } {
+    # Register an event handler for the executable changed event.
+    # This handler just copies the event into a global Python object.
+    gdb_test_multiline "Add connection_removed event" \
+	"python" "" \
+	"global_exec_changed_event = None" "" \
+	"def executable_changed(event):" "" \
+	"   global global_exec_changed_event" "" \
+	"   global_exec_changed_event = event" "" \
+	"gdb.events.executable_changed.connect (executable_changed)" "" \
+	"end" ""
+}
+
 # Start gdbserver, but always in extended-remote mode, and then
 # connect to it from GDB.
 set res [gdbserver_start "--multi" $target_exec]
@@ -59,6 +72,22 @@ set gdbserver_protocol "extended-remote"
 set gdbserver_gdbport [lindex $res 1]
 gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport
 
+if { [allow_python_tests] } {
+    # When connecting to a remote target, if the user has not told GDB
+    # which executable to use, then GDB will figure out an executable
+    # from the remote target.
+    #
+    # As a result we expect to have seen an executable changed event.
+    with_test_prefix "after connecting" {
+	gdb_test "python print(global_exec_changed_event)" \
+	    "<gdb.ExecutableChangedEvent object at $hex>"
+	gdb_test "python print(global_exec_changed_event.progspace.executable_filename)" \
+	    [string_to_regexp target:$target_exec]
+	gdb_test "python print(global_exec_changed_event.reload)" "False"
+	gdb_test_no_output "python global_exec_changed_event = None"
+    }
+}
+
 # Issue a 'file' command and parse the output.  We look for a couple
 # of specific things to ensure that we are correctly reading the exec
 # from the remote target.
@@ -104,6 +133,20 @@ gdb_assert { $saw_read_of_remote_exec } \
 gdb_assert { $saw_read_of_syms_from_exec } \
     "symbols were read from remote exec file"
 
+if { [allow_python_tests] } {
+    # The 'file' command forces GDB to always load the executable,
+    # even if the same filename is used.  In this case, as the
+    # filename is the same, this will show as a reload event.
+    with_test_prefix "after 'file' command" {
+	gdb_test "python print(global_exec_changed_event)" \
+	    "<gdb.ExecutableChangedEvent object at $hex>"
+	gdb_test "python print(global_exec_changed_event.progspace.executable_filename)" \
+	    [string_to_regexp target:$target_exec]
+	gdb_test "python print(global_exec_changed_event.reload)" "True"
+	gdb_test_no_output "python global_exec_changed_event = None"
+    }
+}
+
 # Start the inferior (with the 'start' command), use TESTNAME for any
 # pass/fail calls.  EXPECT_REREAD should be true or false and
 # indicates if we expect to too a line like:
@@ -155,10 +198,24 @@ proc start_inferior { testname expect_reread } {
 # see the symbols re-read now.
 start_inferior "start inferior the first time" false
 
+if { [allow_python_tests] } {
+    # The executable hasn't changed.
+    with_test_prefix "after starting inferior for the first time" {
+	gdb_test "python print(global_exec_changed_event)" "None"
+    }
+}
+
 # Re-start the inferior.  The executable is unchanged so we should not
 # see the symbol file being re-read.
 start_inferior "start inferior a second time" false
 
+if { [allow_python_tests] } {
+    # The executable still hasn't changed.
+    with_test_prefix "after starting inferior for the second time" {
+	gdb_test "python print(global_exec_changed_event)" "None"
+    }
+}
+
 # Delay for a short while so, when we touch the exec, we know the
 # timestamp will change.
 sleep 1
@@ -172,3 +229,16 @@ if { $status != 0 } {
 # Start the inferior again, we expect to see the symbols being re-read
 # from the remote file.
 start_inferior "start inferior a third time" true
+
+if { [allow_python_tests] } {
+    # The executable has now changed on disk.  This will be a reload
+    # event.
+    with_test_prefix "after starting inferior for the third time" {
+	gdb_test "python print(global_exec_changed_event)" \
+	    "<gdb.ExecutableChangedEvent object at $hex>"
+	gdb_test "python print(global_exec_changed_event.progspace.executable_filename)" \
+	    [string_to_regexp target:$target_exec]
+	gdb_test "python print(global_exec_changed_event.reload)" "True"
+	gdb_test_no_output "python global_exec_changed_event = None"
+    }
+}
-- 
2.25.4

Re: [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c

[Tom Tromey]

>> However, there is one case where this will change: opening dwo files.
>> Now, the way the background reading is implemented, the worst case here
>> is some inefficiency: the main thread might close all the BFDs, and then
>> the background reader might immediately reopen one.

Andrew> Right now I don't see bfd/cache.c as thread safe, so are you planning to
Andrew> make it thread safe?

Yeah.  I sent a series to the binutils list recently.

Andrew> The other option I considered, is that you could add a mechanism to
Andrew> allow BFDs to be opened without going through the BFD cache.  In this
Andrew> case, I think your thread safety issues go away, and also, you no longer
Andrew> care about my bfd_cache_close_all() calls.  The BFD cache exists to
Andrew> handle systems that only allow a small number of open files (they quote
Andrew> 20 as being a limit on some systems), so avoiding the cache might run
Andrew> into an open FD limit issue ... but I'm not sure if this low limit
Andrew> problem is really a thing on systems that are going to be running
Andrew> multiple threads?

We could definitely do this for the files the DWARF reader opens in the
worker threads.

Not sure if I will do this or not though.  Adding an explicit
bfd_cache_close call would also take care of the problem, and I think
the reader only needs the file open from the call to open_dwo_file (in
open_and_init_dwo_file) through mapping the sections -- and they could
be mapped more eagerly in open_and_init_dwo_file.

Andrew> I'd really prefer to avoid relying on folk to remember that they need to
Andrew> call bfd_cache_close_all() in certain places.  I think its far too easy
Andrew> to forget, and these are bugs that are only going to crop up once in a
Andrew> while .... A user is running a particular Python extension, and performs
Andrew> a particular action, just at the same time as they recompile their test
Andrew> binary ... and suddenly GDB "misses" that the test executable changed.
Andrew> Bugs like this can be a nightmare to track down.

Yeah.

Tom

Re: [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>> However, there is one case where this will change: opening dwo files.
>>> Now, the way the background reading is implemented, the worst case here
>>> is some inefficiency: the main thread might close all the BFDs, and then
>>> the background reader might immediately reopen one.
>
> Andrew> Right now I don't see bfd/cache.c as thread safe, so are you planning to
> Andrew> make it thread safe?
>
> Yeah.  I sent a series to the binutils list recently.

Neat.  It sounds like your background DWARF reader is further along than
I'd thought.

I'm trying to figure out what I should do to move this patch forward?

I guess the existing bfd_cache_close_all() calls could cause problems
for your background reader if the on-disk file changes between two reads
from a BFD, and if GDB's main thread calls bfd_cache_close_all() in
between those reads, e.g. one BFD section might indicate an offset into
another section, and after the on-disk file changes the section offset
is no longer valid...

But you could just ignore the existing bfd_cache_close_all() calls, and
rely on BFD to reopen the files as needed, and rely on GDB's error
detection to handle any race conditions caused by the on-disk file
changing...

I'm happy to do whatever is needed to integrate this patch with your
upcoming work, I'm just trying to figure out what work needs to be done:
if you're currently ignoring bfd_cache_close_all() calls, and relying on
BFD to reopen, and GDB's error detection, then I guess this patch is
fine as is.  But if you have some other solution for the
bfd_cache_close_all() calls, then I guess this patch is stepping on your
toes, in which case, I want to help combine these two work items if I
can.

>
> Andrew> The other option I considered, is that you could add a mechanism to
> Andrew> allow BFDs to be opened without going through the BFD cache.  In this
> Andrew> case, I think your thread safety issues go away, and also, you no longer
> Andrew> care about my bfd_cache_close_all() calls.  The BFD cache exists to
> Andrew> handle systems that only allow a small number of open files (they quote
> Andrew> 20 as being a limit on some systems), so avoiding the cache might run
> Andrew> into an open FD limit issue ... but I'm not sure if this low limit
> Andrew> problem is really a thing on systems that are going to be running
> Andrew> multiple threads?
>
> We could definitely do this for the files the DWARF reader opens in the
> worker threads.
>
> Not sure if I will do this or not though.  Adding an explicit
> bfd_cache_close call would also take care of the problem, and I think
> the reader only needs the file open from the call to open_dwo_file (in
> open_and_init_dwo_file) through mapping the sections -- and they could
> be mapped more eagerly in open_and_init_dwo_file.

It sounds like you're already heading towards making the bfd cache
thread safe, so I don't think these ideas would be needed.

Thanks,
Andrew

>
> Andrew> I'd really prefer to avoid relying on folk to remember that they need to
> Andrew> call bfd_cache_close_all() in certain places.  I think its far too easy
> Andrew> to forget, and these are bugs that are only going to crop up once in a
> Andrew> while .... A user is running a particular Python extension, and performs
> Andrew> a particular action, just at the same time as they recompile their test
> Andrew> binary ... and suddenly GDB "misses" that the test executable changed.
> Andrew> Bugs like this can be a nightmare to track down.
>
> Yeah.
>
> Tom

Re: [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> I'm trying to figure out what I should do to move this patch
Andrew> forward?

I think you should probably just check it in.

Andrew> I guess the existing bfd_cache_close_all() calls could cause problems
Andrew> for your background reader if the on-disk file changes between two reads
Andrew> from a BFD, and if GDB's main thread calls bfd_cache_close_all() in
Andrew> between those reads, e.g. one BFD section might indicate an offset into
Andrew> another section, and after the on-disk file changes the section offset
Andrew> is no longer valid...

gdb already isn't really robust if the file changes while gdb is
running, at least not when trust-readonly-sections is enabled.

However, the background reader maps most section data in the main
thread, before launching the indexer tasks.  So, I guess maybe it could
happen, but if you are rebuilding your program while invoking "file"
you're already exposed to races.

Tom

Re: [PATCH 0/2] BFD cache management And Exec file with target: prefix

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> This started with patch #2, which improves GDB's handling of the main
Andrew> executable having a target: prefix, but that ran into an issue with
Andrew> the bfd cache.

Andrew> So patch #1 aims to address this cache issue.

Andrew> Changes in V2:

Andrew>   - No longer using observers to call bfd_cache_close_all() (see V1
Andrew>     discussion for reasons), instead I'm calling directly from core
Andrew>     GDB after the before_prompt and target_resumed observers have been
Andrew>     notified.

This looks good to me.
Approved-By: Tom Tromey <tom@tromey.com>

Tom