Re: [PATCH 1/8] Add Aarch64 SVE target description

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

On 2018-05-11 06:52 AM, Alan Hayward wrote:
> This patch adds the SVE target description. However, no code will
> yet use it - that comes in the later patches.
> 
> The create_feature_aarch64_sve function is not generated from XML.
> This is because we need to know the sve vector size (VQ) in order
> to size the registers correctly.
> 
> A VQ of 0 is used when the hardware does not support SVE.
> (SVE hardware will always have a valid vector size). I considered
> using a bool to indicate SVE in addition to the VQ. Whilst this
> may be slightly more readable initially, I think it's a little
> odd to have two variables, eg:
> aarch64_create_target_description (bool sve_supported, long vq)
> 
> Alan.

Hi Alan,

This patch LGTM, I just noted some nits.

>  /* Initialize the current architecture based on INFO.  If possible,
> @@ -2864,7 +2875,8 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  
>    /* Ensure we always have a target descriptor.  */
>    if (!tdesc_has_registers (tdesc))
> -    tdesc = aarch64_read_description ();
> +    /* SVE is not yet supported.  */
> +    tdesc = aarch64_read_description (0);

When there there is a comment above the single statement branch, braces become required:

  if (!tdesc_has_registers (tdesc))
    {
      /* SVE is not yet supported.  */
      tdesc = aarch64_read_description (0);
    }

> diff --git a/gdb/arch/aarch64.c b/gdb/arch/aarch64.c
> index b85e460b6b..d1ec5cedf8 100644
> --- a/gdb/arch/aarch64.c
> +++ b/gdb/arch/aarch64.c
> @@ -21,11 +21,13 @@
>  
>  #include "../features/aarch64-core.c"
>  #include "../features/aarch64-fpu.c"
> +#include "../features/aarch64-sve.c"
>  
> -/* Create the aarch64 target description.  */
> +/* Create the aarch64 target description.  A non zero VQ value indicates both
> +   the presence of SVE and the SVE vector quotient.  */

What does "SVE vector quotient" mean?  Is there maybe a simpler way to say it?

Could you move this comment to the .h and put

/* See arch/aarch64.h.  */

here?

> diff --git a/gdb/features/aarch64-sve.c b/gdb/features/aarch64-sve.c
> new file mode 100644
> index 0000000000..6442640a73
> --- /dev/null
> +++ b/gdb/features/aarch64-sve.c
> @@ -0,0 +1,158 @@
> +/* Copyright (C) 2017 Free Software Foundation, Inc.

2018

Thanks,

Simon