gdb: replace architecture_changed with new_architecture observer
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Testing passed
|
Commit Message
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(-)
base-commit: b8ead7d503a7b3719716d42164299c02abd658cf
Comments
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>
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
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
>
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
@@ -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);
@@ -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)
@@ -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);
@@ -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. */
@@ -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;