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

[Luis Machado <luis.machado@arm.com>]

On 10/12/23 15: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);
>  
> diff --git a/gdb/inferior.c b/gdb/inferior.c
> index efe57cceae3..1778723863e 100644
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -179,7 +179,6 @@ inferior::set_arch (gdbarch *arch)
>    gdb_assert (arch != nullptr);
>    gdb_assert (gdbarch_initialized_p (arch));
>    m_gdbarch = arch;
> -  gdb::observers::architecture_changed.notify (this, arch);
>  
>    process_stratum_target *proc_target = this->process_target ();
>    if (proc_target != nullptr)
> diff --git a/gdb/observable.c b/gdb/observable.c
> index 09613b2ddda..f2e65b11604 100644
> --- a/gdb/observable.c
> +++ b/gdb/observable.c
> @@ -52,7 +52,7 @@ DEFINE_OBSERVABLE (about_to_proceed);
>  DEFINE_OBSERVABLE (breakpoint_created);
>  DEFINE_OBSERVABLE (breakpoint_deleted);
>  DEFINE_OBSERVABLE (breakpoint_modified);
> -DEFINE_OBSERVABLE (architecture_changed);
> +DEFINE_OBSERVABLE (new_architecture);
>  DEFINE_OBSERVABLE (thread_ptid_changed);
>  DEFINE_OBSERVABLE (inferior_added);
>  DEFINE_OBSERVABLE (inferior_appeared);
> diff --git a/gdb/observable.h b/gdb/observable.h
> index acb05e9b535..a535eedcd38 100644
> --- a/gdb/observable.h
> +++ b/gdb/observable.h
> @@ -153,10 +153,9 @@ extern observable<struct breakpoint */* b */> breakpoint_deleted;
>     is the modified breakpoint.  */
>  extern observable<struct breakpoint */* b */> breakpoint_modified;
>  
> -/* INF's architecture has changed.  The argument NEWARCH is a
> -   pointer to the new architecture.  */
> -extern observable<inferior */* inf */, struct gdbarch */* newarch */>
> -  architecture_changed;
> +/* GDB has instantiated a new architecture, NEWARCH is a pointer to the new
> +   architecture.  */
> +extern observable<struct gdbarch */* newarch */> new_architecture;
>  
>  /* The thread's ptid has changed.  The OLD_PTID parameter specifies
>     the old value, and NEW_PTID specifies the new value.  */
> diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
> index f8b142dd52c..4de81c0a7eb 100644
> --- a/gdb/python/py-unwind.c
> +++ b/gdb/python/py-unwind.c
> @@ -945,7 +945,7 @@ static const registry<gdbarch>::key<pyuw_gdbarch_data_type> pyuw_gdbarch_data;
>     intermediary.  */
>  
>  static void
> -pyuw_on_new_gdbarch (inferior *inf, gdbarch *newarch)
> +pyuw_on_new_gdbarch (gdbarch *newarch)
>  {
>    struct pyuw_gdbarch_data_type *data = pyuw_gdbarch_data.get (newarch);
>    if (data == nullptr)
> @@ -974,8 +974,8 @@ pyuw_on_new_gdbarch (inferior *inf, gdbarch *newarch)
>  static int CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION
>  gdbpy_initialize_unwind (void)
>  {
> -  gdb::observers::architecture_changed.attach (pyuw_on_new_gdbarch,
> -					       "py-unwind");
> +  gdb::observers::new_architecture.attach (pyuw_on_new_gdbarch,
> +					   "py-unwind");
>  
>    if (PyType_Ready (&pending_frame_object_type) < 0)
>      return -1;
> 
> base-commit: b8ead7d503a7b3719716d42164299c02abd658cf

I've confirmed this works as intended on a SVE-enabled target with multiple threads.

Reviewed-by: Luis Machado <luis.machado@arm.com>
Tested-by: Luis Machado <luis.machado@arm.com>