Re: [PATCH] gdb: replace architecture_changed with new_architecture observer

[Simon Marchi <simark@simark.ca>]

On 2023-10-12 10:22, Andrew Burgess wrote:
> This commit replaces the architecture_changed observer with a
> new_architecture observer.
> 
> Currently the only user of the architecture_changed observer is the
> Python code, which uses this observer to register the Python unwinder
> with the architecture.
> 
> The problem is that the architecture_changed observer is triggered
> from inferior::set_arch(), which only sees the inferior-wide gdbarch
> value.  For targets that use thread-specific architectures, these
> never trigger the architecture_changed observer, and so never have the
> Python unwinder registered with them.
> 
> When it comes to unwinding GDB makes use of the frame's gdbarch, which
> is based on the thread's regcache gdbarch, which is set in
> get_thread_regcache to the value returned from
> target_thread_architecture, which is not always the inferiors gdbarch
> value, it might be a thread-specific gdbarch which has not passed
> through inferior::set_arch().
> 
> The new_architecture observer will be triggered from
> gdbarch_find_by_info, whenever a new gdbarch is created and
> initialised.  As GDB caches and reuses gdbarch values, we should
> expect to see each new architecture trigger the new_architecture
> observer just once.
> 
> After this commit, targets that make use of thread-specific
> architectures should be able to make use of Python unwinders.
> 
> As I don't have access to a machine that makes use of thread-specific
> architectures right now, I asked Luis to confirm that an AArch64
> target that uses SVE/SME can't use the Python unwinders in threads
> that are using a thread-specific architectures, and he confirmed that
> this is indeed the case, see this discussion:
> 
>   https://inbox.sourceware.org/gdb/87wmvsat8i.fsf@redhat.com
> ---
>  gdb/arch-utils.c       | 2 ++
>  gdb/inferior.c         | 1 -
>  gdb/observable.c       | 2 +-
>  gdb/observable.h       | 7 +++----
>  gdb/python/py-unwind.c | 6 +++---
>  5 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index 60a50ea5b2c..1c83bbe3a58 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -1476,6 +1476,8 @@ gdbarch_find_by_info (struct gdbarch_info info)
>    verify_gdbarch (new_gdbarch);
>    new_gdbarch->initialized_p = true;
>  
> +  gdb::observers::new_architecture.notify (new_gdbarch);
> +
>    if (gdbarch_debug)
>      gdbarch_dump (new_gdbarch, gdb_stdlog);

Just a detail, but I would put the notify after the gdbarch_dump call.
This way gdbarch_find_by_info does all it needs to do, and then it lets
other parts of GDB do their thing.

Otherwise, that looks sensible to me:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon