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

[Simon Marchi <simon.marchi@ericsson.com>]

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