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

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

On 11/6/23 16:24, Andrew Burgess wrote:
> 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
> 

Hi Andrew,

thanks for the review.

I've submitted a v2 ( 
https://sourceware.org/pipermail/gdb-patches/2023-November/203850.html ) 
that uses std::shared_ptr for the address_space for both the 
program_space and the inferior.

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

These two functions are gone in the v2.

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

In the v2 I've left aspace non-private.

I could add a follow-up patch that makes a aspace private, but for 
backporting purposes I'd like to keep this patch a simple as possible.

Thanks,
- Tom

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