* [PATCH 1/8] Add Aarch64 SVE target description
2018-05-11 10:53 [PATCH 0/8] Add SVE support for Aarch64 GDB Alan Hayward
@ 2018-05-11 10:53 ` Alan Hayward
2018-05-11 14:56 ` Eli Zaretskii
2018-05-31 11:56 ` Simon Marchi
2018-05-11 10:53 ` [PATCH 6/8] Aarch64 SVE pseudo register support Alan Hayward
` (7 subsequent siblings)
8 siblings, 2 replies; 47+ messages in thread
From: Alan Hayward @ 2018-05-11 10:53 UTC (permalink / raw)
To: gdb-patches; +Cc: nd, Alan Hayward
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.
2018-05-11 Alan Hayward <alan.hayward@arm.com>
gdb/
* aarch64-linux-nat.c (aarch64_linux_read_description):
Add parmeter zero.
* aarch64-linux-tdep.c (aarch64_linux_core_read_description):
Likewise.
* aarch64-tdep.c (tdesc_aarch64_list): Add.
(aarch64_read_description): Use VQ to index tdesc_aarch64_list.
(aarch64_gdbarch_init): Add parmeter zero.
* aarch64-tdep.h (aarch64_read_description): Add VQ parmeter.
* arch/aarch64.c (aarch64_create_target_description): Check VQ.
* arch/aarch64.h (aarch64_create_target_description): Add VQ.
parmeter.
* doc/gdb.texinfo: Describe SVE feature
* features/aarch64-sve.c: New file.
gdbserver/
* linux-aarch64-tdesc.c (aarch64_linux_read_description): Add
null VQ.
---
gdb/aarch64-linux-nat.c | 3 +-
gdb/aarch64-linux-tdep.c | 5 +-
gdb/aarch64-tdep.c | 30 +++++--
gdb/aarch64-tdep.h | 2 +-
gdb/arch/aarch64.c | 12 ++-
gdb/arch/aarch64.h | 5 +-
gdb/doc/gdb.texinfo | 4 +
gdb/features/aarch64-sve.c | 158 ++++++++++++++++++++++++++++++++++++
gdb/gdbserver/linux-aarch64-tdesc.c | 3 +-
9 files changed, 204 insertions(+), 18 deletions(-)
create mode 100644 gdb/features/aarch64-sve.c
diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 908b83a49a..9a6ee91d2d 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -541,7 +541,8 @@ aarch64_linux_nat_target::read_description ()
if (ret == 0)
return tdesc_arm_with_neon;
else
- return aarch64_read_description ();
+ /* SVE not yet supported. */
+ return aarch64_read_description (0);
}
/* Convert a native/host siginfo object, into/from the siginfo in the
diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index 1f3e888e40..b84dcfa9ed 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -233,7 +233,8 @@ aarch64_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
NULL, cb_data);
}
-/* Implement the "core_read_description" gdbarch method. */
+/* Implement the "core_read_description" gdbarch method. SVE not yet
+ supported. */
static const struct target_desc *
aarch64_linux_core_read_description (struct gdbarch *gdbarch,
@@ -244,7 +245,7 @@ aarch64_linux_core_read_description (struct gdbarch *gdbarch,
if (target_auxv_search (target, AT_HWCAP, &aarch64_hwcap) != 1)
return NULL;
- return aarch64_read_description ();
+ return aarch64_read_description (0);
}
/* Implementation of `gdbarch_stap_is_single_operand', as defined in
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 01566b475f..806a3dac55 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -70,6 +70,9 @@
#define AARCH64_H0_REGNUM (AARCH64_S0_REGNUM + 32)
#define AARCH64_B0_REGNUM (AARCH64_H0_REGNUM + 32)
+/* All possible aarch64 target descriptors. */
+struct target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1];
+
/* The standard register names, and all the valid aliases for them. */
static const struct
{
@@ -2827,18 +2830,26 @@ aarch64_displaced_step_hw_singlestep (struct gdbarch *gdbarch,
return 1;
}
-/* Get the correct target description. */
+/* Get the correct target description for the given VQ value.
+ If VQ is zero then it is assumed SVE is not supported.
+ (It is not possible to set VQ to zero on an SVE system). */
const target_desc *
-aarch64_read_description ()
+aarch64_read_description (long vq)
{
- static target_desc *aarch64_tdesc = NULL;
- target_desc **tdesc = &aarch64_tdesc;
+ if (vq > AARCH64_MAX_SVE_VQ)
+ error (_("VQ is %ld, maximum supported value is %d"), vq,
+ AARCH64_MAX_SVE_VQ);
+
+ struct target_desc *tdesc = tdesc_aarch64_list[vq];
- if (*tdesc == NULL)
- *tdesc = aarch64_create_target_description ();
+ if (tdesc == NULL)
+ {
+ tdesc = aarch64_create_target_description (vq);
+ tdesc_aarch64_list[vq] = tdesc;
+ }
- return *tdesc;
+ return tdesc;
}
/* 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);
gdb_assert (tdesc);
@@ -3077,7 +3089,7 @@ When on, AArch64 specific debugging is enabled."),
selftests::register_test ("aarch64-process-record",
selftests::aarch64_process_record_test);
selftests::record_xml_tdesc ("aarch64.xml",
- aarch64_create_target_description ());
+ aarch64_create_target_description (0));
#endif
}
diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
index c806125fb7..c9fd7b3578 100644
--- a/gdb/aarch64-tdep.h
+++ b/gdb/aarch64-tdep.h
@@ -75,7 +75,7 @@ struct gdbarch_tdep
int (*aarch64_syscall_record) (struct regcache *regcache, unsigned long svc_number);
};
-const target_desc *aarch64_read_description ();
+const target_desc *aarch64_read_description (long vq);
extern int aarch64_process_record (struct gdbarch *gdbarch,
struct regcache *regcache, CORE_ADDR addr);
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. */
target_desc *
-aarch64_create_target_description ()
+aarch64_create_target_description (long vq)
{
target_desc *tdesc = allocate_target_description ();
@@ -36,7 +38,11 @@ aarch64_create_target_description ()
long regnum = 0;
regnum = create_feature_aarch64_core (tdesc, regnum);
- regnum = create_feature_aarch64_fpu (tdesc, regnum);
+
+ if (vq == 0)
+ regnum = create_feature_aarch64_fpu (tdesc, regnum);
+ else
+ regnum = create_feature_aarch64_sve (tdesc, regnum, vq);
return tdesc;
}
diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
index 86185f596a..1846e04163 100644
--- a/gdb/arch/aarch64.h
+++ b/gdb/arch/aarch64.h
@@ -22,7 +22,7 @@
#include "common/tdesc.h"
-target_desc *aarch64_create_target_description ();
+target_desc *aarch64_create_target_description (long vq);
/* Register numbers of various important registers. */
enum aarch64_regnum
@@ -48,4 +48,7 @@ enum aarch64_regnum
#define AARCH64_V_REGS_NUM 32
#define AARCH64_NUM_REGS AARCH64_FPCR_REGNUM + 1
+/* Maximum supported VQ value. Increase if required. */
+#define AARCH64_MAX_SVE_VQ 16
+
#endif /* ARCH_AARCH64_H */
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 28f083f96e..04f811032a 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -42134,6 +42134,10 @@ The @samp{org.gnu.gdb.aarch64.fpu} feature is optional. If present,
it should contain registers @samp{v0} through @samp{v31}, @samp{fpsr},
and @samp{fpcr}.
+The @samp{org.gnu.gdb.aarch64.sve} feature is optional. If present,
+it should contain registers @samp{z0} through @samp{z31}, @samp{p0}
+through @samp{p15}, @samp{ffr} and @samp{vg}.
+
@node ARC Features
@subsection ARC Features
@cindex target descriptions, ARC Features
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.
+
+ This file is part of GDB.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include "common/tdesc.h"
+
+/* This function is NOT auto generated from xml. Create the aarch64 with SVE
+ feature into RESULT, where SCALE is the number of 128 bit chunks in a Z
+ register. */
+
+static int
+create_feature_aarch64_sve (struct target_desc *result, long regnum,
+ int scale)
+{
+ struct tdesc_feature *feature;
+ tdesc_type *element_type, *field_type;
+ tdesc_type_with_fields *type_with_fields;
+
+ feature = tdesc_create_feature (result, "org.gnu.gdb.aarch64.sve");
+
+ element_type = tdesc_named_type (feature, "ieee_double");
+ tdesc_create_vector (feature, "svevdf", element_type, 2 * scale);
+
+ element_type = tdesc_named_type (feature, "uint64");
+ tdesc_create_vector (feature, "svevdu", element_type, 2 * scale);
+
+ element_type = tdesc_named_type (feature, "int64");
+ tdesc_create_vector (feature, "svevds", element_type, 2 * scale);
+
+ element_type = tdesc_named_type (feature, "ieee_single");
+ tdesc_create_vector (feature, "svevsf", element_type, 4 * scale);
+
+ element_type = tdesc_named_type (feature, "uint32");
+ tdesc_create_vector (feature, "svevsu", element_type, 4 * scale);
+
+ element_type = tdesc_named_type (feature, "int32");
+ tdesc_create_vector (feature, "svevss", element_type, 4 * scale);
+
+ element_type = tdesc_named_type (feature, "uint16");
+ tdesc_create_vector (feature, "svevhu", element_type, 8 * scale);
+
+ element_type = tdesc_named_type (feature, "int16");
+ tdesc_create_vector (feature, "svevhs", element_type, 8 * scale);
+
+ element_type = tdesc_named_type (feature, "uint8");
+ tdesc_create_vector (feature, "svevbu", element_type, 16 * scale);
+
+ element_type = tdesc_named_type (feature, "int8");
+ tdesc_create_vector (feature, "svevbs", element_type, 16 * scale);
+
+ type_with_fields = tdesc_create_union (feature, "svevnd");
+ field_type = tdesc_named_type (feature, "svevdf");
+ tdesc_add_field (type_with_fields, "f", field_type);
+ field_type = tdesc_named_type (feature, "svevdu");
+ tdesc_add_field (type_with_fields, "u", field_type);
+ field_type = tdesc_named_type (feature, "svevds");
+ tdesc_add_field (type_with_fields, "s", field_type);
+
+ type_with_fields = tdesc_create_union (feature, "svevns");
+ field_type = tdesc_named_type (feature, "svevsf");
+ tdesc_add_field (type_with_fields, "f", field_type);
+ field_type = tdesc_named_type (feature, "svevsu");
+ tdesc_add_field (type_with_fields, "u", field_type);
+ field_type = tdesc_named_type (feature, "svevss");
+ tdesc_add_field (type_with_fields, "s", field_type);
+
+ type_with_fields = tdesc_create_union (feature, "svevnh");
+ field_type = tdesc_named_type (feature, "svevhu");
+ tdesc_add_field (type_with_fields, "u", field_type);
+ field_type = tdesc_named_type (feature, "svevhs");
+ tdesc_add_field (type_with_fields, "s", field_type);
+
+ type_with_fields = tdesc_create_union (feature, "svevnb");
+ field_type = tdesc_named_type (feature, "svevbu");
+ tdesc_add_field (type_with_fields, "u", field_type);
+ field_type = tdesc_named_type (feature, "svevbs");
+ tdesc_add_field (type_with_fields, "s", field_type);
+
+ type_with_fields = tdesc_create_union (feature, "svev");
+ field_type = tdesc_named_type (feature, "svevnd");
+ tdesc_add_field (type_with_fields, "d", field_type);
+ field_type = tdesc_named_type (feature, "svevns");
+ tdesc_add_field (type_with_fields, "s", field_type);
+ field_type = tdesc_named_type (feature, "svevnh");
+ tdesc_add_field (type_with_fields, "h", field_type);
+ field_type = tdesc_named_type (feature, "svevnb");
+ tdesc_add_field (type_with_fields, "b", field_type);
+
+ field_type = tdesc_named_type (feature, "uint8");
+ tdesc_create_vector (feature, "svep", field_type, 2 * scale);
+
+ tdesc_create_reg (feature, "z0", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z1", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z2", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z3", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z4", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z5", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z6", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z7", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z8", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z9", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z10", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z11", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z12", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z13", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z14", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z15", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z16", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z17", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z18", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z19", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z20", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z21", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z22", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z23", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z24", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z25", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z26", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z27", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z28", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z29", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z30", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "z31", regnum++, 1, NULL, 128 * scale, "svev");
+ tdesc_create_reg (feature, "fpsr", regnum++, 1, NULL, 32, "int");
+ tdesc_create_reg (feature, "fpcr", regnum++, 1, NULL, 32, "int");
+ tdesc_create_reg (feature, "p0", regnum++, 1, NULL, 16 * scale, "svep");
+ tdesc_create_reg (feature, "p1", regnum++, 1, NULL, 16 * scale, "svep");
+ tdesc_create_reg (feature, "p2", regnum++, 1, NULL, 16 * scale, "svep");
+ tdesc_create_reg (feature, "p3", regnum++, 1, NULL, 16 * scale, "svep");
+ tdesc_create_reg (feature, "p4", regnum++, 1, NULL, 16 * scale, "svep");
+ tdesc_create_reg (feature, "p5", regnum++, 1, NULL, 16 * scale, "svep");
+ tdesc_create_reg (feature, "p6", regnum++, 1, NULL, 16 * scale, "svep");
+ tdesc_create_reg (feature, "p7", regnum++, 1, NULL, 16 * scale, "svep");
+ tdesc_create_reg (feature, "p8", regnum++, 1, NULL, 16 * scale, "svep");
+ tdesc_create_reg (feature, "p9", regnum++, 1, NULL, 16 * scale, "svep");
+ tdesc_create_reg (feature, "p10", regnum++, 1, NULL, 16 * scale, "svep");
+ tdesc_create_reg (feature, "p11", regnum++, 1, NULL, 16 * scale, "svep");
+ tdesc_create_reg (feature, "p12", regnum++, 1, NULL, 16 * scale, "svep");
+ tdesc_create_reg (feature, "p13", regnum++, 1, NULL, 16 * scale, "svep");
+ tdesc_create_reg (feature, "p14", regnum++, 1, NULL, 16 * scale, "svep");
+ tdesc_create_reg (feature, "p15", regnum++, 1, NULL, 16 * scale, "svep");
+ tdesc_create_reg (feature, "ffr", regnum++, 1, NULL, 16 * scale, "svep");
+ tdesc_create_reg (feature, "vg", regnum++, 1, NULL, 64, "int");
+ return regnum;
+}
diff --git a/gdb/gdbserver/linux-aarch64-tdesc.c b/gdb/gdbserver/linux-aarch64-tdesc.c
index 9f7b9e5c85..3b36c47cfa 100644
--- a/gdb/gdbserver/linux-aarch64-tdesc.c
+++ b/gdb/gdbserver/linux-aarch64-tdesc.c
@@ -32,7 +32,8 @@ aarch64_linux_read_description ()
if (*tdesc == NULL)
{
- *tdesc = aarch64_create_target_description ();
+ /* SVE not yet supported. */
+ *tdesc = aarch64_create_target_description (0);
init_target_desc (*tdesc);
--
2.15.1 (Apple Git-101)
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 1/8] Add Aarch64 SVE target description
2018-05-11 10:53 ` [PATCH 1/8] Add Aarch64 SVE target description Alan Hayward
@ 2018-05-11 14:56 ` Eli Zaretskii
2018-05-11 16:46 ` Alan Hayward
2018-05-31 11:56 ` Simon Marchi
1 sibling, 1 reply; 47+ messages in thread
From: Eli Zaretskii @ 2018-05-11 14:56 UTC (permalink / raw)
To: Alan Hayward; +Cc: gdb-patches, nd
> From: Alan Hayward <alan.hayward@arm.com>
> Cc: nd@arm.com, Alan Hayward <alan.hayward@arm.com>
> Date: Fri, 11 May 2018 11:52:49 +0100
>
> * doc/gdb.texinfo: Describe SVE feature
OK for this part.
Thanks.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/8] Add Aarch64 SVE target description
2018-05-11 14:56 ` Eli Zaretskii
@ 2018-05-11 16:46 ` Alan Hayward
0 siblings, 0 replies; 47+ messages in thread
From: Alan Hayward @ 2018-05-11 16:46 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches, nd
> On 11 May 2018, at 13:12, Eli Zaretskii <eliz@gnu.org> wrote:
>
>> From: Alan Hayward <alan.hayward@arm.com>
>> Cc: nd@arm.com, Alan Hayward <alan.hayward@arm.com>
>> Date: Fri, 11 May 2018 11:52:49 +0100
>>
>> * doc/gdb.texinfo: Describe SVE feature
>
> OK for this part.
>
Thanks for the quick review!
Alan.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/8] Add Aarch64 SVE target description
2018-05-11 10:53 ` [PATCH 1/8] Add Aarch64 SVE target description Alan Hayward
2018-05-11 14:56 ` Eli Zaretskii
@ 2018-05-31 11:56 ` Simon Marchi
2018-05-31 14:12 ` Alan Hayward
1 sibling, 1 reply; 47+ messages in thread
From: Simon Marchi @ 2018-05-31 11:56 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
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
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 1/8] Add Aarch64 SVE target description
2018-05-31 11:56 ` Simon Marchi
@ 2018-05-31 14:12 ` Alan Hayward
0 siblings, 0 replies; 47+ messages in thread
From: Alan Hayward @ 2018-05-31 14:12 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches, nd
Thanks for the reviews.
Pushed this with changes as below.
> On 31 May 2018, at 12:09, Simon Marchi <simon.marchi@ericsson.com> 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.
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 6/8] Aarch64 SVE pseudo register support
2018-05-11 10:53 [PATCH 0/8] Add SVE support for Aarch64 GDB Alan Hayward
2018-05-11 10:53 ` [PATCH 1/8] Add Aarch64 SVE target description Alan Hayward
@ 2018-05-11 10:53 ` Alan Hayward
2018-05-31 13:26 ` Simon Marchi
2018-05-31 14:59 ` Pedro Alves
2018-05-11 10:53 ` [PATCH 8/8] Ptrace support for Aarch64 SVE Alan Hayward
` (6 subsequent siblings)
8 siblings, 2 replies; 47+ messages in thread
From: Alan Hayward @ 2018-05-11 10:53 UTC (permalink / raw)
To: gdb-patches; +Cc: nd, Alan Hayward
Add the functionality for reading/writing psuedo registers.
On SVE the V registers are pseudo registers. This is supported
by adding AARCH64_SVE_V0_REGNUM.
2018-05-11 Alan Hayward <alan.hayward@arm.com>
* aarch64-tdep.c (AARCH64_SVE_V0_REGNUM): Add define.
(aarch64_vnv_type): Add function.
(aarch64_pseudo_register_name): Add V regs for SVE.
(aarch64_pseudo_register_type): Likewise.
(aarch64_pseudo_register_reggroup_p): Likewise.
(aarch64_pseudo_read_value_2): Use V0 offset for SVE
(aarch64_pseudo_read_value): Add V regs for SVE.
(aarch64_pseudo_write_2): Use V0 offset for SVE
(aarch64_pseudo_write): Add V regs for SVE.
* aarch64-tdep.h (struct gdbarch_tdep): Add vnv_type.
---
gdb/aarch64-tdep.c | 151 +++++++++++++++++++++++++++++++++++++++++++++--------
gdb/aarch64-tdep.h | 1 +
2 files changed, 130 insertions(+), 22 deletions(-)
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 003fefb3c9..6a40a081cb 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -69,6 +69,7 @@
#define AARCH64_S0_REGNUM (AARCH64_D0_REGNUM + 32)
#define AARCH64_H0_REGNUM (AARCH64_S0_REGNUM + 32)
#define AARCH64_B0_REGNUM (AARCH64_H0_REGNUM + 32)
+#define AARCH64_SVE_V0_REGNUM (AARCH64_B0_REGNUM + 32)
/* All possible aarch64 target descriptors. */
struct target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1];
@@ -1766,6 +1767,33 @@ aarch64_vnb_type (struct gdbarch *gdbarch)
return tdep->vnb_type;
}
+/* Return the type for an AdvSISD V register. */
+
+static struct type *
+aarch64_vnv_type (struct gdbarch *gdbarch)
+{
+ struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
+ if (tdep->vnv_type == NULL)
+ {
+ struct type *t;
+ struct type *elem;
+
+ t = arch_composite_type (gdbarch, "__gdb_builtin_type_vnv",
+ TYPE_CODE_UNION);
+
+ append_composite_type_field (t, "d", aarch64_vnd_type (gdbarch));
+ append_composite_type_field (t, "s", aarch64_vns_type (gdbarch));
+ append_composite_type_field (t, "h", aarch64_vnh_type (gdbarch));
+ append_composite_type_field (t, "b", aarch64_vnb_type (gdbarch));
+ append_composite_type_field (t, "q", aarch64_vnq_type (gdbarch));
+
+ tdep->vnv_type = t;
+ }
+
+ return tdep->vnv_type;
+}
+
/* Implement the "dwarf2_reg_to_regnum" gdbarch method. */
static int
@@ -2114,6 +2142,8 @@ aarch64_gen_return_address (struct gdbarch *gdbarch,
static const char *
aarch64_pseudo_register_name (struct gdbarch *gdbarch, int regnum)
{
+ struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
static const char *const q_name[] =
{
"q0", "q1", "q2", "q3",
@@ -2191,6 +2221,25 @@ aarch64_pseudo_register_name (struct gdbarch *gdbarch, int regnum)
if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
return b_name[regnum - AARCH64_B0_REGNUM];
+ if (tdep->has_sve ())
+ {
+ static const char *const sve_v_name[] =
+ {
+ "v0", "v1", "v2", "v3",
+ "v4", "v5", "v6", "v7",
+ "v8", "v9", "v10", "v11",
+ "v12", "v13", "v14", "v15",
+ "v16", "v17", "v18", "v19",
+ "v20", "v21", "v22", "v23",
+ "v24", "v25", "v26", "v27",
+ "v28", "v29", "v30", "v31",
+ };
+
+ if (regnum >= AARCH64_SVE_V0_REGNUM
+ && regnum < AARCH64_SVE_V0_REGNUM + AARCH64_V_REGS_NUM)
+ return sve_v_name[regnum - AARCH64_SVE_V0_REGNUM];
+ }
+
internal_error (__FILE__, __LINE__,
_("aarch64_pseudo_register_name: bad register number %d"),
regnum);
@@ -2201,6 +2250,8 @@ aarch64_pseudo_register_name (struct gdbarch *gdbarch, int regnum)
static struct type *
aarch64_pseudo_register_type (struct gdbarch *gdbarch, int regnum)
{
+ struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
regnum -= gdbarch_num_regs (gdbarch);
if (regnum >= AARCH64_Q0_REGNUM && regnum < AARCH64_Q0_REGNUM + 32)
@@ -2218,6 +2269,10 @@ aarch64_pseudo_register_type (struct gdbarch *gdbarch, int regnum)
if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
return aarch64_vnb_type (gdbarch);
+ if (tdep->has_sve () && regnum >= AARCH64_SVE_V0_REGNUM
+ && regnum < AARCH64_SVE_V0_REGNUM + 32)
+ return aarch64_vnv_type (gdbarch);
+
internal_error (__FILE__, __LINE__,
_("aarch64_pseudo_register_type: bad register number %d"),
regnum);
@@ -2229,6 +2284,8 @@ static int
aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
struct reggroup *group)
{
+ struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
regnum -= gdbarch_num_regs (gdbarch);
if (regnum >= AARCH64_Q0_REGNUM && regnum < AARCH64_Q0_REGNUM + 32)
@@ -2243,6 +2300,9 @@ aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
return group == all_reggroup || group == vector_reggroup;
else if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
return group == all_reggroup || group == vector_reggroup;
+ else if (tdep->has_sve () && regnum >= AARCH64_SVE_V0_REGNUM
+ && regnum < AARCH64_SVE_V0_REGNUM + 32)
+ return group == all_reggroup || group == vector_reggroup;
return group == all_reggroup;
}
@@ -2250,17 +2310,30 @@ aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
/* Inner version of aarch64_pseudo_read_value. */
static struct value *
-aarch64_pseudo_read_value_2 (readable_regcache *regcache, int regnum_offset,
+aarch64_pseudo_read_value_2 (struct gdbarch *gdbarch,
+ readable_regcache *regcache, int regnum_offset,
int regsize, struct value *result_value)
{
- gdb_byte reg_buf[V_REGISTER_SIZE];
+ struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+ gdb_byte v_buf[V_REGISTER_SIZE], *reg_buf;
+ gdb_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);
unsigned v_regnum = AARCH64_V0_REGNUM + regnum_offset;
+ /* Enough space to read a full vector register. */
+ if (tdep->has_sve ())
+ reg_buf = (gdb_byte *) xmalloc (register_size (gdbarch, AARCH64_V0_REGNUM));
+ else
+ reg_buf = v_buf;
+
if (regcache->raw_read (v_regnum, reg_buf) != REG_VALID)
mark_value_bytes_unavailable (result_value, 0,
TYPE_LENGTH (value_type (result_value)));
else
memcpy (value_contents_raw (result_value), reg_buf, regsize);
+
+ if (tdep->has_sve ())
+ xfree (reg_buf);
+
return result_value;
}
@@ -2270,6 +2343,7 @@ static struct value *
aarch64_pseudo_read_value (struct gdbarch *gdbarch, readable_regcache *regcache,
int regnum)
{
+ struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
struct value *result_value = allocate_value (register_type (gdbarch, regnum));
VALUE_LVAL (result_value) = lval_register;
@@ -2278,45 +2352,67 @@ aarch64_pseudo_read_value (struct gdbarch *gdbarch, readable_regcache *regcache,
regnum -= gdbarch_num_regs (gdbarch);
if (regnum >= AARCH64_Q0_REGNUM && regnum < AARCH64_Q0_REGNUM + 32)
- return aarch64_pseudo_read_value_2 (regcache, regnum - AARCH64_Q0_REGNUM,
+ return aarch64_pseudo_read_value_2 (gdbarch, regcache,
+ regnum - AARCH64_Q0_REGNUM,
Q_REGISTER_SIZE, result_value);
if (regnum >= AARCH64_D0_REGNUM && regnum < AARCH64_D0_REGNUM + 32)
- return aarch64_pseudo_read_value_2 (regcache, regnum - AARCH64_D0_REGNUM,
+ return aarch64_pseudo_read_value_2 (gdbarch, regcache,
+ regnum - AARCH64_D0_REGNUM,
D_REGISTER_SIZE, result_value);
if (regnum >= AARCH64_S0_REGNUM && regnum < AARCH64_S0_REGNUM + 32)
- return aarch64_pseudo_read_value_2 (regcache, regnum - AARCH64_S0_REGNUM,
+ return aarch64_pseudo_read_value_2 (gdbarch, regcache,
+ regnum - AARCH64_S0_REGNUM,
S_REGISTER_SIZE, result_value);
if (regnum >= AARCH64_H0_REGNUM && regnum < AARCH64_H0_REGNUM + 32)
- return aarch64_pseudo_read_value_2 (regcache, regnum - AARCH64_H0_REGNUM,
+ return aarch64_pseudo_read_value_2 (gdbarch, regcache,
+ regnum - AARCH64_H0_REGNUM,
H_REGISTER_SIZE, result_value);
if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
- return aarch64_pseudo_read_value_2 (regcache, regnum - AARCH64_B0_REGNUM,
+ return aarch64_pseudo_read_value_2 (gdbarch, regcache,
+ regnum - AARCH64_B0_REGNUM,
B_REGISTER_SIZE, result_value);
+ if (tdep->has_sve () && regnum >= AARCH64_SVE_V0_REGNUM
+ && regnum < AARCH64_SVE_V0_REGNUM + 32)
+ return aarch64_pseudo_read_value_2 (gdbarch, regcache,
+ regnum - AARCH64_SVE_V0_REGNUM,
+ V_REGISTER_SIZE, result_value);
+
gdb_assert_not_reached ("regnum out of bound");
}
/* Inner version of aarch64_pseudo_write. */
static void
-aarch64_pseudo_write_2 (struct regcache *regcache, int regnum_offset,
- int regsize, const gdb_byte *buf)
+aarch64_pseudo_write_2 (struct gdbarch *gdbarch, struct regcache *regcache,
+ int regnum_offset, int regsize, const gdb_byte *buf)
{
- gdb_byte reg_buf[V_REGISTER_SIZE];
+ struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+ gdb_byte v_buf[V_REGISTER_SIZE], *reg_buf;
+ gdb_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);
unsigned v_regnum = AARCH64_V0_REGNUM + regnum_offset;
+ /* Enough space to write a full vector register. */
+ if (tdep->has_sve ())
+ reg_buf = (gdb_byte *) xmalloc (register_size (gdbarch, AARCH64_V0_REGNUM));
+ else
+ reg_buf = v_buf;
+
/* Ensure the register buffer is zero, we want gdb writes of the
various 'scalar' pseudo registers to behavior like architectural
writes, register width bytes are written the remainder are set to
zero. */
- memset (reg_buf, 0, sizeof (reg_buf));
+ memset (reg_buf, 0, register_size (gdbarch, AARCH64_V0_REGNUM));
memcpy (reg_buf, buf, regsize);
regcache->raw_write (v_regnum, reg_buf);
+
+ if (tdep->has_sve ())
+ xfree (reg_buf);
}
/* Implement the "pseudo_register_write" gdbarch method. */
@@ -2325,28 +2421,39 @@ static void
aarch64_pseudo_write (struct gdbarch *gdbarch, struct regcache *regcache,
int regnum, const gdb_byte *buf)
{
-
+ struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
regnum -= gdbarch_num_regs (gdbarch);
if (regnum >= AARCH64_Q0_REGNUM && regnum < AARCH64_Q0_REGNUM + 32)
- return aarch64_pseudo_write_2 (regcache, regnum - AARCH64_Q0_REGNUM,
- Q_REGISTER_SIZE, buf);
+ return aarch64_pseudo_write_2 (gdbarch, regcache,
+ regnum - AARCH64_Q0_REGNUM, Q_REGISTER_SIZE,
+ buf);
if (regnum >= AARCH64_D0_REGNUM && regnum < AARCH64_D0_REGNUM + 32)
- return aarch64_pseudo_write_2 (regcache, regnum - AARCH64_D0_REGNUM,
- D_REGISTER_SIZE, buf);
+ return aarch64_pseudo_write_2 (gdbarch, regcache,
+ regnum - AARCH64_D0_REGNUM, D_REGISTER_SIZE,
+ buf);
if (regnum >= AARCH64_S0_REGNUM && regnum < AARCH64_S0_REGNUM + 32)
- return aarch64_pseudo_write_2 (regcache, regnum - AARCH64_S0_REGNUM,
- S_REGISTER_SIZE, buf);
+ return aarch64_pseudo_write_2 (gdbarch, regcache,
+ regnum - AARCH64_S0_REGNUM, S_REGISTER_SIZE,
+ buf);
if (regnum >= AARCH64_H0_REGNUM && regnum < AARCH64_H0_REGNUM + 32)
- return aarch64_pseudo_write_2 (regcache, regnum - AARCH64_H0_REGNUM,
- H_REGISTER_SIZE, buf);
+ return aarch64_pseudo_write_2 (gdbarch, regcache,
+ regnum - AARCH64_H0_REGNUM, H_REGISTER_SIZE,
+ buf);
if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
- return aarch64_pseudo_write_2 (regcache, regnum - AARCH64_B0_REGNUM,
- B_REGISTER_SIZE, buf);
+ return aarch64_pseudo_write_2 (gdbarch, regcache,
+ regnum - AARCH64_B0_REGNUM, B_REGISTER_SIZE,
+ buf);
+
+ if (tdep->has_sve () && regnum >= AARCH64_SVE_V0_REGNUM
+ && regnum < AARCH64_SVE_V0_REGNUM + 32)
+ return aarch64_pseudo_write_2 (gdbarch, regcache,
+ regnum - AARCH64_SVE_V0_REGNUM,
+ V_REGISTER_SIZE, buf);
gdb_assert_not_reached ("regnum out of bound");
}
diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
index 046de6228f..f4c04d7867 100644
--- a/gdb/aarch64-tdep.h
+++ b/gdb/aarch64-tdep.h
@@ -70,6 +70,7 @@ struct gdbarch_tdep
struct type *vns_type;
struct type *vnh_type;
struct type *vnb_type;
+ struct type *vnv_type;
/* syscall record. */
int (*aarch64_syscall_record) (struct regcache *regcache, unsigned long svc_number);
--
2.15.1 (Apple Git-101)
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 6/8] Aarch64 SVE pseudo register support
2018-05-11 10:53 ` [PATCH 6/8] Aarch64 SVE pseudo register support Alan Hayward
@ 2018-05-31 13:26 ` Simon Marchi
2018-06-04 13:29 ` Alan Hayward
2018-05-31 14:59 ` Pedro Alves
1 sibling, 1 reply; 47+ messages in thread
From: Simon Marchi @ 2018-05-31 13:26 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
On 2018-05-11 06:52 AM, Alan Hayward wrote:
> Add the functionality for reading/writing psuedo registers.
"pseudo"
>
> On SVE the V registers are pseudo registers. This is supported
> by adding AARCH64_SVE_V0_REGNUM.
>
> 2018-05-11 Alan Hayward <alan.hayward@arm.com>
>
> * aarch64-tdep.c (AARCH64_SVE_V0_REGNUM): Add define.
> (aarch64_vnv_type): Add function.
> (aarch64_pseudo_register_name): Add V regs for SVE.
> (aarch64_pseudo_register_type): Likewise.
> (aarch64_pseudo_register_reggroup_p): Likewise.
> (aarch64_pseudo_read_value_2): Use V0 offset for SVE
> (aarch64_pseudo_read_value): Add V regs for SVE.
> (aarch64_pseudo_write_2): Use V0 offset for SVE
> (aarch64_pseudo_write): Add V regs for SVE.
> * aarch64-tdep.h (struct gdbarch_tdep): Add vnv_type.
> ---
> gdb/aarch64-tdep.c | 151 +++++++++++++++++++++++++++++++++++++++++++++--------
> gdb/aarch64-tdep.h | 1 +
> 2 files changed, 130 insertions(+), 22 deletions(-)
>
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 003fefb3c9..6a40a081cb 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -69,6 +69,7 @@
> #define AARCH64_S0_REGNUM (AARCH64_D0_REGNUM + 32)
> #define AARCH64_H0_REGNUM (AARCH64_S0_REGNUM + 32)
> #define AARCH64_B0_REGNUM (AARCH64_H0_REGNUM + 32)
> +#define AARCH64_SVE_V0_REGNUM (AARCH64_B0_REGNUM + 32)
>
> /* All possible aarch64 target descriptors. */
> struct target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1];
> @@ -1766,6 +1767,33 @@ aarch64_vnb_type (struct gdbarch *gdbarch)
> return tdep->vnb_type;
> }
>
> +/* Return the type for an AdvSISD V register. */
> +
> +static struct type *
> +aarch64_vnv_type (struct gdbarch *gdbarch)
> +{
> + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +
> + if (tdep->vnv_type == NULL)
> + {
> + struct type *t;
> + struct type *elem;
> +
> + t = arch_composite_type (gdbarch, "__gdb_builtin_type_vnv",
> + TYPE_CODE_UNION);
> +
> + append_composite_type_field (t, "d", aarch64_vnd_type (gdbarch));
> + append_composite_type_field (t, "s", aarch64_vns_type (gdbarch));
> + append_composite_type_field (t, "h", aarch64_vnh_type (gdbarch));
> + append_composite_type_field (t, "b", aarch64_vnb_type (gdbarch));
> + append_composite_type_field (t, "q", aarch64_vnq_type (gdbarch));
> +
> + tdep->vnv_type = t;
> + }
> +
> + return tdep->vnv_type;
> +}
> +
> /* Implement the "dwarf2_reg_to_regnum" gdbarch method. */
>
> static int
> @@ -2114,6 +2142,8 @@ aarch64_gen_return_address (struct gdbarch *gdbarch,
> static const char *
> aarch64_pseudo_register_name (struct gdbarch *gdbarch, int regnum)
> {
> + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +
> static const char *const q_name[] =
> {
> "q0", "q1", "q2", "q3",
> @@ -2191,6 +2221,25 @@ aarch64_pseudo_register_name (struct gdbarch *gdbarch, int regnum)
> if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
> return b_name[regnum - AARCH64_B0_REGNUM];
>
> + if (tdep->has_sve ())
> + {
> + static const char *const sve_v_name[] =
> + {
> + "v0", "v1", "v2", "v3",
> + "v4", "v5", "v6", "v7",
> + "v8", "v9", "v10", "v11",
> + "v12", "v13", "v14", "v15",
> + "v16", "v17", "v18", "v19",
> + "v20", "v21", "v22", "v23",
> + "v24", "v25", "v26", "v27",
> + "v28", "v29", "v30", "v31",
> + };
> +
> + if (regnum >= AARCH64_SVE_V0_REGNUM
> + && regnum < AARCH64_SVE_V0_REGNUM + AARCH64_V_REGS_NUM)
> + return sve_v_name[regnum - AARCH64_SVE_V0_REGNUM];
> + }
> +
> internal_error (__FILE__, __LINE__,
> _("aarch64_pseudo_register_name: bad register number %d"),
> regnum);
> @@ -2201,6 +2250,8 @@ aarch64_pseudo_register_name (struct gdbarch *gdbarch, int regnum)
> static struct type *
> aarch64_pseudo_register_type (struct gdbarch *gdbarch, int regnum)
> {
> + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +
> regnum -= gdbarch_num_regs (gdbarch);
>
> if (regnum >= AARCH64_Q0_REGNUM && regnum < AARCH64_Q0_REGNUM + 32)
> @@ -2218,6 +2269,10 @@ aarch64_pseudo_register_type (struct gdbarch *gdbarch, int regnum)
> if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
> return aarch64_vnb_type (gdbarch);
>
> + if (tdep->has_sve () && regnum >= AARCH64_SVE_V0_REGNUM
> + && regnum < AARCH64_SVE_V0_REGNUM + 32)
> + return aarch64_vnv_type (gdbarch);
> +
> internal_error (__FILE__, __LINE__,
> _("aarch64_pseudo_register_type: bad register number %d"),
> regnum);
> @@ -2229,6 +2284,8 @@ static int
> aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
> struct reggroup *group)
> {
> + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +
> regnum -= gdbarch_num_regs (gdbarch);
>
> if (regnum >= AARCH64_Q0_REGNUM && regnum < AARCH64_Q0_REGNUM + 32)
> @@ -2243,6 +2300,9 @@ aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
> return group == all_reggroup || group == vector_reggroup;
> else if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
> return group == all_reggroup || group == vector_reggroup;
> + else if (tdep->has_sve () && regnum >= AARCH64_SVE_V0_REGNUM
> + && regnum < AARCH64_SVE_V0_REGNUM + 32)
> + return group == all_reggroup || group == vector_reggroup;
>
> return group == all_reggroup;
> }
> @@ -2250,17 +2310,30 @@ aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
> /* Inner version of aarch64_pseudo_read_value. */
>
> static struct value *
> -aarch64_pseudo_read_value_2 (readable_regcache *regcache, int regnum_offset,
> +aarch64_pseudo_read_value_2 (struct gdbarch *gdbarch,
> + readable_regcache *regcache, int regnum_offset,
> int regsize, struct value *result_value)
> {
> - gdb_byte reg_buf[V_REGISTER_SIZE];
> + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> + gdb_byte v_buf[V_REGISTER_SIZE], *reg_buf;
> + gdb_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);
> unsigned v_regnum = AARCH64_V0_REGNUM + regnum_offset;
>
> + /* Enough space to read a full vector register. */
> + if (tdep->has_sve ())
> + reg_buf = (gdb_byte *) xmalloc (register_size (gdbarch, AARCH64_V0_REGNUM));
> + else
> + reg_buf = v_buf;
If the size of a register (even with SVE) will always be reasonable to allocate on the
stack, maybe you could just use
gdb_byte reg_buf[register_size (gdbarch, AARCH64_V0_REGNUM)];
(If there is always a register with number AARCH64_V0_REGNUM, that is)
Otherwise, it would be good to use an std::unique_ptr/gdb::unique_xmalloc_ptr to avoid
the manual xfree.
Same in aarch64_pseudo_write_2.
Simon
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 6/8] Aarch64 SVE pseudo register support
2018-05-31 13:26 ` Simon Marchi
@ 2018-06-04 13:29 ` Alan Hayward
0 siblings, 0 replies; 47+ messages in thread
From: Alan Hayward @ 2018-06-04 13:29 UTC (permalink / raw)
To: Simon Marchi, Pedro Alves; +Cc: gdb-patches, nd
Update of patch posted below.
> On 31 May 2018, at 15:57, Pedro Alves <palves@redhat.com> wrote:
>
> On 05/11/2018 11:52 AM, Alan Hayward wrote:
>
>> +/* Return the type for an AdvSISD V register. */
>> +
>> +static struct type *
>> +aarch64_vnv_type (struct gdbarch *gdbarch)
>> +{
>> + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>> +
>> + if (tdep->vnv_type == NULL)
>> + {
>> + struct type *t;
>> + struct type *elem;
>
> ELEM appears unused.
>
Leftover from copy paste of previous block of code. Removed.
> On 31 May 2018, at 13:21, Simon Marchi <simon.marchi@ericsson.com> wrote:
>
> On 2018-05-11 06:52 AM, Alan Hayward wrote:
>> Add the functionality for reading/writing psuedo registers.
>
> “pseudo"
Done.
>>
>> static struct value *
>> -aarch64_pseudo_read_value_2 (readable_regcache *regcache, int regnum_offset,
>> +aarch64_pseudo_read_value_2 (struct gdbarch *gdbarch,
>> + readable_regcache *regcache, int regnum_offset,
>> int regsize, struct value *result_value)
>> {
>> - gdb_byte reg_buf[V_REGISTER_SIZE];
>> + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>> + gdb_byte v_buf[V_REGISTER_SIZE], *reg_buf;
>> + gdb_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);
>> unsigned v_regnum = AARCH64_V0_REGNUM + regnum_offset;
>>
>> + /* Enough space to read a full vector register. */
>> + if (tdep->has_sve ())
>> + reg_buf = (gdb_byte *) xmalloc (register_size (gdbarch, AARCH64_V0_REGNUM));
>> + else
>> + reg_buf = v_buf;
>
> If the size of a register (even with SVE) will always be reasonable to allocate on the
> stack, maybe you could just use
>
> gdb_byte reg_buf[register_size (gdbarch, AARCH64_V0_REGNUM)];
>
> (If there is always a register with number AARCH64_V0_REGNUM, that is)
>
> Otherwise, it would be good to use an std::unique_ptr/gdb::unique_xmalloc_ptr to avoid
> the manual xfree.
>
> Same in aarch64_pseudo_write_2.
>
Thinking about this a little more, on an SVE system, a compiler
spill of a SVE register is going to result in it going to the
stack anyway.
Max size of an SVE register we support is 256 bytes, which
doesn't seem unreasonable to add to the stack.
Updated to use an array. Are you happy with the new version?
Thanks,
Alan.
diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
index b6b9b30e715291fb0512f214a88ce49445fb290b..76243b7de53483bf8394adc1f7c493798da25b06 100644
--- a/gdb/aarch64-tdep.h
+++ b/gdb/aarch64-tdep.h
@@ -70,6 +70,7 @@ struct gdbarch_tdep
struct type *vns_type;
struct type *vnh_type;
struct type *vnb_type;
+ struct type *vnv_type;
/* syscall record. */
int (*aarch64_syscall_record) (struct regcache *regcache, unsigned long svc_number);
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index b752338178f28a1169234a76941a1546bbc9dfa7..f04729f402ec56416cd772e5ca94f0681ea85beb 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -69,6 +69,7 @@
#define AARCH64_S0_REGNUM (AARCH64_D0_REGNUM + 32)
#define AARCH64_H0_REGNUM (AARCH64_S0_REGNUM + 32)
#define AARCH64_B0_REGNUM (AARCH64_H0_REGNUM + 32)
+#define AARCH64_SVE_V0_REGNUM (AARCH64_B0_REGNUM + 32)
/* All possible aarch64 target descriptors. */
struct target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1];
@@ -1766,6 +1767,30 @@ aarch64_vnb_type (struct gdbarch *gdbarch)
return tdep->vnb_type;
}
+/* Return the type for an AdvSISD V register. */
+
+static struct type *
+aarch64_vnv_type (struct gdbarch *gdbarch)
+{
+ struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
+ if (tdep->vnv_type == NULL)
+ {
+ struct type *t = arch_composite_type (gdbarch, "__gdb_builtin_type_vnv",
+ TYPE_CODE_UNION);
+
+ append_composite_type_field (t, "d", aarch64_vnd_type (gdbarch));
+ append_composite_type_field (t, "s", aarch64_vns_type (gdbarch));
+ append_composite_type_field (t, "h", aarch64_vnh_type (gdbarch));
+ append_composite_type_field (t, "b", aarch64_vnb_type (gdbarch));
+ append_composite_type_field (t, "q", aarch64_vnq_type (gdbarch));
+
+ tdep->vnv_type = t;
+ }
+
+ return tdep->vnv_type;
+}
+
/* Implement the "dwarf2_reg_to_regnum" gdbarch method. */
static int
@@ -2114,6 +2139,8 @@ aarch64_gen_return_address (struct gdbarch *gdbarch,
static const char *
aarch64_pseudo_register_name (struct gdbarch *gdbarch, int regnum)
{
+ struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
static const char *const q_name[] =
{
"q0", "q1", "q2", "q3",
@@ -2191,6 +2218,25 @@ aarch64_pseudo_register_name (struct gdbarch *gdbarch, int regnum)
if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
return b_name[regnum - AARCH64_B0_REGNUM];
+ if (tdep->has_sve ())
+ {
+ static const char *const sve_v_name[] =
+ {
+ "v0", "v1", "v2", "v3",
+ "v4", "v5", "v6", "v7",
+ "v8", "v9", "v10", "v11",
+ "v12", "v13", "v14", "v15",
+ "v16", "v17", "v18", "v19",
+ "v20", "v21", "v22", "v23",
+ "v24", "v25", "v26", "v27",
+ "v28", "v29", "v30", "v31",
+ };
+
+ if (regnum >= AARCH64_SVE_V0_REGNUM
+ && regnum < AARCH64_SVE_V0_REGNUM + AARCH64_V_REGS_NUM)
+ return sve_v_name[regnum - AARCH64_SVE_V0_REGNUM];
+ }
+
internal_error (__FILE__, __LINE__,
_("aarch64_pseudo_register_name: bad register number %d"),
regnum);
@@ -2201,6 +2247,8 @@ aarch64_pseudo_register_name (struct gdbarch *gdbarch, int regnum)
static struct type *
aarch64_pseudo_register_type (struct gdbarch *gdbarch, int regnum)
{
+ struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
regnum -= gdbarch_num_regs (gdbarch);
if (regnum >= AARCH64_Q0_REGNUM && regnum < AARCH64_Q0_REGNUM + 32)
@@ -2218,6 +2266,10 @@ aarch64_pseudo_register_type (struct gdbarch *gdbarch, int regnum)
if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
return aarch64_vnb_type (gdbarch);
+ if (tdep->has_sve () && regnum >= AARCH64_SVE_V0_REGNUM
+ && regnum < AARCH64_SVE_V0_REGNUM + 32)
+ return aarch64_vnv_type (gdbarch);
+
internal_error (__FILE__, __LINE__,
_("aarch64_pseudo_register_type: bad register number %d"),
regnum);
@@ -2229,6 +2281,8 @@ static int
aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
struct reggroup *group)
{
+ struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
regnum -= gdbarch_num_regs (gdbarch);
if (regnum >= AARCH64_Q0_REGNUM && regnum < AARCH64_Q0_REGNUM + 32)
@@ -2243,6 +2297,9 @@ aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
return group == all_reggroup || group == vector_reggroup;
else if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
return group == all_reggroup || group == vector_reggroup;
+ else if (tdep->has_sve () && regnum >= AARCH64_SVE_V0_REGNUM
+ && regnum < AARCH64_SVE_V0_REGNUM + 32)
+ return group == all_reggroup || group == vector_reggroup;
return group == all_reggroup;
}
@@ -2250,17 +2307,22 @@ aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
/* Helper for aarch64_pseudo_read_value. */
static struct value *
-aarch64_pseudo_read_value_1 (readable_regcache *regcache, int regnum_offset,
+aarch64_pseudo_read_value_1 (struct gdbarch *gdbarch,
+ readable_regcache *regcache, int regnum_offset,
int regsize, struct value *result_value)
{
- gdb_byte reg_buf[V_REGISTER_SIZE];
unsigned v_regnum = AARCH64_V0_REGNUM + regnum_offset;
+ /* Enough space for a full vector register. */
+ gdb_byte reg_buf[register_size (gdbarch, AARCH64_V0_REGNUM)];
+ gdb_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);
+
if (regcache->raw_read (v_regnum, reg_buf) != REG_VALID)
mark_value_bytes_unavailable (result_value, 0,
TYPE_LENGTH (value_type (result_value)));
else
memcpy (value_contents_raw (result_value), reg_buf, regsize);
+
return result_value;
}
@@ -2270,6 +2332,7 @@ static struct value *
aarch64_pseudo_read_value (struct gdbarch *gdbarch, readable_regcache *regcache,
int regnum)
{
+ struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
struct value *result_value = allocate_value (register_type (gdbarch, regnum));
VALUE_LVAL (result_value) = lval_register;
@@ -2278,42 +2341,56 @@ aarch64_pseudo_read_value (struct gdbarch *gdbarch, readable_regcache *regcache,
regnum -= gdbarch_num_regs (gdbarch);
if (regnum >= AARCH64_Q0_REGNUM && regnum < AARCH64_Q0_REGNUM + 32)
- return aarch64_pseudo_read_value_1 (regcache, regnum - AARCH64_Q0_REGNUM,
+ return aarch64_pseudo_read_value_1 (gdbarch, regcache,
+ regnum - AARCH64_Q0_REGNUM,
Q_REGISTER_SIZE, result_value);
if (regnum >= AARCH64_D0_REGNUM && regnum < AARCH64_D0_REGNUM + 32)
- return aarch64_pseudo_read_value_1 (regcache, regnum - AARCH64_D0_REGNUM,
+ return aarch64_pseudo_read_value_1 (gdbarch, regcache,
+ regnum - AARCH64_D0_REGNUM,
D_REGISTER_SIZE, result_value);
if (regnum >= AARCH64_S0_REGNUM && regnum < AARCH64_S0_REGNUM + 32)
- return aarch64_pseudo_read_value_1 (regcache, regnum - AARCH64_S0_REGNUM,
+ return aarch64_pseudo_read_value_1 (gdbarch, regcache,
+ regnum - AARCH64_S0_REGNUM,
S_REGISTER_SIZE, result_value);
if (regnum >= AARCH64_H0_REGNUM && regnum < AARCH64_H0_REGNUM + 32)
- return aarch64_pseudo_read_value_1 (regcache, regnum - AARCH64_H0_REGNUM,
+ return aarch64_pseudo_read_value_1 (gdbarch, regcache,
+ regnum - AARCH64_H0_REGNUM,
H_REGISTER_SIZE, result_value);
if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
- return aarch64_pseudo_read_value_1 (regcache, regnum - AARCH64_B0_REGNUM,
+ return aarch64_pseudo_read_value_1 (gdbarch, regcache,
+ regnum - AARCH64_B0_REGNUM,
B_REGISTER_SIZE, result_value);
+ if (tdep->has_sve () && regnum >= AARCH64_SVE_V0_REGNUM
+ && regnum < AARCH64_SVE_V0_REGNUM + 32)
+ return aarch64_pseudo_read_value_1 (gdbarch, regcache,
+ regnum - AARCH64_SVE_V0_REGNUM,
+ V_REGISTER_SIZE, result_value);
+
gdb_assert_not_reached ("regnum out of bound");
}
/* Helper for aarch64_pseudo_write. */
static void
-aarch64_pseudo_write_1 (struct regcache *regcache, int regnum_offset,
- int regsize, const gdb_byte *buf)
+aarch64_pseudo_write_1 (struct gdbarch *gdbarch, struct regcache *regcache,
+ int regnum_offset, int regsize, const gdb_byte *buf)
{
- gdb_byte reg_buf[V_REGISTER_SIZE];
unsigned v_regnum = AARCH64_V0_REGNUM + regnum_offset;
+ /* Enough space for a full vector register. */
+ gdb_byte reg_buf[register_size (gdbarch, AARCH64_V0_REGNUM)];
+ gdb_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);
+
/* Ensure the register buffer is zero, we want gdb writes of the
various 'scalar' pseudo registers to behavior like architectural
writes, register width bytes are written the remainder are set to
zero. */
- memset (reg_buf, 0, sizeof (reg_buf));
+ memset (reg_buf, 0, register_size (gdbarch, AARCH64_V0_REGNUM));
memcpy (reg_buf, buf, regsize);
regcache->raw_write (v_regnum, reg_buf);
@@ -2325,27 +2402,39 @@ static void
aarch64_pseudo_write (struct gdbarch *gdbarch, struct regcache *regcache,
int regnum, const gdb_byte *buf)
{
+ struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
regnum -= gdbarch_num_regs (gdbarch);
if (regnum >= AARCH64_Q0_REGNUM && regnum < AARCH64_Q0_REGNUM + 32)
- return aarch64_pseudo_write_1 (regcache, regnum - AARCH64_Q0_REGNUM,
- Q_REGISTER_SIZE, buf);
+ return aarch64_pseudo_write_1 (gdbarch, regcache,
+ regnum - AARCH64_Q0_REGNUM, Q_REGISTER_SIZE,
+ buf);
if (regnum >= AARCH64_D0_REGNUM && regnum < AARCH64_D0_REGNUM + 32)
- return aarch64_pseudo_write_1 (regcache, regnum - AARCH64_D0_REGNUM,
- D_REGISTER_SIZE, buf);
+ return aarch64_pseudo_write_1 (gdbarch, regcache,
+ regnum - AARCH64_D0_REGNUM, D_REGISTER_SIZE,
+ buf);
if (regnum >= AARCH64_S0_REGNUM && regnum < AARCH64_S0_REGNUM + 32)
- return aarch64_pseudo_write_1 (regcache, regnum - AARCH64_S0_REGNUM,
- S_REGISTER_SIZE, buf);
+ return aarch64_pseudo_write_1 (gdbarch, regcache,
+ regnum - AARCH64_S0_REGNUM, S_REGISTER_SIZE,
+ buf);
if (regnum >= AARCH64_H0_REGNUM && regnum < AARCH64_H0_REGNUM + 32)
- return aarch64_pseudo_write_1 (regcache, regnum - AARCH64_H0_REGNUM,
- H_REGISTER_SIZE, buf);
+ return aarch64_pseudo_write_1 (gdbarch, regcache,
+ regnum - AARCH64_H0_REGNUM, H_REGISTER_SIZE,
+ buf);
if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
- return aarch64_pseudo_write_1 (regcache, regnum - AARCH64_B0_REGNUM,
- B_REGISTER_SIZE, buf);
+ return aarch64_pseudo_write_1 (gdbarch, regcache,
+ regnum - AARCH64_B0_REGNUM, B_REGISTER_SIZE,
+ buf);
+
+ if (tdep->has_sve () && regnum >= AARCH64_SVE_V0_REGNUM
+ && regnum < AARCH64_SVE_V0_REGNUM + 32)
+ return aarch64_pseudo_write_1 (gdbarch, regcache,
+ regnum - AARCH64_SVE_V0_REGNUM,
+ V_REGISTER_SIZE, buf);
gdb_assert_not_reached ("regnum out of bound");
}
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 6/8] Aarch64 SVE pseudo register support
2018-05-11 10:53 ` [PATCH 6/8] Aarch64 SVE pseudo register support Alan Hayward
2018-05-31 13:26 ` Simon Marchi
@ 2018-05-31 14:59 ` Pedro Alves
1 sibling, 0 replies; 47+ messages in thread
From: Pedro Alves @ 2018-05-31 14:59 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
On 05/11/2018 11:52 AM, Alan Hayward wrote:
> +/* Return the type for an AdvSISD V register. */
> +
> +static struct type *
> +aarch64_vnv_type (struct gdbarch *gdbarch)
> +{
> + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +
> + if (tdep->vnv_type == NULL)
> + {
> + struct type *t;
> + struct type *elem;
ELEM appears unused.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 8/8] Ptrace support for Aarch64 SVE
2018-05-11 10:53 [PATCH 0/8] Add SVE support for Aarch64 GDB Alan Hayward
2018-05-11 10:53 ` [PATCH 1/8] Add Aarch64 SVE target description Alan Hayward
2018-05-11 10:53 ` [PATCH 6/8] Aarch64 SVE pseudo register support Alan Hayward
@ 2018-05-11 10:53 ` Alan Hayward
2018-05-31 13:40 ` Simon Marchi
2018-05-31 20:17 ` Simon Marchi
2018-05-11 10:53 ` [PATCH 7/8] Add methods to gdbserver regcache and raw_compare Alan Hayward
` (5 subsequent siblings)
8 siblings, 2 replies; 47+ messages in thread
From: Alan Hayward @ 2018-05-11 10:53 UTC (permalink / raw)
To: gdb-patches; +Cc: nd, Alan Hayward
Add support for reading and writing registers for Aarch64 SVE.
I've made this functionality common as it will be required for
gdbserver when gdbsever sve support is added.
Given that gdbserver does not yet call this function, I am
happy to remove the regcache commonise functions from the
previous patch. However, this would result in code in nat/
that does not compile for gdbserver. I wanted to avoid that.
We need to support the cases where the kernel only gives us a
fpsimd structure. This occurs when there is no active SVE state
in the kernel (for example, after starting a new process).
As per the covering email description, I've included large chunks
of linux kernel headers within an ifdef. Formatting of these macros
remains identical to the Kernel versions (ie not adapted to GNU style).
Added checks to make sure the vector length has not changed whilst
the process is running.
2018-05-11 Alan Hayward <alan.hayward@arm.com>
gdb/
* aarch64-linux-nat.c (fetch_sveregs_from_thread): New function.
(store_sveregs_to_thread): Likewise.
(aarch64_linux_fetch_inferior_registers): Check for SVE.
(aarch64_linux_store_inferior_registers): Likewise.
* nat/aarch64-sve-linux-ptrace.c (aarch64_sve_get_sveregs): New
function.
(aarch64_sve_regs_copy_to_regcache): Likewise.
(aarch64_sve_regs_copy_from_regcache): Likewise.
* nat/aarch64-sve-linux-ptrace.h (aarch64_sve_get_sveregs): New
declaration.
(aarch64_sve_regs_copy_to_regcache): Likewise.
(aarch64_sve_regs_copy_from_regcache): Likewise.
(sve_context): Structure from Linux headers.
(SVE_SIG_ZREGS_SIZE): Define from Linux headers.
(SVE_SIG_ZREG_SIZE): Likewise.
(SVE_SIG_PREG_SIZE): Likewise.
(SVE_SIG_FFR_SIZE): Likewise.
(SVE_SIG_REGS_OFFSET): Likewise.
(SVE_SIG_ZREGS_OFFSET): Likewise.
(SVE_SIG_ZREG_OFFSET): Likewise.
(SVE_SIG_ZREGS_SIZE): Likewise.
(SVE_SIG_PREGS_OFFSET): Likewise.
(SVE_SIG_PREG_OFFSET): Likewise.
(SVE_SIG_PREGS_SIZE): Likewise.
(SVE_SIG_FFR_OFFSET): Likewise.
(SVE_SIG_REGS_SIZE): Likewise.
(SVE_SIG_CONTEXT_SIZE): Likewise.
(SVE_PT_REGS_MASK): Likewise.
(SVE_PT_REGS_FPSIMD): Likewise.
(SVE_PT_REGS_SVE): Likewise.
(SVE_PT_VL_INHERIT): Likewise.
(SVE_PT_VL_ONEXEC): Likewise.
(SVE_PT_REGS_OFFSET): Likewise.
(SVE_PT_FPSIMD_OFFSET): Likewise.
(SVE_PT_FPSIMD_SIZE): Likewise.
(SVE_PT_SVE_ZREG_SIZE): Likewise.
(SVE_PT_SVE_PREG_SIZE): Likewise.
(SVE_PT_SVE_FFR_SIZE): Likewise.
(SVE_PT_SVE_FPSR_SIZE): Likewise.
(SVE_PT_SVE_FPCR_SIZE): Likewise.
(__SVE_SIG_TO_PT): Likewise.
(SVE_PT_SVE_OFFSET): Likewise.
(SVE_PT_SVE_ZREGS_OFFSET): Likewise.
(SVE_PT_SVE_ZREG_OFFSET): Likewise.
(SVE_PT_SVE_ZREGS_SIZE): Likewise.
(SVE_PT_SVE_PREGS_OFFSET): Likewise.
(SVE_PT_SVE_PREG_OFFSET): Likewise.
(SVE_PT_SVE_PREGS_SIZE): Likewise.
(SVE_PT_SVE_FFR_OFFSET): Likewise.
(SVE_PT_SVE_FPSR_OFFSET): Likewise.
(SVE_PT_SVE_FPCR_OFFSET): Likewise.
(SVE_PT_SVE_SIZE): Likewise.
(SVE_PT_SIZE): Likewise.
(HAS_SVE_STATE): New define.
gdbserver
* Makefile.in: Add aarch64-sve-linux-ptrace.c.
---
gdb/aarch64-linux-nat.c | 57 +++++++-
gdb/gdbserver/Makefile.in | 1 +
gdb/nat/aarch64-sve-linux-ptrace.c | 263 +++++++++++++++++++++++++++++++++++++
gdb/nat/aarch64-sve-linux-ptrace.h | 120 +++++++++++++++++
4 files changed, 439 insertions(+), 2 deletions(-)
diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 1270b7e0c0..bc570ade6d 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -388,19 +388,65 @@ store_fpregs_to_thread (const struct regcache *regcache)
}
}
+/* Fill GDB's register array with the sve register values
+ from the current thread. */
+
+static void
+fetch_sveregs_from_thread (struct regcache *regcache)
+{
+ gdb_byte *base = aarch64_sve_get_sveregs (ptid_get_lwp (inferior_ptid));
+ aarch64_sve_regs_copy_to_regcache (regcache, base);
+ xfree (base);
+}
+
+/* Store to the current thread the valid sve register
+ values in the GDB's register array. */
+
+static void
+store_sveregs_to_thread (struct regcache *regcache)
+{
+ gdb_byte *base;
+ int ret, tid;
+ struct iovec iovec;
+
+ tid = ptid_get_lwp (inferior_ptid);
+
+ /* Obtain a dump of SVE registers from ptrace. */
+ base = aarch64_sve_get_sveregs (tid);
+
+ /* Overwrite with regcache state. */
+ aarch64_sve_regs_copy_from_regcache (regcache, base);
+
+ /* Write back to the kernel. */
+ iovec.iov_base = base;
+ iovec.iov_len = ((struct user_sve_header *) base)->size;
+ ret = ptrace (PTRACE_SETREGSET, tid, NT_ARM_SVE, &iovec);
+ xfree (base);
+
+ if (ret < 0)
+ perror_with_name (_("Unable to store sve registers"));
+}
+
/* Implement the "fetch_registers" target_ops method. */
void
aarch64_linux_nat_target::fetch_registers (struct regcache *regcache,
int regno)
{
+ struct gdbarch_tdep *tdep = gdbarch_tdep (regcache->arch ());
+
if (regno == -1)
{
fetch_gregs_from_thread (regcache);
- fetch_fpregs_from_thread (regcache);
+ if (tdep->has_sve ())
+ fetch_sveregs_from_thread (regcache);
+ else
+ fetch_fpregs_from_thread (regcache);
}
else if (regno < AARCH64_V0_REGNUM)
fetch_gregs_from_thread (regcache);
+ else if (tdep->has_sve ())
+ fetch_sveregs_from_thread (regcache);
else
fetch_fpregs_from_thread (regcache);
}
@@ -411,13 +457,20 @@ void
aarch64_linux_nat_target::store_registers (struct regcache *regcache,
int regno)
{
+ struct gdbarch_tdep *tdep = gdbarch_tdep (regcache->arch ());
+
if (regno == -1)
{
store_gregs_to_thread (regcache);
- store_fpregs_to_thread (regcache);
+ if (tdep->has_sve ())
+ store_sveregs_to_thread (regcache);
+ else
+ store_fpregs_to_thread (regcache);
}
else if (regno < AARCH64_V0_REGNUM)
store_gregs_to_thread (regcache);
+ else if (tdep->has_sve ())
+ store_sveregs_to_thread (regcache);
else
store_fpregs_to_thread (regcache);
}
diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index c377378809..22ff19a2d2 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -218,6 +218,7 @@ SFILES = \
$(srcdir)/common/tdesc.c \
$(srcdir)/common/vec.c \
$(srcdir)/common/xml-utils.c \
+ $(srcdir)/nat/aarch64-sve-linux-ptrace.c \
$(srcdir)/nat/linux-btrace.c \
$(srcdir)/nat/linux-namespaces.c \
$(srcdir)/nat/linux-osdata.c \
diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c
index 9381786fda..84c7a41f40 100644
--- a/gdb/nat/aarch64-sve-linux-ptrace.c
+++ b/gdb/nat/aarch64-sve-linux-ptrace.c
@@ -25,6 +25,13 @@
#include "aarch64-sve-linux-ptrace.h"
#include "arch/aarch64.h"
+#ifndef GDBSERVER
+#include "defs.h"
+#endif
+#include "regcache.h"
+
+static bool vq_change_warned = false;
+
/* Read VQ for the given tid using ptrace. If SVE is not supported then zero
is returned (on a system that supports SVE, then VQ cannot be zeo). */
@@ -50,3 +57,259 @@ aarch64_sve_get_vq (int tid)
return vq;
}
+
+/* Read the current SVE register set using ptrace, allocating space as
+ required. */
+
+gdb_byte *
+aarch64_sve_get_sveregs (int tid)
+{
+ int ret;
+ struct iovec iovec;
+ struct user_sve_header header;
+ long vq = aarch64_sve_get_vq (tid);
+
+ if (vq == 0)
+ perror_with_name (_("Unable to fetch sve register header"));
+
+ /* A ptrace call with NT_ARM_SVE will return a header followed by either a
+ dump of all the SVE and FP registers, or an fpsimd structure (identical to
+ the one returned by NT_FPREGSET) if the kernel has not yet executed any
+ SVE code. Make sure we allocate enough space for a full SVE dump. */
+
+ iovec.iov_len = SVE_PT_SIZE (vq, SVE_PT_REGS_SVE);
+ iovec.iov_base = xmalloc (iovec.iov_len);
+
+ ret = ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec);
+ if (ret < 0)
+ perror_with_name (_("Unable to fetch sve registers"));
+
+ return (gdb_byte *) iovec.iov_base;
+}
+
+/* Put the registers from linux structure buf into regcache. */
+
+void
+aarch64_sve_regs_copy_to_regcache (struct regcache *regcache, const void *buf)
+{
+ char *base = (char*) buf;
+ int i;
+ struct user_sve_header *header = (struct user_sve_header *) buf;
+ long vq, vg_regcache;
+
+ vq = sve_vq_from_vl (header->vl);
+
+ /* Sanity check the data in the header. */
+ gdb_assert (sve_vl_valid (header->vl));
+ gdb_assert (SVE_PT_SIZE (vq, header->flags) == header->size);
+
+ regcache->raw_collect (AARCH64_SVE_VG_REGNUM, &vg_regcache);
+ if (vg_regcache == 0)
+ {
+ /* VG has not been set. */
+ vg_regcache = sve_vg_from_vl (header->vl);
+ regcache->raw_supply (AARCH64_SVE_VG_REGNUM, &vg_regcache);
+ }
+ else if (vg_regcache != sve_vg_from_vl (header->vl) && !vq_change_warned)
+ {
+ /* Vector length on the running process has changed. GDB currently does
+ not support this and will result in GDB showing incorrect partially
+ incorrect data for the vector registers. Warn once and continue. We
+ do not expect many programs to exhibit this behaviour. To fix this
+ we need to spot the change earlier and generate a new target
+ descriptor. */
+ warning (_("Vector length has changed (%ld to %d). "
+ "Vector registers may show incorrect data."),
+ vg_regcache, sve_vg_from_vl (header->vl));
+ vq_change_warned = true;
+ }
+
+ if (HAS_SVE_STATE (*header))
+ {
+ /* The register dump contains a set of SVE registers. */
+
+ for (i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++)
+ regcache->raw_supply (AARCH64_SVE_Z0_REGNUM + i,
+ base + SVE_PT_SVE_ZREG_OFFSET (vq, i));
+
+ for (i = 0; i < AARCH64_SVE_P_REGS_NUM; i++)
+ regcache->raw_supply (AARCH64_SVE_P0_REGNUM + i,
+ base + SVE_PT_SVE_PREG_OFFSET (vq, i));
+
+ regcache->raw_supply (AARCH64_SVE_FFR_REGNUM,
+ base + SVE_PT_SVE_FFR_OFFSET (vq));
+ regcache->raw_supply (AARCH64_FPSR_REGNUM,
+ base + SVE_PT_SVE_FPSR_OFFSET (vq));
+ regcache->raw_supply (AARCH64_FPCR_REGNUM,
+ base + SVE_PT_SVE_FPCR_OFFSET (vq));
+ }
+ else
+ {
+ /* There is no SVE state yet - the register dump contains a fpsimd
+ structure instead. These registers still exist in the hardware, but
+ the kernel has not yet initialised them, and so they will be null. */
+
+ char *zero_reg = (char *) alloca (SVE_PT_SVE_ZREG_SIZE (vq));
+ struct user_fpsimd_state *fpsimd
+ = (struct user_fpsimd_state *)(base + SVE_PT_FPSIMD_OFFSET);
+
+ /* Copy across the V registers from fpsimd structure to the Z registers,
+ ensuring the non overlapping state is set to null. */
+
+ memset (zero_reg, 0, SVE_PT_SVE_ZREG_SIZE (vq));
+
+ for (i = 0; i <= AARCH64_SVE_Z_REGS_NUM; i++)
+ {
+ memcpy (zero_reg, &fpsimd->vregs[i], sizeof (__int128_t));
+ regcache->raw_supply (AARCH64_SVE_Z0_REGNUM + i, zero_reg);
+ }
+
+ regcache->raw_supply (AARCH64_FPSR_REGNUM, &fpsimd->fpsr);
+ regcache->raw_supply (AARCH64_FPCR_REGNUM, &fpsimd->fpcr);
+
+ /* Clear the SVE only registers. */
+
+ for (i = 0; i <= AARCH64_SVE_P_REGS_NUM; i++)
+ regcache->raw_supply (AARCH64_SVE_P0_REGNUM + i, zero_reg);
+
+ regcache->raw_supply (AARCH64_SVE_FFR_REGNUM, zero_reg);
+ }
+}
+
+
+/* Put the registers from regcache into linux structure buf. */
+
+void
+aarch64_sve_regs_copy_from_regcache (struct regcache *regcache, void *buf)
+{
+ struct user_sve_header *header = (struct user_sve_header *) buf;
+ char *base = (char*) buf;
+ long vq, vg_regcache;
+ int i;
+
+ vq = sve_vq_from_vl (header->vl);
+
+ /* Sanity check the data in the header. */
+ gdb_assert (sve_vl_valid (header->vl));
+ gdb_assert (SVE_PT_SIZE (vq, header->flags) == header->size);
+
+ regcache->raw_collect (AARCH64_SVE_VG_REGNUM, &vg_regcache);
+ if (vg_regcache != 0 && vg_regcache != sve_vg_from_vl (header->vl))
+ /* Vector length on the running process has changed. GDB currently does
+ not support this and will result in GDB writing invalid data back to the
+ vector registers. Error and exit. We do not expect many programs to
+ exhibit this behaviour. To fix this we need to spot the change earlier
+ and generate a new target descriptor. */
+ error (_("Vector length has changed. Cannot write back registers."));
+
+ if (!HAS_SVE_STATE (*header))
+ {
+ /* There is no SVE state yet - the register dump contains a fpsimd
+ structure instead. Where possible we want to write the regcache data
+ back to the kernel using the fpsimd structure. However, if we cannot
+ then we'll need to reformat the fpsimd into a full SVE structure,
+ resulting in the initialization of SVE state written back to the
+ kernel, which is why we try to avoid it. */
+
+ int has_sve_state = 0;
+ char *zero_reg = (char *) alloca (SVE_PT_SVE_ZREG_SIZE (vq));
+ struct user_fpsimd_state *fpsimd
+ = (struct user_fpsimd_state *)(base + SVE_PT_FPSIMD_OFFSET);
+
+ memset (zero_reg, 0, SVE_PT_SVE_ZREG_SIZE (vq));
+
+ /* Check in the regcache if any of the Z registers are set after the
+ first 128 bits, or if any of the other SVE registers are set. */
+
+ for (i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++)
+ {
+ has_sve_state |= regcache->raw_compare (AARCH64_SVE_Z0_REGNUM + i,
+ zero_reg, sizeof (__int128_t));
+ if (has_sve_state != 0)
+ break;
+ }
+
+ if (has_sve_state == 0)
+ for (i = 0; i < AARCH64_SVE_P_REGS_NUM; i++)
+ {
+ has_sve_state |= regcache->raw_compare (AARCH64_SVE_P0_REGNUM + i,
+ zero_reg, 0);
+ if (has_sve_state != 0)
+ break;
+ }
+
+ if (has_sve_state == 0)
+ has_sve_state |= regcache->raw_compare (AARCH64_SVE_FFR_REGNUM,
+ zero_reg, 0);
+
+ /* If no SVE state exists, then use the existing fpsimd structure to
+ write out state and return. */
+
+ if (has_sve_state == 0)
+ {
+ /* The collects of the Z registers will overflow the size of a vreg.
+ There is enough space in the structure to allow for this, but we
+ cannot overflow into the next register as we might not be
+ collecting every register. */
+
+ for (i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++)
+ {
+ if (REG_VALID == regcache->get_register_status (
+ AARCH64_SVE_Z0_REGNUM + i))
+ {
+ regcache->raw_collect (AARCH64_SVE_Z0_REGNUM + i, zero_reg);
+ memcpy (&fpsimd->vregs[i], zero_reg, sizeof (__int128_t));
+ }
+ }
+
+ if (REG_VALID == regcache->get_register_status (AARCH64_FPSR_REGNUM))
+ regcache->raw_collect (AARCH64_FPSR_REGNUM, &fpsimd->fpsr);
+ if (REG_VALID == regcache->get_register_status (AARCH64_FPCR_REGNUM))
+ regcache->raw_collect ( AARCH64_FPCR_REGNUM, &fpsimd->fpcr);
+
+ return;
+ }
+
+ /* Otherwise, reformat the fpsimd structure into a full SVE set, by
+ expanding the V registers (working backwards so we don't splat
+ registers before they are copied) and using null for everything else.
+ Note that enough space for a full SVE dump was originally allocated
+ for base. */
+
+ header->flags |= SVE_PT_REGS_SVE;
+ header->size = SVE_PT_SIZE (vq, SVE_PT_REGS_SVE);
+
+ memcpy (base + SVE_PT_SVE_FPSR_OFFSET (vq), &fpsimd->fpsr,
+ sizeof (uint32_t));
+ memcpy (base + SVE_PT_SVE_FPCR_OFFSET (vq), &fpsimd->fpcr,
+ sizeof (uint32_t));
+
+ for (i = AARCH64_SVE_Z_REGS_NUM; i >= 0 ; i--)
+ {
+ memcpy (base + SVE_PT_SVE_ZREG_OFFSET (vq, i), &fpsimd->vregs[i],
+ sizeof (__int128_t));
+ }
+ }
+
+ /* Replace the kernel values with those from regcache. */
+
+ for (i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++)
+ if (REG_VALID == regcache->get_register_status (AARCH64_SVE_Z0_REGNUM + i))
+ regcache->raw_collect (AARCH64_SVE_Z0_REGNUM + i,
+ base + SVE_PT_SVE_ZREG_OFFSET (vq, i));
+
+ for (i = 0; i < AARCH64_SVE_P_REGS_NUM; i++)
+ if (REG_VALID == regcache->get_register_status (AARCH64_SVE_P0_REGNUM + i))
+ regcache->raw_collect (AARCH64_SVE_P0_REGNUM + i,
+ base + SVE_PT_SVE_PREG_OFFSET (vq, i));
+
+ if (REG_VALID == regcache->get_register_status (AARCH64_SVE_FFR_REGNUM))
+ regcache->raw_collect (AARCH64_SVE_FFR_REGNUM,
+ base + SVE_PT_SVE_FFR_OFFSET (vq));
+ if (REG_VALID == regcache->get_register_status (AARCH64_FPSR_REGNUM))
+ regcache->raw_collect (AARCH64_FPSR_REGNUM,
+ base + SVE_PT_SVE_FPSR_OFFSET (vq));
+ if (REG_VALID == regcache->get_register_status (AARCH64_FPCR_REGNUM))
+ regcache->raw_collect (AARCH64_FPCR_REGNUM,
+ base + SVE_PT_SVE_FPCR_OFFSET (vq));
+}
diff --git a/gdb/nat/aarch64-sve-linux-ptrace.h b/gdb/nat/aarch64-sve-linux-ptrace.h
index b318150ac1..eff90ea2ea 100644
--- a/gdb/nat/aarch64-sve-linux-ptrace.h
+++ b/gdb/nat/aarch64-sve-linux-ptrace.h
@@ -34,10 +34,31 @@
extern unsigned long aarch64_sve_get_vq (int tid);
+/* Read the current SVE register set using ptrace, allocating space as
+ required. */
+
+extern gdb_byte *aarch64_sve_get_sveregs (int tid);
+
+/* Put the registers from linux structure buf into regcache. */
+
+extern void aarch64_sve_regs_copy_to_regcache (struct regcache *regcache,
+ const void *buf);
+
+/* Put the registers from regcache into linux structure buf. */
+
+extern void aarch64_sve_regs_copy_from_regcache (struct regcache *regcache,
+ void *buf);
+
/* Structures and defines taken from sigcontext.h. */
#ifndef SVE_SIG_ZREGS_SIZE
+struct sve_context {
+ struct _aarch64_ctx head;
+ __u16 vl;
+ __u16 __reserved[3];
+};
+
#define SVE_VQ_BYTES 16 /* number of bytes per quadword */
#define SVE_VQ_MIN 1
@@ -52,6 +73,35 @@ extern unsigned long aarch64_sve_get_vq (int tid);
#define sve_vl_valid(vl) \
((vl) % SVE_VQ_BYTES == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
+#define SVE_SIG_ZREG_SIZE(vq) ((__u32)(vq) * SVE_VQ_BYTES)
+#define SVE_SIG_PREG_SIZE(vq) ((__u32)(vq) * (SVE_VQ_BYTES / 8))
+#define SVE_SIG_FFR_SIZE(vq) SVE_SIG_PREG_SIZE(vq)
+
+#define SVE_SIG_REGS_OFFSET \
+ ((sizeof(struct sve_context) + (SVE_VQ_BYTES - 1)) \
+ / SVE_VQ_BYTES * SVE_VQ_BYTES)
+
+#define SVE_SIG_ZREGS_OFFSET SVE_SIG_REGS_OFFSET
+#define SVE_SIG_ZREG_OFFSET(vq, n) \
+ (SVE_SIG_ZREGS_OFFSET + SVE_SIG_ZREG_SIZE(vq) * (n))
+#define SVE_SIG_ZREGS_SIZE(vq) \
+ (SVE_SIG_ZREG_OFFSET(vq, SVE_NUM_ZREGS) - SVE_SIG_ZREGS_OFFSET)
+
+#define SVE_SIG_PREGS_OFFSET(vq) \
+ (SVE_SIG_ZREGS_OFFSET + SVE_SIG_ZREGS_SIZE(vq))
+#define SVE_SIG_PREG_OFFSET(vq, n) \
+ (SVE_SIG_PREGS_OFFSET(vq) + SVE_SIG_PREG_SIZE(vq) * (n))
+#define SVE_SIG_PREGS_SIZE(vq) \
+ (SVE_SIG_PREG_OFFSET(vq, SVE_NUM_PREGS) - SVE_SIG_PREGS_OFFSET(vq))
+
+#define SVE_SIG_FFR_OFFSET(vq) \
+ (SVE_SIG_PREGS_OFFSET(vq) + SVE_SIG_PREGS_SIZE(vq))
+
+#define SVE_SIG_REGS_SIZE(vq) \
+ (SVE_SIG_FFR_OFFSET(vq) + SVE_SIG_FFR_SIZE(vq) - SVE_SIG_REGS_OFFSET)
+
+#define SVE_SIG_CONTEXT_SIZE(vq) (SVE_SIG_REGS_OFFSET + SVE_SIG_REGS_SIZE(vq))
+
#endif /* SVE_SIG_ZREGS_SIZE. */
@@ -68,6 +118,76 @@ struct user_sve_header {
__u16 __reserved;
};
+
+#define SVE_PT_REGS_MASK 1
+
+#define SVE_PT_REGS_FPSIMD 0
+#define SVE_PT_REGS_SVE SVE_PT_REGS_MASK
+
+#define SVE_PT_VL_INHERIT (PR_SVE_VL_INHERIT >> 16)
+#define SVE_PT_VL_ONEXEC (PR_SVE_SET_VL_ONEXEC >> 16)
+
+#define SVE_PT_REGS_OFFSET \
+ ((sizeof(struct sve_context) + (SVE_VQ_BYTES - 1)) \
+ / SVE_VQ_BYTES * SVE_VQ_BYTES)
+
+#define SVE_PT_FPSIMD_OFFSET SVE_PT_REGS_OFFSET
+
+#define SVE_PT_FPSIMD_SIZE(vq, flags) (sizeof(struct user_fpsimd_state))
+
+#define SVE_PT_SVE_ZREG_SIZE(vq) SVE_SIG_ZREG_SIZE(vq)
+#define SVE_PT_SVE_PREG_SIZE(vq) SVE_SIG_PREG_SIZE(vq)
+#define SVE_PT_SVE_FFR_SIZE(vq) SVE_SIG_FFR_SIZE(vq)
+#define SVE_PT_SVE_FPSR_SIZE sizeof(__u32)
+#define SVE_PT_SVE_FPCR_SIZE sizeof(__u32)
+
+#define __SVE_SIG_TO_PT(offset) \
+ ((offset) - SVE_SIG_REGS_OFFSET + SVE_PT_REGS_OFFSET)
+
+#define SVE_PT_SVE_OFFSET SVE_PT_REGS_OFFSET
+
+#define SVE_PT_SVE_ZREGS_OFFSET \
+ __SVE_SIG_TO_PT(SVE_SIG_ZREGS_OFFSET)
+#define SVE_PT_SVE_ZREG_OFFSET(vq, n) \
+ __SVE_SIG_TO_PT(SVE_SIG_ZREG_OFFSET(vq, n))
+#define SVE_PT_SVE_ZREGS_SIZE(vq) \
+ (SVE_PT_SVE_ZREG_OFFSET(vq, SVE_NUM_ZREGS) - SVE_PT_SVE_ZREGS_OFFSET)
+
+#define SVE_PT_SVE_PREGS_OFFSET(vq) \
+ __SVE_SIG_TO_PT(SVE_SIG_PREGS_OFFSET(vq))
+#define SVE_PT_SVE_PREG_OFFSET(vq, n) \
+ __SVE_SIG_TO_PT(SVE_SIG_PREG_OFFSET(vq, n))
+#define SVE_PT_SVE_PREGS_SIZE(vq) \
+ (SVE_PT_SVE_PREG_OFFSET(vq, SVE_NUM_PREGS) - \
+ SVE_PT_SVE_PREGS_OFFSET(vq))
+
+#define SVE_PT_SVE_FFR_OFFSET(vq) \
+ __SVE_SIG_TO_PT(SVE_SIG_FFR_OFFSET(vq))
+
+#define SVE_PT_SVE_FPSR_OFFSET(vq) \
+ ((SVE_PT_SVE_FFR_OFFSET(vq) + SVE_PT_SVE_FFR_SIZE(vq) + \
+ (SVE_VQ_BYTES - 1)) \
+ / SVE_VQ_BYTES * SVE_VQ_BYTES)
+#define SVE_PT_SVE_FPCR_OFFSET(vq) \
+ (SVE_PT_SVE_FPSR_OFFSET(vq) + SVE_PT_SVE_FPSR_SIZE)
+
+#define SVE_PT_SVE_SIZE(vq, flags) \
+ ((SVE_PT_SVE_FPCR_OFFSET(vq) + SVE_PT_SVE_FPCR_SIZE \
+ - SVE_PT_SVE_OFFSET + (SVE_VQ_BYTES - 1)) \
+ / SVE_VQ_BYTES * SVE_VQ_BYTES)
+
+#define SVE_PT_SIZE(vq, flags) \
+ (((flags) & SVE_PT_REGS_MASK) == SVE_PT_REGS_SVE ? \
+ SVE_PT_SVE_OFFSET + SVE_PT_SVE_SIZE(vq, flags) \
+ : SVE_PT_FPSIMD_OFFSET + SVE_PT_FPSIMD_SIZE(vq, flags))
+
#endif /* SVE_PT_SVE_ZREG_SIZE. */
+
+/* Indicates whether a SVE ptrace header is followed by SVE registers or a
+ fpsimd structure. */
+
+#define HAS_SVE_STATE(header) ((header).flags && SVE_PT_REGS_SVE)
+
+
#endif /* aarch64-sve-linux-ptrace.h */
--
2.15.1 (Apple Git-101)
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 8/8] Ptrace support for Aarch64 SVE
2018-05-11 10:53 ` [PATCH 8/8] Ptrace support for Aarch64 SVE Alan Hayward
@ 2018-05-31 13:40 ` Simon Marchi
2018-05-31 14:56 ` Alan Hayward
2018-05-31 20:17 ` Simon Marchi
1 sibling, 1 reply; 47+ messages in thread
From: Simon Marchi @ 2018-05-31 13:40 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
Hi Alan,
Since there is a good number of macros copied from the Linux kernel, it might be
a good idea to isolate in their own file. It would also be good to identify
precisely which file they come from (the path relative to the root of the linux
repo) and the git commit you used.
Also, since the copied code is rather large and non-trivial, does it pose copyright
problems? If we want to include it, maybe that separate file should not state that
the copyright is owned by the FSF, since it's not the case? Maybe others have
experience with this kind of things, or maybe we should get an advice from the FSF directly.
> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c
> index 9381786fda..84c7a41f40 100644
> --- a/gdb/nat/aarch64-sve-linux-ptrace.c
> +++ b/gdb/nat/aarch64-sve-linux-ptrace.c
> @@ -25,6 +25,13 @@
> #include "aarch64-sve-linux-ptrace.h"
> #include "arch/aarch64.h"
>
> +#ifndef GDBSERVER
> +#include "defs.h"
> +#endif
> +#include "regcache.h"
Hmm we try not add any more "#ifdef GDBSERVER" in the common code.
https://sourceware.org/gdb/wiki/Common#Header_files_in_common_code_.28defs.h_vs_server.h.2C_etc..29
Instead, we should try defining a common interface (probably in common/common-regcache.h?) that the
common code will use, and that regcaches from GDB and GDBserver will implement.
> +
> +static bool vq_change_warned = false;
> +
> /* Read VQ for the given tid using ptrace. If SVE is not supported then zero
> is returned (on a system that supports SVE, then VQ cannot be zeo). */
>
> @@ -50,3 +57,259 @@ aarch64_sve_get_vq (int tid)
>
> return vq;
> }
> +
> +/* Read the current SVE register set using ptrace, allocating space as
> + required. */
Put a reference to the .h here.
Since this returns allocated memory, could we return an RAII object? Either
std::vector, std::unique_ptr or gdb::unique_xmalloc_ptr.
> +
> +gdb_byte *
> +aarch64_sve_get_sveregs (int tid)
> +{
> + int ret;
> + struct iovec iovec;
> + struct user_sve_header header;
> + long vq = aarch64_sve_get_vq (tid);
> +
> + if (vq == 0)
> + perror_with_name (_("Unable to fetch sve register header"));
> +
> + /* A ptrace call with NT_ARM_SVE will return a header followed by either a
> + dump of all the SVE and FP registers, or an fpsimd structure (identical to
> + the one returned by NT_FPREGSET) if the kernel has not yet executed any
> + SVE code. Make sure we allocate enough space for a full SVE dump. */
> +
> + iovec.iov_len = SVE_PT_SIZE (vq, SVE_PT_REGS_SVE);
> + iovec.iov_base = xmalloc (iovec.iov_len);
> +
> + ret = ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec);
> + if (ret < 0)
> + perror_with_name (_("Unable to fetch sve registers"));
> +
> + return (gdb_byte *) iovec.iov_base;
> +}
> +
> +/* Put the registers from linux structure buf into regcache. */
> +
> +void
> +aarch64_sve_regs_copy_to_regcache (struct regcache *regcache, const void *buf)
> +{
> + char *base = (char*) buf;
> + int i;
> + struct user_sve_header *header = (struct user_sve_header *) buf;
> + long vq, vg_regcache;
> +
> + vq = sve_vq_from_vl (header->vl);
> +
> + /* Sanity check the data in the header. */
> + gdb_assert (sve_vl_valid (header->vl));
> + gdb_assert (SVE_PT_SIZE (vq, header->flags) == header->size);
Again, we shouldn't use gdb_assert here, since this validates external input.
> +
> + regcache->raw_collect (AARCH64_SVE_VG_REGNUM, &vg_regcache);
When fetching registers, won't it be usually to fill a shiny new, empty regcache?
In that case, won't it always fall into the "if" branch?
In any case, should we check the status of the VG register to make sure it's REG_VALID
before we try to collect it?
> + if (vg_regcache == 0)
> + {
> + /* VG has not been set. */
> + vg_regcache = sve_vg_from_vl (header->vl);
> + regcache->raw_supply (AARCH64_SVE_VG_REGNUM, &vg_regcache);
> + }
> + else if (vg_regcache != sve_vg_from_vl (header->vl) && !vq_change_warned)
> + {
> + /* Vector length on the running process has changed. GDB currently does
> + not support this and will result in GDB showing incorrect partially
> + incorrect data for the vector registers. Warn once and continue. We
> + do not expect many programs to exhibit this behaviour. To fix this
> + we need to spot the change earlier and generate a new target
> + descriptor. */
> + warning (_("Vector length has changed (%ld to %d). "
> + "Vector registers may show incorrect data."),
Perhaps mention "SVE vector length has changed..."? Otherwise the user may wonder
what vectors we are talking about.
> + vg_regcache, sve_vg_from_vl (header->vl));
> + vq_change_warned = true;
> + }
> +
> + if (HAS_SVE_STATE (*header))
> + {
> + /* The register dump contains a set of SVE registers. */
> +
> + for (i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++)
> + regcache->raw_supply (AARCH64_SVE_Z0_REGNUM + i,
> + base + SVE_PT_SVE_ZREG_OFFSET (vq, i));
> +
> + for (i = 0; i < AARCH64_SVE_P_REGS_NUM; i++)
> + regcache->raw_supply (AARCH64_SVE_P0_REGNUM + i,
> + base + SVE_PT_SVE_PREG_OFFSET (vq, i));
> +
> + regcache->raw_supply (AARCH64_SVE_FFR_REGNUM,
> + base + SVE_PT_SVE_FFR_OFFSET (vq));
> + regcache->raw_supply (AARCH64_FPSR_REGNUM,
> + base + SVE_PT_SVE_FPSR_OFFSET (vq));
> + regcache->raw_supply (AARCH64_FPCR_REGNUM,
> + base + SVE_PT_SVE_FPCR_OFFSET (vq));
Align the second line with the first argument. Here's an example, though
using spaces instead of tabs to make sure (hopefully) the mail clients display it
correctly.
regcache->raw_supply (AARCH64_SVE_Z0_REGNUM + i,
base + SVE_PT_SVE_ZREG_OFFSET (vq, i));
There are many instances throughout this file.
Simon
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 8/8] Ptrace support for Aarch64 SVE
2018-05-31 13:40 ` Simon Marchi
@ 2018-05-31 14:56 ` Alan Hayward
2018-06-01 15:17 ` Simon Marchi
0 siblings, 1 reply; 47+ messages in thread
From: Alan Hayward @ 2018-05-31 14:56 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches, nd
> On 31 May 2018, at 14:22, Simon Marchi <simon.marchi@ericsson.com> wrote:
>
> Hi Alan,
>
> Since there is a good number of macros copied from the Linux kernel, it might be
> a good idea to isolate in their own file. It would also be good to identify
> precisely which file they come from (the path relative to the root of the linux
> repo) and the git commit you used.
>
> Also, since the copied code is rather large and non-trivial, does it pose copyright
> problems? If we want to include it, maybe that separate file should not state that
> the copyright is owned by the FSF, since it's not the case? Maybe others have
> experience with this kind of things, or maybe we should get an advice from the FSF directly.
A new file makes sense (especially with regards to copyright). However, the plan was
to eventually remove the macros when "enough" time has passed - keeping everything
in aarch64-sve-linux-ptrace.h made it simpler than removing files.
Is there a minimum kernel version that gdb requires to link against in order to build?
The relevant defines are in 4.15. The kernel is now on 4.16.9.
I’m guessing this must have come up before when other targets added new kernel features?
I didn’t want to suddenly destroy everyone’s aarch64/targetall build trees without
warning.
>> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c
>> index 9381786fda..84c7a41f40 100644
>> --- a/gdb/nat/aarch64-sve-linux-ptrace.c
>> +++ b/gdb/nat/aarch64-sve-linux-ptrace.c
>> @@ -25,6 +25,13 @@
>> #include "aarch64-sve-linux-ptrace.h"
>> #include "arch/aarch64.h"
>>
>> +#ifndef GDBSERVER
>> +#include "defs.h"
>> +#endif
>> +#include "regcache.h"
>
> Hmm we try not add any more "#ifdef GDBSERVER" in the common code.
>
> https://sourceware.org/gdb/wiki/Common#Header_files_in_common_code_.28defs.h_vs_server.h.2C_etc..29
>
> Instead, we should try defining a common interface (probably in common/common-regcache.h?) that the
> common code will use, and that regcaches from GDB and GDBserver will implement.
I tried using common-defs.h, but gdb/regcache.h requires defines from
defs.h - RequireLongest and maybe others.
Putting defs.h at the top of gdb/regcache.h then broke in a weird way.
A lot of fiddling later, and I hadn’t found a way to make it work.
Creating common/common-regcache.h gets a bit odd because, the functions
I need for gdbserver (raw_supply, raw_collect and get_register_status)
on gdb come from:
class reg_buffer
...
enum register_status get_register_status (int regnum) const;
...
class readable_regcache : public reg_buffer
...
class detached_regcache : public readable_regcache
...
void raw_supply (int regnum, const void *buf);
...
class regcache : public detached_regcache
...
void raw_collect (int regnum, void *buf) const;
...
I don’t think that this would work:
class regcache : public detached_regcache, common_regcache
>
>> +
>> +static bool vq_change_warned = false;
>> +
>> /* Read VQ for the given tid using ptrace. If SVE is not supported then zero
>> is returned (on a system that supports SVE, then VQ cannot be zeo). */
>>
>> @@ -50,3 +57,259 @@ aarch64_sve_get_vq (int tid)
>>
>> return vq;
>> }
>> +
>> +/* Read the current SVE register set using ptrace, allocating space as
>> + required. */
>
> Put a reference to the .h here.
>
> Since this returns allocated memory, could we return an RAII object? Either
> std::vector, std::unique_ptr or gdb::unique_xmalloc_ptr.
Yes, I’ll update with one of those. Is there any documentation explaining
when to use which of these?
>
>> +
>> +gdb_byte *
>> +aarch64_sve_get_sveregs (int tid)
>> +{
>> + int ret;
>> + struct iovec iovec;
>> + struct user_sve_header header;
>> + long vq = aarch64_sve_get_vq (tid);
>> +
>> + if (vq == 0)
>> + perror_with_name (_("Unable to fetch sve register header"));
>> +
>> + /* A ptrace call with NT_ARM_SVE will return a header followed by either a
>> + dump of all the SVE and FP registers, or an fpsimd structure (identical to
>> + the one returned by NT_FPREGSET) if the kernel has not yet executed any
>> + SVE code. Make sure we allocate enough space for a full SVE dump. */
>> +
>> + iovec.iov_len = SVE_PT_SIZE (vq, SVE_PT_REGS_SVE);
>> + iovec.iov_base = xmalloc (iovec.iov_len);
>> +
>> + ret = ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec);
>> + if (ret < 0)
>> + perror_with_name (_("Unable to fetch sve registers"));
>> +
>> + return (gdb_byte *) iovec.iov_base;
>> +}
>> +
>> +/* Put the registers from linux structure buf into regcache. */
>> +
>> +void
>> +aarch64_sve_regs_copy_to_regcache (struct regcache *regcache, const void *buf)
>> +{
>> + char *base = (char*) buf;
>> + int i;
>> + struct user_sve_header *header = (struct user_sve_header *) buf;
>> + long vq, vg_regcache;
>> +
>> + vq = sve_vq_from_vl (header->vl);
>> +
>> + /* Sanity check the data in the header. */
>> + gdb_assert (sve_vl_valid (header->vl));
>> + gdb_assert (SVE_PT_SIZE (vq, header->flags) == header->size);
>
> Again, we shouldn't use gdb_assert here, since this validates external input.
Ok.
>
>> +
>> + regcache->raw_collect (AARCH64_SVE_VG_REGNUM, &vg_regcache);
>
> When fetching registers, won't it be usually to fill a shiny new, empty regcache?
> In that case, won't it always fall into the "if" branch?
>
> In any case, should we check the status of the VG register to make sure it's REG_VALID
> before we try to collect it?
I thought the same regcache was used each time registers needed reading?
I’ll double check this.
Either way, yes, should probably check REG_VALID
>
>> + if (vg_regcache == 0)
>> + {
>> + /* VG has not been set. */
>> + vg_regcache = sve_vg_from_vl (header->vl);
>> + regcache->raw_supply (AARCH64_SVE_VG_REGNUM, &vg_regcache);
>> + }
>> + else if (vg_regcache != sve_vg_from_vl (header->vl) && !vq_change_warned)
>> + {
>> + /* Vector length on the running process has changed. GDB currently does
>> + not support this and will result in GDB showing incorrect partially
>> + incorrect data for the vector registers. Warn once and continue. We
>> + do not expect many programs to exhibit this behaviour. To fix this
>> + we need to spot the change earlier and generate a new target
>> + descriptor. */
>> + warning (_("Vector length has changed (%ld to %d). "
>> + "Vector registers may show incorrect data."),
>
> Perhaps mention "SVE vector length has changed..."? Otherwise the user may wonder
> what vectors we are talking about.
Agreed.
>
>> + vg_regcache, sve_vg_from_vl (header->vl));
>> + vq_change_warned = true;
>> + }
>> +
>> + if (HAS_SVE_STATE (*header))
>> + {
>> + /* The register dump contains a set of SVE registers. */
>> +
>> + for (i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++)
>> + regcache->raw_supply (AARCH64_SVE_Z0_REGNUM + i,
>> + base + SVE_PT_SVE_ZREG_OFFSET (vq, i));
>> +
>> + for (i = 0; i < AARCH64_SVE_P_REGS_NUM; i++)
>> + regcache->raw_supply (AARCH64_SVE_P0_REGNUM + i,
>> + base + SVE_PT_SVE_PREG_OFFSET (vq, i));
>> +
>> + regcache->raw_supply (AARCH64_SVE_FFR_REGNUM,
>> + base + SVE_PT_SVE_FFR_OFFSET (vq));
>> + regcache->raw_supply (AARCH64_FPSR_REGNUM,
>> + base + SVE_PT_SVE_FPSR_OFFSET (vq));
>> + regcache->raw_supply (AARCH64_FPCR_REGNUM,
>> + base + SVE_PT_SVE_FPCR_OFFSET (vq));
>
> Align the second line with the first argument. Here's an example, though
> using spaces instead of tabs to make sure (hopefully) the mail clients display it
> correctly.
>
> regcache->raw_supply (AARCH64_SVE_Z0_REGNUM + i,
> base + SVE_PT_SVE_ZREG_OFFSET (vq, i));
>
> There are many instances throughout this file.
>
I think that’s leftover from when I changed from "raw_supply (regcache,”
to “regcache->raw_supply (“.
I’ll fix it up.
Alan.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 8/8] Ptrace support for Aarch64 SVE
2018-05-31 14:56 ` Alan Hayward
@ 2018-06-01 15:17 ` Simon Marchi
2018-06-04 15:49 ` Alan Hayward
0 siblings, 1 reply; 47+ messages in thread
From: Simon Marchi @ 2018-06-01 15:17 UTC (permalink / raw)
To: Alan Hayward; +Cc: gdb-patches, nd
On 2018-05-31 10:46 AM, Alan Hayward wrote:
>
>
>> On 31 May 2018, at 14:22, Simon Marchi <simon.marchi@ericsson.com> wrote:
>>
>> Hi Alan,
>>
>> Since there is a good number of macros copied from the Linux kernel, it might be
>> a good idea to isolate in their own file. It would also be good to identify
>> precisely which file they come from (the path relative to the root of the linux
>> repo) and the git commit you used.
>>
>> Also, since the copied code is rather large and non-trivial, does it pose copyright
>> problems? If we want to include it, maybe that separate file should not state that
>> the copyright is owned by the FSF, since it's not the case? Maybe others have
>> experience with this kind of things, or maybe we should get an advice from the FSF directly.
>
> A new file makes sense (especially with regards to copyright). However, the plan was
> to eventually remove the macros when "enough" time has passed - keeping everything
> in aarch64-sve-linux-ptrace.h made it simpler than removing files.
How come? Removing a file is not complicated.
> Is there a minimum kernel version that gdb requires to link against in order to build?
> The relevant defines are in 4.15. The kernel is now on 4.16.9.
>
> I’m guessing this must have come up before when other targets added new kernel features?
> I didn’t want to suddenly destroy everyone’s aarch64/targetall build trees without
> warning.
I don't think there's a specific Linux kernel version we require to build GDB, but it's
good to support as far back as possible with a reasonable effort. Since they have only
been added very recently, I would keep our copy of the macros for a while, if it's not
in the way of anything.
>>> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c
>>> index 9381786fda..84c7a41f40 100644
>>> --- a/gdb/nat/aarch64-sve-linux-ptrace.c
>>> +++ b/gdb/nat/aarch64-sve-linux-ptrace.c
>>> @@ -25,6 +25,13 @@
>>> #include "aarch64-sve-linux-ptrace.h"
>>> #include "arch/aarch64.h"
>>>
>>> +#ifndef GDBSERVER
>>> +#include "defs.h"
>>> +#endif
>>> +#include "regcache.h"
>>
>> Hmm we try not add any more "#ifdef GDBSERVER" in the common code.
>>
>> https://sourceware.org/gdb/wiki/Common#Header_files_in_common_code_.28defs.h_vs_server.h.2C_etc..29
>>
>> Instead, we should try defining a common interface (probably in common/common-regcache.h?) that the
>> common code will use, and that regcaches from GDB and GDBserver will implement.
>
> I tried using common-defs.h, but gdb/regcache.h requires defines from
> defs.h - RequireLongest and maybe others.
> Putting defs.h at the top of gdb/regcache.h then broke in a weird way.
> A lot of fiddling later, and I hadn’t found a way to make it work.
>
> Creating common/common-regcache.h gets a bit odd because, the functions
> I need for gdbserver (raw_supply, raw_collect and get_register_status)
> on gdb come from:
>
>
> class reg_buffer
> ...
> enum register_status get_register_status (int regnum) const;
> ...
>
>
> class readable_regcache : public reg_buffer
> ...
>
>
> class detached_regcache : public readable_regcache
> ...
> void raw_supply (int regnum, const void *buf);
> ...
>
>
> class regcache : public detached_regcache
> ...
> void raw_collect (int regnum, void *buf) const;
> ...
>
>
> I don’t think that this would work:
> class regcache : public detached_regcache, common_regcache
I did some quick hacking and it seems to work to have this in common/common-regcache.h
struct reg_buffer_common
{
virtual ~reg_buffer_common () = default;
virtual void raw_supply (int regnum, const void *buf) = 0;
virtual void raw_collect (int regnum, void *buf) const = 0;
virtual bool raw_compare (int regnum, const void *buf, int offset) const = 0;
virtual register_status get_register_status (int regnum) const = 0;
};
and make your code in nat/ take a pointer to reg_buffer_common. gdb's reg_buffer
and gdbserver's regcache should inherit from reg_buffer_common. I built
it on other regcache refactor patches I have in the pipeline, so maybe some other
changes in there are needed, maybe not. I pushed the result on this branch, it's
a bit raw but it should be enough to show the idea.
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/simark/regcache-for-alan
>> Since this returns allocated memory, could we return an RAII object? Either
>> std::vector, std::unique_ptr or gdb::unique_xmalloc_ptr.
>
> Yes, I’ll update with one of those. Is there any documentation explaining
> when to use which of these?
I don't think so, if you think it would be relevant we could add it to the wiki.
In short:
std::vector: Allocates and manages its own memory. This is good when we allocate
the space ourselves in GDB, and is especially good for buffers that need to change
size over time. By default, an std::vector<gdb_byte> would zero-out the buffer
on allocation. To avoid that when we don't need it, we have a specialization,
gdb::byte_vector, that leaves the memory uninitialized. You could
gdb::byte_vector buf (iovec.iov_len);
iovec.iov_base = buf.data ();
...
return buf;
std::unique_ptr: It is used to wrap a pointer to something that has been allocated
with "new", it will free it with "delete". You could do
std::unique_ptr<gdb_byte> buf (new gdb_byte[iovec.iov_len]);
iovec.iov_base = buf.get ()
...
return buf;
gdb::unique_xmalloc_ptr: It is like std::unique_ptr, but frees the memory using
xfree. It is useful to gradually add some RAII to GDB code that uses xmalloc.
For new code, I would either use std::vector or std::unique_ptr.
I just noticed that in the code in the current patch leaks the allocated memory
if ptrace fails and we call perror_with_name. Using an object that manages
memory will fix that.
>> When fetching registers, won't it be usually to fill a shiny new, empty regcache?
>> In that case, won't it always fall into the "if" branch?
>>
>> In any case, should we check the status of the VG register to make sure it's REG_VALID
>> before we try to collect it?
>
> I thought the same regcache was used each time registers needed reading?
> I’ll double check this.
> Either way, yes, should probably check REG_VALID
Hmm I'm not sure either. Indeed it would make sense that we re-use the same regcache.
Thanks for double-checking.
Thanks,
Simon
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 8/8] Ptrace support for Aarch64 SVE
2018-06-01 15:17 ` Simon Marchi
@ 2018-06-04 15:49 ` Alan Hayward
0 siblings, 0 replies; 47+ messages in thread
From: Alan Hayward @ 2018-06-04 15:49 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches, nd
>>>> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c
>>>> index 9381786fda..84c7a41f40 100644
>>>> --- a/gdb/nat/aarch64-sve-linux-ptrace.c
>>>> +++ b/gdb/nat/aarch64-sve-linux-ptrace.c
>>>> @@ -25,6 +25,13 @@
>>>> #include "aarch64-sve-linux-ptrace.h"
>>>> #include "arch/aarch64.h"
>>>>
>>>> +#ifndef GDBSERVER
>>>> +#include "defs.h"
>>>> +#endif
>>>> +#include "regcache.h"
>>>
>>> Hmm we try not add any more "#ifdef GDBSERVER" in the common code.
>>>
>>> https://sourceware.org/gdb/wiki/Common#Header_files_in_common_code_.28defs.h_vs_server.h.2C_etc..29
>>>
>>> Instead, we should try defining a common interface (probably in common/common-regcache.h?) that the
>>> common code will use, and that regcaches from GDB and GDBserver will implement.
>>
>> I tried using common-defs.h, but gdb/regcache.h requires defines from
>> defs.h - RequireLongest and maybe others.
>> Putting defs.h at the top of gdb/regcache.h then broke in a weird way.
>> A lot of fiddling later, and I hadn’t found a way to make it work.
>>
>> Creating common/common-regcache.h gets a bit odd because, the functions
>> I need for gdbserver (raw_supply, raw_collect and get_register_status)
>> on gdb come from:
>>
>>
>> class reg_buffer
>> ...
>> enum register_status get_register_status (int regnum) const;
>> ...
>>
>>
>> class readable_regcache : public reg_buffer
>> ...
>>
>>
>> class detached_regcache : public readable_regcache
>> ...
>> void raw_supply (int regnum, const void *buf);
>> ...
>>
>>
>> class regcache : public detached_regcache
>> ...
>> void raw_collect (int regnum, void *buf) const;
>> ...
>>
>>
>> I don’t think that this would work:
>> class regcache : public detached_regcache, common_regcache
>
> I did some quick hacking and it seems to work to have this in common/common-regcache.h
>
> struct reg_buffer_common
> {
> virtual ~reg_buffer_common () = default;
> virtual void raw_supply (int regnum, const void *buf) = 0;
> virtual void raw_collect (int regnum, void *buf) const = 0;
> virtual bool raw_compare (int regnum, const void *buf, int offset) const = 0;
> virtual register_status get_register_status (int regnum) const = 0;
> };
>
> and make your code in nat/ take a pointer to reg_buffer_common. gdb's reg_buffer
> and gdbserver's regcache should inherit from reg_buffer_common. I built
> it on other regcache refactor patches I have in the pipeline, so maybe some other
> changes in there are needed, maybe not. I pushed the result on this branch, it's
> a bit raw but it should be enough to show the idea.
>
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/simark/regcache-for-alan
Thanks for trying this out, however I tried adding your changes
into my build, but that results in the following:
../../src/binutils-gdb/gdb/linux-fork.c: In function ‘void fork_save_infrun_state(fork_info*, int)’:
../../src/binutils-gdb/gdb/linux-fork.c:298:75: error: invalid new-expression of abstract class type ‘readonly_detached_regcache’
fp->savedregs = new readonly_detached_regcache (*get_current_regcache ());
^
In file included from ../../src/binutils-gdb/gdb/gdbarch.h:70:0,
from ../../src/binutils-gdb/gdb/defs.h:531,
from ../../src/binutils-gdb/gdb/linux-fork.c:20:
../../src/binutils-gdb/gdb/regcache.h:367:7: note: because the following virtual functions are pure within ‘readonly_detached_regcache’:
class readonly_detached_regcache : public readable_regcache
^
In file included from ../../src/binutils-gdb/gdb/regcache.h:23:0,
from ../../src/binutils-gdb/gdb/gdbarch.h:70,
from ../../src/binutils-gdb/gdb/defs.h:531,
from ../../src/binutils-gdb/gdb/linux-fork.c:20:
../../src/binutils-gdb/gdb/common/common-regcache.h:68:16: note: virtual void reg_buffer_common::raw_supply(int, const void*)
virtual void raw_supply (int regnum, const void *buf) = 0;
^
../../src/binutils-gdb/gdb/common/common-regcache.h:69:16: note: virtual void reg_buffer_common::raw_collect(int, void*) const
virtual void raw_collect (int regnum, void *buf) const = 0;
And then similar errors trying to create a register_dump_reg_buffer.
I could then add the following to readonly_detached_regcache:
void raw_supply (int regnum, const void *buf) override
{ gdb_assert(false); }
void raw_collect (int regnum, void *buf) const override
{ gdb_assert(false); }
...Or even make the methods in reg_buffer_common have a
{ gdb_assert(false); } body.
But that feels wrong, as it’s breaking the nice regcache abstractions.
Any thoughts?
Alan.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 8/8] Ptrace support for Aarch64 SVE
2018-05-11 10:53 ` [PATCH 8/8] Ptrace support for Aarch64 SVE Alan Hayward
2018-05-31 13:40 ` Simon Marchi
@ 2018-05-31 20:17 ` Simon Marchi
1 sibling, 0 replies; 47+ messages in thread
From: Simon Marchi @ 2018-05-31 20:17 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
On 2018-05-11 06:52 AM, Alan Hayward wrote:
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index 1270b7e0c0..bc570ade6d 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -388,19 +388,65 @@ store_fpregs_to_thread (const struct regcache *regcache)
> }
> }
>
> +/* Fill GDB's register array with the sve register values
> + from the current thread. */
> +
> +static void
> +fetch_sveregs_from_thread (struct regcache *regcache)
> +{
> + gdb_byte *base = aarch64_sve_get_sveregs (ptid_get_lwp (inferior_ptid));
> + aarch64_sve_regs_copy_to_regcache (regcache, base);
> + xfree (base);
> +}
> +
> +/* Store to the current thread the valid sve register
> + values in the GDB's register array. */
> +
> +static void
> +store_sveregs_to_thread (struct regcache *regcache)
> +{
> + gdb_byte *base;
> + int ret, tid;
> + struct iovec iovec;
> +
> + tid = ptid_get_lwp (inferior_ptid);
I've just noticed these two uses of inferior_ptid. I think we should get the ptid
from the regcache instead.
Simon
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 7/8] Add methods to gdbserver regcache and raw_compare
2018-05-11 10:53 [PATCH 0/8] Add SVE support for Aarch64 GDB Alan Hayward
` (2 preceding siblings ...)
2018-05-11 10:53 ` [PATCH 8/8] Ptrace support for Aarch64 SVE Alan Hayward
@ 2018-05-11 10:53 ` Alan Hayward
2018-05-31 14:57 ` Pedro Alves
2018-05-11 10:53 ` [PATCH 4/8] Enable SVE for GDB Alan Hayward
` (4 subsequent siblings)
8 siblings, 1 reply; 47+ messages in thread
From: Alan Hayward @ 2018-05-11 10:53 UTC (permalink / raw)
To: gdb-patches; +Cc: nd, Alan Hayward
Add additional functions to gdbserver regcache to make it more like gdb
regcache. This will allow the next patch to add a common function which
uses regcache.
The alternatives for the next patch would be to either duplicate the
common code in both gdb and gdbserver, or alternatively pass function
pointers for read register, write register, get status to the common code.
In addition, add a register compare function. This will be used in the
next patch. Alternatively instead of adding a new function, I could read
into a buffer and then compare.
2018-05-11 Alan Hayward <alan.hayward@arm.com>
gdb/
* regcache.c (regcache::raw_compare): New function.
* regcache.h (regcache::raw_compare): New declaration.
gdbserver/
* regcache.c (register_data): New function.
(supply_register): Call member function.
(regcache::raw_supply): Replacement for supply_register.
(collect_register): Call member function.
(regcache::raw_collect): Replacement for collect_register.
(regcache::get_register_status): New function.
(regcache::raw_compare): Likewise.
* regcache.h: (regcache::raw_supply): New declaration.
* (regcache::raw_collect): Likewise.
* (regcache::raw_compare): Likewise.
* (regcache::get_register_status): Likewise.
---
gdb/gdbserver/regcache.c | 55 ++++++++++++++++++++++++++++++++++++++----------
gdb/gdbserver/regcache.h | 8 +++++++
gdb/regcache.c | 17 +++++++++++++++
gdb/regcache.h | 2 ++
4 files changed, 71 insertions(+), 11 deletions(-)
diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
index 0718b9f9a0..88f0db0483 100644
--- a/gdb/gdbserver/regcache.c
+++ b/gdb/gdbserver/regcache.c
@@ -300,7 +300,7 @@ regcache_register_size (const struct regcache *regcache, int n)
}
static unsigned char *
-register_data (struct regcache *regcache, int n, int fetch)
+register_data (const struct regcache *regcache, int n, int fetch)
{
return (regcache->registers
+ find_register_by_number (regcache->tdesc, n).offset / 8);
@@ -312,23 +312,27 @@ register_data (struct regcache *regcache, int n, int fetch)
void
supply_register (struct regcache *regcache, int n, const void *buf)
+{
+ return regcache->raw_supply (n, buf);
+}
+
+void
+regcache::raw_supply (int n, const void *buf)
{
if (buf)
{
- memcpy (register_data (regcache, n, 0), buf,
- register_size (regcache->tdesc, n));
+ memcpy (register_data (this, n, 0), buf, register_size (tdesc, n));
#ifndef IN_PROCESS_AGENT
- if (regcache->register_status != NULL)
- regcache->register_status[n] = REG_VALID;
+ if (register_status != NULL)
+ register_status[n] = REG_VALID;
#endif
}
else
{
- memset (register_data (regcache, n, 0), 0,
- register_size (regcache->tdesc, n));
+ memset (register_data (this, n, 0), 0, register_size (tdesc, n));
#ifndef IN_PROCESS_AGENT
- if (regcache->register_status != NULL)
- regcache->register_status[n] = REG_UNAVAILABLE;
+ if (register_status != NULL)
+ register_status[n] = REG_UNAVAILABLE;
#endif
}
}
@@ -410,10 +414,16 @@ supply_register_by_name (struct regcache *regcache,
void
collect_register (struct regcache *regcache, int n, void *buf)
{
- memcpy (buf, register_data (regcache, n, 1),
- register_size (regcache->tdesc, n));
+ regcache->raw_collect (n, buf);
+}
+
+void
+regcache::raw_collect (int n, void *buf) const
+{
+ memcpy (buf, register_data (this, n, 1), register_size (tdesc, n));
}
+
enum register_status
regcache_raw_read_unsigned (struct regcache *regcache, int regnum,
ULONGEST *val)
@@ -480,3 +490,26 @@ regcache_write_pc (struct regcache *regcache, CORE_ADDR pc)
}
#endif
+
+enum register_status
+regcache::get_register_status (int regnum) const
+{
+#ifndef IN_PROCESS_AGENT
+ gdb_assert (regnum >= 0 && regnum < tdesc->reg_defs.size ());
+ return (enum register_status) (register_status[regnum]);
+#else
+ return REG_VALID;
+#endif
+}
+
+/* Compare the contents of the register stored in the regcache (ignoring the
+ first OFFSET bytes) to the contents of BUF (without any offset). Returns 0
+ if identical. */
+
+int
+regcache::raw_compare (int regnum, const void *buf, int offset) const
+{
+ gdb_assert (register_size (tdesc, regnum) > offset);
+ return memcmp (buf, register_data (this, regnum, 1) + offset,
+ register_size (tdesc, regnum) - offset);
+}
diff --git a/gdb/gdbserver/regcache.h b/gdb/gdbserver/regcache.h
index 2c0df648f6..b3631bebd2 100644
--- a/gdb/gdbserver/regcache.h
+++ b/gdb/gdbserver/regcache.h
@@ -45,6 +45,14 @@ struct regcache
/* One of REG_UNAVAILBLE or REG_VALID. */
unsigned char *register_status;
#endif
+
+ void raw_supply (int regnum, const void *buf);
+
+ void raw_collect (int regnum, void *buf) const;
+
+ int raw_compare (int regnum, const void *buf, int offset) const;
+
+ enum register_status get_register_status (int regnum) const;
};
struct regcache *init_register_cache (struct regcache *regcache,
diff --git a/gdb/regcache.c b/gdb/regcache.c
index a3cc7743a2..92b9a69593 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1197,6 +1197,23 @@ regcache::collect_regset (const struct regset *regset,
transfer_regset (regset, NULL, regnum, NULL, buf, size);
}
+/* Compare the contents of the register stored in the regcache (ignoring the
+ first OFFSET bytes) to the contents of BUF (without any offset). Returns 0
+ if identical. */
+
+int
+regcache::raw_compare (int regnum, const void *buf, int offset) const
+{
+ const char *regbuf;
+ size_t size;
+
+ gdb_assert (buf != NULL);
+ assert_regnum (regnum);
+
+ regbuf = (const char *) register_buffer (regnum);
+ size = m_descr->sizeof_register[regnum];
+ return memcmp (buf, regbuf + offset, size - offset);
+}
/* Special handling for register PC. */
diff --git a/gdb/regcache.h b/gdb/regcache.h
index d7bb8b5c93..61d6e33c98 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -357,6 +357,8 @@ public:
void collect_regset (const struct regset *regset, int regnum,
void *buf, size_t size) const;
+ int raw_compare (int regnum, const void *buf, int offset) const;
+
ptid_t ptid () const
{
return m_ptid;
--
2.15.1 (Apple Git-101)
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 7/8] Add methods to gdbserver regcache and raw_compare
2018-05-11 10:53 ` [PATCH 7/8] Add methods to gdbserver regcache and raw_compare Alan Hayward
@ 2018-05-31 14:57 ` Pedro Alves
0 siblings, 0 replies; 47+ messages in thread
From: Pedro Alves @ 2018-05-31 14:57 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
On 05/11/2018 11:52 AM, Alan Hayward wrote:
> +/* Compare the contents of the register stored in the regcache (ignoring the
> + first OFFSET bytes) to the contents of BUF (without any offset). Returns 0
> + if identical. */
Is this a tristate return, like memcmp? If yes, that should be explicitly documented.
If not, I'd suggest renaming the function to something like raw_equals or
raw_contents_eq, flipping the logic around and return a bool, with true
meaning equal.
The description should probably be moved to the .h file.
> +
> +int
> +regcache::raw_compare (int regnum, const void *buf, int offset) const
> +{
> + gdb_assert (register_size (tdesc, regnum) > offset);
> + return memcmp (buf, register_data (this, regnum, 1) + offset,
> + register_size (tdesc, regnum) - offset);
> +}
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 4/8] Enable SVE for GDB
2018-05-11 10:53 [PATCH 0/8] Add SVE support for Aarch64 GDB Alan Hayward
` (3 preceding siblings ...)
2018-05-11 10:53 ` [PATCH 7/8] Add methods to gdbserver regcache and raw_compare Alan Hayward
@ 2018-05-11 10:53 ` Alan Hayward
2018-05-31 12:22 ` Simon Marchi
` (2 more replies)
2018-05-11 10:53 ` [PATCH 2/8] Function for reading the Aarch64 SVE vector length Alan Hayward
` (3 subsequent siblings)
8 siblings, 3 replies; 47+ messages in thread
From: Alan Hayward @ 2018-05-11 10:53 UTC (permalink / raw)
To: gdb-patches; +Cc: nd, Alan Hayward
This patch enables SVE support for GDB by reading the VQ when
creating a target description.
It also ensures that SVE is taken into account when creating
the tdep structure, and stores the current VQ value directly
in tdep.
With this patch, gdb on an aarch64 system with SVE will now detect
SVE. The SVE registers will be displayed (but the contents will be
invalid). The following patches fill out the contents.
2018-05-101 Alan Hayward <alan.hayward@arm.com>
* aarch64-linux-nat.c (aarch64_linux_read_description): Support SVE.
* aarch64-tdep.c (aarch64_get_tdesc_vq): New function.
(aarch64_gdbarch_init): Check for SVE.
* aarch64-tdep.h (gdbarch_tdep::has_sve): New function.
---
gdb/aarch64-linux-nat.c | 4 +--
gdb/aarch64-tdep.c | 73 +++++++++++++++++++++++++++++++++++++++----------
gdb/aarch64-tdep.h | 9 ++++++
3 files changed, 69 insertions(+), 17 deletions(-)
diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 9a6ee91d2d..1270b7e0c0 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -32,6 +32,7 @@
#include "aarch32-linux-nat.h"
#include "nat/aarch64-linux.h"
#include "nat/aarch64-linux-hw-point.h"
+#include "nat/aarch64-sve-linux-ptrace.h"
#include "elf/external.h"
#include "elf/common.h"
@@ -541,8 +542,7 @@ aarch64_linux_nat_target::read_description ()
if (ret == 0)
return tdesc_arm_with_neon;
else
- /* SVE not yet supported. */
- return aarch64_read_description (0);
+ return aarch64_read_description (aarch64_sve_get_vq (tid));
}
/* Convert a native/host siginfo object, into/from the siginfo in the
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 1dc31a43bd..7893579d58 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -2873,6 +2873,26 @@ aarch64_read_description (long vq)
return tdesc;
}
+/* Return the VQ used when creating the target description TDESC. */
+
+static long
+aarch64_get_tdesc_vq (const struct target_desc *tdesc)
+{
+ const struct tdesc_feature *feature_sve;
+
+ if (!tdesc_has_registers (tdesc))
+ return 0;
+
+ feature_sve = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.sve");
+
+ if (feature_sve == nullptr)
+ return 0;
+
+ long vl = tdesc_register_size (feature_sve, aarch64_sve_register_names[0]);
+ return sve_vq_from_vl (vl);
+}
+
+
/* 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.
@@ -2890,20 +2910,22 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
const struct target_desc *tdesc = info.target_desc;
int i;
int valid_p = 1;
- const struct tdesc_feature *feature;
+ const struct tdesc_feature *feature_core;
+ const struct tdesc_feature *feature_fpu;
+ const struct tdesc_feature *feature_sve;
int num_regs = 0;
int num_pseudo_regs = 0;
/* Ensure we always have a target descriptor. */
if (!tdesc_has_registers (tdesc))
- /* SVE is not yet supported. */
tdesc = aarch64_read_description (0);
-
gdb_assert (tdesc);
- feature = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.core");
+ feature_core = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.core");
+ feature_fpu = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.fpu");
+ feature_sve = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.sve");
- if (feature == NULL)
+ if (feature_core == NULL)
return NULL;
tdesc_data = tdesc_data_alloc ();
@@ -2911,25 +2933,45 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
/* Validate the descriptor provides the mandatory core R registers
and allocate their numbers. */
for (i = 0; i < ARRAY_SIZE (aarch64_r_register_names); i++)
- valid_p &=
- tdesc_numbered_register (feature, tdesc_data, AARCH64_X0_REGNUM + i,
- aarch64_r_register_names[i]);
+ valid_p &= tdesc_numbered_register (feature_core, tdesc_data,
+ AARCH64_X0_REGNUM + i,
+ aarch64_r_register_names[i]);
num_regs = AARCH64_X0_REGNUM + i;
- /* Look for the V registers. */
- feature = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.fpu");
- if (feature)
+ /* Add the V registers. */
+ if (feature_fpu != NULL)
{
+ gdb_assert (feature_sve == NULL);
+
/* Validate the descriptor provides the mandatory V registers
- and allocate their numbers. */
+ and allocate their numbers. */
for (i = 0; i < ARRAY_SIZE (aarch64_v_register_names); i++)
- valid_p &=
- tdesc_numbered_register (feature, tdesc_data, AARCH64_V0_REGNUM + i,
- aarch64_v_register_names[i]);
+ valid_p &= tdesc_numbered_register (feature_fpu, tdesc_data,
+ AARCH64_V0_REGNUM + i,
+ aarch64_v_register_names[i]);
num_regs = AARCH64_V0_REGNUM + i;
+ }
+
+ /* Add the SVE registers. */
+ if (feature_sve != NULL)
+ {
+ gdb_assert (feature_fpu == NULL);
+
+ /* Validate the descriptor provides the mandatory sve registers
+ and allocate their numbers. */
+ for (i = 0; i < ARRAY_SIZE (aarch64_sve_register_names); i++)
+ valid_p &= tdesc_numbered_register (feature_sve, tdesc_data,
+ AARCH64_SVE_Z0_REGNUM + i,
+ aarch64_sve_register_names[i]);
+ num_regs = AARCH64_SVE_Z0_REGNUM + i;
+ num_pseudo_regs += 32; /* add the Vn register pseudos. */
+ }
+
+ if (feature_fpu != NULL || feature_sve != NULL)
+ {
num_pseudo_regs += 32; /* add the Qn scalar register pseudos */
num_pseudo_regs += 32; /* add the Dn scalar register pseudos */
num_pseudo_regs += 32; /* add the Sn scalar register pseudos */
@@ -2969,6 +3011,7 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
tdep->lowest_pc = 0x20;
tdep->jb_pc = -1; /* Longjump support not enabled by default. */
tdep->jb_elt_size = 8;
+ tdep->vq = aarch64_get_tdesc_vq (tdesc);
set_gdbarch_push_dummy_call (gdbarch, aarch64_push_dummy_call);
set_gdbarch_frame_align (gdbarch, aarch64_frame_align);
diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
index c9fd7b3578..046de6228f 100644
--- a/gdb/aarch64-tdep.h
+++ b/gdb/aarch64-tdep.h
@@ -73,6 +73,15 @@ struct gdbarch_tdep
/* syscall record. */
int (*aarch64_syscall_record) (struct regcache *regcache, unsigned long svc_number);
+
+ /* The VQ value for SVE targets, or zero if SVE is not supported. */
+ long vq;
+
+ /* Returns true if the target supports SVE. */
+ bool has_sve ()
+ {
+ return vq != 0;
+ }
};
const target_desc *aarch64_read_description (long vq);
--
2.15.1 (Apple Git-101)
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 4/8] Enable SVE for GDB
2018-05-11 10:53 ` [PATCH 4/8] Enable SVE for GDB Alan Hayward
@ 2018-05-31 12:22 ` Simon Marchi
2018-06-04 11:19 ` Alan Hayward
2018-05-31 14:58 ` Pedro Alves
2018-05-31 16:13 ` Pedro Alves
2 siblings, 1 reply; 47+ messages in thread
From: Simon Marchi @ 2018-05-31 12:22 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
On 2018-05-11 06:52 AM, Alan Hayward wrote:
> This patch enables SVE support for GDB by reading the VQ when
> creating a target description.
>
> It also ensures that SVE is taken into account when creating
> the tdep structure, and stores the current VQ value directly
> in tdep.
>
> With this patch, gdb on an aarch64 system with SVE will now detect
> SVE. The SVE registers will be displayed (but the contents will be
> invalid). The following patches fill out the contents.
LGTM, with some nits.
> @@ -2911,25 +2933,45 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> /* Validate the descriptor provides the mandatory core R registers
> and allocate their numbers. */
> for (i = 0; i < ARRAY_SIZE (aarch64_r_register_names); i++)
> - valid_p &=
> - tdesc_numbered_register (feature, tdesc_data, AARCH64_X0_REGNUM + i,
> - aarch64_r_register_names[i]);
> + valid_p &= tdesc_numbered_register (feature_core, tdesc_data,
> + AARCH64_X0_REGNUM + i,
> + aarch64_r_register_names[i]);
>
> num_regs = AARCH64_X0_REGNUM + i;
>
> - /* Look for the V registers. */
> - feature = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.fpu");
> - if (feature)
> + /* Add the V registers. */
> + if (feature_fpu != NULL)
> {
> + gdb_assert (feature_sve == NULL);
Again, if this situation can result from a bad input passed to GDB (a bad tdesc
sent by the remote), we shouldn't gdb_assert on it, but show an error message
and gracefully fail.
> +
> /* Validate the descriptor provides the mandatory V registers
> - and allocate their numbers. */
> + and allocate their numbers. */
> for (i = 0; i < ARRAY_SIZE (aarch64_v_register_names); i++)
> - valid_p &=
> - tdesc_numbered_register (feature, tdesc_data, AARCH64_V0_REGNUM + i,
> - aarch64_v_register_names[i]);
> + valid_p &= tdesc_numbered_register (feature_fpu, tdesc_data,
> + AARCH64_V0_REGNUM + i,
> + aarch64_v_register_names[i]);
>
> num_regs = AARCH64_V0_REGNUM + i;
> + }
> +
> + /* Add the SVE registers. */
> + if (feature_sve != NULL)
> + {
> + gdb_assert (feature_fpu == NULL);
Same here.
> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
> index c9fd7b3578..046de6228f 100644
> --- a/gdb/aarch64-tdep.h
> +++ b/gdb/aarch64-tdep.h
> @@ -73,6 +73,15 @@ struct gdbarch_tdep
>
> /* syscall record. */
> int (*aarch64_syscall_record) (struct regcache *regcache, unsigned long svc_number);
> +
> + /* The VQ value for SVE targets, or zero if SVE is not supported. */
> + long vq;
> +
> + /* Returns true if the target supports SVE. */
> + bool has_sve ()
> + {
> + return vq != 0;
> + }
This method can be const.
Simon
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 4/8] Enable SVE for GDB
2018-05-31 12:22 ` Simon Marchi
@ 2018-06-04 11:19 ` Alan Hayward
0 siblings, 0 replies; 47+ messages in thread
From: Alan Hayward @ 2018-06-04 11:19 UTC (permalink / raw)
To: Simon Marchi, Pedro Alves; +Cc: gdb-patches, nd
Push with minor changes as suggested below.
> On 31 May 2018, at 12:55, Simon Marchi <simon.marchi@ericsson.com> wrote:
>
> On 2018-05-11 06:52 AM, Alan Hayward wrote:
>> This patch enables SVE support for GDB by reading the VQ when
>> creating a target description.
>>
>> It also ensures that SVE is taken into account when creating
>> the tdep structure, and stores the current VQ value directly
>> in tdep.
>>
>> With this patch, gdb on an aarch64 system with SVE will now detect
>> SVE. The SVE registers will be displayed (but the contents will be
>> invalid). The following patches fill out the contents.
>
> LGTM, with some nits.
>
>> @@ -2911,25 +2933,45 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>> /* Validate the descriptor provides the mandatory core R registers
>> and allocate their numbers. */
>> for (i = 0; i < ARRAY_SIZE (aarch64_r_register_names); i++)
>> - valid_p &=
>> - tdesc_numbered_register (feature, tdesc_data, AARCH64_X0_REGNUM + i,
>> - aarch64_r_register_names[i]);
>> + valid_p &= tdesc_numbered_register (feature_core, tdesc_data,
>> + AARCH64_X0_REGNUM + i,
>> + aarch64_r_register_names[i]);
>>
>> num_regs = AARCH64_X0_REGNUM + i;
>>
>> - /* Look for the V registers. */
>> - feature = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.fpu");
>> - if (feature)
>> + /* Add the V registers. */
>> + if (feature_fpu != NULL)
>> {
>> + gdb_assert (feature_sve == NULL);
>
> Again, if this situation can result from a bad input passed to GDB (a bad tdesc
> sent by the remote), we shouldn't gdb_assert on it, but show an error message
> and gracefully fail.
Ok. Updated to use _error.
if (feature_sve != NULL)
error (_("Program contains both fpu and SVE features."));
>
>> +
>> /* Validate the descriptor provides the mandatory V registers
>> - and allocate their numbers. */
>> + and allocate their numbers. */
>> for (i = 0; i < ARRAY_SIZE (aarch64_v_register_names); i++)
>> - valid_p &=
>> - tdesc_numbered_register (feature, tdesc_data, AARCH64_V0_REGNUM + i,
>> - aarch64_v_register_names[i]);
>> + valid_p &= tdesc_numbered_register (feature_fpu, tdesc_data,
>> + AARCH64_V0_REGNUM + i,
>> + aarch64_v_register_names[i]);
>>
>> num_regs = AARCH64_V0_REGNUM + i;
>> + }
>> +
>> + /* Add the SVE registers. */
>> + if (feature_sve != NULL)
>> + {
>> + gdb_assert (feature_fpu == NULL);
>
> Same here.
I removed this one as it’s redundant - it’ll be caught by the error above.
>
>> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
>> index c9fd7b3578..046de6228f 100644
>> --- a/gdb/aarch64-tdep.h
>> +++ b/gdb/aarch64-tdep.h
>> @@ -73,6 +73,15 @@ struct gdbarch_tdep
>>
>> /* syscall record. */
>> int (*aarch64_syscall_record) (struct regcache *regcache, unsigned long svc_number);
>> +
>> + /* The VQ value for SVE targets, or zero if SVE is not supported. */
>> + long vq;
>> +
>> + /* Returns true if the target supports SVE. */
>> + bool has_sve ()
>> + {
>> + return vq != 0;
>> + }
>
> This method can be const.
>
Done.
> On 31 May 2018, at 15:59, Pedro Alves <palves@redhat.com> wrote:
>
> On 05/11/2018 11:52 AM, Alan Hayward wrote:
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -2873,6 +2873,26 @@ aarch64_read_description (long vq)
>> return tdesc;
>> }
>>
>> +/* Return the VQ used when creating the target description TDESC. */
>> +
>> +static long
>> +aarch64_get_tdesc_vq (const struct target_desc *tdesc)
>
> Is this use of "long" significant? I mean, is it assuming 64-bit?
> I ask because longs are not 64-bit on x64 Windows, so it would
> do the wrong thing when cross debugging.
>
Fixed by using both follow on patch "[PATCH] Use uint64_t for SVE VQ” and
obvious patch below:
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 6674b7654e..0172e4ccd1 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -2875,7 +2875,7 @@ aarch64_read_description (uint64_t vq)
/* Return the VQ used when creating the target description TDESC. */
-static long
+static uint64_t
aarch64_get_tdesc_vq (const struct target_desc *tdesc)
{
const struct tdesc_feature *feature_sve;
@@ -2888,7 +2888,8 @@ aarch64_get_tdesc_vq (const struct target_desc *tdesc)
if (feature_sve == nullptr)
return 0;
- long vl = tdesc_register_size (feature_sve, aarch64_sve_register_names[0]);
+ uint64_t vl = tdesc_register_size (feature_sve,
+ aarch64_sve_register_names[0]);
return sve_vq_from_vl (vl);
}
diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
index b6b9b30e71..598a0aafa2 100644
--- a/gdb/aarch64-tdep.h
+++ b/gdb/aarch64-tdep.h
@@ -75,7 +75,7 @@ struct gdbarch_tdep
int (*aarch64_syscall_record) (struct regcache *regcache, unsigned long svc_number);
/* The VQ value for SVE targets, or zero if SVE is not supported. */
- long vq;
+ uint64_t vq;
/* Returns true if the target supports SVE. */
bool has_sve () const
Thanks!
Alan.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/8] Enable SVE for GDB
2018-05-11 10:53 ` [PATCH 4/8] Enable SVE for GDB Alan Hayward
2018-05-31 12:22 ` Simon Marchi
@ 2018-05-31 14:58 ` Pedro Alves
2018-05-31 16:13 ` Pedro Alves
2 siblings, 0 replies; 47+ messages in thread
From: Pedro Alves @ 2018-05-31 14:58 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
On 05/11/2018 11:52 AM, Alan Hayward wrote:
> + if (feature_sve != NULL)
> + {
> + gdb_assert (feature_fpu == NULL);
> +
> + /* Validate the descriptor provides the mandatory sve registers
"description". Please "grep -i" your patches for "descriptor", since
this appears in other spots.
Uppercase "SVE", I'd assume. I noticed that at least patch #8 uses
lowercase "sve" in an error message, which should probably be fixed.
Suggest grepping the patches for that too.
> + and allocate their numbers. */
> + for (i = 0; i < ARRAY_SIZE (aarch64_sve_register_names); i++)
> + valid_p &= tdesc_numbered_register (feature_sve, tdesc_data,
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 4/8] Enable SVE for GDB
2018-05-11 10:53 ` [PATCH 4/8] Enable SVE for GDB Alan Hayward
2018-05-31 12:22 ` Simon Marchi
2018-05-31 14:58 ` Pedro Alves
@ 2018-05-31 16:13 ` Pedro Alves
2018-05-31 16:20 ` Alan Hayward
2 siblings, 1 reply; 47+ messages in thread
From: Pedro Alves @ 2018-05-31 16:13 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
On 05/11/2018 11:52 AM, Alan Hayward wrote:
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -2873,6 +2873,26 @@ aarch64_read_description (long vq)
> return tdesc;
> }
>
> +/* Return the VQ used when creating the target description TDESC. */
> +
> +static long
> +aarch64_get_tdesc_vq (const struct target_desc *tdesc)
Is this use of "long" significant? I mean, is it assuming 64-bit?
I ask because longs are not 64-bit on x64 Windows, so it would
do the wrong thing when cross debugging.
> +{
> + const struct tdesc_feature *feature_sve;
> +
> + if (!tdesc_has_registers (tdesc))
> + return 0;
> +
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 4/8] Enable SVE for GDB
2018-05-31 16:13 ` Pedro Alves
@ 2018-05-31 16:20 ` Alan Hayward
2018-05-31 16:27 ` Pedro Alves
0 siblings, 1 reply; 47+ messages in thread
From: Alan Hayward @ 2018-05-31 16:20 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches, nd
> On 31 May 2018, at 15:59, Pedro Alves <palves@redhat.com> wrote:
>
> On 05/11/2018 11:52 AM, Alan Hayward wrote:
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -2873,6 +2873,26 @@ aarch64_read_description (long vq)
>> return tdesc;
>> }
>>
>> +/* Return the VQ used when creating the target description TDESC. */
>> +
>> +static long
>> +aarch64_get_tdesc_vq (const struct target_desc *tdesc)
>
> Is this use of "long" significant? I mean, is it assuming 64-bit?
> I ask because longs are not 64-bit on x64 Windows, so it would
> do the wrong thing when cross debugging.
In the kernel structure it’s a 16bit value.
However, the VG "register" in the regcache is a 64bit value. (It’s not
a real system register, but helps a great deal to see it as one in gdb,
and we need it as a dwarf register.) I made it 64bits to match the
minimum size of all the other aarch64 registers.
I did have it as 16bits at one point, but it went wrong when using it
with dwarf as it expects all the dwarf registers to be the minimum
register.
I chose to do the conversion from 16bits to 64bits at the point vg is
read from the kernel. This makes the code easy as from then on as it’s
always 64bits throughout.
Alternatively, I could have kept as 16bits up to the point it hits
the regcache. But that leaves two different types around the code.
Alan.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/8] Enable SVE for GDB
2018-05-31 16:20 ` Alan Hayward
@ 2018-05-31 16:27 ` Pedro Alves
2018-05-31 18:06 ` Alan Hayward
0 siblings, 1 reply; 47+ messages in thread
From: Pedro Alves @ 2018-05-31 16:27 UTC (permalink / raw)
To: Alan Hayward; +Cc: gdb-patches, nd
On 05/31/2018 05:13 PM, Alan Hayward wrote:
>
>
>> On 31 May 2018, at 15:59, Pedro Alves <palves@redhat.com> wrote:
>>
>> On 05/11/2018 11:52 AM, Alan Hayward wrote:
>>> --- a/gdb/aarch64-tdep.c
>>> +++ b/gdb/aarch64-tdep.c
>>> @@ -2873,6 +2873,26 @@ aarch64_read_description (long vq)
>>> return tdesc;
>>> }
>>>
>>> +/* Return the VQ used when creating the target description TDESC. */
>>> +
>>> +static long
>>> +aarch64_get_tdesc_vq (const struct target_desc *tdesc)
>>
>> Is this use of "long" significant? I mean, is it assuming 64-bit?
>> I ask because longs are not 64-bit on x64 Windows, so it would
>> do the wrong thing when cross debugging.
>
> In the kernel structure it’s a 16bit value.
>
> However, the VG "register" in the regcache is a 64bit value. (It’s not
> a real system register, but helps a great deal to see it as one in gdb,
> and we need it as a dwarf register.) I made it 64bits to match the
> minimum size of all the other aarch64 registers.
OK, fine to make it 64-bit, but then the "do the wrong thing when
cross debugging" point applies. "long" is 32-bit on some hosts.
I don't really know what values VG can take -- can they be
negative? I.e., is sign extension expected? You did not seem to address
this in your reply. Why not use host-independent ULONGEST, for example?
> I did have it as 16bits at one point, but it went wrong when using it
> with dwarf as it expects all the dwarf registers to be the minimum
> register.
>
> I chose to do the conversion from 16bits to 64bits at the point vg is
> read from the kernel. This makes the code easy as from then on as it’s
> always 64bits throughout.
>
> Alternatively, I could have kept as 16bits up to the point it hits
> the regcache. But that leaves two different types around the code.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 4/8] Enable SVE for GDB
2018-05-31 16:27 ` Pedro Alves
@ 2018-05-31 18:06 ` Alan Hayward
0 siblings, 0 replies; 47+ messages in thread
From: Alan Hayward @ 2018-05-31 18:06 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches, nd
> On 31 May 2018, at 17:18, Pedro Alves <palves@redhat.com> wrote:
>
> On 05/31/2018 05:13 PM, Alan Hayward wrote:
>>
>>
>>> On 31 May 2018, at 15:59, Pedro Alves <palves@redhat.com> wrote:
>>>
>>> On 05/11/2018 11:52 AM, Alan Hayward wrote:
>>>> --- a/gdb/aarch64-tdep.c
>>>> +++ b/gdb/aarch64-tdep.c
>>>> @@ -2873,6 +2873,26 @@ aarch64_read_description (long vq)
>>>> return tdesc;
>>>> }
>>>>
>>>> +/* Return the VQ used when creating the target description TDESC. */
>>>> +
>>>> +static long
>>>> +aarch64_get_tdesc_vq (const struct target_desc *tdesc)
>>>
>>> Is this use of "long" significant? I mean, is it assuming 64-bit?
>>> I ask because longs are not 64-bit on x64 Windows, so it would
>>> do the wrong thing when cross debugging.
>>
>> In the kernel structure it’s a 16bit value.
>>
>> However, the VG "register" in the regcache is a 64bit value. (It’s not
>> a real system register, but helps a great deal to see it as one in gdb,
>> and we need it as a dwarf register.) I made it 64bits to match the
>> minimum size of all the other aarch64 registers.
>
> OK, fine to make it 64-bit, but then the "do the wrong thing when
> cross debugging" point applies. "long" is 32-bit on some hosts.
> I don't really know what values VG can take -- can they be
> negative? I.e., is sign extension expected? You did not seem to address
> this in your reply. Why not use host-independent ULONGEST, for example?
Right, I didn’t spot that. It can’t be negative, only 0 or a value
divisible by 2.
To match the register, I’m thinking uint64_t would be right then.
Alan.
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 2/8] Function for reading the Aarch64 SVE vector length.
2018-05-11 10:53 [PATCH 0/8] Add SVE support for Aarch64 GDB Alan Hayward
` (4 preceding siblings ...)
2018-05-11 10:53 ` [PATCH 4/8] Enable SVE for GDB Alan Hayward
@ 2018-05-11 10:53 ` Alan Hayward
2018-05-31 12:06 ` Simon Marchi
` (2 more replies)
2018-05-11 11:52 ` [PATCH 5/8] Add aarch64 psuedo help functions Alan Hayward
` (2 subsequent siblings)
8 siblings, 3 replies; 47+ messages in thread
From: Alan Hayward @ 2018-05-11 10:53 UTC (permalink / raw)
To: gdb-patches; +Cc: nd, Alan Hayward
Add a method for reading the SVE vector length using ptrace. This returns
0 for systems without SVE support.
Note the defines taken from Linux kernel headers in aarch64-sve-linux-ptrace.h.
See the covering email for details about this.
There are multiple ways of expressing the vector length. Thankfully these are
all wll defined. I've added convertors for going from one to the other.
Hopefully this will help to prevent future confusion.
2018-05-11 Alan Hayward <alan.hayward@arm.com>
gdb/
* Makefile.in: Add new header.
* gdb/arch/aarch64.h (sve_vg_from_vl): New macro.
(sve_vl_from_vg): Likewise.
(sve_vq_from_vl): Likewise.
(sve_vl_from_vq): Likewise.
(sve_vq_from_vg): Likewise.
(sve_vg_from_vq): Likewise.
* configure.nat: Add new c file.
* nat/aarch64-sve-linux-ptrace.c: New file.
* nat/aarch64-sve-linux-ptrace.h: New file.
gdbserver/
* configure.srv: Add new c/h file.
---
gdb/Makefile.in | 1 +
gdb/arch/aarch64.h | 17 +++++++++
gdb/configure.nat | 2 +-
gdb/gdbserver/configure.srv | 1 +
gdb/nat/aarch64-sve-linux-ptrace.c | 52 +++++++++++++++++++++++++++
gdb/nat/aarch64-sve-linux-ptrace.h | 73 ++++++++++++++++++++++++++++++++++++++
6 files changed, 145 insertions(+), 1 deletion(-)
create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.c
create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.h
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 87d74a7703..64042d1bd1 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1478,6 +1478,7 @@ HFILES_NO_SRCDIR = \
mi/mi-parse.h \
nat/aarch64-linux.h \
nat/aarch64-linux-hw-point.h \
+ nat/aarch64-sve-linux-ptrace.h \
nat/amd64-linux-siginfo.h \
nat/gdb_ptrace.h \
nat/gdb_thread_db.h \
diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
index 1846e04163..af0b157c51 100644
--- a/gdb/arch/aarch64.h
+++ b/gdb/arch/aarch64.h
@@ -48,6 +48,23 @@ enum aarch64_regnum
#define AARCH64_V_REGS_NUM 32
#define AARCH64_NUM_REGS AARCH64_FPCR_REGNUM + 1
+/* There are a number of ways of expressing the current SVE vector size:
+
+ VL : Vector Length.
+ The number of bytes in an SVE Z register.
+ VQ : Vector Quotient.
+ The number of 128bit chunks in an SVE Z register.
+ VG : Vector Gradient.
+ The number of 64bit chunks in an SVE Z register. */
+
+#define sve_vg_from_vl(vl) ((vl) / 8)
+#define sve_vl_from_vg(vg) ((vg) * 8)
+#define sve_vq_from_vl(vl) ((vl) / 0x10)
+#define sve_vl_from_vq(vq) ((vq) * 0x10)
+#define sve_vq_from_vg(vg) (sve_vq_from_vl (sve_vl_from_vg (vg)))
+#define sve_vg_from_vq(vq) (sve_vg_from_vl (sve_vl_from_vq (vq)))
+
+
/* Maximum supported VQ value. Increase if required. */
#define AARCH64_MAX_SVE_VQ 16
diff --git a/gdb/configure.nat b/gdb/configure.nat
index 6b0f44fede..d7d79adaca 100644
--- a/gdb/configure.nat
+++ b/gdb/configure.nat
@@ -228,7 +228,7 @@ case ${gdb_host} in
aarch64)
# Host: AArch64 based machine running GNU/Linux
NATDEPFILES="${NATDEPFILES} aarch64-linux-nat.o \
- aarch32-linux-nat.o aarch64-linux-hw-point.o aarch64-linux.o"
+ aarch32-linux-nat.o aarch64-linux-hw-point.o aarch64-linux.o aarch64-sve-linux-ptrace.o"
;;
arm)
# Host: ARM based machine running GNU/Linux
diff --git a/gdb/gdbserver/configure.srv b/gdb/gdbserver/configure.srv
index ffeefb9b92..8bf0dcc650 100644
--- a/gdb/gdbserver/configure.srv
+++ b/gdb/gdbserver/configure.srv
@@ -54,6 +54,7 @@ case "${target}" in
srv_tgtobj="$srv_tgtobj arch/aarch64-insn.o"
srv_tgtobj="$srv_tgtobj arch/aarch64.o"
srv_tgtobj="$srv_tgtobj linux-aarch64-tdesc.o"
+ srv_tgtobj="$srv_tgtobj aarch64-sve-linux-ptrace.o"
srv_tgtobj="${srv_tgtobj} $srv_linux_obj"
srv_linux_regsets=yes
srv_linux_thread_db=yes
diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c
new file mode 100644
index 0000000000..9381786fda
--- /dev/null
+++ b/gdb/nat/aarch64-sve-linux-ptrace.c
@@ -0,0 +1,52 @@
+/* Common target dependent for AArch64 systems.
+
+ Copyright (C) 2017 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include <sys/utsname.h>
+#include <sys/uio.h>
+#include "common-defs.h"
+#include "elf/external.h"
+#include "elf/common.h"
+#include "aarch64-sve-linux-ptrace.h"
+#include "arch/aarch64.h"
+
+/* Read VQ for the given tid using ptrace. If SVE is not supported then zero
+ is returned (on a system that supports SVE, then VQ cannot be zeo). */
+
+unsigned long
+aarch64_sve_get_vq (int tid)
+{
+ struct iovec iovec;
+ struct user_sve_header header;
+
+ iovec.iov_len = sizeof (header);
+ iovec.iov_base = &header;
+
+ /* Ptrace gives the vector length in bytes. Convert it to VQ, the number of
+ 128bit chunks in a Z register. We use VQ because 128bits is the minimum
+ a Z register can increase in size. */
+
+ if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec) < 0)
+ /* SVE is not supported. */
+ return 0;
+
+ long vq = sve_vq_from_vl (header.vl);
+ gdb_assert (sve_vl_valid (header.vl));
+
+ return vq;
+}
diff --git a/gdb/nat/aarch64-sve-linux-ptrace.h b/gdb/nat/aarch64-sve-linux-ptrace.h
new file mode 100644
index 0000000000..b318150ac1
--- /dev/null
+++ b/gdb/nat/aarch64-sve-linux-ptrace.h
@@ -0,0 +1,73 @@
+/* Common target dependent for AArch64 systems.
+
+ Copyright (C) 2017 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#ifndef AARCH64_SVE_LINUX_PTRACE_H
+#define AARCH64_SVE_LINUX_PTRACE_H
+
+/* Where indicated, this file contains defines and macros lifted directly from
+ the Linux kernel headers, with no modification.
+ Refer to Linux kernel documentation for details. */
+
+#include <asm/sigcontext.h>
+#include <sys/utsname.h>
+#include <sys/ptrace.h>
+#include <asm/ptrace.h>
+
+/* Read VQ for the given tid using ptrace. If SVE is not supported then zero
+ is returned (on a system that supports SVE, then VQ cannot be zeo). */
+
+extern unsigned long aarch64_sve_get_vq (int tid);
+
+/* Structures and defines taken from sigcontext.h. */
+
+#ifndef SVE_SIG_ZREGS_SIZE
+
+#define SVE_VQ_BYTES 16 /* number of bytes per quadword */
+
+#define SVE_VQ_MIN 1
+#define SVE_VQ_MAX 512
+
+#define SVE_VL_MIN (SVE_VQ_MIN * SVE_VQ_BYTES)
+#define SVE_VL_MAX (SVE_VQ_MAX * SVE_VQ_BYTES)
+
+#define SVE_NUM_ZREGS 32
+#define SVE_NUM_PREGS 16
+
+#define sve_vl_valid(vl) \
+ ((vl) % SVE_VQ_BYTES == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
+
+#endif /* SVE_SIG_ZREGS_SIZE. */
+
+
+/* Structures and defines taken from ptrace.h. */
+
+#ifndef SVE_PT_SVE_ZREG_SIZE
+
+struct user_sve_header {
+ __u32 size; /* total meaningful regset content in bytes */
+ __u32 max_size; /* maxmium possible size for this thread */
+ __u16 vl; /* current vector length */
+ __u16 max_vl; /* maximum possible vector length */
+ __u16 flags;
+ __u16 __reserved;
+};
+
+#endif /* SVE_PT_SVE_ZREG_SIZE. */
+
+#endif /* aarch64-sve-linux-ptrace.h */
--
2.15.1 (Apple Git-101)
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 2/8] Function for reading the Aarch64 SVE vector length.
2018-05-11 10:53 ` [PATCH 2/8] Function for reading the Aarch64 SVE vector length Alan Hayward
@ 2018-05-31 12:06 ` Simon Marchi
2018-05-31 14:18 ` Alan Hayward
2018-05-31 14:57 ` Pedro Alves
2018-06-05 20:01 ` Sergio Durigan Junior
2 siblings, 1 reply; 47+ messages in thread
From: Simon Marchi @ 2018-05-31 12:06 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
Hi Alan,
LGTM, with some nits.
On 2018-05-11 06:52 AM, Alan Hayward wrote:
> Add a method for reading the SVE vector length using ptrace. This returns
> 0 for systems without SVE support.
>
> Note the defines taken from Linux kernel headers in aarch64-sve-linux-ptrace.h.
> See the covering email for details about this.
Using ptrace to read the SVE registers (and the SVE register length) will work even
for kernels that didn't have these macros?
> There are multiple ways of expressing the vector length. Thankfully these are
> all wll defined. I've added convertors for going from one to the other.
> Hopefully this will help to prevent future confusion.
Copyright 2017 -> 2018 in the new files.
>
> 2018-05-11 Alan Hayward <alan.hayward@arm.com>
>
> gdb/
> * Makefile.in: Add new header.
> * gdb/arch/aarch64.h (sve_vg_from_vl): New macro.
> (sve_vl_from_vg): Likewise.
> (sve_vq_from_vl): Likewise.
> (sve_vl_from_vq): Likewise.
> (sve_vq_from_vg): Likewise.
> (sve_vg_from_vq): Likewise.
> * configure.nat: Add new c file.
> * nat/aarch64-sve-linux-ptrace.c: New file.
> * nat/aarch64-sve-linux-ptrace.h: New file.
>
> gdbserver/
> * configure.srv: Add new c/h file.
> ---
> gdb/Makefile.in | 1 +
> gdb/arch/aarch64.h | 17 +++++++++
> gdb/configure.nat | 2 +-
> gdb/gdbserver/configure.srv | 1 +
> gdb/nat/aarch64-sve-linux-ptrace.c | 52 +++++++++++++++++++++++++++
> gdb/nat/aarch64-sve-linux-ptrace.h | 73 ++++++++++++++++++++++++++++++++++++++
> 6 files changed, 145 insertions(+), 1 deletion(-)
> create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.c
> create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.h
>
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 87d74a7703..64042d1bd1 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -1478,6 +1478,7 @@ HFILES_NO_SRCDIR = \
> mi/mi-parse.h \
> nat/aarch64-linux.h \
> nat/aarch64-linux-hw-point.h \
> + nat/aarch64-sve-linux-ptrace.h \
> nat/amd64-linux-siginfo.h \
> nat/gdb_ptrace.h \
> nat/gdb_thread_db.h \
> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
> index 1846e04163..af0b157c51 100644
> --- a/gdb/arch/aarch64.h
> +++ b/gdb/arch/aarch64.h
> @@ -48,6 +48,23 @@ enum aarch64_regnum
> #define AARCH64_V_REGS_NUM 32
> #define AARCH64_NUM_REGS AARCH64_FPCR_REGNUM + 1
>
> +/* There are a number of ways of expressing the current SVE vector size:
> +
> + VL : Vector Length.
> + The number of bytes in an SVE Z register.
> + VQ : Vector Quotient.
> + The number of 128bit chunks in an SVE Z register.
> + VG : Vector Gradient.
> + The number of 64bit chunks in an SVE Z register. */
> +
> +#define sve_vg_from_vl(vl) ((vl) / 8)
> +#define sve_vl_from_vg(vg) ((vg) * 8)
> +#define sve_vq_from_vl(vl) ((vl) / 0x10)
> +#define sve_vl_from_vq(vq) ((vq) * 0x10)
> +#define sve_vq_from_vg(vg) (sve_vq_from_vl (sve_vl_from_vg (vg)))
> +#define sve_vg_from_vq(vq) (sve_vg_from_vl (sve_vl_from_vq (vq)))
> +
> +
> /* Maximum supported VQ value. Increase if required. */
> #define AARCH64_MAX_SVE_VQ 16
>
> diff --git a/gdb/configure.nat b/gdb/configure.nat
> index 6b0f44fede..d7d79adaca 100644
> --- a/gdb/configure.nat
> +++ b/gdb/configure.nat
> @@ -228,7 +228,7 @@ case ${gdb_host} in
> aarch64)
> # Host: AArch64 based machine running GNU/Linux
> NATDEPFILES="${NATDEPFILES} aarch64-linux-nat.o \
> - aarch32-linux-nat.o aarch64-linux-hw-point.o aarch64-linux.o"
> + aarch32-linux-nat.o aarch64-linux-hw-point.o aarch64-linux.o aarch64-sve-linux-ptrace.o"
Please wrap to 80 columns.
> ;;
> arm)
> # Host: ARM based machine running GNU/Linux
> diff --git a/gdb/gdbserver/configure.srv b/gdb/gdbserver/configure.srv
> index ffeefb9b92..8bf0dcc650 100644
> --- a/gdb/gdbserver/configure.srv
> +++ b/gdb/gdbserver/configure.srv
> @@ -54,6 +54,7 @@ case "${target}" in
> srv_tgtobj="$srv_tgtobj arch/aarch64-insn.o"
> srv_tgtobj="$srv_tgtobj arch/aarch64.o"
> srv_tgtobj="$srv_tgtobj linux-aarch64-tdesc.o"
> + srv_tgtobj="$srv_tgtobj aarch64-sve-linux-ptrace.o"
> srv_tgtobj="${srv_tgtobj} $srv_linux_obj"
> srv_linux_regsets=yes
> srv_linux_thread_db=yes
> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c
> new file mode 100644
> index 0000000000..9381786fda
> --- /dev/null
> +++ b/gdb/nat/aarch64-sve-linux-ptrace.c
> @@ -0,0 +1,52 @@
> +/* Common target dependent for AArch64 systems.
> +
> + Copyright (C) 2017 Free Software Foundation, Inc.
> +
> + This file is part of GDB.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +#include <sys/utsname.h>
> +#include <sys/uio.h>
> +#include "common-defs.h"
> +#include "elf/external.h"
> +#include "elf/common.h"
> +#include "aarch64-sve-linux-ptrace.h"
> +#include "arch/aarch64.h"
> +
> +/* Read VQ for the given tid using ptrace. If SVE is not supported then zero
> + is returned (on a system that supports SVE, then VQ cannot be zeo). */
Here, put a reference to the .h as usual.
> +
> +unsigned long
> +aarch64_sve_get_vq (int tid)
> +{
> + struct iovec iovec;
> + struct user_sve_header header;
> +
> + iovec.iov_len = sizeof (header);
> + iovec.iov_base = &header;
> +
> + /* Ptrace gives the vector length in bytes. Convert it to VQ, the number of
> + 128bit chunks in a Z register. We use VQ because 128bits is the minimum
> + a Z register can increase in size. */
> +
> + if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec) < 0)
> + /* SVE is not supported. */
> + return 0;
Add braces here.
> +
> + long vq = sve_vq_from_vl (header.vl);
> + gdb_assert (sve_vl_valid (header.vl));
We should avoid gdb_assert for bad input values (including what we receive from the
kernel). Could we display a warning and return 0?
> +
> + return vq;
> +}
> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.h b/gdb/nat/aarch64-sve-linux-ptrace.h
> new file mode 100644
> index 0000000000..b318150ac1
> --- /dev/null
> +++ b/gdb/nat/aarch64-sve-linux-ptrace.h
> @@ -0,0 +1,73 @@
> +/* Common target dependent for AArch64 systems.
> +
> + Copyright (C) 2017 Free Software Foundation, Inc.
> +
> + This file is part of GDB.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +#ifndef AARCH64_SVE_LINUX_PTRACE_H
> +#define AARCH64_SVE_LINUX_PTRACE_H
> +
> +/* Where indicated, this file contains defines and macros lifted directly from
> + the Linux kernel headers, with no modification.
> + Refer to Linux kernel documentation for details. */
> +
> +#include <asm/sigcontext.h>
> +#include <sys/utsname.h>
> +#include <sys/ptrace.h>
> +#include <asm/ptrace.h>
> +
> +/* Read VQ for the given tid using ptrace. If SVE is not supported then zero
> + is returned (on a system that supports SVE, then VQ cannot be zeo). */
zeo -> zero.
> +extern unsigned long aarch64_sve_get_vq (int tid);
> +
> +/* Structures and defines taken from sigcontext.h. */
> +
> +#ifndef SVE_SIG_ZREGS_SIZE
> +
> +#define SVE_VQ_BYTES 16 /* number of bytes per quadword */
> +
> +#define SVE_VQ_MIN 1
> +#define SVE_VQ_MAX 512
> +
> +#define SVE_VL_MIN (SVE_VQ_MIN * SVE_VQ_BYTES)
> +#define SVE_VL_MAX (SVE_VQ_MAX * SVE_VQ_BYTES)
> +
> +#define SVE_NUM_ZREGS 32
> +#define SVE_NUM_PREGS 16
> +
> +#define sve_vl_valid(vl) \
> + ((vl) % SVE_VQ_BYTES == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
> +
> +#endif /* SVE_SIG_ZREGS_SIZE. */
> +
> +
> +/* Structures and defines taken from ptrace.h. */
> +
> +#ifndef SVE_PT_SVE_ZREG_SIZE
> +
> +struct user_sve_header {
> + __u32 size; /* total meaningful regset content in bytes */
> + __u32 max_size; /* maxmium possible size for this thread */
> + __u16 vl; /* current vector length */
> + __u16 max_vl; /* maximum possible vector length */
> + __u16 flags;
> + __u16 __reserved;
> +};
> +
> +#endif /* SVE_PT_SVE_ZREG_SIZE. */
> +
> +#endif /* aarch64-sve-linux-ptrace.h */
>
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 2/8] Function for reading the Aarch64 SVE vector length.
2018-05-31 12:06 ` Simon Marchi
@ 2018-05-31 14:18 ` Alan Hayward
0 siblings, 0 replies; 47+ messages in thread
From: Alan Hayward @ 2018-05-31 14:18 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches, nd
Pushed with changes as below.
> On 31 May 2018, at 12:44, Simon Marchi <simon.marchi@ericsson.com> wrote:
>
> Hi Alan,
>
> LGTM, with some nits.
>
> On 2018-05-11 06:52 AM, Alan Hayward wrote:
>> Add a method for reading the SVE vector length using ptrace. This returns
>> 0 for systems without SVE support.
>>
>> Note the defines taken from Linux kernel headers in aarch64-sve-linux-ptrace.h.
>> See the covering email for details about this.
>
> Using ptrace to read the SVE registers (and the SVE register length) will work even
> for kernels that didn't have these macros?
(Answering both possible ways I thought the question was asking).
The macros were added to the kernel in the same batch of patches that
added all the Linux sve support. So, the macros should always exist
on SVE enabled kernels.
The macros are only calculating offsets within the variable length
structure. There’s is nothing preventing you using them on (say)
x86 linux 3.0 to parse the contents of a copy of the structure.
It’s possible that they might end up getting used for reading sve
core files.
>
>> There are multiple ways of expressing the vector length. Thankfully these are
>> all wll defined. I've added convertors for going from one to the other.
>> Hopefully this will help to prevent future confusion.
> Copyright 2017 -> 2018 in the new files.
Ok. (I started these patches quite a while ago!)
>
>>
>> 2018-05-11 Alan Hayward <alan.hayward@arm.com>
>>
>> gdb/
>> * Makefile.in: Add new header.
>> * gdb/arch/aarch64.h (sve_vg_from_vl): New macro.
>> (sve_vl_from_vg): Likewise.
>> (sve_vq_from_vl): Likewise.
>> (sve_vl_from_vq): Likewise.
>> (sve_vq_from_vg): Likewise.
>> (sve_vg_from_vq): Likewise.
>> * configure.nat: Add new c file.
>> * nat/aarch64-sve-linux-ptrace.c: New file.
>> * nat/aarch64-sve-linux-ptrace.h: New file.
>>
>> gdbserver/
>> * configure.srv: Add new c/h file.
>> ---
>> gdb/Makefile.in | 1 +
>> gdb/arch/aarch64.h | 17 +++++++++
>> gdb/configure.nat | 2 +-
>> gdb/gdbserver/configure.srv | 1 +
>> gdb/nat/aarch64-sve-linux-ptrace.c | 52 +++++++++++++++++++++++++++
>> gdb/nat/aarch64-sve-linux-ptrace.h | 73 ++++++++++++++++++++++++++++++++++++++
>> 6 files changed, 145 insertions(+), 1 deletion(-)
>> create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.c
>> create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.h
>>
>> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
>> index 87d74a7703..64042d1bd1 100644
>> --- a/gdb/Makefile.in
>> +++ b/gdb/Makefile.in
>> @@ -1478,6 +1478,7 @@ HFILES_NO_SRCDIR = \
>> mi/mi-parse.h \
>> nat/aarch64-linux.h \
>> nat/aarch64-linux-hw-point.h \
>> + nat/aarch64-sve-linux-ptrace.h \
>> nat/amd64-linux-siginfo.h \
>> nat/gdb_ptrace.h \
>> nat/gdb_thread_db.h \
>> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
>> index 1846e04163..af0b157c51 100644
>> --- a/gdb/arch/aarch64.h
>> +++ b/gdb/arch/aarch64.h
>> @@ -48,6 +48,23 @@ enum aarch64_regnum
>> #define AARCH64_V_REGS_NUM 32
>> #define AARCH64_NUM_REGS AARCH64_FPCR_REGNUM + 1
>>
>> +/* There are a number of ways of expressing the current SVE vector size:
>> +
>> + VL : Vector Length.
>> + The number of bytes in an SVE Z register.
>> + VQ : Vector Quotient.
>> + The number of 128bit chunks in an SVE Z register.
>> + VG : Vector Gradient.
>> + The number of 64bit chunks in an SVE Z register. */
>> +
>> +#define sve_vg_from_vl(vl) ((vl) / 8)
>> +#define sve_vl_from_vg(vg) ((vg) * 8)
>> +#define sve_vq_from_vl(vl) ((vl) / 0x10)
>> +#define sve_vl_from_vq(vq) ((vq) * 0x10)
>> +#define sve_vq_from_vg(vg) (sve_vq_from_vl (sve_vl_from_vg (vg)))
>> +#define sve_vg_from_vq(vq) (sve_vg_from_vl (sve_vl_from_vq (vq)))
>> +
>> +
>> /* Maximum supported VQ value. Increase if required. */
>> #define AARCH64_MAX_SVE_VQ 16
>>
>> diff --git a/gdb/configure.nat b/gdb/configure.nat
>> index 6b0f44fede..d7d79adaca 100644
>> --- a/gdb/configure.nat
>> +++ b/gdb/configure.nat
>> @@ -228,7 +228,7 @@ case ${gdb_host} in
>> aarch64)
>> # Host: AArch64 based machine running GNU/Linux
>> NATDEPFILES="${NATDEPFILES} aarch64-linux-nat.o \
>> - aarch32-linux-nat.o aarch64-linux-hw-point.o aarch64-linux.o"
>> + aarch32-linux-nat.o aarch64-linux-hw-point.o aarch64-linux.o aarch64-sve-linux-ptrace.o"
>
> Please wrap to 80 columns.
Ok.
>
>> ;;
>> arm)
>> # Host: ARM based machine running GNU/Linux
>> diff --git a/gdb/gdbserver/configure.srv b/gdb/gdbserver/configure.srv
>> index ffeefb9b92..8bf0dcc650 100644
>> --- a/gdb/gdbserver/configure.srv
>> +++ b/gdb/gdbserver/configure.srv
>> @@ -54,6 +54,7 @@ case "${target}" in
>> srv_tgtobj="$srv_tgtobj arch/aarch64-insn.o"
>> srv_tgtobj="$srv_tgtobj arch/aarch64.o"
>> srv_tgtobj="$srv_tgtobj linux-aarch64-tdesc.o"
>> + srv_tgtobj="$srv_tgtobj aarch64-sve-linux-ptrace.o"
>> srv_tgtobj="${srv_tgtobj} $srv_linux_obj"
>> srv_linux_regsets=yes
>> srv_linux_thread_db=yes
>> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c
>> new file mode 100644
>> index 0000000000..9381786fda
>> --- /dev/null
>> +++ b/gdb/nat/aarch64-sve-linux-ptrace.c
>> @@ -0,0 +1,52 @@
>> +/* Common target dependent for AArch64 systems.
>> +
>> + Copyright (C) 2017 Free Software Foundation, Inc.
>> +
>> + This file is part of GDB.
>> +
>> + This program is free software; you can redistribute it and/or modify
>> + it under the terms of the GNU General Public License as published by
>> + the Free Software Foundation; either version 3 of the License, or
>> + (at your option) any later version.
>> +
>> + This program is distributed in the hope that it will be useful,
>> + but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + GNU General Public License for more details.
>> +
>> + You should have received a copy of the GNU General Public License
>> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
>> +
>> +#include <sys/utsname.h>
>> +#include <sys/uio.h>
>> +#include "common-defs.h"
>> +#include "elf/external.h"
>> +#include "elf/common.h"
>> +#include "aarch64-sve-linux-ptrace.h"
>> +#include "arch/aarch64.h"
>> +
>> +/* Read VQ for the given tid using ptrace. If SVE is not supported then zero
>> + is returned (on a system that supports SVE, then VQ cannot be zeo). */
>
> Here, put a reference to the .h as usual.
Ok.
>
>> +
>> +unsigned long
>> +aarch64_sve_get_vq (int tid)
>> +{
>> + struct iovec iovec;
>> + struct user_sve_header header;
>> +
>> + iovec.iov_len = sizeof (header);
>> + iovec.iov_base = &header;
>> +
>> + /* Ptrace gives the vector length in bytes. Convert it to VQ, the number of
>> + 128bit chunks in a Z register. We use VQ because 128bits is the minimum
>> + a Z register can increase in size. */
>> +
>> + if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec) < 0)
>> + /* SVE is not supported. */
>> + return 0;
>
> Add braces here.
Ok.
>
>> +
>> + long vq = sve_vq_from_vl (header.vl);
>> + gdb_assert (sve_vl_valid (header.vl));
>
> We should avoid gdb_assert for bad input values (including what we receive from the
> kernel). Could we display a warning and return 0?
Changed to:
+ if (!sve_vl_valid (header.vl))
+ {
+ warning (_("Invalid SVE state from kernel; SVE disabled."));
+ return 0;
+ }
>
>> +
>> + return vq;
>> +}
>> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.h b/gdb/nat/aarch64-sve-linux-ptrace.h
>> new file mode 100644
>> index 0000000000..b318150ac1
>> --- /dev/null
>> +++ b/gdb/nat/aarch64-sve-linux-ptrace.h
>> @@ -0,0 +1,73 @@
>> +/* Common target dependent for AArch64 systems.
>> +
>> + Copyright (C) 2017 Free Software Foundation, Inc.
>> +
>> + This file is part of GDB.
>> +
>> + This program is free software; you can redistribute it and/or modify
>> + it under the terms of the GNU General Public License as published by
>> + the Free Software Foundation; either version 3 of the License, or
>> + (at your option) any later version.
>> +
>> + This program is distributed in the hope that it will be useful,
>> + but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + GNU General Public License for more details.
>> +
>> + You should have received a copy of the GNU General Public License
>> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
>> +
>> +#ifndef AARCH64_SVE_LINUX_PTRACE_H
>> +#define AARCH64_SVE_LINUX_PTRACE_H
>> +
>> +/* Where indicated, this file contains defines and macros lifted directly from
>> + the Linux kernel headers, with no modification.
>> + Refer to Linux kernel documentation for details. */
>> +
>> +#include <asm/sigcontext.h>
>> +#include <sys/utsname.h>
>> +#include <sys/ptrace.h>
>> +#include <asm/ptrace.h>
>> +
>> +/* Read VQ for the given tid using ptrace. If SVE is not supported then zero
>> + is returned (on a system that supports SVE, then VQ cannot be zeo). */
>
> zeo -> zero.
Ok.
>
>> +extern unsigned long aarch64_sve_get_vq (int tid);
>> +
>> +/* Structures and defines taken from sigcontext.h. */
>> +
>> +#ifndef SVE_SIG_ZREGS_SIZE
>> +
>> +#define SVE_VQ_BYTES 16 /* number of bytes per quadword */
>> +
>> +#define SVE_VQ_MIN 1
>> +#define SVE_VQ_MAX 512
>> +
>> +#define SVE_VL_MIN (SVE_VQ_MIN * SVE_VQ_BYTES)
>> +#define SVE_VL_MAX (SVE_VQ_MAX * SVE_VQ_BYTES)
>> +
>> +#define SVE_NUM_ZREGS 32
>> +#define SVE_NUM_PREGS 16
>> +
>> +#define sve_vl_valid(vl) \
>> + ((vl) % SVE_VQ_BYTES == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
>> +
>> +#endif /* SVE_SIG_ZREGS_SIZE. */
>> +
>> +
>> +/* Structures and defines taken from ptrace.h. */
>> +
>> +#ifndef SVE_PT_SVE_ZREG_SIZE
>> +
>> +struct user_sve_header {
>> + __u32 size; /* total meaningful regset content in bytes */
>> + __u32 max_size; /* maxmium possible size for this thread */
>> + __u16 vl; /* current vector length */
>> + __u16 max_vl; /* maximum possible vector length */
>> + __u16 flags;
>> + __u16 __reserved;
>> +};
>> +
>> +#endif /* SVE_PT_SVE_ZREG_SIZE. */
>> +
>> +#endif /* aarch64-sve-linux-ptrace.h */
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 2/8] Function for reading the Aarch64 SVE vector length.
2018-05-11 10:53 ` [PATCH 2/8] Function for reading the Aarch64 SVE vector length Alan Hayward
2018-05-31 12:06 ` Simon Marchi
@ 2018-05-31 14:57 ` Pedro Alves
2018-06-05 20:01 ` Sergio Durigan Junior
2 siblings, 0 replies; 47+ messages in thread
From: Pedro Alves @ 2018-05-31 14:57 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
On 05/11/2018 11:52 AM, Alan Hayward wrote:
> Add a method for reading the SVE vector length using ptrace. This returns
> 0 for systems without SVE support.
>
> Note the defines taken from Linux kernel headers in aarch64-sve-linux-ptrace.h.
> See the covering email for details about this.
Note that since the cover email doesn't make it to the git repo, it's better to
do things the other way around, leave the details in the commit, and a
summary in the cover letter. Someone looking at git log won't have a reference
to the cover letter. Or, if it makes sense for readers of the code, as
opposed to just a rationale for a change, put it in comments instead. (I don't
know whether that's true, just stating a principle.)
>
> There are multiple ways of expressing the vector length. Thankfully these are
> all wll defined. I've added convertors for going from one to the other.
"well", "converters".
> +struct user_sve_header {
> + __u32 size; /* total meaningful regset content in bytes */
> + __u32 max_size; /* maxmium possible size for this thread */
"maximum"
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 2/8] Function for reading the Aarch64 SVE vector length.
2018-05-11 10:53 ` [PATCH 2/8] Function for reading the Aarch64 SVE vector length Alan Hayward
2018-05-31 12:06 ` Simon Marchi
2018-05-31 14:57 ` Pedro Alves
@ 2018-06-05 20:01 ` Sergio Durigan Junior
2018-06-05 22:06 ` [PATCH] Guard declarations of 'sve_*_from_*' macros on Aarch64 (and unbreak build) Sergio Durigan Junior
2 siblings, 1 reply; 47+ messages in thread
From: Sergio Durigan Junior @ 2018-06-05 20:01 UTC (permalink / raw)
To: Alan Hayward; +Cc: gdb-patches, nd
On Friday, May 11 2018, Alan Hayward wrote:
> Add a method for reading the SVE vector length using ptrace. This returns
> 0 for systems without SVE support.
>
> Note the defines taken from Linux kernel headers in aarch64-sve-linux-ptrace.h.
> See the covering email for details about this.
>
> There are multiple ways of expressing the vector length. Thankfully these are
> all wll defined. I've added convertors for going from one to the other.
> Hopefully this will help to prevent future confusion.
Hi Alan,
I'm trying to build GDB on Fedora Rawhide Aarch64, and I'm seeing the
following breakage:
CXX aarch64-linux-nat.o
In file included from ../../gdb/aarch64-tdep.h:25,
from ../../gdb/aarch64-linux-nat.c:30:
../../gdb/arch/aarch64.h:79: error: "sve_vq_from_vl" redefined [-Werror]
#define sve_vq_from_vl(vl) ((vl) / 0x10)
In file included from /usr/include/bits/sigcontext.h:30,
from /usr/include/signal.h:291,
from build-gnulib/import/signal.h:52,
from ../../gdb/linux-nat.h:23,
from ../../gdb/aarch64-linux-nat.c:26:
/usr/include/asm/sigcontext.h:154: note: this is the location of the previous definition
#define sve_vq_from_vl(vl) ((vl) / SVE_VQ_BYTES)
In file included from ../../gdb/aarch64-tdep.h:25,
from ../../gdb/aarch64-linux-nat.c:30:
../../gdb/arch/aarch64.h:80: error: "sve_vl_from_vq" redefined [-Werror]
#define sve_vl_from_vq(vq) ((vq) * 0x10)
In file included from /usr/include/bits/sigcontext.h:30,
from /usr/include/signal.h:291,
from build-gnulib/import/signal.h:52,
from ../../gdb/linux-nat.h:23,
from ../../gdb/aarch64-linux-nat.c:26:
/usr/include/asm/sigcontext.h:155: note: this is the location of the previous definition
#define sve_vl_from_vq(vq) ((vq) * SVE_VQ_BYTES)
CXX aarch32-linux-nat.o
CXX aarch64-linux-hw-point.o
CXX aarch64-linux.o
CXX aarch64-sve-linux-ptrace.o
CXX cli/cli-cmds.o
In file included from ../../gdb/nat/aarch64-sve-linux-ptrace.c:26:
../../gdb/arch/aarch64.h:79: error: "sve_vq_from_vl" redefined [-Werror]
#define sve_vq_from_vl(vl) ((vl) / 0x10)
In file included from ../../gdb/nat/aarch64-sve-linux-ptrace.h:27,
from ../../gdb/nat/aarch64-sve-linux-ptrace.c:25:
/usr/include/asm/sigcontext.h:154: note: this is the location of the previous definition
#define sve_vq_from_vl(vl) ((vl) / SVE_VQ_BYTES)
In file included from ../../gdb/nat/aarch64-sve-linux-ptrace.c:26:
../../gdb/arch/aarch64.h:80: error: "sve_vl_from_vq" redefined [-Werror]
#define sve_vl_from_vq(vq) ((vq) * 0x10)
In file included from ../../gdb/nat/aarch64-sve-linux-ptrace.h:27,
from ../../gdb/nat/aarch64-sve-linux-ptrace.c:25:
/usr/include/asm/sigcontext.h:155: note: this is the location of the previous definition
#define sve_vl_from_vq(vq) ((vq) * SVE_VQ_BYTES)
I think it may be a good idea to guard these #define's with #ifndef's.
I'm trying to get ahold of a machine to test this (the build failure
happened on a machine which I don't have direct control over).
Thanks,
> 2018-05-11 Alan Hayward <alan.hayward@arm.com>
>
> gdb/
> * Makefile.in: Add new header.
> * gdb/arch/aarch64.h (sve_vg_from_vl): New macro.
> (sve_vl_from_vg): Likewise.
> (sve_vq_from_vl): Likewise.
> (sve_vl_from_vq): Likewise.
> (sve_vq_from_vg): Likewise.
> (sve_vg_from_vq): Likewise.
> * configure.nat: Add new c file.
> * nat/aarch64-sve-linux-ptrace.c: New file.
> * nat/aarch64-sve-linux-ptrace.h: New file.
>
> gdbserver/
> * configure.srv: Add new c/h file.
> ---
> gdb/Makefile.in | 1 +
> gdb/arch/aarch64.h | 17 +++++++++
> gdb/configure.nat | 2 +-
> gdb/gdbserver/configure.srv | 1 +
> gdb/nat/aarch64-sve-linux-ptrace.c | 52 +++++++++++++++++++++++++++
> gdb/nat/aarch64-sve-linux-ptrace.h | 73 ++++++++++++++++++++++++++++++++++++++
> 6 files changed, 145 insertions(+), 1 deletion(-)
> create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.c
> create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.h
>
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 87d74a7703..64042d1bd1 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -1478,6 +1478,7 @@ HFILES_NO_SRCDIR = \
> mi/mi-parse.h \
> nat/aarch64-linux.h \
> nat/aarch64-linux-hw-point.h \
> + nat/aarch64-sve-linux-ptrace.h \
> nat/amd64-linux-siginfo.h \
> nat/gdb_ptrace.h \
> nat/gdb_thread_db.h \
> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
> index 1846e04163..af0b157c51 100644
> --- a/gdb/arch/aarch64.h
> +++ b/gdb/arch/aarch64.h
> @@ -48,6 +48,23 @@ enum aarch64_regnum
> #define AARCH64_V_REGS_NUM 32
> #define AARCH64_NUM_REGS AARCH64_FPCR_REGNUM + 1
>
> +/* There are a number of ways of expressing the current SVE vector size:
> +
> + VL : Vector Length.
> + The number of bytes in an SVE Z register.
> + VQ : Vector Quotient.
> + The number of 128bit chunks in an SVE Z register.
> + VG : Vector Gradient.
> + The number of 64bit chunks in an SVE Z register. */
> +
> +#define sve_vg_from_vl(vl) ((vl) / 8)
> +#define sve_vl_from_vg(vg) ((vg) * 8)
> +#define sve_vq_from_vl(vl) ((vl) / 0x10)
> +#define sve_vl_from_vq(vq) ((vq) * 0x10)
> +#define sve_vq_from_vg(vg) (sve_vq_from_vl (sve_vl_from_vg (vg)))
> +#define sve_vg_from_vq(vq) (sve_vg_from_vl (sve_vl_from_vq (vq)))
> +
> +
> /* Maximum supported VQ value. Increase if required. */
> #define AARCH64_MAX_SVE_VQ 16
>
> diff --git a/gdb/configure.nat b/gdb/configure.nat
> index 6b0f44fede..d7d79adaca 100644
> --- a/gdb/configure.nat
> +++ b/gdb/configure.nat
> @@ -228,7 +228,7 @@ case ${gdb_host} in
> aarch64)
> # Host: AArch64 based machine running GNU/Linux
> NATDEPFILES="${NATDEPFILES} aarch64-linux-nat.o \
> - aarch32-linux-nat.o aarch64-linux-hw-point.o aarch64-linux.o"
> + aarch32-linux-nat.o aarch64-linux-hw-point.o aarch64-linux.o aarch64-sve-linux-ptrace.o"
> ;;
> arm)
> # Host: ARM based machine running GNU/Linux
> diff --git a/gdb/gdbserver/configure.srv b/gdb/gdbserver/configure.srv
> index ffeefb9b92..8bf0dcc650 100644
> --- a/gdb/gdbserver/configure.srv
> +++ b/gdb/gdbserver/configure.srv
> @@ -54,6 +54,7 @@ case "${target}" in
> srv_tgtobj="$srv_tgtobj arch/aarch64-insn.o"
> srv_tgtobj="$srv_tgtobj arch/aarch64.o"
> srv_tgtobj="$srv_tgtobj linux-aarch64-tdesc.o"
> + srv_tgtobj="$srv_tgtobj aarch64-sve-linux-ptrace.o"
> srv_tgtobj="${srv_tgtobj} $srv_linux_obj"
> srv_linux_regsets=yes
> srv_linux_thread_db=yes
> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c
> new file mode 100644
> index 0000000000..9381786fda
> --- /dev/null
> +++ b/gdb/nat/aarch64-sve-linux-ptrace.c
> @@ -0,0 +1,52 @@
> +/* Common target dependent for AArch64 systems.
> +
> + Copyright (C) 2017 Free Software Foundation, Inc.
> +
> + This file is part of GDB.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +#include <sys/utsname.h>
> +#include <sys/uio.h>
> +#include "common-defs.h"
> +#include "elf/external.h"
> +#include "elf/common.h"
> +#include "aarch64-sve-linux-ptrace.h"
> +#include "arch/aarch64.h"
> +
> +/* Read VQ for the given tid using ptrace. If SVE is not supported then zero
> + is returned (on a system that supports SVE, then VQ cannot be zeo). */
> +
> +unsigned long
> +aarch64_sve_get_vq (int tid)
> +{
> + struct iovec iovec;
> + struct user_sve_header header;
> +
> + iovec.iov_len = sizeof (header);
> + iovec.iov_base = &header;
> +
> + /* Ptrace gives the vector length in bytes. Convert it to VQ, the number of
> + 128bit chunks in a Z register. We use VQ because 128bits is the minimum
> + a Z register can increase in size. */
> +
> + if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec) < 0)
> + /* SVE is not supported. */
> + return 0;
> +
> + long vq = sve_vq_from_vl (header.vl);
> + gdb_assert (sve_vl_valid (header.vl));
> +
> + return vq;
> +}
> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.h b/gdb/nat/aarch64-sve-linux-ptrace.h
> new file mode 100644
> index 0000000000..b318150ac1
> --- /dev/null
> +++ b/gdb/nat/aarch64-sve-linux-ptrace.h
> @@ -0,0 +1,73 @@
> +/* Common target dependent for AArch64 systems.
> +
> + Copyright (C) 2017 Free Software Foundation, Inc.
> +
> + This file is part of GDB.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +#ifndef AARCH64_SVE_LINUX_PTRACE_H
> +#define AARCH64_SVE_LINUX_PTRACE_H
> +
> +/* Where indicated, this file contains defines and macros lifted directly from
> + the Linux kernel headers, with no modification.
> + Refer to Linux kernel documentation for details. */
> +
> +#include <asm/sigcontext.h>
> +#include <sys/utsname.h>
> +#include <sys/ptrace.h>
> +#include <asm/ptrace.h>
> +
> +/* Read VQ for the given tid using ptrace. If SVE is not supported then zero
> + is returned (on a system that supports SVE, then VQ cannot be zeo). */
> +
> +extern unsigned long aarch64_sve_get_vq (int tid);
> +
> +/* Structures and defines taken from sigcontext.h. */
> +
> +#ifndef SVE_SIG_ZREGS_SIZE
> +
> +#define SVE_VQ_BYTES 16 /* number of bytes per quadword */
> +
> +#define SVE_VQ_MIN 1
> +#define SVE_VQ_MAX 512
> +
> +#define SVE_VL_MIN (SVE_VQ_MIN * SVE_VQ_BYTES)
> +#define SVE_VL_MAX (SVE_VQ_MAX * SVE_VQ_BYTES)
> +
> +#define SVE_NUM_ZREGS 32
> +#define SVE_NUM_PREGS 16
> +
> +#define sve_vl_valid(vl) \
> + ((vl) % SVE_VQ_BYTES == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
> +
> +#endif /* SVE_SIG_ZREGS_SIZE. */
> +
> +
> +/* Structures and defines taken from ptrace.h. */
> +
> +#ifndef SVE_PT_SVE_ZREG_SIZE
> +
> +struct user_sve_header {
> + __u32 size; /* total meaningful regset content in bytes */
> + __u32 max_size; /* maxmium possible size for this thread */
> + __u16 vl; /* current vector length */
> + __u16 max_vl; /* maximum possible vector length */
> + __u16 flags;
> + __u16 __reserved;
> +};
> +
> +#endif /* SVE_PT_SVE_ZREG_SIZE. */
> +
> +#endif /* aarch64-sve-linux-ptrace.h */
> --
> 2.15.1 (Apple Git-101)
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
^ permalink raw reply [flat|nested] 47+ messages in thread* [PATCH] Guard declarations of 'sve_*_from_*' macros on Aarch64 (and unbreak build)
2018-06-05 20:01 ` Sergio Durigan Junior
@ 2018-06-05 22:06 ` Sergio Durigan Junior
2018-06-05 23:37 ` Sergio Durigan Junior
2018-06-06 7:34 ` Alan Hayward
0 siblings, 2 replies; 47+ messages in thread
From: Sergio Durigan Junior @ 2018-06-05 22:06 UTC (permalink / raw)
To: GDB Patches; +Cc: Alan Hayward, nd, Sergio Durigan Junior
Commit 122394f1476b1c925a281b15399119500c8231c1 ("Function for reading
the Aarch64 SVE vector length") has added macros to manipulate SVE
vector sizes based on Linux kernel sources, but did not guard them
with #ifndef's, which breaks the build when the system headers already
have these macros:
CXX aarch64-linux-nat.o
In file included from ../../gdb/aarch64-tdep.h:25,
from ../../gdb/aarch64-linux-nat.c:30:
../../gdb/arch/aarch64.h:79: error: "sve_vq_from_vl" redefined [-Werror]
#define sve_vq_from_vl(vl) ((vl) / 0x10)
In file included from /usr/include/bits/sigcontext.h:30,
from /usr/include/signal.h:291,
from build-gnulib/import/signal.h:52,
from ../../gdb/linux-nat.h:23,
from ../../gdb/aarch64-linux-nat.c:26:
/usr/include/asm/sigcontext.h:154: note: this is the location of the previous definition
#define sve_vq_from_vl(vl) ((vl) / SVE_VQ_BYTES)
In file included from ../../gdb/aarch64-tdep.h:25,
from ../../gdb/aarch64-linux-nat.c:30:
../../gdb/arch/aarch64.h:80: error: "sve_vl_from_vq" redefined [-Werror]
#define sve_vl_from_vq(vq) ((vq) * 0x10)
In file included from /usr/include/bits/sigcontext.h:30,
from /usr/include/signal.h:291,
from build-gnulib/import/signal.h:52,
from ../../gdb/linux-nat.h:23,
from ../../gdb/aarch64-linux-nat.c:26:
/usr/include/asm/sigcontext.h:155: note: this is the location of the previous definition
#define sve_vl_from_vq(vq) ((vq) * SVE_VQ_BYTES)
In order to fix this breakage, this commit guards the declaration of
the macros using #ifndef's. It is important to mention that the
system headers use a value to multiply/divide the vector
length (SVE_VQ_BYTES, defined as 16 on the system I'm using) which is
different than the values being used by the macros defined in GDB. I
wasn't sure how to consolidate that, so I chose to ignore this
discrepancy.
Tested by making sure GDB compiles fine again. Since the system I'm
using doesn't have support for SVE, there's no way I can really test
these changes.
gdb/ChangeLog:
2018-06-05 Sergio Durigan Junior <sergiodj@redhat.com>
* arch/aarch64.h (sve_vg_from_vl): Guard with #ifndef.
(sve_vl_from_vg): Likewise.
(sve_vq_from_vl): Likewise.
(sve_vl_from_vq): Likewise.
(sve_vq_from_vg): Likewise.
(sve_vg_from_vq): Likewise.
---
gdb/arch/aarch64.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
index 9040d8d4c8..c378a4b239 100644
--- a/gdb/arch/aarch64.h
+++ b/gdb/arch/aarch64.h
@@ -74,12 +74,24 @@ enum aarch64_regnum
VG : Vector Gradient.
The number of 64bit chunks in an SVE Z register. */
+#ifndef sve_vg_from_vl
#define sve_vg_from_vl(vl) ((vl) / 8)
+#endif
+#ifndef sve_vl_from_vg
#define sve_vl_from_vg(vg) ((vg) * 8)
+#endif
+#ifndef sve_vq_from_vl
#define sve_vq_from_vl(vl) ((vl) / 0x10)
+#endif
+#ifndef sve_vl_from_vq
#define sve_vl_from_vq(vq) ((vq) * 0x10)
+#endif
+#ifndef sve_vq_from_vg
#define sve_vq_from_vg(vg) (sve_vq_from_vl (sve_vl_from_vg (vg)))
+#endif
+#ifndef sve_vg_from_vq
#define sve_vg_from_vq(vq) (sve_vg_from_vl (sve_vl_from_vq (vq)))
+#endif
/* Maximum supported VQ value. Increase if required. */
--
2.14.3
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH] Guard declarations of 'sve_*_from_*' macros on Aarch64 (and unbreak build)
2018-06-05 22:06 ` [PATCH] Guard declarations of 'sve_*_from_*' macros on Aarch64 (and unbreak build) Sergio Durigan Junior
@ 2018-06-05 23:37 ` Sergio Durigan Junior
2018-06-06 7:34 ` Alan Hayward
1 sibling, 0 replies; 47+ messages in thread
From: Sergio Durigan Junior @ 2018-06-05 23:37 UTC (permalink / raw)
To: GDB Patches; +Cc: Alan Hayward, nd
On Tuesday, June 05 2018, I wrote:
> In order to fix this breakage, this commit guards the declaration of
> the macros using #ifndef's. It is important to mention that the
> system headers use a value to multiply/divide the vector
> length (SVE_VQ_BYTES, defined as 16 on the system I'm using) which is
> different than the values being used by the macros defined in GDB. I
> wasn't sure how to consolidate that, so I chose to ignore this
> discrepancy.
Actually, I just saw that there's no discrepancy. The system header
defines the following macros:
#define SVE_VQ_BYTES 16 /* number of bytes per quadword */
...
#define sve_vq_from_vl(vl) ((vl) / SVE_VQ_BYTES)
#define sve_vl_from_vq(vq) ((vq) * SVE_VQ_BYTES)
and GDB does this:
#ifndef sve_vq_from_vl
#define sve_vq_from_vl(vl) ((vl) / 0x10)
#endif
#ifndef sve_vl_from_vq
#define sve_vl_from_vq(vq) ((vq) * 0x10)
#endif
which is fine. I'll remove this comment from the commit log.
Thanks,
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH] Guard declarations of 'sve_*_from_*' macros on Aarch64 (and unbreak build)
2018-06-05 22:06 ` [PATCH] Guard declarations of 'sve_*_from_*' macros on Aarch64 (and unbreak build) Sergio Durigan Junior
2018-06-05 23:37 ` Sergio Durigan Junior
@ 2018-06-06 7:34 ` Alan Hayward
2018-06-06 21:19 ` Simon Marchi
2018-06-06 21:36 ` Sergio Durigan Junior
1 sibling, 2 replies; 47+ messages in thread
From: Alan Hayward @ 2018-06-06 7:34 UTC (permalink / raw)
To: Sergio Durigan Junior; +Cc: GDB Patches, nd
> On 5 Jun 2018, at 23:05, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>
> Commit 122394f1476b1c925a281b15399119500c8231c1 ("Function for reading
> the Aarch64 SVE vector length") has added macros to manipulate SVE
> vector sizes based on Linux kernel sources, but did not guard them
> with #ifndef's, which breaks the build when the system headers already
> have these macros:
>
> CXX aarch64-linux-nat.o
> In file included from ../../gdb/aarch64-tdep.h:25,
> from ../../gdb/aarch64-linux-nat.c:30:
> ../../gdb/arch/aarch64.h:79: error: "sve_vq_from_vl" redefined [-Werror]
> #define sve_vq_from_vl(vl) ((vl) / 0x10)
>
> In file included from /usr/include/bits/sigcontext.h:30,
> from /usr/include/signal.h:291,
> from build-gnulib/import/signal.h:52,
> from ../../gdb/linux-nat.h:23,
> from ../../gdb/aarch64-linux-nat.c:26:
> /usr/include/asm/sigcontext.h:154: note: this is the location of the previous definition
> #define sve_vq_from_vl(vl) ((vl) / SVE_VQ_BYTES)
>
> In file included from ../../gdb/aarch64-tdep.h:25,
> from ../../gdb/aarch64-linux-nat.c:30:
> ../../gdb/arch/aarch64.h:80: error: "sve_vl_from_vq" redefined [-Werror]
> #define sve_vl_from_vq(vq) ((vq) * 0x10)
>
> In file included from /usr/include/bits/sigcontext.h:30,
> from /usr/include/signal.h:291,
> from build-gnulib/import/signal.h:52,
> from ../../gdb/linux-nat.h:23,
> from ../../gdb/aarch64-linux-nat.c:26:
> /usr/include/asm/sigcontext.h:155: note: this is the location of the previous definition
> #define sve_vl_from_vq(vq) ((vq) * SVE_VQ_BYTES)
>
> In order to fix this breakage, this commit guards the declaration of
> the macros using #ifndef’s.
Apologies for breaking this.
I originally had them in nat/aarch64-sve-linux-ptrace.h, within the
SVE_SIG_ZREGS_SIZE block, which does guard appropriately.
Thanks for fixing so quickly.
> It is important to mention that the
> system headers use a value to multiply/divide the vector
> length (SVE_VQ_BYTES, defined as 16 on the system I'm using) which is
> different than the values being used by the macros defined in GDB. I
> wasn't sure how to consolidate that, so I chose to ignore this
> discrepancy.
>
Yes, (as you pointed out in the next email), they compile down to the same.
When I moved them I changed to explicit values to remove the dependency.
> Tested by making sure GDB compiles fine again. Since the system I'm
> using doesn't have support for SVE, there's no way I can really test
> these changes.
>
Changes work fine for me on my SVE builds.
> gdb/ChangeLog:
> 2018-06-05 Sergio Durigan Junior <sergiodj@redhat.com>
>
> * arch/aarch64.h (sve_vg_from_vl): Guard with #ifndef.
> (sve_vl_from_vg): Likewise.
> (sve_vq_from_vl): Likewise.
> (sve_vl_from_vq): Likewise.
> (sve_vq_from_vg): Likewise.
> (sve_vg_from_vq): Likewise.
> ---
> gdb/arch/aarch64.h | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
> index 9040d8d4c8..c378a4b239 100644
> --- a/gdb/arch/aarch64.h
> +++ b/gdb/arch/aarch64.h
> @@ -74,12 +74,24 @@ enum aarch64_regnum
> VG : Vector Gradient.
> The number of 64bit chunks in an SVE Z register. */
>
> +#ifndef sve_vg_from_vl
> #define sve_vg_from_vl(vl) ((vl) / 8)
> +#endif
> +#ifndef sve_vl_from_vg
> #define sve_vl_from_vg(vg) ((vg) * 8)
> +#endif
The guards around these first two aren’t needed. The kernel only
defines the VQ to/from VL macros - as those are the only values the
kernel cares about. Only GDB cares about VG because that is needed
for dwarf.
> +#ifndef sve_vq_from_vl
> #define sve_vq_from_vl(vl) ((vl) / 0x10)
> +#endif
> +#ifndef sve_vl_from_vq
> #define sve_vl_from_vq(vq) ((vq) * 0x10)
> +#endif
These two are fine!
> +#ifndef sve_vq_from_vg
> #define sve_vq_from_vg(vg) (sve_vq_from_vl (sve_vl_from_vg (vg)))
> +#endif
> +#ifndef sve_vg_from_vq
> #define sve_vg_from_vq(vq) (sve_vg_from_vl (sve_vl_from_vq (vq)))
> +#endif
Again these last two aren’t needed.
FYI, either today or tomorrow I’ll be posting a new set of SVE patches.
As part of that series, due to review comments, I’ll be moving the all
the linux kernel defines to two new files which contain only kernel
defines (From ptrace.h and sigcontext.h). I’ll double check this works
with new header files. Regardless of that, I think your patch should
still go in to unbreak the build until then.
Changes are good to me if the VG guards are removed (but I’m not a
maintainer so cannot officially approve it).
Alan.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH] Guard declarations of 'sve_*_from_*' macros on Aarch64 (and unbreak build)
2018-06-06 7:34 ` Alan Hayward
@ 2018-06-06 21:19 ` Simon Marchi
2018-06-06 21:36 ` Sergio Durigan Junior
1 sibling, 0 replies; 47+ messages in thread
From: Simon Marchi @ 2018-06-06 21:19 UTC (permalink / raw)
To: Alan Hayward, Sergio Durigan Junior; +Cc: GDB Patches, nd
On 2018-06-06 03:34 AM, Alan Hayward wrote:
> FYI, either today or tomorrow I’ll be posting a new set of SVE patches.
> As part of that series, due to review comments, I’ll be moving the all
> the linux kernel defines to two new files which contain only kernel
> defines (From ptrace.h and sigcontext.h). I’ll double check this works
> with new header files. Regardless of that, I think your patch should
> still go in to unbreak the build until then.
>
>
> Changes are good to me if the VG guards are removed (but I’m not a
> maintainer so cannot officially approve it).
LGTM with Alan's proposed changes.
Simon
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH] Guard declarations of 'sve_*_from_*' macros on Aarch64 (and unbreak build)
2018-06-06 7:34 ` Alan Hayward
2018-06-06 21:19 ` Simon Marchi
@ 2018-06-06 21:36 ` Sergio Durigan Junior
1 sibling, 0 replies; 47+ messages in thread
From: Sergio Durigan Junior @ 2018-06-06 21:36 UTC (permalink / raw)
To: Alan Hayward; +Cc: GDB Patches, nd, Simon Marchi
On Wednesday, June 06 2018, Alan Hayward wrote:
>> On 5 Jun 2018, at 23:05, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>>
>> Commit 122394f1476b1c925a281b15399119500c8231c1 ("Function for reading
>> the Aarch64 SVE vector length") has added macros to manipulate SVE
>> vector sizes based on Linux kernel sources, but did not guard them
>> with #ifndef's, which breaks the build when the system headers already
>> have these macros:
>>
>> CXX aarch64-linux-nat.o
>> In file included from ../../gdb/aarch64-tdep.h:25,
>> from ../../gdb/aarch64-linux-nat.c:30:
>> ../../gdb/arch/aarch64.h:79: error: "sve_vq_from_vl" redefined [-Werror]
>> #define sve_vq_from_vl(vl) ((vl) / 0x10)
>>
>> In file included from /usr/include/bits/sigcontext.h:30,
>> from /usr/include/signal.h:291,
>> from build-gnulib/import/signal.h:52,
>> from ../../gdb/linux-nat.h:23,
>> from ../../gdb/aarch64-linux-nat.c:26:
>> /usr/include/asm/sigcontext.h:154: note: this is the location of the previous definition
>> #define sve_vq_from_vl(vl) ((vl) / SVE_VQ_BYTES)
>>
>> In file included from ../../gdb/aarch64-tdep.h:25,
>> from ../../gdb/aarch64-linux-nat.c:30:
>> ../../gdb/arch/aarch64.h:80: error: "sve_vl_from_vq" redefined [-Werror]
>> #define sve_vl_from_vq(vq) ((vq) * 0x10)
>>
>> In file included from /usr/include/bits/sigcontext.h:30,
>> from /usr/include/signal.h:291,
>> from build-gnulib/import/signal.h:52,
>> from ../../gdb/linux-nat.h:23,
>> from ../../gdb/aarch64-linux-nat.c:26:
>> /usr/include/asm/sigcontext.h:155: note: this is the location of the previous definition
>> #define sve_vl_from_vq(vq) ((vq) * SVE_VQ_BYTES)
>>
>> In order to fix this breakage, this commit guards the declaration of
>> the macros using #ifndef’s.
>
> Apologies for breaking this.
> I originally had them in nat/aarch64-sve-linux-ptrace.h, within the
> SVE_SIG_ZREGS_SIZE block, which does guard appropriately.
>
> Thanks for fixing so quickly.
No problem.
>> Tested by making sure GDB compiles fine again. Since the system I'm
>> using doesn't have support for SVE, there's no way I can really test
>> these changes.
>>
>
> Changes work fine for me on my SVE builds.
Thanks for testing!
>> gdb/ChangeLog:
>> 2018-06-05 Sergio Durigan Junior <sergiodj@redhat.com>
>>
>> * arch/aarch64.h (sve_vg_from_vl): Guard with #ifndef.
>> (sve_vl_from_vg): Likewise.
>> (sve_vq_from_vl): Likewise.
>> (sve_vl_from_vq): Likewise.
>> (sve_vq_from_vg): Likewise.
>> (sve_vg_from_vq): Likewise.
>> ---
>> gdb/arch/aarch64.h | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
>> index 9040d8d4c8..c378a4b239 100644
>> --- a/gdb/arch/aarch64.h
>> +++ b/gdb/arch/aarch64.h
>> @@ -74,12 +74,24 @@ enum aarch64_regnum
>> VG : Vector Gradient.
>> The number of 64bit chunks in an SVE Z register. */
>>
>> +#ifndef sve_vg_from_vl
>> #define sve_vg_from_vl(vl) ((vl) / 8)
>> +#endif
>> +#ifndef sve_vl_from_vg
>> #define sve_vl_from_vg(vg) ((vg) * 8)
>> +#endif
>
> The guards around these first two aren’t needed. The kernel only
> defines the VQ to/from VL macros - as those are the only values the
> kernel cares about. Only GDB cares about VG because that is needed
> for dwarf.
Ah, fair enough. Removed.
>> +#ifndef sve_vq_from_vl
>> #define sve_vq_from_vl(vl) ((vl) / 0x10)
>> +#endif
>> +#ifndef sve_vl_from_vq
>> #define sve_vl_from_vq(vq) ((vq) * 0x10)
>> +#endif
>
> These two are fine!
>
>> +#ifndef sve_vq_from_vg
>> #define sve_vq_from_vg(vg) (sve_vq_from_vl (sve_vl_from_vg (vg)))
>> +#endif
>> +#ifndef sve_vg_from_vq
>> #define sve_vg_from_vq(vq) (sve_vg_from_vl (sve_vl_from_vq (vq)))
>> +#endif
>
> Again these last two aren’t needed.
Removed.
On Wednesday, June 06 2018, Simon Marchi wrote:
> On 2018-06-06 03:34 AM, Alan Hayward wrote:
>> FYI, either today or tomorrow I’ll be posting a new set of SVE patches.
>> As part of that series, due to review comments, I’ll be moving the all
>> the linux kernel defines to two new files which contain only kernel
>> defines (From ptrace.h and sigcontext.h). I’ll double check this works
>> with new header files. Regardless of that, I think your patch should
>> still go in to unbreak the build until then.
>>
>>
>> Changes are good to me if the VG guards are removed (but I’m not a
>> maintainer so cannot officially approve it).
>
> LGTM with Alan's proposed changes.
Thanks, pushed:
e5a77256e8961294b3ea7d483124834311ca363b
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 5/8] Add aarch64 psuedo help functions
2018-05-11 10:53 [PATCH 0/8] Add SVE support for Aarch64 GDB Alan Hayward
` (5 preceding siblings ...)
2018-05-11 10:53 ` [PATCH 2/8] Function for reading the Aarch64 SVE vector length Alan Hayward
@ 2018-05-11 11:52 ` Alan Hayward
2018-05-31 13:22 ` Simon Marchi
2018-05-11 12:12 ` [PATCH 3/8] Add SVE register defines Alan Hayward
2018-05-22 11:00 ` [PATCH 0/8] Add SVE support for Aarch64 GDB Alan Hayward
8 siblings, 1 reply; 47+ messages in thread
From: Alan Hayward @ 2018-05-11 11:52 UTC (permalink / raw)
To: gdb-patches; +Cc: nd, Alan Hayward
Reduce code copy/paste by adding two helper functions for
aarch64_pseudo_read_value and aarch64_pseudo_write
The patch does not change any functionality.
2018-05-11 Alan Hayward <alan.hayward@arm.com>
* aarch64-tdep.c (aarch64_pseudo_read_value_2): New helper func.
(aarch64_pseudo_write_2): Likewise.
(aarch64_pseudo_read_value): Use helper.
(aarch64_pseudo_write): Likewise.
---
gdb/aarch64-tdep.c | 173 +++++++++++++++++------------------------------------
1 file changed, 54 insertions(+), 119 deletions(-)
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 7893579d58..003fefb3c9 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -2247,109 +2247,67 @@ aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
return group == all_reggroup;
}
+/* Inner version of aarch64_pseudo_read_value. */
+
+static struct value *
+aarch64_pseudo_read_value_2 (readable_regcache *regcache, int regnum_offset,
+ int regsize, struct value *result_value)
+{
+ gdb_byte reg_buf[V_REGISTER_SIZE];
+ unsigned v_regnum = AARCH64_V0_REGNUM + regnum_offset;
+
+ if (regcache->raw_read (v_regnum, reg_buf) != REG_VALID)
+ mark_value_bytes_unavailable (result_value, 0,
+ TYPE_LENGTH (value_type (result_value)));
+ else
+ memcpy (value_contents_raw (result_value), reg_buf, regsize);
+ return result_value;
+ }
+
/* Implement the "pseudo_register_read_value" gdbarch method. */
static struct value *
-aarch64_pseudo_read_value (struct gdbarch *gdbarch,
- readable_regcache *regcache,
+aarch64_pseudo_read_value (struct gdbarch *gdbarch, readable_regcache *regcache,
int regnum)
{
- gdb_byte reg_buf[V_REGISTER_SIZE];
- struct value *result_value;
- gdb_byte *buf;
+ struct value *result_value = allocate_value (register_type (gdbarch, regnum));
- result_value = allocate_value (register_type (gdbarch, regnum));
VALUE_LVAL (result_value) = lval_register;
VALUE_REGNUM (result_value) = regnum;
- buf = value_contents_raw (result_value);
regnum -= gdbarch_num_regs (gdbarch);
if (regnum >= AARCH64_Q0_REGNUM && regnum < AARCH64_Q0_REGNUM + 32)
- {
- enum register_status status;
- unsigned v_regnum;
-
- v_regnum = AARCH64_V0_REGNUM + regnum - AARCH64_Q0_REGNUM;
- status = regcache->raw_read (v_regnum, reg_buf);
- if (status != REG_VALID)
- mark_value_bytes_unavailable (result_value, 0,
- TYPE_LENGTH (value_type (result_value)));
- else
- memcpy (buf, reg_buf, Q_REGISTER_SIZE);
- return result_value;
- }
+ return aarch64_pseudo_read_value_2 (regcache, regnum - AARCH64_Q0_REGNUM,
+ Q_REGISTER_SIZE, result_value);
if (regnum >= AARCH64_D0_REGNUM && regnum < AARCH64_D0_REGNUM + 32)
- {
- enum register_status status;
- unsigned v_regnum;
-
- v_regnum = AARCH64_V0_REGNUM + regnum - AARCH64_D0_REGNUM;
- status = regcache->raw_read (v_regnum, reg_buf);
- if (status != REG_VALID)
- mark_value_bytes_unavailable (result_value, 0,
- TYPE_LENGTH (value_type (result_value)));
- else
- memcpy (buf, reg_buf, D_REGISTER_SIZE);
- return result_value;
- }
+ return aarch64_pseudo_read_value_2 (regcache, regnum - AARCH64_D0_REGNUM,
+ D_REGISTER_SIZE, result_value);
if (regnum >= AARCH64_S0_REGNUM && regnum < AARCH64_S0_REGNUM + 32)
- {
- enum register_status status;
- unsigned v_regnum;
-
- v_regnum = AARCH64_V0_REGNUM + regnum - AARCH64_S0_REGNUM;
- status = regcache->raw_read (v_regnum, reg_buf);
- if (status != REG_VALID)
- mark_value_bytes_unavailable (result_value, 0,
- TYPE_LENGTH (value_type (result_value)));
- else
- memcpy (buf, reg_buf, S_REGISTER_SIZE);
- return result_value;
- }
+ return aarch64_pseudo_read_value_2 (regcache, regnum - AARCH64_S0_REGNUM,
+ S_REGISTER_SIZE, result_value);
if (regnum >= AARCH64_H0_REGNUM && regnum < AARCH64_H0_REGNUM + 32)
- {
- enum register_status status;
- unsigned v_regnum;
-
- v_regnum = AARCH64_V0_REGNUM + regnum - AARCH64_H0_REGNUM;
- status = regcache->raw_read (v_regnum, reg_buf);
- if (status != REG_VALID)
- mark_value_bytes_unavailable (result_value, 0,
- TYPE_LENGTH (value_type (result_value)));
- else
- memcpy (buf, reg_buf, H_REGISTER_SIZE);
- return result_value;
- }
+ return aarch64_pseudo_read_value_2 (regcache, regnum - AARCH64_H0_REGNUM,
+ H_REGISTER_SIZE, result_value);
if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
- {
- enum register_status status;
- unsigned v_regnum;
-
- v_regnum = AARCH64_V0_REGNUM + regnum - AARCH64_B0_REGNUM;
- status = regcache->raw_read (v_regnum, reg_buf);
- if (status != REG_VALID)
- mark_value_bytes_unavailable (result_value, 0,
- TYPE_LENGTH (value_type (result_value)));
- else
- memcpy (buf, reg_buf, B_REGISTER_SIZE);
- return result_value;
- }
+ return aarch64_pseudo_read_value_2 (regcache, regnum - AARCH64_B0_REGNUM,
+ B_REGISTER_SIZE, result_value);
gdb_assert_not_reached ("regnum out of bound");
}
-/* Implement the "pseudo_register_write" gdbarch method. */
+/* Inner version of aarch64_pseudo_write. */
static void
-aarch64_pseudo_write (struct gdbarch *gdbarch, struct regcache *regcache,
- int regnum, const gdb_byte *buf)
+aarch64_pseudo_write_2 (struct regcache *regcache, int regnum_offset,
+ int regsize, const gdb_byte *buf)
{
gdb_byte reg_buf[V_REGISTER_SIZE];
+ unsigned v_regnum = AARCH64_V0_REGNUM + regnum_offset;
/* Ensure the register buffer is zero, we want gdb writes of the
various 'scalar' pseudo registers to behavior like architectural
@@ -2357,61 +2315,38 @@ aarch64_pseudo_write (struct gdbarch *gdbarch, struct regcache *regcache,
zero. */
memset (reg_buf, 0, sizeof (reg_buf));
+ memcpy (reg_buf, buf, regsize);
+ regcache->raw_write (v_regnum, reg_buf);
+}
+
+/* Implement the "pseudo_register_write" gdbarch method. */
+
+static void
+aarch64_pseudo_write (struct gdbarch *gdbarch, struct regcache *regcache,
+ int regnum, const gdb_byte *buf)
+{
+
regnum -= gdbarch_num_regs (gdbarch);
if (regnum >= AARCH64_Q0_REGNUM && regnum < AARCH64_Q0_REGNUM + 32)
- {
- /* pseudo Q registers */
- unsigned v_regnum;
-
- v_regnum = AARCH64_V0_REGNUM + regnum - AARCH64_Q0_REGNUM;
- memcpy (reg_buf, buf, Q_REGISTER_SIZE);
- regcache_raw_write (regcache, v_regnum, reg_buf);
- return;
- }
+ return aarch64_pseudo_write_2 (regcache, regnum - AARCH64_Q0_REGNUM,
+ Q_REGISTER_SIZE, buf);
if (regnum >= AARCH64_D0_REGNUM && regnum < AARCH64_D0_REGNUM + 32)
- {
- /* pseudo D registers */
- unsigned v_regnum;
-
- v_regnum = AARCH64_V0_REGNUM + regnum - AARCH64_D0_REGNUM;
- memcpy (reg_buf, buf, D_REGISTER_SIZE);
- regcache_raw_write (regcache, v_regnum, reg_buf);
- return;
- }
+ return aarch64_pseudo_write_2 (regcache, regnum - AARCH64_D0_REGNUM,
+ D_REGISTER_SIZE, buf);
if (regnum >= AARCH64_S0_REGNUM && regnum < AARCH64_S0_REGNUM + 32)
- {
- unsigned v_regnum;
-
- v_regnum = AARCH64_V0_REGNUM + regnum - AARCH64_S0_REGNUM;
- memcpy (reg_buf, buf, S_REGISTER_SIZE);
- regcache_raw_write (regcache, v_regnum, reg_buf);
- return;
- }
+ return aarch64_pseudo_write_2 (regcache, regnum - AARCH64_S0_REGNUM,
+ S_REGISTER_SIZE, buf);
if (regnum >= AARCH64_H0_REGNUM && regnum < AARCH64_H0_REGNUM + 32)
- {
- /* pseudo H registers */
- unsigned v_regnum;
-
- v_regnum = AARCH64_V0_REGNUM + regnum - AARCH64_H0_REGNUM;
- memcpy (reg_buf, buf, H_REGISTER_SIZE);
- regcache_raw_write (regcache, v_regnum, reg_buf);
- return;
- }
+ return aarch64_pseudo_write_2 (regcache, regnum - AARCH64_H0_REGNUM,
+ H_REGISTER_SIZE, buf);
if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
- {
- /* pseudo B registers */
- unsigned v_regnum;
-
- v_regnum = AARCH64_V0_REGNUM + regnum - AARCH64_B0_REGNUM;
- memcpy (reg_buf, buf, B_REGISTER_SIZE);
- regcache_raw_write (regcache, v_regnum, reg_buf);
- return;
- }
+ return aarch64_pseudo_write_2 (regcache, regnum - AARCH64_B0_REGNUM,
+ B_REGISTER_SIZE, buf);
gdb_assert_not_reached ("regnum out of bound");
}
--
2.15.1 (Apple Git-101)
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 5/8] Add aarch64 psuedo help functions
2018-05-11 11:52 ` [PATCH 5/8] Add aarch64 psuedo help functions Alan Hayward
@ 2018-05-31 13:22 ` Simon Marchi
2018-05-31 15:20 ` Pedro Alves
2018-06-04 13:13 ` Alan Hayward
0 siblings, 2 replies; 47+ messages in thread
From: Simon Marchi @ 2018-05-31 13:22 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
psuedo -> pseudo in the title.
On 2018-05-11 06:52 AM, Alan Hayward wrote:
> Reduce code copy/paste by adding two helper functions for
> aarch64_pseudo_read_value and aarch64_pseudo_write
> The patch does not change any functionality.
>
> 2018-05-11 Alan Hayward <alan.hayward@arm.com>
>
> * aarch64-tdep.c (aarch64_pseudo_read_value_2): New helper func.
> (aarch64_pseudo_write_2): Likewise.
> (aarch64_pseudo_read_value): Use helper.
> (aarch64_pseudo_write): Likewise.
> ---
> gdb/aarch64-tdep.c | 173 +++++++++++++++++------------------------------------
> 1 file changed, 54 insertions(+), 119 deletions(-)
>
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 7893579d58..003fefb3c9 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -2247,109 +2247,67 @@ aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
> return group == all_reggroup;
> }
>
> +/* Inner version of aarch64_pseudo_read_value. */
> +
> +static struct value *
> +aarch64_pseudo_read_value_2 (readable_regcache *regcache, int regnum_offset,
> + int regsize, struct value *result_value)
It's not a big deal, but in other GDB parts we actually often use the _1 suffix
for the helper functions.
But otherwise, LGTM.
Simon
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 5/8] Add aarch64 psuedo help functions
2018-05-31 13:22 ` Simon Marchi
@ 2018-05-31 15:20 ` Pedro Alves
2018-06-04 13:13 ` Alan Hayward
1 sibling, 0 replies; 47+ messages in thread
From: Pedro Alves @ 2018-05-31 15:20 UTC (permalink / raw)
To: Simon Marchi, Alan Hayward, gdb-patches; +Cc: nd
On 05/31/2018 01:06 PM, Simon Marchi wrote:
>> +/* Inner version of aarch64_pseudo_read_value. */
>> +
>> +static struct value *
>> +aarch64_pseudo_read_value_2 (readable_regcache *regcache, int regnum_offset,
>> + int regsize, struct value *result_value)
>
> It's not a big deal, but in other GDB parts we actually often use the _1 suffix
> for the helper functions.
>
Also, I'd think "Helper for aarch64_pseudo_read_value." would be clearer,
because "inner version" sounds a bit odd to me.
> But otherwise, LGTM.
To me too.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 5/8] Add aarch64 psuedo help functions
2018-05-31 13:22 ` Simon Marchi
2018-05-31 15:20 ` Pedro Alves
@ 2018-06-04 13:13 ` Alan Hayward
1 sibling, 0 replies; 47+ messages in thread
From: Alan Hayward @ 2018-06-04 13:13 UTC (permalink / raw)
To: Simon Marchi, Pedro Alves; +Cc: gdb-patches, nd
Pushed with minor changes as suggested below.
> On 31 May 2018, at 13:06, Simon Marchi <simon.marchi@ericsson.com> wrote:
>
> psuedo -> pseudo in the title.
Ok.
>
> On 2018-05-11 06:52 AM, Alan Hayward wrote:
>> Reduce code copy/paste by adding two helper functions for
>> aarch64_pseudo_read_value and aarch64_pseudo_write
>> The patch does not change any functionality.
>>
>> 2018-05-11 Alan Hayward <alan.hayward@arm.com>
>>
>> * aarch64-tdep.c (aarch64_pseudo_read_value_2): New helper func.
>> (aarch64_pseudo_write_2): Likewise.
>> (aarch64_pseudo_read_value): Use helper.
>> (aarch64_pseudo_write): Likewise.
>> ---
>> gdb/aarch64-tdep.c | 173 +++++++++++++++++------------------------------------
>> 1 file changed, 54 insertions(+), 119 deletions(-)
>>
>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>> index 7893579d58..003fefb3c9 100644
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -2247,109 +2247,67 @@ aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
>> return group == all_reggroup;
>> }
>>
>> +/* Inner version of aarch64_pseudo_read_value. */
>> +
>> +static struct value *
>> +aarch64_pseudo_read_value_2 (readable_regcache *regcache, int regnum_offset,
>> + int regsize, struct value *result_value)
>
> It's not a big deal, but in other GDB parts we actually often use the _1 suffix
> for the helper functions.
Changed to aarch64_pseudo_read_value_1 and aarch64_pseudo_write_1.
>
> But otherwise, LGTM.
>
> Simon
>
> On 31 May 2018, at 15:57, Pedro Alves <palves@redhat.com> wrote:
>
> On 05/31/2018 01:06 PM, Simon Marchi wrote:
>
>>> +/* Inner version of aarch64_pseudo_read_value. */
>>> +
>>> +static struct value *
>>> +aarch64_pseudo_read_value_2 (readable_regcache *regcache, int regnum_offset,
>>> + int regsize, struct value *result_value)
>>
>> It's not a big deal, but in other GDB parts we actually often use the _1 suffix
>> for the helper functions.
>>
>
> Also, I'd think "Helper for aarch64_pseudo_read_value." would be clearer,
> because "inner version" sounds a bit odd to me.
Updated as suggested. Also updated write function too.
>
>> But otherwise, LGTM.
>
> To me too.
>
> Thanks,
> Pedro Alves
Thanks!
Alan.
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 3/8] Add SVE register defines
2018-05-11 10:53 [PATCH 0/8] Add SVE support for Aarch64 GDB Alan Hayward
` (6 preceding siblings ...)
2018-05-11 11:52 ` [PATCH 5/8] Add aarch64 psuedo help functions Alan Hayward
@ 2018-05-11 12:12 ` Alan Hayward
2018-06-01 8:33 ` Alan Hayward
2018-05-22 11:00 ` [PATCH 0/8] Add SVE support for Aarch64 GDB Alan Hayward
8 siblings, 1 reply; 47+ messages in thread
From: Alan Hayward @ 2018-05-11 12:12 UTC (permalink / raw)
To: gdb-patches; +Cc: nd, Alan Hayward
Add all the SVE register defines used by the later patches.
In order to prevent gaps in the register numbering, the Z registers
reuse the V register numbers (which become pseudos on SVE).
2018-05-11 Alan Hayward <alan.hayward@arm.com>
* aarch64-tdep.c (aarch64_sve_register_names): New const
var.
* arch/aarch64.h (enum aarch64_regnum): Add SVE entries.
(AARCH64_SVE_Z_REGS_NUM): New define.
(AARCH64_SVE_P_REGS_NUM): Likewise.
(AARCH64_SVE_NUM_REGS): Likewise.
---
gdb/aarch64-tdep.c | 21 +++++++++++++++++++++
gdb/arch/aarch64.h | 15 ++++++++++++++-
2 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 806a3dac55..1dc31a43bd 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -156,6 +156,27 @@ static const char *const aarch64_v_register_names[] =
"fpcr"
};
+/* The SVE 'Z' and 'P' registers. */
+static const char *const aarch64_sve_register_names[] =
+{
+ /* These registers must appear in consecutive RAW register number
+ order and they must begin with AARCH64_SVE_Z0_REGNUM! */
+ "z0", "z1", "z2", "z3",
+ "z4", "z5", "z6", "z7",
+ "z8", "z9", "z10", "z11",
+ "z12", "z13", "z14", "z15",
+ "z16", "z17", "z18", "z19",
+ "z20", "z21", "z22", "z23",
+ "z24", "z25", "z26", "z27",
+ "z28", "z29", "z30", "z31",
+ "fpsr", "fpcr",
+ "p0", "p1", "p2", "p3",
+ "p4", "p5", "p6", "p7",
+ "p8", "p9", "p10", "p11",
+ "p12", "p13", "p14", "p15",
+ "ffr", "vg"
+};
+
/* AArch64 prologue cache structure. */
struct aarch64_prologue_cache
{
diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
index af0b157c51..9855e6f286 100644
--- a/gdb/arch/aarch64.h
+++ b/gdb/arch/aarch64.h
@@ -24,7 +24,9 @@
target_desc *aarch64_create_target_description (long vq);
-/* Register numbers of various important registers. */
+/* Register numbers of various important registers.
+ Note that on SVE, the Z registers reuse the V register numbers and the V
+ registers become pseudo registers. */
enum aarch64_regnum
{
AARCH64_X0_REGNUM, /* First integer register. */
@@ -35,8 +37,15 @@ enum aarch64_regnum
AARCH64_CPSR_REGNUM, /* Current Program Status Register. */
AARCH64_V0_REGNUM, /* First fp/vec register. */
AARCH64_V31_REGNUM = AARCH64_V0_REGNUM + 31, /* Last fp/vec register. */
+ AARCH64_SVE_Z0_REGNUM = AARCH64_V0_REGNUM, /* First SVE Z register. */
+ AARCH64_SVE_Z31_REGNUM = AARCH64_V31_REGNUM, /* Last SVE Z register. */
AARCH64_FPSR_REGNUM, /* Floating Point Status Register. */
AARCH64_FPCR_REGNUM, /* Floating Point Control Register. */
+ AARCH64_SVE_P0_REGNUM, /* First SVE predicate register. */
+ AARCH64_SVE_P15_REGNUM = AARCH64_SVE_P0_REGNUM + 15, /* Last SVE predicate
+ register. */
+ AARCH64_SVE_FFR_REGNUM, /* SVE First Fault Register. */
+ AARCH64_SVE_VG_REGNUM, /* SVE Vector Gradient. */
/* Other useful registers. */
AARCH64_LAST_X_ARG_REGNUM = AARCH64_X0_REGNUM + 7,
@@ -46,7 +55,11 @@ enum aarch64_regnum
#define AARCH64_X_REGS_NUM 31
#define AARCH64_V_REGS_NUM 32
+#define AARCH64_SVE_Z_REGS_NUM AARCH64_V_REGS_NUM
+#define AARCH64_SVE_P_REGS_NUM 16
#define AARCH64_NUM_REGS AARCH64_FPCR_REGNUM + 1
+#define AARCH64_SVE_NUM_REGS AARCH64_SVE_VG_REGNUM + 1
+
/* There are a number of ways of expressing the current SVE vector size:
--
2.15.1 (Apple Git-101)
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 3/8] Add SVE register defines
2018-05-11 12:12 ` [PATCH 3/8] Add SVE register defines Alan Hayward
@ 2018-06-01 8:33 ` Alan Hayward
2018-06-01 15:18 ` Simon Marchi
0 siblings, 1 reply; 47+ messages in thread
From: Alan Hayward @ 2018-06-01 8:33 UTC (permalink / raw)
To: gdb-patches, Pedro Alves, Simon Marchi; +Cc: nd
Pedro, Simon:
Thanks for all the reviews.
Part 3 slipped under the net. Could some ok this one?
(Looking at it again, I’m not keen on the ! in the comment, but was
just duplicating the previous comment.)
Thanks,
Alan.
> On 11 May 2018, at 11:52, Alan Hayward <Alan.Hayward@arm.com> wrote:
>
> Add all the SVE register defines used by the later patches.
>
> In order to prevent gaps in the register numbering, the Z registers
> reuse the V register numbers (which become pseudos on SVE).
>
> 2018-05-11 Alan Hayward <alan.hayward@arm.com>
>
> * aarch64-tdep.c (aarch64_sve_register_names): New const
> var.
> * arch/aarch64.h (enum aarch64_regnum): Add SVE entries.
> (AARCH64_SVE_Z_REGS_NUM): New define.
> (AARCH64_SVE_P_REGS_NUM): Likewise.
> (AARCH64_SVE_NUM_REGS): Likewise.
> ---
> gdb/aarch64-tdep.c | 21 +++++++++++++++++++++
> gdb/arch/aarch64.h | 15 ++++++++++++++-
> 2 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 806a3dac55..1dc31a43bd 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -156,6 +156,27 @@ static const char *const aarch64_v_register_names[] =
> "fpcr"
> };
>
> +/* The SVE 'Z' and 'P' registers. */
> +static const char *const aarch64_sve_register_names[] =
> +{
> + /* These registers must appear in consecutive RAW register number
> + order and they must begin with AARCH64_SVE_Z0_REGNUM! */
> + "z0", "z1", "z2", "z3",
> + "z4", "z5", "z6", "z7",
> + "z8", "z9", "z10", "z11",
> + "z12", "z13", "z14", "z15",
> + "z16", "z17", "z18", "z19",
> + "z20", "z21", "z22", "z23",
> + "z24", "z25", "z26", "z27",
> + "z28", "z29", "z30", "z31",
> + "fpsr", "fpcr",
> + "p0", "p1", "p2", "p3",
> + "p4", "p5", "p6", "p7",
> + "p8", "p9", "p10", "p11",
> + "p12", "p13", "p14", "p15",
> + "ffr", "vg"
> +};
> +
> /* AArch64 prologue cache structure. */
> struct aarch64_prologue_cache
> {
> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
> index af0b157c51..9855e6f286 100644
> --- a/gdb/arch/aarch64.h
> +++ b/gdb/arch/aarch64.h
> @@ -24,7 +24,9 @@
>
> target_desc *aarch64_create_target_description (long vq);
>
> -/* Register numbers of various important registers. */
> +/* Register numbers of various important registers.
> + Note that on SVE, the Z registers reuse the V register numbers and the V
> + registers become pseudo registers. */
> enum aarch64_regnum
> {
> AARCH64_X0_REGNUM, /* First integer register. */
> @@ -35,8 +37,15 @@ enum aarch64_regnum
> AARCH64_CPSR_REGNUM, /* Current Program Status Register. */
> AARCH64_V0_REGNUM, /* First fp/vec register. */
> AARCH64_V31_REGNUM = AARCH64_V0_REGNUM + 31, /* Last fp/vec register. */
> + AARCH64_SVE_Z0_REGNUM = AARCH64_V0_REGNUM, /* First SVE Z register. */
> + AARCH64_SVE_Z31_REGNUM = AARCH64_V31_REGNUM, /* Last SVE Z register. */
> AARCH64_FPSR_REGNUM, /* Floating Point Status Register. */
> AARCH64_FPCR_REGNUM, /* Floating Point Control Register. */
> + AARCH64_SVE_P0_REGNUM, /* First SVE predicate register. */
> + AARCH64_SVE_P15_REGNUM = AARCH64_SVE_P0_REGNUM + 15, /* Last SVE predicate
> + register. */
> + AARCH64_SVE_FFR_REGNUM, /* SVE First Fault Register. */
> + AARCH64_SVE_VG_REGNUM, /* SVE Vector Gradient. */
>
> /* Other useful registers. */
> AARCH64_LAST_X_ARG_REGNUM = AARCH64_X0_REGNUM + 7,
> @@ -46,7 +55,11 @@ enum aarch64_regnum
>
> #define AARCH64_X_REGS_NUM 31
> #define AARCH64_V_REGS_NUM 32
> +#define AARCH64_SVE_Z_REGS_NUM AARCH64_V_REGS_NUM
> +#define AARCH64_SVE_P_REGS_NUM 16
> #define AARCH64_NUM_REGS AARCH64_FPCR_REGNUM + 1
> +#define AARCH64_SVE_NUM_REGS AARCH64_SVE_VG_REGNUM + 1
> +
>
> /* There are a number of ways of expressing the current SVE vector size:
>
> --
> 2.15.1 (Apple Git-101)
>
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 3/8] Add SVE register defines
2018-06-01 8:33 ` Alan Hayward
@ 2018-06-01 15:18 ` Simon Marchi
0 siblings, 0 replies; 47+ messages in thread
From: Simon Marchi @ 2018-06-01 15:18 UTC (permalink / raw)
To: Alan Hayward, gdb-patches, Pedro Alves; +Cc: nd
On 2018-06-01 04:33 AM, Alan Hayward wrote:
> Pedro, Simon:
> Thanks for all the reviews.
>
> Part 3 slipped under the net. Could some ok this one?
>
> (Looking at it again, I’m not keen on the ! in the comment, but was
> just duplicating the previous comment.)
>
> Thanks,
> Alan.
>
>> On 11 May 2018, at 11:52, Alan Hayward <Alan.Hayward@arm.com> wrote:
>>
>> Add all the SVE register defines used by the later patches.
>>
>> In order to prevent gaps in the register numbering, the Z registers
>> reuse the V register numbers (which become pseudos on SVE).
Ah sorry, yes this LGTM.
Simon
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/8] Add SVE support for Aarch64 GDB
2018-05-11 10:53 [PATCH 0/8] Add SVE support for Aarch64 GDB Alan Hayward
` (7 preceding siblings ...)
2018-05-11 12:12 ` [PATCH 3/8] Add SVE register defines Alan Hayward
@ 2018-05-22 11:00 ` Alan Hayward
2018-05-29 12:09 ` [PING 2][PATCH " Alan Hayward
8 siblings, 1 reply; 47+ messages in thread
From: Alan Hayward @ 2018-05-22 11:00 UTC (permalink / raw)
To: gdb-patches; +Cc: nd
Ping.
Thanks,
Alan.
> On 11 May 2018, at 11:52, Alan Hayward <Alan.Hayward@arm.com> wrote:
>
> This set of patches adds gdb support for SVE on AArch64.
>
> SVE is the new vector extension for Aarch64. SVE is different in that the
> length of a vector register is not fixed - it is can vary for different
> hardware implementors. All code compiled for SVE is vector length agnostic.
> The Linux kernel can then further restrict the vector length up to the
> supported maximum for the hardware. Potentially different process and
> threads can have different vector lengths, and they could change at any
> time. However, in practice, we expect the vector length to remain constant
> across the lifetime of a process.
> This set of patches assumes the vector length will not change and either
> warns once (when reading registers) or errors (when writing registers) if the
> vector length has changed. A future set of patches will offer full support.
>
> This series does not support gdbserver. However, where it makes sense I have
> commonised as much code as possible and have added the groundwork in
> gdbserver. Enabling gdbserver should be a small set of additional patches.
>
> Core files and watchpoints are not yet supported.
>
> The vector length is read from ptrace and is required to create the target
> description for the running process. There is no hardcoded SVE XML.
>
> The patches require recent linux header files for all the SVE ptrace macros.
> To allow builds for older headers, I've added all required macros and
> structures within ifdef checks. I'm not sure if this is the ideal solution.
> When including kernel header code, I've not performed any reformatting (ie
> they are not in GNU style).
>
> One of the later patches adds functions to gdbserver regcache so that I can
> add common functionality that works on gdb and gdbserver.
>
> Given there are no working SVE systems available today, this was manually
> tested on an Aarch64 SVE emulator running Ubuntu. In addition I've run make
> check on X86 and Aarch64 builds for both unix and native-gdbserver.
>
>
> Alan.
>
> Alan Hayward (8):
> Add Aarch64 SVE target description
> Function for reading the Aarch64 SVE vector length.
> Add SVE register defines
> Enable SVE for GDB
> Add aarch64 psuedo help functions
> Aarch64 SVE pseudo register support
> Add methods to gdbserver regcache and raw_compare
> Ptrace support for Aarch64 SVE
>
> gdb/Makefile.in | 1 +
> gdb/aarch64-linux-nat.c | 60 +++++-
> gdb/aarch64-linux-tdep.c | 5 +-
> gdb/aarch64-tdep.c | 402 +++++++++++++++++++++++-------------
> gdb/aarch64-tdep.h | 12 +-
> gdb/arch/aarch64.c | 12 +-
> gdb/arch/aarch64.h | 37 +++-
> gdb/configure.nat | 2 +-
> gdb/doc/gdb.texinfo | 4 +
> gdb/features/aarch64-sve.c | 158 ++++++++++++++
> gdb/gdbserver/Makefile.in | 1 +
> gdb/gdbserver/configure.srv | 1 +
> gdb/gdbserver/linux-aarch64-tdesc.c | 3 +-
> gdb/gdbserver/regcache.c | 55 ++++-
> gdb/gdbserver/regcache.h | 8 +
> gdb/nat/aarch64-sve-linux-ptrace.c | 315 ++++++++++++++++++++++++++++
> gdb/nat/aarch64-sve-linux-ptrace.h | 193 +++++++++++++++++
> gdb/regcache.c | 17 ++
> gdb/regcache.h | 2 +
> 19 files changed, 1122 insertions(+), 166 deletions(-)
> create mode 100644 gdb/features/aarch64-sve.c
> create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.c
> create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.h
>
> --
> 2.15.1 (Apple Git-101)
>
^ permalink raw reply [flat|nested] 47+ messages in thread* [PING 2][PATCH 0/8] Add SVE support for Aarch64 GDB
2018-05-22 11:00 ` [PATCH 0/8] Add SVE support for Aarch64 GDB Alan Hayward
@ 2018-05-29 12:09 ` Alan Hayward
2018-05-29 14:35 ` Omair Javaid
0 siblings, 1 reply; 47+ messages in thread
From: Alan Hayward @ 2018-05-29 12:09 UTC (permalink / raw)
To: gdb-patches; +Cc: nd
Ping ping.
Thanks,
Alan.
> On 22 May 2018, at 09:34, Alan Hayward <Alan.Hayward@arm.com> wrote:
>
> Ping.
>
>
> Thanks,
> Alan.
>
>> On 11 May 2018, at 11:52, Alan Hayward <Alan.Hayward@arm.com> wrote:
>>
>> This set of patches adds gdb support for SVE on AArch64.
>>
>> SVE is the new vector extension for Aarch64. SVE is different in that the
>> length of a vector register is not fixed - it is can vary for different
>> hardware implementors. All code compiled for SVE is vector length agnostic.
>> The Linux kernel can then further restrict the vector length up to the
>> supported maximum for the hardware. Potentially different process and
>> threads can have different vector lengths, and they could change at any
>> time. However, in practice, we expect the vector length to remain constant
>> across the lifetime of a process.
>> This set of patches assumes the vector length will not change and either
>> warns once (when reading registers) or errors (when writing registers) if the
>> vector length has changed. A future set of patches will offer full support.
>>
>> This series does not support gdbserver. However, where it makes sense I have
>> commonised as much code as possible and have added the groundwork in
>> gdbserver. Enabling gdbserver should be a small set of additional patches.
>>
>> Core files and watchpoints are not yet supported.
>>
>> The vector length is read from ptrace and is required to create the target
>> description for the running process. There is no hardcoded SVE XML.
>>
>> The patches require recent linux header files for all the SVE ptrace macros.
>> To allow builds for older headers, I've added all required macros and
>> structures within ifdef checks. I'm not sure if this is the ideal solution.
>> When including kernel header code, I've not performed any reformatting (ie
>> they are not in GNU style).
>>
>> One of the later patches adds functions to gdbserver regcache so that I can
>> add common functionality that works on gdb and gdbserver.
>>
>> Given there are no working SVE systems available today, this was manually
>> tested on an Aarch64 SVE emulator running Ubuntu. In addition I've run make
>> check on X86 and Aarch64 builds for both unix and native-gdbserver.
>>
>>
>> Alan.
>>
>> Alan Hayward (8):
>> Add Aarch64 SVE target description
>> Function for reading the Aarch64 SVE vector length.
>> Add SVE register defines
>> Enable SVE for GDB
>> Add aarch64 psuedo help functions
>> Aarch64 SVE pseudo register support
>> Add methods to gdbserver regcache and raw_compare
>> Ptrace support for Aarch64 SVE
>>
>> gdb/Makefile.in | 1 +
>> gdb/aarch64-linux-nat.c | 60 +++++-
>> gdb/aarch64-linux-tdep.c | 5 +-
>> gdb/aarch64-tdep.c | 402 +++++++++++++++++++++++-------------
>> gdb/aarch64-tdep.h | 12 +-
>> gdb/arch/aarch64.c | 12 +-
>> gdb/arch/aarch64.h | 37 +++-
>> gdb/configure.nat | 2 +-
>> gdb/doc/gdb.texinfo | 4 +
>> gdb/features/aarch64-sve.c | 158 ++++++++++++++
>> gdb/gdbserver/Makefile.in | 1 +
>> gdb/gdbserver/configure.srv | 1 +
>> gdb/gdbserver/linux-aarch64-tdesc.c | 3 +-
>> gdb/gdbserver/regcache.c | 55 ++++-
>> gdb/gdbserver/regcache.h | 8 +
>> gdb/nat/aarch64-sve-linux-ptrace.c | 315 ++++++++++++++++++++++++++++
>> gdb/nat/aarch64-sve-linux-ptrace.h | 193 +++++++++++++++++
>> gdb/regcache.c | 17 ++
>> gdb/regcache.h | 2 +
>> 19 files changed, 1122 insertions(+), 166 deletions(-)
>> create mode 100644 gdb/features/aarch64-sve.c
>> create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.c
>> create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.h
>>
>> --
>> 2.15.1 (Apple Git-101)
>>
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PING 2][PATCH 0/8] Add SVE support for Aarch64 GDB
2018-05-29 12:09 ` [PING 2][PATCH " Alan Hayward
@ 2018-05-29 14:35 ` Omair Javaid
2018-05-29 14:59 ` Alan Hayward
0 siblings, 1 reply; 47+ messages in thread
From: Omair Javaid @ 2018-05-29 14:35 UTC (permalink / raw)
To: Alan Hayward; +Cc: gdb-patches, nd
Hi Alan,
I would like to help you review and get these patches accepted upstream.
https://developer.arm.com/docs/ddi0584/latest/arm-architecture-reference-manual-supplement-the-scalable-vector-extension-sve-for-armv8-a
Do you think above link has the latest documents on SVE?
Also can we test these patches with QEMU or any other virtualization model?
What is the status of corresponding ptrace changes in kernel? Are they
already accepted which kernel versions will have that code?
I will go forward the say more on your patches in coming days.
Thanks!
On 29 May 2018 at 13:59, Alan Hayward <Alan.Hayward@arm.com> wrote:
> Ping ping.
>
> Thanks,
> Alan.
>
>> On 22 May 2018, at 09:34, Alan Hayward <Alan.Hayward@arm.com> wrote:
>>
>> Ping.
>>
>>
>> Thanks,
>> Alan.
>>
>>> On 11 May 2018, at 11:52, Alan Hayward <Alan.Hayward@arm.com> wrote:
>>>
>>> This set of patches adds gdb support for SVE on AArch64.
>>>
>>> SVE is the new vector extension for Aarch64. SVE is different in that the
>>> length of a vector register is not fixed - it is can vary for different
>>> hardware implementors. All code compiled for SVE is vector length agnostic.
>>> The Linux kernel can then further restrict the vector length up to the
>>> supported maximum for the hardware. Potentially different process and
>>> threads can have different vector lengths, and they could change at any
>>> time. However, in practice, we expect the vector length to remain constant
>>> across the lifetime of a process.
>>> This set of patches assumes the vector length will not change and either
>>> warns once (when reading registers) or errors (when writing registers) if the
>>> vector length has changed. A future set of patches will offer full support.
>>>
>>> This series does not support gdbserver. However, where it makes sense I have
>>> commonised as much code as possible and have added the groundwork in
>>> gdbserver. Enabling gdbserver should be a small set of additional patches.
>>>
>>> Core files and watchpoints are not yet supported.
>>>
>>> The vector length is read from ptrace and is required to create the target
>>> description for the running process. There is no hardcoded SVE XML.
>>>
>>> The patches require recent linux header files for all the SVE ptrace macros.
>>> To allow builds for older headers, I've added all required macros and
>>> structures within ifdef checks. I'm not sure if this is the ideal solution.
>>> When including kernel header code, I've not performed any reformatting (ie
>>> they are not in GNU style).
>>>
>>> One of the later patches adds functions to gdbserver regcache so that I can
>>> add common functionality that works on gdb and gdbserver.
>>>
>>> Given there are no working SVE systems available today, this was manually
>>> tested on an Aarch64 SVE emulator running Ubuntu. In addition I've run make
>>> check on X86 and Aarch64 builds for both unix and native-gdbserver.
>>>
>>>
>>> Alan.
>>>
>>> Alan Hayward (8):
>>> Add Aarch64 SVE target description
>>> Function for reading the Aarch64 SVE vector length.
>>> Add SVE register defines
>>> Enable SVE for GDB
>>> Add aarch64 psuedo help functions
>>> Aarch64 SVE pseudo register support
>>> Add methods to gdbserver regcache and raw_compare
>>> Ptrace support for Aarch64 SVE
>>>
>>> gdb/Makefile.in | 1 +
>>> gdb/aarch64-linux-nat.c | 60 +++++-
>>> gdb/aarch64-linux-tdep.c | 5 +-
>>> gdb/aarch64-tdep.c | 402 +++++++++++++++++++++++-------------
>>> gdb/aarch64-tdep.h | 12 +-
>>> gdb/arch/aarch64.c | 12 +-
>>> gdb/arch/aarch64.h | 37 +++-
>>> gdb/configure.nat | 2 +-
>>> gdb/doc/gdb.texinfo | 4 +
>>> gdb/features/aarch64-sve.c | 158 ++++++++++++++
>>> gdb/gdbserver/Makefile.in | 1 +
>>> gdb/gdbserver/configure.srv | 1 +
>>> gdb/gdbserver/linux-aarch64-tdesc.c | 3 +-
>>> gdb/gdbserver/regcache.c | 55 ++++-
>>> gdb/gdbserver/regcache.h | 8 +
>>> gdb/nat/aarch64-sve-linux-ptrace.c | 315 ++++++++++++++++++++++++++++
>>> gdb/nat/aarch64-sve-linux-ptrace.h | 193 +++++++++++++++++
>>> gdb/regcache.c | 17 ++
>>> gdb/regcache.h | 2 +
>>> 19 files changed, 1122 insertions(+), 166 deletions(-)
>>> create mode 100644 gdb/features/aarch64-sve.c
>>> create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.c
>>> create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.h
>>>
>>> --
>>> 2.15.1 (Apple Git-101)
>>>
>>
>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PING 2][PATCH 0/8] Add SVE support for Aarch64 GDB
2018-05-29 14:35 ` Omair Javaid
@ 2018-05-29 14:59 ` Alan Hayward
0 siblings, 0 replies; 47+ messages in thread
From: Alan Hayward @ 2018-05-29 14:59 UTC (permalink / raw)
To: Omair Javaid; +Cc: gdb-patches, nd
> On 29 May 2018, at 14:06, Omair Javaid <omair.javaid@linaro.org> wrote:
>
> Hi Alan,
>
> I would like to help you review and get these patches accepted upstream.
>
Thanks!
> https://developer.arm.com/docs/ddi0584/latest/arm-architecture-reference-manual-supplement-the-scalable-vector-extension-sve-for-armv8-a
>
> Do you think above link has the latest documents on SVE?
Yes, that’ll give you the latest docs for SVE.
At the last GNU cauldron I gave a summary of SVE, including the
latest status on Linux, and the changes required for gdb:
https://slideslive.com/38902367/supporting-variable-register-sizes-for-the-arm-scalable-vector-extension-sve-in-gdb
The regcache slide is out of date - it’s an early plan for
detached_regcache/readable_regcache etc. Not sure if the video
is ok, as I dare not watch it :)
Also, there is also a nice delve into some simple programming
using SVE here:
http://developer.arm.com/hpc/a-sneak-peek-into-sve-and-vla-programming
Binutils fully supports SVE already.
(I should have put all this in the patch summary).
>
> Also can we test these patches with QEMU or any other virtualization model?
SVE support for QEMU is being worked on by Linaro. The changelog for
the latest release (2.12) says preliminary SVE support. I can look into
this a little more if you want - but I suspect it won’t be good enough yet.
(You’ll need to emulate a full linux kernel - otherwise qemu will fail
when it hits ptrace).
I’ve been using Arm’s Fast Models, but that’s a paid for product and sadly
not something I can easily give you access to.
What happens with other ports to obscure architectures for which most people
/ reviewers don't have access too?
At a minimum, the existing buildbot setup will confirm that nothing is
broken for normal aarch64.
>
> What is the status of corresponding ptrace changes in kernel? Are they
> already accepted which kernel versions will have that code?
SVE support went into Linux 4.15 in a single batch patches.
Easiest way to check is to look for NT_ARM_SVE in elf.h:
https://github.com/torvalds/linux/blob/v4.15/include/uapi/linux/elf.h
>
> I will go forward the say more on your patches in coming days.
Note, because this patch set doesn’t support gdbserver, it makes the last
two patches a little strange. I now have gdbserver support working too
(it’s just a few small changes). When I get to V2 of the set, I’ll
probably add in the gdbserver changes.
Thanks,
Alan.
>
> Thanks!
>
> On 29 May 2018 at 13:59, Alan Hayward <Alan.Hayward@arm.com> wrote:
>> Ping ping.
>>
>> Thanks,
>> Alan.
>>
>>> On 22 May 2018, at 09:34, Alan Hayward <Alan.Hayward@arm.com> wrote:
>>>
>>> Ping.
>>>
>>>
>>> Thanks,
>>> Alan.
>>>
>>>> On 11 May 2018, at 11:52, Alan Hayward <Alan.Hayward@arm.com> wrote:
>>>>
>>>> This set of patches adds gdb support for SVE on AArch64.
>>>>
>>>> SVE is the new vector extension for Aarch64. SVE is different in that the
>>>> length of a vector register is not fixed - it is can vary for different
>>>> hardware implementors. All code compiled for SVE is vector length agnostic.
>>>> The Linux kernel can then further restrict the vector length up to the
>>>> supported maximum for the hardware. Potentially different process and
>>>> threads can have different vector lengths, and they could change at any
>>>> time. However, in practice, we expect the vector length to remain constant
>>>> across the lifetime of a process.
>>>> This set of patches assumes the vector length will not change and either
>>>> warns once (when reading registers) or errors (when writing registers) if the
>>>> vector length has changed. A future set of patches will offer full support.
>>>>
>>>> This series does not support gdbserver. However, where it makes sense I have
>>>> commonised as much code as possible and have added the groundwork in
>>>> gdbserver. Enabling gdbserver should be a small set of additional patches.
>>>>
>>>> Core files and watchpoints are not yet supported.
>>>>
>>>> The vector length is read from ptrace and is required to create the target
>>>> description for the running process. There is no hardcoded SVE XML.
>>>>
>>>> The patches require recent linux header files for all the SVE ptrace macros.
>>>> To allow builds for older headers, I've added all required macros and
>>>> structures within ifdef checks. I'm not sure if this is the ideal solution.
>>>> When including kernel header code, I've not performed any reformatting (ie
>>>> they are not in GNU style).
>>>>
>>>> One of the later patches adds functions to gdbserver regcache so that I can
>>>> add common functionality that works on gdb and gdbserver.
>>>>
>>>> Given there are no working SVE systems available today, this was manually
>>>> tested on an Aarch64 SVE emulator running Ubuntu. In addition I've run make
>>>> check on X86 and Aarch64 builds for both unix and native-gdbserver.
>>>>
>>>>
>>>> Alan.
>>>>
>>>> Alan Hayward (8):
>>>> Add Aarch64 SVE target description
>>>> Function for reading the Aarch64 SVE vector length.
>>>> Add SVE register defines
>>>> Enable SVE for GDB
>>>> Add aarch64 psuedo help functions
>>>> Aarch64 SVE pseudo register support
>>>> Add methods to gdbserver regcache and raw_compare
>>>> Ptrace support for Aarch64 SVE
>>>>
>>>> gdb/Makefile.in | 1 +
>>>> gdb/aarch64-linux-nat.c | 60 +++++-
>>>> gdb/aarch64-linux-tdep.c | 5 +-
>>>> gdb/aarch64-tdep.c | 402 +++++++++++++++++++++++-------------
>>>> gdb/aarch64-tdep.h | 12 +-
>>>> gdb/arch/aarch64.c | 12 +-
>>>> gdb/arch/aarch64.h | 37 +++-
>>>> gdb/configure.nat | 2 +-
>>>> gdb/doc/gdb.texinfo | 4 +
>>>> gdb/features/aarch64-sve.c | 158 ++++++++++++++
>>>> gdb/gdbserver/Makefile.in | 1 +
>>>> gdb/gdbserver/configure.srv | 1 +
>>>> gdb/gdbserver/linux-aarch64-tdesc.c | 3 +-
>>>> gdb/gdbserver/regcache.c | 55 ++++-
>>>> gdb/gdbserver/regcache.h | 8 +
>>>> gdb/nat/aarch64-sve-linux-ptrace.c | 315 ++++++++++++++++++++++++++++
>>>> gdb/nat/aarch64-sve-linux-ptrace.h | 193 +++++++++++++++++
>>>> gdb/regcache.c | 17 ++
>>>> gdb/regcache.h | 2 +
>>>> 19 files changed, 1122 insertions(+), 166 deletions(-)
>>>> create mode 100644 gdb/features/aarch64-sve.c
>>>> create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.c
>>>> create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.h
>>>>
>>>> --
>>>> 2.15.1 (Apple Git-101)
>>>>
>>>
>>
^ permalink raw reply [flat|nested] 47+ messages in thread