[PATCH v2 0/2] [gdb] Fix segfault in for_each_block

[PATCH v2 0/2] [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 two patches, each of them by themselves sufficient to no longer
trigger the segfault.

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 second 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 added an assert that PTRACE_PEEKUSER doesn't fail (errno != 0) and ran into
it in test-case gdb.base/access-mem-running.exp, during trying to "set a
breakpoint while the process is running".  It's clear that it's quite common
for this to happen, and it's surprising that this doesn't cause more problems.

I'll eventually file PRs for these two issues, 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).

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

Tom de Vries (2):
  [gdb] Fix segfault in for_each_block, part 1
  [gdb] Fix segfault in for_each_block, part 2

 gdb/infrun.c    | 12 +++++++++++-
 gdb/progspace.c | 37 +++++++++++++++++++++++++++----------
 gdb/progspace.h | 11 ++++++++++-
 3 files changed, 48 insertions(+), 12 deletions(-)

--
2.35.3

Tom de Vries (2):
  [gdb] Fix segfault in for_each_block, part 1
  [gdb] Fix segfault in for_each_block, part 2

 gdb/breakpoint.c             | 29 ++++++++++++++++-------------
 gdb/inferior.c               |  8 ++------
 gdb/inferior.h               |  2 +-
 gdb/infrun.c                 | 28 +++++++++++++++++++---------
 gdb/linux-nat.c              |  2 +-
 gdb/process-stratum-target.c |  2 +-
 gdb/progspace.c              | 29 +++++++++++++++++------------
 gdb/progspace.h              |  5 +++--
 gdb/record-btrace.c          |  2 +-
 gdb/regcache.c               |  2 +-
 gdb/target-dcache.c          | 11 ++++++-----
 11 files changed, 68 insertions(+), 52 deletions(-)


base-commit: d1bd9787f9ad8c49fb463d8d53411aeb5f9c179b
-- 
2.35.3

[PATCH v2 1/2] [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                 | 16 ++++++++--------
 gdb/linux-nat.c              |  2 +-
 gdb/process-stratum-target.c |  2 +-
 gdb/progspace.c              | 29 +++++++++++++++++------------
 gdb/progspace.h              |  5 +++--
 gdb/record-btrace.c          |  2 +-
 gdb/regcache.c               |  2 +-
 gdb/target-dcache.c          | 11 ++++++-----
 11 files changed, 57 insertions(+), 51 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..f80dced115c 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 = NULL;
 
   /* The program space bound to this inferior.  */
   struct program_space *pspace = NULL;
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 4fde96800fb..f0f35c23c9c 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -529,8 +529,8 @@ 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 (new address_space ());
+	      child_inf->aspace = child_inf->pspace->aspace;
 	      child_inf->removable = true;
 	      clone_program_space (child_inf->pspace, parent_inf->pspace);
 	    }
@@ -608,8 +608,8 @@ 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 (new 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 +618,8 @@ 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 (new 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 +1029,7 @@ handle_vfork_child_exec_or_exit (int exec)
       if (vfork_parent->pending_detach)
 	{
 	  struct program_space *pspace;
-	  struct address_space *aspace;
+	  std::shared_ptr<address_space> aspace;
 
 	  /* follow-fork child, detach-on-fork on.  */
 
@@ -5905,7 +5905,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..2e68c71bc21 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,9 @@ maybe_new_address_space (void)
       return program_spaces[0]->aspace;
     }
 
-  return new address_space ();
+  std::shared_ptr<address_space> res;
+  res.reset (new address_space ());
+  return std::move (res);
 }
 
 /* Start counting over from scratch.  */
@@ -96,9 +98,17 @@ remove_program_space (program_space *pspace)
 /* See progspace.h.  */
 
 program_space::program_space (address_space *aspace_)
-  : num (++last_program_space_num),
-    aspace (aspace_)
+  : num (++last_program_space_num)
 {
+  aspace.reset (aspace_);
+  program_spaces.push_back (this);
+  gdb::observers::new_program_space.notify (this);
+}
+
+program_space::program_space (std::shared_ptr<address_space> aspace_)
+  : num (++last_program_space_num)
+{
+  aspace = aspace_;
   program_spaces.push_back (this);
   gdb::observers::new_program_space.notify (this);
 }
@@ -122,8 +132,7 @@ 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;
+  aspace = nullptr;
 }
 
 /* See progspace.h.  */
@@ -413,16 +422,12 @@ update_address_spaces (void)
     {
       struct address_space *aspace = new address_space ();
 
-      delete current_program_space->aspace;
       for (struct program_space *pspace : program_spaces)
-	pspace->aspace = aspace;
+	pspace->aspace.reset (aspace);
     }
   else
     for (struct program_space *pspace : program_spaces)
-      {
-	delete pspace->aspace;
-	pspace->aspace = new address_space ();
-      }
+      pspace->aspace.reset (new address_space ());
 
   for (inferior *inf : all_inferiors ())
     if (gdbarch_has_global_solist (current_inferior ()->arch ()))
diff --git a/gdb/progspace.h b/gdb/progspace.h
index a22e427400e..c796329dd18 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -193,6 +193,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 +335,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 = nullptr;
 
   /* 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 +446,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/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

[PATCH v2 2/2] [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 | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index f0f35c23c9c..94a0924653f 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1105,13 +1105,23 @@ handle_vfork_child_exec_or_exit (int exec)
 	     go ahead and create a new one for this exiting
 	     inferior.  */
 
+	  std::shared_ptr<address_space> aspace;
+	  {
+	    /* Temporarily switch to the vfork parent, to facilitate ptrace
+	       calls done during maybe_new_address_space.  */
+	    scoped_restore save_inferior_ptid
+	      = make_scoped_restore (&inferior_ptid);
+	    inferior_ptid = ptid_t (vfork_parent->pid);
+	    aspace = maybe_new_address_space ();
+	  }
+
 	  /* 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 ());
+	  inf->pspace = new program_space (aspace);
 	  inf->aspace = inf->pspace->aspace;
 	  set_current_program_space (inf->pspace);
 	  inf->removable = true;
-- 
2.35.3

Re: [PATCH v2 1/2] [gdb] Fix segfault in for_each_block, part 1

[Simon Marchi]

Thanks for the update.  I noted a few comments on the use of shared_ptr.

> 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;

Does it work if you remove the pspace variable too and just do:

  inf->pspace = new program_space (maybe_new_address_space ());

> @@ -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;

Same here, if it works.

> @@ -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 = NULL;

Remove `= NULL`.

> @@ -529,8 +529,8 @@ 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 (new address_space ());
> +	      child_inf->aspace = child_inf->pspace->aspace;

I would suggest removing the program_space(address_space *) constructor,
and replacing these calls with:

  new program_space (std::make_shared<address_space> ())

I think the `address_space *` is a bit dangerous.  If you pass an
address_space that is managed by a shared_ptr elsewhere, there will be
two shared_ptr separately managing this address_space, which will result
in a double free.

> @@ -1029,7 +1029,7 @@ handle_vfork_child_exec_or_exit (int exec)
>        if (vfork_parent->pending_detach)
>  	{
>  	  struct program_space *pspace;
> -	  struct address_space *aspace;
> +	  std::shared_ptr<address_space> aspace;

Especially for non-trivially constructible objects, declare the variable
where first used, to avoid constructing an empty object and then doing
an assignment.  In this case, we can even steal the reference from the
inferior to avoid in incref + decref (not a big deal but heh, why not):

  std::shared_ptr<address_space> aspace = std::move (inf->aspace);

After that call, inf->aspace is empty (contains nullptr), at least
according to:

  https://en.cppreference.com/w/cpp/memory/shared_ptr/shared_ptr

> @@ -67,7 +67,9 @@ maybe_new_address_space (void)
>        return program_spaces[0]->aspace;
>      }
>  
> -  return new address_space ();
> +  std::shared_ptr<address_space> res;
> +  res.reset (new address_space ());
> +  return std::move (res);

return std::make_shared<address_space> ();

> @@ -96,9 +98,17 @@ remove_program_space (program_space *pspace)
>  /* See progspace.h.  */
>  
>  program_space::program_space (address_space *aspace_)
> -  : num (++last_program_space_num),
> -    aspace (aspace_)
> +  : num (++last_program_space_num)
>  {
> +  aspace.reset (aspace_);
> +  program_spaces.push_back (this);
> +  gdb::observers::new_program_space.notify (this);
> +}
> +
> +program_space::program_space (std::shared_ptr<address_space> aspace_)
> +  : num (++last_program_space_num)
> +{
> +  aspace = aspace_;

Initialize aspace in the initialization list, and std::move it while at
it to avoid an incref/decref:

   program_space::program_space (std::shared_ptr<address_space> aspace_)
     : num (++last_program_space_num),
       aspace (std::move (aspace_))

> @@ -122,8 +132,7 @@ 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;
> +  aspace = nullptr;

This should not be necessary, it will happen when the aspace shared_ptr
gets destroyed (right after this).

> @@ -413,16 +422,12 @@ update_address_spaces (void)
>      {
>        struct address_space *aspace = new address_space ();
>  
> -      delete current_program_space->aspace;
>        for (struct program_space *pspace : program_spaces)
> -	pspace->aspace = aspace;
> +	pspace->aspace.reset (aspace);

I think this is a wrong use of shared_ptr, you tell separate
shared_ptrs to start managing this address_space object.  They'll all
have a refcount of 1, and will all try to delete the address space
later.

Instead, create one shared_ptr like this:

  auto aspace = std::make_shared<address_space> ();

And then assign to each (the loop doesn't need to change in fact):

  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.reset (new address_space ());

This would work, but to be more robust and avoid the pitfalls I
explained elsewhere, I would suggest creating address spaces only
through std::make_shared.  So:

  pspace->aspace = std::make_shared<address_space> ();


There is also a slight performance advantage in using make_shared,
explained here:

  https://ddanilov.me/shared-ptr-is-evil/

make_shared allocates the shared_ptr's control block (the shared thing
that holds the refcount) and the actual object in the same buffer,
whereas using reset + new allocates two separate buffers.

> @@ -334,7 +335,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 = nullptr;

No need to initialize with nullptr (just like unique_ptr) (and it gets
initialized in the constructor anyway).

Simon

Re: [PATCH v2 2/2] [gdb] Fix segfault in for_each_block, part 2

[Simon Marchi]

On 11/7/23 08:19, Tom de Vries wrote:
> 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.

I would suggest adding some assertion:

 - assert that inferior_ptid != null_ptid in
   ppc_linux_nat_target::auxv_parse and
   s390_linux_nat_target::auxv_parse
 - assert that tid != 0 in ppc_linux_target_wordsize (and in
   s390_target_wordsize), though it could use a bit of refactoring to
   take tid as a parameter like ppc_linux_target_wordsize)


> 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 | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index f0f35c23c9c..94a0924653f 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -1105,13 +1105,23 @@ handle_vfork_child_exec_or_exit (int exec)
>  	     go ahead and create a new one for this exiting
>  	     inferior.  */
>  
> +	  std::shared_ptr<address_space> aspace;
> +	  {
> +	    /* Temporarily switch to the vfork parent, to facilitate ptrace
> +	       calls done during maybe_new_address_space.  */
> +	    scoped_restore save_inferior_ptid
> +	      = make_scoped_restore (&inferior_ptid);
> +	    inferior_ptid = ptid_t (vfork_parent->pid);
> +	    aspace = maybe_new_address_space ();
> +	  }

I guess that's fine as a minimal patch, but it scares me when we just
set part of the global state and inferior_ptid / current_inferior /
current_program_space don't match.  maybe_new_address_space calls
gdbarch_has_shared_address_space, which calls linux_is_uclinux, which
reads auxv.  auxv can be read in different ways.  It's hard to get
convinced that only setting inferior_ptid is enough.

Since we need a scoped_restore_current_thread in the current scope, can
you move it a bit higher?  Something like (untested):

  scoped_restore_current_thread restore_thread;

  /* 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);

Below, you can std::move the aspace when creating the new program_space:

  inf->pspace = new program_space (std::move (aspace));

Even better, I think, would be to pass the vfork parent inferior as a
new `existing_inferior` parameter to maybe_new_address_space, and pass
it all the way through linux_is_uclinux (and do some inferior switching
there in order to do the target calls).  The existing_inferior parameter
would be an inferior that maybe_new_address_space can inspect to
determine if a new address space is needed, and from which it can get
an existing address space, if it determines that the address space
should be shared between the existing and the new inferior.  But I think
it's something that can be done later, switching to the parent here is
fine to keep the patch small and easily backportable.

Simon

Re: [PATCH v2 1/2] [gdb] Fix segfault in for_each_block, part 1

[Tom de Vries]

On 11/7/23 16:00, Simon Marchi wrote:
> Thanks for the update.  I noted a few comments on the use of shared_ptr.
> 
>> 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;
> 
> Does it work if you remove the pspace variable too and just do:
> 
>    inf->pspace = new program_space (maybe_new_address_space ());
> 
>> @@ -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;
> 
> Same here, if it works.
> 

It does work, but the change is user visible, so I've factored that out 
into a separate patch in v3 ( 
https://sourceware.org/pipermail/gdb-patches/2023-November/203931.html ).

>> @@ -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 = NULL;
> 
> Remove `= NULL`.
> 

Done.

>> @@ -529,8 +529,8 @@ 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 (new address_space ());
>> +	      child_inf->aspace = child_inf->pspace->aspace;
> 
> I would suggest removing the program_space(address_space *) constructor,
> and replacing these calls with:
> 
>    new program_space (std::make_shared<address_space> ())
> 

Ah, I was wondering about something like that, make sense, thanks.

> I think the `address_space *` is a bit dangerous.  If you pass an
> address_space that is managed by a shared_ptr elsewhere, there will be
> two shared_ptr separately managing this address_space, which will result
> in a double free.
> 
>> @@ -1029,7 +1029,7 @@ handle_vfork_child_exec_or_exit (int exec)
>>         if (vfork_parent->pending_detach)
>>   	{
>>   	  struct program_space *pspace;
>> -	  struct address_space *aspace;
>> +	  std::shared_ptr<address_space> aspace;
> 
> Especially for non-trivially constructible objects, declare the variable
> where first used, to avoid constructing an empty object and then doing
> an assignment.  In this case, we can even steal the reference from the
> inferior to avoid in incref + decref (not a big deal but heh, why not):
> 
>    std::shared_ptr<address_space> aspace = std::move (inf->aspace);
> 
> After that call, inf->aspace is empty (contains nullptr), at least
> according to:
> 
>    https://en.cppreference.com/w/cpp/memory/shared_ptr/shared_ptr
> 

Done.

>> @@ -67,7 +67,9 @@ maybe_new_address_space (void)
>>         return program_spaces[0]->aspace;
>>       }
>>   
>> -  return new address_space ();
>> +  std::shared_ptr<address_space> res;
>> +  res.reset (new address_space ());
>> +  return std::move (res);
> 
> return std::make_shared<address_space> ();
> 

Done.

>> @@ -96,9 +98,17 @@ remove_program_space (program_space *pspace)
>>   /* See progspace.h.  */
>>   
>>   program_space::program_space (address_space *aspace_)
>> -  : num (++last_program_space_num),
>> -    aspace (aspace_)
>> +  : num (++last_program_space_num)
>>   {
>> +  aspace.reset (aspace_);
>> +  program_spaces.push_back (this);
>> +  gdb::observers::new_program_space.notify (this);
>> +}
>> +
>> +program_space::program_space (std::shared_ptr<address_space> aspace_)
>> +  : num (++last_program_space_num)
>> +{
>> +  aspace = aspace_;
> 
> Initialize aspace in the initialization list, and std::move it while at
> it to avoid an incref/decref:
> 

Ack, done.

>     program_space::program_space (std::shared_ptr<address_space> aspace_)
>       : num (++last_program_space_num),
>         aspace (std::move (aspace_))
> 
>> @@ -122,8 +132,7 @@ 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;
>> +  aspace = nullptr;
> 
> This should not be necessary, it will happen when the aspace shared_ptr
> gets destroyed (right after this).
> 

Ack, done.

>> @@ -413,16 +422,12 @@ update_address_spaces (void)
>>       {
>>         struct address_space *aspace = new address_space ();
>>   
>> -      delete current_program_space->aspace;
>>         for (struct program_space *pspace : program_spaces)
>> -	pspace->aspace = aspace;
>> +	pspace->aspace.reset (aspace);
> 
> I think this is a wrong use of shared_ptr, you tell separate
> shared_ptrs to start managing this address_space object.  They'll all
> have a refcount of 1, and will all try to delete the address space
> later.
> 

Ouch.  I wondered why this didn't surface in my testing, but I guess 
that's because I only tested native, and this is only exercised with remote.

> Instead, create one shared_ptr like this:
> 
>    auto aspace = std::make_shared<address_space> ();
> 
> And then assign to each (the loop doesn't need to change in fact):
> 
>    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.reset (new address_space ());
> 
> This would work, but to be more robust and avoid the pitfalls I
> explained elsewhere, I would suggest creating address spaces only
> through std::make_shared.  So:
> 
>    pspace->aspace = std::make_shared<address_space> ();
> 
> 
> There is also a slight performance advantage in using make_shared,
> explained here:
> 
>    https://ddanilov.me/shared-ptr-is-evil/
> 
> make_shared allocates the shared_ptr's control block (the shared thing
> that holds the refcount) and the actual object in the same buffer,
> whereas using reset + new allocates two separate buffers.
> 

Done.

>> @@ -334,7 +335,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 = nullptr;
> 
> No need to initialize with nullptr (just like unique_ptr) (and it gets
> initialized in the constructor anyway).

Done.

V3 submitted here ( 
https://sourceware.org/pipermail/gdb-patches/2023-November/203934.html ).

Thanks,
- Tom

Re: [PATCH v2 2/2] [gdb] Fix segfault in for_each_block, part 2

[Tom de Vries]

On 11/7/23 16:26, Simon Marchi wrote:
> On 11/7/23 08:19, Tom de Vries wrote:
>> 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.
> 
> I would suggest adding some assertion:
> 
>   - assert that inferior_ptid != null_ptid in
>     ppc_linux_nat_target::auxv_parse and
>     s390_linux_nat_target::auxv_parse
>   - assert that tid != 0 in ppc_linux_target_wordsize (and in
>     s390_target_wordsize), though it could use a bit of refactoring to
>     take tid as a parameter like ppc_linux_target_wordsize)
> 

I've added these, though I haven't been able to test on s390.

>> 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 | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>> index f0f35c23c9c..94a0924653f 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -1105,13 +1105,23 @@ handle_vfork_child_exec_or_exit (int exec)
>>   	     go ahead and create a new one for this exiting
>>   	     inferior.  */
>>   
>> +	  std::shared_ptr<address_space> aspace;
>> +	  {
>> +	    /* Temporarily switch to the vfork parent, to facilitate ptrace
>> +	       calls done during maybe_new_address_space.  */
>> +	    scoped_restore save_inferior_ptid
>> +	      = make_scoped_restore (&inferior_ptid);
>> +	    inferior_ptid = ptid_t (vfork_parent->pid);
>> +	    aspace = maybe_new_address_space ();
>> +	  }
> 
> I guess that's fine as a minimal patch, but it scares me when we just
> set part of the global state and inferior_ptid / current_inferior /
> current_program_space don't match.  maybe_new_address_space calls
> gdbarch_has_shared_address_space, which calls linux_is_uclinux, which
> reads auxv.  auxv can be read in different ways.  It's hard to get
> convinced that only setting inferior_ptid is enough.
> 
> Since we need a scoped_restore_current_thread in the current scope, can
> you move it a bit higher?  Something like (untested):
> 
>    scoped_restore_current_thread restore_thread;
> 
>    /* 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);
> 
> Below, you can std::move the aspace when creating the new program_space:
> 
>    inf->pspace = new program_space (std::move (aspace));
> 

Done, v3 available here ( 
https://sourceware.org/pipermail/gdb-patches/2023-November/203933.html ).

Thanks,
- Tom

> Even better, I think, would be to pass the vfork parent inferior as a
> new `existing_inferior` parameter to maybe_new_address_space, and pass
> it all the way through linux_is_uclinux (and do some inferior switching
> there in order to do the target calls).  The existing_inferior parameter
> would be an inferior that maybe_new_address_space can inspect to
> determine if a new address space is needed, and from which it can get
> an existing address space, if it determines that the address space
> should be shared between the existing and the new inferior.  But I think
> it's something that can be done later, switching to the parent here is
> fine to keep the patch small and easily backportable.
> 
> Simon

Re: [PATCH v2 1/2] [gdb] Fix segfault in for_each_block, part 1

[Simon Marchi]

On 11/9/23 10:07, Tom de Vries wrote:
> On 11/7/23 16:00, Simon Marchi wrote:
>> Thanks for the update.  I noted a few comments on the use of shared_ptr.
>>
>>> 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;
>>
>> Does it work if you remove the pspace variable too and just do:
>>
>>    inf->pspace = new program_space (maybe_new_address_space ());
>>
>>> @@ -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;
>>
>> Same here, if it works.
>>
> 
> It does work, but the change is user visible, so I've factored that out into a separate patch in v3 ( https://sourceware.org/pipermail/gdb-patches/2023-November/203931.html ).

Oh, well if it changes the behavior of GDB, maybe that's not a good idea
then.  I suggested that just because it would make the code cleaner, but
I don't think that's a good reason to justify changing the behavior.

>>> @@ -413,16 +422,12 @@ update_address_spaces (void)
>>>       {
>>>         struct address_space *aspace = new address_space ();
>>>   -      delete current_program_space->aspace;
>>>         for (struct program_space *pspace : program_spaces)
>>> -    pspace->aspace = aspace;
>>> +    pspace->aspace.reset (aspace);
>>
>> I think this is a wrong use of shared_ptr, you tell separate
>> shared_ptrs to start managing this address_space object.  They'll all
>> have a refcount of 1, and will all try to delete the address space
>> later.
>>
> 
> Ouch.  I wondered why this didn't surface in my testing, but I guess that's because I only tested native, and this is only exercised with remote.

I don't think any regular testing exercises this, you'd need to test
against an uclinux target (an MMU-less Linux) to reach that code.

Simon