[PATCH v2 1/2] gdb/aarch64: Fix thread's gdbarch when SVE vector length changes
Luis Machado
luis.machado@arm.com
Fri Aug 5 17:26:47 GMT 2022
Hi,
On 8/5/22 01:46, Thiago Jung Bauermann via Gdb-patches wrote:
> When the inferior program changes the SVE length, GDB can stop tracking
> some registers as it obtains the new gdbarch that corresponds to the
> updated length:
>
> Breakpoint 1, do_sve_ioctl_test () at sve-ioctls.c:44
> 44 res = prctl(PR_SVE_SET_VL, i, 0, 0, 0, 0);
> (gdb) print i
> $2 = 32
> (gdb) info registers
> ⋮
> [ snip registers x0 to x30 ]
> ⋮
> sp 0xffffffffeff0 0xffffffffeff0
> pc 0xaaaaaaaaa8ac 0xaaaaaaaaa8ac <do_sve_ioctl_test+112>
> cpsr 0x60000000 [ EL=0 BTYPE=0 C Z ]
> fpsr 0x0 0
> fpcr 0x0 0
> vg 0x8 8
> tpidr 0xfffff7fcb320 0xfffff7fcb320
> (gdb) next
> 45 if (res < 0) {
> (gdb) info registers
> ⋮
> [ snip registers x0 to x30 ]
> ⋮
> sp 0xffffffffeff0 0xffffffffeff0
> pc 0xaaaaaaaaa8cc 0xaaaaaaaaa8cc <do_sve_ioctl_test+144>
> cpsr 0x200000 [ EL=0 BTYPE=0 SS ]
> fpsr 0x0 0
> fpcr 0x0 0
> vg 0x4 4
> (gdb)
>
> Notice that register tpidr disappeared when vg (which holds the vector
> length) changed from 8 to 4. The tpidr register is provided by the
> org.gnu.gdb.aarch64.tls feature.
>
> This happens because the code that searches for a new gdbarch to match the
> new vector length in aarch64_linux_nat_target::thread_architecture doesn't
> take into account the features present in the target description associated
> with the previous gdbarch. This patch makes it do that.
>
> Since the id member of struct gdbarch_info is now unused, it's removed.
> ---
> gdb/aarch64-linux-nat.c | 11 ++++++++---
> gdb/aarch64-tdep.c | 41 ++++++++++++++++++++++++-----------------
> gdb/aarch64-tdep.h | 2 ++
> gdb/gdbarch.h | 13 ++-----------
> 4 files changed, 36 insertions(+), 31 deletions(-)
>
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index a457fcd48ad8..eda79ec6d35c 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -900,11 +900,16 @@ aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
>
> /* We reach here if the vector length for the thread is different from its
> value at process start. Lookup gdbarch via info (potentially creating a
> - new one), stashing the vector length inside id. Use -1 for when SVE
> - unavailable, to distinguish from an unset value of 0. */
> + new one) by using a target description that corresponds to the new vq value
> + and the current architecture features. */
> +
> + const struct target_desc *tdesc = gdbarch_target_desc (inf->gdbarch);
> + aarch64_features features = aarch64_features_from_target_desc (tdesc);
> + features.vq = vq;
> +
> struct gdbarch_info info;
> info.bfd_arch_info = bfd_lookup_arch (bfd_arch_aarch64, bfd_mach_aarch64);
> - info.id = (int *) (vq == 0 ? -1 : vq);
> + info.target_desc = aarch64_read_description (features);
> return gdbarch_find_by_info (info);
> }
>
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 8670197a8889..c9ce4f95447c 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -3372,6 +3372,27 @@ aarch64_get_tdesc_vq (const struct target_desc *tdesc)
> return sve_vq_from_vl (vl);
> }
>
> +/* Get the AArch64 features present in the given target description. */
> +
> +aarch64_features
> +aarch64_features_from_target_desc (const struct target_desc *tdesc)
> +{
> + aarch64_features features;
> +
> + if (tdesc == nullptr)
> + return features;
> +
> + features.vq = aarch64_get_tdesc_vq (tdesc);
> + features.pauth
> + = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.pauth") != nullptr);
> + features.mte
> + = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.mte") != nullptr);
> + features.tls
> + = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.tls") != nullptr);
> +
> + return features;
> +}
> +
> /* Implement the "cannot_store_register" gdbarch method. */
>
> static int
> @@ -3422,17 +3443,7 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> int i, num_regs = 0, num_pseudo_regs = 0;
> int first_pauth_regnum = -1, ra_sign_state_offset = -1;
> int first_mte_regnum = -1, tls_regnum = -1;
> -
> - /* Use the vector length passed via the target info. Here -1 is used for no
> - SVE, and 0 is unset. If unset then use the vector length from the existing
> - tdesc. */
> - uint64_t vq = 0;
> - if (info.id == (int *) -1)
> - vq = 0;
> - else if (info.id != 0)
> - vq = (uint64_t) info.id;
> - else
> - vq = aarch64_get_tdesc_vq (info.target_desc);
> + uint64_t vq = aarch64_get_tdesc_vq (info.target_desc);
>
> if (vq > AARCH64_MAX_SVE_VQ)
> internal_error (__FILE__, __LINE__, _("VQ out of bounds: %s (max %d)"),
> @@ -3452,12 +3463,8 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> /* Ensure we always have a target descriptor, and that it is for the given VQ
> value. */
> const struct target_desc *tdesc = info.target_desc;
> - if (!tdesc_has_registers (tdesc) || vq != aarch64_get_tdesc_vq (tdesc))
> - {
> - aarch64_features features;
> - features.vq = vq;
> - tdesc = aarch64_read_description (features);
> - }
> + if (!tdesc_has_registers (tdesc))
> + tdesc = aarch64_read_description ({});
> gdb_assert (tdesc);
>
> feature_core = tdesc_find_feature (tdesc,"org.gnu.gdb.aarch64.core");
> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
> index 5bdd733dce32..d8513023c376 100644
> --- a/gdb/aarch64-tdep.h
> +++ b/gdb/aarch64-tdep.h
> @@ -121,6 +121,8 @@ struct aarch64_gdbarch_tdep : gdbarch_tdep_base
> };
>
> const target_desc *aarch64_read_description (const aarch64_features &features);
> +aarch64_features
> +aarch64_features_from_target_desc (const struct target_desc *tdesc);
>
> extern int aarch64_process_record (struct gdbarch *gdbarch,
> struct regcache *regcache, CORE_ADDR addr);
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index 324958469a7e..1287f0214885 100644
> --- a/gdb/gdbarch.h
> +++ b/gdb/gdbarch.h
> @@ -250,17 +250,8 @@ struct gdbarch_info
>
> bfd *abfd = nullptr;
>
> - union
> - {
> - /* Architecture-specific target description data. Numerous targets
> - need only this, so give them an easy way to hold it. */
> - struct tdesc_arch_data *tdesc_data;
> -
> - /* SPU file system ID. This is a single integer, so using the
> - generic form would only complicate code. Other targets may
> - reuse this member if suitable. */
> - int *id;
> - };
> + /* Architecture-specific target description data. */
> + struct tdesc_arch_data *tdesc_data;
>
> enum gdb_osabi osabi = GDB_OSABI_UNKNOWN;
>
This version looks good to me. Thanks for addressing the comments.
I think the gdbarch.h change is reasonably obvious that folks wouldn't mind it going in
along with this fix.
More information about the Gdb-patches
mailing list