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

[Tom de Vries <tdevries@suse.de>]

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