[PATCH v3 0/3] [gdb] Fix segfault in for_each_block

[PATCH v3 0/3] [gdb] Fix segfault in for_each_block

[Tom de Vries]

This patch series fixes PR gdb/30547, a segfault when running test-case
gdb.base/vfork-follow-parent.exp on powerpc64 (likewise on s390x).

There are three patches, the first and third by themselves sufficient to no
longer trigger the segfault.

The second is a slight refactoring which is user visible, so it comes as
a separate patch.

The root cause of the problem is that linux_is_uclinux, and consequently
gdbarch_has_shared_address_space returns an incorrect value.

The first patch makes gdb more robust against gdbarch_has_shared_address_space
returning incorrect values, by eliminating a call to it.

The third patch addresses the root cause.

Tested on top of trunk on x86_64-linux and ppc64le-linux.
Tested on top of gdb-14-branch on ppc64-linux.

[ I used gdb-14-branch for ppc64-linux, because I can't build trunk anymore
with system gcc 4.8.5 (CentOS-7), due to the recent c++17 requirement (and
just before that, some gcc bug in atomic support), and that's all I have
readily available on that machine. ]

There is still scope to fix things further.

When I started to investigate, I noticed that I only ran into the segfault on
ppc64 and s390x, two big-endian architectures, so I sort of expected to find an
endian-related problem.

Instead, the problem was ppc_linux_target_wordsize returning 4 instead of 8,
which causes gdb to interpret the 8-byte entry auxv vector using 4-byte
words, causing an incorrect linux_is_uclinux == true.

The same problem happens on ppc64le (ppc_linux_target_wordsize returns 4),
it's just that the incorrect word size doesn't change the outcome of:
- target_auxv_search (AT_NULL, &dummy) == 1, and
- target_auxv_search (AT_PAGESZ, &dummy) == 1
so linux_is_uclinux returns false, as it should.

This suggest a too forgiving parsing of the auxv vector, which should be made
more strict.

Finally, it should be fixed that ppc_linux_target_wordsize returns 4 in a
process with wordsize == 8.

I've file PRs for these two issues:
- [gdb] Make auxv parsing more strict
  https://sourceware.org/bugzilla/show_bug.cgi?id=31040
- [gdb/tdep] ppc_linux_target_wordsize silently returns wrong result
  https://sourceware.org/bugzilla/show_bug.cgi?id=31038

For now my interest is to backport at least one, possibly both patches from
this series to fix this PR on the gdb 14 release branch (and the 13.2 based
distro packages I maintain).

Submission history:

v3:
- fix std::shared_ptr usage issues after review by Simon
- add patch "[gdb] Eliminate local var pspace in inferior.c"
  following up on comments by Simon
- update third patch according to comments by Simon

v2:
- use std::shared_ptr as suggested by Andrew and Simon

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30547

Tom de Vries (3):
  [gdb] Fix segfault in for_each_block, part 1
  [gdb] Eliminate local var pspace in inferior.c
  [gdb] Fix segfault in for_each_block, part 2

 gdb/breakpoint.c                              | 29 ++++++++-------
 gdb/inferior.c                                | 24 ++++--------
 gdb/inferior.h                                |  2 +-
 gdb/infrun.c                                  | 37 +++++++++++--------
 gdb/linux-nat.c                               |  2 +-
 gdb/nat/ppc-linux.c                           |  2 +
 gdb/ppc-linux-nat.c                           |  2 +
 gdb/process-stratum-target.c                  |  2 +-
 gdb/progspace.c                               | 21 ++++-------
 gdb/progspace.h                               |  6 +--
 gdb/record-btrace.c                           |  2 +-
 gdb/regcache.c                                |  2 +-
 gdb/s390-linux-nat.c                          |  5 ++-
 gdb/scoped-mock-context.h                     |  2 +-
 gdb/target-dcache.c                           | 11 +++---
 .../gdb.python/py-progspace-events.exp        |  4 +-
 16 files changed, 78 insertions(+), 75 deletions(-)


base-commit: e7d74879870833fb0c42282da940e60fee13948e
-- 
2.35.3

[PATCH v3 1/3] [gdb] Fix segfault in for_each_block, part 1

[Tom de Vries]

When running test-case gdb.base/vfork-follow-parent.exp on powerpc64 (likewise
on s390x), I run into:
...
(gdb) PASS: gdb.base/vfork-follow-parent.exp: \
  exec_file=vfork-follow-parent-exit: target-non-stop=on: non-stop=off: \
  resolution_method=schedule-multiple: print unblock_parent = 1
continue^M
Continuing.^M
Reading symbols from vfork-follow-parent-exit...^M
^M
^M
Fatal signal: Segmentation fault^M
----- Backtrace -----^M
0x1027d3e7 gdb_internal_backtrace_1^M
        src/gdb/bt-utils.c:122^M
0x1027d54f _Z22gdb_internal_backtracev^M
        src/gdb/bt-utils.c:168^M
0x1057643f handle_fatal_signal^M
        src/gdb/event-top.c:889^M
0x10576677 handle_sigsegv^M
        src/gdb/event-top.c:962^M
0x3fffa7610477 ???^M
0x103f2144 for_each_block^M
        src/gdb/dcache.c:199^M
0x103f235b _Z17dcache_invalidateP13dcache_struct^M
        src/gdb/dcache.c:251^M
0x10bde8c7 _Z24target_dcache_invalidatev^M
        src/gdb/target-dcache.c:50^M
...
or similar.

The root cause for the segmentation fault is that linux_is_uclinux gives an
incorrect result: it should always return false, given that we're running on a
regular linux system, but instead it returns first true, then false.

In more detail, the segmentation fault happens as follows:
- a program space with an address space is created
- a second program space is about to be created. maybe_new_address_space
  is called, and because linux_is_uclinux returns true, maybe_new_address_space
  returns false, and no new address space is created
- a second program space with the same address space is created
- a program space is deleted. Because linux_is_uclinux now returns false,
  gdbarch_has_shared_address_space (current_inferior ()->arch ()) returns
  false, and the address space is deleted
- when gdb uses the address space of the remaining program space, we run into
  the segfault, because the address space is deleted.

Hardcoding linux_is_uclinux to false makes the test-case pass.

We leave addressing the root cause for the following commit in this series.

For now, prevent the segmentation fault by making the address space a refcounted
object.

This was already suggested here [1]:
...
A better solution might be to have the address spaces be reference counted
...

Tested on top of trunk on x86_64-linux and ppc64le-linux.
Tested on top of gdb-14-branch on ppc64-linux.

PR gdb/30547
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30547

[1] https://sourceware.org/pipermail/gdb-patches/2023-October/202928.html
---
 gdb/breakpoint.c             | 29 ++++++++++++++++-------------
 gdb/inferior.c               |  8 ++------
 gdb/inferior.h               |  2 +-
 gdb/infrun.c                 | 21 +++++++++++----------
 gdb/linux-nat.c              |  2 +-
 gdb/process-stratum-target.c |  2 +-
 gdb/progspace.c              | 21 ++++++++-------------
 gdb/progspace.h              |  6 +++---
 gdb/record-btrace.c          |  2 +-
 gdb/regcache.c               |  2 +-
 gdb/scoped-mock-context.h    |  2 +-
 gdb/target-dcache.c          | 11 ++++++-----
 12 files changed, 52 insertions(+), 56 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index fe09dbcbeee..0b086fbeab3 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1723,7 +1723,7 @@ one_breakpoint_xfer_memory (gdb_byte *readbuf, gdb_byte *writebuf,
   int bptoffset = 0;
 
   if (!breakpoint_address_match (target_info->placed_address_space, 0,
-				 current_program_space->aspace, 0))
+				 current_program_space->aspace.get (), 0))
     {
       /* The breakpoint is inserted in a different address space.  */
       return;
@@ -2409,7 +2409,7 @@ should_be_inserted (struct bp_location *bl)
      a breakpoint.  */
   if ((bl->loc_type == bp_loc_software_breakpoint
        || bl->loc_type == bp_loc_hardware_breakpoint)
-      && stepping_past_instruction_at (bl->pspace->aspace,
+      && stepping_past_instruction_at (bl->pspace->aspace.get (),
 				       bl->address)
       /* The single-step breakpoint may be inserted at the location
 	 we're trying to step if the instruction branches to itself.
@@ -2847,7 +2847,7 @@ insert_bp_location (struct bp_location *bl,
      read the breakpoint instead of returning the data saved in
      the breakpoint location's shadow contents.  */
   bl->target_info.reqstd_address = bl->address;
-  bl->target_info.placed_address_space = bl->pspace->aspace;
+  bl->target_info.placed_address_space = bl->pspace->aspace.get ();
   bl->target_info.length = bl->length;
 
   /* When working with target-side conditions, we must pass all the conditions
@@ -4429,7 +4429,7 @@ bp_location_inserted_here_p (const struct bp_location *bl,
 			     const address_space *aspace, CORE_ADDR pc)
 {
   if (bl->inserted
-      && breakpoint_address_match (bl->pspace->aspace, bl->address,
+      && breakpoint_address_match (bl->pspace->aspace.get (), bl->address,
 				   aspace, pc))
     {
       /* An unmapped overlay can't be a match.  */
@@ -4508,7 +4508,7 @@ hardware_watchpoint_inserted_in_range (const address_space *aspace,
 	continue;
 
       for (bp_location &loc : bpt.locations ())
-	if (loc.pspace->aspace == aspace && loc.inserted)
+	if (loc.pspace->aspace.get () == aspace && loc.inserted)
 	  {
 	    CORE_ADDR l, h;
 
@@ -7330,10 +7330,10 @@ breakpoint_location_address_match (struct bp_location *bl,
 				   const address_space *aspace,
 				   CORE_ADDR addr)
 {
-  return (breakpoint_address_match (bl->pspace->aspace, bl->address,
+  return (breakpoint_address_match (bl->pspace->aspace.get (), bl->address,
 				    aspace, addr)
 	  || (bl->length
-	      && breakpoint_address_match_range (bl->pspace->aspace,
+	      && breakpoint_address_match_range (bl->pspace->aspace.get (),
 						 bl->address, bl->length,
 						 aspace, addr)));
 }
@@ -7350,7 +7350,7 @@ breakpoint_location_address_range_overlap (struct bp_location *bl,
 					   CORE_ADDR addr, int len)
 {
   if (gdbarch_has_global_breakpoints (current_inferior ()->arch ())
-      || bl->pspace->aspace == aspace)
+      || bl->pspace->aspace.get () == aspace)
     {
       int bl_len = bl->length != 0 ? bl->length : 1;
 
@@ -7407,8 +7407,10 @@ breakpoint_locations_match (const struct bp_location *loc1,
     /* We compare bp_location.length in order to cover ranged
        breakpoints.  Keep this in sync with
        bp_location_is_less_than.  */
-    return (breakpoint_address_match (loc1->pspace->aspace, loc1->address,
-				     loc2->pspace->aspace, loc2->address)
+    return (breakpoint_address_match (loc1->pspace->aspace.get (),
+				      loc1->address,
+				      loc2->pspace->aspace.get (),
+				      loc2->address)
 	    && (loc1->loc_type == loc2->loc_type || sw_hw_bps_match)
 	    && loc1->length == loc2->length);
 }
@@ -9568,8 +9570,9 @@ ranged_breakpoint::breakpoint_hit (const struct bp_location *bl,
       || ws.sig () != GDB_SIGNAL_TRAP)
     return 0;
 
-  return breakpoint_address_match_range (bl->pspace->aspace, bl->address,
-					 bl->length, aspace, bp_addr);
+  return breakpoint_address_match_range (bl->pspace->aspace.get (),
+					 bl->address, bl->length, aspace,
+					 bp_addr);
 }
 
 /* Implement the "resources_needed" method for ranged breakpoints.  */
@@ -12015,7 +12018,7 @@ code_breakpoint::breakpoint_hit (const struct bp_location *bl,
       || ws.sig () != GDB_SIGNAL_TRAP)
     return 0;
 
-  if (!breakpoint_address_match (bl->pspace->aspace, bl->address,
+  if (!breakpoint_address_match (bl->pspace->aspace.get (), bl->address,
 				 aspace, bp_addr))
     return 0;
 
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 1778723863e..37b4fad6e44 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -843,15 +843,13 @@ remove_inferior_command (const char *args, int from_tty)
 struct inferior *
 add_inferior_with_spaces (void)
 {
-  struct address_space *aspace;
   struct program_space *pspace;
   struct inferior *inf;
 
   /* If all inferiors share an address space on this system, this
      doesn't really return a new address space; otherwise, it
      really does.  */
-  aspace = maybe_new_address_space ();
-  pspace = new program_space (aspace);
+  pspace = new program_space (maybe_new_address_space ());
   inf = add_inferior (0);
   inf->pspace = pspace;
   inf->aspace = pspace->aspace;
@@ -1014,15 +1012,13 @@ clone_inferior_command (const char *args, int from_tty)
 
   for (i = 0; i < copies; ++i)
     {
-      struct address_space *aspace;
       struct program_space *pspace;
       struct inferior *inf;
 
       /* If all inferiors share an address space on this system, this
 	 doesn't really return a new address space; otherwise, it
 	 really does.  */
-      aspace = maybe_new_address_space ();
-      pspace = new program_space (aspace);
+      pspace = new program_space (maybe_new_address_space ());
       inf = add_inferior (0);
       inf->pspace = pspace;
       inf->aspace = pspace->aspace;
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 33eff7a9141..db75d6b5ecb 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -584,7 +584,7 @@ class inferior : public refcounted_object,
   bool removable = false;
 
   /* The address space bound to this inferior.  */
-  struct address_space *aspace = NULL;
+  std::shared_ptr<address_space> aspace;
 
   /* The program space bound to this inferior.  */
   struct program_space *pspace = NULL;
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 4c7eb9be792..34c495097e5 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -529,8 +529,9 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
 	    }
 	  else
 	    {
-	      child_inf->aspace = new address_space ();
-	      child_inf->pspace = new program_space (child_inf->aspace);
+	      child_inf->pspace
+		= new program_space (std::make_shared<address_space> ());
+	      child_inf->aspace = child_inf->pspace->aspace;
 	      child_inf->removable = true;
 	      clone_program_space (child_inf->pspace, parent_inf->pspace);
 	    }
@@ -608,8 +609,9 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
 
 	  child_inf->aspace = parent_inf->aspace;
 	  child_inf->pspace = parent_inf->pspace;
-	  parent_inf->aspace = new address_space ();
-	  parent_inf->pspace = new program_space (parent_inf->aspace);
+	  parent_inf->pspace
+	    = new program_space (std::make_shared<address_space> ());
+	  parent_inf->aspace = parent_inf->pspace->aspace;
 	  clone_program_space (parent_inf->pspace, child_inf->pspace);
 
 	  /* The parent inferior is still the current one, so keep things
@@ -618,8 +620,9 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
 	}
       else
 	{
-	  child_inf->aspace = new address_space ();
-	  child_inf->pspace = new program_space (child_inf->aspace);
+	  child_inf->pspace
+	    = new program_space (std::make_shared<address_space> ());
+	  child_inf->aspace = child_inf->pspace->aspace;
 	  child_inf->removable = true;
 	  child_inf->symfile_flags = SYMFILE_NO_READ;
 	  clone_program_space (child_inf->pspace, parent_inf->pspace);
@@ -1029,7 +1032,6 @@ handle_vfork_child_exec_or_exit (int exec)
       if (vfork_parent->pending_detach)
 	{
 	  struct program_space *pspace;
-	  struct address_space *aspace;
 
 	  /* follow-fork child, detach-on-fork on.  */
 
@@ -1054,9 +1056,8 @@ handle_vfork_child_exec_or_exit (int exec)
 	     of" a hack.  */
 
 	  pspace = inf->pspace;
-	  aspace = inf->aspace;
-	  inf->aspace = nullptr;
 	  inf->pspace = nullptr;
+	  std::shared_ptr<address_space> aspace = std::move (inf->aspace);
 
 	  if (print_inferior_events)
 	    {
@@ -5905,7 +5906,7 @@ handle_inferior_event (struct execution_control_state *ecs)
 	      = get_thread_arch_aspace_regcache (parent_inf,
 						 ecs->ws.child_ptid (),
 						 gdbarch,
-						 parent_inf->aspace);
+						 parent_inf->aspace.get ());
 	    /* Read PC value of parent process.  */
 	    parent_pc = regcache_read_pc (regcache);
 
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 1c9756c18bd..ca6e57cdaf9 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4361,7 +4361,7 @@ linux_nat_target::thread_address_space (ptid_t ptid)
 
   inf = find_inferior_pid (this, pid);
   gdb_assert (inf != NULL);
-  return inf->aspace;
+  return inf->aspace.get ();
 }
 
 /* Return the cached value of the processor core for thread PTID.  */
diff --git a/gdb/process-stratum-target.c b/gdb/process-stratum-target.c
index 173c3ecad13..ebd47bb9282 100644
--- a/gdb/process-stratum-target.c
+++ b/gdb/process-stratum-target.c
@@ -37,7 +37,7 @@ process_stratum_target::thread_address_space (ptid_t ptid)
 		      "address space of thread %s\n"),
 		    target_pid_to_str (ptid).c_str ());
 
-  return inf->aspace;
+  return inf->aspace.get ();
 }
 
 struct gdbarch *
diff --git a/gdb/progspace.c b/gdb/progspace.c
index 839707e9d71..2d769134ac6 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -55,7 +55,7 @@ address_space::address_space ()
    return a pointer to an existing address space, in case inferiors
    share an address space on this target system.  */
 
-struct address_space *
+std::shared_ptr<address_space>
 maybe_new_address_space (void)
 {
   int shared_aspace
@@ -67,7 +67,7 @@ maybe_new_address_space (void)
       return program_spaces[0]->aspace;
     }
 
-  return new address_space ();
+  return std::make_shared<address_space> ();
 }
 
 /* Start counting over from scratch.  */
@@ -95,9 +95,9 @@ remove_program_space (program_space *pspace)
 
 /* See progspace.h.  */
 
-program_space::program_space (address_space *aspace_)
+program_space::program_space (std::shared_ptr<address_space> aspace_)
   : num (++last_program_space_num),
-    aspace (aspace_)
+       aspace (std::move (aspace_))
 {
   program_spaces.push_back (this);
   gdb::observers::new_program_space.notify (this);
@@ -122,8 +122,6 @@ program_space::~program_space ()
   /* Defer breakpoint re-set because we don't want to create new
      locations for this pspace which we're tearing down.  */
   clear_symtab_users (SYMFILE_DEFER_BP_RESET);
-  if (!gdbarch_has_shared_address_space (current_inferior ()->arch ()))
-    delete this->aspace;
 }
 
 /* See progspace.h.  */
@@ -411,18 +409,14 @@ update_address_spaces (void)
 
   if (shared_aspace)
     {
-      struct address_space *aspace = new address_space ();
+      auto aspace = std::make_shared<address_space> ();
 
-      delete current_program_space->aspace;
       for (struct program_space *pspace : program_spaces)
 	pspace->aspace = aspace;
     }
   else
     for (struct program_space *pspace : program_spaces)
-      {
-	delete pspace->aspace;
-	pspace->aspace = new address_space ();
-      }
+      pspace->aspace = std::make_shared<address_space> ();
 
   for (inferior *inf : all_inferiors ())
     if (gdbarch_has_global_solist (current_inferior ()->arch ()))
@@ -459,5 +453,6 @@ initialize_progspace (void)
      modules have done that.  Do this before
      initialize_current_architecture, because that accesses the ebfd
      of current_program_space.  */
-  current_program_space = new program_space (new address_space ());
+  current_program_space
+    = new program_space (std::make_shared<address_space> ());
 }
diff --git a/gdb/progspace.h b/gdb/progspace.h
index a22e427400e..c169690681b 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -192,7 +192,7 @@ struct program_space
 {
   /* Constructs a new empty program space, binds it to ASPACE, and
      adds it to the program space list.  */
-  explicit program_space (address_space *aspace);
+  explicit program_space (std::shared_ptr<address_space> aspace);
 
   /* Releases a program space, and all its contents (shared libraries,
      objfiles, and any other references to the program space in other
@@ -334,7 +334,7 @@ struct program_space
      are global, then this field is ignored (we don't currently
      support inferiors sharing a program space if the target doesn't
      make breakpoints global).  */
-  struct address_space *aspace = NULL;
+  std::shared_ptr<address_space> aspace;
 
   /* True if this program space's section offsets don't yet represent
      the final offsets of the "live" address space (that is, the
@@ -445,7 +445,7 @@ class scoped_restore_current_program_space
 /* Maybe create a new address space object, and add it to the list, or
    return a pointer to an existing address space, in case inferiors
    share an address space.  */
-extern struct address_space *maybe_new_address_space (void);
+extern std::shared_ptr<address_space> maybe_new_address_space (void);
 
 /* Update all program spaces matching to address spaces.  The user may
    have created several program spaces, and loaded executables into
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index abab79f3132..4a1ecdd1ad6 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -2321,7 +2321,7 @@ record_btrace_replay_at_breakpoint (struct thread_info *tp)
   if (insn == NULL)
     return 0;
 
-  return record_check_stopped_by_breakpoint (tp->inf->aspace, insn->pc,
+  return record_check_stopped_by_breakpoint (tp->inf->aspace.get (), insn->pc,
 					     &btinfo->stop_reason);
 }
 
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 2e48c025809..239656d1987 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1628,7 +1628,7 @@ get_thread_arch_aspace_regcache_and_check (inferior *inf_for_target_calls,
      the current inferior's gdbarch.  Also use the current inferior's address
      space.  */
   gdbarch *arch = inf_for_target_calls->arch ();
-  address_space *aspace = inf_for_target_calls->aspace;
+  address_space *aspace = inf_for_target_calls->aspace.get ();
   regcache *regcache = get_thread_arch_aspace_regcache (inf_for_target_calls,
 							ptid, arch, aspace);
 
diff --git a/gdb/scoped-mock-context.h b/gdb/scoped-mock-context.h
index 5b5f46df7ba..f6bd63a5ae7 100644
--- a/gdb/scoped-mock-context.h
+++ b/gdb/scoped-mock-context.h
@@ -38,7 +38,7 @@ struct scoped_mock_context
 
   Target mock_target;
   ptid_t mock_ptid {1, 1};
-  program_space mock_pspace {new address_space ()};
+  program_space mock_pspace {std::make_shared<address_space> ()};
   inferior mock_inferior {mock_ptid.pid ()};
   thread_info mock_thread {&mock_inferior, mock_ptid};
 
diff --git a/gdb/target-dcache.c b/gdb/target-dcache.c
index 13c2888e7ea..b1b772ab76e 100644
--- a/gdb/target-dcache.c
+++ b/gdb/target-dcache.c
@@ -33,7 +33,7 @@ int
 target_dcache_init_p (void)
 {
   DCACHE *dcache
-    = target_dcache_aspace_key.get (current_program_space->aspace);
+    = target_dcache_aspace_key.get (current_program_space->aspace.get ());
 
   return (dcache != NULL);
 }
@@ -44,7 +44,7 @@ void
 target_dcache_invalidate (void)
 {
   DCACHE *dcache
-    = target_dcache_aspace_key.get (current_program_space->aspace);
+    = target_dcache_aspace_key.get (current_program_space->aspace.get ());
 
   if (dcache != NULL)
     dcache_invalidate (dcache);
@@ -56,7 +56,7 @@ target_dcache_invalidate (void)
 DCACHE *
 target_dcache_get (void)
 {
-  return target_dcache_aspace_key.get (current_program_space->aspace);
+  return target_dcache_aspace_key.get (current_program_space->aspace.get ());
 }
 
 /* Return the target dcache.  If it is not initialized yet, initialize
@@ -66,12 +66,13 @@ DCACHE *
 target_dcache_get_or_init (void)
 {
   DCACHE *dcache
-    = target_dcache_aspace_key.get (current_program_space->aspace);
+    = target_dcache_aspace_key.get (current_program_space->aspace.get ());
 
   if (dcache == NULL)
     {
       dcache = dcache_init ();
-      target_dcache_aspace_key.set (current_program_space->aspace, dcache);
+      target_dcache_aspace_key.set (current_program_space->aspace.get (),
+				    dcache);
     }
 
   return dcache;
-- 
2.35.3

Re: [PATCH v3 1/3] [gdb] Fix segfault in for_each_block, part 1

[Simon Marchi]

On 11/9/23 10:00, Tom de Vries wrote:
> @@ -95,9 +95,9 @@ remove_program_space (program_space *pspace)
>  
>  /* See progspace.h.  */
>  
> -program_space::program_space (address_space *aspace_)
> +program_space::program_space (std::shared_ptr<address_space> aspace_)
>    : num (++last_program_space_num),
> -    aspace (aspace_)
> +       aspace (std::move (aspace_))

The alignment here is not right, `aspace` should be aligned with `num`.

I asked Pedro if he had any thoughts about this patch, and he suggested
not to use shared_ptr (it's not ideal for various performance reasons,
including its use of atomic erations), but to use refcounted_object
(like you initially had) plus gdb::ref_ptr with an appropriate policy
that makes it behave like shared_ptr (deletes the object when the
refcount reaches 0).  I was not really familiar with that, but I gave it
a try to avoid making you re-implement this again.  Here's a patch (only
built-tested) that applies on top of your patch 1/3 that switches to
ref_ptr.

Sorry for leading you to the wrong direction again.

Simon


From 164ffe71449a53579f58be026f4619d8b73e41ff Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Thu, 9 Nov 2023 16:59:46 +0000
Subject: [PATCH] fixup for patch 1, use gdb::ref_ptr

Change-Id: Iad4c992db97664b1ecb97ad60cc36eda6dfb5659
---
 gdb/inferior.h                 |  2 +-
 gdb/infrun.c                   | 11 +++---
 gdb/progspace.c                | 15 ++++----
 gdb/progspace.h                | 64 +++++++++++++++++++++-------------
 gdb/scoped-mock-context.h      |  2 +-
 gdbsupport/refcounted-object.h | 17 +++++++++
 6 files changed, 69 insertions(+), 42 deletions(-)

diff --git a/gdb/inferior.h b/gdb/inferior.h
index db75d6b5ecb..3f74cb5d8d4 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -584,7 +584,7 @@ class inferior : public refcounted_object,
   bool removable = false;

   /* The address space bound to this inferior.  */
-  std::shared_ptr<address_space> aspace;
+  address_space_ref_ptr aspace;

   /* The program space bound to this inferior.  */
   struct program_space *pspace = NULL;
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 34c495097e5..95620d479d8 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -529,8 +529,7 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
 	    }
 	  else
 	    {
-	      child_inf->pspace
-		= new program_space (std::make_shared<address_space> ());
+	      child_inf->pspace = new program_space (new_address_space ());
 	      child_inf->aspace = child_inf->pspace->aspace;
 	      child_inf->removable = true;
 	      clone_program_space (child_inf->pspace, parent_inf->pspace);
@@ -609,8 +608,7 @@ holding the child stopped.  Try \"set detach-on-fork\" or \

 	  child_inf->aspace = parent_inf->aspace;
 	  child_inf->pspace = parent_inf->pspace;
-	  parent_inf->pspace
-	    = new program_space (std::make_shared<address_space> ());
+	  parent_inf->pspace = new program_space (new_address_space ());
 	  parent_inf->aspace = parent_inf->pspace->aspace;
 	  clone_program_space (parent_inf->pspace, child_inf->pspace);

@@ -620,8 +618,7 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
 	}
       else
 	{
-	  child_inf->pspace
-	    = new program_space (std::make_shared<address_space> ());
+	  child_inf->pspace = new program_space (new_address_space ());
 	  child_inf->aspace = child_inf->pspace->aspace;
 	  child_inf->removable = true;
 	  child_inf->symfile_flags = SYMFILE_NO_READ;
@@ -1057,7 +1054,7 @@ handle_vfork_child_exec_or_exit (int exec)

 	  pspace = inf->pspace;
 	  inf->pspace = nullptr;
-	  std::shared_ptr<address_space> aspace = std::move (inf->aspace);
+	  address_space_ref_ptr aspace = std::move (inf->aspace);

 	  if (print_inferior_events)
 	    {
diff --git a/gdb/progspace.c b/gdb/progspace.c
index 2d769134ac6..c0f4b725516 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -55,8 +55,8 @@ address_space::address_space ()
    return a pointer to an existing address space, in case inferiors
    share an address space on this target system.  */

-std::shared_ptr<address_space>
-maybe_new_address_space (void)
+address_space_ref_ptr
+maybe_new_address_space ()
 {
   int shared_aspace
     = gdbarch_has_shared_address_space (current_inferior ()->arch ());
@@ -67,7 +67,7 @@ maybe_new_address_space (void)
       return program_spaces[0]->aspace;
     }

-  return std::make_shared<address_space> ();
+  return new_address_space ();
 }

 /* Start counting over from scratch.  */
@@ -95,7 +95,7 @@ remove_program_space (program_space *pspace)

 /* See progspace.h.  */

-program_space::program_space (std::shared_ptr<address_space> aspace_)
+program_space::program_space (address_space_ref_ptr aspace_)
   : num (++last_program_space_num),
        aspace (std::move (aspace_))
 {
@@ -409,14 +409,14 @@ update_address_spaces (void)

   if (shared_aspace)
     {
-      auto aspace = std::make_shared<address_space> ();
+      address_space_ref_ptr aspace = new_address_space ();

       for (struct program_space *pspace : program_spaces)
 	pspace->aspace = aspace;
     }
   else
     for (struct program_space *pspace : program_spaces)
-      pspace->aspace = std::make_shared<address_space> ();
+      pspace->aspace = new_address_space ();

   for (inferior *inf : all_inferiors ())
     if (gdbarch_has_global_solist (current_inferior ()->arch ()))
@@ -453,6 +453,5 @@ initialize_progspace (void)
      modules have done that.  Do this before
      initialize_current_architecture, because that accesses the ebfd
      of current_program_space.  */
-  current_program_space
-    = new program_space (std::make_shared<address_space> ());
+  current_program_space = new program_space (new_address_space ());
 }
diff --git a/gdb/progspace.h b/gdb/progspace.h
index c169690681b..163cbf6a0f8 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -29,6 +29,8 @@
 #include "gdbsupport/next-iterator.h"
 #include "gdbsupport/safe-iterator.h"
 #include "gdbsupport/intrusive_list.h"
+#include "gdbsupport/refcounted-object.h"
+#include "gdbsupport/gdb_ref_ptr.h"
 #include <list>
 #include <vector>

@@ -43,6 +45,40 @@ struct shobj;

 typedef std::list<std::unique_ptr<objfile>> objfile_list;

+/* An address space.  It is used for comparing if
+   pspaces/inferior/threads see the same address space and for
+   associating caches to each address space.  */
+struct address_space : public refcounted_object
+{
+  /* Create a new address space object, and add it to the list.  */
+  address_space ();
+  DISABLE_COPY_AND_ASSIGN (address_space);
+
+  /* Returns the integer address space id of this address space.  */
+  int num () const
+  {
+    return m_num;
+  }
+
+  /* Per aspace data-pointers required by other GDB modules.  */
+  registry<address_space> registry_fields;
+
+private:
+  int m_num;
+};
+
+using address_space_ref_ptr
+  = gdb::ref_ptr<address_space,
+		 refcounted_object_delete_ref_policy<address_space>>;
+
+/* Create a new address space.  */
+
+static inline address_space_ref_ptr
+new_address_space ()
+{
+  return address_space_ref_ptr::new_reference (new address_space);
+}
+
 /* An iterator that wraps an iterator over std::unique_ptr<objfile>,
    and dereferences the returned object.  This is useful for iterating
    over a list of shared pointers and returning raw pointers -- which
@@ -192,7 +228,7 @@ struct program_space
 {
   /* Constructs a new empty program space, binds it to ASPACE, and
      adds it to the program space list.  */
-  explicit program_space (std::shared_ptr<address_space> aspace);
+  explicit program_space (address_space_ref_ptr aspace);

   /* Releases a program space, and all its contents (shared libraries,
      objfiles, and any other references to the program space in other
@@ -334,7 +370,7 @@ struct program_space
      are global, then this field is ignored (we don't currently
      support inferiors sharing a program space if the target doesn't
      make breakpoints global).  */
-  std::shared_ptr<address_space> aspace;
+  address_space_ref_ptr aspace;

   /* True if this program space's section offsets don't yet represent
      the final offsets of the "live" address space (that is, the
@@ -381,28 +417,6 @@ struct program_space
   std::vector<target_section> m_target_sections;
 };

-/* An address space.  It is used for comparing if
-   pspaces/inferior/threads see the same address space and for
-   associating caches to each address space.  */
-struct address_space
-{
-  /* Create a new address space object, and add it to the list.  */
-  address_space ();
-  DISABLE_COPY_AND_ASSIGN (address_space);
-
-  /* Returns the integer address space id of this address space.  */
-  int num () const
-  {
-    return m_num;
-  }
-
-  /* Per aspace data-pointers required by other GDB modules.  */
-  registry<address_space> registry_fields;
-
-private:
-  int m_num;
-};
-
 /* The list of all program spaces.  There's always at least one.  */
 extern std::vector<struct program_space *>program_spaces;

@@ -445,7 +459,7 @@ class scoped_restore_current_program_space
 /* Maybe create a new address space object, and add it to the list, or
    return a pointer to an existing address space, in case inferiors
    share an address space.  */
-extern std::shared_ptr<address_space> maybe_new_address_space (void);
+extern address_space_ref_ptr maybe_new_address_space ();

 /* Update all program spaces matching to address spaces.  The user may
    have created several program spaces, and loaded executables into
diff --git a/gdb/scoped-mock-context.h b/gdb/scoped-mock-context.h
index f6bd63a5ae7..70b913b949b 100644
--- a/gdb/scoped-mock-context.h
+++ b/gdb/scoped-mock-context.h
@@ -38,7 +38,7 @@ struct scoped_mock_context

   Target mock_target;
   ptid_t mock_ptid {1, 1};
-  program_space mock_pspace {std::make_shared<address_space> ()};
+  program_space mock_pspace {new_address_space ()};
   inferior mock_inferior {mock_ptid.pid ()};
   thread_info mock_thread {&mock_inferior, mock_ptid};

diff --git a/gdbsupport/refcounted-object.h b/gdbsupport/refcounted-object.h
index d8fdb950043..294fd873df1 100644
--- a/gdbsupport/refcounted-object.h
+++ b/gdbsupport/refcounted-object.h
@@ -67,4 +67,21 @@ struct refcounted_object_ref_policy
   }
 };

+/* A policy class to interface gdb::ref_ptr with a refcounted_object, that
+   deletes the object once the refcount reaches 0..  */
+
+template<typename T>
+struct refcounted_object_delete_ref_policy
+{
+  static void incref (T *obj)
+  { obj->incref (); }
+
+  static void decref (T *obj)
+  {
+    obj->decref ();
+    if (obj->refcount () == 0)
+      delete obj;
+  }
+};
+
 #endif /* COMMON_REFCOUNTED_OBJECT_H */
-- 
2.39.2

Re: [PATCH v3 1/3] [gdb] Fix segfault in for_each_block, part 1

[Tom de Vries]

On 11/9/23 19:19, Simon Marchi wrote:
> On 11/9/23 10:00, Tom de Vries wrote:
>> @@ -95,9 +95,9 @@ remove_program_space (program_space *pspace)
>>   
>>   /* See progspace.h.  */
>>   
>> -program_space::program_space (address_space *aspace_)
>> +program_space::program_space (std::shared_ptr<address_space> aspace_)
>>     : num (++last_program_space_num),
>> -    aspace (aspace_)
>> +       aspace (std::move (aspace_))
> 
> The alignment here is not right, `aspace` should be aligned with `num`.
> 

Fixed.

> I asked Pedro if he had any thoughts about this patch, and he suggested
> not to use shared_ptr (it's not ideal for various performance reasons,
> including its use of atomic erations), but to use refcounted_object
> (like you initially had) plus gdb::ref_ptr with an appropriate policy
> that makes it behave like shared_ptr (deletes the object when the
> refcount reaches 0).  I was not really familiar with that, but I gave it
> a try to avoid making you re-implement this again.  Here's a patch (only
> built-tested) that applies on top of your patch 1/3 that switches to
> ref_ptr.
> 

Thanks :) !

> Sorry for leading you to the wrong direction again.
> 

No problem, it was actually useful for me to learn about and try out 
std::shared_ptr.

Anyway, I've submitted a v4 ( 
https://sourceware.org/pipermail/gdb-patches/2023-November/203941.html ) 
that integrates your patch.

Thanks,
- Tom

> Simon
> 
> 
>  From 164ffe71449a53579f58be026f4619d8b73e41ff Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@efficios.com>
> Date: Thu, 9 Nov 2023 16:59:46 +0000
> Subject: [PATCH] fixup for patch 1, use gdb::ref_ptr
> 
> Change-Id: Iad4c992db97664b1ecb97ad60cc36eda6dfb5659
> ---
>   gdb/inferior.h                 |  2 +-
>   gdb/infrun.c                   | 11 +++---
>   gdb/progspace.c                | 15 ++++----
>   gdb/progspace.h                | 64 +++++++++++++++++++++-------------
>   gdb/scoped-mock-context.h      |  2 +-
>   gdbsupport/refcounted-object.h | 17 +++++++++
>   6 files changed, 69 insertions(+), 42 deletions(-)
> 
> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index db75d6b5ecb..3f74cb5d8d4 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -584,7 +584,7 @@ class inferior : public refcounted_object,
>     bool removable = false;
> 
>     /* The address space bound to this inferior.  */
> -  std::shared_ptr<address_space> aspace;
> +  address_space_ref_ptr aspace;
> 
>     /* The program space bound to this inferior.  */
>     struct program_space *pspace = NULL;
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 34c495097e5..95620d479d8 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -529,8 +529,7 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
>   	    }
>   	  else
>   	    {
> -	      child_inf->pspace
> -		= new program_space (std::make_shared<address_space> ());
> +	      child_inf->pspace = new program_space (new_address_space ());
>   	      child_inf->aspace = child_inf->pspace->aspace;
>   	      child_inf->removable = true;
>   	      clone_program_space (child_inf->pspace, parent_inf->pspace);
> @@ -609,8 +608,7 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
> 
>   	  child_inf->aspace = parent_inf->aspace;
>   	  child_inf->pspace = parent_inf->pspace;
> -	  parent_inf->pspace
> -	    = new program_space (std::make_shared<address_space> ());
> +	  parent_inf->pspace = new program_space (new_address_space ());
>   	  parent_inf->aspace = parent_inf->pspace->aspace;
>   	  clone_program_space (parent_inf->pspace, child_inf->pspace);
> 
> @@ -620,8 +618,7 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
>   	}
>         else
>   	{
> -	  child_inf->pspace
> -	    = new program_space (std::make_shared<address_space> ());
> +	  child_inf->pspace = new program_space (new_address_space ());
>   	  child_inf->aspace = child_inf->pspace->aspace;
>   	  child_inf->removable = true;
>   	  child_inf->symfile_flags = SYMFILE_NO_READ;
> @@ -1057,7 +1054,7 @@ handle_vfork_child_exec_or_exit (int exec)
> 
>   	  pspace = inf->pspace;
>   	  inf->pspace = nullptr;
> -	  std::shared_ptr<address_space> aspace = std::move (inf->aspace);
> +	  address_space_ref_ptr aspace = std::move (inf->aspace);
> 
>   	  if (print_inferior_events)
>   	    {
> diff --git a/gdb/progspace.c b/gdb/progspace.c
> index 2d769134ac6..c0f4b725516 100644
> --- a/gdb/progspace.c
> +++ b/gdb/progspace.c
> @@ -55,8 +55,8 @@ address_space::address_space ()
>      return a pointer to an existing address space, in case inferiors
>      share an address space on this target system.  */
> 
> -std::shared_ptr<address_space>
> -maybe_new_address_space (void)
> +address_space_ref_ptr
> +maybe_new_address_space ()
>   {
>     int shared_aspace
>       = gdbarch_has_shared_address_space (current_inferior ()->arch ());
> @@ -67,7 +67,7 @@ maybe_new_address_space (void)
>         return program_spaces[0]->aspace;
>       }
> 
> -  return std::make_shared<address_space> ();
> +  return new_address_space ();
>   }
> 
>   /* Start counting over from scratch.  */
> @@ -95,7 +95,7 @@ remove_program_space (program_space *pspace)
> 
>   /* See progspace.h.  */
> 
> -program_space::program_space (std::shared_ptr<address_space> aspace_)
> +program_space::program_space (address_space_ref_ptr aspace_)
>     : num (++last_program_space_num),
>          aspace (std::move (aspace_))
>   {
> @@ -409,14 +409,14 @@ update_address_spaces (void)
> 
>     if (shared_aspace)
>       {
> -      auto aspace = std::make_shared<address_space> ();
> +      address_space_ref_ptr aspace = new_address_space ();
> 
>         for (struct program_space *pspace : program_spaces)
>   	pspace->aspace = aspace;
>       }
>     else
>       for (struct program_space *pspace : program_spaces)
> -      pspace->aspace = std::make_shared<address_space> ();
> +      pspace->aspace = new_address_space ();
> 
>     for (inferior *inf : all_inferiors ())
>       if (gdbarch_has_global_solist (current_inferior ()->arch ()))
> @@ -453,6 +453,5 @@ initialize_progspace (void)
>        modules have done that.  Do this before
>        initialize_current_architecture, because that accesses the ebfd
>        of current_program_space.  */
> -  current_program_space
> -    = new program_space (std::make_shared<address_space> ());
> +  current_program_space = new program_space (new_address_space ());
>   }
> diff --git a/gdb/progspace.h b/gdb/progspace.h
> index c169690681b..163cbf6a0f8 100644
> --- a/gdb/progspace.h
> +++ b/gdb/progspace.h
> @@ -29,6 +29,8 @@
>   #include "gdbsupport/next-iterator.h"
>   #include "gdbsupport/safe-iterator.h"
>   #include "gdbsupport/intrusive_list.h"
> +#include "gdbsupport/refcounted-object.h"
> +#include "gdbsupport/gdb_ref_ptr.h"
>   #include <list>
>   #include <vector>
> 
> @@ -43,6 +45,40 @@ struct shobj;
> 
>   typedef std::list<std::unique_ptr<objfile>> objfile_list;
> 
> +/* An address space.  It is used for comparing if
> +   pspaces/inferior/threads see the same address space and for
> +   associating caches to each address space.  */
> +struct address_space : public refcounted_object
> +{
> +  /* Create a new address space object, and add it to the list.  */
> +  address_space ();
> +  DISABLE_COPY_AND_ASSIGN (address_space);
> +
> +  /* Returns the integer address space id of this address space.  */
> +  int num () const
> +  {
> +    return m_num;
> +  }
> +
> +  /* Per aspace data-pointers required by other GDB modules.  */
> +  registry<address_space> registry_fields;
> +
> +private:
> +  int m_num;
> +};
> +
> +using address_space_ref_ptr
> +  = gdb::ref_ptr<address_space,
> +		 refcounted_object_delete_ref_policy<address_space>>;
> +
> +/* Create a new address space.  */
> +
> +static inline address_space_ref_ptr
> +new_address_space ()
> +{
> +  return address_space_ref_ptr::new_reference (new address_space);
> +}
> +
>   /* An iterator that wraps an iterator over std::unique_ptr<objfile>,
>      and dereferences the returned object.  This is useful for iterating
>      over a list of shared pointers and returning raw pointers -- which
> @@ -192,7 +228,7 @@ struct program_space
>   {
>     /* Constructs a new empty program space, binds it to ASPACE, and
>        adds it to the program space list.  */
> -  explicit program_space (std::shared_ptr<address_space> aspace);
> +  explicit program_space (address_space_ref_ptr aspace);
> 
>     /* Releases a program space, and all its contents (shared libraries,
>        objfiles, and any other references to the program space in other
> @@ -334,7 +370,7 @@ struct program_space
>        are global, then this field is ignored (we don't currently
>        support inferiors sharing a program space if the target doesn't
>        make breakpoints global).  */
> -  std::shared_ptr<address_space> aspace;
> +  address_space_ref_ptr aspace;
> 
>     /* True if this program space's section offsets don't yet represent
>        the final offsets of the "live" address space (that is, the
> @@ -381,28 +417,6 @@ struct program_space
>     std::vector<target_section> m_target_sections;
>   };
> 
> -/* An address space.  It is used for comparing if
> -   pspaces/inferior/threads see the same address space and for
> -   associating caches to each address space.  */
> -struct address_space
> -{
> -  /* Create a new address space object, and add it to the list.  */
> -  address_space ();
> -  DISABLE_COPY_AND_ASSIGN (address_space);
> -
> -  /* Returns the integer address space id of this address space.  */
> -  int num () const
> -  {
> -    return m_num;
> -  }
> -
> -  /* Per aspace data-pointers required by other GDB modules.  */
> -  registry<address_space> registry_fields;
> -
> -private:
> -  int m_num;
> -};
> -
>   /* The list of all program spaces.  There's always at least one.  */
>   extern std::vector<struct program_space *>program_spaces;
> 
> @@ -445,7 +459,7 @@ class scoped_restore_current_program_space
>   /* Maybe create a new address space object, and add it to the list, or
>      return a pointer to an existing address space, in case inferiors
>      share an address space.  */
> -extern std::shared_ptr<address_space> maybe_new_address_space (void);
> +extern address_space_ref_ptr maybe_new_address_space ();
> 
>   /* Update all program spaces matching to address spaces.  The user may
>      have created several program spaces, and loaded executables into
> diff --git a/gdb/scoped-mock-context.h b/gdb/scoped-mock-context.h
> index f6bd63a5ae7..70b913b949b 100644
> --- a/gdb/scoped-mock-context.h
> +++ b/gdb/scoped-mock-context.h
> @@ -38,7 +38,7 @@ struct scoped_mock_context
> 
>     Target mock_target;
>     ptid_t mock_ptid {1, 1};
> -  program_space mock_pspace {std::make_shared<address_space> ()};
> +  program_space mock_pspace {new_address_space ()};
>     inferior mock_inferior {mock_ptid.pid ()};
>     thread_info mock_thread {&mock_inferior, mock_ptid};
> 
> diff --git a/gdbsupport/refcounted-object.h b/gdbsupport/refcounted-object.h
> index d8fdb950043..294fd873df1 100644
> --- a/gdbsupport/refcounted-object.h
> +++ b/gdbsupport/refcounted-object.h
> @@ -67,4 +67,21 @@ struct refcounted_object_ref_policy
>     }
>   };
> 
> +/* A policy class to interface gdb::ref_ptr with a refcounted_object, that
> +   deletes the object once the refcount reaches 0..  */
> +
> +template<typename T>
> +struct refcounted_object_delete_ref_policy
> +{
> +  static void incref (T *obj)
> +  { obj->incref (); }
> +
> +  static void decref (T *obj)
> +  {
> +    obj->decref ();
> +    if (obj->refcount () == 0)
> +      delete obj;
> +  }
> +};
> +
>   #endif /* COMMON_REFCOUNTED_OBJECT_H */

[PATCH v3 2/3] [gdb] Eliminate local var pspace in inferior.c

[Tom de Vries]

Eliminate local var pspace from functions add_inferior_with_spaces and
clone_inferior_command.

This interchanges the order of the new program space / new inferior events.

Update test-case gdb.python/py-progspace-events.exp to reflect that.

Tested on top of trunk on x86_64-linux and ppc64le-linux.
Tested on top of gdb-14-branch on ppc64-linux.

Suggested-By: Simon Marchi <simon.marchi@polymtl.ca>
---
 gdb/inferior.c                                | 20 +++++++------------
 .../gdb.python/py-progspace-events.exp        |  4 ++--
 2 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/gdb/inferior.c b/gdb/inferior.c
index 37b4fad6e44..e0c5567579f 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -843,16 +843,13 @@ remove_inferior_command (const char *args, int from_tty)
 struct inferior *
 add_inferior_with_spaces (void)
 {
-  struct program_space *pspace;
-  struct inferior *inf;
+  struct inferior *inf = add_inferior (0);
 
   /* If all inferiors share an address space on this system, this
      doesn't really return a new address space; otherwise, it
      really does.  */
-  pspace = new program_space (maybe_new_address_space ());
-  inf = add_inferior (0);
-  inf->pspace = pspace;
-  inf->aspace = pspace->aspace;
+  inf->pspace = new program_space (maybe_new_address_space ());
+  inf->aspace = inf->pspace->aspace;
 
   /* Setup the inferior's initial arch, based on information obtained
      from the global "set ..." options.  */
@@ -1012,16 +1009,13 @@ clone_inferior_command (const char *args, int from_tty)
 
   for (i = 0; i < copies; ++i)
     {
-      struct program_space *pspace;
-      struct inferior *inf;
+      struct inferior *inf = add_inferior (0);
 
       /* If all inferiors share an address space on this system, this
 	 doesn't really return a new address space; otherwise, it
 	 really does.  */
-      pspace = new program_space (maybe_new_address_space ());
-      inf = add_inferior (0);
-      inf->pspace = pspace;
-      inf->aspace = pspace->aspace;
+      inf->pspace = new program_space (maybe_new_address_space ());
+      inf->aspace = inf->pspace->aspace;
       inf->set_arch (orginf->arch ());
 
       switch_to_inferior_and_push_target (inf, no_connection, orginf);
@@ -1031,7 +1025,7 @@ clone_inferior_command (const char *args, int from_tty)
       if (inf->tdesc_info.from_user_p ())
 	inf->tdesc_info = orginf->tdesc_info;
 
-      clone_program_space (pspace, orginf->pspace);
+      clone_program_space (inf->pspace, orginf->pspace);
 
       /* Copy properties from the original inferior to the new one.  */
       inf->set_args (orginf->args ());
diff --git a/gdb/testsuite/gdb.python/py-progspace-events.exp b/gdb/testsuite/gdb.python/py-progspace-events.exp
index 60ae290c0b2..fb3e3e86899 100644
--- a/gdb/testsuite/gdb.python/py-progspace-events.exp
+++ b/gdb/testsuite/gdb.python/py-progspace-events.exp
@@ -118,8 +118,8 @@ gdb_continue_to_end
 # manually added and removed.
 gdb_test "add-inferior" \
     [multi_line \
-	 "^NewProgspaceEvent: <gdb.Progspace object at $hex>" \
-	 "\\\[New inferior 3\\\]" \
+	 "^\\\[New inferior 3\\\]" \
+	 "NewProgspaceEvent: <gdb.Progspace object at $hex>" \
 	 "Added inferior 3\[^\r\n\]*"]
 gdb_test "remove-inferior 3" \
     "^FreeProgspaceEvent: <gdb.Progspace object at $hex>"
-- 
2.35.3

Re: [PATCH v3 2/3] [gdb] Eliminate local var pspace in inferior.c

[Simon Marchi]

On 11/9/23 10:00, Tom de Vries wrote:
> Eliminate local var pspace from functions add_inferior_with_spaces and
> clone_inferior_command.
> 
> This interchanges the order of the new program space / new inferior events.
> 
> Update test-case gdb.python/py-progspace-events.exp to reflect that.
> 
> Tested on top of trunk on x86_64-linux and ppc64le-linux.
> Tested on top of gdb-14-branch on ppc64-linux.
> 
> Suggested-By: Simon Marchi <simon.marchi@polymtl.ca>

Sorry, when I suggested this cleanup, I wasn't aware it would have
user-visible changes.  Since there isn't a need for this except "make
the code a little nicer", I retract my suggestion.

Simon

Re: [PATCH v3 2/3] [gdb] Eliminate local var pspace in inferior.c

[Tom de Vries]

On 11/9/23 17:31, Simon Marchi wrote:
> On 11/9/23 10:00, Tom de Vries wrote:
>> Eliminate local var pspace from functions add_inferior_with_spaces and
>> clone_inferior_command.
>>
>> This interchanges the order of the new program space / new inferior events.
>>
>> Update test-case gdb.python/py-progspace-events.exp to reflect that.
>>
>> Tested on top of trunk on x86_64-linux and ppc64le-linux.
>> Tested on top of gdb-14-branch on ppc64-linux.
>>
>> Suggested-By: Simon Marchi <simon.marchi@polymtl.ca>
> 
> Sorry, when I suggested this cleanup, I wasn't aware it would have
> user-visible changes.  Since there isn't a need for this except "make
> the code a little nicer", I retract my suggestion.

Ack, dropped in v4.

Thanks,
- Tom

[PATCH v3 3/3] [gdb] Fix segfault in for_each_block, part 2

[Tom de Vries]

The previous commit describes PR gdb/30547, a segfault when running test-case
gdb.base/vfork-follow-parent.exp on powerpc64 (likewise on s390x).

The root cause for the segmentation fault is that linux_is_uclinux gives an
incorrect result: it returns true instead of false.

So, why does linux_is_uclinux:
...
int
linux_is_uclinux (void)
{
  CORE_ADDR dummy;

  return (target_auxv_search (AT_NULL, &dummy) > 0
	  && target_auxv_search (AT_PAGESZ, &dummy) == 0);
...
return true?

This is because ppc_linux_target_wordsize returns 4 instead of 8, causing
ppc_linux_nat_target::auxv_parse to misinterpret the auxv vector.

So, why does ppc_linux_target_wordsize:
...
int
ppc_linux_target_wordsize (int tid)
{
  int wordsize = 4;

  /* Check for 64-bit inferior process.	 This is the case when the host is
     64-bit, and in addition the top bit of the MSR register is set.  */
  long msr;

  errno = 0;
  msr = (long) ptrace (PTRACE_PEEKUSER, tid, PT_MSR * 8, 0);
  if (errno == 0 && ppc64_64bit_inferior_p (msr))
    wordsize = 8;

  return wordsize;
}
...
return 4?

Specifically, we get this result because because tid == 0, so we get
errno == ESRCH.

The tid == 0 is caused by the switch_to_no_thread in
handle_vfork_child_exec_or_exit:
...
	  /* Switch to no-thread while running clone_program_space, so
	     that clone_program_space doesn't want to read the
	     selected frame of a dead process.  */
	  scoped_restore_current_thread restore_thread;
	  switch_to_no_thread ();

	  inf->pspace = new program_space (maybe_new_address_space ());
...
but moving the maybe_new_address_space call to before that gives us the
same result.  The tid is no longer 0, but we still get ESRCH because the
thread has exited.

Fix this in handle_vfork_child_exec_or_exit by doing the
maybe_new_address_space call in the context of the vfork parent.

Tested on top of trunk on x86_64-linux and ppc64le-linux.
Tested on top of gdb-14-branch on ppc64-linux.

PR gdb/30547
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30547
---
 gdb/infrun.c         | 16 +++++++++++-----
 gdb/nat/ppc-linux.c  |  2 ++
 gdb/ppc-linux-nat.c  |  2 ++
 gdb/s390-linux-nat.c |  5 ++++-
 4 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 34c495097e5..5c9f8003e2b 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1106,13 +1106,19 @@ handle_vfork_child_exec_or_exit (int exec)
 	     go ahead and create a new one for this exiting
 	     inferior.  */
 
-	  /* Switch to no-thread while running clone_program_space, so
-	     that clone_program_space doesn't want to read the
-	     selected frame of a dead process.  */
 	  scoped_restore_current_thread restore_thread;
-	  switch_to_no_thread ();
 
-	  inf->pspace = new program_space (maybe_new_address_space ());
+	  /* Temporarily switch to the vfork parent, to facilitate ptrace
+	     calls done during maybe_new_address_space.  */
+	  switch_to_thread (any_live_thread_of_inferior (vfork_parent));
+	  std::shared_ptr<address_space> aspace = maybe_new_address_space ();
+
+	  /* Switch back to the vfork child inferior.  Switch to no-thread
+	     while running clone_program_space, so that clone_program_space
+	     doesn't want to read the selected frame of a dead process.  */
+	  switch_to_inferior_no_thread (inf);
+
+	  inf->pspace = new program_space (std::move (aspace));
 	  inf->aspace = inf->pspace->aspace;
 	  set_current_program_space (inf->pspace);
 	  inf->removable = true;
diff --git a/gdb/nat/ppc-linux.c b/gdb/nat/ppc-linux.c
index 0957d1b58a7..74549754806 100644
--- a/gdb/nat/ppc-linux.c
+++ b/gdb/nat/ppc-linux.c
@@ -78,6 +78,8 @@ ppc64_64bit_inferior_p (long msr)
 int
 ppc_linux_target_wordsize (int tid)
 {
+  gdb_assert (tid != 0);
+
   int wordsize = 4;
 
   /* Check for 64-bit inferior process.  This is the case when the host is
diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index a0205119f00..70dd20ad963 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -1914,6 +1914,8 @@ ppc_linux_nat_target::auxv_parse (const gdb_byte **readptr,
 				  const gdb_byte *endptr, CORE_ADDR *typep,
 				  CORE_ADDR *valp)
 {
+  gdb_assert (inferior_ptid != null_ptid);
+
   int tid = inferior_ptid.lwp ();
   if (tid == 0)
     tid = inferior_ptid.pid ();
diff --git a/gdb/s390-linux-nat.c b/gdb/s390-linux-nat.c
index 0aa3edea8aa..1a94ce487c3 100644
--- a/gdb/s390-linux-nat.c
+++ b/gdb/s390-linux-nat.c
@@ -949,10 +949,12 @@ s390_target_wordsize (void)
   /* Check for 64-bit inferior process.  This is the case when the host is
      64-bit, and in addition bit 32 of the PSW mask is set.  */
 #ifdef __s390x__
+  int tid = s390_inferior_tid ();
+  gdb_assert (tid == 0);
   long pswm;
 
   errno = 0;
-  pswm = (long) ptrace (PTRACE_PEEKUSER, s390_inferior_tid (), PT_PSWMASK, 0);
+  pswm = (long) ptrace (PTRACE_PEEKUSER, tid, PT_PSWMASK, 0);
   if (errno == 0 && (pswm & 0x100000000ul) != 0)
     wordsize = 8;
 #endif
@@ -965,6 +967,7 @@ s390_linux_nat_target::auxv_parse (const gdb_byte **readptr,
 				   const gdb_byte *endptr, CORE_ADDR *typep,
 				   CORE_ADDR *valp)
 {
+  gdb_assert (inferior_ptid != null_ptid);
   int sizeof_auxv_field = s390_target_wordsize ();
   bfd_endian byte_order = gdbarch_byte_order (current_inferior ()->arch ());
   const gdb_byte *ptr = *readptr;
-- 
2.35.3