[PATCH v2 0/5] Random cleanup patches

[PATCH v2 0/5] Random cleanup patches

[Simon Marchi]

This is v2 of:

https://inbox.sourceware.org/gdb-patches/20240206171514.119244-1-simon.marchi@efficios.com/T/#m396f40ecb617db9e71beae00f82f21bae92a36cd

The only difference is to revert using the current inferior in patch 4,
as described here:

https://inbox.sourceware.org/gdb-patches/20240206171514.119244-1-simon.marchi@efficios.com/T/#mb86bb48b17bdb01e5c0c28609aa168cb25722958

Simon Marchi (5):
  gdb: add program_space parameter to mark_breakpoints_out
  gdb: add inferior parameter to breakpoint_init_inferior
  gdb: add program_space parameter to disable_breakpoints_in_shlibs
  gdb: add program_space parameter to clear_solib
  gdb: remove unnecessary nullptr check in remove_user_added_objfile

 gdb/breakpoint.c  | 35 +++++++++++------------------------
 gdb/breakpoint.h  | 22 ++++++++++++++++++----
 gdb/corelow.c     |  2 +-
 gdb/infcmd.c      | 10 +++++-----
 gdb/infrun.c      |  6 +++---
 gdb/solib.c       | 18 +++++++++---------
 gdb/solib.h       |  6 +++---
 gdb/symtab.h      |  4 ----
 gdb/target.c      |  4 ++--
 gdb/windows-nat.c |  2 +-
 10 files changed, 53 insertions(+), 56 deletions(-)


base-commit: 6fb99666f4bbc79708acb8efb2d80e57de67b80b
-- 
2.43.0

[PATCH v2 1/5] gdb: add program_space parameter to mark_breakpoints_out

[Simon Marchi]

Make the current_program_space reference bubble up one level.

Change-Id: Idc8ed78d23bf3bb2969f6963d8cc049f26901c29
---
 gdb/breakpoint.c | 8 ++++----
 gdb/breakpoint.h | 5 +++--
 gdb/infrun.c     | 2 +-
 gdb/target.c     | 2 +-
 4 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 7d1171ec35eb..d844ef7baf25 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4216,13 +4216,13 @@ remove_breakpoint (struct bp_location *bl)
   return remove_breakpoint_1 (bl, REMOVE_BREAKPOINT);
 }
 
-/* Clear the "inserted" flag in all breakpoints.  */
+/* See breakpoint.h.  */
 
 void
-mark_breakpoints_out (void)
+mark_breakpoints_out (program_space *pspace)
 {
   for (bp_location *bl : all_bp_locations ())
-    if (bl->pspace == current_program_space)
+    if (bl->pspace == pspace)
       bl->inserted = 0;
 }
 
@@ -4248,7 +4248,7 @@ breakpoint_init_inferior (enum inf_context context)
   if (gdbarch_has_global_breakpoints (current_inferior ()->arch ()))
     return;
 
-  mark_breakpoints_out ();
+  mark_breakpoints_out (pspace);
 
   for (breakpoint &b : all_breakpoints_safe ())
     {
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 93c49520e8ce..e7bebed2725d 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1764,8 +1764,9 @@ extern void breakpoint_set_inferior (struct breakpoint *b, int inferior);
 
 extern void breakpoint_set_task (struct breakpoint *b, int task);
 
-/* Clear the "inserted" flag in all breakpoints.  */
-extern void mark_breakpoints_out (void);
+/* Clear the "inserted" flag in all breakpoints locations in PSPACE.  */
+
+extern void mark_breakpoints_out (program_space *pspace);
 
 extern struct breakpoint *create_jit_event_breakpoint (struct gdbarch *,
 						       CORE_ADDR);
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 87965fb12742..d00a98906d71 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1247,7 +1247,7 @@ follow_exec (ptid_t ptid, const char *exec_file_target)
      value that was overwritten with a TRAP instruction).  Since
      we now have a new a.out, those shadow contents aren't valid.  */
 
-  mark_breakpoints_out ();
+  mark_breakpoints_out (current_program_space);
 
   /* The target reports the exec event to the main thread, even if
      some other thread does the exec, and even if the main thread was
diff --git a/gdb/target.c b/gdb/target.c
index ec75f952ea20..e72e22da69a3 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3628,7 +3628,7 @@ generic_mourn_inferior (void)
   /* Mark breakpoints uninserted in case something tries to delete a
      breakpoint while we delete the inferior's threads (which would
      fail, since the inferior is long gone).  */
-  mark_breakpoints_out ();
+  mark_breakpoints_out (inf->pspace);
 
   if (inf->pid != 0)
     exit_inferior (inf);
-- 
2.43.0

[PATCH v2 2/5] gdb: add inferior parameter to breakpoint_init_inferior

[Simon Marchi]

By inspection, I believe that breakpoint_init_inferior doesn't call
anything that relies on the current program space or inferior.  So,
add an inferior parameter, to make the current inferior / program space
references bubble up one level.

Change-Id: Ib07b7a6d360e324f6ae1aa502dd314b8cce421b7
---
 gdb/breakpoint.c | 22 +++++-----------------
 gdb/breakpoint.h | 12 +++++++++++-
 gdb/infcmd.c     | 10 +++++-----
 gdb/infrun.c     |  4 ++--
 gdb/target.c     |  2 +-
 5 files changed, 24 insertions(+), 26 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index d844ef7baf25..8d8e97400e37 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4226,33 +4226,21 @@ mark_breakpoints_out (program_space *pspace)
       bl->inserted = 0;
 }
 
-/* Clear the "inserted" flag in all breakpoints and delete any
-   breakpoints which should go away between runs of the program.
-
-   Plus other such housekeeping that has to be done for breakpoints
-   between runs.
-
-   Note: this function gets called at the end of a run (by
-   generic_mourn_inferior) and when a run begins (by
-   init_wait_for_inferior).  */
-
-
+/* See breakpoint.h.  */
 
 void
-breakpoint_init_inferior (enum inf_context context)
+breakpoint_init_inferior (inferior *inf, inf_context context)
 {
-  struct program_space *pspace = current_program_space;
-
   /* If breakpoint locations are shared across processes, then there's
      nothing to do.  */
-  if (gdbarch_has_global_breakpoints (current_inferior ()->arch ()))
+  if (gdbarch_has_global_breakpoints (inf->arch ()))
     return;
 
-  mark_breakpoints_out (pspace);
+  mark_breakpoints_out (inf->pspace);
 
   for (breakpoint &b : all_breakpoints_safe ())
     {
-      if (b.has_locations () && b.first_loc ().pspace != pspace)
+      if (b.has_locations () && b.first_loc ().pspace != inf->pspace)
 	continue;
 
       switch (b.type)
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index e7bebed2725d..8530a7127945 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1515,7 +1515,17 @@ extern struct breakpoint *clone_momentary_breakpoint (struct breakpoint *bpkt);
 
 extern void set_ignore_count (int, int, int);
 
-extern void breakpoint_init_inferior (enum inf_context);
+/* Clear the "inserted" flag in all breakpoint locations of INF's program space
+   and delete any breakpoints which should go away between runs of the program.
+
+   Plus other such housekeeping that has to be done for breakpoints
+   between runs.
+
+   Note: this function gets called at the end of a run (by
+   generic_mourn_inferior) and when a run begins (by
+   init_wait_for_inferior).  */
+
+extern void breakpoint_init_inferior (inferior *inf, inf_context context);
 
 extern void breakpoint_auto_delete (bpstat *);
 
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 54063b37beb8..4e17a6b06295 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2824,14 +2824,14 @@ detach_command (const char *args, int from_tty)
   /* Hold a strong reference to the target while (maybe)
      detaching the parent.  Otherwise detaching could close the
      target.  */
-  auto target_ref
-    = target_ops_ref::new_reference (current_inferior ()->process_target ());
+  inferior *inf = current_inferior ();
+  auto target_ref = target_ops_ref::new_reference (inf->process_target ());
 
   /* Save this before detaching, since detaching may unpush the
      process_stratum target.  */
   bool was_non_stop_p = target_is_non_stop_p ();
 
-  target_detach (current_inferior (), from_tty);
+  target_detach (inf, from_tty);
 
   update_previous_thread ();
 
@@ -2840,11 +2840,11 @@ detach_command (const char *args, int from_tty)
      this within target_detach because that is also used when
      following child forks, and in that case we will want to transfer
      breakpoints to the child, not delete them.  */
-  breakpoint_init_inferior (inf_exited);
+  breakpoint_init_inferior (inf, inf_exited);
 
   /* If the solist is global across inferiors, don't clear it when we
      detach from a single inferior.  */
-  if (!gdbarch_has_global_solist (current_inferior ()->arch ()))
+  if (!gdbarch_has_global_solist (inf->arch ()))
     no_shared_libraries (nullptr, from_tty);
 
   if (deprecated_detach_hook)
diff --git a/gdb/infrun.c b/gdb/infrun.c
index d00a98906d71..fa68ab8296e9 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1297,7 +1297,7 @@ follow_exec (ptid_t ptid, const char *exec_file_target)
   /* We've followed the inferior through an exec.  Therefore, the
      inferior has essentially been killed & reborn.  */
 
-  breakpoint_init_inferior (inf_execd);
+  breakpoint_init_inferior (current_inferior (), inf_execd);
 
   gdb::unique_xmalloc_ptr<char> exec_file_host
     = exec_file_find (exec_file_target, nullptr);
@@ -3819,7 +3819,7 @@ init_wait_for_inferior (void)
 {
   /* These are meaningless until the first time through wait_for_inferior.  */
 
-  breakpoint_init_inferior (inf_starting);
+  breakpoint_init_inferior (current_inferior (), inf_starting);
 
   clear_proceed_status (0);
 
diff --git a/gdb/target.c b/gdb/target.c
index e72e22da69a3..bbc1badc9e19 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3636,7 +3636,7 @@ generic_mourn_inferior (void)
   /* Note this wipes step-resume breakpoints, so needs to be done
      after exit_inferior, which ends up referencing the step-resume
      breakpoints through clear_thread_inferior_resources.  */
-  breakpoint_init_inferior (inf_exited);
+  breakpoint_init_inferior (inf, inf_exited);
 
   registers_changed ();
 
-- 
2.43.0

[PATCH v2 3/5] gdb: add program_space parameter to disable_breakpoints_in_shlibs

[Simon Marchi]

Make the current_program_space reference bubble up one level.

Change-Id: Ide917aa306bff1872d961244901d79f65d2da62e
---
 gdb/breakpoint.c  | 7 +++----
 gdb/breakpoint.h  | 5 ++++-
 gdb/solib.c       | 2 +-
 gdb/windows-nat.c | 2 +-
 4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 8d8e97400e37..5f05657a8b3e 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7972,11 +7972,10 @@ create_and_insert_solib_event_breakpoint (struct gdbarch *gdbarch, CORE_ADDR add
   return b;
 }
 
-/* Disable any breakpoints that are on code in shared libraries.  Only
-   apply to enabled breakpoints, disabled ones can just stay disabled.  */
+/* See breakpoint.h.  */
 
 void
-disable_breakpoints_in_shlibs (void)
+disable_breakpoints_in_shlibs (program_space *pspace)
 {
   for (bp_location *loc : all_bp_locations ())
     {
@@ -7992,7 +7991,7 @@ disable_breakpoints_in_shlibs (void)
 	   || (b->type == bp_jit_event)
 	   || (b->type == bp_hardware_breakpoint)
 	   || (is_tracepoint (b)))
-	  && loc->pspace == current_program_space
+	  && loc->pspace == pspace
 	  && !loc->shlib_disabled
 	  && solib_name_from_address (loc->pspace, loc->address)
 	  )
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 8530a7127945..226e4d06993e 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1802,7 +1802,10 @@ extern void remove_solib_event_breakpoints (void);
    delete at next stop disposition.  */
 extern void remove_solib_event_breakpoints_at_next_stop (void);
 
-extern void disable_breakpoints_in_shlibs (void);
+/* Disable any breakpoints that are on code in shared libraries in PSPACE.
+   Only apply to enabled breakpoints, disabled ones can just stay disabled.  */
+
+extern void disable_breakpoints_in_shlibs (program_space *pspace);
 
 /* This function returns true if B is a catchpoint.  */
 
diff --git a/gdb/solib.c b/gdb/solib.c
index 0a888430cf9b..98cda039a833 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -1187,7 +1187,7 @@ clear_solib (void)
 {
   const solib_ops *ops = gdbarch_so_ops (current_inferior ()->arch ());
 
-  disable_breakpoints_in_shlibs ();
+  disable_breakpoints_in_shlibs (current_program_space);
 
   current_program_space->so_list.clear_and_dispose ([] (solib *so) {
     notify_solib_unloaded (current_program_space, *so);
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 48b0d10d24c7..7f3044fc61de 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -1906,7 +1906,7 @@ windows_nat_target::do_initial_windows_stuff (DWORD pid, bool attaching)
   inf = current_inferior ();
   if (!inf->target_is_pushed (this))
     inf->push_target (this);
-  disable_breakpoints_in_shlibs ();
+  disable_breakpoints_in_shlibs (current_program_space);
   windows_clear_solib ();
   clear_proceed_status (0);
   init_wait_for_inferior ();
-- 
2.43.0

[PATCH v2 4/5] gdb: add program_space parameter to clear_solib

[Simon Marchi]

Make the current_program_space reference bubble up one level.

Remove one unnecessary declaration of clear_solib.

Change-Id: I234e2c8c0b71713364fc7b76cee2bee2b026bd6d
---
 gdb/corelow.c |  2 +-
 gdb/solib.c   | 16 ++++++++--------
 gdb/solib.h   |  6 +++---
 gdb/symtab.h  |  4 ----
 4 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index 43c1f69b1317..f291b2aba191 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -335,7 +335,7 @@ core_target::clear_core ()
 
       /* Clear out solib state while the bfd is still open.  See
 	 comments in clear_solib in solib.c.  */
-      clear_solib ();
+      clear_solib (current_program_space);
 
       current_program_space->cbfd.reset (nullptr);
     }
diff --git a/gdb/solib.c b/gdb/solib.c
index 98cda039a833..991ff156d12f 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -1180,23 +1180,23 @@ solib_keep_data_in_core (CORE_ADDR vaddr, unsigned long size)
     return false;
 }
 
-/* Called by free_all_symtabs */
+/* See solib.h.  */
 
 void
-clear_solib (void)
+clear_solib (program_space *pspace)
 {
   const solib_ops *ops = gdbarch_so_ops (current_inferior ()->arch ());
 
-  disable_breakpoints_in_shlibs (current_program_space);
+  disable_breakpoints_in_shlibs (pspace);
 
-  current_program_space->so_list.clear_and_dispose ([] (solib *so) {
-    notify_solib_unloaded (current_program_space, *so);
-    current_program_space->remove_target_sections (so);
+  pspace->so_list.clear_and_dispose ([pspace] (solib *so) {
+    notify_solib_unloaded (pspace, *so);
+    pspace->remove_target_sections (so);
     delete so;
   });
 
   if (ops->clear_solib != nullptr)
-    ops->clear_solib (current_program_space);
+    ops->clear_solib (pspace);
 }
 
 /* Shared library startup support.  When GDB starts up the inferior,
@@ -1244,7 +1244,7 @@ no_shared_libraries (const char *ignored, int from_tty)
      access to their associated objfiles.  Therefore, we can not purge the
      solibs' objfiles before clear_solib has been called.  */
 
-  clear_solib ();
+  clear_solib (current_program_space);
   objfile_purge_solibs ();
 }
 
diff --git a/gdb/solib.h b/gdb/solib.h
index 69183278318b..f7a93c0718f0 100644
--- a/gdb/solib.h
+++ b/gdb/solib.h
@@ -42,10 +42,10 @@ extern bool debug_solib;
 #define SOLIB_SCOPED_DEBUG_START_END(fmt, ...) \
   scoped_debug_start_end (debug_solib, "solib", fmt, ##__VA_ARGS__)
 
-/* Called when we free all symtabs, to free the shared library information
-   as well.  */
+/* Called when we free all symtabs of PSPACE, to free the shared library
+   information as well.  */
 
-extern void clear_solib (void);
+extern void clear_solib (program_space *pspace);
 
 /* Called to add symbols from a shared library to gdb's symbol table.  */
 
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 3b067a77b3c8..ca5a5b0f7fde 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2423,10 +2423,6 @@ extern bool find_line_pc_range (struct symtab_and_line, CORE_ADDR *,
 
 extern void resolve_sal_pc (struct symtab_and_line *);
 
-/* solib.c */
-
-extern void clear_solib (void);
-
 /* The reason we're calling into a completion match list collector
    function.  */
 enum class complete_symbol_mode
-- 
2.43.0

[PATCH v2 5/5] gdb: remove unnecessary nullptr check in remove_user_added_objfile

[Simon Marchi]

Since commit 74daa597e74 ("gdb: add all_objfiles_removed observer"), the
objfile passed to the free_objfile observable can't be nullptr.

Change-Id: If215aa051ab43c068b11746938022c7efca09caa
---
 gdb/solib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/solib.c b/gdb/solib.c
index 991ff156d12f..952897c37fc9 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -1695,7 +1695,7 @@ gdb_bfd_lookup_symbol (bfd *abfd,
 static void
 remove_user_added_objfile (struct objfile *objfile)
 {
-  if (objfile != 0 && objfile->flags & OBJF_USERLOADED)
+  if (objfile->flags & OBJF_USERLOADED)
     {
       for (solib &so : objfile->pspace->solibs ())
 	if (so.objfile == objfile)
-- 
2.43.0

Re: [PATCH v2 4/5] gdb: add program_space parameter to clear_solib

[Andrew Burgess]

Simon Marchi <simon.marchi@efficios.com> writes:

> Make the current_program_space reference bubble up one level.
>
> Remove one unnecessary declaration of clear_solib.
>
> Change-Id: I234e2c8c0b71713364fc7b76cee2bee2b026bd6d
> ---
>  gdb/corelow.c |  2 +-
>  gdb/solib.c   | 16 ++++++++--------
>  gdb/solib.h   |  6 +++---
>  gdb/symtab.h  |  4 ----
>  4 files changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index 43c1f69b1317..f291b2aba191 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -335,7 +335,7 @@ core_target::clear_core ()
>  
>        /* Clear out solib state while the bfd is still open.  See
>  	 comments in clear_solib in solib.c.  */
> -      clear_solib ();
> +      clear_solib (current_program_space);
>  
>        current_program_space->cbfd.reset (nullptr);
>      }
> diff --git a/gdb/solib.c b/gdb/solib.c
> index 98cda039a833..991ff156d12f 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -1180,23 +1180,23 @@ solib_keep_data_in_core (CORE_ADDR vaddr, unsigned long size)
>      return false;
>  }
>  
> -/* Called by free_all_symtabs */
> +/* See solib.h.  */
>  
>  void
> -clear_solib (void)
> +clear_solib (program_space *pspace)
>  {
>    const solib_ops *ops = gdbarch_so_ops (current_inferior ()->arch ());
>  
> -  disable_breakpoints_in_shlibs (current_program_space);

In the previous patch, when you added this use of current_program_space,
I considered commenting that you should instead do:

  disable_breakpoints_in_shlibs (current_inferior ()->pspace);

because we already use current_inferior() in this function, and the
current_program_space is really a property of the current_inferior.

But I figured such a comment was a bit trivial, and didn't make much
difference.

But with this patch I think instead of passing the PSPACE argument we
should be passing an 'inferior *inf' argument here, which we'd then
forward to the gdbarch_so_ops call, and then use inf->pspace instead of
PSPACE throughout this function.

What are your thoughts?

Thanks,
Andrew

> +  disable_breakpoints_in_shlibs (pspace);
>  
> -  current_program_space->so_list.clear_and_dispose ([] (solib *so) {
> -    notify_solib_unloaded (current_program_space, *so);
> -    current_program_space->remove_target_sections (so);
> +  pspace->so_list.clear_and_dispose ([pspace] (solib *so) {
> +    notify_solib_unloaded (pspace, *so);
> +    pspace->remove_target_sections (so);
>      delete so;
>    });
>  
>    if (ops->clear_solib != nullptr)
> -    ops->clear_solib (current_program_space);
> +    ops->clear_solib (pspace);
>  }
>  
>  /* Shared library startup support.  When GDB starts up the inferior,
> @@ -1244,7 +1244,7 @@ no_shared_libraries (const char *ignored, int from_tty)
>       access to their associated objfiles.  Therefore, we can not purge the
>       solibs' objfiles before clear_solib has been called.  */
>  
> -  clear_solib ();
> +  clear_solib (current_program_space);
>    objfile_purge_solibs ();
>  }
>  
> diff --git a/gdb/solib.h b/gdb/solib.h
> index 69183278318b..f7a93c0718f0 100644
> --- a/gdb/solib.h
> +++ b/gdb/solib.h
> @@ -42,10 +42,10 @@ extern bool debug_solib;
>  #define SOLIB_SCOPED_DEBUG_START_END(fmt, ...) \
>    scoped_debug_start_end (debug_solib, "solib", fmt, ##__VA_ARGS__)
>  
> -/* Called when we free all symtabs, to free the shared library information
> -   as well.  */
> +/* Called when we free all symtabs of PSPACE, to free the shared library
> +   information as well.  */
>  
> -extern void clear_solib (void);
> +extern void clear_solib (program_space *pspace);
>  
>  /* Called to add symbols from a shared library to gdb's symbol table.  */
>  
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index 3b067a77b3c8..ca5a5b0f7fde 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -2423,10 +2423,6 @@ extern bool find_line_pc_range (struct symtab_and_line, CORE_ADDR *,
>  
>  extern void resolve_sal_pc (struct symtab_and_line *);
>  
> -/* solib.c */
> -
> -extern void clear_solib (void);
> -
>  /* The reason we're calling into a completion match list collector
>     function.  */
>  enum class complete_symbol_mode
> -- 
> 2.43.0

Re: [PATCH v2 0/5] Random cleanup patches

[Andrew Burgess]

Simon Marchi <simon.marchi@efficios.com> writes:

> This is v2 of:
>
> https://inbox.sourceware.org/gdb-patches/20240206171514.119244-1-simon.marchi@efficios.com/T/#m396f40ecb617db9e71beae00f82f21bae92a36cd
>
> The only difference is to revert using the current inferior in patch 4,
> as described here:
>
> https://inbox.sourceware.org/gdb-patches/20240206171514.119244-1-simon.marchi@efficios.com/T/#mb86bb48b17bdb01e5c0c28609aa168cb25722958

So I didn't read V1 of this series prior to commenting on patch #4.
Having been back to re-read the V1 series, and the problems that you ran
into, I still think passing the current_inferior() would be a better
choice in #4.

For everything else:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

Re: [PATCH v2 4/5] gdb: add program_space parameter to clear_solib

[Simon Marchi]

On 2/8/24 09:29, Andrew Burgess wrote:
> Simon Marchi <simon.marchi@efficios.com> writes:
> 
>> Make the current_program_space reference bubble up one level.
>>
>> Remove one unnecessary declaration of clear_solib.
>>
>> Change-Id: I234e2c8c0b71713364fc7b76cee2bee2b026bd6d
>> ---
>>  gdb/corelow.c |  2 +-
>>  gdb/solib.c   | 16 ++++++++--------
>>  gdb/solib.h   |  6 +++---
>>  gdb/symtab.h  |  4 ----
>>  4 files changed, 12 insertions(+), 16 deletions(-)
>>
>> diff --git a/gdb/corelow.c b/gdb/corelow.c
>> index 43c1f69b1317..f291b2aba191 100644
>> --- a/gdb/corelow.c
>> +++ b/gdb/corelow.c
>> @@ -335,7 +335,7 @@ core_target::clear_core ()
>>  
>>        /* Clear out solib state while the bfd is still open.  See
>>  	 comments in clear_solib in solib.c.  */
>> -      clear_solib ();
>> +      clear_solib (current_program_space);
>>  
>>        current_program_space->cbfd.reset (nullptr);
>>      }
>> diff --git a/gdb/solib.c b/gdb/solib.c
>> index 98cda039a833..991ff156d12f 100644
>> --- a/gdb/solib.c
>> +++ b/gdb/solib.c
>> @@ -1180,23 +1180,23 @@ solib_keep_data_in_core (CORE_ADDR vaddr, unsigned long size)
>>      return false;
>>  }
>>  
>> -/* Called by free_all_symtabs */
>> +/* See solib.h.  */
>>  
>>  void
>> -clear_solib (void)
>> +clear_solib (program_space *pspace)
>>  {
>>    const solib_ops *ops = gdbarch_so_ops (current_inferior ()->arch ());
>>  
>> -  disable_breakpoints_in_shlibs (current_program_space);
> 
> In the previous patch, when you added this use of current_program_space,
> I considered commenting that you should instead do:
> 
>   disable_breakpoints_in_shlibs (current_inferior ()->pspace);
> 
> because we already use current_inferior() in this function, and the
> current_program_space is really a property of the current_inferior.

Most of the time they match, but not always, unfortunately (otherwise,
we would not need the current_program_space global, we could always use
current_inferior()->pspace).

> But I figured such a comment was a bit trivial, and didn't make much
> difference.

As explained in my self-reply on v1, I think that clear_solib is called
with a program space (or with the current program space, prior to the
patch) that doesn't match `current_inferior ()->pspace`.  So I don't
think that doing:

    disable_breakpoints_in_shlibs (current_inferior ()->pspace);

would do what we want.  It would call disable_breakpoints_in_shlibs with
the wrong pspace.

> But with this patch I think instead of passing the PSPACE argument we
> should be passing an 'inferior *inf' argument here, which we'd then
> forward to the gdbarch_so_ops call, and then use inf->pspace instead of
> PSPACE throughout this function.
> 
> What are your thoughts?

The solib operations are really a per-pspace thing, so I think it makes
sense to pass a pspace.  The inferior doesn't need to be involved.  The
only reason why we need the inferior here is to get an arch, to get the
solib_ops implementation used for that pspace, to be able to tell it:
clear all you know about that pspace.

In the series that I have in the pipeline, a first step is to add a
pspace -> solib_ops backlink to indicate what is the solib_ops used to
provide the solibs for that pspace.  That will remove the need for the
current_inferior call here, and should make that function completely
inferior-agnostic.  A second step (not relevant for this discussion, but
just FYI) will be to make it possible for multiple solib_ops to provide
solibs for a given pspace.  That will be useful for heterogenous
programs where you have CPU code and GPU code loaded in the same address
space.  The pspace -> solib_ops backlink will then become a list of
solib_ops pointers.

Simon

Re: [PATCH v2 4/5] gdb: add program_space parameter to clear_solib

[Andrew Burgess]

Simon Marchi <simon.marchi@efficios.com> writes:

> On 2/8/24 09:29, Andrew Burgess wrote:
>> Simon Marchi <simon.marchi@efficios.com> writes:
>> 
>>> Make the current_program_space reference bubble up one level.
>>>
>>> Remove one unnecessary declaration of clear_solib.
>>>
>>> Change-Id: I234e2c8c0b71713364fc7b76cee2bee2b026bd6d
>>> ---
>>>  gdb/corelow.c |  2 +-
>>>  gdb/solib.c   | 16 ++++++++--------
>>>  gdb/solib.h   |  6 +++---
>>>  gdb/symtab.h  |  4 ----
>>>  4 files changed, 12 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/gdb/corelow.c b/gdb/corelow.c
>>> index 43c1f69b1317..f291b2aba191 100644
>>> --- a/gdb/corelow.c
>>> +++ b/gdb/corelow.c
>>> @@ -335,7 +335,7 @@ core_target::clear_core ()
>>>  
>>>        /* Clear out solib state while the bfd is still open.  See
>>>  	 comments in clear_solib in solib.c.  */
>>> -      clear_solib ();
>>> +      clear_solib (current_program_space);
>>>  
>>>        current_program_space->cbfd.reset (nullptr);
>>>      }
>>> diff --git a/gdb/solib.c b/gdb/solib.c
>>> index 98cda039a833..991ff156d12f 100644
>>> --- a/gdb/solib.c
>>> +++ b/gdb/solib.c
>>> @@ -1180,23 +1180,23 @@ solib_keep_data_in_core (CORE_ADDR vaddr, unsigned long size)
>>>      return false;
>>>  }
>>>  
>>> -/* Called by free_all_symtabs */
>>> +/* See solib.h.  */
>>>  
>>>  void
>>> -clear_solib (void)
>>> +clear_solib (program_space *pspace)
>>>  {
>>>    const solib_ops *ops = gdbarch_so_ops (current_inferior ()->arch ());
>>>  
>>> -  disable_breakpoints_in_shlibs (current_program_space);
>> 
>> In the previous patch, when you added this use of current_program_space,
>> I considered commenting that you should instead do:
>> 
>>   disable_breakpoints_in_shlibs (current_inferior ()->pspace);
>> 
>> because we already use current_inferior() in this function, and the
>> current_program_space is really a property of the current_inferior.
>
> Most of the time they match, but not always, unfortunately (otherwise,
> we would not need the current_program_space global, we could always use
> current_inferior()->pspace).
>
>> But I figured such a comment was a bit trivial, and didn't make much
>> difference.
>
> As explained in my self-reply on v1, I think that clear_solib is called
> with a program space (or with the current program space, prior to the
> patch) that doesn't match `current_inferior ()->pspace`.  So I don't
> think that doing:
>
>     disable_breakpoints_in_shlibs (current_inferior ()->pspace);
>
> would do what we want.  It would call disable_breakpoints_in_shlibs with
> the wrong pspace.
>
>> But with this patch I think instead of passing the PSPACE argument we
>> should be passing an 'inferior *inf' argument here, which we'd then
>> forward to the gdbarch_so_ops call, and then use inf->pspace instead of
>> PSPACE throughout this function.
>> 
>> What are your thoughts?
>
> The solib operations are really a per-pspace thing, so I think it makes
> sense to pass a pspace.  The inferior doesn't need to be involved.  The
> only reason why we need the inferior here is to get an arch, to get the
> solib_ops implementation used for that pspace, to be able to tell it:
> clear all you know about that pspace.
>
> In the series that I have in the pipeline, a first step is to add a
> pspace -> solib_ops backlink to indicate what is the solib_ops used to
> provide the solibs for that pspace.  That will remove the need for the
> current_inferior call here, and should make that function completely
> inferior-agnostic.  A second step (not relevant for this discussion, but
> just FYI) will be to make it possible for multiple solib_ops to provide
> solibs for a given pspace.  That will be useful for heterogenous
> programs where you have CPU code and GPU code loaded in the same address
> space.  The pspace -> solib_ops backlink will then become a list of
> solib_ops pointers.

Thanks for adding this last part.  This was what worried me about the
idea of a pspace -> solib_ops link, multiple gdbarch within a single
program_space.

So I'm convinced that this is the right way to go.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

Re: [PATCH v2 4/5] gdb: add program_space parameter to clear_solib

[Simon Marchi]

On 2024-02-09 09:59, Andrew Burgess wrote:
> Thanks for adding this last part.  This was what worried me about the
> idea of a pspace -> solib_ops link, multiple gdbarch within a single
> program_space.
> 
> So I'm convinced that this is the right way to go.

Cool, thanks.

Just to be clear, we can have multiple gdbarches within a pspace today
(e.g. the now-removed SPU port), we just can't have them specify
different solib_ops.  Just moving to have a single pspace -> solib_ops
link wouldn't be a regression, it would just make what we have now more
explicit.

Simon

Re: [PATCH v2 0/5] Random cleanup patches

[Simon Marchi]

On 2024-02-08 09:39, Andrew Burgess wrote:
> Simon Marchi <simon.marchi@efficios.com> writes:
> 
>> This is v2 of:
>>
>> https://inbox.sourceware.org/gdb-patches/20240206171514.119244-1-simon.marchi@efficios.com/T/#m396f40ecb617db9e71beae00f82f21bae92a36cd
>>
>> The only difference is to revert using the current inferior in patch 4,
>> as described here:
>>
>> https://inbox.sourceware.org/gdb-patches/20240206171514.119244-1-simon.marchi@efficios.com/T/#mb86bb48b17bdb01e5c0c28609aa168cb25722958
> 
> So I didn't read V1 of this series prior to commenting on patch #4.
> Having been back to re-read the V1 series, and the problems that you ran
> into, I still think passing the current_inferior() would be a better
> choice in #4.
> 
> For everything else:
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>

The comments on patch #4 were resolved, so I'll push that series now.
Thanks!

Simon