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

[Lancelot SIX <lsix@lancelotsix.com>]

Hi Andrew,

On Thu, Oct 12, 2023 at 03:22:18PM +0100, 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:

FYI, I have also tested this patch with AMDGPU which uses
thread-specific gdbarch for the GPU threads (a.k.a waves).  Without the
patch, the unwinder you provided in the other thread is not executed,
but it is after applying this patch.

> 
>   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/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");

Just one nit here, with the observer rename, this can fix in a single
line.

Tested-By: Lancelot Six <lancelot.six@amd.com>

Best,
Laneclot.
>  
>    if (PyType_Ready (&pending_frame_object_type) < 0)
>      return -1;
> 
> base-commit: b8ead7d503a7b3719716d42164299c02abd658cf
> -- 
> 2.25.4
>