Re: [PATCH 4/8] Enable SVE for GDB

Simon Marchi
2018-05-31T11:56:17Z

On 2018-05-11 06:52 AM, Alan Hayward wrote:
> This patch enables SVE support for GDB by reading the VQ when
> creating a target description.
>
> It also ensures that SVE is taken into account when creating
> the tdep structure, and stores the current VQ value directly
> in tdep.
>
> With this patch, gdb on an aarch64 system with SVE will now detect
> SVE. The SVE registers will be displayed (but the contents will be
> invalid). The following patches fill out the contents.

LGTM, with some nits.

> @@ -2911,25 +2933,45 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> /* Validate the descriptor provides the mandatory core R registers
> and allocate their numbers. */
> for (i = 0; i < ARRAY_SIZE (aarch64_r_register_names); i++)
> - valid_p &=
> - tdesc_numbered_register (feature, tdesc_data, AARCH64_X0_REGNUM + i,
> - aarch64_r_register_names[i]);
> + valid_p &= tdesc_numbered_register (feature_core, tdesc_data,
> + AARCH64_X0_REGNUM + i,
> + aarch64_r_register_names[i]);
>
> num_regs = AARCH64_X0_REGNUM + i;
>
> - /* Look for the V registers. */
> - feature = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.fpu");
> - if (feature)
> + /* Add the V registers. */
> + if (feature_fpu != NULL)
> {
> + gdb_assert (feature_sve == NULL);

Again, if this situation can result from a bad input passed to GDB (a bad tdesc
sent by the remote), we shouldn't gdb_assert on it, but show an error message
and gracefully fail.

> +
> /* Validate the descriptor provides the mandatory V registers
> - and allocate their numbers. */
> + and allocate their numbers. */
> for (i = 0; i < ARRAY_SIZE (aarch64_v_register_names); i++)
> - valid_p &=
> - tdesc_numbered_register (feature, tdesc_data, AARCH64_V0_REGNUM + i,
> - aarch64_v_register_names[i]);
> + valid_p &= tdesc_numbered_register (feature_fpu, tdesc_data,
> + AARCH64_V0_REGNUM + i,
> + aarch64_v_register_names[i]);
>
> num_regs = AARCH64_V0_REGNUM + i;
> + }
> +
> + /* Add the SVE registers. */
> + if (feature_sve != NULL)
> + {
> + gdb_assert (feature_fpu == NULL);

Same here.

> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
> index c9fd7b3578..046de6228f 100644
> --- a/gdb/aarch64-tdep.h
> +++ b/gdb/aarch64-tdep.h
> @@ -73,6 +73,15 @@ struct gdbarch_tdep
>
> /* syscall record. */
> int (*aarch64_syscall_record) (struct regcache *regcache, unsigned long svc_number);
> +
> + /* The VQ value for SVE targets, or zero if SVE is not supported. */
> + long vq;
> +
> + /* Returns true if the target supports SVE. */
> + bool has_sve ()
> + {
> + return vq != 0;
> + }

This method can be const.

Simon