* [PATCH 0/2] BFD cache management And Exec file with target: prefix @ 2023-10-25 15:08 Andrew Burgess 2023-10-25 15:08 ` [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c Andrew Burgess ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Andrew Burgess @ 2023-10-25 15:08 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c 2023-10-25 15:08 [PATCH 0/2] BFD cache management And Exec file with target: prefix Andrew Burgess @ 2023-10-25 15:08 ` Andrew Burgess 2023-10-26 9:06 ` Lancelot SIX 2023-10-27 19:30 ` Tom Tromey 2023-10-25 15:08 ` [PATCH 2/2] gdb: fix reopen_exec_file for files with target: prefix Andrew Burgess 2023-10-30 13:41 ` [PATCH 0/2] BFD cache management And Exec file " Andrew Burgess 2 siblings, 2 replies; 15+ messages in thread From: Andrew Burgess @ 2023-10-25 15:08 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c 2023-10-25 15:08 ` [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c Andrew Burgess @ 2023-10-26 9:06 ` Lancelot SIX 2023-10-27 8:49 ` Andrew Burgess 2023-10-27 19:30 ` Tom Tromey 1 sibling, 1 reply; 15+ messages in thread From: Lancelot SIX @ 2023-10-26 9:06 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c 2023-10-26 9:06 ` Lancelot SIX @ 2023-10-27 8:49 ` Andrew Burgess 0 siblings, 0 replies; 15+ messages in thread From: Andrew Burgess @ 2023-10-27 8:49 UTC (permalink / raw) To: Lancelot SIX; +Cc: gdb-patches 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 >> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c 2023-10-25 15:08 ` [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c Andrew Burgess 2023-10-26 9:06 ` Lancelot SIX @ 2023-10-27 19:30 ` Tom Tromey 2023-10-30 10:20 ` Andrew Burgess 1 sibling, 1 reply; 15+ messages in thread From: Tom Tromey @ 2023-10-27 19:30 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c 2023-10-27 19:30 ` Tom Tromey @ 2023-10-30 10:20 ` Andrew Burgess 2023-10-31 18:28 ` Tom Tromey 0 siblings, 1 reply; 15+ messages in thread From: Andrew Burgess @ 2023-10-30 10:20 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c 2023-10-30 10:20 ` Andrew Burgess @ 2023-10-31 18:28 ` Tom Tromey 2023-11-01 10:46 ` Andrew Burgess 0 siblings, 1 reply; 15+ messages in thread From: Tom Tromey @ 2023-10-31 18:28 UTC (permalink / raw) To: Andrew Burgess; +Cc: Tom Tromey, gdb-patches >> 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c 2023-10-31 18:28 ` Tom Tromey @ 2023-11-01 10:46 ` Andrew Burgess 2023-11-12 23:38 ` Tom Tromey 0 siblings, 1 reply; 15+ messages in thread From: Andrew Burgess @ 2023-11-01 10:46 UTC (permalink / raw) To: Tom Tromey; +Cc: Tom Tromey, gdb-patches 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c 2023-11-01 10:46 ` Andrew Burgess @ 2023-11-12 23:38 ` Tom Tromey 0 siblings, 0 replies; 15+ messages in thread From: Tom Tromey @ 2023-11-12 23:38 UTC (permalink / raw) To: Andrew Burgess; +Cc: Tom Tromey, gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2] gdb: fix reopen_exec_file for files with target: prefix 2023-10-25 15:08 [PATCH 0/2] BFD cache management And Exec file with target: prefix Andrew Burgess 2023-10-25 15:08 ` [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c Andrew Burgess @ 2023-10-25 15:08 ` Andrew Burgess 2023-10-27 18:39 ` Tom Tromey 2023-10-30 13:41 ` [PATCH 0/2] BFD cache management And Exec file " Andrew Burgess 2 siblings, 1 reply; 15+ messages in thread From: Andrew Burgess @ 2023-10-25 15:08 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] gdb: fix reopen_exec_file for files with target: prefix 2023-10-25 15:08 ` [PATCH 2/2] gdb: fix reopen_exec_file for files with target: prefix Andrew Burgess @ 2023-10-27 18:39 ` Tom Tromey 0 siblings, 0 replies; 15+ messages in thread From: Tom Tromey @ 2023-10-27 18:39 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 0/2] BFD cache management And Exec file with target: prefix 2023-10-25 15:08 [PATCH 0/2] BFD cache management And Exec file with target: prefix Andrew Burgess 2023-10-25 15:08 ` [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c Andrew Burgess 2023-10-25 15:08 ` [PATCH 2/2] gdb: fix reopen_exec_file for files with target: prefix Andrew Burgess @ 2023-10-30 13:41 ` Andrew Burgess 2023-10-30 13:41 ` [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c Andrew Burgess ` (2 more replies) 2 siblings, 3 replies; 15+ messages in thread From: Andrew Burgess @ 2023-10-30 13:41 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess, Lancelot SIX, Tom Tromey 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c 2023-10-30 13:41 ` [PATCH 0/2] BFD cache management And Exec file " Andrew Burgess @ 2023-10-30 13:41 ` Andrew Burgess 2023-10-30 13:41 ` [PATCH 2/2] gdb: fix reopen_exec_file for files with target: prefix Andrew Burgess 2023-11-12 23:40 ` [PATCH 0/2] BFD cache management And Exec file " Tom Tromey 2 siblings, 0 replies; 15+ messages in thread From: Andrew Burgess @ 2023-10-30 13:41 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess, Lancelot SIX, Tom Tromey 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2] gdb: fix reopen_exec_file for files with target: prefix 2023-10-30 13:41 ` [PATCH 0/2] BFD cache management And Exec file " Andrew Burgess 2023-10-30 13:41 ` [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c Andrew Burgess @ 2023-10-30 13:41 ` Andrew Burgess 2023-11-12 23:40 ` [PATCH 0/2] BFD cache management And Exec file " Tom Tromey 2 siblings, 0 replies; 15+ messages in thread From: Andrew Burgess @ 2023-10-30 13:41 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess, Lancelot SIX, Tom Tromey 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] BFD cache management And Exec file with target: prefix 2023-10-30 13:41 ` [PATCH 0/2] BFD cache management And Exec file " Andrew Burgess 2023-10-30 13:41 ` [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c Andrew Burgess 2023-10-30 13:41 ` [PATCH 2/2] gdb: fix reopen_exec_file for files with target: prefix Andrew Burgess @ 2023-11-12 23:40 ` Tom Tromey 2 siblings, 0 replies; 15+ messages in thread From: Tom Tromey @ 2023-11-12 23:40 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches, Lancelot SIX, 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2023-11-12 23:39 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-10-25 15:08 [PATCH 0/2] BFD cache management And Exec file with target: prefix Andrew Burgess 2023-10-25 15:08 ` [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c Andrew Burgess 2023-10-26 9:06 ` Lancelot SIX 2023-10-27 8:49 ` Andrew Burgess 2023-10-27 19:30 ` Tom Tromey 2023-10-30 10:20 ` Andrew Burgess 2023-10-31 18:28 ` Tom Tromey 2023-11-01 10:46 ` Andrew Burgess 2023-11-12 23:38 ` Tom Tromey 2023-10-25 15:08 ` [PATCH 2/2] gdb: fix reopen_exec_file for files with target: prefix Andrew Burgess 2023-10-27 18:39 ` Tom Tromey 2023-10-30 13:41 ` [PATCH 0/2] BFD cache management And Exec file " Andrew Burgess 2023-10-30 13:41 ` [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c Andrew Burgess 2023-10-30 13:41 ` [PATCH 2/2] gdb: fix reopen_exec_file for files with target: prefix Andrew Burgess 2023-11-12 23:40 ` [PATCH 0/2] BFD cache management And Exec file " Tom Tromey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).