Thanks for the reviews. Pushed this with changes as below. > On 31 May 2018, at 12:09, Simon Marchi wrote: > > 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); > } ok. > >> 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? > It’s explained in a later patch, but I should have explained it here. Updated to (and moved into header): /* Create the aarch64 target description. A non zero VQ value indicates both the presence of SVE and the Vector Quotient - the number of 128bit chunks in an SVE Z register. */ > Could you move this comment to the .h and put > > /* See arch/aarch64.h. */ > > here? Ok. > >> 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 > Ok. Alan. &j!z޶׎Ib֫rnr