[6/8] gdb: add all_objfiles_removed observer

Message ID 20231004022305.298534-7-simon.marchi@polymtl.ca
State New
Headers
Series Split new_objfile observable |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 fail Patch failed to apply

Commit Message

Simon Marchi Oct. 4, 2023, 2:20 a.m. UTC
  From: Simon Marchi <simon.marchi@efficios.com>

The new_objfile observer is currently used to indicate both when a new
objfile is added to program space (when passed non-nullptr) and when all
objfiles of a program space were just removed (when passed nullptr).
I think this is confusing (and Andrew apparently thinks so too [1]).
Add a new "all_objfiles_removed" observer to remove the second role from
"new_objfile".

Some existing users of new_objfile do nothing if the passed objfile is
nullptr.  For them, we can simply drop the nullptr check.  For others,
add a new all_objfiles_removed callback, and refactor things a bit to
keep the existing behavior as much as possible.

Some callbacks relied on current_program_space, and following
the refactoring now use either objfile->pspace or the pspace passed to
all_objfiles_removed.  I think this should be relatively safe, and in
general a step in the right direction.

On the notify side, I found only one call site to change from
new_objfile to all_objfiles_removed, in clear_symtab_users.  It is not
entirely clear to me that this is entirely correct.  clear_symtab_users
appears to be called in spots that don't remove all objfiles
(functions finish_new_objfile, remove_symbol_file_command, reread_symbols,
do_module_cleanups).  But I think that this patch at least makes the
current code clearer.

[1] https://gitlab.com/gnutools/binutils-gdb/-/commit/a0a031bce0527b1521788b5dad640e7883b3a252

Change-Id: Icb648f72862e056267f30f44dd439bd4ec766f13
---
 gdb/ada-lang.c           |  6 ++++--
 gdb/ada-tasks.c          | 41 ++++++++++++++++++----------------------
 gdb/agent.c              |  2 +-
 gdb/aix-thread.c         |  7 +++----
 gdb/arm-tdep.c           |  2 +-
 gdb/auto-load.c          | 29 ++++++++--------------------
 gdb/auxv.c               | 13 +++++++------
 gdb/linux-thread-db.c    |  5 ++---
 gdb/observable.c         |  1 +
 gdb/observable.h         |  7 ++++---
 gdb/python/py-inferior.c | 30 ++++++++++++++++-------------
 gdb/remote.c             | 18 +++++++++++++-----
 gdb/sol-thread.c         |  3 +--
 gdb/symfile.c            |  2 +-
 gdb/symtab.c             | 19 +++++++++++++------
 gdb/tui/tui-hooks.c      | 13 +++++++++++--
 16 files changed, 105 insertions(+), 93 deletions(-)
  

Comments

Tom Tromey Oct. 5, 2023, 4:35 p.m. UTC | #1
>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> On the notify side, I found only one call site to change from
Simon> new_objfile to all_objfiles_removed, in clear_symtab_users.  It is not
Simon> entirely clear to me that this is entirely correct.  clear_symtab_users
Simon> appears to be called in spots that don't remove all objfiles
Simon> (functions finish_new_objfile, remove_symbol_file_command, reread_symbols,
Simon> do_module_cleanups).  But I think that this patch at least makes the
Simon> current code clearer.

FWIW there are a couple of bugs open about clear_symtab_users:

https://sourceware.org/bugzilla/show_bug.cgi?id=17938
https://sourceware.org/bugzilla/show_bug.cgi?id=17857

Tom
  
Simon Marchi Oct. 5, 2023, 5:13 p.m. UTC | #2
On 10/5/23 12:35, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> On the notify side, I found only one call site to change from
> Simon> new_objfile to all_objfiles_removed, in clear_symtab_users.  It is not
> Simon> entirely clear to me that this is entirely correct.  clear_symtab_users
> Simon> appears to be called in spots that don't remove all objfiles
> Simon> (functions finish_new_objfile, remove_symbol_file_command, reread_symbols,
> Simon> do_module_cleanups).  But I think that this patch at least makes the
> Simon> current code clearer.
> 
> FWIW there are a couple of bugs open about clear_symtab_users:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=17938
> https://sourceware.org/bugzilla/show_bug.cgi?id=17857

Thanks, I'll keep that in mind when working on solibs.

Simon
  

Patch

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 4dc405580645..a37e56375341 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -13884,7 +13884,7 @@  static struct cmd_list_element *show_ada_list;
 static void
 ada_new_objfile_observer (struct objfile *objfile)
 {
-  ada_clear_symbol_cache (current_program_space);
+  ada_clear_symbol_cache (objfile->pspace);
 }
 
 /* This module's 'free_objfile' observer.  */
@@ -13892,7 +13892,7 @@  ada_new_objfile_observer (struct objfile *objfile)
 static void
 ada_free_objfile_observer (struct objfile *objfile)
 {
-  ada_clear_symbol_cache (current_program_space);
+  ada_clear_symbol_cache (objfile->pspace);
 }
 
 /* Charsets known to GNAT.  */
@@ -14025,6 +14025,8 @@  DWARF attribute."),
 
   /* The ada-lang observers.  */
   gdb::observers::new_objfile.attach (ada_new_objfile_observer, "ada-lang");
+  gdb::observers::all_objfiles_removed.attach (ada_clear_symbol_cache,
+					       "ada-lang");
   gdb::observers::free_objfile.attach (ada_free_objfile_observer, "ada-lang");
   gdb::observers::inferior_exit.attach (ada_inferior_exit, "ada-lang");
 
diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
index 14b0bf11c2a6..1de6b0fc930b 100644
--- a/gdb/ada-tasks.c
+++ b/gdb/ada-tasks.c
@@ -1469,38 +1469,31 @@  ada_tasks_normal_stop_observer (struct bpstat *unused_args, int unused_args2)
   ada_task_list_changed (current_inferior ());
 }
 
-/* A routine to be called when the objfiles have changed.  */
+/* Clear data associated to PSPACE and all inferiors using that program
+   space.  */
 
 static void
-ada_tasks_new_objfile_observer (struct objfile *objfile)
+ada_tasks_clear_pspace_data (program_space *pspace)
 {
-  /* Invalidate the relevant data in our program-space data.  */
-
-  if (objfile == NULL)
-    {
-      /* All objfiles are being cleared, so we should clear all
-	 our caches for all program spaces.  */
-      for (struct program_space *pspace : program_spaces)
-	ada_tasks_invalidate_pspace_data (pspace);
-    }
-  else
-    {
-      /* The associated program-space data might have changed after
-	 this objfile was added.  Invalidate all cached data.  */
-      ada_tasks_invalidate_pspace_data (objfile->pspace);
-    }
+  /* The associated program-space data might have changed after
+     this objfile was added.  Invalidate all cached data.  */
+  ada_tasks_invalidate_pspace_data (pspace);
 
   /* Invalidate the per-inferior cache for all inferiors using
-     this objfile (or, in other words, for all inferiors who have
-     the same program-space as the objfile's program space).
-     If all objfiles are being cleared (OBJFILE is NULL), then
-     clear the caches for all inferiors.  */
-
+     this program space.  */
   for (inferior *inf : all_inferiors ())
-    if (objfile == NULL || inf->pspace == objfile->pspace)
+    if (inf->pspace == pspace)
       ada_tasks_invalidate_inferior_data (inf);
 }
 
+/* Called when a new objfile was added.  */
+
+static void
+ada_tasks_new_objfile_observer (objfile *objfile)
+{
+  ada_tasks_clear_pspace_data (objfile->pspace);
+}
+
 /* The qcs command line flags for the "task apply" commands.  Keep
    this in sync with the "frame apply" commands.  */
 
@@ -1667,6 +1660,8 @@  _initialize_tasks ()
 				      "ada-tasks");
   gdb::observers::new_objfile.attach (ada_tasks_new_objfile_observer,
 				      "ada-tasks");
+  gdb::observers::all_objfiles_removed.attach (ada_tasks_clear_pspace_data,
+					       "ada-tasks");
 
   static struct cmd_list_element *task_cmd_list;
   static struct cmd_list_element *task_apply_list;
diff --git a/gdb/agent.c b/gdb/agent.c
index e2ad3b582fa2..d7e48589ab9a 100644
--- a/gdb/agent.c
+++ b/gdb/agent.c
@@ -65,7 +65,7 @@  set_can_use_agent (const char *args, int from_tty, struct cmd_list_element *c)
 static void
 agent_new_objfile (struct objfile *objfile)
 {
-  if (objfile == NULL || agent_loaded_p ())
+  if (agent_loaded_p ())
     return;
 
   if (can_use_agent == can_use_agent_off)
diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index 1f7b3c511cf2..2077491d3cf7 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -1109,14 +1109,13 @@  pd_disable (inferior *inf)
 
 /* new_objfile observer callback.
 
-   If OBJFILE is non-null, check whether a threaded application is
-   being debugged, and if so, prepare for thread debugging.  */
+   Check whether a threaded application is being debugged, and if so, prepare
+   for thread debugging.  */
 
 static void
 new_objfile (struct objfile *objfile)
 {
-  if (objfile)
-    pd_enable (current_inferior ());
+  pd_enable (current_inferior ());
 }
 
 /* Attach to process specified by ARGS.  */
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 24952840e085..7e069d187708 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -2543,7 +2543,7 @@  arm_exidx_new_objfile (struct objfile *objfile)
   LONGEST i;
 
   /* If we've already touched this file, do nothing.  */
-  if (!objfile || arm_exidx_data_key.get (objfile->obfd.get ()) != NULL)
+  if (arm_exidx_data_key.get (objfile->obfd.get ()) != nullptr)
     return;
 
   /* Read contents of exception table and index.  */
diff --git a/gdb/auto-load.c b/gdb/auto-load.c
index 37a44cc2eaa6..ae5efa3ac126 100644
--- a/gdb/auto-load.c
+++ b/gdb/auto-load.c
@@ -1138,7 +1138,11 @@  auto_load_section_scripts (struct objfile *objfile, const char *section_name)
     }
 }
 
-/* Load any auto-loaded scripts for OBJFILE.  */
+/* Load any auto-loaded scripts for OBJFILE.
+
+   Two flavors of auto-loaded scripts are supported.
+   1) based on the path to the objfile
+   2) from .debug_gdb_scripts section  */
 
 void
 load_auto_scripts_for_objfile (struct objfile *objfile)
@@ -1160,25 +1164,6 @@  load_auto_scripts_for_objfile (struct objfile *objfile)
   auto_load_section_scripts (objfile, AUTO_SECTION_NAME);
 }
 
-/* This is a new_objfile observer callback to auto-load scripts.
-
-   Two flavors of auto-loaded scripts are supported.
-   1) based on the path to the objfile
-   2) from .debug_gdb_scripts section  */
-
-static void
-auto_load_new_objfile (struct objfile *objfile)
-{
-  if (!objfile)
-    {
-      /* OBJFILE is NULL when loading a new "main" symbol-file.  */
-      clear_section_scripts (current_program_space);
-      return;
-    }
-
-  load_auto_scripts_for_objfile (objfile);
-}
-
 /* Collect scripts to be printed in a vec.  */
 
 struct collect_matching_scripts_data
@@ -1531,9 +1516,11 @@  _initialize_auto_load ()
     python_name_help, guile_name_help;
   const char *suffix;
 
-  gdb::observers::new_objfile.attach (auto_load_new_objfile,
+  gdb::observers::new_objfile.attach (load_auto_scripts_for_objfile,
 				      auto_load_new_objfile_observer_token,
 				      "auto-load");
+  gdb::observers::all_objfiles_removed.attach (clear_section_scripts,
+					       "auto-load");  
   add_setshow_boolean_cmd ("gdb-scripts", class_support,
 			   &auto_load_gdb_scripts, _("\
 Enable or disable auto-loading of canned sequences of commands scripts."), _("\
diff --git a/gdb/auxv.c b/gdb/auxv.c
index 9f599b04a4fe..d25e8b494ab2 100644
--- a/gdb/auxv.c
+++ b/gdb/auxv.c
@@ -344,14 +344,14 @@  invalidate_auxv_cache_inf (struct inferior *inf)
   auxv_inferior_data.clear (inf);
 }
 
-/* Invalidate current inferior's auxv cache when all symbol table data is
-   cleared (indicated by OBJFILE being nullptr).  */
+/* Invalidate the auxv cache for all inferiors using PSPACE.  */
 
 static void
-auxv_new_objfile_observer (struct objfile *objfile)
+auxv_all_objfiles_removed (program_space *pspace)
 {
-  if (objfile == nullptr)
-    invalidate_auxv_cache_inf (current_inferior ());
+  for (inferior *inf : all_inferiors ())
+    if (inf->pspace == pspace)
+      invalidate_auxv_cache_inf (inf);
 }
 
 /* See auxv.h.  */
@@ -615,5 +615,6 @@  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::new_objfile.attach (auxv_new_objfile_observer, "auxv");
+  gdb::observers::all_objfiles_removed.attach (auxv_all_objfiles_removed,
+					       "auxv");
 }
diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
index 16c250c104da..cef78f599be0 100644
--- a/gdb/linux-thread-db.c
+++ b/gdb/linux-thread-db.c
@@ -1278,13 +1278,12 @@  thread_db_new_objfile (struct objfile *objfile)
   /* This observer must always be called with inferior_ptid set
      correctly.  */
 
-  if (objfile != NULL
-      /* libpthread with separate debug info has its debug info file already
+  if (/* libpthread with separate debug info has its debug info file already
 	 loaded (and notified without successful thread_db initialization)
 	 the time gdb::observers::new_objfile.notify is called for the library itself.
 	 Static executables have their separate debug info loaded already
 	 before the inferior has started.  */
-      && objfile->separate_debug_objfile_backlink == NULL
+      objfile->separate_debug_objfile_backlink == NULL
       /* Only check for thread_db if we loaded libpthread,
 	 or if this is the main symbol file.
 	 We need to check OBJF_MAINLINE to handle the case of debugging
diff --git a/gdb/observable.c b/gdb/observable.c
index 93a842e911fe..09613b2ddda3 100644
--- a/gdb/observable.c
+++ b/gdb/observable.c
@@ -42,6 +42,7 @@  DEFINE_OBSERVABLE (inferior_forked);
 DEFINE_OBSERVABLE (solib_loaded);
 DEFINE_OBSERVABLE (solib_unloaded);
 DEFINE_OBSERVABLE (new_objfile);
+DEFINE_OBSERVABLE (all_objfiles_removed);
 DEFINE_OBSERVABLE (free_objfile);
 DEFINE_OBSERVABLE (new_thread);
 DEFINE_OBSERVABLE (thread_exit);
diff --git a/gdb/observable.h b/gdb/observable.h
index d9fc5cbcc8bc..5a2144030b3e 100644
--- a/gdb/observable.h
+++ b/gdb/observable.h
@@ -107,11 +107,12 @@  extern observable<struct so_list */* solib */> solib_loaded;
 extern observable<struct program_space */* pspace */, struct so_list */* solib */>
   solib_unloaded;
 
-/* The symbol file specified by OBJFILE has been loaded.  Called
-   with OBJFILE equal to NULL to indicate previously loaded symbol
-   table data has now been invalidated.  */
+/* The symbol file specified by OBJFILE has been loaded.  */
 extern observable<struct objfile */* objfile */> new_objfile;
 
+/*  All objfiles from PSPACE were removed.  */
+extern observable<program_space */* pspace */> all_objfiles_removed;
+
 /* The object file specified by OBJFILE is about to be freed.  */
 extern observable<struct objfile */* objfile */> free_objfile;
 
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index e7a9d822409b..1f20b9acd112 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -190,20 +190,22 @@  python_new_objfile (struct objfile *objfile)
   if (!gdb_python_initialized)
     return;
 
-  gdbpy_enter enter_py (objfile != NULL
-			? objfile->arch ()
-			: target_gdbarch ());
+  gdbpy_enter enter_py (objfile->arch ());
 
-  if (objfile == NULL)
-    {
-      if (emit_clear_objfiles_event (current_program_space) < 0)
-	gdbpy_print_stack ();
-    }
-  else
-    {
-      if (emit_new_objfile_event (objfile) < 0)
-	gdbpy_print_stack ();
-    }
+  if (emit_new_objfile_event (objfile) < 0)
+    gdbpy_print_stack ();
+}
+
+static void
+python_all_objfiles_removed (program_space *pspace)
+{
+  if (!gdb_python_initialized)
+    return;
+
+  gdbpy_enter enter_py (target_gdbarch ());
+
+  if (emit_clear_objfiles_event (pspace) < 0)
+    gdbpy_print_stack ();
 }
 
 /* Emit a Python event when an objfile is about to be removed.  */
@@ -1020,6 +1022,8 @@  gdbpy_initialize_inferior (void)
   gdb::observers::new_objfile.attach
     (python_new_objfile, "py-inferior",
      { &auto_load_new_objfile_observer_token });
+  gdb::observers::all_objfiles_removed.attach (python_all_objfiles_removed,
+					       "py-inferior");
   gdb::observers::free_objfile.attach (python_free_objfile, "py-inferior");
   gdb::observers::inferior_added.attach (python_new_inferior, "py-inferior");
   gdb::observers::inferior_removed.attach (python_inferior_deleted,
diff --git a/gdb/remote.c b/gdb/remote.c
index 9bb4f1de5982..ae08c980efce 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -14963,14 +14963,12 @@  show_remote_cmd (const char *args, int from_tty)
       }
 }
 
+/* Some change happened in PSPACE's objfile list (obfiles added or removed),
+   offer all inferiors using that program space a change to look up symbols.  */
 
-/* Function to be called whenever a new objfile (shlib) is detected.  */
 static void
-remote_new_objfile (struct objfile *objfile)
+remote_objfile_changed_check_symbols (program_space *pspace)
 {
-  /* The objfile change happened in that program space.  */
-  program_space *pspace = current_program_space;
-
   /* The affected program space is possibly shared by multiple inferiors.
      Consider sending a qSymbol packet for each of the inferiors using that
      program space.  */
@@ -15019,6 +15017,14 @@  remote_new_objfile (struct objfile *objfile)
   }
 }
 
+/* Function to be called whenever a new objfile (shlib) is detected.  */
+
+static void
+remote_new_objfile (struct objfile *objfile)
+{
+  remote_objfile_changed_check_symbols (objfile->pspace);
+}
+
 /* Pull all the tracepoints defined on the target and create local
    data structures representing them.  We don't want to create real
    tracepoints yet, we don't want to mess up the user's existing
@@ -15333,6 +15339,8 @@  _initialize_remote ()
 
   /* Hook into new objfile notification.  */
   gdb::observers::new_objfile.attach (remote_new_objfile, "remote");
+  gdb::observers::all_objfiles_removed.attach
+    (remote_objfile_changed_check_symbols, "remote");
 
 #if 0
   init_remote_threadtests ();
diff --git a/gdb/sol-thread.c b/gdb/sol-thread.c
index ed1a803fbdb0..c152b44dea63 100644
--- a/gdb/sol-thread.c
+++ b/gdb/sol-thread.c
@@ -668,8 +668,7 @@  check_for_thread_db (void)
 static void
 sol_thread_new_objfile (struct objfile *objfile)
 {
-  if (objfile != NULL)
-    check_for_thread_db ();
+  check_for_thread_db ();
 }
 
 /* Clean up after the inferior dies.  */
diff --git a/gdb/symfile.c b/gdb/symfile.c
index cc35a5389ee3..d8b67d094ab2 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2902,7 +2902,7 @@  clear_symtab_users (symfile_add_flags add_flags)
   clear_displays ();
   clear_last_displayed_sal ();
   clear_pc_function_cache ();
-  gdb::observers::new_objfile.notify (NULL);
+  gdb::observers::all_objfiles_removed.notify (current_program_space);
 
   /* Now that the various caches have been cleared, we can re_set
      our breakpoints without risking it using stale data.  */
diff --git a/gdb/symtab.c b/gdb/symtab.c
index afad782fcdbf..e9bc7b2c9339 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -1695,13 +1695,18 @@  maintenance_print_symbol_cache_statistics (const char *args, int from_tty)
 static void
 symtab_new_objfile_observer (struct objfile *objfile)
 {
-  /* Ideally we'd use OBJFILE->pspace, but OBJFILE may be NULL.  */
-  symbol_cache_flush (current_program_space);
+  symbol_cache_flush (objfile->pspace);
+}
+
+/* This module's 'all_objfiles_removed' observer.  */
+
+static void
+symtab_all_objfiles_removed (program_space *pspace)
+{
+  symbol_cache_flush (pspace);
 
-  /* When all objfiles have been removed (OBJFILE is nullptr), then forget
-     everything we know about the main function.  */
-  if (objfile == nullptr)
-    set_main_name (current_program_space, nullptr, language_unknown);
+  /* Forget everything we know about the main function.  */
+  set_main_name (pspace, nullptr, language_unknown);
 }
 
 /* This module's 'free_objfile' observer.  */
@@ -7022,5 +7027,7 @@  the use of prologue scanners."),
   deprecate_cmd (c, "maintenancelist flush symbol-cache");
 
   gdb::observers::new_objfile.attach (symtab_new_objfile_observer, "symtab");
+  gdb::observers::all_objfiles_removed.attach (symtab_all_objfiles_removed,
+					       "symtab");
   gdb::observers::free_objfile.attach (symtab_free_objfile_observer, "symtab");
 }
diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
index d67c3d7f3531..6525f0f2b6cf 100644
--- a/gdb/tui/tui-hooks.c
+++ b/gdb/tui/tui-hooks.c
@@ -49,13 +49,20 @@ 
 
 #include "gdb_curses.h"
 
-static void
-tui_new_objfile_hook (struct objfile* objfile)
+static void tui_on_objfiles_changed ()
 {
   if (tui_active)
     tui_display_main ();
 }
 
+static void
+tui_new_objfile_hook (struct objfile* objfile)
+{ tui_on_objfiles_changed (); }
+
+static void
+tui_all_objfiles_removed (program_space *pspace)
+{ tui_on_objfiles_changed (); }
+
 /* Prevent recursion of deprecated_register_changed_hook().  */
 static bool tui_refreshing_registers = false;
 
@@ -283,4 +290,6 @@  _initialize_tui_hooks ()
 {
   /* Install the permanent hooks.  */
   gdb::observers::new_objfile.attach (tui_new_objfile_hook, "tui-hooks");
+  gdb::observers::all_objfiles_removed.attach (tui_all_objfiles_removed,
+					       "tui-hooks");
 }