[2/8] AARCH64 SVE: Add gdbarch methods

Message ID D46B0D83.154DE%alan.hayward@arm.com
State New, archived
Headers

Commit Message

Alan Hayward Dec. 5, 2016, 12:26 p.m. UTC
  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 ()

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.

Tested on x86 and aarch64.
Ok to commit?

Alan.
  

Comments

Yao Qi Dec. 12, 2016, 8:47 p.m. UTC | #1
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.
  
Alan Hayward Dec. 13, 2016, 12:24 p.m. UTC | #2
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
  

Patch

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"
-
 #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.  */
+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)
+{
+  VEC(cached_reg_t) *regcache = (VEC(cached_reg_t)*)registers;
+  return NULL;
+}
+
 /* Initialize the current architecture based on INFO.  If possible,
    re-use an architecture from ARCHES, which is a list of
    architectures already created during this debugging session.
@@ -2773,6 +2785,9 @@  aarch64_gdbarch_init (struct gdbarch_info info,
struct gdbarch_list *arches)
   set_tdesc_pseudo_register_type (gdbarch, aarch64_pseudo_register_type);
   set_tdesc_pseudo_register_reggroup_p (gdbarch,
 					aarch64_pseudo_register_reggroup_p);
+  set_gdbarch_target_description_changed_p
+    (gdbarch, aarch64_target_description_changed_p);
+  set_gdbarch_target_get_tdep_info (gdbarch,
aarch64_target_get_tdep_info);

   /* ABI */
   set_gdbarch_short_bit (gdbarch, 16);
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 
830ebed31206ff5a794681a6036b0f09db5b4cf8..8a2eab04332c96ce6b53c5311265b0954
e932d06 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -267,4 +267,9 @@  extern void default_guess_tracepoint_registers (struct
gdbarch *gdbarch,
 						struct regcache *regcache,
 						CORE_ADDR addr);

+extern int default_target_description_changed_p (struct gdbarch *gdbarch,
+						 ptid_t ptid,
+						 void *registers);
+extern void *default_target_get_tdep_info (void *registers);
+
 #endif
diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 
d64a73db2eb9ca7be6c2c98120d82db3781af88a..01d4221c93fe6d401ffed8f70d0badd42
18b8cdd 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -964,6 +964,20 @@  default_guess_tracepoint_registers (struct gdbarch
*gdbarch,
   regcache_raw_supply (regcache, pc_regno, regs);
 }

+int
+default_target_description_changed_p (struct gdbarch *gdbarch,
+				      ptid_t ptid,
+				      void *registers)
+{
+  return 0;
+}
+
+void *
+default_target_get_tdep_info (void *registers)
+{
+  return NULL;
+}
+
 /* -Wmissing-prototypes */
 extern initialize_file_ftype _initialize_gdbarch_utils;

diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 
3f3c002a5a53f7c24109902a8b5b35e631bda741..4892daa36c8804b802bec1f34d7c085ab
844bd89 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -1545,6 +1545,18 @@  typedef int
(gdbarch_addressable_memory_unit_size_ftype) (struct gdbarch *gdbarc
 extern int gdbarch_addressable_memory_unit_size (struct gdbarch *gdbarch);
 extern void set_gdbarch_addressable_memory_unit_size (struct gdbarch
*gdbarch, gdbarch_addressable_memory_unit_size_ftype
*addressable_memory_unit_size);

+/* Given a list of registers, check if the target description has
changed. */
+
+typedef int (gdbarch_target_description_changed_p_ftype) (struct gdbarch
*gdbarch, ptid_t ptid, void *registers);
+extern int gdbarch_target_description_changed_p (struct gdbarch *gdbarch,
ptid_t ptid, void *registers);
+extern void set_gdbarch_target_description_changed_p (struct gdbarch
*gdbarch, gdbarch_target_description_changed_p_ftype
*target_description_changed_p);
+
+/* Given a list of registers, return a tdep info. */
+
+typedef void* (gdbarch_target_get_tdep_info_ftype) (void *registers);
+extern void* gdbarch_target_get_tdep_info (struct gdbarch *gdbarch, void
*registers);
+extern void set_gdbarch_target_get_tdep_info (struct gdbarch *gdbarch,
gdbarch_target_get_tdep_info_ftype *target_get_tdep_info);
+
 /* Definition for an unknown syscall, used basically in error-cases.  */
 #define UNKNOWN_SYSCALL (-1)

diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 
20bcfd794b2bbe59fddf0450e89f20b3770d1250..348eab8cd20a42a1189ea2dd6a01612ad
535b5db 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -339,6 +339,8 @@  struct gdbarch
   gdbarch_gcc_target_options_ftype *gcc_target_options;
   gdbarch_gnu_triplet_regexp_ftype *gnu_triplet_regexp;
   gdbarch_addressable_memory_unit_size_ftype
*addressable_memory_unit_size;
+  gdbarch_target_description_changed_p_ftype
*target_description_changed_p;
+  gdbarch_target_get_tdep_info_ftype *target_get_tdep_info;
 };

 /* Create a new ``struct gdbarch'' based on information provided by
@@ -447,6 +449,8 @@  gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->gcc_target_options = default_gcc_target_options;
   gdbarch->gnu_triplet_regexp = default_gnu_triplet_regexp;
   gdbarch->addressable_memory_unit_size =
default_addressable_memory_unit_size;
+  gdbarch->target_description_changed_p =
default_target_description_changed_p;
+  gdbarch->target_get_tdep_info = default_target_get_tdep_info;
   /* gdbarch_alloc() */

   return gdbarch;
@@ -696,6 +700,8 @@  verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of gcc_target_options, invalid_p == 0 */
   /* Skip verify of gnu_triplet_regexp, invalid_p == 0 */
   /* Skip verify of addressable_memory_unit_size, invalid_p == 0 */
+  /* Skip verify of target_description_changed_p, invalid_p == 0 */
+  /* Skip verify of target_get_tdep_info, invalid_p == 0 */
   std::string buf = ui_file_as_string (log);
   if (!buf.empty ())
     internal_error (__FILE__, __LINE__,
@@ -1415,6 +1421,12 @@  gdbarch_dump (struct gdbarch *gdbarch, struct
ui_file *file)
                       "gdbarch_dump: target_desc = %s\n",
                       host_address_to_string (gdbarch->target_desc));
   fprintf_unfiltered (file,
+                      "gdbarch_dump: target_description_changed_p =
<%s>\n",
+                      host_address_to_string
(gdbarch->target_description_changed_p));
+  fprintf_unfiltered (file,
+                      "gdbarch_dump: target_get_tdep_info = <%s>\n",
+                      host_address_to_string
(gdbarch->target_get_tdep_info));
+  fprintf_unfiltered (file,
                       "gdbarch_dump: gdbarch_unwind_pc_p() = %d\n",
                       gdbarch_unwind_pc_p (gdbarch));
   fprintf_unfiltered (file,
@@ -4962,6 +4974,40 @@  set_gdbarch_addressable_memory_unit_size (struct
gdbarch *gdbarch,
   gdbarch->addressable_memory_unit_size = addressable_memory_unit_size;
 }

+int
+gdbarch_target_description_changed_p (struct gdbarch *gdbarch, ptid_t
ptid, void *registers)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->target_description_changed_p != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_target_description_changed_p
called\n");
+  return gdbarch->target_description_changed_p (gdbarch, ptid, registers);
+}
+
+void
+set_gdbarch_target_description_changed_p (struct gdbarch *gdbarch,
+                  
gdbarch_target_description_changed_p_ftype target_description_changed_p)
+{
+  gdbarch->target_description_changed_p = target_description_changed_p;
+}
+
+void*
+gdbarch_target_get_tdep_info (struct gdbarch *gdbarch, void *registers)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->target_get_tdep_info != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_target_get_tdep_info
called\n");
+  return gdbarch->target_get_tdep_info (registers);
+}
+
+void
+set_gdbarch_target_get_tdep_info (struct gdbarch *gdbarch,
+                                  gdbarch_target_get_tdep_info_ftype
target_get_tdep_info)
+{
+  gdbarch->target_get_tdep_info = target_get_tdep_info;
+}
+

 /* Keep a registry of per-architecture data-pointers required by GDB
    modules.  */
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
+
 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);
+
+
 /* Read a packet from the remote machine, with error checking, and
    store it in *BUF.  Resize *BUF using xrealloc if necessary to hold
    the result, and update *SIZEOF_BUF.  If FOREVER, wait forever
diff --git a/gdb/remote.c b/gdb/remote.c
index 
ef6c54e1c421b18f1cb3ac24c52a1e5063ede427..03485d5a202f0c8a571a3ae8405d8d5b4
577a8d0 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6303,13 +6303,6 @@  remote_console_output (char *msg)
   gdb_flush (gdb_stdtarg);
 }

-typedef struct cached_reg
-{
-  int num;
-  gdb_byte data[MAX_REGISTER_SIZE];
-} cached_reg_t;
-
-DEF_VEC_O(cached_reg_t);

 typedef struct stop_reply
 {