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

[Andrew Burgess <aburgess@redhat.com>]

Tom de Vries <tdevries@suse.de> writes:

> 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/progspace.c | 37 +++++++++++++++++++++++++++----------
>  gdb/progspace.h | 11 ++++++++++-
>  2 files changed, 37 insertions(+), 11 deletions(-)
>
> diff --git a/gdb/progspace.c b/gdb/progspace.c
> index 839707e9d71..4fea21f0ca1 100644
> --- a/gdb/progspace.c
> +++ b/gdb/progspace.c
> @@ -96,9 +96,9 @@ 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)
>  {
> +  set_aspace (aspace_);
>    program_spaces.push_back (this);
>    gdb::observers::new_program_space.notify (this);
>  }
> @@ -122,8 +122,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;
> +  reset_aspace ();
>  }
>  
>  /* See progspace.h.  */
> @@ -409,20 +408,19 @@ update_address_spaces (void)
>  
>    init_address_spaces ();
>  
> +  for (struct program_space *pspace : program_spaces)
> +    pspace->reset_aspace ();
> +
>    if (shared_aspace)
>      {
>        struct address_space *aspace = new address_space ();
>  
> -      delete current_program_space->aspace;
>        for (struct program_space *pspace : program_spaces)
> -	pspace->aspace = aspace;
> +	pspace->set_aspace (aspace);
>      }
>    else
>      for (struct program_space *pspace : program_spaces)
> -      {
> -	delete pspace->aspace;
> -	pspace->aspace = new address_space ();
> -      }
> +      pspace->set_aspace (new address_space ());
>  
>    for (inferior *inf : all_inferiors ())
>      if (gdbarch_has_global_solist (current_inferior ()->arch ()))
> @@ -433,8 +431,27 @@ update_address_spaces (void)
>  
>  \f
>  
> +void
> +program_space::set_aspace (struct address_space *aspace_)
> +{
> +  aspace = aspace_;
> +
> +  aspace->incref ();
> +}
> +
>  /* See progspace.h.  */
>  
> +void
> +program_space::reset_aspace ()
> +{
> +  aspace->decref ();
> +
> +  if (aspace->refcount () == 0)
> +    delete aspace;
> +
> +  aspace = nullptr;
> +}

I wouldn't have expected the reference counting to be done manually like
this.  I would have expected either:

  * Use a std::shared_ptr<address_space> within program_space, and then
    either also hold a std::shared_ptr within the inferior too, or
    potentially loan out raw pointers (to the inferior) using
    std::shared_ptr::get, or

  * Create a new type using gdb::ref_ptr ... but thinking about it,
    given the reference counting policy on this would be pretty vanilla,
    if you had done this, I think I'd be asking why not just use
    std::shared_ptr.

> +
>  void
>  program_space::clear_solib_cache ()
>  {
> diff --git a/gdb/progspace.h b/gdb/progspace.h
> index a22e427400e..065ca38e255 100644
> --- a/gdb/progspace.h
> +++ b/gdb/progspace.h
> @@ -336,6 +336,10 @@ struct program_space
>       make breakpoints global).  */
>    struct address_space *aspace = NULL;
>  
> +  void set_aspace (struct address_space *aspace);
> +
> +  void reset_aspace ();

These should have explanatory comments.  It feels like at a minimum
aspace should be made private, though I really think its type should be
changed to something that manages the reference counting for us.

Thanks,
Andrew

> +
>    /* True if this program space's section offsets don't yet represent
>       the final offsets of the "live" address space (that is, the
>       section addresses still require the relocation offsets to be
> @@ -384,12 +388,17 @@ struct program_space
>  /* 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
> +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);
>  
> +  ~address_space ()
> +  {
> +    gdb_assert (refcount () == 0);
> +  }
> +
>    /* Returns the integer address space id of this address space.  */
>    int num () const
>    {
> -- 
> 2.35.3