[PATCH 0/9] Add executable_changed event to Python API

[PATCH 0/9] Add executable_changed event to Python API

[Andrew Burgess]

When I started, I thought this would be trivial; I wanted to expose
the executable_changed observer through the Python API.

Well, it turns out that the executable filename isn't directly visible
from the Python API, so having an event for when it changes doesn't
make a great deal of sense.  So the first 3 patches sort that out.

Then, once I started looking at how the executable_changed observer is
used, it feels like it is currently overloaded a little, and, as a
result, triggers more than it needs to.  The next 3 patches try to
sort this out.

Then patch 7 adds some additional arguments to the executable changed
observer.

Patch 8 is the trivial patch that exposes the observer as an event in
the Python API.

.... And patch 9 fixes a final bug/annoyance which I ran into while
writing the tests for patch 8.

---

Andrew Burgess (9):
  gdb/doc: extend the description for Progspace.filename
  gdb/python: new Progspace.symbol_file attribute
  gdb/python: new Progspace.executable_filename attribute
  gdb: remove one user of the executable changed observer
  gdb: remove final user of the executable_changed observer
  gdb: remove unnecessary notification of executable_changed observer
  gdb: pass more arguments to the executable_changed observer
  gdb/python: make the executable_changed event available from Python
  gdb: use reopen_exec_file from reread_symbols

 gdb/NEWS                                  |  16 ++
 gdb/auxv.c                                |  10 +-
 gdb/doc/python.texi                       |  71 +++++++-
 gdb/exec.c                                |  11 +-
 gdb/observable.h                          |  18 +-
 gdb/python/py-all-events.def              |   1 +
 gdb/python/py-event-types.def             |   5 +
 gdb/python/py-progspace.c                 | 100 ++++++++++-
 gdb/symfile.c                             |  22 +--
 gdb/symtab.c                              |  18 +-
 gdb/testsuite/gdb.python/py-exec-file.c   |  22 +++
 gdb/testsuite/gdb.python/py-exec-file.exp | 195 ++++++++++++++++++++++
 gdb/testsuite/gdb.python/py-progspace.exp |   9 +
 13 files changed, 458 insertions(+), 40 deletions(-)
 create mode 100644 gdb/testsuite/gdb.python/py-exec-file.c
 create mode 100644 gdb/testsuite/gdb.python/py-exec-file.exp


base-commit: 5a6dafd5f1110e8b642fdf65c13d8543e469a09e
-- 
2.25.4

[PATCH 1/9] gdb/doc: extend the description for Progspace.filename

[Andrew Burgess]

Extend the description for Progspace.filename in the documentation to
mention what the returned string is actually the filename
for (e.g. that it is the filename passed to the 'symbol-file' or
'file' command).

Also document that this attribute will be None if no symbol file is
currently loaded.
---
 gdb/doc/python.texi       | 7 ++++++-
 gdb/python/py-progspace.c | 2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 5b13958aeaf..ba9b0141e13 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -5034,7 +5034,12 @@
 class.
 
 @defvar Progspace.filename
-The file name of the progspace as a string.
+The file name, as a string, of the main symbol file (from which debug
+symbols have been loaded) for the progspace, e.g.@: the argument to
+the @kbd{symbol-file} or @kbd{file} commands.
+
+If there is no main symbol table currently loaded, then this attribute
+will be @code{None}.
 @end defvar
 
 @defvar Progspace.pretty_printers
diff --git a/gdb/python/py-progspace.c b/gdb/python/py-progspace.c
index b98ac8dde61..2b1d1605ca0 100644
--- a/gdb/python/py-progspace.c
+++ b/gdb/python/py-progspace.c
@@ -572,7 +572,7 @@ static gdb_PyGetSetDef pspace_getset[] =
   { "__dict__", gdb_py_generic_dict, NULL,
     "The __dict__ for this progspace.", &pspace_object_type },
   { "filename", pspy_get_filename, NULL,
-    "The progspace's main filename, or None.", NULL },
+    "The filename of the progspace's main symbol file, or None.", nullptr },
   { "pretty_printers", pspy_get_printers, pspy_set_printers,
     "Pretty printers.", NULL },
   { "frame_filters", pspy_get_frame_filters, pspy_set_frame_filters,
-- 
2.25.4

[PATCH 2/9] gdb/python: new Progspace.symbol_file attribute

[Andrew Burgess]

Add a new Progspace.symbol_file attribute.  This attribute holds the
gdb.Objfile object that corresponds to Progspace.filename, or None if
there is no main symbol file currently set.

Currently, to get this gdb.Objfile, a user would need to use
Progspace.objfiles, and then search for the objfile with a name that
matches Progspace.filename -- which should work just fine, but having
direct access seems a little nicer.
---
 gdb/NEWS                                  |  5 +++++
 gdb/doc/python.texi                       | 17 +++++++++++++++++
 gdb/python/py-progspace.c                 | 23 +++++++++++++++++++++++
 gdb/testsuite/gdb.python/py-progspace.exp |  9 +++++++++
 4 files changed, 54 insertions(+)

diff --git a/gdb/NEWS b/gdb/NEWS
index 98ff00d5efc..f2f5dabb287 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -286,6 +286,11 @@ info main
      might be array- or string-like, even if they do not have the
      corresponding type code.
 
+  ** New attribute Progspace.symbol_file.  This attribute holds the
+     gdb.Objfile that corresponds to Progspace.filename (when
+     Progspace.filename is not None), otherwise, this attribute is
+     itself None.
+
 *** Changes in GDB 13
 
 * MI version 1 is deprecated, and will be removed in GDB 14.
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index ba9b0141e13..03299cc3cf7 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -5042,6 +5042,23 @@
 will be @code{None}.
 @end defvar
 
+@defvar Progspace.symbol_file
+The @code{gdb.Objfile} representing the main symbol file (from which
+debug symbols have been loaded) for the @code{gdb.Progspace}.  This is
+the symbol file set by the @kbd{symbol-file} or @kbd{file} commands.
+
+This will be the @code{gdb.Objfile} representing
+@code{Progspace.filename} when @code{Progspace.filename} is not
+@code{None}.
+
+If there is no main symbol table currently loaded, then this attribute
+will be @code{None}.
+
+If the @code{Progspace} is invalid, i.e.@:, when
+@code{Progspace.is_valid()} returns @code{False}, then attempting to
+access this attribute will raise a @code{RuntimeError} exception.
+@end defvar
+
 @defvar Progspace.pretty_printers
 The @code{pretty_printers} attribute is a list of functions.  It is
 used to look up pretty-printers.  A @code{Value} is passed to each
diff --git a/gdb/python/py-progspace.c b/gdb/python/py-progspace.c
index 2b1d1605ca0..929c5f4fa70 100644
--- a/gdb/python/py-progspace.c
+++ b/gdb/python/py-progspace.c
@@ -111,6 +111,26 @@ pspy_get_filename (PyObject *self, void *closure)
   Py_RETURN_NONE;
 }
 
+/* Implement the gdb.Progspace.symbol_file attribute.  Retun the
+   gdb.Objfile corresponding to the currently loaded symbol-file, or None
+   if no symbol-file is loaded.  If the Progspace is invalid then raise an
+   exception.  */
+
+static PyObject *
+pspy_get_symbol_file (PyObject *self, void *closure)
+{
+  pspace_object *obj = (pspace_object *) self;
+
+  PSPY_REQUIRE_VALID (obj);
+
+  struct objfile *objfile = obj->pspace->symfile_object_file;
+
+  if (objfile != nullptr)
+    return objfile_to_objfile_object (objfile).release ();
+
+  Py_RETURN_NONE;
+}
+
 static void
 pspy_dealloc (PyObject *self)
 {
@@ -573,6 +593,9 @@ static gdb_PyGetSetDef pspace_getset[] =
     "The __dict__ for this progspace.", &pspace_object_type },
   { "filename", pspy_get_filename, NULL,
     "The filename of the progspace's main symbol file, or None.", nullptr },
+  { "symbol_file", pspy_get_symbol_file, nullptr,
+    "The gdb.Objfile for the progspace's main symbol file, or None.",
+    nullptr},
   { "pretty_printers", pspy_get_printers, pspy_set_printers,
     "Pretty printers.", NULL },
   { "frame_filters", pspy_get_frame_filters, pspy_set_frame_filters,
diff --git a/gdb/testsuite/gdb.python/py-progspace.exp b/gdb/testsuite/gdb.python/py-progspace.exp
index f0dc208ae4b..befd6433e47 100644
--- a/gdb/testsuite/gdb.python/py-progspace.exp
+++ b/gdb/testsuite/gdb.python/py-progspace.exp
@@ -30,6 +30,8 @@ clean_restart
 
 gdb_test "python print (gdb.current_progspace().filename)" "None" \
   "current progspace filename (None)"
+gdb_test "python print (gdb.current_progspace().symbol_file)" "None" \
+    "current progspace symbol_file is None"
 gdb_test "python print (gdb.progspaces())" "\\\[<gdb.Progspace object at $hex>\\\]"
 
 gdb_test_no_output "python dir(gdb.current_progspace())"
@@ -42,6 +44,10 @@ gdb_py_test_silent_cmd "python progspace = gdb.current_progspace()" \
 gdb_test "python print (progspace.filename)" "py-progspace" \
   "current progspace filename (py-progspace)"
 
+gdb_test "python print (gdb.current_progspace().symbol_file)" \
+    "<gdb.Objfile filename=.*/py-progspace>" \
+    "current progspace symbol_file is set correctly"
+
 gdb_py_test_silent_cmd "python progspace.random_attribute = 42" \
     "Set random attribute in progspace" 1
 gdb_test "python print (progspace.random_attribute)" "42" \
@@ -100,3 +106,6 @@ gdb_test "inferior 1" "Switching to inferior 1.*"
 gdb_test_no_output "remove-inferiors 2"
 gdb_test "python print (progspace2.objfiles ())" \
     "RuntimeError: Program space no longer exists.*"
+
+gdb_test "python print (progspace2.symbol_file)" \
+    "RuntimeError: Program space no longer exists.*"
-- 
2.25.4

[PATCH 3/9] gdb/python: new Progspace.executable_filename attribute

[Andrew Burgess]

Add a new Progspace.executable_filename attribute that contains the
path to the executable for this program space, or None if no
executable is set.

The path within this attribute will be set by the "exec-file" and/or
"file" commands.

Accessing this attribute for an invalid program space will raise an
exception.

This new attribute is similar too, but not the same as the existing
gdb.Progspace.filename attribute.  If I could change the past, I'd
change the 'filename' attribute to 'symbol_filename', which is what it
actually represents.  The old attribute will be set by the
'symbol-file' command, while the new attribute is set by the
'exec-file' command.  Obviously the 'file' command sets both of these
attributes.
---
 gdb/NEWS                                  |   6 ++
 gdb/doc/python.texi                       |  15 ++++
 gdb/python/py-progspace.c                 |  21 +++++
 gdb/testsuite/gdb.python/py-exec-file.c   |  22 +++++
 gdb/testsuite/gdb.python/py-exec-file.exp | 100 ++++++++++++++++++++++
 5 files changed, 164 insertions(+)
 create mode 100644 gdb/testsuite/gdb.python/py-exec-file.c
 create mode 100644 gdb/testsuite/gdb.python/py-exec-file.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index f2f5dabb287..93bc9c6a2c0 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -291,6 +291,12 @@ info main
      Progspace.filename is not None), otherwise, this attribute is
      itself None.
 
+  ** New attribute Progspace.executable_filename.  This attribute
+     holds a string containing a path set by the "exec-file" or "file"
+     commands, or None if no executable file is set.  This isn't the
+     exact string passed by the user to these commands; the path will
+     have been partially resolved to an absolute path.
+
 *** Changes in GDB 13
 
 * MI version 1 is deprecated, and will be removed in GDB 14.
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 03299cc3cf7..206cf6b4e18 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -5059,6 +5059,21 @@
 access this attribute will raise a @code{RuntimeError} exception.
 @end defvar
 
+@defvar Progspace.executable_filename
+The file name, as a string, of the executable file in use by this
+program space.  The executable file is the file that @value{GDBN} will
+invoke in order to start an inferior when using a native target.  The
+file name within this attribute is updated by the @kbd{exec-file} and
+@kbd{file} commands.
+
+If no executable is currently set within this @code{Progspace} then
+this attribute contains @code{None}.
+
+If the @code{Progspace} is invalid, i.e.@:, when
+@code{Progspace.is_valid()} returns @code{False}, then attempting to
+access this attribute will raise a @code{RuntimeError} exception.
+@end defvar
+
 @defvar Progspace.pretty_printers
 The @code{pretty_printers} attribute is a list of functions.  It is
 used to look up pretty-printers.  A @code{Value} is passed to each
diff --git a/gdb/python/py-progspace.c b/gdb/python/py-progspace.c
index 929c5f4fa70..1319978d34a 100644
--- a/gdb/python/py-progspace.c
+++ b/gdb/python/py-progspace.c
@@ -131,6 +131,25 @@ pspy_get_symbol_file (PyObject *self, void *closure)
   Py_RETURN_NONE;
 }
 
+/* Implement the gdb.Progspace.executable_filename attribute.  Retun a
+   string containing the name of the current executable, or None if no
+   executable is currently set.  If the Progspace is invalid then raise an
+   exception.  */
+
+static PyObject *
+pspy_get_exec_file (PyObject *self, void *closure)
+{
+  pspace_object *obj = (pspace_object *) self;
+
+  PSPY_REQUIRE_VALID (obj);
+
+  const char *filename = obj->pspace->exec_filename.get ();
+  if (filename != nullptr)
+    return host_string_to_python_string (filename).release ();
+
+  Py_RETURN_NONE;
+}
+
 static void
 pspy_dealloc (PyObject *self)
 {
@@ -596,6 +615,8 @@ static gdb_PyGetSetDef pspace_getset[] =
   { "symbol_file", pspy_get_symbol_file, nullptr,
     "The gdb.Objfile for the progspace's main symbol file, or None.",
     nullptr},
+  { "executable_filename", pspy_get_exec_file, nullptr,
+    "The filename for the progspace's executable, or None.", nullptr},
   { "pretty_printers", pspy_get_printers, pspy_set_printers,
     "Pretty printers.", NULL },
   { "frame_filters", pspy_get_frame_filters, pspy_set_frame_filters,
diff --git a/gdb/testsuite/gdb.python/py-exec-file.c b/gdb/testsuite/gdb.python/py-exec-file.c
new file mode 100644
index 00000000000..8cbea3b6892
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-exec-file.c
@@ -0,0 +1,22 @@
+/* This test program is part of GDB, the GNU debugger.
+
+   Copyright 2023 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+main ()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.python/py-exec-file.exp b/gdb/testsuite/gdb.python/py-exec-file.exp
new file mode 100644
index 00000000000..14e5088af1c
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-exec-file.exp
@@ -0,0 +1,100 @@
+# Copyright (C) 2023 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+require allow_python_tests
+
+load_lib gdb-python.exp
+
+standard_testfile
+
+set binfile1 ${binfile}-a
+set binfile2 ${binfile}-b
+
+if {[build_executable "failed to prepare first executable" \
+	 $binfile1 $srcfile]} {
+    return -1
+}
+
+if {[build_executable "failed to prepare second executable" \
+	 $binfile2 $srcfile]} {
+    return -1
+}
+
+set binfile1 [gdb_remote_download host $binfile1]
+set binfile2 [gdb_remote_download host $binfile2]
+
+# Check that the executable_filename is set correctly after using the
+# 'file' command.
+with_test_prefix "using 'file' command" {
+    clean_restart
+
+    gdb_test "python print(gdb.current_progspace().executable_filename)" \
+	"None" \
+	"check executable_filename when no file is loaded"
+
+    gdb_test "file $binfile1" \
+	"Reading symbols from [string_to_regexp $binfile1]\\.\\.\\..*" \
+	"load first executable"
+    gdb_test "python print(gdb.current_progspace().executable_filename)" \
+	"[string_to_regexp $binfile1]" \
+	"check executable_filename when first executable is loaded"
+
+    gdb_test "file $binfile2" \
+	"Reading symbols from [string_to_regexp $binfile2]\\.\\.\\..*" \
+	"load second executable" \
+	"Load new symbol table from .*\? .y or n. " "y"
+    gdb_test "python print(gdb.current_progspace().executable_filename)" \
+	"[string_to_regexp $binfile2]" \
+	"check executable_filename when second executable is loaded"
+
+    gdb_unload
+    gdb_test "python print(gdb.current_progspace().executable_filename)" \
+	"None" \
+	"check executable_filename after unloading file"
+}
+
+# Check that the executable_filename is correctly set when we only set
+# the exec-file.
+with_test_prefix "using 'exec-file' command" {
+    clean_restart
+    gdb_test_no_output "exec-file $binfile1" \
+	"load first executable"
+    gdb_test "python print(gdb.current_progspace().executable_filename)" \
+	"[string_to_regexp $binfile1]" \
+	"check executable_filename when first executable is loaded"
+
+    gdb_test_no_output "exec-file $binfile2" \
+	"load second executable"
+    gdb_test "python print(gdb.current_progspace().executable_filename)" \
+	"[string_to_regexp $binfile2]" \
+	"check executable_filename when second executable is loaded"
+
+    gdb_test "exec-file" "No executable file now\\."
+    gdb_test "python print(gdb.current_progspace().executable_filename)" \
+	"None" \
+	"check executable_filename after unloading file"
+}
+
+# Check that setting the symbol-file doesn't cause the
+# executable_filename to be set.
+with_test_prefix "using 'symbol-file' command" {
+    clean_restart
+    gdb_test "symbol-file $binfile1" \
+	"Reading symbols from [string_to_regexp $binfile1]\\.\\.\\..*" \
+	"load first executable"
+    gdb_test "python print(gdb.current_progspace().executable_filename)" \
+	"None" \
+	"check executable_filename after setting symbol-file"
+}
-- 
2.25.4

[PATCH 4/9] gdb: remove one user of the executable changed observer

[Andrew Burgess]

My goal for the next few commits is to expose the executable_changed
observable from the Python API.

However, there is call to the executable_changed observable in the
reread_symbols function (in symfile.c), and this doesn't actually
correspond to the executable changing.  My idea then, is to remove
this use of the executable_changed observable, but, before I can do
that, I need to check that nothing is going to break, and that
requires my to think about the current users of this observable.

One current user of executable_changed is in symtab.c.  We add an
executable_changed observer that calls:

  set_main_name (nullptr, language_unknown);

to discard all information about the main function when the executable
changes.

However, changing the executable doesn't actually change the debug
information.  The debug information changes when the symbol-file
changes, so I think this observer is in slightly the wrong place.

The new_objfile observable is (unfortunately) overloaded, it is called
when a new objfile is loaded, and also (when its argument is nullptr),
when all debug information should be discarded.

It turns out that there is already a new_objfile observer in
symtab.c.  I propose that, when the argument is nullptr (indicating
all debug info should be discarded), that we should call set_main_name
to discard the information about the main function.  We can then
remove the executable_changed observer from symtab.c.

All tests still pass, and, in my local world, I added some debug
printf calls, and I think we are still discarded the main information
everywhere we need to.
---
 gdb/symtab.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index c0c2454d967..2d446331ed1 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -100,6 +100,8 @@ static struct block_symbol
 			    enum block_enum block_index,
 			    const char *name, const domain_enum domain);
 
+static void set_main_name (const char *name, enum language lang);
+
 /* Type of the data stored on the program space.  */
 
 struct main_info
@@ -1694,6 +1696,11 @@ symtab_new_objfile_observer (struct objfile *objfile)
 {
   /* Ideally we'd use OBJFILE->pspace, but OBJFILE may be NULL.  */
   symbol_cache_flush (current_program_space);
+
+  /* When all objfiles have been removed (OBJFILE is nullptr), then forget
+     everything we know about the main function.  */
+  if (objfile == nullptr)
+    set_main_name (nullptr, language_unknown);
 }
 
 /* This module's 'free_objfile' observer.  */
@@ -6329,15 +6336,6 @@ main_language (void)
   return info->language_of_main;
 }
 
-/* Handle ``executable_changed'' events for the symtab module.  */
-
-static void
-symtab_observer_executable_changed (void)
-{
-  /* NAME_OF_MAIN may no longer be the same, so reset it for now.  */
-  set_main_name (NULL, language_unknown);
-}
-
 /* Return 1 if the supplied producer string matches the ARM RealView
    compiler (armcc).  */
 
@@ -7020,8 +7018,6 @@ the use of prologue scanners."),
 		     class_maintenance, 0, &maintenancelist);
   deprecate_cmd (c, "maintenancelist flush symbol-cache");
 
-  gdb::observers::executable_changed.attach (symtab_observer_executable_changed,
-					     "symtab");
   gdb::observers::new_objfile.attach (symtab_new_objfile_observer, "symtab");
   gdb::observers::free_objfile.attach (symtab_free_objfile_observer, "symtab");
 }
-- 
2.25.4

[PATCH 5/9] gdb: remove final user of the executable_changed observer

[Andrew Burgess]

This commit continues with the task started in the previous commit,
and is similar in many ways.

The goal of the next couple of commits is to expose the
executable_changed observable in the Python API as an event.  Before I
do this I would like to remove the additional call to the
executable_changed observable which can be found in the reread_symbols
function in the symfile.c file, as I don't believe that this use
actually corresponds to a change in the current executable.

The previous commit removed one user of the executable_changed
observable and replaced it with a new_obfile observer instead, and
this commit does the same thing.

In auxv.c we use the executable_changed observable to call
invalidate_auxv_cache, which then calls:

  invalidate_auxv_cache_inf (current_inferior ());

The auxv cache is already (additionally) cleared when an inferior
exits and when an inferior appears.

As with the previous commit, I think we can safely replace the use of
the executable_changed observable with a use of the new_obfile
observable.  All the tests still pass, and with some locally placed
printf calls, I think that the cache is still being cleared in all the
cases that should matter.
---
 gdb/auxv.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/gdb/auxv.c b/gdb/auxv.c
index 3c27c1f1ffe..9f599b04a4f 100644
--- a/gdb/auxv.c
+++ b/gdb/auxv.c
@@ -344,12 +344,14 @@ invalidate_auxv_cache_inf (struct inferior *inf)
   auxv_inferior_data.clear (inf);
 }
 
-/* Invalidate current inferior's auxv cache.  */
+/* Invalidate current inferior's auxv cache when all symbol table data is
+   cleared (indicated by OBJFILE being nullptr).  */
 
 static void
-invalidate_auxv_cache (void)
+auxv_new_objfile_observer (struct objfile *objfile)
 {
-  invalidate_auxv_cache_inf (current_inferior ());
+  if (objfile == nullptr)
+    invalidate_auxv_cache_inf (current_inferior ());
 }
 
 /* See auxv.h.  */
@@ -613,5 +615,5 @@ This is information provided by the operating system at program startup."));
   /* Observers used to invalidate the auxv cache when needed.  */
   gdb::observers::inferior_exit.attach (invalidate_auxv_cache_inf, "auxv");
   gdb::observers::inferior_appeared.attach (invalidate_auxv_cache_inf, "auxv");
-  gdb::observers::executable_changed.attach (invalidate_auxv_cache, "auxv");
+  gdb::observers::new_objfile.attach (auxv_new_objfile_observer, "auxv");
 }
-- 
2.25.4

[PATCH 6/9] gdb: remove unnecessary notification of executable_changed observer

[Andrew Burgess]

This commit continues the work of the previous two commits.

My goal, in the next couple of commits, is to expose the
executable_changed observable in the Python API as an event.  However,
before I do that I want to remove the use of the executable_changed
observable from the reread_symbols function in symfile.c as this use
isn't directly associated with a change of the executable file, and so
seems wrong.

In the previous two commits I have removed all users of the
executable_changed observer as I believe those users can, and should,
actually be listening for the new_objfile observable instead, so now
there are no users of the executable_changed observable.

As such, I think removing the use of executable_changed from the
function reread_symbols is perfectly safe, and correct.  At this point
the executable has not been changed, so we shouldn't be sending an
executable_changed notification, and, as there is nobody listening to
this observable, we can't break anything by removing this call.

There should be no user visible changes after this commit.
---
 gdb/symfile.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 85a9c4e1da0..43fd45c4050 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2684,10 +2684,6 @@ reread_symbols (int from_tty)
 	 clear_symtab_users above.  Notify the new files now.  */
       for (auto iter : new_objfiles)
 	gdb::observers::new_objfile.notify (iter);
-
-      /* At least one objfile has changed, so we can consider that
-	 the executable we're debugging has changed too.  */
-      gdb::observers::executable_changed.notify ();
     }
 }
 \f
-- 
2.25.4

[PATCH 7/9] gdb: pass more arguments to the executable_changed observer

[Andrew Burgess]

This commit continues the work of the previous few commits.

My goal is to expose the executable_changed observer through the
Python API as an event.

At this point adding executable_changed as an event to the Python API
is trivial, but before I do that I would like to add some additional
arguments to the observable, which currently has no arguments at all.

The new arguments I wish to add are:

 1. The program_space in which the executable was changed, and

 2. A boolean flag that will indicate if the executable changed to a
 whole new path, or if GDB just spotted that the executable changed on
 disk (e.g. the user recompiled the executable).

In this commit I change the signature of the observable and then pass
the arguments through at the one place where this observable is
notified.

As there are (currently) no users of this observable nothing else
needs updating.  In the next commit I'll add a listener for this
observable in the Python code, and expose this as an event in the
Python API.

Additionally, with this change, it should be possible to update the
insight debugger to make use of this observable rather than using the
deprecated_exec_file_display_hook (as it currently does), which will
then allow this hook to be removed from GDB.

There should be no user visible changes after this commit.
---
 gdb/exec.c       | 11 ++++++++++-
 gdb/observable.h | 18 +++++++++++++-----
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/gdb/exec.c b/gdb/exec.c
index 07759725711..a1396c2aa3d 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -501,7 +501,16 @@ exec_file_attach (const char *filename, int from_tty)
     }
 
   bfd_cache_close_all ();
-  gdb::observers::executable_changed.notify ();
+
+  /* Are are loading the same executable?  */
+  bfd *prev_bfd = exec_bfd_holder.get ();
+  bfd *curr_bfd = current_program_space->exec_bfd ();
+  bool reload_p = (((prev_bfd != nullptr) == (curr_bfd != nullptr))
+		   && (prev_bfd == nullptr
+		       || (strcmp (bfd_get_filename (prev_bfd),
+				   bfd_get_filename (curr_bfd)) == 0)));
+
+  gdb::observers::executable_changed.notify (current_program_space, reload_p);
 }
 
 /*  Process the first arg in ARGS as the new exec file.
diff --git a/gdb/observable.h b/gdb/observable.h
index c0bafc51f14..e5305fa081e 100644
--- a/gdb/observable.h
+++ b/gdb/observable.h
@@ -31,6 +31,7 @@ struct inferior;
 struct process_stratum_target;
 struct target_ops;
 struct trace_state_variable;
+struct program_space;
 
 namespace gdb
 {
@@ -60,11 +61,18 @@ extern observable<enum gdb_signal /* siggnal */> signal_received;
 /* The target's register contents have changed.  */
 extern observable<struct target_ops */* target */> target_changed;
 
-/* The executable being debugged by GDB has changed: The user
-   decided to debug a different program, or the program he was
-   debugging has been modified since being loaded by the debugger
-   (by being recompiled, for instance).  */
-extern observable<> executable_changed;
+/* The executable being debugged by GDB in PSPACE has changed: The user
+   decided to debug a different program, or the program he was debugging
+   has been modified since being loaded by the debugger (by being
+   recompiled, for instance).  The path to the new executable can be found
+   by examining PSPACE->exec_filename.
+
+   When RELOAD is true the path to the executable hasn't changed, but the
+   file does appear to have changed, so GDB reloaded it, e.g. if the user
+   recompiled the executable.  when RELOAD is false then the path to the
+   executable has not changed.  */
+extern observable<struct program_space */* pspace */,
+		  bool /*reload */> executable_changed;
 
 /* gdb has just connected to an inferior.  For 'run', gdb calls this
    observer while the inferior is still stopped at the entry-point
-- 
2.25.4

[PATCH 8/9] gdb/python: make the executable_changed event available from Python

[Andrew Burgess]

This commit makes the executable_changed observable available through
the Python API as an event.  There's nothing particularly interesting
going on here, it just follows the same pattern as many of the other
Python events we support.

The new event registry is called events.executable_changed, and this
emits an ExecutableChangedEvent object which has two attributes, a
gdb.Progspace called 'progspace', this is the program space in which
the executable changed, and a Boolean called 'reload', which is True
if the same executable changed on disk and has been reloaded, or is
False when a new executable has been loaded.

One interesting thing did come up during testing though, you'll notice
the test contains a setup_kfail call.  During testing I observed that
the executable_changed event would trigger twice when GDB restarted an
inferior.  However, the ExecutableChangedEvent object is identical for
both calls, so the wrong information is never sent out, we just see
one too many events.

I tracked this down to how the reload_symbols function (symfile.c)
takes care to also reload the executable, however, I've split fixing
this into a separate commit, so see the next commit for details.
---
 gdb/NEWS                                  |   5 ++
 gdb/doc/python.texi                       |  32 +++++++
 gdb/python/py-all-events.def              |   1 +
 gdb/python/py-event-types.def             |   5 ++
 gdb/python/py-progspace.c                 |  54 ++++++++++++
 gdb/testsuite/gdb.python/py-exec-file.exp | 100 ++++++++++++++++++++++
 6 files changed, 197 insertions(+)

diff --git a/gdb/NEWS b/gdb/NEWS
index 93bc9c6a2c0..10975dbb27b 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -297,6 +297,11 @@ info main
      exact string passed by the user to these commands; the path will
      have been partially resolved to an absolute path.
 
+  ** A new executable_changed event registry is available.  This event
+     emits ExecutableChangedEvent objects, which have 'progspace' (a
+     gdb.Progspace) and 'reload' (a Boolean) attributes.  This event
+     is emitted when gdb.Progspace.executable_filename changes.
+
 *** Changes in GDB 13
 
 * MI version 1 is deprecated, and will be removed in GDB 14.
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 206cf6b4e18..9f7c7cb076a 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -3893,6 +3893,38 @@
 The @code{gdb.TargetConnection} that is being removed.
 @end defvar
 
+@item events.executable_changed
+Emit @code{gdb.ExecutableChangedEvent} which indicates that the
+@code{gdb.Progspace.executable_filename} has changed.
+
+Changed can mean that either, the value in
+@code{gdb.Progspace.executable_filename} has changed to a new path, or
+the executable pointed to by @code{gdb.Progspace.executable_filename}
+has changed on disk, and @value{GDBN} has reloaded it.
+
+@defvar ExecutableChangedEvent.progspace
+The @code{gdb.Progspace} in which the current executable has changed.
+The path to the updated executable will be visible in
+@code{gdb.Progspace.executable_filename} (@pxref{Progspaces In Python}).
+@end defvar
+@defvar ExecutableChangedEvent.reload
+This attribute will be @code{True} if the value of
+@code{gdb.Progspace.executable_filename} didn't change, but the file
+pointed to instead changed on disk, and @value{GDBN} reloaded it.
+
+When this attribute is @code{False}, the value in
+@code{gdb.Progspace.executable_filename} was changed to point to a new
+file.
+@end defvar
+
+Remember that @value{GDBN} tracks the executable file, and the symbol
+file separately, these are visible as
+@code{gdb.Progspace.executable_filename} and
+@code{gdb.Progspace.filename} respectively.  When using the @kbd{file}
+command, @value{GDBN} updates both of these fields, but the executable
+file is updated first, so when this event is emitted, the executable
+filename will have changed, but the symbol filename might still hold
+its previous value.
 @end table
 
 @node Threads In Python
diff --git a/gdb/python/py-all-events.def b/gdb/python/py-all-events.def
index aa28f2c6f8b..04a12e1bdf0 100644
--- a/gdb/python/py-all-events.def
+++ b/gdb/python/py-all-events.def
@@ -42,3 +42,4 @@ GDB_PY_DEFINE_EVENT(breakpoint_modified)
 GDB_PY_DEFINE_EVENT(before_prompt)
 GDB_PY_DEFINE_EVENT(gdb_exiting)
 GDB_PY_DEFINE_EVENT(connection_removed)
+GDB_PY_DEFINE_EVENT(executable_changed)
diff --git a/gdb/python/py-event-types.def b/gdb/python/py-event-types.def
index 395d6c07447..b862094650d 100644
--- a/gdb/python/py-event-types.def
+++ b/gdb/python/py-event-types.def
@@ -125,3 +125,8 @@ GDB_PY_DEFINE_EVENT_TYPE (connection,
 			  "ConnectionEvent",
 			  "GDB connection added or removed object",
 			  event_object_type);
+
+GDB_PY_DEFINE_EVENT_TYPE (executable_changed,
+			  "ExecutableChangedEvent",
+			  "GDB executable changed event",
+			  event_object_type);
diff --git a/gdb/python/py-progspace.c b/gdb/python/py-progspace.c
index 1319978d34a..082509b2b5b 100644
--- a/gdb/python/py-progspace.c
+++ b/gdb/python/py-progspace.c
@@ -26,6 +26,8 @@
 #include "arch-utils.h"
 #include "solib.h"
 #include "block.h"
+#include "py-event.h"
+#include "observable.h"
 
 struct pspace_object
 {
@@ -592,9 +594,61 @@ gdbpy_is_progspace (PyObject *obj)
   return PyObject_TypeCheck (obj, &pspace_object_type);
 }
 
+/* Emit an ExecutableChangedEvent event to REGISTRY.  Return 0 on success,
+   or a negative value on error.  PSPACE is the program_space in which the
+   current executable has changed, and RELOAD_P is true if the executable
+   path stayed the same, but the file on disk changed, or false if the
+   executable path actually changed.  */
+
+static int
+emit_executable_changed_event (eventregistry_object *registry,
+			       struct program_space *pspace, bool reload_p)
+{
+  gdbpy_ref<> event_obj
+    = create_event_object (&executable_changed_event_object_type);
+  if (event_obj == nullptr)
+    return -1;
+
+  gdbpy_ref<> py_pspace = pspace_to_pspace_object (pspace);
+  if (py_pspace == nullptr
+      || evpy_add_attribute (event_obj.get (), "progspace",
+			     py_pspace.get ()) < 0)
+    return -1;
+
+  gdbpy_ref<> py_reload_p (PyBool_FromLong (reload_p ? 1 : 0));
+  if (py_reload_p == nullptr
+      || evpy_add_attribute (event_obj.get (), "reload",
+			     py_reload_p.get ()) < 0)
+    return -1;
+
+  return evpy_emit_event (event_obj.get (), registry);
+}
+
+/* Listener for the executable_changed observable, this is called when the
+   current executable within PSPACE changes.  RELOAD_P is true if the
+   executable path stayed the same but the file changed on disk.  RELOAD_P
+   is false if the executable path was changed.  */
+
+static void
+gdbpy_executable_changed (struct program_space *pspace, bool reload_p)
+{
+  if (!gdb_python_initialized)
+    return;
+
+  gdbpy_enter enter_py;
+
+  if (!evregpy_no_listeners_p (gdb_py_events.executable_changed))
+    if (emit_executable_changed_event (gdb_py_events.executable_changed,
+				       pspace, reload_p) < 0)
+      gdbpy_print_stack ();
+}
+
 static int CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION
 gdbpy_initialize_pspace (void)
 {
+  gdb::observers::executable_changed.attach (gdbpy_executable_changed,
+					     "py-progspace");
+
   if (PyType_Ready (&pspace_object_type) < 0)
     return -1;
 
diff --git a/gdb/testsuite/gdb.python/py-exec-file.exp b/gdb/testsuite/gdb.python/py-exec-file.exp
index 14e5088af1c..5ad3cd7e50f 100644
--- a/gdb/testsuite/gdb.python/py-exec-file.exp
+++ b/gdb/testsuite/gdb.python/py-exec-file.exp
@@ -35,11 +35,59 @@ if {[build_executable "failed to prepare second executable" \
 set binfile1 [gdb_remote_download host $binfile1]
 set binfile2 [gdb_remote_download host $binfile2]
 
+# Setup a Python function to listen for the executable changed event.
+proc setup_exec_change_handler {} {
+    gdb_py_test_silent_cmd \
+	[multi_line \
+	     "python" \
+	     "def reset_state():" \
+	     "   global exec_changed_state" \
+	     "   exec_changed_state = \[0, None, None\]" \
+	     "end" ] \
+	"build reset_state function" 0
+
+    gdb_py_test_silent_cmd \
+	[multi_line \
+	     "python" \
+	     "def executable_changed(event):" \
+	     "   global exec_changed_state" \
+	     "   exec_changed_state\[0\] += 1" \
+	     "   exec_changed_state\[1\] = event.progspace.executable_filename" \
+	     "   exec_changed_state\[2\] = event.reload" \
+	     "end" ] \
+	"build executable_changed function" 0
+
+    gdb_test_no_output -nopass "python reset_state()"
+    gdb_test_no_output "python gdb.events.executable_changed.connect(executable_changed)"
+}
+
+# Check the global Python state that is updated when the
+# executable_changed event occurs, and then reset the global state.
+# FILENAME is a string, the name of the new executable file.  RELOAD
+# is a string, which should be 'True' or 'False', and represents if
+# the executable file was reloaded, or changed.
+proc check_exec_change { filename_re reload testname } {
+    if { $filename_re ne "None" } {
+	set filename_re "'$filename_re'"
+    }
+    if { $filename_re eq "None" && $reload eq "None" } {
+	set count 0
+    } else {
+	set count 1
+    }
+    gdb_test "python print(exec_changed_state)" \
+	"\\\[$count, $filename_re, $reload\\\]" \
+	$testname
+    gdb_test_no_output -nopass "python reset_state()"
+}
+
 # Check that the executable_filename is set correctly after using the
 # 'file' command.
 with_test_prefix "using 'file' command" {
     clean_restart
 
+    setup_exec_change_handler
+
     gdb_test "python print(gdb.current_progspace().executable_filename)" \
 	"None" \
 	"check executable_filename when no file is loaded"
@@ -51,6 +99,9 @@ with_test_prefix "using 'file' command" {
 	"[string_to_regexp $binfile1]" \
 	"check executable_filename when first executable is loaded"
 
+    check_exec_change [string_to_regexp $binfile1] False \
+	"check executable_changed state after first executable was loaded"
+
     gdb_test "file $binfile2" \
 	"Reading symbols from [string_to_regexp $binfile2]\\.\\.\\..*" \
 	"load second executable" \
@@ -59,42 +110,91 @@ with_test_prefix "using 'file' command" {
 	"[string_to_regexp $binfile2]" \
 	"check executable_filename when second executable is loaded"
 
+    check_exec_change [string_to_regexp $binfile2] False \
+	"check executable_changed state after second executable was loaded"
+
     gdb_unload
     gdb_test "python print(gdb.current_progspace().executable_filename)" \
 	"None" \
 	"check executable_filename after unloading file"
+
+    check_exec_change None False \
+	"check executable_changed state after unloading the executable"
 }
 
 # Check that the executable_filename is correctly set when we only set
 # the exec-file.
 with_test_prefix "using 'exec-file' command" {
     clean_restart
+
+    setup_exec_change_handler
+
     gdb_test_no_output "exec-file $binfile1" \
 	"load first executable"
     gdb_test "python print(gdb.current_progspace().executable_filename)" \
 	"[string_to_regexp $binfile1]" \
 	"check executable_filename when first executable is loaded"
 
+    check_exec_change [string_to_regexp $binfile1] False \
+	"check executable_changed state after first executable was loaded"
+
     gdb_test_no_output "exec-file $binfile2" \
 	"load second executable"
     gdb_test "python print(gdb.current_progspace().executable_filename)" \
 	"[string_to_regexp $binfile2]" \
 	"check executable_filename when second executable is loaded"
 
+    check_exec_change [string_to_regexp $binfile2] False \
+	"check executable_changed state after second executable was loaded"
+
     gdb_test "exec-file" "No executable file now\\."
     gdb_test "python print(gdb.current_progspace().executable_filename)" \
 	"None" \
 	"check executable_filename after unloading file"
+
+    check_exec_change None False \
+	"check executable_changed state after unloading the executable"
 }
 
 # Check that setting the symbol-file doesn't cause the
 # executable_filename to be set.
 with_test_prefix "using 'symbol-file' command" {
     clean_restart
+
+    setup_exec_change_handler
+
     gdb_test "symbol-file $binfile1" \
 	"Reading symbols from [string_to_regexp $binfile1]\\.\\.\\..*" \
 	"load first executable"
     gdb_test "python print(gdb.current_progspace().executable_filename)" \
 	"None" \
 	"check executable_filename after setting symbol-file"
+
+    check_exec_change None None \
+	"check executable_changed state after setting symbol-file"
+}
+
+# Check the executable_changed event when the executable changes on disk.
+with_test_prefix "exec changes on disk" {
+    clean_restart $binfile1
+
+    setup_exec_change_handler
+
+    runto_main
+
+    gdb_test_no_output "shell sleep 1" \
+	"ensure executable is at least 1 second old"
+
+    gdb_test "shell touch ${binfile1}" "" \
+	"update the executable on disk"
+
+    runto_main
+
+    # There is currently an issue where the executable_changed event
+    # will trigger twice during an inferior restart.  This should be
+    # fixed in the next commit, at which point this kfail can be
+    # removed.
+    setup_kfail "????" *-*-*
+    check_exec_change [string_to_regexp $binfile1] True \
+	"check executable_changed state after exec changed on disk"
 }
-- 
2.25.4

[PATCH 9/9] gdb: use reopen_exec_file from reread_symbols

[Andrew Burgess]

This commit fixes an issue that was discovered while writing the tests
for the previous commit.

I noticed that, when GDB restarts an inferior, the executable_changed
event would trigger twice.  The first notification would originate
from:

  #0  exec_file_attach (filename=0x4046680 "/tmp/hello.x", from_tty=0) at ../../src/gdb/exec.c:513
  #1  0x00000000006f3adb in reopen_exec_file () at ../../src/gdb/corefile.c:122
  #2  0x0000000000e6a3f2 in generic_mourn_inferior () at ../../src/gdb/target.c:3682
  #3  0x0000000000995121 in inf_child_target::mourn_inferior (this=0x2fe95c0 <the_amd64_linux_nat_target>) at ../../src/gdb/inf-child.c:192
  #4  0x0000000000995cff in inf_ptrace_target::mourn_inferior (this=0x2fe95c0 <the_amd64_linux_nat_target>) at ../../src/gdb/inf-ptrace.c:125
  #5  0x0000000000a32472 in linux_nat_target::mourn_inferior (this=0x2fe95c0 <the_amd64_linux_nat_target>) at ../../src/gdb/linux-nat.c:3609
  #6  0x0000000000e68a40 in target_mourn_inferior (ptid=...) at ../../src/gdb/target.c:2761
  #7  0x0000000000a323ec in linux_nat_target::kill (this=0x2fe95c0 <the_amd64_linux_nat_target>) at ../../src/gdb/linux-nat.c:3593
  #8  0x0000000000e64d1c in target_kill () at ../../src/gdb/target.c:924
  #9  0x00000000009a19bc in kill_if_already_running (from_tty=1) at ../../src/gdb/infcmd.c:328
  #10 0x00000000009a1a6f in run_command_1 (args=0x0, from_tty=1, run_how=RUN_STOP_AT_MAIN) at ../../src/gdb/infcmd.c:381
  #11 0x00000000009a20a5 in start_command (args=0x0, from_tty=1) at ../../src/gdb/infcmd.c:527
  #12 0x000000000068dc5d in do_simple_func (args=0x0, from_tty=1, c=0x35c7200) at ../../src/gdb/cli/cli-decode.c:95

While the second originates from:

  #0  exec_file_attach (filename=0x3d7a1d0 "/tmp/hello.x", from_tty=0) at ../../src/gdb/exec.c:513
  #1  0x0000000000dfe525 in reread_symbols (from_tty=1) at ../../src/gdb/symfile.c:2517
  #2  0x00000000009a1a98 in run_command_1 (args=0x0, from_tty=1, run_how=RUN_STOP_AT_MAIN) at ../../src/gdb/infcmd.c:398
  #3  0x00000000009a20a5 in start_command (args=0x0, from_tty=1) at ../../src/gdb/infcmd.c:527
  #4  0x000000000068dc5d in do_simple_func (args=0x0, from_tty=1, c=0x35c7200) at ../../src/gdb/cli/cli-decode.c:95

In the first case the call to exec_file_attach first passes through
reopen_exec_file.  The reopen_exec_file performs a modification time
check on the executable file, and only calls exec_file_attach if the
executable has changed on disk since it was last loaded.

However, in the second case things work a little differently.  In this
case GDB is really trying to reread the debug symbol.  As such, we
iterate over the objfiles list, and for each of those we check the
modification time, if the file on disk has changed then we reload the
debug symbols from that file.

However, there is an additional check, if the objfile has the same
name as the executable then we will call exec_file_attach, but we do
so without checking the cached modification time that indicates when
the executable was last reloaded, as a result, we reload the
executable twice.

In this commit I propose that reread_symbols be changed to
unconditionally call reopen_exec_file before performing the objfile
iteration.  This will ensure that, if the executable has changed, then
the executable will be reloaded, however, if the executable has
already been recently reloaded, we will not reload it for a second
time.

After handling the executable, GDB can then iterate over the objfiles
list and reload them in the normal way.

With this done I now see the executable reloaded only once when GDB
restarts an inferior, which means I can remove the kfail that I added
to the gdb.python/py-exec-file.exp test in the previous commit.
---
 gdb/symfile.c                             | 18 +++++-------------
 gdb/testsuite/gdb.python/py-exec-file.exp |  5 -----
 2 files changed, 5 insertions(+), 18 deletions(-)

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 43fd45c4050..fac8ec19a22 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2457,11 +2457,10 @@ remove_symbol_file_command (const char *args, int from_tty)
 void
 reread_symbols (int from_tty)
 {
-  long new_modtime;
-  struct stat new_statbuf;
-  int res;
   std::vector<struct objfile *> new_objfiles;
 
+  reopen_exec_file ();
+
   for (objfile *objfile : current_program_space->objfiles ())
     {
       if (objfile->obfd.get () == NULL)
@@ -2475,6 +2474,8 @@ reread_symbols (int from_tty)
 	 `ar', often called a `static library' on most systems, though
 	 a `shared library' on AIX is also an archive), then you should
 	 stat on the archive name, not member name.  */
+      int res;
+      struct stat new_statbuf;
       if (objfile->obfd->my_archive)
 	res = stat (bfd_get_filename (objfile->obfd->my_archive), &new_statbuf);
       else
@@ -2486,7 +2487,7 @@ reread_symbols (int from_tty)
 		      objfile_name (objfile));
 	  continue;
 	}
-      new_modtime = new_statbuf.st_mtime;
+      long new_modtime = new_statbuf.st_mtime;
       if (new_modtime != objfile->mtime)
 	{
 	  gdb_printf (_("`%ps' has changed; re-reading symbols.\n"),
@@ -2508,15 +2509,6 @@ reread_symbols (int from_tty)
 	  /* We need to do this whenever any symbols go away.  */
 	  clear_symtab_users_cleanup defer_clear_users (0);
 
-	  if (current_program_space->exec_bfd () != NULL
-	      && filename_cmp (bfd_get_filename (objfile->obfd.get ()),
-			       bfd_get_filename (current_program_space->exec_bfd ())) == 0)
-	    {
-	      /* Reload EXEC_BFD without asking anything.  */
-
-	      exec_file_attach (bfd_get_filename (objfile->obfd.get ()), 0);
-	    }
-
 	  /* Keep the calls order approx. the same as in free_objfile.  */
 
 	  /* Free the separate debug objfiles.  It will be
diff --git a/gdb/testsuite/gdb.python/py-exec-file.exp b/gdb/testsuite/gdb.python/py-exec-file.exp
index 5ad3cd7e50f..7aa19a867d7 100644
--- a/gdb/testsuite/gdb.python/py-exec-file.exp
+++ b/gdb/testsuite/gdb.python/py-exec-file.exp
@@ -190,11 +190,6 @@ with_test_prefix "exec changes on disk" {
 
     runto_main
 
-    # There is currently an issue where the executable_changed event
-    # will trigger twice during an inferior restart.  This should be
-    # fixed in the next commit, at which point this kfail can be
-    # removed.
-    setup_kfail "????" *-*-*
     check_exec_change [string_to_regexp $binfile1] True \
 	"check executable_changed state after exec changed on disk"
 }
-- 
2.25.4

Re: [PATCH 1/9] gdb/doc: extend the description for Progspace.filename

[Eli Zaretskii]

> Date: Sat, 16 Sep 2023 11:18:02 +0100
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
> 
> Extend the description for Progspace.filename in the documentation to
> mention what the returned string is actually the filename
> for (e.g. that it is the filename passed to the 'symbol-file' or
> 'file' command).
> 
> Also document that this attribute will be None if no symbol file is
> currently loaded.
> ---
>  gdb/doc/python.texi       | 7 ++++++-
>  gdb/python/py-progspace.c | 2 +-
>  2 files changed, 7 insertions(+), 2 deletions(-)

The python.texi part is approved, thanks.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH 3/9] gdb/python: new Progspace.executable_filename attribute

[Eli Zaretskii]

> Date: Sat, 16 Sep 2023 11:18:04 +0100
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
> 
> Add a new Progspace.executable_filename attribute that contains the
> path to the executable for this program space, or None if no
> executable is set.
> 
> The path within this attribute will be set by the "exec-file" and/or
> "file" commands.
> 
> Accessing this attribute for an invalid program space will raise an
> exception.
> 
> This new attribute is similar too, but not the same as the existing
> gdb.Progspace.filename attribute.  If I could change the past, I'd
> change the 'filename' attribute to 'symbol_filename', which is what it
> actually represents.  The old attribute will be set by the
> 'symbol-file' command, while the new attribute is set by the
> 'exec-file' command.  Obviously the 'file' command sets both of these
> attributes.
> ---
>  gdb/NEWS                                  |   6 ++
>  gdb/doc/python.texi                       |  15 ++++
>  gdb/python/py-progspace.c                 |  21 +++++
>  gdb/testsuite/gdb.python/py-exec-file.c   |  22 +++++
>  gdb/testsuite/gdb.python/py-exec-file.exp | 100 ++++++++++++++++++++++
>  5 files changed, 164 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.python/py-exec-file.c
>  create mode 100644 gdb/testsuite/gdb.python/py-exec-file.exp

Thanks.

> diff --git a/gdb/NEWS b/gdb/NEWS
> index f2f5dabb287..93bc9c6a2c0 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -291,6 +291,12 @@ info main
>       Progspace.filename is not None), otherwise, this attribute is
>       itself None.
>  
> +  ** New attribute Progspace.executable_filename.  This attribute
> +     holds a string containing a path set by the "exec-file" or "file"
> +     commands, or None if no executable file is set.  This isn't the
> +     exact string passed by the user to these commands; the path will
> +     have been partially resolved to an absolute path.

GNU Coding Standards frown on using "path" for anything but PATH-style
directory lists.  So please use "file name" here.

> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> index 03299cc3cf7..206cf6b4e18 100644
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -5059,6 +5059,21 @@
>  access this attribute will raise a @code{RuntimeError} exception.
>  @end defvar
>  
> +@defvar Progspace.executable_filename
> +The file name, as a string, of the executable file in use by this
> +program space.  The executable file is the file that @value{GDBN} will
> +invoke in order to start an inferior when using a native target.  The
> +file name within this attribute is updated by the @kbd{exec-file} and
> +@kbd{file} commands.
> +
> +If no executable is currently set within this @code{Progspace} then
> +this attribute contains @code{None}.
> +
> +If the @code{Progspace} is invalid, i.e.@:, when
> +@code{Progspace.is_valid()} returns @code{False}, then attempting to
> +access this attribute will raise a @code{RuntimeError} exception.
> +@end defvar

This part is OK.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH 2/9] gdb/python: new Progspace.symbol_file attribute

[Eli Zaretskii]

> Date: Sat, 16 Sep 2023 11:18:03 +0100
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
> 
> Add a new Progspace.symbol_file attribute.  This attribute holds the
> gdb.Objfile object that corresponds to Progspace.filename, or None if
> there is no main symbol file currently set.
> 
> Currently, to get this gdb.Objfile, a user would need to use
> Progspace.objfiles, and then search for the objfile with a name that
> matches Progspace.filename -- which should work just fine, but having
> direct access seems a little nicer.
> ---
>  gdb/NEWS                                  |  5 +++++
>  gdb/doc/python.texi                       | 17 +++++++++++++++++
>  gdb/python/py-progspace.c                 | 23 +++++++++++++++++++++++
>  gdb/testsuite/gdb.python/py-progspace.exp |  9 +++++++++
>  4 files changed, 54 insertions(+)

Thanks, the documentation parts are approved.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH 8/9] gdb/python: make the executable_changed event available from Python

[Eli Zaretskii]

> Date: Sat, 16 Sep 2023 11:18:09 +0100
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
> 
>  gdb/NEWS                                  |   5 ++
>  gdb/doc/python.texi                       |  32 +++++++
>  gdb/python/py-all-events.def              |   1 +
>  gdb/python/py-event-types.def             |   5 ++
>  gdb/python/py-progspace.c                 |  54 ++++++++++++
>  gdb/testsuite/gdb.python/py-exec-file.exp | 100 ++++++++++++++++++++++
>  6 files changed, 197 insertions(+)

Thanks.

> diff --git a/gdb/NEWS b/gdb/NEWS
> index 93bc9c6a2c0..10975dbb27b 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -297,6 +297,11 @@ info main
>       exact string passed by the user to these commands; the path will
>       have been partially resolved to an absolute path.
>  
> +  ** A new executable_changed event registry is available.  This event
> +     emits ExecutableChangedEvent objects, which have 'progspace' (a
> +     gdb.Progspace) and 'reload' (a Boolean) attributes.  This event
> +     is emitted when gdb.Progspace.executable_filename changes.
> +
>  *** Changes in GDB 13

This part is OK.

> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> index 206cf6b4e18..9f7c7cb076a 100644
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -3893,6 +3893,38 @@
>  The @code{gdb.TargetConnection} that is being removed.
>  @end defvar
>  
> +@item events.executable_changed
> +Emit @code{gdb.ExecutableChangedEvent} which indicates that the
> +@code{gdb.Progspace.executable_filename} has changed.
> +
> +Changed can mean that either, the value in
> +@code{gdb.Progspace.executable_filename} has changed to a new path, or
> +the executable pointed to by @code{gdb.Progspace.executable_filename}
> +has changed on disk, and @value{GDBN} has reloaded it.

This reads awkwardly, and uses "path" when it means "file name".
Suggest to rephrase:

  This event is emitted when either the value of
  @code{gdb.Progspace.executable_filename} has changed to name a
  different file, or the executable file named by
  @code{gdb.Progspace.executable_filename} has changed on disk, and
  @value{GDBN} has therefore reloaded it.

> +@defvar ExecutableChangedEvent.progspace
> +The @code{gdb.Progspace} in which the current executable has changed.
> +The path to the updated executable will be visible in
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
"The file name of the updated executable..."

> +This attribute will be @code{True} if the value of
> +@code{gdb.Progspace.executable_filename} didn't change, but the file
> +pointed to instead changed on disk, and @value{GDBN} reloaded it.
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
"...but the file it names changed on disk instead, ...."

> +When this attribute is @code{False}, the value in
> +@code{gdb.Progspace.executable_filename} was changed to point to a new
> +file.                                                ^^^^^^^^^^^^^^^^^
   ^^^^
"...to name a different file."

> +Remember that @value{GDBN} tracks the executable file, and the symbol
                                                        ^
That comma is redundant.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH 9/9] gdb: use reopen_exec_file from reread_symbols

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
 
Andrew> +  reopen_exec_file ();

This addition could probably use a comment explaining why it is needed.

Andrew> -      new_modtime = new_statbuf.st_mtime;
Andrew> +      long new_modtime = new_statbuf.st_mtime;

Might as well change this to time_t now.

I don't really understand how reread_symbols interacts with target:
files.  It seems like this is just broken.

At AdaCore we carry a patch that changes this code to use bfd_stat.
According to internal notes, this was pushed:

https://sourceware.org/legacy-ml/gdb-patches/2020-01/msg00386.html

... but I don't see it in gdb.  Maybe we never resolved the 'stat' issue
in gnulib?

Anyway, the reason I bring this up is that reopen_exec_file calls
bfd_cache_close_all -- but then the loop in reread_symbols, in the
bfd_stat case, will reopen the files (BFD uses fstat).  This seems
unfortunate.

I don't think gdb has a very clear idea of when bfd_cache_close_all
ought to be called.  It would be good to clear this up.  I'm not very
clear on it myself.  Maybe it is to avoid ETXTBUSY errors -- in which
case it seems like it should be called just before starting the
inferior.  But, ISTR some error on Windows as well, though I don't know
exactly what... so maybe the files need to be guaranteed-closed in other
situations as well.

Tom

Re: [PATCH 0/9] Add executable_changed event to Python API

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> When I started, I thought this would be trivial; I wanted to expose
Andrew> the executable_changed observer through the Python API.
[...]

I read this series and sent some comments on patch #9.
Really just the one nit has to be changed before landing, I guess, as
the other things there are pre-existing bugs.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 8/9] gdb/python: make the executable_changed event available from Python

[Andrew Burgess]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Sat, 16 Sep 2023 11:18:09 +0100
>> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
>> 
>>  gdb/NEWS                                  |   5 ++
>>  gdb/doc/python.texi                       |  32 +++++++
>>  gdb/python/py-all-events.def              |   1 +
>>  gdb/python/py-event-types.def             |   5 ++
>>  gdb/python/py-progspace.c                 |  54 ++++++++++++
>>  gdb/testsuite/gdb.python/py-exec-file.exp | 100 ++++++++++++++++++++++
>>  6 files changed, 197 insertions(+)
>
> Thanks.
>
>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index 93bc9c6a2c0..10975dbb27b 100644
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -297,6 +297,11 @@ info main
>>       exact string passed by the user to these commands; the path will
>>       have been partially resolved to an absolute path.
>>  
>> +  ** A new executable_changed event registry is available.  This event
>> +     emits ExecutableChangedEvent objects, which have 'progspace' (a
>> +     gdb.Progspace) and 'reload' (a Boolean) attributes.  This event
>> +     is emitted when gdb.Progspace.executable_filename changes.
>> +
>>  *** Changes in GDB 13
>
> This part is OK.
>
>> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
>> index 206cf6b4e18..9f7c7cb076a 100644
>> --- a/gdb/doc/python.texi
>> +++ b/gdb/doc/python.texi
>> @@ -3893,6 +3893,38 @@
>>  The @code{gdb.TargetConnection} that is being removed.
>>  @end defvar
>>  
>> +@item events.executable_changed
>> +Emit @code{gdb.ExecutableChangedEvent} which indicates that the
>> +@code{gdb.Progspace.executable_filename} has changed.
>> +
>> +Changed can mean that either, the value in
>> +@code{gdb.Progspace.executable_filename} has changed to a new path, or
>> +the executable pointed to by @code{gdb.Progspace.executable_filename}
>> +has changed on disk, and @value{GDBN} has reloaded it.
>
> This reads awkwardly, and uses "path" when it means "file name".
> Suggest to rephrase:
>
>   This event is emitted when either the value of
>   @code{gdb.Progspace.executable_filename} has changed to name a
>   different file, or the executable file named by
>   @code{gdb.Progspace.executable_filename} has changed on disk, and
>   @value{GDBN} has therefore reloaded it.
>
>> +@defvar ExecutableChangedEvent.progspace
>> +The @code{gdb.Progspace} in which the current executable has changed.
>> +The path to the updated executable will be visible in
>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> "The file name of the updated executable..."
>
>> +This attribute will be @code{True} if the value of
>> +@code{gdb.Progspace.executable_filename} didn't change, but the file
>> +pointed to instead changed on disk, and @value{GDBN} reloaded it.
>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> "...but the file it names changed on disk instead, ...."
>
>> +When this attribute is @code{False}, the value in
>> +@code{gdb.Progspace.executable_filename} was changed to point to a new
>> +file.                                                ^^^^^^^^^^^^^^^^^
>    ^^^^
> "...to name a different file."
>
>> +Remember that @value{GDBN} tracks the executable file, and the symbol
>                                                         ^
> That comma is redundant.
>
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Thanks Eli,  I made all the updates you suggested for this series before
pushing it.

Thanks,
Andrew

Re: [PATCH 9/9] gdb: use reopen_exec_file from reread_symbols

[Andrew Burgess]

Tom Tromey <tromey@adacore.com> writes:

>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>  
> Andrew> +  reopen_exec_file ();
>
> This addition could probably use a comment explaining why it is
> needed.

I added a comment.

>
> Andrew> -      new_modtime = new_statbuf.st_mtime;
> Andrew> +      long new_modtime = new_statbuf.st_mtime;
>
> Might as well change this to time_t now.

I fixed this.

>
> I don't really understand how reread_symbols interacts with target:
> files.  It seems like this is just broken.

Yup.

>
> At AdaCore we carry a patch that changes this code to use bfd_stat.
> According to internal notes, this was pushed:
>
> https://sourceware.org/legacy-ml/gdb-patches/2020-01/msg00386.html
>
> ... but I don't see it in gdb.  Maybe we never resolved the 'stat' issue
> in gnulib?

I guess not.  I already posted a series to look at this:

  https://inbox.sourceware.org/gdb-patches/cover.1695824275.git.aburgess@redhat.com/

But after reviewing your series I've updated this:

  https://inbox.sourceware.org/gdb-patches/cover.1695909469.git.aburgess@redhat.com/

I didn't fully understand all the discussion w.r.t. gnulib stat vs
system stat, but I'm hoping the change above, which is less that your
originally proposed full change, might be mergable.

Though even after that series reopen_exec_file is still broken for
target: files, I guess I'll see how the above goes before fixing that
too.

> Anyway, the reason I bring this up is that reopen_exec_file calls
> bfd_cache_close_all -- but then the loop in reread_symbols, in the
> bfd_stat case, will reopen the files (BFD uses fstat).  This seems
> unfortunate.
>
> I don't think gdb has a very clear idea of when bfd_cache_close_all
> ought to be called.  It would be good to clear this up.  I'm not very
> clear on it myself.  Maybe it is to avoid ETXTBUSY errors -- in which
> case it seems like it should be called just before starting the
> inferior.  But, ISTR some error on Windows as well, though I don't know
> exactly what... so maybe the files need to be guaranteed-closed in other
> situations as well.

I agree this stuff doesn't seem well defined at all, but I don't think I
made anything particularly worse in this respect, so I'm leaving that as
a problem for the future.

I did take a quick look at when bfd_cache_close_all is currently called,
and we do have a call in symbol_file_add_with_addrs, which is used when
a solib is loaded -- which means in many cases, when an inferior starts
we will end up calling bfd_cache_close_all anyway.  Note: I'm not
arguing that this is an actual well designed solution, just that in many
cases we're not going to hit any problems because of this.

Thanks,
Andrew

Re: [PATCH 9/9] gdb: use reopen_exec_file from reread_symbols

[Tom Tromey]

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

Andrew> I didn't fully understand all the discussion w.r.t. gnulib stat vs
Andrew> system stat, but I'm hoping the change above, which is less that your
Andrew> originally proposed full change, might be mergable.

FTR (and IIUC) the issue is that gnulib's stat yields different times on
Windows hosts, because it tries to handle the timezone -- on Unix, stat
reports in UTC, but on Windows it may report in local time.

The result is that BFD will report times that are offset from the times
reported by gnulib.

gnulib doesn't provide a way to change this behavior, so one proposal in
the thread was to hack gnulib.

>> I don't think gdb has a very clear idea of when bfd_cache_close_all
>> ought to be called.

Andrew> I agree this stuff doesn't seem well defined at all, but I don't think I
Andrew> made anything particularly worse in this respect, so I'm leaving that as
Andrew> a problem for the future.

One thing I wonder is whether bfd_cache_close_all is even needed.
gdbserver doesn't do this and AFAIK, gdb just keeps all those remote fds
open.

Maybe we're missing some testing scenario, though, like gdbserver
extended-remote with "gdb --write".

Tom

Re: [PATCH 9/9] gdb: use reopen_exec_file from reread_symbols

[Andrew Burgess]

Tom Tromey <tromey@adacore.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> I didn't fully understand all the discussion w.r.t. gnulib stat vs
> Andrew> system stat, but I'm hoping the change above, which is less that your
> Andrew> originally proposed full change, might be mergable.
>
> FTR (and IIUC) the issue is that gnulib's stat yields different times on
> Windows hosts, because it tries to handle the timezone -- on Unix, stat
> reports in UTC, but on Windows it may report in local time.
>
> The result is that BFD will report times that are offset from the times
> reported by gnulib.
>
> gnulib doesn't provide a way to change this behavior, so one proposal in
> the thread was to hack gnulib.
>
>>> I don't think gdb has a very clear idea of when bfd_cache_close_all
>>> ought to be called.
>
> Andrew> I agree this stuff doesn't seem well defined at all, but I don't think I
> Andrew> made anything particularly worse in this respect, so I'm leaving that as
> Andrew> a problem for the future.
>
> One thing I wonder is whether bfd_cache_close_all is even needed.
> gdbserver doesn't do this and AFAIK, gdb just keeps all those remote fds
> open.
>
> Maybe we're missing some testing scenario, though, like gdbserver
> extended-remote with "gdb --write".

So, I touched a bfd_cache_close_all recently[1], and dug into the history
of these calls a little at the time.  They were first added to GDB in
this commit:

  commit ce7d45220e4ed342d4a77fcd2f312e85e1100971
  Date:   Fri Jul 30 12:05:45 2004 +0000

Before this there were no bfd_cache_close_all calls in the GDB tree.

The mailing list thread associated with this commit is:

  https://sourceware.org/pipermail/gdb-patches/2004-July/036407.html

Though this thread seems to reference some older thread that I've not
been able to track down.  The above thread is titled:

  [RFA] win32: bfd_cache_close after kill

Here is the text from the older (unfound) thread that is quoted:

  > When the inferior is killed, it is safe the release the different file
  > handles that BFD keeps open. It is particularly useful on Win32 (and
  > presumably on HP UX) to be able to recompile and restart a new debugging
  > session without quitting GDB...

Now I've never done any development work on Win32, I very occasionally
boot a Windows machine to do a test build of GDB, but beyond that my
knowledge of Windows is like 20+ years old :-/  However, I do have a
vague memory that Windows didn't like you writing to a file that was
opened by some other application?  Maybe this memory is wrong, but that
would seem to align with the above text.

If that is the case, then it would seem (to me) that we want to ensure
BFD is not holding open any files at a time when GDB itself is not
actively doing anything that might read from a file, this would be:

  1. When GDB is sat idle at a prompt, and

  2. When GDB is blocked waiting for an inferior event.

Having a bfd_cache_close_all call in e.g. reopen_exec_file, seems pretty
pointless really, as it's not impossible that GDB will then immediately
re-open the BFD from some other function.

I wonder if we should remove all bfd_cache_close_all calls, and just
have two calls associated with the two states above?

[1] https://inbox.sourceware.org/gdb-patches/6219a1ae148c0b9796f7153b9a6c3b7173fb8f5a.1694706545.git.aburgess@redhat.com/

Thanks,
Andrew

Re: [PATCH 9/9] gdb: use reopen_exec_file from reread_symbols

[Eli Zaretskii]

> Cc: Tom Tromey <tromey@adacore.com>,  Andrew Burgess via Gdb-patches
>  <gdb-patches@sourceware.org>
> Date: Thu, 28 Sep 2023 12:15:18 -0600
> From: Tom Tromey via Gdb-patches <gdb-patches@sourceware.org>
> 
> >>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
> 
> Andrew> I didn't fully understand all the discussion w.r.t. gnulib stat vs
> Andrew> system stat, but I'm hoping the change above, which is less that your
> Andrew> originally proposed full change, might be mergable.
> 
> FTR (and IIUC) the issue is that gnulib's stat yields different times on
> Windows hosts, because it tries to handle the timezone -- on Unix, stat
> reports in UTC, but on Windows it may report in local time.

Are you sure?  That's not what I know: AFAIK on Windows 'stat' reports
times in UTC, not in local time.  At least for some (relatively old)
implementation of MSVCRT.DLL, the Windows libc, for which I have
sources, 'stat' clearly converts the file's times to UTC.  This page:

  https://learn.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-2013/14h5k7ff(v=vs.120)

also seems to imply that file times are indeed in UTC.

If we are unsure, we should talk to the Gnulib folks about this, they
probably know more about that.

Re: [PATCH 9/9] gdb: use reopen_exec_file from reread_symbols

[Eli Zaretskii]

> Cc: Tom Tromey <tromey@adacore.com>, Andrew Burgess via Gdb-patches
>  <gdb-patches@sourceware.org>
> Date: Fri, 29 Sep 2023 11:17:12 +0100
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
> 
> Here is the text from the older (unfound) thread that is quoted:
> 
>   > When the inferior is killed, it is safe the release the different file
>   > handles that BFD keeps open. It is particularly useful on Win32 (and
>   > presumably on HP UX) to be able to recompile and restart a new debugging
>   > session without quitting GDB...
> 
> Now I've never done any development work on Win32, I very occasionally
> boot a Windows machine to do a test build of GDB, but beyond that my
> knowledge of Windows is like 20+ years old :-/  However, I do have a
> vague memory that Windows didn't like you writing to a file that was
> opened by some other application?  Maybe this memory is wrong, but that
> would seem to align with the above text.

Yes, Windows will not allow you to write to a file opened by another
process.  (It is possible to open a file in a "shared" mode which will
then allow that, but few applications do that, and the default is not
"shared".)

> If that is the case, then it would seem (to me) that we want to ensure
> BFD is not holding open any files at a time when GDB itself is not
> actively doing anything that might read from a file, this would be:
> 
>   1. When GDB is sat idle at a prompt, and
> 
>   2. When GDB is blocked waiting for an inferior event.

Right.  The best would be for GDB to open the file, read from or write
to it, then close it immediately when it's done with it.

Re: [PATCH 9/9] gdb: use reopen_exec_file from reread_symbols

[Tom Tromey]

>> FTR (and IIUC) the issue is that gnulib's stat yields different times on
>> Windows hosts, because it tries to handle the timezone -- on Unix, stat
>> reports in UTC, but on Windows it may report in local time.

Eli> Are you sure?  That's not what I know: AFAIK on Windows 'stat' reports
Eli> times in UTC, not in local time.

I think so, see https://www.gnu.org/software/gnulib/manual/html_node/stat.html

Tom

Re: [PATCH 9/9] gdb: use reopen_exec_file from reread_symbols

[Tom Tromey]

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

Andrew>   1. When GDB is sat idle at a prompt, and

Andrew>   2. When GDB is blocked waiting for an inferior event.

Andrew> Having a bfd_cache_close_all call in e.g. reopen_exec_file, seems pretty
Andrew> pointless really, as it's not impossible that GDB will then immediately
Andrew> re-open the BFD from some other function.

Andrew> I wonder if we should remove all bfd_cache_close_all calls, and just
Andrew> have two calls associated with the two states above?

I'm a little wary of #1 because I'm working on a patch series to move
DWARF reading into the background.  That is, reading would be happening
while the prompt is visible.

Maybe the DWARF reader should be calling bfd_cache_close when it knows
it is done with a particular BFD.  I think really all that's needed is
to have it open while mapping the debug sections.

I suppose the lack of this code in gdbserver is just another
local/remote divergence that nobody has ever noticed.

Tom

Re: [PATCH 9/9] gdb: use reopen_exec_file from reread_symbols

[Eli Zaretskii]

> From: Tom Tromey <tromey@adacore.com>
> Cc: Tom Tromey <tromey@adacore.com>,  aburgess@redhat.com,
>   gdb-patches@sourceware.org
> Date: Mon, 02 Oct 2023 08:02:23 -0600
> 
> >> FTR (and IIUC) the issue is that gnulib's stat yields different times on
> >> Windows hosts, because it tries to handle the timezone -- on Unix, stat
> >> reports in UTC, but on Windows it may report in local time.
> 
> Eli> Are you sure?  That's not what I know: AFAIK on Windows 'stat' reports
> Eli> times in UTC, not in local time.
> 
> I think so, see https://www.gnu.org/software/gnulib/manual/html_node/stat.html

If you mean this part:

  The st_atime, st_ctime, st_mtime fields are affected by the current
  time zone and by the DST flag of the current time zone on some
  platforms: mingw, MSVC 14 (when the environment variable TZ is set).

then I'm not sure they mean the times are reported as local times.
They just say "are affected".  My interpretation of that is that on
MinGW and MSVC times might be off by, like, 1 hour, if the DST flag at
the file's time and at current system time is different.

But I've CC'ed Bruno, who knows this stuff better, to ask him to
clarify this for us.