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

[Simon Marchi <simark@simark.ca>]

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

That reminded me of a patch I started to work on a while ago to remove
program_space::aspace, which I have to get back to.  The rationale is
that there might be a 1:N relation ship between one program space and
many address spaces (see diagram in progspace.h), so having this one
pointer does not represent well the relationship.  An inferior is always
bound to a single address space, so my patch changes the code to use
inferior::aspace instead.

I think that even with my patch, we would still have the problem of
knowing when to delete an address space.  When you have many inferiors
sharing an address space, and you delete an inferior, how do you know if
you should delete the address space?  Perhaps we can implement a method
like program_space::empty, but for address_space.

However, I still think that using refcounting for address spaces (and
perhaps for program spaces too?) is a good solution.  It would spare us
of having to deal with the logic of knowing when address space is no
longer used.  Refcounting isn't always the right solution, but I don't
see any downsides in this case.

Implementation question: is there any reason you didn't choose
std::shared_ptr?  For objects that are new'ed / delete'd, that seems
like a good choice.

Simon