Re: [PATCH 2/8] AARCH64 SVE: Add gdbarch methods

[Yao Qi <qiyaoltc at gmail dot com>]

On 16-12-05 12:26:43, Alan Hayward wrote:
> This is part of a series adding AARCH64 SVE support to gdb and gdbserver.
> 
> This patch simply adds two gdbarch methods:
> target_description_changed_p ()
> target_get_tdep_info ()

We need more rationale on why do we add these two new gdbarch methods.

> 
> In a later patch, the aarch64 version of these methods will need to access
> regcache as a VEC(cached_reg_t).
> 
> These method will remain unused until a later patch in the series.

Could you add the code using these gdbarch methods in the same patch?
It is odd to add a function in one patch, and use it in another.

> 
> Tested on x86 and aarch64.
> Ok to commit?
> 
> Alan.
> 
> 
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 
> 6b95d7c8654521bb58d3af32d898b23380320915..aba7a8525529102d01e3c7cd282a9ee79
> 3d643fe 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -44,23 +44,17 @@
>  #include "infcall.h"
>  #include "ax.h"
>  #include "ax-gdb.h"
> -
>  #include "aarch64-tdep.h"
> -
>  #include "elf-bfd.h"
>  #include "elf/aarch64.h"
> -
>  #include "vec.h"
> -
>  #include "record.h"
>  #include "record-full.h"
> -
>  #include "features/aarch64.c"
> -
>  #include "arch/aarch64-insn.h"
> -

Is it intended to remove these blank lines?

>  #include "opcode/aarch64.h"
>  #include <algorithm>
> +#include "remote.h"
> 



>  #define submask(x) ((1L << ((x) + 1)) - 1)
>  #define bit(obj,st) (((obj) >> (st)) & 1)
> @@ -2643,6 +2637,24 @@ aarch64_displaced_step_hw_singlestep (struct
> gdbarch *gdbarch,
>    return 1;
>  }
> 
> +/* Implement the "target_description_changed_p" gdbarch method.  */

A blank line is needed.

> +static int
> +aarch64_target_description_changed_p (struct gdbarch *gdbarch,
> +				      ptid_t ptid,
> +				      void *registers)
> +{
> +  VEC(cached_reg_t) *registerlist = (VEC(cached_reg_t)*)registers;
> +  return 0;
> +}
> +
> +/* Implement the "target_get_tdep_info" gdbarch method.  */
> +void *
> +aarch64_target_get_tdep_info (void *registers)
> +{

Likewise.

> +  VEC(cached_reg_t) *regcache = (VEC(cached_reg_t)*)registers;
> +  return NULL;
> +}
> +
> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> index 
> ba57008300d2e463f67bab6561f76908f0933dfb..1eaea537a985ddda6208199feabd8d14e
> 91e837c 100755
> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -1163,6 +1163,12 @@ m:const char
> *:gnu_triplet_regexp:void:::default_gnu_triplet_regexp::0
>  # each address in memory.
>  m:int:addressable_memory_unit_size:void:::default_addressable_memory_unit_
> size::0
> 
> +# Given a list of registers, check if the target description has changed.
> +m:int:target_description_changed_p:ptid_t ptid, void *registers:ptid,
> registers::default_target_description_changed_p::0
> +
> +# Given a list of registers, return a tdep info.
> +f:void*:target_get_tdep_info:void
> *registers:registers::default_target_get_tdep_info::0

More documentation on these two methods are needed.

> +
>  EOF
>  }
> 
> diff --git a/gdb/remote.h b/gdb/remote.h
> index 
> 75e7e670ea1cde00897ef2a3c402161b76b9b564..ffd1758efc5e3b7e5244b599b288fa8d0
> 1c804fe 100644
> --- a/gdb/remote.h
> +++ b/gdb/remote.h
> @@ -23,6 +23,15 @@
> 
>  struct target_desc;
> 
> +typedef struct cached_reg
> +{
> +  int num;
> +  gdb_byte data[MAX_REGISTER_SIZE];
> +} cached_reg_t;
> +
> +DEF_VEC_O(cached_reg_t);
> +
> +

We may need to move this structure to regcache.h, and rename it with
"reg_info" for example.  Then, "struct reg_info" in python/py-unwind.c
can be removed.  A good side-effect of this change is that we remove one
usage of MAX_REGISTER_SIZE.

-- 
Yao