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

[Alan Hayward <alan dot hayward at arm dot com>]

On 12/12/2016 20:47, "Yao Qi" <qiyaoltc@gmail.com> wrote:

>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.


Agreed that on it’s own this patch looks a little odd.

My other option is to combine this and patch 8 together into one patch.
I originally tried that but I felt it made the patch too cluttered.

The idea of this patch was to contain all the uninteresting boilerplate
code that will support the interesting code in the later patches.

This same point also applies to patch 3/8 and 4/8.

I’ll update the explanations/comments for this patch and 3/8, 4/8.

If required I can combine them into the later patches, but I’d rather
leave them separate.



>
>> 
>> Tested on x86 and aarch64.
>> Ok to commit?
>> 
>> Alan.
>> 
>> 
>> 
>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>> index 
>> 
>>6b95d7c8654521bb58d3af32d898b23380320915..aba7a8525529102d01e3c7cd282a9ee
>>79
>> 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?

Yes - I was trying to make the file look a little cleaner. I’ll remove
them.


>
>>  #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.

Ok.



>
>> +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.

Ok.



>
>> +  VEC(cached_reg_t) *regcache = (VEC(cached_reg_t)*)registers;
>> +  return NULL;
>> +}
>> +
>> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
>> index 
>> 
>>ba57008300d2e463f67bab6561f76908f0933dfb..1eaea537a985ddda6208199feabd8d1
>>4e
>> 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.

Ok.



>
>> +
>>  EOF
>>  }
>> 
>> diff --git a/gdb/remote.h b/gdb/remote.h
>> index 
>> 
>>75e7e670ea1cde00897ef2a3c402161b76b9b564..ffd1758efc5e3b7e5244b599b288fa8
>>d0
>> 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.

Ok.



>
>-- 
>Yao