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

[Simon Marchi <simark@simark.ca>]

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