* Re: [PATCH] gdb: replace architecture_changed with new_architecture observer
2023-10-12 14:22 [PATCH] gdb: replace architecture_changed with new_architecture observer Andrew Burgess
@ 2023-10-12 15:04 ` Luis Machado
2023-10-12 15:33 ` Simon Marchi
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Luis Machado @ 2023-10-12 15:04 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
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>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] gdb: replace architecture_changed with new_architecture observer
2023-10-12 14:22 [PATCH] gdb: replace architecture_changed with new_architecture observer Andrew Burgess
2023-10-12 15:04 ` Luis Machado
@ 2023-10-12 15:33 ` Simon Marchi
2023-10-13 16:22 ` Lancelot SIX
2023-10-16 9:06 ` Andrew Burgess
3 siblings, 0 replies; 5+ messages in thread
From: Simon Marchi @ 2023-10-12 15:33 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches; +Cc: Luis Machado
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gdb: replace architecture_changed with new_architecture observer
2023-10-12 14:22 [PATCH] gdb: replace architecture_changed with new_architecture observer Andrew Burgess
2023-10-12 15:04 ` Luis Machado
2023-10-12 15:33 ` Simon Marchi
@ 2023-10-13 16:22 ` Lancelot SIX
2023-10-16 9:06 ` Andrew Burgess
3 siblings, 0 replies; 5+ messages in thread
From: Lancelot SIX @ 2023-10-13 16:22 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches, Luis Machado
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] gdb: replace architecture_changed with new_architecture observer
2023-10-12 14:22 [PATCH] gdb: replace architecture_changed with new_architecture observer Andrew Burgess
` (2 preceding siblings ...)
2023-10-13 16:22 ` Lancelot SIX
@ 2023-10-16 9:06 ` Andrew Burgess
3 siblings, 0 replies; 5+ messages in thread
From: Andrew Burgess @ 2023-10-16 9:06 UTC (permalink / raw)
To: gdb-patches; +Cc: Luis Machado, Simon Marchi, Lancelot SIX
Andrew Burgess <aburgess@redhat.com> writes:
> This commit replaces the architecture_changed observer with a
> new_architecture observer.
Thanks all for the feedback. I made the changes suggested by Simon and
Lancelot, and pushed this patch.
Thanks,
Andrew
>
> 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
> --
> 2.25.4
^ permalink raw reply [flat|nested] 5+ messages in thread