* [PATCH v2 03/10] Add reg_buffer_common
2018-06-06 15:16 [PATCH v2 00/10] gdb/gdbserver support for aarch64 SVE Alan Hayward
@ 2018-06-06 15:17 ` Alan Hayward
2018-06-07 20:19 ` Simon Marchi
2018-06-06 15:17 ` [PATCH v2 04/10] Add regcache raw_compare method Alan Hayward
` (8 subsequent siblings)
9 siblings, 1 reply; 56+ messages in thread
From: Alan Hayward @ 2018-06-06 15:17 UTC (permalink / raw)
To: gdb-patches; +Cc: nd, Alan Hayward
A purely virtual class containing functions from gdb/regcache.h
Both the gdb regcache structures and gdbserver regcache inherit
directly from reg_buffer_common. This will allow for common
functions which require the use of a regcache.
2018-06-06 Alan Hayward <alan.hayward@arm.com>
gdb/
* common/common-regcache.h (reg_buffer_common): New structure.
* regcache.h (reg_buffer_common:get_register_status) Add override.
(reg_buffer_common:raw_supply): Add dummy function.
reg_buffer_common:raw_collect): Likewise
(readable_regcache:raw_supply) Add override.
(detached_regcache:raw_collect) Likewise.
gdbserver/
* regcache.c (new_register_cache): Use new.
(free_register_cache): Use delete.
register_data): Use const.
(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.h (regcache:raw_supply): Add dummy function.
(regcache:raw_collect): Likewise
(regcache:get_register_status) Likewise.
---
gdb/common/common-regcache.h | 8 ++++++++
gdb/gdbserver/regcache.c | 47 ++++++++++++++++++++++++++++++++------------
gdb/gdbserver/regcache.h | 18 +++++++++++------
gdb/regcache.h | 19 ++++++++++++++----
4 files changed, 69 insertions(+), 23 deletions(-)
diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h
index 696ba00955..487da0a7fb 100644
--- a/gdb/common/common-regcache.h
+++ b/gdb/common/common-regcache.h
@@ -62,4 +62,12 @@ extern enum register_status regcache_raw_read_unsigned
ULONGEST regcache_raw_get_unsigned (struct regcache *regcache, int regnum);
+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 register_status get_register_status (int regnum) const = 0;
+};
+
#endif /* COMMON_REGCACHE_H */
diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
index 0718b9f9a0..857721ee3d 100644
--- a/gdb/gdbserver/regcache.c
+++ b/gdb/gdbserver/regcache.c
@@ -159,7 +159,7 @@ init_register_cache (struct regcache *regcache,
struct regcache *
new_register_cache (const struct target_desc *tdesc)
{
- struct regcache *regcache = XCNEW (struct regcache);
+ struct regcache *regcache = new struct regcache;
gdb_assert (tdesc->registers_size != 0);
@@ -174,7 +174,7 @@ free_register_cache (struct regcache *regcache)
if (regcache->registers_owned)
free (regcache->registers);
free (regcache->register_status);
- free (regcache);
+ delete regcache;
}
}
@@ -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,14 @@ 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
+}
diff --git a/gdb/gdbserver/regcache.h b/gdb/gdbserver/regcache.h
index 2c0df648f6..1842f1d9cf 100644
--- a/gdb/gdbserver/regcache.h
+++ b/gdb/gdbserver/regcache.h
@@ -28,23 +28,29 @@ struct target_desc;
inferior; this is primarily for simplicity, as the performance
benefit is minimal. */
-struct regcache
+struct regcache : public reg_buffer_common
{
/* The regcache's target description. */
- const struct target_desc *tdesc;
+ const struct target_desc *tdesc = nullptr;
/* Whether the REGISTERS buffer's contents are valid. If false, we
haven't fetched the registers from the target yet. Not that this
register cache is _not_ pass-through, unlike GDB's. Note that
"valid" here is unrelated to whether the registers are available
in a traceframe. For that, check REGISTER_STATUS below. */
- int registers_valid;
- int registers_owned;
- unsigned char *registers;
+ int registers_valid = 0;
+ int registers_owned = 0;
+ unsigned char *registers = nullptr;
#ifndef IN_PROCESS_AGENT
/* One of REG_UNAVAILBLE or REG_VALID. */
- unsigned char *register_status;
+ unsigned char *register_status = nullptr;
#endif
+
+ void raw_supply (int regnum, const void *buf) override;
+
+ void raw_collect (int regnum, void *buf) const override;
+
+ enum register_status get_register_status (int regnum) const override;
};
struct regcache *init_register_cache (struct regcache *regcache,
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 3edddf47e1..b559a10752 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -139,7 +139,7 @@ typedef struct cached_reg
/* Buffer of registers. */
-class reg_buffer
+class reg_buffer : public reg_buffer_common
{
public:
reg_buffer (gdbarch *gdbarch, bool has_pseudo);
@@ -151,13 +151,24 @@ public:
/* Get the availability status of the value of register REGNUM in this
buffer. */
- enum register_status get_register_status (int regnum) const;
+ enum register_status get_register_status (int regnum) const override;
virtual ~reg_buffer ()
{
xfree (m_registers);
xfree (m_register_status);
}
+
+ virtual void raw_supply (int regnum, const void *buf) override
+ {
+ gdb_assert (false);
+ }
+
+ virtual void raw_collect (int regnum, void *buf) const override
+ {
+ gdb_assert (false);
+ }
+
protected:
/* Assert on the range of REGNUM. */
void assert_regnum (int regnum) const;
@@ -235,7 +246,7 @@ public:
{}
/* Supply register REGNUM, whose contents are stored in BUF, to REGCACHE. */
- void raw_supply (int regnum, const void *buf);
+ void raw_supply (int regnum, const void *buf) override;
void raw_supply (int regnum, const reg_buffer &src)
{
@@ -293,7 +304,7 @@ public:
void raw_update (int regnum) override;
/* Collect register REGNUM from REGCACHE and store its contents in BUF. */
- void raw_collect (int regnum, void *buf) const;
+ void raw_collect (int regnum, void *buf) const override;
void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
bool is_signed) const;
--
2.15.1 (Apple Git-101)
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 03/10] Add reg_buffer_common
2018-06-06 15:17 ` [PATCH v2 03/10] Add reg_buffer_common Alan Hayward
@ 2018-06-07 20:19 ` Simon Marchi
2018-06-07 20:42 ` Simon Marchi
2018-06-08 14:14 ` Alan Hayward
0 siblings, 2 replies; 56+ messages in thread
From: Simon Marchi @ 2018-06-07 20:19 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
Hi Alan,
Just some quick comments.
I get this when building on x86-64 with --enable-targets=all:
CXX aarch64-tdep.o
In file included from /home/emaisin/src/binutils-gdb/gdb/nat/aarch64-sve-linux-ptrace.h:29:0,
from /home/emaisin/src/binutils-gdb/gdb/aarch64-tdep.c:61:
/home/emaisin/src/binutils-gdb/gdb/nat/aarch64-linux-sigcontext.h:19:22: error: field ‘head’ has incomplete type ‘_aarch64_ctx’
struct _aarch64_ctx head;
^
/home/emaisin/src/binutils-gdb/gdb/nat/aarch64-linux-sigcontext.h:19:9: note: forward declaration of ‘struct _aarch64_ctx’
struct _aarch64_ctx head;
^
First, we should not include "nat/aarch64-sve-linux-ptrace.h" (a file that only makes
sense when building on AArch64) in aarch64-tdep.c, a file built on all architecture
when including the support for AArch64 debugging. It looks like aarch64-tdep.c
needs sve_vq_from_vl. Maybe that definition could be moved to arch/, which can be
included in aarch64-tdep.c.
Then, is the _aarch64_ctx structure guaranteed to be defined on older AArch64 kernels
or should we include it too?
On 2018-06-06 11:16 AM, Alan Hayward wrote:
> A purely virtual class containing functions from gdb/regcache.h
>
> Both the gdb regcache structures and gdbserver regcache inherit
> directly from reg_buffer_common. This will allow for common
> functions which require the use of a regcache.
>
> 2018-06-06 Alan Hayward <alan.hayward@arm.com>
>
> gdb/
> * common/common-regcache.h (reg_buffer_common): New structure.
> * regcache.h (reg_buffer_common:get_register_status) Add override.
> (reg_buffer_common:raw_supply): Add dummy function.
> reg_buffer_common:raw_collect): Likewise
> (readable_regcache:raw_supply) Add override.
> (detached_regcache:raw_collect) Likewise.
>
> gdbserver/
> * regcache.c (new_register_cache): Use new.
> (free_register_cache): Use delete.
> register_data): Use const.
> (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.h (regcache:raw_supply): Add dummy function.
> (regcache:raw_collect): Likewise
> (regcache:get_register_status) Likewise.
> ---
> gdb/common/common-regcache.h | 8 ++++++++
> gdb/gdbserver/regcache.c | 47 ++++++++++++++++++++++++++++++++------------
> gdb/gdbserver/regcache.h | 18 +++++++++++------
> gdb/regcache.h | 19 ++++++++++++++----
> 4 files changed, 69 insertions(+), 23 deletions(-)
>
> diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h
> index 696ba00955..487da0a7fb 100644
> --- a/gdb/common/common-regcache.h
> +++ b/gdb/common/common-regcache.h
> @@ -62,4 +62,12 @@ extern enum register_status regcache_raw_read_unsigned
>
> ULONGEST regcache_raw_get_unsigned (struct regcache *regcache, int regnum);
>
> +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 register_status get_register_status (int regnum) const = 0;
> +};
Ideally, we would gather the documentation for these methods here. Where they
are implemented/overriden, we can maybe add a reference such as
/* See struct reg_buffer_common. */
?
> diff --git a/gdb/regcache.h b/gdb/regcache.h
> index 3edddf47e1..b559a10752 100644
> --- a/gdb/regcache.h
> +++ b/gdb/regcache.h
> @@ -139,7 +139,7 @@ typedef struct cached_reg
>
> /* Buffer of registers. */
>
> -class reg_buffer
> +class reg_buffer : public reg_buffer_common
> {
> public:
> reg_buffer (gdbarch *gdbarch, bool has_pseudo);
> @@ -151,13 +151,24 @@ public:
>
> /* Get the availability status of the value of register REGNUM in this
> buffer. */
> - enum register_status get_register_status (int regnum) const;
> + enum register_status get_register_status (int regnum) const override;
>
> virtual ~reg_buffer ()
> {
> xfree (m_registers);
> xfree (m_register_status);
> }
> +
> + virtual void raw_supply (int regnum, const void *buf) override
> + {
> + gdb_assert (false);
> + }
> +
> + virtual void raw_collect (int regnum, void *buf) const override
> + {
> + gdb_assert (false);
> + }
Hmm, I understand why you need to do this right now. But what do you think of the
idea of moving the supply and collect implementations up to reg_buffer? I think
that the supply/collect operations are a good fit to go in reg_buffer. Essentially
they just peek/poke in the buffer. The regcache layer's responsibility is then to
use that register buffer to implement a cache in front of the target registers,
and offer the API to properly read/write registers (including pseudo ones).
For reference here's the patch in the regcache-for-alan branch that did this:
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=0e04bb35765d2717818dddd88328cb975f417b2c;hp=ca9f66e37913be55abaea44813a768b40673a39a
Simon
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 03/10] Add reg_buffer_common
2018-06-07 20:19 ` Simon Marchi
@ 2018-06-07 20:42 ` Simon Marchi
2018-06-08 14:14 ` Alan Hayward
1 sibling, 0 replies; 56+ messages in thread
From: Simon Marchi @ 2018-06-07 20:42 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
On 2018-06-07 04:18 PM, Simon Marchi wrote:
> Hi Alan,
>
> Just some quick comments.
>
> I get this when building on x86-64 with --enable-targets=all:
>
> CXX aarch64-tdep.o
> In file included from /home/emaisin/src/binutils-gdb/gdb/nat/aarch64-sve-linux-ptrace.h:29:0,
> from /home/emaisin/src/binutils-gdb/gdb/aarch64-tdep.c:61:
> /home/emaisin/src/binutils-gdb/gdb/nat/aarch64-linux-sigcontext.h:19:22: error: field ‘head’ has incomplete type ‘_aarch64_ctx’
> struct _aarch64_ctx head;
> ^
> /home/emaisin/src/binutils-gdb/gdb/nat/aarch64-linux-sigcontext.h:19:9: note: forward declaration of ‘struct _aarch64_ctx’
> struct _aarch64_ctx head;
> ^
>
> First, we should not include "nat/aarch64-sve-linux-ptrace.h" (a file that only makes
> sense when building on AArch64) in aarch64-tdep.c, a file built on all architecture
> when including the support for AArch64 debugging. It looks like aarch64-tdep.c
> needs sve_vq_from_vl. Maybe that definition could be moved to arch/, which can be
> included in aarch64-tdep.c.
>
> Then, is the _aarch64_ctx structure guaranteed to be defined on older AArch64 kernels
> or should we include it too?
Sorry, I mixed things up, this feedback is about patch 2/10 actually. The other
comments apply to this patch (3/10).
Simon
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 03/10] Add reg_buffer_common
2018-06-07 20:19 ` Simon Marchi
2018-06-07 20:42 ` Simon Marchi
@ 2018-06-08 14:14 ` Alan Hayward
2018-06-10 22:21 ` Simon Marchi
1 sibling, 1 reply; 56+ messages in thread
From: Alan Hayward @ 2018-06-08 14:14 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches, nd
> On 7 Jun 2018, at 21:18, Simon Marchi <simon.marchi@ericsson.com> wrote:
>
> Hi Alan,
>
> Just some quick comments.
>
<snip - moved to 02 thread>
>> ---
>> gdb/common/common-regcache.h | 8 ++++++++
>> gdb/gdbserver/regcache.c | 47 ++++++++++++++++++++++++++++++++------------
>> gdb/gdbserver/regcache.h | 18 +++++++++++------
>> gdb/regcache.h | 19 ++++++++++++++----
>> 4 files changed, 69 insertions(+), 23 deletions(-)
>>
>> diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h
>> index 696ba00955..487da0a7fb 100644
>> --- a/gdb/common/common-regcache.h
>> +++ b/gdb/common/common-regcache.h
>> @@ -62,4 +62,12 @@ extern enum register_status regcache_raw_read_unsigned
>>
>> ULONGEST regcache_raw_get_unsigned (struct regcache *regcache, int regnum);
>>
>> +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 register_status get_register_status (int regnum) const = 0;
>> +};
>
> Ideally, we would gather the documentation for these methods here. Where they
> are implemented/overriden, we can maybe add a reference such as
>
> /* See struct reg_buffer_common. */
>
> ?
Agreed. Updated all the comments for all the moved functions.
I noticed the comment for transfer_regset had become detached from the function,
so I move it back to the right place.
>
>> diff --git a/gdb/regcache.h b/gdb/regcache.h
>> index 3edddf47e1..b559a10752 100644
>> --- a/gdb/regcache.h
>> +++ b/gdb/regcache.h
>> @@ -139,7 +139,7 @@ typedef struct cached_reg
>>
>> /* Buffer of registers. */
>>
>> -class reg_buffer
>> +class reg_buffer : public reg_buffer_common
>> {
>> public:
>> reg_buffer (gdbarch *gdbarch, bool has_pseudo);
>> @@ -151,13 +151,24 @@ public:
>>
>> /* Get the availability status of the value of register REGNUM in this
>> buffer. */
>> - enum register_status get_register_status (int regnum) const;
>> + enum register_status get_register_status (int regnum) const override;
>>
>> virtual ~reg_buffer ()
>> {
>> xfree (m_registers);
>> xfree (m_register_status);
>> }
>> +
>> + virtual void raw_supply (int regnum, const void *buf) override
>> + {
>> + gdb_assert (false);
>> + }
>> +
>> + virtual void raw_collect (int regnum, void *buf) const override
>> + {
>> + gdb_assert (false);
>> + }
>
> Hmm, I understand why you need to do this right now. But what do you think of the
> idea of moving the supply and collect implementations up to reg_buffer? I think
> that the supply/collect operations are a good fit to go in reg_buffer. Essentially
> they just peek/poke in the buffer. The regcache layer's responsibility is then to
> use that register buffer to implement a cache in front of the target registers,
> and offer the API to properly read/write registers (including pseudo ones).
>
> For reference here's the patch in the regcache-for-alan branch that did this:
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=0e04bb35765d2717818dddd88328cb975f417b2c;hp=ca9f66e37913be55abaea44813a768b40673a39a
>
I’m happy with that. I hadn’t that there was no methods for copying in and out
of reg_buffer. Your change improves that.
Ok, so updated with changes as above. Are you ok with this version?
diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h
index 696ba00955bcdec85ab2a1abd7e030cd10418524..29d9a81182ad1a5797b080e136b682fe59ecef37 100644
--- a/gdb/common/common-regcache.h
+++ b/gdb/common/common-regcache.h
@@ -62,4 +62,19 @@ extern enum register_status regcache_raw_read_unsigned
ULONGEST regcache_raw_get_unsigned (struct regcache *regcache, int regnum);
+struct reg_buffer_common
+{
+ virtual ~reg_buffer_common () = default;
+
+ /* Get the availability status of the value of register REGNUM in this
+ buffer. */
+ virtual register_status get_register_status (int regnum) const = 0;
+
+ /* Supply register REGNUM, whose contents are stored in BUF, to REGCACHE. */
+ virtual void raw_supply (int regnum, const void *buf) = 0;
+
+ /* Collect register REGNUM from REGCACHE and store its contents in BUF. */
+ virtual void raw_collect (int regnum, void *buf) const = 0;
+};
+
#endif /* COMMON_REGCACHE_H */
diff --git a/gdb/gdbserver/regcache.h b/gdb/gdbserver/regcache.h
index 2c0df648f6d439517b56844d00b1bf0a1c9eafd1..352c1df3f9eeacd622a3d5d668ff4ea98aea9c6f 100644
--- a/gdb/gdbserver/regcache.h
+++ b/gdb/gdbserver/regcache.h
@@ -28,23 +28,32 @@ struct target_desc;
inferior; this is primarily for simplicity, as the performance
benefit is minimal. */
-struct regcache
+struct regcache : public reg_buffer_common
{
/* The regcache's target description. */
- const struct target_desc *tdesc;
+ const struct target_desc *tdesc = nullptr;
/* Whether the REGISTERS buffer's contents are valid. If false, we
haven't fetched the registers from the target yet. Not that this
register cache is _not_ pass-through, unlike GDB's. Note that
"valid" here is unrelated to whether the registers are available
in a traceframe. For that, check REGISTER_STATUS below. */
- int registers_valid;
- int registers_owned;
- unsigned char *registers;
+ int registers_valid = 0;
+ int registers_owned = 0;
+ unsigned char *registers = nullptr;
#ifndef IN_PROCESS_AGENT
/* One of REG_UNAVAILBLE or REG_VALID. */
- unsigned char *register_status;
+ unsigned char *register_status = nullptr;
#endif
+
+ /* See common/common-regcache.h. */
+ enum register_status get_register_status (int regnum) const override;
+
+ /* See common/common-regcache.h. */
+ void raw_supply (int regnum, const void *buf) override;
+
+ /* See common/common-regcache.h. */
+ void raw_collect (int regnum, void *buf) const override;
};
struct regcache *init_register_cache (struct regcache *regcache,
diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
index 0718b9f9a04d58577f71ab14887e18dc05fee018..83837525a18c1dc96b6e9d24397064e247ced459 100644
--- a/gdb/gdbserver/regcache.c
+++ b/gdb/gdbserver/regcache.c
@@ -159,7 +159,7 @@ init_register_cache (struct regcache *regcache,
struct regcache *
new_register_cache (const struct target_desc *tdesc)
{
- struct regcache *regcache = XCNEW (struct regcache);
+ struct regcache *regcache = new struct regcache;
gdb_assert (tdesc->registers_size != 0);
@@ -174,7 +174,7 @@ free_register_cache (struct regcache *regcache)
if (regcache->registers_owned)
free (regcache->registers);
free (regcache->register_status);
- free (regcache);
+ delete regcache;
}
}
@@ -300,35 +300,37 @@ 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);
}
-/* Supply register N, whose contents are stored in BUF, to REGCACHE.
- If BUF is NULL, the register's value is recorded as
- unavailable. */
-
void
supply_register (struct regcache *regcache, int n, const void *buf)
+{
+ return regcache->raw_supply (n, buf);
+}
+
+/* See common/common-regcache.h. */
+
+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,8 +412,15 @@ 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);
+}
+
+/* See common/common-regcache.h. */
+
+void
+regcache::raw_collect (int n, void *buf) const
+{
+ memcpy (buf, register_data (this, n, 1), register_size (tdesc, n));
}
enum register_status
@@ -480,3 +489,16 @@ regcache_write_pc (struct regcache *regcache, CORE_ADDR pc)
}
#endif
+
+/* See common/common-regcache.h. */
+
+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
+}
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 3edddf47e12f8e9ecb6528bdac061d4135df2d5b..4e5659b25bc530a4ced32520825136534be2e642 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -139,7 +139,7 @@ typedef struct cached_reg
/* Buffer of registers. */
-class reg_buffer
+class reg_buffer : public reg_buffer_common
{
public:
reg_buffer (gdbarch *gdbarch, bool has_pseudo);
@@ -149,9 +149,42 @@ public:
/* Return regcache's architecture. */
gdbarch *arch () const;
- /* Get the availability status of the value of register REGNUM in this
- buffer. */
- enum register_status get_register_status (int regnum) const;
+ /* See common/common-regcache.h. */
+ enum register_status get_register_status (int regnum) const override;
+
+ /* See common/common-regcache.h. */
+ void raw_collect (int regnum, void *buf) const override;
+
+ /* Collect register REGNUM from REGCACHE. Store collected value as an integer
+ at address ADDR, in target endian, with length ADDR_LEN and sign IS_SIGNED.
+ If ADDR_LEN is greater than the register size, then the integer will be
+ sign or zero extended. If ADDR_LEN is smaller than the register size, then
+ the most significant bytes of the integer will be truncated. */
+ void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
+ bool is_signed) const;
+
+ /* See common/common-regcache.h. */
+ void raw_supply (int regnum, const void *buf) override;
+
+ void raw_supply (int regnum, const reg_buffer &src)
+ {
+ raw_supply (regnum, src.register_buffer (regnum));
+ }
+
+ /* Supply register REGNUM to REGCACHE. Value to supply is an integer stored
+ at address ADDR, in target endian, with length ADDR_LEN and sign IS_SIGNED.
+ If the register size is greater than ADDR_LEN, then the integer will be
+ sign or zero extended. If the register size is smaller than the integer,
+ then the most significant bytes of the integer will be truncated. */
+ void raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len,
+ bool is_signed);
+
+ /* Supply register REGNUM with zeroed value to REGCACHE. This is not the same
+ as calling raw_supply with NULL (which will set the state to
+ unavailable). */
+ void raw_supply_zeroed (int regnum);
+
+ void invalidate (int regnum);
virtual ~reg_buffer ()
{
@@ -234,24 +267,9 @@ public:
: readable_regcache (gdbarch, has_pseudo)
{}
- /* Supply register REGNUM, whose contents are stored in BUF, to REGCACHE. */
- void raw_supply (int regnum, const void *buf);
-
- void raw_supply (int regnum, const reg_buffer &src)
- {
- raw_supply (regnum, src.register_buffer (regnum));
- }
-
void raw_update (int regnum) override
{}
- void raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len,
- bool is_signed);
-
- void raw_supply_zeroed (int regnum);
-
- void invalidate (int regnum);
-
DISABLE_COPY_AND_ASSIGN (detached_regcache);
};
@@ -292,12 +310,6 @@ public:
void raw_update (int regnum) override;
- /* Collect register REGNUM from REGCACHE and store its contents in BUF. */
- void raw_collect (int regnum, void *buf) const;
-
- void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
- bool is_signed) const;
-
/* Partial transfer of raw registers. Perform read, modify, write style
operations. */
void raw_write_part (int regnum, int offset, int len, const gdb_byte *buf);
diff --git a/gdb/regcache.c b/gdb/regcache.c
index a914b548cadce1d375f5d72b0936b2258b90e08f..032fef0d34ac635c96dd6c39426f5c6d04f28095 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -320,6 +320,8 @@ regcache::restore (readonly_detached_regcache *src)
}
}
+/* See common/common-regcache.h. */
+
enum register_status
reg_buffer::get_register_status (int regnum) const
{
@@ -329,7 +331,7 @@ reg_buffer::get_register_status (int regnum) const
}
void
-detached_regcache::invalidate (int regnum)
+reg_buffer::invalidate (int regnum)
{
assert_regnum (regnum);
m_register_status[regnum] = REG_UNKNOWN;
@@ -879,8 +881,10 @@ regcache::cooked_write_part (int regnum, int offset, int len,
write_part (regnum, offset, len, buf, false);
}
+/* See common/common-regcache.h. */
+
void
-detached_regcache::raw_supply (int regnum, const void *buf)
+reg_buffer::raw_supply (int regnum, const void *buf)
{
void *regbuf;
size_t size;
@@ -905,15 +909,11 @@ detached_regcache::raw_supply (int regnum, const void *buf)
}
}
-/* Supply register REGNUM to REGCACHE. Value to supply is an integer stored at
- address ADDR, in target endian, with length ADDR_LEN and sign IS_SIGNED. If
- the register size is greater than ADDR_LEN, then the integer will be sign or
- zero extended. If the register size is smaller than the integer, then the
- most significant bytes of the integer will be truncated. */
+/* See regcache.h. */
void
-detached_regcache::raw_supply_integer (int regnum, const gdb_byte *addr,
- int addr_len, bool is_signed)
+reg_buffer::raw_supply_integer (int regnum, const gdb_byte *addr,
+ int addr_len, bool is_signed)
{
enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
gdb_byte *regbuf;
@@ -929,12 +929,10 @@ detached_regcache::raw_supply_integer (int regnum, const gdb_byte *addr,
m_register_status[regnum] = REG_VALID;
}
-/* Supply register REGNUM with zeroed value to REGCACHE. This is not the same
- as calling raw_supply with NULL (which will set the state to
- unavailable). */
+/* See regcache.h. */
void
-detached_regcache::raw_supply_zeroed (int regnum)
+reg_buffer::raw_supply_zeroed (int regnum)
{
void *regbuf;
size_t size;
@@ -948,8 +946,10 @@ detached_regcache::raw_supply_zeroed (int regnum)
m_register_status[regnum] = REG_VALID;
}
+/* See common/common-regcache.h. */
+
void
-regcache::raw_collect (int regnum, void *buf) const
+reg_buffer::raw_collect (int regnum, void *buf) const
{
const void *regbuf;
size_t size;
@@ -962,19 +962,11 @@ regcache::raw_collect (int regnum, void *buf) const
memcpy (buf, regbuf, size);
}
-/* Transfer a single or all registers belonging to a certain register
- set to or from a buffer. This is the main worker function for
- regcache_supply_regset and regcache_collect_regset. */
-
-/* Collect register REGNUM from REGCACHE. Store collected value as an integer
- at address ADDR, in target endian, with length ADDR_LEN and sign IS_SIGNED.
- If ADDR_LEN is greater than the register size, then the integer will be sign
- or zero extended. If ADDR_LEN is smaller than the register size, then the
- most significant bytes of the integer will be truncated. */
+/* See regcache.h. */
void
-regcache::raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
- bool is_signed) const
+reg_buffer::raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
+ bool is_signed) const
{
enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
const gdb_byte *regbuf;
@@ -989,6 +981,10 @@ regcache::raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
byte_order);
}
+/* Transfer a single or all registers belonging to a certain register
+ set to or from a buffer. This is the main worker function for
+ regcache_supply_regset and regcache_collect_regset. */
+
void
regcache::transfer_regset (const struct regset *regset,
struct regcache *out_regcache,
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 03/10] Add reg_buffer_common
2018-06-08 14:14 ` Alan Hayward
@ 2018-06-10 22:21 ` Simon Marchi
0 siblings, 0 replies; 56+ messages in thread
From: Simon Marchi @ 2018-06-10 22:21 UTC (permalink / raw)
To: Alan Hayward, Simon Marchi; +Cc: gdb-patches, nd
On 2018-06-08 10:14 AM, Alan Hayward wrote:
>
>> On 7 Jun 2018, at 21:18, Simon Marchi <simon.marchi@ericsson.com> wrote:
>>
>> Hi Alan,
>>
>> Just some quick comments.
>>
>
> <snip - moved to 02 thread>
>
>>> ---
>>> gdb/common/common-regcache.h | 8 ++++++++
>>> gdb/gdbserver/regcache.c | 47 ++++++++++++++++++++++++++++++++------------
>>> gdb/gdbserver/regcache.h | 18 +++++++++++------
>>> gdb/regcache.h | 19 ++++++++++++++----
>>> 4 files changed, 69 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h
>>> index 696ba00955..487da0a7fb 100644
>>> --- a/gdb/common/common-regcache.h
>>> +++ b/gdb/common/common-regcache.h
>>> @@ -62,4 +62,12 @@ extern enum register_status regcache_raw_read_unsigned
>>>
>>> ULONGEST regcache_raw_get_unsigned (struct regcache *regcache, int regnum);
>>>
>>> +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 register_status get_register_status (int regnum) const = 0;
>>> +};
>>
>> Ideally, we would gather the documentation for these methods here. Where they
>> are implemented/overriden, we can maybe add a reference such as
>>
>> /* See struct reg_buffer_common. */
>>
>> ?
>
> Agreed. Updated all the comments for all the moved functions.
>
> I noticed the comment for transfer_regset had become detached from the function,
> so I move it back to the right place.
>
>>
>>> diff --git a/gdb/regcache.h b/gdb/regcache.h
>>> index 3edddf47e1..b559a10752 100644
>>> --- a/gdb/regcache.h
>>> +++ b/gdb/regcache.h
>>> @@ -139,7 +139,7 @@ typedef struct cached_reg
>>>
>>> /* Buffer of registers. */
>>>
>>> -class reg_buffer
>>> +class reg_buffer : public reg_buffer_common
>>> {
>>> public:
>>> reg_buffer (gdbarch *gdbarch, bool has_pseudo);
>>> @@ -151,13 +151,24 @@ public:
>>>
>>> /* Get the availability status of the value of register REGNUM in this
>>> buffer. */
>>> - enum register_status get_register_status (int regnum) const;
>>> + enum register_status get_register_status (int regnum) const override;
>>>
>>> virtual ~reg_buffer ()
>>> {
>>> xfree (m_registers);
>>> xfree (m_register_status);
>>> }
>>> +
>>> + virtual void raw_supply (int regnum, const void *buf) override
>>> + {
>>> + gdb_assert (false);
>>> + }
>>> +
>>> + virtual void raw_collect (int regnum, void *buf) const override
>>> + {
>>> + gdb_assert (false);
>>> + }
>>
>> Hmm, I understand why you need to do this right now. But what do you think of the
>> idea of moving the supply and collect implementations up to reg_buffer? I think
>> that the supply/collect operations are a good fit to go in reg_buffer. Essentially
>> they just peek/poke in the buffer. The regcache layer's responsibility is then to
>> use that register buffer to implement a cache in front of the target registers,
>> and offer the API to properly read/write registers (including pseudo ones).
>>
>> For reference here's the patch in the regcache-for-alan branch that did this:
>> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=0e04bb35765d2717818dddd88328cb975f417b2c;hp=ca9f66e37913be55abaea44813a768b40673a39a
>>
>
> I’m happy with that. I hadn’t that there was no methods for copying in and out
> of reg_buffer. Your change improves that.
>
> Ok, so updated with changes as above. Are you ok with this version?
Yes, that LGTM, thanks.
Simon
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 04/10] Add regcache raw_compare method
2018-06-06 15:16 [PATCH v2 00/10] gdb/gdbserver support for aarch64 SVE Alan Hayward
2018-06-06 15:17 ` [PATCH v2 03/10] Add reg_buffer_common Alan Hayward
@ 2018-06-06 15:17 ` Alan Hayward
2018-06-07 20:56 ` Simon Marchi
2018-06-06 15:17 ` [PATCH v2 02/10] Add Aarch64 SVE Linux headers Alan Hayward
` (7 subsequent siblings)
9 siblings, 1 reply; 56+ messages in thread
From: Alan Hayward @ 2018-06-06 15:17 UTC (permalink / raw)
To: gdb-patches; +Cc: nd, Alan Hayward
raw_compare returns the same as a memcmp (0 for success,
the diff otherwise). Kept with tristate return as this
feels potentally more useful than a simple true/false return.
(Happy to change if not).
2018-06-06 Alan Hayward <alan.hayward@arm.com>
gdb/
* common/common-regcache.h (raw_compare): New function.
* regcache.c (regcache::raw_compare): Likewise.
* regcache.h (regcache::raw_compare): New declaration.
gdbserver/
* regcache.c (regcache::raw_compare): New function.
* regcache.h (regcache::raw_compare): New declaration.
---
gdb/common/common-regcache.h | 1 +
gdb/gdbserver/regcache.c | 10 ++++++++++
gdb/gdbserver/regcache.h | 5 +++++
gdb/regcache.c | 15 +++++++++++++++
gdb/regcache.h | 11 +++++++++++
5 files changed, 42 insertions(+)
diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h
index 487da0a7fb..e91439bec5 100644
--- a/gdb/common/common-regcache.h
+++ b/gdb/common/common-regcache.h
@@ -67,6 +67,7 @@ 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 int raw_compare (int regnum, const void *buf, int offset) const = 0;
virtual register_status get_register_status (int regnum) const = 0;
};
diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
index 857721ee3d..9648428349 100644
--- a/gdb/gdbserver/regcache.c
+++ b/gdb/gdbserver/regcache.c
@@ -501,3 +501,13 @@ regcache::get_register_status (int regnum) const
return REG_VALID;
#endif
}
+
+/* See gdbserver/regcache.h. */
+
+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 1842f1d9cf..b26f39a8ad 100644
--- a/gdb/gdbserver/regcache.h
+++ b/gdb/gdbserver/regcache.h
@@ -50,6 +50,11 @@ struct regcache : public reg_buffer_common
void raw_collect (int regnum, void *buf) const override;
+ /* Compare the contents of the register stored in the regcache (ignoring the
+ first OFFSET bytes) to the contents of BUF (without any offset). Returns
+ the same result as memcmp. */
+ int raw_compare (int regnum, const void *buf, int offset) const override;
+
enum register_status get_register_status (int regnum) const override;
};
diff --git a/gdb/regcache.c b/gdb/regcache.c
index a914b548ca..383e355e9f 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1082,6 +1082,21 @@ regcache::collect_regset (const struct regset *regset,
transfer_regset (regset, NULL, regnum, NULL, buf, size);
}
+/* See regcache.h. */
+
+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 b559a10752..03fb4f2452 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -169,6 +169,12 @@ public:
gdb_assert (false);
}
+ virtual int raw_compare (int regnum, const void *buf,
+ int offset) const override
+ {
+ gdb_assert (false);
+ }
+
protected:
/* Assert on the range of REGNUM. */
void assert_regnum (int regnum) const;
@@ -325,6 +331,11 @@ public:
void collect_regset (const struct regset *regset, int regnum,
void *buf, size_t size) const;
+ /* Compare the contents of the register stored in the regcache (ignoring the
+ first OFFSET bytes) to the contents of BUF (without any offset). Returns
+ the same result as memcmp. */
+ int raw_compare (int regnum, const void *buf, int offset) const override;
+
/* Return REGCACHE's ptid. */
ptid_t ptid () const
--
2.15.1 (Apple Git-101)
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 04/10] Add regcache raw_compare method
2018-06-06 15:17 ` [PATCH v2 04/10] Add regcache raw_compare method Alan Hayward
@ 2018-06-07 20:56 ` Simon Marchi
2018-06-08 15:16 ` Alan Hayward
0 siblings, 1 reply; 56+ messages in thread
From: Simon Marchi @ 2018-06-07 20:56 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
On 2018-06-06 11:16 AM, Alan Hayward wrote:
> raw_compare returns the same as a memcmp (0 for success,
> the diff otherwise). Kept with tristate return as this
> feels potentally more useful than a simple true/false return.
> (Happy to change if not).
I would err on the bool side, but I don't really mind, the important is
that it's documented properly. We still use many ints as bool in GDB, so
someone seeing an int return value could easily think it's 0 -> not equal,
1 -> equal.
Speaking of doc, I would suggest (as in the previous patch) to centralize
the doc in the class/struct at the root, so we don't duplicate it.
> 2018-06-06 Alan Hayward <alan.hayward@arm.com>
>
> gdb/
> * common/common-regcache.h (raw_compare): New function.
> * regcache.c (regcache::raw_compare): Likewise.
> * regcache.h (regcache::raw_compare): New declaration.
>
> gdbserver/
> * regcache.c (regcache::raw_compare): New function.
> * regcache.h (regcache::raw_compare): New declaration.
> ---
> gdb/common/common-regcache.h | 1 +
> gdb/gdbserver/regcache.c | 10 ++++++++++
> gdb/gdbserver/regcache.h | 5 +++++
> gdb/regcache.c | 15 +++++++++++++++
> gdb/regcache.h | 11 +++++++++++
> 5 files changed, 42 insertions(+)
>
> diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h
> index 487da0a7fb..e91439bec5 100644
> --- a/gdb/common/common-regcache.h
> +++ b/gdb/common/common-regcache.h
> @@ -67,6 +67,7 @@ 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 int raw_compare (int regnum, const void *buf, int offset) const = 0;
> virtual register_status get_register_status (int regnum) const = 0;
> };
>
> diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
> index 857721ee3d..9648428349 100644
> --- a/gdb/gdbserver/regcache.c
> +++ b/gdb/gdbserver/regcache.c
> @@ -501,3 +501,13 @@ regcache::get_register_status (int regnum) const
> return REG_VALID;
> #endif
> }
> +
> +/* See gdbserver/regcache.h. */
> +
> +int
> +regcache::raw_compare (int regnum, const void *buf, int offset) const
> +{
> + gdb_assert (register_size (tdesc, regnum) > offset);
Should we check ">= offset"? I think it could be useful for some edge cases
where we could avoid an "if (offset < size)" in the caller, if we know offset
could be equal to size. memcmp would return 0 (equal), which is fine.
Maybe assign "register_size (tdesc, regnum)" to a variable (e.g. reg_size)
and use it in both places?
> + 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 1842f1d9cf..b26f39a8ad 100644
> --- a/gdb/gdbserver/regcache.h
> +++ b/gdb/gdbserver/regcache.h
> @@ -50,6 +50,11 @@ struct regcache : public reg_buffer_common
>
> void raw_collect (int regnum, void *buf) const override;
>
> + /* Compare the contents of the register stored in the regcache (ignoring the
> + first OFFSET bytes) to the contents of BUF (without any offset). Returns
> + the same result as memcmp. */
> + int raw_compare (int regnum, const void *buf, int offset) const override;
> +
> enum register_status get_register_status (int regnum) const override;
> };
>
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index a914b548ca..383e355e9f 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -1082,6 +1082,21 @@ regcache::collect_regset (const struct regset *regset,
> transfer_regset (regset, NULL, regnum, NULL, buf, size);
> }
>
> +/* See regcache.h. */
> +
> +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);
Should we assert that offset is < or <= size here too?
Simon
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 04/10] Add regcache raw_compare method
2018-06-07 20:56 ` Simon Marchi
@ 2018-06-08 15:16 ` Alan Hayward
2018-06-10 22:26 ` Simon Marchi
0 siblings, 1 reply; 56+ messages in thread
From: Alan Hayward @ 2018-06-08 15:16 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches, nd
> On 7 Jun 2018, at 21:55, Simon Marchi <simon.marchi@ericsson.com> wrote:
>
> On 2018-06-06 11:16 AM, Alan Hayward wrote:
>> raw_compare returns the same as a memcmp (0 for success,
>> the diff otherwise). Kept with tristate return as this
>> feels potentally more useful than a simple true/false return.
>> (Happy to change if not).
>
> I would err on the bool side, but I don't really mind, the important is
> that it's documented properly. We still use many ints as bool in GDB, so
> someone seeing an int return value could easily think it's 0 -> not equal,
> 1 -> equal.
Ok, updated.
>
> Speaking of doc, I would suggest (as in the previous patch) to centralize
> the doc in the class/struct at the root, so we don't duplicate it.
Done.
>
>> 2018-06-06 Alan Hayward <alan.hayward@arm.com>
>>
>> gdb/
>> * common/common-regcache.h (raw_compare): New function.
>> * regcache.c (regcache::raw_compare): Likewise.
>> * regcache.h (regcache::raw_compare): New declaration.
>>
>> gdbserver/
>> * regcache.c (regcache::raw_compare): New function.
>> * regcache.h (regcache::raw_compare): New declaration.
>> ---
>> gdb/common/common-regcache.h | 1 +
>> gdb/gdbserver/regcache.c | 10 ++++++++++
>> gdb/gdbserver/regcache.h | 5 +++++
>> gdb/regcache.c | 15 +++++++++++++++
>> gdb/regcache.h | 11 +++++++++++
>> 5 files changed, 42 insertions(+)
>>
>> diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h
>> index 487da0a7fb..e91439bec5 100644
>> --- a/gdb/common/common-regcache.h
>> +++ b/gdb/common/common-regcache.h
>> @@ -67,6 +67,7 @@ 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 int raw_compare (int regnum, const void *buf, int offset) const = 0;
>> virtual register_status get_register_status (int regnum) const = 0;
>> };
>>
>> diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
>> index 857721ee3d..9648428349 100644
>> --- a/gdb/gdbserver/regcache.c
>> +++ b/gdb/gdbserver/regcache.c
>> @@ -501,3 +501,13 @@ regcache::get_register_status (int regnum) const
>> return REG_VALID;
>> #endif
>> }
>> +
>> +/* See gdbserver/regcache.h. */
>> +
>> +int
>> +regcache::raw_compare (int regnum, const void *buf, int offset) const
>> +{
>> + gdb_assert (register_size (tdesc, regnum) > offset);
>
> Should we check ">= offset"? I think it could be useful for some edge cases
> where we could avoid an "if (offset < size)" in the caller, if we know offset
> could be equal to size. memcmp would return 0 (equal), which is fine.
>
Given that memcmp can cope with this, then ok.
(I need to make sure I update the patch that uses this function too!)
> Maybe assign "register_size (tdesc, regnum)" to a variable (e.g. reg_size)
> and use it in both places?
Done.
>
>> + 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 1842f1d9cf..b26f39a8ad 100644
>> --- a/gdb/gdbserver/regcache.h
>> +++ b/gdb/gdbserver/regcache.h
>> @@ -50,6 +50,11 @@ struct regcache : public reg_buffer_common
>>
>> void raw_collect (int regnum, void *buf) const override;
>>
>> + /* Compare the contents of the register stored in the regcache (ignoring the
>> + first OFFSET bytes) to the contents of BUF (without any offset). Returns
>> + the same result as memcmp. */
>> + int raw_compare (int regnum, const void *buf, int offset) const override;
>> +
>> enum register_status get_register_status (int regnum) const override;
>> };
>>
>> diff --git a/gdb/regcache.c b/gdb/regcache.c
>> index a914b548ca..383e355e9f 100644
>> --- a/gdb/regcache.c
>> +++ b/gdb/regcache.c
>> @@ -1082,6 +1082,21 @@ regcache::collect_regset (const struct regset *regset,
>> transfer_regset (regset, NULL, regnum, NULL, buf, size);
>> }
>>
>> +/* See regcache.h. */
>> +
>> +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);
>
> Should we assert that offset is < or <= size here too?
>
Not sure why I didn’t add this.
Patch update with changes above. Are you ok with this version?
diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h
index 29d9a81182ad1a5797b080e136b682fe59ecef37..fe3ece7ac52d60d07d718ad617f30be2f7133b5f 100644
--- a/gdb/common/common-regcache.h
+++ b/gdb/common/common-regcache.h
@@ -75,6 +75,11 @@ struct reg_buffer_common
/* Collect register REGNUM from REGCACHE and store its contents in BUF. */
virtual void raw_collect (int regnum, void *buf) const = 0;
+
+ /* Compare the contents of the register stored in the regcache (ignoring the
+ first OFFSET bytes) to the contents of BUF (without any offset). Returns
+ True if the same. */
+ virtual bool raw_compare (int regnum, const void *buf, int offset) const = 0;
};
#endif /* COMMON_REGCACHE_H */
diff --git a/gdb/gdbserver/regcache.h b/gdb/gdbserver/regcache.h
index 352c1df3f9eeacd622a3d5d668ff4ea98aea9c6f..b4c4c20ebd368f56f14af758301ff7d55fd16dae 100644
--- a/gdb/gdbserver/regcache.h
+++ b/gdb/gdbserver/regcache.h
@@ -54,6 +54,9 @@ struct regcache : public reg_buffer_common
/* See common/common-regcache.h. */
void raw_collect (int regnum, void *buf) const override;
+
+ /* See common/common-regcache.h. */
+ bool raw_compare (int regnum, const void *buf, int offset) const override;
};
struct regcache *init_register_cache (struct regcache *regcache,
diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
index 83837525a18c1dc96b6e9d24397064e247ced459..735ce7bccfc7a058f8604620928f3894d4655887 100644
--- a/gdb/gdbserver/regcache.c
+++ b/gdb/gdbserver/regcache.c
@@ -502,3 +502,17 @@ regcache::get_register_status (int regnum) const
return REG_VALID;
#endif
}
+
+/* See common/common-regcache.h. */
+
+bool
+regcache::raw_compare (int regnum, const void *buf, int offset) const
+{
+ gdb_assert (buf != NULL);
+
+ const unsigned char *regbuf = register_data (this, regnum, 1);
+ int size = register_size (tdesc, regnum);
+ gdb_assert (size >= offset);
+
+ return (memcmp (buf, regbuf + offset, size - offset) == 0);
+}
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 4e5659b25bc530a4ced32520825136534be2e642..511864858d24a7fa53047fdc247846a551b121af 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -191,6 +191,10 @@ public:
xfree (m_registers);
xfree (m_register_status);
}
+
+ /* See common/common-regcache.h. */
+ bool raw_compare (int regnum, const void *buf, int offset) const override;
+
protected:
/* Assert on the range of REGNUM. */
void assert_regnum (int regnum) const;
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 032fef0d34ac635c96dd6c39426f5c6d04f28095..fd0fd7318c8cca0083c897d2bd6ba1361dd627c0 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1078,6 +1078,20 @@ regcache::collect_regset (const struct regset *regset,
transfer_regset (regset, NULL, regnum, NULL, buf, size);
}
+/* See common/common-regcache.h. */
+
+bool
+reg_buffer::raw_compare (int regnum, const void *buf, int offset) const
+{
+ gdb_assert (buf != NULL);
+ assert_regnum (regnum);
+
+ const char *regbuf = (const char *) register_buffer (regnum);
+ size_t size = m_descr->sizeof_register[regnum];
+ gdb_assert (size >= offset);
+
+ return (memcmp (buf, regbuf + offset, size - offset) == 0);
+}
/* Special handling for register PC. */
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 04/10] Add regcache raw_compare method
2018-06-08 15:16 ` Alan Hayward
@ 2018-06-10 22:26 ` Simon Marchi
0 siblings, 0 replies; 56+ messages in thread
From: Simon Marchi @ 2018-06-10 22:26 UTC (permalink / raw)
To: Alan Hayward, Simon Marchi; +Cc: gdb-patches, nd
On 2018-06-08 11:16 AM, Alan Hayward wrote:
> diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h
> index 29d9a81182ad1a5797b080e136b682fe59ecef37..fe3ece7ac52d60d07d718ad617f30be2f7133b5f 100644
> --- a/gdb/common/common-regcache.h
> +++ b/gdb/common/common-regcache.h
> @@ -75,6 +75,11 @@ struct reg_buffer_common
>
> /* Collect register REGNUM from REGCACHE and store its contents in BUF. */
> virtual void raw_collect (int regnum, void *buf) const = 0;
> +
> + /* Compare the contents of the register stored in the regcache (ignoring the
> + first OFFSET bytes) to the contents of BUF (without any offset). Returns
> + True if the same. */
True -> true (since it's written this way in C++, unlike Python for example).
LGTM with that fixed.
Simon
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 02/10] Add Aarch64 SVE Linux headers
2018-06-06 15:16 [PATCH v2 00/10] gdb/gdbserver support for aarch64 SVE Alan Hayward
2018-06-06 15:17 ` [PATCH v2 03/10] Add reg_buffer_common Alan Hayward
2018-06-06 15:17 ` [PATCH v2 04/10] Add regcache raw_compare method Alan Hayward
@ 2018-06-06 15:17 ` Alan Hayward
2018-06-08 14:13 ` Alan Hayward
2018-06-06 15:17 ` [PATCH v2 09/10] Ptrace support for AArch64 SVE gdbsever Alan Hayward
` (6 subsequent siblings)
9 siblings, 1 reply; 56+ messages in thread
From: Alan Hayward @ 2018-06-06 15:17 UTC (permalink / raw)
To: gdb-patches; +Cc: nd, Alan Hayward
Take all the new SVE defines/structures in the Linux kernel from sigcontext.h
and ptrace.h, placing into their own files. Those parts that already existed
in gdb codebase have been moved to the new files.
Contents are copied directly without any formatting changes.
Copied from the release tag for Linux 4.17
This change ensures gdb can still be built using older Linux kernel headers.
Confirmed builds with both new and old headers.
2018-06-06 Alan Hayward <alan.hayward@arm.com>
* aarch64-tdep.c: Add include
* arch/aarch64.h (sve_vq_from_vl): Remove
(sve_vl_from_vq): Likewise.
* nat/aarch64-linux-ptrace.h: New file.
* nat/aarch64-linux-sigcontext.h: New file.
* nat/aarch64-sve-linux-ptrace.h (aarch64_sve_get_vq): Move to
new files.
(SVE_VQ_BYTES): Likewise.
(SVE_VQ_MIN): Likewise.
(SVE_VQ_MAX): Likewise.
(SVE_VL_MIN): Likewise.
(SVE_VL_MAX): Likewise.
(SVE_NUM_ZREGS): Likewise.
(SVE_NUM_PREGS): Likewise.
(sve_vl_valid): Likewise.
(struct user_sve_header): Likewise.
---
gdb/aarch64-tdep.c | 1 +
gdb/arch/aarch64.h | 5 +-
gdb/nat/aarch64-linux-ptrace.h | 150 +++++++++++++++++++++++++++++++++++++
gdb/nat/aarch64-linux-sigcontext.h | 129 +++++++++++++++++++++++++++++++
gdb/nat/aarch64-sve-linux-ptrace.h | 46 ++----------
5 files changed, 289 insertions(+), 42 deletions(-)
create mode 100644 gdb/nat/aarch64-linux-ptrace.h
create mode 100644 gdb/nat/aarch64-linux-sigcontext.h
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 2fe6a4006c..0438ab83cd 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -58,6 +58,7 @@
#include "opcode/aarch64.h"
#include <algorithm>
+#include "nat/aarch64-sve-linux-ptrace.h"
#define submask(x) ((1L << ((x) + 1)) - 1)
#define bit(obj,st) (((obj) >> (st)) & 1)
diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
index 9040d8d4c8..04e41f71df 100644
--- a/gdb/arch/aarch64.h
+++ b/gdb/arch/aarch64.h
@@ -76,11 +76,10 @@ enum aarch64_regnum
#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)))
-
+/* sve_vq_from_vl(vl) : see nat/aarch64-linux-sigcontext.h
+ sve_vl_from_vq(vq) : see nat/aarch64-linux-sigcontext.h. */
/* Maximum supported VQ value. Increase if required. */
#define AARCH64_MAX_SVE_VQ 16
diff --git a/gdb/nat/aarch64-linux-ptrace.h b/gdb/nat/aarch64-linux-ptrace.h
new file mode 100644
index 0000000000..1d0bf1b314
--- /dev/null
+++ b/gdb/nat/aarch64-linux-ptrace.h
@@ -0,0 +1,150 @@
+/* This file contains Aarch64 Linux ptrace defines. It is required for those
+ compiling with older kernel headers. Eventually, when the kernel defines are
+ old enough, this file should be removed.
+
+ Contents are directly copied directly from
+ linux/arch/arm64/include/uapi/asm/ptrace.h in Linux 4.17.
+
+ See:
+ https://github.com/torvalds/linux/blob/v4.17/arch/arm64/include/uapi/asm/ptrace.h
+*/
+
+#ifndef AARCH64_LINUX_PTRACE_H
+#define AARCH64_LINUX_PTRACE_H
+
+/* SVE/FP/SIMD state (NT_ARM_SVE) */
+
+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;
+};
+
+/* Definitions for user_sve_header.flags: */
+#define SVE_PT_REGS_MASK (1 << 0)
+
+#define SVE_PT_REGS_FPSIMD 0
+#define SVE_PT_REGS_SVE SVE_PT_REGS_MASK
+
+/*
+ * Common SVE_PT_* flags:
+ * These must be kept in sync with prctl interface in <linux/ptrace.h>
+ */
+#define SVE_PT_VL_INHERIT (PR_SVE_VL_INHERIT >> 16)
+#define SVE_PT_VL_ONEXEC (PR_SVE_SET_VL_ONEXEC >> 16)
+
+
+/*
+ * The remainder of the SVE state follows struct user_sve_header. The
+ * total size of the SVE state (including header) depends on the
+ * metadata in the header: SVE_PT_SIZE(vq, flags) gives the total size
+ * of the state in bytes, including the header.
+ *
+ * Refer to <asm/sigcontext.h> for details of how to pass the correct
+ * "vq" argument to these macros.
+ */
+
+/* Offset from the start of struct user_sve_header to the register data */
+#define SVE_PT_REGS_OFFSET \
+ ((sizeof(struct sve_context) + (SVE_VQ_BYTES - 1)) \
+ / SVE_VQ_BYTES * SVE_VQ_BYTES)
+
+/*
+ * The register data content and layout depends on the value of the
+ * flags field.
+ */
+
+/*
+ * (flags & SVE_PT_REGS_MASK) == SVE_PT_REGS_FPSIMD case:
+ *
+ * The payload starts at offset SVE_PT_FPSIMD_OFFSET, and is of type
+ * struct user_fpsimd_state. Additional data might be appended in the
+ * future: use SVE_PT_FPSIMD_SIZE(vq, flags) to compute the total size.
+ * SVE_PT_FPSIMD_SIZE(vq, flags) will never be less than
+ * sizeof(struct user_fpsimd_state).
+ */
+
+#define SVE_PT_FPSIMD_OFFSET SVE_PT_REGS_OFFSET
+
+#define SVE_PT_FPSIMD_SIZE(vq, flags) (sizeof(struct user_fpsimd_state))
+
+/*
+ * (flags & SVE_PT_REGS_MASK) == SVE_PT_REGS_SVE case:
+ *
+ * The payload starts at offset SVE_PT_SVE_OFFSET, and is of size
+ * SVE_PT_SVE_SIZE(vq, flags).
+ *
+ * Additional macros describe the contents and layout of the payload.
+ * For each, SVE_PT_SVE_x_OFFSET(args) is the start offset relative to
+ * the start of struct user_sve_header, and SVE_PT_SVE_x_SIZE(args) is
+ * the size in bytes:
+ *
+ * x type description
+ * - ---- -----------
+ * ZREGS \
+ * ZREG |
+ * PREGS | refer to <asm/sigcontext.h>
+ * PREG |
+ * FFR /
+ *
+ * FPSR uint32_t FPSR
+ * FPCR uint32_t FPCR
+ *
+ * Additional data might be appended in the future.
+ */
+
+#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)
+
+/*
+ * Any future extension appended after FPCR must be aligned to the next
+ * 128-bit boundary.
+ */
+
+#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 /* AARCH64_LINUX_PTRACE_H */
diff --git a/gdb/nat/aarch64-linux-sigcontext.h b/gdb/nat/aarch64-linux-sigcontext.h
new file mode 100644
index 0000000000..5dc1b7db3f
--- /dev/null
+++ b/gdb/nat/aarch64-linux-sigcontext.h
@@ -0,0 +1,129 @@
+/* This file contains Aarch64 Linux sigcontext defines. It is required for those
+ compiling with older kernel headers. Eventually, when the kernel defines are
+ old enough, this file should be removed.
+
+ Contents are directly copied directly from
+ linux/arch/arm64/include/uapi/asm/sigcontext.h in Linux 4.17.
+
+ See:
+ https://github.com/torvalds/linux/blob/v4.17/arch/arm64/include/uapi/asm/sigcontext.h
+*/
+
+#ifndef AARCH64_LINUX_SIGCONTEXT_H
+#define AARCH64_LINUX_SIGCONTEXT_H
+
+
+#define SVE_MAGIC 0x53564501
+
+struct sve_context {
+ struct _aarch64_ctx head;
+ __u16 vl;
+ __u16 __reserved[3];
+};
+
+/*
+ * The SVE architecture leaves space for future expansion of the
+ * vector length beyond its initial architectural limit of 2048 bits
+ * (16 quadwords).
+ *
+ * See linux/Documentation/arm64/sve.txt for a description of the VL/VQ
+ * terminology.
+ */
+#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)
+#define sve_vq_from_vl(vl) ((vl) / SVE_VQ_BYTES)
+#define sve_vl_from_vq(vq) ((vq) * SVE_VQ_BYTES)
+
+/*
+ * If the SVE registers are currently live for the thread at signal delivery,
+ * sve_context.head.size >=
+ * SVE_SIG_CONTEXT_SIZE(sve_vq_from_vl(sve_context.vl))
+ * and the register data may be accessed using the SVE_SIG_*() macros.
+ *
+ * If sve_context.head.size <
+ * SVE_SIG_CONTEXT_SIZE(sve_vq_from_vl(sve_context.vl)),
+ * the SVE registers were not live for the thread and no register data
+ * is included: in this case, the SVE_SIG_*() macros should not be
+ * used except for this check.
+ *
+ * The same convention applies when returning from a signal: a caller
+ * will need to remove or resize the sve_context block if it wants to
+ * make the SVE registers live when they were previously non-live or
+ * vice-versa. This may require the the caller to allocate fresh
+ * memory and/or move other context blocks in the signal frame.
+ *
+ * Changing the vector length during signal return is not permitted:
+ * sve_context.vl must equal the thread's current vector length when
+ * doing a sigreturn.
+ *
+ *
+ * Note: for all these macros, the "vq" argument denotes the SVE
+ * vector length in quadwords (i.e., units of 128 bits).
+ *
+ * The correct way to obtain vq is to use sve_vq_from_vl(vl). The
+ * result is valid if and only if sve_vl_valid(vl) is true. This is
+ * guaranteed for a struct sve_context written by the kernel.
+ *
+ *
+ * Additional macros describe the contents and layout of the payload.
+ * For each, SVE_SIG_x_OFFSET(args) is the start offset relative to
+ * the start of struct sve_context, and SVE_SIG_x_SIZE(args) is the
+ * size in bytes:
+ *
+ * x type description
+ * - ---- -----------
+ * REGS the entire SVE context
+ *
+ * ZREGS __uint128_t[SVE_NUM_ZREGS][vq] all Z-registers
+ * ZREG __uint128_t[vq] individual Z-register Zn
+ *
+ * PREGS uint16_t[SVE_NUM_PREGS][vq] all P-registers
+ * PREG uint16_t[vq] individual P-register Pn
+ *
+ * FFR uint16_t[vq] first-fault status register
+ *
+ * Additional data might be appended in the future.
+ */
+
+#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 /* AARCH64_LINUX_SIGCONTEXT_H */
diff --git a/gdb/nat/aarch64-sve-linux-ptrace.h b/gdb/nat/aarch64-sve-linux-ptrace.h
index 61f841466c..2d6f5714c0 100644
--- a/gdb/nat/aarch64-sve-linux-ptrace.h
+++ b/gdb/nat/aarch64-sve-linux-ptrace.h
@@ -20,54 +20,22 @@
#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 zero). */
-
-uint64_t 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. */
+#include "aarch64-linux-sigcontext.h"
+#endif
#ifndef SVE_PT_SVE_ZREG_SIZE
+#include "aarch64-linux-ptrace.h"
+#endif
-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;
-};
+/* 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 zero). */
-#endif /* SVE_PT_SVE_ZREG_SIZE. */
+uint64_t aarch64_sve_get_vq (int tid);
#endif /* aarch64-sve-linux-ptrace.h */
--
2.15.1 (Apple Git-101)
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 02/10] Add Aarch64 SVE Linux headers
2018-06-06 15:17 ` [PATCH v2 02/10] Add Aarch64 SVE Linux headers Alan Hayward
@ 2018-06-08 14:13 ` Alan Hayward
2018-06-08 14:37 ` Simon Marchi
0 siblings, 1 reply; 56+ messages in thread
From: Alan Hayward @ 2018-06-08 14:13 UTC (permalink / raw)
To: GDB Patches, Simon Marchi; +Cc: nd
(Moved review to correct thread)
Thanks for the reviews.
> On 7 Jun 2018, at 21:18, Simon Marchi <simon.marchi@ericsson.com> wrote:
>
> Hi Alan,
>
> Just some quick comments.
>
> I get this when building on x86-64 with --enable-targets=all:
Hmm.. I had lost that flag from my build script. I Re-added it, and
reproduced the issues.
>
> CXX aarch64-tdep.o
> In file included from /home/emaisin/src/binutils-gdb/gdb/nat/aarch64-sve-linux-ptrace.h:29:0,
> from /home/emaisin/src/binutils-gdb/gdb/aarch64-tdep.c:61:
> /home/emaisin/src/binutils-gdb/gdb/nat/aarch64-linux-sigcontext.h:19:22: error: field ‘head’ has incomplete type ‘_aarch64_ctx’
> struct _aarch64_ctx head;
> ^
> /home/emaisin/src/binutils-gdb/gdb/nat/aarch64-linux-sigcontext.h:19:9: note: forward declaration of ‘struct _aarch64_ctx’
> struct _aarch64_ctx head;
> ^
>
> First, we should not include "nat/aarch64-sve-linux-ptrace.h" (a file that only makes
> sense when building on AArch64) in aarch64-tdep.c, a file built on all architecture
> when including the support for AArch64 debugging. It looks like aarch64-tdep.c
> needs sve_vq_from_vl. Maybe that definition could be moved to arch/, which can be
> included in aarch64-tdep.c.
>
I had put it in there because I wanted to try and make it a complete block
copied from Linux. The issue makes sense, so I’ve updated to restore
sve_vq_from_vl/sve_vl_from_vq back to arch/aarch64.h and removed it from
nat/aarch64-linux-sigcontext.h
> Then, is the _aarch64_ctx structure guaranteed to be defined on older AArch64 kernels
> or should we include it too?
_aarch64_ctx is part of the standard aarch64 signal handling. A quick git blame gives
me 2012 - which is roughly the age of aarch64. So, it should always be defined.
Updated patch below. Checked it builds (with other sve patches) on:
X86 all-targets
Aarch64 Linux 4.10 (pre sve headers) ubuntu 16.04
Aarch64 Linux 4.15 (with sve headers) ubuntu 18.04
Are you ok with the new version?
diff --git a/gdb/nat/aarch64-linux-ptrace.h b/gdb/nat/aarch64-linux-ptrace.h
new file mode 100644
index 0000000000000000000000000000000000000000..1d0bf1b314038457632eef22e1c2d010d1604c93
--- /dev/null
+++ b/gdb/nat/aarch64-linux-ptrace.h
@@ -0,0 +1,150 @@
+/* This file contains Aarch64 Linux ptrace defines. It is required for those
+ compiling with older kernel headers. Eventually, when the kernel defines are
+ old enough, this file should be removed.
+
+ Contents are directly copied directly from
+ linux/arch/arm64/include/uapi/asm/ptrace.h in Linux 4.17.
+
+ See:
+ https://github.com/torvalds/linux/blob/v4.17/arch/arm64/include/uapi/asm/ptrace.h
+*/
+
+#ifndef AARCH64_LINUX_PTRACE_H
+#define AARCH64_LINUX_PTRACE_H
+
+/* SVE/FP/SIMD state (NT_ARM_SVE) */
+
+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;
+};
+
+/* Definitions for user_sve_header.flags: */
+#define SVE_PT_REGS_MASK (1 << 0)
+
+#define SVE_PT_REGS_FPSIMD 0
+#define SVE_PT_REGS_SVE SVE_PT_REGS_MASK
+
+/*
+ * Common SVE_PT_* flags:
+ * These must be kept in sync with prctl interface in <linux/ptrace.h>
+ */
+#define SVE_PT_VL_INHERIT (PR_SVE_VL_INHERIT >> 16)
+#define SVE_PT_VL_ONEXEC (PR_SVE_SET_VL_ONEXEC >> 16)
+
+
+/*
+ * The remainder of the SVE state follows struct user_sve_header. The
+ * total size of the SVE state (including header) depends on the
+ * metadata in the header: SVE_PT_SIZE(vq, flags) gives the total size
+ * of the state in bytes, including the header.
+ *
+ * Refer to <asm/sigcontext.h> for details of how to pass the correct
+ * "vq" argument to these macros.
+ */
+
+/* Offset from the start of struct user_sve_header to the register data */
+#define SVE_PT_REGS_OFFSET \
+ ((sizeof(struct sve_context) + (SVE_VQ_BYTES - 1)) \
+ / SVE_VQ_BYTES * SVE_VQ_BYTES)
+
+/*
+ * The register data content and layout depends on the value of the
+ * flags field.
+ */
+
+/*
+ * (flags & SVE_PT_REGS_MASK) == SVE_PT_REGS_FPSIMD case:
+ *
+ * The payload starts at offset SVE_PT_FPSIMD_OFFSET, and is of type
+ * struct user_fpsimd_state. Additional data might be appended in the
+ * future: use SVE_PT_FPSIMD_SIZE(vq, flags) to compute the total size.
+ * SVE_PT_FPSIMD_SIZE(vq, flags) will never be less than
+ * sizeof(struct user_fpsimd_state).
+ */
+
+#define SVE_PT_FPSIMD_OFFSET SVE_PT_REGS_OFFSET
+
+#define SVE_PT_FPSIMD_SIZE(vq, flags) (sizeof(struct user_fpsimd_state))
+
+/*
+ * (flags & SVE_PT_REGS_MASK) == SVE_PT_REGS_SVE case:
+ *
+ * The payload starts at offset SVE_PT_SVE_OFFSET, and is of size
+ * SVE_PT_SVE_SIZE(vq, flags).
+ *
+ * Additional macros describe the contents and layout of the payload.
+ * For each, SVE_PT_SVE_x_OFFSET(args) is the start offset relative to
+ * the start of struct user_sve_header, and SVE_PT_SVE_x_SIZE(args) is
+ * the size in bytes:
+ *
+ * x type description
+ * - ---- -----------
+ * ZREGS \
+ * ZREG |
+ * PREGS | refer to <asm/sigcontext.h>
+ * PREG |
+ * FFR /
+ *
+ * FPSR uint32_t FPSR
+ * FPCR uint32_t FPCR
+ *
+ * Additional data might be appended in the future.
+ */
+
+#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)
+
+/*
+ * Any future extension appended after FPCR must be aligned to the next
+ * 128-bit boundary.
+ */
+
+#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 /* AARCH64_LINUX_PTRACE_H */
diff --git a/gdb/nat/aarch64-linux-sigcontext.h b/gdb/nat/aarch64-linux-sigcontext.h
new file mode 100644
index 0000000000000000000000000000000000000000..7253b85cc1f28859a68293c02d87052a48aa567f
--- /dev/null
+++ b/gdb/nat/aarch64-linux-sigcontext.h
@@ -0,0 +1,127 @@
+/* This file contains Aarch64 Linux sigcontext defines. It is required for those
+ compiling with older kernel headers. Eventually, when the kernel defines are
+ old enough, this file should be removed.
+
+ Contents are directly copied directly from
+ linux/arch/arm64/include/uapi/asm/sigcontext.h in Linux 4.17.
+
+ See:
+ https://github.com/torvalds/linux/blob/v4.17/arch/arm64/include/uapi/asm/sigcontext.h
+*/
+
+#ifndef AARCH64_LINUX_SIGCONTEXT_H
+#define AARCH64_LINUX_SIGCONTEXT_H
+
+
+#define SVE_MAGIC 0x53564501
+
+struct sve_context {
+ struct _aarch64_ctx head;
+ __u16 vl;
+ __u16 __reserved[3];
+};
+
+/*
+ * The SVE architecture leaves space for future expansion of the
+ * vector length beyond its initial architectural limit of 2048 bits
+ * (16 quadwords).
+ *
+ * See linux/Documentation/arm64/sve.txt for a description of the VL/VQ
+ * terminology.
+ */
+#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)
+
+/*
+ * If the SVE registers are currently live for the thread at signal delivery,
+ * sve_context.head.size >=
+ * SVE_SIG_CONTEXT_SIZE(sve_vq_from_vl(sve_context.vl))
+ * and the register data may be accessed using the SVE_SIG_*() macros.
+ *
+ * If sve_context.head.size <
+ * SVE_SIG_CONTEXT_SIZE(sve_vq_from_vl(sve_context.vl)),
+ * the SVE registers were not live for the thread and no register data
+ * is included: in this case, the SVE_SIG_*() macros should not be
+ * used except for this check.
+ *
+ * The same convention applies when returning from a signal: a caller
+ * will need to remove or resize the sve_context block if it wants to
+ * make the SVE registers live when they were previously non-live or
+ * vice-versa. This may require the the caller to allocate fresh
+ * memory and/or move other context blocks in the signal frame.
+ *
+ * Changing the vector length during signal return is not permitted:
+ * sve_context.vl must equal the thread's current vector length when
+ * doing a sigreturn.
+ *
+ *
+ * Note: for all these macros, the "vq" argument denotes the SVE
+ * vector length in quadwords (i.e., units of 128 bits).
+ *
+ * The correct way to obtain vq is to use sve_vq_from_vl(vl). The
+ * result is valid if and only if sve_vl_valid(vl) is true. This is
+ * guaranteed for a struct sve_context written by the kernel.
+ *
+ *
+ * Additional macros describe the contents and layout of the payload.
+ * For each, SVE_SIG_x_OFFSET(args) is the start offset relative to
+ * the start of struct sve_context, and SVE_SIG_x_SIZE(args) is the
+ * size in bytes:
+ *
+ * x type description
+ * - ---- -----------
+ * REGS the entire SVE context
+ *
+ * ZREGS __uint128_t[SVE_NUM_ZREGS][vq] all Z-registers
+ * ZREG __uint128_t[vq] individual Z-register Zn
+ *
+ * PREGS uint16_t[SVE_NUM_PREGS][vq] all P-registers
+ * PREG uint16_t[vq] individual P-register Pn
+ *
+ * FFR uint16_t[vq] first-fault status register
+ *
+ * Additional data might be appended in the future.
+ */
+
+#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 /* AARCH64_LINUX_SIGCONTEXT_H */
diff --git a/gdb/nat/aarch64-sve-linux-ptrace.h b/gdb/nat/aarch64-sve-linux-ptrace.h
index 61f841466c8279c14322894e4cedbe3b6e39db4b..2d6f5714c0fd77cd51142500ba04dd0a70717d2d 100644
--- a/gdb/nat/aarch64-sve-linux-ptrace.h
+++ b/gdb/nat/aarch64-sve-linux-ptrace.h
@@ -20,54 +20,22 @@
#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 zero). */
-
-uint64_t 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. */
+#include "aarch64-linux-sigcontext.h"
+#endif
#ifndef SVE_PT_SVE_ZREG_SIZE
+#include "aarch64-linux-ptrace.h"
+#endif
-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;
-};
+/* 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 zero). */
-#endif /* SVE_PT_SVE_ZREG_SIZE. */
+uint64_t aarch64_sve_get_vq (int tid);
#endif /* aarch64-sve-linux-ptrace.h */
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 02/10] Add Aarch64 SVE Linux headers
2018-06-08 14:13 ` Alan Hayward
@ 2018-06-08 14:37 ` Simon Marchi
2018-06-08 15:23 ` Simon Marchi
2018-06-08 15:27 ` [PATCH v2 02/10] Add Aarch64 SVE Linux headers Alan Hayward
0 siblings, 2 replies; 56+ messages in thread
From: Simon Marchi @ 2018-06-08 14:37 UTC (permalink / raw)
To: Alan Hayward; +Cc: GDB Patches, Simon Marchi, nd
On 2018-06-08 10:13, Alan Hayward wrote:
> (Moved review to correct thread)
> Thanks for the reviews.
>
>> On 7 Jun 2018, at 21:18, Simon Marchi <simon.marchi@ericsson.com>
>> wrote:
>>
>> Hi Alan,
>>
>> Just some quick comments.
>>
>> I get this when building on x86-64 with --enable-targets=all:
>
> Hmm.. I had lost that flag from my build script. I Re-added it, and
> reproduced the issues.
>
>>
>> CXX aarch64-tdep.o
>> In file included from
>> /home/emaisin/src/binutils-gdb/gdb/nat/aarch64-sve-linux-ptrace.h:29:0,
>> from
>> /home/emaisin/src/binutils-gdb/gdb/aarch64-tdep.c:61:
>> /home/emaisin/src/binutils-gdb/gdb/nat/aarch64-linux-sigcontext.h:19:22:
>> error: field ‘head’ has incomplete type ‘_aarch64_ctx’
>> struct _aarch64_ctx head;
>> ^
>> /home/emaisin/src/binutils-gdb/gdb/nat/aarch64-linux-sigcontext.h:19:9:
>> note: forward declaration of ‘struct _aarch64_ctx’
>> struct _aarch64_ctx head;
>> ^
>>
>> First, we should not include "nat/aarch64-sve-linux-ptrace.h" (a file
>> that only makes
>> sense when building on AArch64) in aarch64-tdep.c, a file built on all
>> architecture
>> when including the support for AArch64 debugging. It looks like
>> aarch64-tdep.c
>> needs sve_vq_from_vl. Maybe that definition could be moved to arch/,
>> which can be
>> included in aarch64-tdep.c.
>>
>
> I had put it in there because I wanted to try and make it a complete
> block
> copied from Linux. The issue makes sense, so I’ve updated to restore
> sve_vq_from_vl/sve_vl_from_vq back to arch/aarch64.h and removed it
> from
> nat/aarch64-linux-sigcontext.h
>
>
>> Then, is the _aarch64_ctx structure guaranteed to be defined on older
>> AArch64 kernels
>> or should we include it too?
>
>
> _aarch64_ctx is part of the standard aarch64 signal handling. A quick
> git blame gives
> me 2012 - which is roughly the age of aarch64. So, it should always be
> defined.
>
>
> Updated patch below. Checked it builds (with other sve patches) on:
> X86 all-targets
> Aarch64 Linux 4.10 (pre sve headers) ubuntu 16.04
> Aarch64 Linux 4.15 (with sve headers) ubuntu 18.04
>
> Are you ok with the new version?
The code looks good to me, thanks. I am still unsure about the
licensing side of it, let me ask the FSF people about it, I'll come back
to you when it's done. I hope it won't take too long!
Simon
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 02/10] Add Aarch64 SVE Linux headers
2018-06-08 14:37 ` Simon Marchi
@ 2018-06-08 15:23 ` Simon Marchi
2018-06-12 14:37 ` Alan Hayward
2018-06-08 15:27 ` [PATCH v2 02/10] Add Aarch64 SVE Linux headers Alan Hayward
1 sibling, 1 reply; 56+ messages in thread
From: Simon Marchi @ 2018-06-08 15:23 UTC (permalink / raw)
To: Alan Hayward; +Cc: GDB Patches, Simon Marchi, nd
On 2018-06-08 10:37, Simon Marchi wrote:
> The code looks good to me, thanks. I am still unsure about the
> licensing side of it, let me ask the FSF people about it, I'll come
> back to you when it's done. I hope it won't take too long!
Hi Alan,
After discussion with other maintainers, it was suggested to avoid
involving the legal staff if we want to resolve this anytime soon.
Since ARM already holds the copyright to these header files anyway (they
were all written by ARM people), you may be able to submit that code as
regular FSF-assigned code, without changing the status of the kernel
copy. But nobody here is a lawyer, so nobody wants to say for sure :).
Maybe it's ok after all if we don't include these headers (at least for
now), and require that GDB for native AArch64 is built against the
headers of a >= 4.15 kernel? They can always be included later, but it
would avoid delaying the inclusion of the feature, since you want to
have it before we branch 8.2.
Simon
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 02/10] Add Aarch64 SVE Linux headers
2018-06-08 15:23 ` Simon Marchi
@ 2018-06-12 14:37 ` Alan Hayward
2018-06-12 14:43 ` Pedro Alves
2018-06-12 14:51 ` Simon Marchi
0 siblings, 2 replies; 56+ messages in thread
From: Alan Hayward @ 2018-06-12 14:37 UTC (permalink / raw)
To: Simon Marchi; +Cc: GDB Patches, Simon Marchi, nd
> On 8 Jun 2018, at 16:23, Simon Marchi <simon.marchi@polymtl.ca> wrote:
>
> On 2018-06-08 10:37, Simon Marchi wrote:
>> The code looks good to me, thanks. I am still unsure about the
>> licensing side of it, let me ask the FSF people about it, I'll come
>> back to you when it's done. I hope it won't take too long!
>
> Hi Alan,
>
> After discussion with other maintainers, it was suggested to avoid involving the legal staff if we want to resolve this anytime soon.
>
> Since ARM already holds the copyright to these header files anyway (they were all written by ARM people), you may be able to submit that code as regular FSF-assigned code, without changing the status of the kernel copy. But nobody here is a lawyer, so nobody wants to say for sure :).
>
> Maybe it's ok after all if we don't include these headers (at least for now), and require that GDB for native AArch64 is built against the headers of a >= 4.15 kernel? They can always be included later, but it would avoid delaying the inclusion of the feature, since you want to have it before we branch 8.2.
>
Sorry, I did miss this one (I think I sent my reply to the previous
one more or less the same time you sent this).
If I commit this, (I think) this is going to cause buildbot to break
for the aarch64 builds.
(Out of interest - I’ve heard people say they tested on buildbot. Are
there some instructions for doing that? I can try it out.)
I suspect updating buildbot is also not a quick fix.
If all that’s not ok (I suspect not), I’ll have a quick word with the
more legal aware people on my side, see if there is any opinion.
Alan.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 02/10] Add Aarch64 SVE Linux headers
2018-06-12 14:37 ` Alan Hayward
@ 2018-06-12 14:43 ` Pedro Alves
2018-06-12 15:06 ` Simon Marchi
2018-06-12 15:09 ` Alan Hayward
2018-06-12 14:51 ` Simon Marchi
1 sibling, 2 replies; 56+ messages in thread
From: Pedro Alves @ 2018-06-12 14:43 UTC (permalink / raw)
To: Alan Hayward, Simon Marchi; +Cc: GDB Patches, Simon Marchi, nd
On 06/12/2018 03:37 PM, Alan Hayward wrote:
>
>
>> On 8 Jun 2018, at 16:23, Simon Marchi <simon.marchi@polymtl.ca> wrote:
>>
>> On 2018-06-08 10:37, Simon Marchi wrote:
>>> The code looks good to me, thanks. I am still unsure about the
>>> licensing side of it, let me ask the FSF people about it, I'll come
>>> back to you when it's done. I hope it won't take too long!
>>
>> Hi Alan,
>>
>> After discussion with other maintainers, it was suggested to avoid involving the legal staff if we want to resolve this anytime soon.
>>
>> Since ARM already holds the copyright to these header files anyway (they were all written by ARM people), you may be able to submit that code as regular FSF-assigned code, without changing the status of the kernel copy. But nobody here is a lawyer, so nobody wants to say for sure :).
>>
>> Maybe it's ok after all if we don't include these headers (at least for now), and require that GDB for native AArch64 is built against the headers of a >= 4.15 kernel? They can always be included later, but it would avoid delaying the inclusion of the feature, since you want to have it before we branch 8.2.
>>
>
> Sorry, I did miss this one (I think I sent my reply to the previous
> one more or less the same time you sent this).
>
> If I commit this,
What's "this" ?
How about we add a configure check to check if the system headers support
the needed SVE bits, and guard the native gdb SVE bits with
HAVE_AARCH64_SVE or something like that?
(I think) this is going to cause buildbot to break
> for the aarch64 builds.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 02/10] Add Aarch64 SVE Linux headers
2018-06-12 14:43 ` Pedro Alves
@ 2018-06-12 15:06 ` Simon Marchi
2018-06-12 15:11 ` Pedro Alves
2018-06-12 15:09 ` Alan Hayward
1 sibling, 1 reply; 56+ messages in thread
From: Simon Marchi @ 2018-06-12 15:06 UTC (permalink / raw)
To: Pedro Alves; +Cc: Alan Hayward, GDB Patches, Simon Marchi, nd
On 2018-06-12 10:43, Pedro Alves wrote:
> What's "this" ?
From what I understand, "this" is the suggestion I made in my previous
mail, require the user to build against the headers of a recent kernel
(that provide the SVE macros), and not provide a stop-gap copy in the
GDB tree. It would break the buildbot, because they have an old kernel
that doesn't provide the SVE macros the GDB code uses (e.g.
SVE_PT_REGS_OFFSET).
> How about we add a configure check to check if the system headers
> support
> the needed SVE bits, and guard the native gdb SVE bits with
> HAVE_AARCH64_SVE or something like that?
I think that would be a good compromise. By default, building on a
machine with an older kernel would exclude SVE support. But it would be
possible to add it by pointing to the headers of a recent kernel. So
when building on a machine with an older kernel...
- ... without any special flags, you don't get SVE support.
- ... with just --enable-sve, you get a configure error.
- ... with --enable-sve and CFLAGS/CXXFLAGS pointing to headers of a
kernel w/ SVE macros, you get SVE support.
Does that make sense?
Simon
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 02/10] Add Aarch64 SVE Linux headers
2018-06-12 15:06 ` Simon Marchi
@ 2018-06-12 15:11 ` Pedro Alves
2018-06-12 15:21 ` Simon Marchi
0 siblings, 1 reply; 56+ messages in thread
From: Pedro Alves @ 2018-06-12 15:11 UTC (permalink / raw)
To: Simon Marchi; +Cc: Alan Hayward, GDB Patches, Simon Marchi, nd
On 06/12/2018 04:06 PM, Simon Marchi wrote:
> On 2018-06-12 10:43, Pedro Alves wrote:
>> What's "this" ?
>
> From what I understand, "this" is the suggestion I made in my previous mail, require the user to build against the headers of a recent kernel (that provide the SVE macros), and not provide a stop-gap copy in the GDB tree. It would break the buildbot, because they have an old kernel that doesn't provide the SVE macros the GDB code uses (e.g. SVE_PT_REGS_OFFSET).
OK, that was not what I had suggested the other day (which was to detect SVE
support at configure time), so I got confused.
>
>> How about we add a configure check to check if the system headers support
>> the needed SVE bits, and guard the native gdb SVE bits with
>> HAVE_AARCH64_SVE or something like that?
>
> I think that would be a good compromise. By default, building on a machine with an older kernel would exclude SVE support. But it would be possible to add it by pointing to the headers of a recent kernel. So when building on a machine with an older kernel...
>
> - ... without any special flags, you don't get SVE support.
> - ... with just --enable-sve, you get a configure error.
> - ... with --enable-sve and CFLAGS/CXXFLAGS pointing to headers of a kernel w/ SVE macros, you get SVE support.
>
> Does that make sense?
Yes. Not sure an --enable-sve switch is necessary (compared to just having
headers vs not having headers), but I'd be fine with having one.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 02/10] Add Aarch64 SVE Linux headers
2018-06-12 15:11 ` Pedro Alves
@ 2018-06-12 15:21 ` Simon Marchi
0 siblings, 0 replies; 56+ messages in thread
From: Simon Marchi @ 2018-06-12 15:21 UTC (permalink / raw)
To: Pedro Alves; +Cc: Alan Hayward, GDB Patches, Simon Marchi, nd
On 2018-06-12 11:11, Pedro Alves wrote:
> On 06/12/2018 04:06 PM, Simon Marchi wrote:
>> I think that would be a good compromise. By default, building on a
>> machine with an older kernel would exclude SVE support. But it would
>> be possible to add it by pointing to the headers of a recent kernel.Â
>> So when building on a machine with an older kernel...
>>
>> - ... without any special flags, you don't get SVE support.
>> - ... with just --enable-sve, you get a configure error.
>> - ... with --enable-sve and CFLAGS/CXXFLAGS pointing to headers of a
>> kernel w/ SVE macros, you get SVE support.
>>
>> Does that make sense?
> Yes. Not sure an --enable-sve switch is necessary (compared to just
> having
> headers vs not having headers), but I'd be fine with having one.
I think it is useful if you want to make sure your build will have the
support:
- auto/not specified: include the support if the prerequisites are
available
- enable: include the support, error at configure if prerequisites are
missing
- disable: don't include the support
Otherwise, just a typo in your include path can result in a build
without the feature you want, and you only discover it later, that's
annoying.
Simon
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 02/10] Add Aarch64 SVE Linux headers
2018-06-12 14:43 ` Pedro Alves
2018-06-12 15:06 ` Simon Marchi
@ 2018-06-12 15:09 ` Alan Hayward
1 sibling, 0 replies; 56+ messages in thread
From: Alan Hayward @ 2018-06-12 15:09 UTC (permalink / raw)
To: Pedro Alves; +Cc: Simon Marchi, GDB Patches, Simon Marchi, nd
> On 12 Jun 2018, at 15:43, Pedro Alves <palves@redhat.com> wrote:
>
> On 06/12/2018 03:37 PM, Alan Hayward wrote:
>>
>>
>>> On 8 Jun 2018, at 16:23, Simon Marchi <simon.marchi@polymtl.ca> wrote:
>>>
>>> On 2018-06-08 10:37, Simon Marchi wrote:
>>>> The code looks good to me, thanks. I am still unsure about the
>>>> licensing side of it, let me ask the FSF people about it, I'll come
>>>> back to you when it's done. I hope it won't take too long!
>>>
>>> Hi Alan,
>>>
>>> After discussion with other maintainers, it was suggested to avoid involving the legal staff if we want to resolve this anytime soon.
>>>
>>> Since ARM already holds the copyright to these header files anyway (they were all written by ARM people), you may be able to submit that code as regular FSF-assigned code, without changing the status of the kernel copy. But nobody here is a lawyer, so nobody wants to say for sure :).
>>>
>>> Maybe it's ok after all if we don't include these headers (at least for now), and require that GDB for native AArch64 is built against the headers of a >= 4.15 kernel? They can always be included later, but it would avoid delaying the inclusion of the feature, since you want to have it before we branch 8.2.
>>>
>>
>> Sorry, I did miss this one (I think I sent my reply to the previous
>> one more or less the same time you sent this).
>>
>> If I commit this,
>
> What's "this" ?
Should have said “if I commit 4/10 without 2/10 it’s going to cause
buildbot to break"
>
> How about we add a configure check to check if the system headers support
> the needed SVE bits, and guard the native gdb SVE bits with
> HAVE_AARCH64_SVE or something like that?
>
Excellent idea. I can have a look at doing this - should be fairly quick to do.
In the meantime, lets keep the other discussion going.
Alan.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 02/10] Add Aarch64 SVE Linux headers
2018-06-12 14:37 ` Alan Hayward
2018-06-12 14:43 ` Pedro Alves
@ 2018-06-12 14:51 ` Simon Marchi
2018-06-12 16:34 ` Sergio Durigan Junior
1 sibling, 1 reply; 56+ messages in thread
From: Simon Marchi @ 2018-06-12 14:51 UTC (permalink / raw)
To: Alan Hayward; +Cc: GDB Patches, Simon Marchi, nd, Sergio Durigan Junior
On 2018-06-12 10:37, Alan Hayward wrote:
> Sorry, I did miss this one (I think I sent my reply to the previous
> one more or less the same time you sent this).
>
> If I commit this, (I think) this is going to cause buildbot to break
> for the aarch64 builds.
> (Out of interest - I’ve heard people say they tested on buildbot. Are
> there some instructions for doing that? I can try it out.)
Hmm you're right. Though maybe we can have additional
commands/configure options specific to the aarch64 builders? They could
download a kernel tarball, install the headers somewhere (that doesn't
take long, no need to build the kernel) and point to them. Sergio,
would that be possible/a good idea?
See this for the Buildbot, in particular the try jobs:
https://sourceware.org/gdb/wiki/BuildBot
> I suspect updating buildbot is also not a quick fix.
I don't think updating the kernel on the AArch64 machines is an option,
but we have control over what the buildbot does.
> If all that’s not ok (I suspect not), I’ll have a quick word with the
> more legal aware people on my side, see if there is any opinion.
Ok, thanks.
Simon
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 02/10] Add Aarch64 SVE Linux headers
2018-06-12 14:51 ` Simon Marchi
@ 2018-06-12 16:34 ` Sergio Durigan Junior
2018-06-12 17:51 ` Alan Hayward
0 siblings, 1 reply; 56+ messages in thread
From: Sergio Durigan Junior @ 2018-06-12 16:34 UTC (permalink / raw)
To: Simon Marchi; +Cc: Alan Hayward, GDB Patches, Simon Marchi, nd
On Tuesday, June 12 2018, Simon Marchi wrote:
> On 2018-06-12 10:37, Alan Hayward wrote:
>> Sorry, I did miss this one (I think I sent my reply to the previous
>> one more or less the same time you sent this).
>>
>> If I commit this, (I think) this is going to cause buildbot to break
>> for the aarch64 builds.
>> (Out of interest - I’ve heard people say they tested on buildbot. Are
>> there some instructions for doing that? I can try it out.)
>
> Hmm you're right. Though maybe we can have additional
> commands/configure options specific to the aarch64 builders? They
> could download a kernel tarball, install the headers somewhere (that
> doesn't take long, no need to build the kernel) and point to them.
> Sergio, would that be possible/a good idea?
I'm not sure. For starters, the Aarch64 builders have kinda been
forgotten since Yao stopped contributing regularly to GDB (he is the
maintainer of the machines behind the builders). So the very first
thing we'd need to do is to put the builders in a good shape again
(they're currently with 273 pending builds in the queue!). This is
something that's been on my TODO list for a while now, and I was going
to ask Alan (or anyone from ARM) if they're not interested in taking
over the maintenance of these machines.
Then, I think the best approach for the SVE builds would be to manually
download a Linux kernel, put the sources somewhere, and then I could
configure a specific builder to build GDB with the SVE headers.
--
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] 56+ messages in thread
* Re: [PATCH v2 02/10] Add Aarch64 SVE Linux headers
2018-06-12 16:34 ` Sergio Durigan Junior
@ 2018-06-12 17:51 ` Alan Hayward
2018-06-12 20:29 ` Sergio Durigan Junior
0 siblings, 1 reply; 56+ messages in thread
From: Alan Hayward @ 2018-06-12 17:51 UTC (permalink / raw)
To: Sergio Durigan Junior
Cc: Simon Marchi, GDB Patches, Simon Marchi, nd, Pedro Alves
> On 12 Jun 2018, at 17:34, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>
> On Tuesday, June 12 2018, Simon Marchi wrote:
>
>> On 2018-06-12 10:37, Alan Hayward wrote:
>>> Sorry, I did miss this one (I think I sent my reply to the previous
>>> one more or less the same time you sent this).
>>>
>>> If I commit this, (I think) this is going to cause buildbot to break
>>> for the aarch64 builds.
>>> (Out of interest - I’ve heard people say they tested on buildbot. Are
>>> there some instructions for doing that? I can try it out.)
>>
>> Hmm you're right. Though maybe we can have additional
>> commands/configure options specific to the aarch64 builders? They
>> could download a kernel tarball, install the headers somewhere (that
>> doesn't take long, no need to build the kernel) and point to them.
>> Sergio, would that be possible/a good idea?
>
> I'm not sure. For starters, the Aarch64 builders have kinda been
> forgotten since Yao stopped contributing regularly to GDB (he is the
> maintainer of the machines behind the builders). So the very first
> thing we'd need to do is to put the builders in a good shape again
> (they're currently with 273 pending builds in the queue!). This is
> something that's been on my TODO list for a while now, and I was going
> to ask Alan (or anyone from ARM) if they're not interested in taking
> over the maintenance of these machines.
Looking after the aarch64 boxes does sound like a job for an Arm person.
I guess it’ll be fairly important to get those queues cleared _before_
8.2 is released. I can certainly take a look at the pending builds in
the next few weeks.
Once I’ve got the sve stuff cleared I wanted to take a step back and see
what general things needed doing for aarch64 (I’ve also got a couple of
lingering non-gdb things to wrap up too, so that’s going to eat into my
time).
>
> Then, I think the best approach for the SVE builds would be to manually
> download a Linux kernel, put the sources somewhere, and then I could
> configure a specific builder to build GDB with the SVE headers.
>
Given the current queues, I suspect we’d not get this done before the 8.2
branch.
I’m thinking configure check of Pedro’s sounds the first step, then once
the aarch64 build queues have been cleared, get some sve builds added.
The SVE headers are in Ubuntu 18.04 - so “all” that’s needed is to do a
dist upgrade on them (I suspect there are probably lots of reasons why
that can’t be done!)
Alan.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 02/10] Add Aarch64 SVE Linux headers
2018-06-12 17:51 ` Alan Hayward
@ 2018-06-12 20:29 ` Sergio Durigan Junior
2018-06-15 9:45 ` Ramana Radhakrishnan
0 siblings, 1 reply; 56+ messages in thread
From: Sergio Durigan Junior @ 2018-06-12 20:29 UTC (permalink / raw)
To: Alan Hayward; +Cc: Simon Marchi, GDB Patches, Simon Marchi, nd, Pedro Alves
On Tuesday, June 12 2018, Alan Hayward wrote:
>> On 12 Jun 2018, at 17:34, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>>
>> On Tuesday, June 12 2018, Simon Marchi wrote:
>>
>>> On 2018-06-12 10:37, Alan Hayward wrote:
>>>> Sorry, I did miss this one (I think I sent my reply to the previous
>>>> one more or less the same time you sent this).
>>>>
>>>> If I commit this, (I think) this is going to cause buildbot to break
>>>> for the aarch64 builds.
>>>> (Out of interest - I’ve heard people say they tested on buildbot. Are
>>>> there some instructions for doing that? I can try it out.)
>>>
>>> Hmm you're right. Though maybe we can have additional
>>> commands/configure options specific to the aarch64 builders? They
>>> could download a kernel tarball, install the headers somewhere (that
>>> doesn't take long, no need to build the kernel) and point to them.
>>> Sergio, would that be possible/a good idea?
>>
>> I'm not sure. For starters, the Aarch64 builders have kinda been
>> forgotten since Yao stopped contributing regularly to GDB (he is the
>> maintainer of the machines behind the builders). So the very first
>> thing we'd need to do is to put the builders in a good shape again
>> (they're currently with 273 pending builds in the queue!). This is
>> something that's been on my TODO list for a while now, and I was going
>> to ask Alan (or anyone from ARM) if they're not interested in taking
>> over the maintenance of these machines.
>
> Looking after the aarch64 boxes does sound like a job for an Arm person.
> I guess it’ll be fairly important to get those queues cleared _before_
> 8.2 is released. I can certainly take a look at the pending builds in
> the next few weeks.
Yeah, what I do in these cases is cancel all of the pending builds,
i.e., start fresh.
I'm glad you're interested in taking care of the machines. TBH, I was
even considering disabling them for now, since at least one machine has
been offline for a long time, and the other has this giant queue. I'm
not sure if you're going to use the same machines as Yao was using
(IIRC, he was using machines from the GCC Compile Farm).
>> Then, I think the best approach for the SVE builds would be to manually
>> download a Linux kernel, put the sources somewhere, and then I could
>> configure a specific builder to build GDB with the SVE headers.
>>
>
> Given the current queues, I suspect we’d not get this done before the 8.2
> branch.
I wouldn't count on that.
> I’m thinking configure check of Pedro’s sounds the first step, then once
> the aarch64 build queues have been cleared, get some sve builds added.
>
> The SVE headers are in Ubuntu 18.04 - so “all” that’s needed is to do a
> dist upgrade on them (I suspect there are probably lots of reasons why
> that can’t be done!)
Yeah, I honestly don't know :-/. If you're planning to continue using
the GCC Farm machine, then I think the best option would be to contact
the admins and ask them.
Feel free to contact me in private if you need help sorting this out.
Cheers,
--
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] 56+ messages in thread
* Re: [PATCH v2 02/10] Add Aarch64 SVE Linux headers
2018-06-12 20:29 ` Sergio Durigan Junior
@ 2018-06-15 9:45 ` Ramana Radhakrishnan
2018-06-15 17:14 ` Alan Hayward
0 siblings, 1 reply; 56+ messages in thread
From: Ramana Radhakrishnan @ 2018-06-15 9:45 UTC (permalink / raw)
To: Sergio Durigan Junior, Alan Hayward
Cc: Simon Marchi, GDB Patches, Simon Marchi, nd, Pedro Alves
On 12/06/2018 21:29, Sergio Durigan Junior wrote:
> On Tuesday, June 12 2018, Alan Hayward wrote:
>
>>> On 12 Jun 2018, at 17:34, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>>>
>>> On Tuesday, June 12 2018, Simon Marchi wrote:
>>>
>>>> On 2018-06-12 10:37, Alan Hayward wrote:
>>>>> Sorry, I did miss this one (I think I sent my reply to the previous
>>>>> one more or less the same time you sent this).
>>>>>
>>>>> If I commit this, (I think) this is going to cause buildbot to break
>>>>> for the aarch64 builds.
>>>>> (Out of interest - I’ve heard people say they tested on buildbot. Are
>>>>> there some instructions for doing that? I can try it out.)
>>>>
>>>> Hmm you're right. Though maybe we can have additional
>>>> commands/configure options specific to the aarch64 builders? They
>>>> could download a kernel tarball, install the headers somewhere (that
>>>> doesn't take long, no need to build the kernel) and point to them.
>>>> Sergio, would that be possible/a good idea?
>>>
>>> I'm not sure. For starters, the Aarch64 builders have kinda been
>>> forgotten since Yao stopped contributing regularly to GDB (he is the
>>> maintainer of the machines behind the builders). So the very first
>>> thing we'd need to do is to put the builders in a good shape again
>>> (they're currently with 273 pending builds in the queue!). This is
>>> something that's been on my TODO list for a while now, and I was going
>>> to ask Alan (or anyone from ARM) if they're not interested in taking
>>> over the maintenance of these machines.
>>
>> Looking after the aarch64 boxes does sound like a job for an Arm person.
>> I guess it’ll be fairly important to get those queues cleared _before_
>> 8.2 is released. I can certainly take a look at the pending builds in
>> the next few weeks.
>
> Yeah, what I do in these cases is cancel all of the pending builds,
> i.e., start fresh.
>
> I'm glad you're interested in taking care of the machines. TBH, I was
> even considering disabling them for now, since at least one machine has
> been offline for a long time, and the other has this giant queue. I'm
> not sure if you're going to use the same machines as Yao was using
> (IIRC, he was using machines from the GCC Compile Farm).
>
>>> Then, I think the best approach for the SVE builds would be to manually
>>> download a Linux kernel, put the sources somewhere, and then I could
>>> configure a specific builder to build GDB with the SVE headers.
>>>
>>
>> Given the current queues, I suspect we’d not get this done before the 8.2
>> branch.
>
> I wouldn't count on that.
>
>> I’m thinking configure check of Pedro’s sounds the first step, then once
>> the aarch64 build queues have been cleared, get some sve builds added.
>>
>> The SVE headers are in Ubuntu 18.04 - so “all†that’s needed is to do a
>> dist upgrade on them (I suspect there are probably lots of reasons why
>> that can’t be done!)
>
> Yeah, I honestly don't know :-/. If you're planning to continue using
> the GCC Farm machine, then I think the best option would be to contact
> the admins and ask them.
>
So I arranged for those machines in the compile farm and I believe I
have super user privileges on them. Updating to 18.04 may be an option
but something I don't want to do remotely and in a rush.
Alan, please reach out to me if you need any help on the compile farm
machines, I can help out.
Ramana
> Feel free to contact me in private if you need help sorting this out.
>
> Cheers,
>
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 02/10] Add Aarch64 SVE Linux headers
2018-06-15 9:45 ` Ramana Radhakrishnan
@ 2018-06-15 17:14 ` Alan Hayward
2018-09-20 21:16 ` Status of the AArch* builders (was: Re: [PATCH v2 02/10] Add Aarch64 SVE Linux headers) Sergio Durigan Junior
0 siblings, 1 reply; 56+ messages in thread
From: Alan Hayward @ 2018-06-15 17:14 UTC (permalink / raw)
To: Ramana Radhakrishnan
Cc: Sergio Durigan Junior, Simon Marchi, GDB Patches, Simon Marchi,
nd, Pedro Alves
> On 15 Jun 2018, at 10:45, Ramana Radhakrishnan <ramana.radhakrishnan@foss.arm.com> wrote:
>
> On 12/06/2018 21:29, Sergio Durigan Junior wrote:
>> On Tuesday, June 12 2018, Alan Hayward wrote:
>>>> On 12 Jun 2018, at 17:34, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>>>>
>>>> On Tuesday, June 12 2018, Simon Marchi wrote:
>>>>
>>>>> On 2018-06-12 10:37, Alan Hayward wrote:
>>>>>> Sorry, I did miss this one (I think I sent my reply to the previous
>>>>>> one more or less the same time you sent this).
>>>>>>
>>>>>> If I commit this, (I think) this is going to cause buildbot to break
>>>>>> for the aarch64 builds.
>>>>>> (Out of interest - I’ve heard people say they tested on buildbot. Are
>>>>>> there some instructions for doing that? I can try it out.)
>>>>>
>>>>> Hmm you're right. Though maybe we can have additional
>>>>> commands/configure options specific to the aarch64 builders? They
>>>>> could download a kernel tarball, install the headers somewhere (that
>>>>> doesn't take long, no need to build the kernel) and point to them.
>>>>> Sergio, would that be possible/a good idea?
>>>>
>>>> I'm not sure. For starters, the Aarch64 builders have kinda been
>>>> forgotten since Yao stopped contributing regularly to GDB (he is the
>>>> maintainer of the machines behind the builders). So the very first
>>>> thing we'd need to do is to put the builders in a good shape again
>>>> (they're currently with 273 pending builds in the queue!). This is
>>>> something that's been on my TODO list for a while now, and I was going
>>>> to ask Alan (or anyone from ARM) if they're not interested in taking
>>>> over the maintenance of these machines.
>>>
>>> Looking after the aarch64 boxes does sound like a job for an Arm person.
>>> I guess it’ll be fairly important to get those queues cleared _before_
>>> 8.2 is released. I can certainly take a look at the pending builds in
>>> the next few weeks.
>> Yeah, what I do in these cases is cancel all of the pending builds,
>> i.e., start fresh.
>> I'm glad you're interested in taking care of the machines. TBH, I was
>> even considering disabling them for now, since at least one machine has
>> been offline for a long time, and the other has this giant queue. I'm
>> not sure if you're going to use the same machines as Yao was using
>> (IIRC, he was using machines from the GCC Compile Farm).
>>>> Then, I think the best approach for the SVE builds would be to manually
>>>> download a Linux kernel, put the sources somewhere, and then I could
>>>> configure a specific builder to build GDB with the SVE headers.
>>>>
>>>
>>> Given the current queues, I suspect we’d not get this done before the 8.2
>>> branch.
>> I wouldn't count on that.
>>> I’m thinking configure check of Pedro’s sounds the first step, then once
>>> the aarch64 build queues have been cleared, get some sve builds added.
>>>
>>> The SVE headers are in Ubuntu 18.04 - so “all” that’s needed is to do a
>>> dist upgrade on them (I suspect there are probably lots of reasons why
>>> that can’t be done!)
>> Yeah, I honestly don't know :-/. If you're planning to continue using
>> the GCC Farm machine, then I think the best option would be to contact
>> the admins and ask them.
>
> So I arranged for those machines in the compile farm and I believe I have super user privileges on them. Updating to 18.04 may be an option but something I don't want to do remotely and in a rush.
>
> Alan, please reach out to me if you need any help on the compile farm machines, I can help out.
>
Oh that’s good news (I thought they had come from Linaro).
If the headers get added to the gdb build then there’s no urgency to
update the boxes to ubuntu 18.04. However, 18.04 is the latest LTS, so
it’s possibly worth doing eventually (but not right now).
Alan.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Status of the AArch* builders (was: Re: [PATCH v2 02/10] Add Aarch64 SVE Linux headers)
2018-06-15 17:14 ` Alan Hayward
@ 2018-09-20 21:16 ` Sergio Durigan Junior
2018-09-24 14:16 ` Alan Hayward
0 siblings, 1 reply; 56+ messages in thread
From: Sergio Durigan Junior @ 2018-09-20 21:16 UTC (permalink / raw)
To: Alan Hayward
Cc: Ramana Radhakrishnan, Simon Marchi, GDB Patches, Simon Marchi,
nd, Pedro Alves
On Friday, June 15 2018, Alan Hayward wrote:
>> On 15 Jun 2018, at 10:45, Ramana Radhakrishnan <ramana.radhakrishnan@foss.arm.com> wrote:
>>
>> On 12/06/2018 21:29, Sergio Durigan Junior wrote:
>>> On Tuesday, June 12 2018, Alan Hayward wrote:
>>>>> On 12 Jun 2018, at 17:34, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>>>>>
>>>>> On Tuesday, June 12 2018, Simon Marchi wrote:
>>>>>
>>>>>> On 2018-06-12 10:37, Alan Hayward wrote:
>>>>>>> Sorry, I did miss this one (I think I sent my reply to the previous
>>>>>>> one more or less the same time you sent this).
>>>>>>>
>>>>>>> If I commit this, (I think) this is going to cause buildbot to break
>>>>>>> for the aarch64 builds.
>>>>>>> (Out of interest - I’ve heard people say they tested on buildbot. Are
>>>>>>> there some instructions for doing that? I can try it out.)
>>>>>>
>>>>>> Hmm you're right. Though maybe we can have additional
>>>>>> commands/configure options specific to the aarch64 builders? They
>>>>>> could download a kernel tarball, install the headers somewhere (that
>>>>>> doesn't take long, no need to build the kernel) and point to them.
>>>>>> Sergio, would that be possible/a good idea?
>>>>>
>>>>> I'm not sure. For starters, the Aarch64 builders have kinda been
>>>>> forgotten since Yao stopped contributing regularly to GDB (he is the
>>>>> maintainer of the machines behind the builders). So the very first
>>>>> thing we'd need to do is to put the builders in a good shape again
>>>>> (they're currently with 273 pending builds in the queue!). This is
>>>>> something that's been on my TODO list for a while now, and I was going
>>>>> to ask Alan (or anyone from ARM) if they're not interested in taking
>>>>> over the maintenance of these machines.
>>>>
>>>> Looking after the aarch64 boxes does sound like a job for an Arm person.
>>>> I guess it’ll be fairly important to get those queues cleared _before_
>>>> 8.2 is released. I can certainly take a look at the pending builds in
>>>> the next few weeks.
>>> Yeah, what I do in these cases is cancel all of the pending builds,
>>> i.e., start fresh.
>>> I'm glad you're interested in taking care of the machines. TBH, I was
>>> even considering disabling them for now, since at least one machine has
>>> been offline for a long time, and the other has this giant queue. I'm
>>> not sure if you're going to use the same machines as Yao was using
>>> (IIRC, he was using machines from the GCC Compile Farm).
>>>>> Then, I think the best approach for the SVE builds would be to manually
>>>>> download a Linux kernel, put the sources somewhere, and then I could
>>>>> configure a specific builder to build GDB with the SVE headers.
>>>>>
>>>>
>>>> Given the current queues, I suspect we’d not get this done before the 8.2
>>>> branch.
>>> I wouldn't count on that.
>>>> I’m thinking configure check of Pedro’s sounds the first step, then once
>>>> the aarch64 build queues have been cleared, get some sve builds added.
>>>>
>>>> The SVE headers are in Ubuntu 18.04 - so “all” that’s needed is to do a
>>>> dist upgrade on them (I suspect there are probably lots of reasons why
>>>> that can’t be done!)
>>> Yeah, I honestly don't know :-/. If you're planning to continue using
>>> the GCC Farm machine, then I think the best option would be to contact
>>> the admins and ask them.
>>
>> So I arranged for those machines in the compile farm and I believe I
>> have super user privileges on them. Updating to 18.04 may be an
>> option but something I don't want to do remotely and in a rush.
>>
>> Alan, please reach out to me if you need any help on the compile farm machines, I can help out.
>>
>
> Oh that’s good news (I thought they had come from Linaro).
>
> If the headers get added to the gdb build then there’s no urgency to
> update the boxes to ubuntu 18.04. However, 18.04 is the latest LTS, so
> it’s possibly worth doing eventually (but not right now).
Hi guys,
Just a ping to see if you have progressed on this. I've left the AArch*
builders there, and now they're *really* behind (more than 1000 builds
in the queue), and at least one of the buildslaves is offline.
I will temporarily remove the builders now, but it would be really nice
to keep having AArch* builders in our BuildBot.
Thanks a lot,
--
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] 56+ messages in thread
* Re: Status of the AArch* builders (was: Re: [PATCH v2 02/10] Add Aarch64 SVE Linux headers)
2018-09-20 21:16 ` Status of the AArch* builders (was: Re: [PATCH v2 02/10] Add Aarch64 SVE Linux headers) Sergio Durigan Junior
@ 2018-09-24 14:16 ` Alan Hayward
2018-09-24 14:42 ` Status of the AArch* builders Sergio Durigan Junior
0 siblings, 1 reply; 56+ messages in thread
From: Alan Hayward @ 2018-09-24 14:16 UTC (permalink / raw)
To: Sergio Durigan Junior
Cc: Ramana Radhakrishnan, Simon Marchi, GDB Patches, Simon Marchi,
nd, Pedro Alves
> On 20 Sep 2018, at 22:15, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>
> On Friday, June 15 2018, Alan Hayward wrote:
>
>>> On 15 Jun 2018, at 10:45, Ramana Radhakrishnan <ramana.radhakrishnan@foss.arm.com> wrote:
>>>
>>> On 12/06/2018 21:29, Sergio Durigan Junior wrote:
>>>> On Tuesday, June 12 2018, Alan Hayward wrote:
>>>>>> On 12 Jun 2018, at 17:34, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>>>>>>
>>>>>> On Tuesday, June 12 2018, Simon Marchi wrote:
>>>>>>
>>>>>>> On 2018-06-12 10:37, Alan Hayward wrote:
>>>>>>>> Sorry, I did miss this one (I think I sent my reply to the previous
>>>>>>>> one more or less the same time you sent this).
>>>>>>>>
>>>>>>>> If I commit this, (I think) this is going to cause buildbot to break
>>>>>>>> for the aarch64 builds.
>>>>>>>> (Out of interest - I’ve heard people say they tested on buildbot. Are
>>>>>>>> there some instructions for doing that? I can try it out.)
>>>>>>>
>>>>>>> Hmm you're right. Though maybe we can have additional
>>>>>>> commands/configure options specific to the aarch64 builders? They
>>>>>>> could download a kernel tarball, install the headers somewhere (that
>>>>>>> doesn't take long, no need to build the kernel) and point to them.
>>>>>>> Sergio, would that be possible/a good idea?
>>>>>>
>>>>>> I'm not sure. For starters, the Aarch64 builders have kinda been
>>>>>> forgotten since Yao stopped contributing regularly to GDB (he is the
>>>>>> maintainer of the machines behind the builders). So the very first
>>>>>> thing we'd need to do is to put the builders in a good shape again
>>>>>> (they're currently with 273 pending builds in the queue!). This is
>>>>>> something that's been on my TODO list for a while now, and I was going
>>>>>> to ask Alan (or anyone from ARM) if they're not interested in taking
>>>>>> over the maintenance of these machines.
>>>>>
>>>>> Looking after the aarch64 boxes does sound like a job for an Arm person.
>>>>> I guess it’ll be fairly important to get those queues cleared _before_
>>>>> 8.2 is released. I can certainly take a look at the pending builds in
>>>>> the next few weeks.
>>>> Yeah, what I do in these cases is cancel all of the pending builds,
>>>> i.e., start fresh.
>>>> I'm glad you're interested in taking care of the machines. TBH, I was
>>>> even considering disabling them for now, since at least one machine has
>>>> been offline for a long time, and the other has this giant queue. I'm
>>>> not sure if you're going to use the same machines as Yao was using
>>>> (IIRC, he was using machines from the GCC Compile Farm).
>>>>>> Then, I think the best approach for the SVE builds would be to manually
>>>>>> download a Linux kernel, put the sources somewhere, and then I could
>>>>>> configure a specific builder to build GDB with the SVE headers.
>>>>>>
>>>>>
>>>>> Given the current queues, I suspect we’d not get this done before the 8.2
>>>>> branch.
>>>> I wouldn't count on that.
>>>>> I’m thinking configure check of Pedro’s sounds the first step, then once
>>>>> the aarch64 build queues have been cleared, get some sve builds added.
>>>>>
>>>>> The SVE headers are in Ubuntu 18.04 - so “all” that’s needed is to do a
>>>>> dist upgrade on them (I suspect there are probably lots of reasons why
>>>>> that can’t be done!)
>>>> Yeah, I honestly don't know :-/. If you're planning to continue using
>>>> the GCC Farm machine, then I think the best option would be to contact
>>>> the admins and ask them.
>>>
>>> So I arranged for those machines in the compile farm and I believe I
>>> have super user privileges on them. Updating to 18.04 may be an
>>> option but something I don't want to do remotely and in a rush.
>>>
>>> Alan, please reach out to me if you need any help on the compile farm machines, I can help out.
>>>
>>
>> Oh that’s good news (I thought they had come from Linaro).
>>
>> If the headers get added to the gdb build then there’s no urgency to
>> update the boxes to ubuntu 18.04. However, 18.04 is the latest LTS, so
>> it’s possibly worth doing eventually (but not right now).
>
> Hi guys,
>
> Just a ping to see if you have progressed on this. I've left the AArch*
> builders there, and now they're *really* behind (more than 1000 builds
> in the queue), and at least one of the buildslaves is offline.
>
> I will temporarily remove the builders now, but it would be really nice
> to keep having AArch* builders in our BuildBot.
>
> Thanks a lot,
Ramana has got some aarch64 machines up on packet.net for use in buildbot instead
of the existing machines. I think a few things just need finalising before they can
be handed over.
Once that’s done I can get buildbot set up on them. Are there some simple instructions
for getting this going?
Alan.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: Status of the AArch* builders
2018-09-24 14:16 ` Alan Hayward
@ 2018-09-24 14:42 ` Sergio Durigan Junior
2018-10-11 9:23 ` Alan Hayward
0 siblings, 1 reply; 56+ messages in thread
From: Sergio Durigan Junior @ 2018-09-24 14:42 UTC (permalink / raw)
To: Alan Hayward
Cc: Ramana Radhakrishnan, Simon Marchi, GDB Patches, Simon Marchi,
nd, Pedro Alves
On Monday, September 24 2018, Alan Hayward wrote:
>> On 20 Sep 2018, at 22:15, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>> Hi guys,
>>
>> Just a ping to see if you have progressed on this. I've left the AArch*
>> builders there, and now they're *really* behind (more than 1000 builds
>> in the queue), and at least one of the buildslaves is offline.
>>
>> I will temporarily remove the builders now, but it would be really nice
>> to keep having AArch* builders in our BuildBot.
>>
>> Thanks a lot,
>
>
> Ramana has got some aarch64 machines up on packet.net for use in buildbot instead
> of the existing machines. I think a few things just need finalising before they can
> be handed over.
That's great news. Thanks for doing that.
> Once that’s done I can get buildbot set up on them. Are there some simple instructions
> for getting this going?
There are instructions on our wiki:
https://sourceware.org/gdb/wiki/BuildBot#How_to_add_your_buildslave
But please do let me know if you need any help. I can take care of the
configuration on my side, so you don't have to submit a patch for the
master.cfg file (although you can if you want).
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] 56+ messages in thread
* Re: Status of the AArch* builders
2018-09-24 14:42 ` Status of the AArch* builders Sergio Durigan Junior
@ 2018-10-11 9:23 ` Alan Hayward
2018-10-12 19:06 ` Sergio Durigan Junior
0 siblings, 1 reply; 56+ messages in thread
From: Alan Hayward @ 2018-10-11 9:23 UTC (permalink / raw)
To: Sergio Durigan Junior
Cc: Ramana Radhakrishnan, Simon Marchi, GDB Patches, Simon Marchi,
nd, Pedro Alves
> On 24 Sep 2018, at 15:39, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>
> On Monday, September 24 2018, Alan Hayward wrote:
>
>>> On 20 Sep 2018, at 22:15, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>>> Hi guys,
>>>
>>> Just a ping to see if you have progressed on this. I've left the AArch*
>>> builders there, and now they're *really* behind (more than 1000 builds
>>> in the queue), and at least one of the buildslaves is offline.
>>>
>>> I will temporarily remove the builders now, but it would be really nice
>>> to keep having AArch* builders in our BuildBot.
>>>
>>> Thanks a lot,
>>
>>
>> Ramana has got some aarch64 machines up on packet.net for use in buildbot instead
>> of the existing machines. I think a few things just need finalising before they can
>> be handed over.
>
> That's great news. Thanks for doing that.
>
>> Once that’s done I can get buildbot set up on them. Are there some simple instructions
>> for getting this going?
>
> There are instructions on our wiki:
>
> https://sourceware.org/gdb/wiki/BuildBot#How_to_add_your_buildslave
>
> But please do let me know if you need any help. I can take care of the
> configuration on my side, so you don't have to submit a patch for the
> master.cfg file (although you can if you want).
>
The machine is now ready for buildbot!
Aarch64, Ubuntu 16.04.5 LTS, 96 cores
I’ve setup buildbot-slave-0.8.14 in a virtualenv/
(Oddly, I had to install twisted==16.4.1, as anything newer than that caused a hang).
I’ve manually checked you can build gdb and run the testsuite.
My recent experiments with the testsuite on Aarch64 show all the threaded tests
are quite racy on a fully loaded ubuntu, whereas on redhat/suse they are fairly
stable. I’m still looking into why this is. But, in the short-term maybe we should
restrict the number of jobs to 32 (or maybe even fewer?)
Sergio, could you please add the relevant server config.
Thanks,
Alan.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: Status of the AArch* builders
2018-10-11 9:23 ` Alan Hayward
@ 2018-10-12 19:06 ` Sergio Durigan Junior
2018-10-15 10:16 ` Alan Hayward
0 siblings, 1 reply; 56+ messages in thread
From: Sergio Durigan Junior @ 2018-10-12 19:06 UTC (permalink / raw)
To: Alan Hayward
Cc: Ramana Radhakrishnan, Simon Marchi, GDB Patches, Simon Marchi,
nd, Pedro Alves
On Thursday, October 11 2018, Alan Hayward wrote:
>> On 24 Sep 2018, at 15:39, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>>
>> On Monday, September 24 2018, Alan Hayward wrote:
>>
>>>> On 20 Sep 2018, at 22:15, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>>>> Hi guys,
>>>>
>>>> Just a ping to see if you have progressed on this. I've left the AArch*
>>>> builders there, and now they're *really* behind (more than 1000 builds
>>>> in the queue), and at least one of the buildslaves is offline.
>>>>
>>>> I will temporarily remove the builders now, but it would be really nice
>>>> to keep having AArch* builders in our BuildBot.
>>>>
>>>> Thanks a lot,
>>>
>>>
>>> Ramana has got some aarch64 machines up on packet.net for use in buildbot instead
>>> of the existing machines. I think a few things just need finalising before they can
>>> be handed over.
>>
>> That's great news. Thanks for doing that.
>>
>>> Once that’s done I can get buildbot set up on them. Are there some simple instructions
>>> for getting this going?
>>
>> There are instructions on our wiki:
>>
>> https://sourceware.org/gdb/wiki/BuildBot#How_to_add_your_buildslave
>>
>> But please do let me know if you need any help. I can take care of the
>> configuration on my side, so you don't have to submit a patch for the
>> master.cfg file (although you can if you want).
>>
>
> The machine is now ready for buildbot!
>
> Aarch64, Ubuntu 16.04.5 LTS, 96 cores
That's great news, Alan!
> I’ve setup buildbot-slave-0.8.14 in a virtualenv/
> (Oddly, I had to install twisted==16.4.1, as anything newer than that caused a hang).
>
> I’ve manually checked you can build gdb and run the testsuite.
>
> My recent experiments with the testsuite on Aarch64 show all the threaded tests
> are quite racy on a fully loaded ubuntu, whereas on redhat/suse they are fairly
> stable. I’m still looking into why this is. But, in the short-term maybe we should
> restrict the number of jobs to 32 (or maybe even fewer?)
Sure, no problem. What do you think of 16?
> Sergio, could you please add the relevant server config.
It's a good idea to follow the instructions here:
<https://sourceware.org/gdb/wiki/BuildBot#Buildslave_configuration>
And make sure that all of the necessary/recommended deps are installed
in the machine. The more deps, the more tests will be performed.
You will need a password to connect to the BuildBot master. I will send
it to you in private.
I also recommend creating at least 3 builders associated with each
slave: native, native-gdbserver, and native-extended-gdbserver. If
you're OK with it, I'll do that.
Last question: is there any special flags needed to build GDB on the
machine?
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] 56+ messages in thread
* Re: Status of the AArch* builders
2018-10-12 19:06 ` Sergio Durigan Junior
@ 2018-10-15 10:16 ` Alan Hayward
2018-10-15 12:42 ` Sergio Durigan Junior
0 siblings, 1 reply; 56+ messages in thread
From: Alan Hayward @ 2018-10-15 10:16 UTC (permalink / raw)
To: Sergio Durigan Junior
Cc: Ramana Radhakrishnan, Simon Marchi, GDB Patches, Simon Marchi,
nd, Pedro Alves
> On 12 Oct 2018, at 20:06, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>
> On Thursday, October 11 2018, Alan Hayward wrote:
>
>>> On 24 Sep 2018, at 15:39, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>>>
>>> On Monday, September 24 2018, Alan Hayward wrote:
>>>
>>>>> On 20 Sep 2018, at 22:15, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>>>>> Hi guys,
>>>>>
>>>>> Just a ping to see if you have progressed on this. I've left the AArch*
>>>>> builders there, and now they're *really* behind (more than 1000 builds
>>>>> in the queue), and at least one of the buildslaves is offline.
>>>>>
>>>>> I will temporarily remove the builders now, but it would be really nice
>>>>> to keep having AArch* builders in our BuildBot.
>>>>>
>>>>> Thanks a lot,
>>>>
>>>>
>>>> Ramana has got some aarch64 machines up on packet.net for use in buildbot instead
>>>> of the existing machines. I think a few things just need finalising before they can
>>>> be handed over.
>>>
>>> That's great news. Thanks for doing that.
>>>
>>>> Once that’s done I can get buildbot set up on them. Are there some simple instructions
>>>> for getting this going?
>>>
>>> There are instructions on our wiki:
>>>
>>> https://sourceware.org/gdb/wiki/BuildBot#How_to_add_your_buildslave
>>>
>>> But please do let me know if you need any help. I can take care of the
>>> configuration on my side, so you don't have to submit a patch for the
>>> master.cfg file (although you can if you want).
>>>
>>
>> The machine is now ready for buildbot!
>>
>> Aarch64, Ubuntu 16.04.5 LTS, 96 cores
>
> That's great news, Alan!
>
>> I’ve setup buildbot-slave-0.8.14 in a virtualenv/
>> (Oddly, I had to install twisted==16.4.1, as anything newer than that caused a hang).
>>
>> I’ve manually checked you can build gdb and run the testsuite.
>>
>> My recent experiments with the testsuite on Aarch64 show all the threaded tests
>> are quite racy on a fully loaded ubuntu, whereas on redhat/suse they are fairly
>> stable. I’m still looking into why this is. But, in the short-term maybe we should
>> restrict the number of jobs to 32 (or maybe even fewer?)
>
> Sure, no problem. What do you think of 16?
I’ve been running some more tests over the weekend. At 32 I still get quite a bit of racy
behaviour, and at 16 it looks roughly the same as an x86 run.
So yes, 16 sounds good.
>
>> Sergio, could you please add the relevant server config.
>
> It's a good idea to follow the instructions here:
>
> <https://sourceware.org/gdb/wiki/BuildBot#Buildslave_configuration>
>
> And make sure that all of the necessary/recommended deps are installed
> in the machine. The more deps, the more tests will be performed.
All looks good.
I’m not sure who gets access to the wiki (looks like I can’t log in).
Errors I noticed:
* There is a mention of both 0.8.14 and 0.8.12 for buildslave
* The Debian specific instructions should probably also be for Ubuntu too.
>
> You will need a password to connect to the BuildBot master. I will send
> it to you in private.
Slave created.
>
> I also recommend creating at least 3 builders associated with each
> slave: native, native-gdbserver, and native-extended-gdbserver. If
> you're OK with it, I'll do that.
>
That’s fine.
> Last question: is there any special flags needed to build GDB on the
> machine?
>
Nope. My usual build line is:
$ configure --enable-sim --disable-gprof --disable-gold --disable-gas
$ make
> 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] 56+ messages in thread
* Re: Status of the AArch* builders
2018-10-15 10:16 ` Alan Hayward
@ 2018-10-15 12:42 ` Sergio Durigan Junior
2018-10-15 14:02 ` Alan Hayward
0 siblings, 1 reply; 56+ messages in thread
From: Sergio Durigan Junior @ 2018-10-15 12:42 UTC (permalink / raw)
To: Alan Hayward
Cc: Ramana Radhakrishnan, Simon Marchi, GDB Patches, Simon Marchi,
nd, Pedro Alves
On Monday, October 15 2018, Alan Hayward wrote:
>> On 12 Oct 2018, at 20:06, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>>
>> On Thursday, October 11 2018, Alan Hayward wrote:
>>
>>> I’ve setup buildbot-slave-0.8.14 in a virtualenv/
>>> (Oddly, I had to install twisted==16.4.1, as anything newer than that caused a hang).
>>>
>>> I’ve manually checked you can build gdb and run the testsuite.
>>>
>>> My recent experiments with the testsuite on Aarch64 show all the threaded tests
>>> are quite racy on a fully loaded ubuntu, whereas on redhat/suse they are fairly
>>> stable. I’m still looking into why this is. But, in the short-term maybe we should
>>> restrict the number of jobs to 32 (or maybe even fewer?)
>>
>> Sure, no problem. What do you think of 16?
>
> I’ve been running some more tests over the weekend. At 32 I still get quite a bit of racy
> behaviour, and at 16 it looks roughly the same as an x86 run.
>
> So yes, 16 sounds good.
Cool, I configured the buildslave to use 16 cores.
>>
>>> Sergio, could you please add the relevant server config.
>>
>> It's a good idea to follow the instructions here:
>>
>> <https://sourceware.org/gdb/wiki/BuildBot#Buildslave_configuration>
>>
>> And make sure that all of the necessary/recommended deps are installed
>> in the machine. The more deps, the more tests will be performed.
>
> All looks good.
>
> I’m not sure who gets access to the wiki (looks like I can’t log in).
> Errors I noticed:
> * There is a mention of both 0.8.14 and 0.8.12 for buildslave
Fixed.
> * The Debian specific instructions should probably also be for Ubuntu too.
Fixed.
Thanks for the heads up.
>>
>> You will need a password to connect to the BuildBot master. I will send
>> it to you in private.
>
> Slave created.
Hm, how did you create the slave? I don't see it connected to the
BuildBot master:
https://gdb-build.sergiodj.net/buildslaves/ubuntu16-aarch64
>>
>> I also recommend creating at least 3 builders associated with each
>> slave: native, native-gdbserver, and native-extended-gdbserver. If
>> you're OK with it, I'll do that.
>>
>
> That’s fine.
Done.
>> Last question: is there any special flags needed to build GDB on the
>> machine?
>>
>
> Nope. My usual build line is:
> $ configure --enable-sim --disable-gprof --disable-gold --disable-gas
> $ make
That's great, it should work without modifications then.
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] 56+ messages in thread
* Re: Status of the AArch* builders
2018-10-15 12:42 ` Sergio Durigan Junior
@ 2018-10-15 14:02 ` Alan Hayward
2018-10-15 15:32 ` Sergio Durigan Junior
0 siblings, 1 reply; 56+ messages in thread
From: Alan Hayward @ 2018-10-15 14:02 UTC (permalink / raw)
To: Sergio Durigan Junior
Cc: Ramana Radhakrishnan, Simon Marchi, GDB Patches, Simon Marchi,
nd, Pedro Alves
>
>>>
>>> You will need a password to connect to the BuildBot master. I will send
>>> it to you in private.
>>
>> Slave created.
>
> Hm, how did you create the slave? I don't see it connected to the
> BuildBot master:
>
> https://gdb-build.sergiodj.net/buildslaves/ubuntu16-aarch64
>
It’s connected properly now.
I've clicked retry on the last aarch64 build (from March 2018), and that has passed.
There is also a new HEAD commit which is now running.
Anything else I need to do?
And is there anything else I should be monitoring (or will all fails just magically go
to the mailing list) ?
Alan.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: Status of the AArch* builders
2018-10-15 14:02 ` Alan Hayward
@ 2018-10-15 15:32 ` Sergio Durigan Junior
2018-10-17 18:46 ` Sergio Durigan Junior
0 siblings, 1 reply; 56+ messages in thread
From: Sergio Durigan Junior @ 2018-10-15 15:32 UTC (permalink / raw)
To: Alan Hayward
Cc: Ramana Radhakrishnan, Simon Marchi, GDB Patches, Simon Marchi,
nd, Pedro Alves
On Monday, October 15 2018, Alan Hayward wrote:
>>
>>>>
>>>> You will need a password to connect to the BuildBot master. I will send
>>>> it to you in private.
>>>
>>> Slave created.
>>
>> Hm, how did you create the slave? I don't see it connected to the
>> BuildBot master:
>>
>> https://gdb-build.sergiodj.net/buildslaves/ubuntu16-aarch64
>>
>
> It’s connected properly now.
Cool.
> I've clicked retry on the last aarch64 build (from March 2018), and that has passed.
>
> There is also a new HEAD commit which is now running.
>
> Anything else I need to do?
> And is there anything else I should be monitoring (or will all fails just magically go
> to the mailing list) ?
For now, I have disabled the email notifications. IME there's always
something that we need to tweak in the first days to make sure that the
builders are running fine. So if you could just keep an eye on the
builds and make sure that everything is OK, I'd appreciate.
After a few days have passed, I will enable the email notifications.
Regular test failures will be sent to the gdb-testers@ ml, and breakages
will be sent to gdb-patches@.
That should be it :-).
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] 56+ messages in thread
* Re: Status of the AArch* builders
2018-10-15 15:32 ` Sergio Durigan Junior
@ 2018-10-17 18:46 ` Sergio Durigan Junior
2018-10-24 9:56 ` Alan Hayward
0 siblings, 1 reply; 56+ messages in thread
From: Sergio Durigan Junior @ 2018-10-17 18:46 UTC (permalink / raw)
To: Alan Hayward
Cc: Ramana Radhakrishnan, Simon Marchi, GDB Patches, Simon Marchi,
nd, Pedro Alves
On Monday, October 15 2018, I wrote:
> On Monday, October 15 2018, Alan Hayward wrote:
>
>>>
>>>>>
>>>>> You will need a password to connect to the BuildBot master. I will send
>>>>> it to you in private.
>>>>
>>>> Slave created.
>>>
>>> Hm, how did you create the slave? I don't see it connected to the
>>> BuildBot master:
>>>
>>> https://gdb-build.sergiodj.net/buildslaves/ubuntu16-aarch64
>>>
>>
>> It’s connected properly now.
>
> Cool.
>
>> I've clicked retry on the last aarch64 build (from March 2018), and that has passed.
>>
>> There is also a new HEAD commit which is now running.
>>
>> Anything else I need to do?
>> And is there anything else I should be monitoring (or will all fails just magically go
>> to the mailing list) ?
>
> For now, I have disabled the email notifications. IME there's always
> something that we need to tweak in the first days to make sure that the
> builders are running fine. So if you could just keep an eye on the
> builds and make sure that everything is OK, I'd appreciate.
>
> After a few days have passed, I will enable the email notifications.
> Regular test failures will be sent to the gdb-testers@ ml, and breakages
> will be sent to gdb-patches@.
For the record, I've now enabled the e-mail notifications for the
Aarch64 builders. I've also added them to the list of Try builders, so
it's possible to submit try jobs to them.
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] 56+ messages in thread
* Re: Status of the AArch* builders
2018-10-17 18:46 ` Sergio Durigan Junior
@ 2018-10-24 9:56 ` Alan Hayward
2018-10-25 16:26 ` Sergio Durigan Junior
0 siblings, 1 reply; 56+ messages in thread
From: Alan Hayward @ 2018-10-24 9:56 UTC (permalink / raw)
To: Sergio Durigan Junior
Cc: Ramana Radhakrishnan, Simon Marchi, GDB Patches, Simon Marchi,
nd, Pedro Alves
> On 17 Oct 2018, at 19:45, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>
>> For now, I have disabled the email notifications. IME there's always
>> something that we need to tweak in the first days to make sure that the
>> builders are running fine. So if you could just keep an eye on the
>> builds and make sure that everything is OK, I'd appreciate.
>>
>> After a few days have passed, I will enable the email notifications.
>> Regular test failures will be sent to the gdb-testers@ ml, and breakages
>> will be sent to gdb-patches@.
>
> For the record, I've now enabled the e-mail notifications for the
> Aarch64 builders. I've also added them to the list of Try builders, so
> it's possible to submit try jobs to them.
>
Currently the AArch64 builders keep going back to failed regressions. This is
mostly due to *all* the gdb.threads and gdb.server tests being racy on AArch64
Ubuntu. Keeping it down to 16 threads has reduced the frequency, but not
removed it.
I’m currently trying to fix up as many of the AArch64 test failures across the
whole test suite as I can. I suspect fixing the threaded issue is going to
take a while longer.
In the meantime is it possible to update just the AArch64 buildbot scripts so
they don’t take into account the thread/server tests for regressions? Either
remove the tests before running, or grep them out of the results. Not sure if
that’s possible with the way it’s set up (and not sure where to look). This
would then give us confidence with the rest of the tests. They can be
reinstated when stable again.
Alan.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: Status of the AArch* builders
2018-10-24 9:56 ` Alan Hayward
@ 2018-10-25 16:26 ` Sergio Durigan Junior
0 siblings, 0 replies; 56+ messages in thread
From: Sergio Durigan Junior @ 2018-10-25 16:26 UTC (permalink / raw)
To: Alan Hayward
Cc: Ramana Radhakrishnan, Simon Marchi, GDB Patches, Simon Marchi,
nd, Pedro Alves
On Wednesday, October 24 2018, Alan Hayward wrote:
>> On 17 Oct 2018, at 19:45, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>>
>>> For now, I have disabled the email notifications. IME there's always
>>> something that we need to tweak in the first days to make sure that the
>>> builders are running fine. So if you could just keep an eye on the
>>> builds and make sure that everything is OK, I'd appreciate.
>>>
>>> After a few days have passed, I will enable the email notifications.
>>> Regular test failures will be sent to the gdb-testers@ ml, and breakages
>>> will be sent to gdb-patches@.
>>
>> For the record, I've now enabled the e-mail notifications for the
>> Aarch64 builders. I've also added them to the list of Try builders, so
>> it's possible to submit try jobs to them.
>>
>
> Currently the AArch64 builders keep going back to failed regressions. This is
> mostly due to *all* the gdb.threads and gdb.server tests being racy on AArch64
> Ubuntu. Keeping it down to 16 threads has reduced the frequency, but not
> removed it.
> I’m currently trying to fix up as many of the AArch64 test failures across the
> whole test suite as I can. I suspect fixing the threaded issue is going to
> take a while longer.
TBH, all builders suffer from this problem. They all have racy tests,
and even though I tried to implement a system to detect such tests and
exclude them from the reports that are sent to gdb-testers, they still
sneak in the reports. That's the main (and sole?) reason why
gdb-testers is currently impossible to follow, and people don't really
read it.
> In the meantime is it possible to update just the AArch64 buildbot scripts so
> they don’t take into account the thread/server tests for regressions? Either
> remove the tests before running, or grep them out of the results. Not sure if
> that’s possible with the way it’s set up (and not sure where to look). This
> would then give us confidence with the rest of the tests. They can be
> reinstated when stable again.
I think it can be done, but it's not so trivial, and I'm busy with other
stuff right now :-/. Sorry about that.
I'll try to take a look at this problem during the weekend. BTW, the
configuration files for the GDB BuildBot live at:
https://git.sergiodj.net/gdb-buildbot.git/
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] 56+ messages in thread
* Re: [PATCH v2 02/10] Add Aarch64 SVE Linux headers
2018-06-08 14:37 ` Simon Marchi
2018-06-08 15:23 ` Simon Marchi
@ 2018-06-08 15:27 ` Alan Hayward
1 sibling, 0 replies; 56+ messages in thread
From: Alan Hayward @ 2018-06-08 15:27 UTC (permalink / raw)
To: Simon Marchi; +Cc: GDB Patches, Simon Marchi, nd
> On 8 Jun 2018, at 15:37, Simon Marchi <simon.marchi@polymtl.ca> wrote:
>
> On 2018-06-08 10:13, Alan Hayward wrote:
>> (Moved review to correct thread)
>> Thanks for the reviews.
>>> On 7 Jun 2018, at 21:18, Simon Marchi <simon.marchi@ericsson.com> wrote:
>>> Hi Alan,
>>> Just some quick comments.
>>> I get this when building on x86-64 with --enable-targets=all:
>> Hmm.. I had lost that flag from my build script. I Re-added it, and
>> reproduced the issues.
>>> CXX aarch64-tdep.o
>>> In file included from /home/emaisin/src/binutils-gdb/gdb/nat/aarch64-sve-linux-ptrace.h:29:0,
>>> from /home/emaisin/src/binutils-gdb/gdb/aarch64-tdep.c:61:
>>> /home/emaisin/src/binutils-gdb/gdb/nat/aarch64-linux-sigcontext.h:19:22: error: field ‘head’ has incomplete type ‘_aarch64_ctx’
>>> struct _aarch64_ctx head;
>>> ^
>>> /home/emaisin/src/binutils-gdb/gdb/nat/aarch64-linux-sigcontext.h:19:9: note: forward declaration of ‘struct _aarch64_ctx’
>>> struct _aarch64_ctx head;
>>> ^
>>> First, we should not include "nat/aarch64-sve-linux-ptrace.h" (a file that only makes
>>> sense when building on AArch64) in aarch64-tdep.c, a file built on all architecture
>>> when including the support for AArch64 debugging. It looks like aarch64-tdep.c
>>> needs sve_vq_from_vl. Maybe that definition could be moved to arch/, which can be
>>> included in aarch64-tdep.c.
>> I had put it in there because I wanted to try and make it a complete block
>> copied from Linux. The issue makes sense, so I’ve updated to restore
>> sve_vq_from_vl/sve_vl_from_vq back to arch/aarch64.h and removed it from
>> nat/aarch64-linux-sigcontext.h
>>> Then, is the _aarch64_ctx structure guaranteed to be defined on older AArch64 kernels
>>> or should we include it too?
>> _aarch64_ctx is part of the standard aarch64 signal handling. A quick
>> git blame gives
>> me 2012 - which is roughly the age of aarch64. So, it should always be defined.
>> Updated patch below. Checked it builds (with other sve patches) on:
>> X86 all-targets
>> Aarch64 Linux 4.10 (pre sve headers) ubuntu 16.04
>> Aarch64 Linux 4.15 (with sve headers) ubuntu 18.04
>> Are you ok with the new version?
>
> The code looks good to me, thanks. I am still unsure about the licensing side of it, let me ask the FSF people about it, I'll come back to you when it's done. I hope it won't take too long!
>
Ok, thanks for chasing that up. Happy to be cc’ed (or not) on any email.
This patch (I think) only blocks 5/10 and 9/10 in the series. The rest should be
ok to still go in (once reviewed).
Alan.
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 09/10] Ptrace support for AArch64 SVE gdbsever
2018-06-06 15:16 [PATCH v2 00/10] gdb/gdbserver support for aarch64 SVE Alan Hayward
` (2 preceding siblings ...)
2018-06-06 15:17 ` [PATCH v2 02/10] Add Aarch64 SVE Linux headers Alan Hayward
@ 2018-06-06 15:17 ` Alan Hayward
2018-06-11 2:43 ` Simon Marchi
2018-06-11 2:44 ` Simon Marchi
2018-06-06 15:17 ` [PATCH v2 01/10] Aarch64 SVE pseudo register support Alan Hayward
` (5 subsequent siblings)
9 siblings, 2 replies; 56+ messages in thread
From: Alan Hayward @ 2018-06-06 15:17 UTC (permalink / raw)
To: gdb-patches; +Cc: nd, Alan Hayward
Add checks to detect SVE tdesc. Easiest way to do this is by checking the
size of the vector registers.
Use the common aarch64 ptrace copy functions for reading/writing registers.
A wrapper is required due to the common functions using reg_buffer_common.
2018-06-06 Alan Hayward <alan.hayward@arm.com>
gdbserver/
* linux-aarch64-low.c (is_sve_tdesc): New function.
(aarch64_sve_regs_copy_to_regcache): Likewise.
(aarch64_sve_regs_copy_from_regcache): Likewise.
(aarch64_regs_info): Add SVE checks.
(initialize_low_arch): Initialize SVE.
---
gdb/gdbserver/linux-aarch64-low.c | 62 +++++++++++++++++++++++++++++++++++++--
1 file changed, 59 insertions(+), 3 deletions(-)
diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
index 9db9a7c1c3..e7fec25a64 100644
--- a/gdb/gdbserver/linux-aarch64-low.c
+++ b/gdb/gdbserver/linux-aarch64-low.c
@@ -73,6 +73,16 @@ is_64bit_tdesc (void)
return register_size (regcache->tdesc, 0) == 8;
}
+static bool
+is_sve_tdesc (void)
+{
+ struct regcache *regcache = get_thread_regcache (current_thread, 0);
+
+ /* Mimimum size of Z0 on SVE is 256bit. If not SVE, then Z0 will be the
+ 128bit V0 register. */
+ return register_size (regcache->tdesc, AARCH64_SVE_Z0_REGNUM) >= 32;
+}
+
/* Implementation of linux_target_ops method "cannot_store_register". */
static int
@@ -514,6 +524,22 @@ aarch64_arch_setup (void)
aarch64_linux_get_debug_reg_capacity (lwpid_of (current_thread));
}
+/* Wrapper for aarch64_sve_regs_copy_to_reg_buf. */
+
+static void
+aarch64_sve_regs_copy_to_regcache (struct regcache *regcache, const void *buf)
+{
+ return aarch64_sve_regs_copy_to_reg_buf (regcache, buf);
+}
+
+/* Wrapper for aarch64_sve_regs_copy_from_reg_buf. */
+
+static void
+aarch64_sve_regs_copy_from_regcache (struct regcache *regcache, void *buf)
+{
+ return aarch64_sve_regs_copy_from_reg_buf (regcache, buf);
+}
+
static struct regset_info aarch64_regsets[] =
{
{ PTRACE_GETREGSET, PTRACE_SETREGSET, NT_PRSTATUS,
@@ -540,15 +566,44 @@ static struct regs_info regs_info_aarch64 =
&aarch64_regsets_info,
};
+static struct regset_info aarch64_sve_regsets[] =
+{
+ { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_PRSTATUS,
+ sizeof (struct user_pt_regs), GENERAL_REGS,
+ aarch64_fill_gregset, aarch64_store_gregset },
+ { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_ARM_SVE,
+ SVE_PT_SIZE (AARCH64_MAX_SVE_VQ, SVE_PT_REGS_SVE), EXTENDED_REGS,
+ aarch64_sve_regs_copy_from_regcache, aarch64_sve_regs_copy_to_regcache
+ },
+ NULL_REGSET
+};
+
+static struct regsets_info aarch64_sve_regsets_info =
+ {
+ aarch64_sve_regsets, /* regsets. */
+ 0, /* num_regsets. */
+ NULL, /* disabled_regsets. */
+ };
+
+static struct regs_info regs_info_aarch64_sve =
+ {
+ NULL, /* regset_bitmap. */
+ NULL, /* usrregs. */
+ &aarch64_sve_regsets_info,
+ };
+
/* Implementation of linux_target_ops method "regs_info". */
static const struct regs_info *
aarch64_regs_info (void)
{
- if (is_64bit_tdesc ())
- return ®s_info_aarch64;
- else
+ if (!is_64bit_tdesc ())
return ®s_info_aarch32;
+
+ if (is_sve_tdesc ())
+ return ®s_info_aarch64_sve;
+
+ return ®s_info_aarch64;
}
/* Implementation of linux_target_ops method "supports_tracepoints". */
@@ -3027,6 +3082,7 @@ initialize_low_arch (void)
initialize_low_arch_aarch32 ();
initialize_regsets_info (&aarch64_regsets_info);
+ initialize_regsets_info (&aarch64_sve_regsets_info);
#if GDB_SELF_TEST
initialize_low_tdesc ();
--
2.15.1 (Apple Git-101)
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 09/10] Ptrace support for AArch64 SVE gdbsever
2018-06-06 15:17 ` [PATCH v2 09/10] Ptrace support for AArch64 SVE gdbsever Alan Hayward
@ 2018-06-11 2:43 ` Simon Marchi
2018-06-11 2:44 ` Simon Marchi
1 sibling, 0 replies; 56+ messages in thread
From: Simon Marchi @ 2018-06-11 2:43 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
On 2018-06-06 11:16 AM, Alan Hayward wrote:
> Add checks to detect SVE tdesc. Easiest way to do this is by checking the
> size of the vector registers.
>
> Use the common aarch64 ptrace copy functions for reading/writing registers.
> A wrapper is required due to the common functions using reg_buffer_common.
Hi Alan,
There are macros not defined in the headers adde in patch 2/10, so I get this since
I build against the headers of an older kernel:
CXX linux-aarch64-low.o
/home/simark/src/binutils-gdb/gdb/gdbserver/linux-aarch64-low.c:559:38: error: 'SVE_PT_REGS_SVE' was not declared in this scope
SVE_PT_SIZE (AARCH64_MAX_SVE_VQ, SVE_PT_REGS_SVE), EXTENDED_REGS,
^~~~~~~~~~~~~~~
/home/simark/src/binutils-gdb/gdb/gdbserver/linux-aarch64-low.c:559:53: error: 'SVE_PT_SIZE' was not declared in this scope
SVE_PT_SIZE (AARCH64_MAX_SVE_VQ, SVE_PT_REGS_SVE), EXTENDED_REGS,
If we decide not to include the Linux kernel header replacements in the GDB tree,
it's not an issue.
Simon
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 09/10] Ptrace support for AArch64 SVE gdbsever
2018-06-06 15:17 ` [PATCH v2 09/10] Ptrace support for AArch64 SVE gdbsever Alan Hayward
2018-06-11 2:43 ` Simon Marchi
@ 2018-06-11 2:44 ` Simon Marchi
1 sibling, 0 replies; 56+ messages in thread
From: Simon Marchi @ 2018-06-11 2:44 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
On 2018-06-06 11:16 AM, Alan Hayward wrote:
> diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
> index 9db9a7c1c3..e7fec25a64 100644
> --- a/gdb/gdbserver/linux-aarch64-low.c
> +++ b/gdb/gdbserver/linux-aarch64-low.c
> @@ -73,6 +73,16 @@ is_64bit_tdesc (void)
> return register_size (regcache->tdesc, 0) == 8;
> }
>
> +static bool
> +is_sve_tdesc (void)
> +{
> + struct regcache *regcache = get_thread_regcache (current_thread, 0);
> +
> + /* Mimimum size of Z0 on SVE is 256bit. If not SVE, then Z0 will be the
> + 128bit V0 register. */
> + return register_size (regcache->tdesc, AARCH64_SVE_Z0_REGNUM) >= 32;
Hmm maybe I read wrong, but can't the SVE registers be 128 bits long? Maybe this
is not up to date, but here it says
"enabling implementation choices for vector lengths that scale from 128 to 2048 bits."
https://community.arm.com/processors/b/blog/posts/technology-update-the-scalable-vector-extension-sve-for-the-armv8-a-architecture
Simon
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 01/10] Aarch64 SVE pseudo register support
2018-06-06 15:16 [PATCH v2 00/10] gdb/gdbserver support for aarch64 SVE Alan Hayward
` (3 preceding siblings ...)
2018-06-06 15:17 ` [PATCH v2 09/10] Ptrace support for AArch64 SVE gdbsever Alan Hayward
@ 2018-06-06 15:17 ` Alan Hayward
2018-06-06 22:17 ` Simon Marchi
2018-06-06 15:17 ` [PATCH v2 07/10] Increase gdbsever PBUFSIZ Alan Hayward
` (4 subsequent siblings)
9 siblings, 1 reply; 56+ messages in thread
From: Alan Hayward @ 2018-06-06 15:17 UTC (permalink / raw)
To: gdb-patches; +Cc: nd, Alan Hayward
Add the functionality for reading/writing pseudo registers.
On SVE the V registers are pseudo registers. This is supported
by adding AARCH64_SVE_V0_REGNUM.
2018-06-06 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 | 131 ++++++++++++++++++++++++++++++++++++++++++++---------
gdb/aarch64-tdep.h | 1 +
2 files changed, 111 insertions(+), 21 deletions(-)
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index c6b1e2a9a3..2fe6a4006c 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");
}
diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
index 598a0aafa2..5a319551e6 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] 56+ messages in thread* Re: [PATCH v2 01/10] Aarch64 SVE pseudo register support
2018-06-06 15:17 ` [PATCH v2 01/10] Aarch64 SVE pseudo register support Alan Hayward
@ 2018-06-06 22:17 ` Simon Marchi
2018-06-07 9:34 ` Alan Hayward
0 siblings, 1 reply; 56+ messages in thread
From: Simon Marchi @ 2018-06-06 22:17 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
Hi Alan,
I have some more nits/suggestions, feel free to cherry-pick the ones you
want and push the result.
On 2018-06-06 11:16 AM, Alan Hayward wrote:
> @@ -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)
Here you use the magical "32" number but in aarch64_pseudo_register_name you used
AARCH64_V_REGS_NUM to refer (I think) to the same number. Would it be good to use
AARCH64_V_REGS_NUM everywhere? It might be good to extract that condition in a
function:
static bool
is_sve_regnum (int regnum)
{
return (regnum >= AARCH64_SVE_V0_REGNUM
&& regnum < AARCH64_SVE_V0_REGNUM + AARCH64_V_REGS_NUM);
}
and use it throughout.
> /* 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)
You could use the gdbarch from regcache.
> {
> - 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);
This is checking a static assertion. You could use static_assert instead, which
produces a compilation error if false. You can either leave it here or put it
next to the aarch64_regnum definition. I have seen the same gdb_assert somewhere
else too, with a static_assert you only need one.
Thanks,
Simon
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 01/10] Aarch64 SVE pseudo register support
2018-06-06 22:17 ` Simon Marchi
@ 2018-06-07 9:34 ` Alan Hayward
0 siblings, 0 replies; 56+ messages in thread
From: Alan Hayward @ 2018-06-07 9:34 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches, nd
Thanks for the review. Updated as below and pushed.
> On 6 Jun 2018, at 23:17, Simon Marchi <simon.marchi@ericsson.com> wrote:
>
> Hi Alan,
>
> I have some more nits/suggestions, feel free to cherry-pick the ones you
> want and push the result.
>
> On 2018-06-06 11:16 AM, Alan Hayward wrote:
>> @@ -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)
>
> Here you use the magical "32" number but in aarch64_pseudo_register_name you used
> AARCH64_V_REGS_NUM to refer (I think) to the same number. Would it be good to use
> AARCH64_V_REGS_NUM everywhere? It might be good to extract that condition in a
> function:
>
> static bool
> is_sve_regnum (int regnum)
> {
> return (regnum >= AARCH64_SVE_V0_REGNUM
> && regnum < AARCH64_SVE_V0_REGNUM + AARCH64_V_REGS_NUM);
> }
>
> and use it throughout.
Updated to use AARCH64_V_REGS_NUM.
I originally used 32 because I was matching the style of the code above which
uses 32. Agreed in this case makes more sense to use AARCH64_V_REGS_NUM.
Didn’t add the func, as here we’re checking against AARCH64_SVE_V0_REGNUM
and not AARCH64_V0_REGNUM.
>
>> /* 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)
>
> You could use the gdbarch from regcache.
Not changed.
Make sense, but regcache is already being passed into aarch64_pseudo_write (which
the function type is defined in gdbarch). Easier to just pass it straight down
given the compiler would have already have put gdbarch in a register.
>
>> {
>> - 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);
>
> This is checking a static assertion. You could use static_assert instead, which
> produces a compilation error if false. You can either leave it here or put it
> next to the aarch64_regnum definition. I have seen the same gdb_assert somewhere
> else too, with a static_assert you only need one.
>
Updated to gdb_static_assert. Left both of them in the code at the same place (there’s
one in the read func and one in the write func).
I wanted something explicit in this function because these are (I think) the only
two functions that rely on the defines being the same.
Thanks,
Alan.
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 07/10] Increase gdbsever PBUFSIZ
2018-06-06 15:16 [PATCH v2 00/10] gdb/gdbserver support for aarch64 SVE Alan Hayward
` (4 preceding siblings ...)
2018-06-06 15:17 ` [PATCH v2 01/10] Aarch64 SVE pseudo register support Alan Hayward
@ 2018-06-06 15:17 ` Alan Hayward
2018-06-11 0:46 ` Simon Marchi
2018-06-06 15:17 ` [PATCH v2 08/10] Enable Aarch64 SVE for gdbserver Alan Hayward
` (3 subsequent siblings)
9 siblings, 1 reply; 56+ messages in thread
From: Alan Hayward @ 2018-06-06 15:17 UTC (permalink / raw)
To: gdb-patches; +Cc: nd, Alan Hayward
PBUFSIZ is no longer big enough for SVE. Increase accordingly.
2018-06-06 Alan Hayward <alan.hayward@arm.com>
gdbserver/
* server.h (PBUFSIZ): Increase size
---
gdb/gdbserver/server.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index 9202df2948..8e197eef8f 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -113,7 +113,7 @@ extern int in_queued_stop_replies (ptid_t ptid);
/* Buffer sizes for transferring memory, registers, etc. Set to a constant
value to accomodate multiple register formats. This value must be at least
as large as the largest register set supported by gdbserver. */
-#define PBUFSIZ 16384
+#define PBUFSIZ 18432
/* Definition for an unknown syscall, used basically in error-cases. */
#define UNKNOWN_SYSCALL (-1)
--
2.15.1 (Apple Git-101)
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 07/10] Increase gdbsever PBUFSIZ
2018-06-06 15:17 ` [PATCH v2 07/10] Increase gdbsever PBUFSIZ Alan Hayward
@ 2018-06-11 0:46 ` Simon Marchi
0 siblings, 0 replies; 56+ messages in thread
From: Simon Marchi @ 2018-06-11 0:46 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
On 2018-06-06 11:16 AM, Alan Hayward wrote:
> PBUFSIZ is no longer big enough for SVE. Increase accordingly.
>
> 2018-06-06 Alan Hayward <alan.hayward@arm.com>
>
> gdbserver/
> * server.h (PBUFSIZ): Increase size
> ---
> gdb/gdbserver/server.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
> index 9202df2948..8e197eef8f 100644
> --- a/gdb/gdbserver/server.h
> +++ b/gdb/gdbserver/server.h
> @@ -113,7 +113,7 @@ extern int in_queued_stop_replies (ptid_t ptid);
> /* Buffer sizes for transferring memory, registers, etc. Set to a constant
> value to accomodate multiple register formats. This value must be at least
> as large as the largest register set supported by gdbserver. */
> -#define PBUFSIZ 16384
> +#define PBUFSIZ 18432
>
> /* Definition for an unknown syscall, used basically in error-cases. */
> #define UNKNOWN_SYSCALL (-1)
>
LGTM, I trust your did your maths correctly :).
Simon
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 08/10] Enable Aarch64 SVE for gdbserver
2018-06-06 15:16 [PATCH v2 00/10] gdb/gdbserver support for aarch64 SVE Alan Hayward
` (5 preceding siblings ...)
2018-06-06 15:17 ` [PATCH v2 07/10] Increase gdbsever PBUFSIZ Alan Hayward
@ 2018-06-06 15:17 ` Alan Hayward
2018-06-11 0:49 ` Simon Marchi
2018-06-06 15:17 ` [PATCH v2 10/10] Remove reg2 section from Aarch64 SVE cores Alan Hayward
` (2 subsequent siblings)
9 siblings, 1 reply; 56+ messages in thread
From: Alan Hayward @ 2018-06-06 15:17 UTC (permalink / raw)
To: gdb-patches; +Cc: nd, Alan Hayward
Simply add the check for SVE, similar to gdb.
2018-06-06 Alan Hayward <alan.hayward@arm.com>
gdbserver/
* linux-aarch64-ipa.c (get_ipa_tdesc): Add null VQ param.
(initialize_low_tracepoint): Likewise
* linux-aarch64-low.c (aarch64_arch_setup): Get VQ.
* linux-aarch64-tdesc-selftest.c (aarch64_tdesc_test): Add null VQ
param.
* linux-aarch64-tdesc.c (aarch64_linux_read_description): Add VQ
checks.
* linux-aarch64-tdesc.h (aarch64_linux_read_description): Add VQ.
---
gdb/gdbserver/linux-aarch64-ipa.c | 9 +++++----
gdb/gdbserver/linux-aarch64-low.c | 6 +++++-
gdb/gdbserver/linux-aarch64-tdesc-selftest.c | 2 +-
gdb/gdbserver/linux-aarch64-tdesc.c | 30 ++++++++++++++++++++--------
gdb/gdbserver/linux-aarch64-tdesc.h | 2 +-
5 files changed, 34 insertions(+), 15 deletions(-)
diff --git a/gdb/gdbserver/linux-aarch64-ipa.c b/gdb/gdbserver/linux-aarch64-ipa.c
index 3095408cf8..98a758f770 100644
--- a/gdb/gdbserver/linux-aarch64-ipa.c
+++ b/gdb/gdbserver/linux-aarch64-ipa.c
@@ -147,12 +147,12 @@ get_raw_reg (const unsigned char *raw_regs, int regnum)
/* Return target_desc to use for IPA, given the tdesc index passed by
gdbserver. Index is ignored, since we have only one tdesc
- at the moment. */
+ at the moment. SVE not yet supported. */
const struct target_desc *
get_ipa_tdesc (int idx)
{
- return aarch64_linux_read_description ();
+ return aarch64_linux_read_description (0);
}
/* Allocate buffer for the jump pads. The branch instruction has a reach
@@ -204,5 +204,6 @@ alloc_jump_pad_buffer (size_t size)
void
initialize_low_tracepoint (void)
{
- aarch64_linux_read_description ();
-}
+ /* SVE not yet supported. */
+ aarch64_linux_read_description (0);
+}
\ No newline at end of file
diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
index 7ea24c2363..9db9a7c1c3 100644
--- a/gdb/gdbserver/linux-aarch64-low.c
+++ b/gdb/gdbserver/linux-aarch64-low.c
@@ -40,6 +40,7 @@
#include "gdb_proc_service.h"
#include "arch/aarch64.h"
#include "linux-aarch64-tdesc.h"
+#include "nat/aarch64-sve-linux-ptrace.h"
#ifdef HAVE_SYS_REG_H
#include <sys/reg.h>
@@ -503,7 +504,10 @@ aarch64_arch_setup (void)
is_elf64 = linux_pid_exe_is_elf_64_file (tid, &machine);
if (is_elf64)
- current_process ()->tdesc = aarch64_linux_read_description ();
+ {
+ uint64_t vq = aarch64_sve_get_vq (tid);
+ current_process ()->tdesc = aarch64_linux_read_description (vq);
+ }
else
current_process ()->tdesc = tdesc_arm_with_neon;
diff --git a/gdb/gdbserver/linux-aarch64-tdesc-selftest.c b/gdb/gdbserver/linux-aarch64-tdesc-selftest.c
index 379951ac86..eef0b9c9a6 100644
--- a/gdb/gdbserver/linux-aarch64-tdesc-selftest.c
+++ b/gdb/gdbserver/linux-aarch64-tdesc-selftest.c
@@ -29,7 +29,7 @@ namespace tdesc {
static void
aarch64_tdesc_test ()
{
- const target_desc *tdesc = aarch64_linux_read_description ();
+ const target_desc *tdesc = aarch64_linux_read_description (0);
SELF_CHECK (*tdesc == *tdesc_aarch64);
}
}
diff --git a/gdb/gdbserver/linux-aarch64-tdesc.c b/gdb/gdbserver/linux-aarch64-tdesc.c
index f0761797e9..f538543175 100644
--- a/gdb/gdbserver/linux-aarch64-tdesc.c
+++ b/gdb/gdbserver/linux-aarch64-tdesc.c
@@ -21,23 +21,37 @@
#include "tdesc.h"
#include "arch/aarch64.h"
#include "linux-aarch32-low.h"
+#include <inttypes.h>
+
+/* All possible aarch64 target descriptors. */
+struct target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1];
/* Create the aarch64 target description. */
const target_desc *
-aarch64_linux_read_description ()
+aarch64_linux_read_description (uint64_t vq)
{
- static target_desc *aarch64_tdesc = NULL;
- target_desc **tdesc = &aarch64_tdesc;
+ if (vq > AARCH64_MAX_SVE_VQ)
+ error (_("VQ is %" PRIu64 ", maximum supported value is %d"), vq,
+ AARCH64_MAX_SVE_VQ);
+
+ struct target_desc *tdesc = tdesc_aarch64_list[vq];
- if (*tdesc == NULL)
+ if (tdesc == NULL)
{
- /* SVE not yet supported. */
- *tdesc = aarch64_create_target_description (0);
+ tdesc = aarch64_create_target_description (vq);
static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
- init_target_desc (*tdesc, expedite_regs_aarch64);
+ static const char *expedite_regs_aarch64_sve[] = { "x29", "sp", "pc",
+ "vg", NULL };
+
+ if (vq == 0)
+ init_target_desc (tdesc, expedite_regs_aarch64);
+ else
+ init_target_desc (tdesc, expedite_regs_aarch64_sve);
+
+ tdesc_aarch64_list[vq] = tdesc;
}
- return *tdesc;
+ return tdesc;
}
diff --git a/gdb/gdbserver/linux-aarch64-tdesc.h b/gdb/gdbserver/linux-aarch64-tdesc.h
index dc362998c2..4d2b883b55 100644
--- a/gdb/gdbserver/linux-aarch64-tdesc.h
+++ b/gdb/gdbserver/linux-aarch64-tdesc.h
@@ -17,7 +17,7 @@
You should have received a copy of the GNU General Public License
along with this program. If not, see <http://www.gnu.org/licenses/>. */
-const target_desc * aarch64_linux_read_description ();
+const target_desc * aarch64_linux_read_description (uint64_t vq);
#if GDB_SELF_TEST
void initialize_low_tdesc ();
--
2.15.1 (Apple Git-101)
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 08/10] Enable Aarch64 SVE for gdbserver
2018-06-06 15:17 ` [PATCH v2 08/10] Enable Aarch64 SVE for gdbserver Alan Hayward
@ 2018-06-11 0:49 ` Simon Marchi
0 siblings, 0 replies; 56+ messages in thread
From: Simon Marchi @ 2018-06-11 0:49 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
On 2018-06-06 11:16 AM, Alan Hayward wrote:
> Simply add the check for SVE, similar to gdb.
>
> 2018-06-06 Alan Hayward <alan.hayward@arm.com>
>
> gdbserver/
> * linux-aarch64-ipa.c (get_ipa_tdesc): Add null VQ param.
> (initialize_low_tracepoint): Likewise
> * linux-aarch64-low.c (aarch64_arch_setup): Get VQ.
> * linux-aarch64-tdesc-selftest.c (aarch64_tdesc_test): Add null VQ
> param.
> * linux-aarch64-tdesc.c (aarch64_linux_read_description): Add VQ
> checks.
> * linux-aarch64-tdesc.h (aarch64_linux_read_description): Add VQ.
LGTM.
Simon
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 10/10] Remove reg2 section from Aarch64 SVE cores
2018-06-06 15:16 [PATCH v2 00/10] gdb/gdbserver support for aarch64 SVE Alan Hayward
` (6 preceding siblings ...)
2018-06-06 15:17 ` [PATCH v2 08/10] Enable Aarch64 SVE for gdbserver Alan Hayward
@ 2018-06-06 15:17 ` Alan Hayward
2018-06-11 2:47 ` Simon Marchi
2018-06-06 15:17 ` [PATCH v2 05/10] Ptrace support for Aarch64 SVE Alan Hayward
2018-06-06 15:17 ` [PATCH v2 06/10] Add Aarch64 SVE dwarf regnums Alan Hayward
9 siblings, 1 reply; 56+ messages in thread
From: Alan Hayward @ 2018-06-06 15:17 UTC (permalink / raw)
To: gdb-patches; +Cc: nd, Alan Hayward
reg2 sections in SVE binaries will cause gdb to segfault on loading
due to miscalculating the register size.
For now, simply remove reg2 from SVE core files. This results in
core files without any vector/float register. Full core support
for SVE will come in a later set of patches.
2018-06-06 Alan Hayward <alan.hayward@arm.com>
gdb/
* aarch64-linux-tdep.c
(aarch64_linux_iterate_over_regset_sections): Check for SVE.
---
gdb/aarch64-linux-tdep.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index 96dc8a1132..b05bb49ae8 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -227,10 +227,13 @@ aarch64_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
void *cb_data,
const struct regcache *regcache)
{
+ struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
cb (".reg", AARCH64_LINUX_SIZEOF_GREGSET, &aarch64_linux_gregset,
NULL, cb_data);
- cb (".reg2", AARCH64_LINUX_SIZEOF_FPREGSET, &aarch64_linux_fpregset,
- NULL, cb_data);
+ if (!tdep->has_sve ())
+ cb (".reg2", AARCH64_LINUX_SIZEOF_FPREGSET, &aarch64_linux_fpregset,
+ NULL, cb_data);
}
/* Implement the "core_read_description" gdbarch method. SVE not yet
--
2.15.1 (Apple Git-101)
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 10/10] Remove reg2 section from Aarch64 SVE cores
2018-06-06 15:17 ` [PATCH v2 10/10] Remove reg2 section from Aarch64 SVE cores Alan Hayward
@ 2018-06-11 2:47 ` Simon Marchi
2018-06-11 16:37 ` Alan Hayward
0 siblings, 1 reply; 56+ messages in thread
From: Simon Marchi @ 2018-06-11 2:47 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
On 2018-06-06 11:16 AM, Alan Hayward wrote:
> reg2 sections in SVE binaries will cause gdb to segfault on loading
> due to miscalculating the register size.
>
> For now, simply remove reg2 from SVE core files. This results in
> core files without any vector/float register. Full core support
> for SVE will come in a later set of patches.
>
> 2018-06-06 Alan Hayward <alan.hayward@arm.com>
>
> gdb/
> * aarch64-linux-tdep.c
> (aarch64_linux_iterate_over_regset_sections): Check for SVE.
> ---
> gdb/aarch64-linux-tdep.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> index 96dc8a1132..b05bb49ae8 100644
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -227,10 +227,13 @@ aarch64_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
> void *cb_data,
> const struct regcache *regcache)
> {
> + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +
> cb (".reg", AARCH64_LINUX_SIZEOF_GREGSET, &aarch64_linux_gregset,
> NULL, cb_data);
> - cb (".reg2", AARCH64_LINUX_SIZEOF_FPREGSET, &aarch64_linux_fpregset,
> - NULL, cb_data);
> + if (!tdep->has_sve ())
> + cb (".reg2", AARCH64_LINUX_SIZEOF_FPREGSET, &aarch64_linux_fpregset,
> + NULL, cb_data);
> }
>
> /* Implement the "core_read_description" gdbarch method. SVE not yet
>
IIUC, this doesn't remove existing features? If a program was using the neon
registers, they will still be available?
If so, this LGTM.
Simon
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 10/10] Remove reg2 section from Aarch64 SVE cores
2018-06-11 2:47 ` Simon Marchi
@ 2018-06-11 16:37 ` Alan Hayward
0 siblings, 0 replies; 56+ messages in thread
From: Alan Hayward @ 2018-06-11 16:37 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches, nd
> On 11 Jun 2018, at 03:47, Simon Marchi <simark@simark.ca> wrote:
>
> On 2018-06-06 11:16 AM, Alan Hayward wrote:
>> reg2 sections in SVE binaries will cause gdb to segfault on loading
>> due to miscalculating the register size.
>>
>> For now, simply remove reg2 from SVE core files. This results in
>> core files without any vector/float register. Full core support
>> for SVE will come in a later set of patches.
>>
>> 2018-06-06 Alan Hayward <alan.hayward@arm.com>
>>
>> gdb/
>> * aarch64-linux-tdep.c
>> (aarch64_linux_iterate_over_regset_sections): Check for SVE.
>> ---
>> gdb/aarch64-linux-tdep.c | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
>> index 96dc8a1132..b05bb49ae8 100644
>> --- a/gdb/aarch64-linux-tdep.c
>> +++ b/gdb/aarch64-linux-tdep.c
>> @@ -227,10 +227,13 @@ aarch64_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
>> void *cb_data,
>> const struct regcache *regcache)
>> {
>> + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>> +
>> cb (".reg", AARCH64_LINUX_SIZEOF_GREGSET, &aarch64_linux_gregset,
>> NULL, cb_data);
>> - cb (".reg2", AARCH64_LINUX_SIZEOF_FPREGSET, &aarch64_linux_fpregset,
>> - NULL, cb_data);
>> + if (!tdep->has_sve ())
>> + cb (".reg2", AARCH64_LINUX_SIZEOF_FPREGSET, &aarch64_linux_fpregset,
>> + NULL, cb_data);
>> }
>>
>> /* Implement the "core_read_description" gdbarch method. SVE not yet
>>
>
> IIUC, this doesn't remove existing features? If a program was using the neon
> registers, they will still be available?
>
> If so, this LGTM.
>
With this patch, on SVE systems, gdb will create cores without any sve OR neon
registers. Whilst not ideal that’s better than gdb segfaulting.
...However, this got me thinking.
SVE core files produced by the kernel will have both FP and SVE sections, even
though the FP section is essentially redundant. So, gdb does need to support
that.
I'm working my way through fixing that. It’ll mostly be a change to regcache
to handle mismatched register sizes to core file slot sizes. I’ll drop this patch
for now, and will hopefully have a new patch posted Tuesday.
Alan.
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 05/10] Ptrace support for Aarch64 SVE
2018-06-06 15:16 [PATCH v2 00/10] gdb/gdbserver support for aarch64 SVE Alan Hayward
` (7 preceding siblings ...)
2018-06-06 15:17 ` [PATCH v2 10/10] Remove reg2 section from Aarch64 SVE cores Alan Hayward
@ 2018-06-06 15:17 ` Alan Hayward
2018-06-10 22:52 ` Simon Marchi
2018-06-06 15:17 ` [PATCH v2 06/10] Add Aarch64 SVE dwarf regnums Alan Hayward
9 siblings, 1 reply; 56+ messages in thread
From: Alan Hayward @ 2018-06-06 15:17 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.
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).
Added checks to make sure the vector length has not changed whilst
the process is running.
2018-06-06 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 | 54 +++++++-
gdb/gdbserver/Makefile.in | 1 +
gdb/nat/aarch64-sve-linux-ptrace.c | 271 ++++++++++++++++++++++++++++++++++++-
gdb/nat/aarch64-sve-linux-ptrace.h | 21 +++
4 files changed, 344 insertions(+), 3 deletions(-)
diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 1e4f937dc9..4db7d0d977 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -384,19 +384,62 @@ 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)
+{
+ std::unique_ptr<gdb_byte> base
+ = aarch64_sve_get_sveregs (ptid_get_lwp (regcache->ptid ()));
+ aarch64_sve_regs_copy_to_reg_buf (regcache, base.get ());
+}
+
+/* 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)
+{
+ int ret;
+ struct iovec iovec;
+ int tid = ptid_get_lwp (regcache->ptid ());
+
+ /* Obtain a dump of SVE registers from ptrace. */
+ std::unique_ptr<gdb_byte> base = aarch64_sve_get_sveregs (tid);
+
+ /* Overwrite with regcache state. */
+ aarch64_sve_regs_copy_from_reg_buf (regcache, base.get ());
+
+ /* Write back to the kernel. */
+ iovec.iov_base = base.get ();
+ iovec.iov_len = ((struct user_sve_header *) base.get ())->size;
+ ret = ptrace (PTRACE_SETREGSET, tid, NT_ARM_SVE, &iovec);
+
+ 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);
}
@@ -407,13 +450,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 675faa4364..f924e6a7f9 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -219,6 +219,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 119656b886..2ab055421a 100644
--- a/gdb/nat/aarch64-sve-linux-ptrace.c
+++ b/gdb/nat/aarch64-sve-linux-ptrace.c
@@ -24,6 +24,10 @@
#include "elf/common.h"
#include "aarch64-sve-linux-ptrace.h"
#include "arch/aarch64.h"
+#include "common-regcache.h"
+#include "common/byte-vector.h"
+
+static bool vq_change_warned = false;
/* See nat/aarch64-sve-linux-ptrace.h. */
@@ -46,7 +50,7 @@ aarch64_sve_get_vq (int tid)
return 0;
}
- long vq = sve_vq_from_vl (header.vl);
+ uint64_t vq = sve_vq_from_vl (header.vl);
if (!sve_vl_valid (header.vl))
{
@@ -56,3 +60,268 @@ aarch64_sve_get_vq (int tid)
return vq;
}
+
+/* See nat/aarch64-sve-linux-ptrace.h. */
+
+std::unique_ptr<gdb_byte>
+aarch64_sve_get_sveregs (int tid)
+{
+ struct iovec iovec;
+ struct user_sve_header header;
+ uint64_t 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);
+ std::unique_ptr<gdb_byte> buf (new gdb_byte[iovec.iov_len]);
+ iovec.iov_base = buf.get ();
+
+ if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec) < 0)
+ perror_with_name (_("Unable to fetch sve registers"));
+
+ return buf;
+}
+
+/* See nat/aarch64-sve-linux-ptrace.h. */
+
+void
+aarch64_sve_regs_copy_to_reg_buf (struct reg_buffer_common *reg_buf,
+ const void *buf)
+{
+ char *base = (char*) buf;
+ int i;
+ struct user_sve_header *header = (struct user_sve_header *) buf;
+ uint64_t vq, vg_reg_buf = 0;
+
+ vq = sve_vq_from_vl (header->vl);
+
+ /* Sanity check the data in the header. */
+ if (!sve_vl_valid (header->vl)
+ || SVE_PT_SIZE (vq, header->flags) != header->size)
+ error (_("Invalid SVE header from kernel."));
+
+ if (REG_VALID == reg_buf->get_register_status (AARCH64_SVE_VG_REGNUM))
+ reg_buf->raw_collect (AARCH64_SVE_VG_REGNUM, &vg_reg_buf);
+
+ if (vg_reg_buf == 0)
+ {
+ /* VG has not been set. */
+ vg_reg_buf = sve_vg_from_vl (header->vl);
+ reg_buf->raw_supply (AARCH64_SVE_VG_REGNUM, &vg_reg_buf);
+ }
+ else if (vg_reg_buf != 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 (_("SVE Vector length has changed (%ld to %d). "
+ "Vector registers may show incorrect data."),
+ vg_reg_buf, 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++)
+ reg_buf->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++)
+ reg_buf->raw_supply (AARCH64_SVE_P0_REGNUM + i,
+ base + SVE_PT_SVE_PREG_OFFSET (vq, i));
+
+ reg_buf->raw_supply (AARCH64_SVE_FFR_REGNUM,
+ base + SVE_PT_SVE_FFR_OFFSET (vq));
+ reg_buf->raw_supply (AARCH64_FPSR_REGNUM,
+ base + SVE_PT_SVE_FPSR_OFFSET (vq));
+ reg_buf->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));
+ reg_buf->raw_supply (AARCH64_SVE_Z0_REGNUM + i, zero_reg);
+ }
+
+ reg_buf->raw_supply (AARCH64_FPSR_REGNUM, &fpsimd->fpsr);
+ reg_buf->raw_supply (AARCH64_FPCR_REGNUM, &fpsimd->fpcr);
+
+ /* Clear the SVE only registers. */
+
+ for (i = 0; i <= AARCH64_SVE_P_REGS_NUM; i++)
+ reg_buf->raw_supply (AARCH64_SVE_P0_REGNUM + i, zero_reg);
+
+ reg_buf->raw_supply (AARCH64_SVE_FFR_REGNUM, zero_reg);
+ }
+}
+
+/* See nat/aarch64-sve-linux-ptrace.h. */
+
+void
+aarch64_sve_regs_copy_from_reg_buf (struct reg_buffer_common *reg_buf,
+ void *buf)
+{
+ struct user_sve_header *header = (struct user_sve_header *) buf;
+ char *base = (char*) buf;
+ uint64_t vq, vg_reg_buf = 0;
+ int i;
+
+ vq = sve_vq_from_vl (header->vl);
+
+ /* Sanity check the data in the header. */
+ if (!sve_vl_valid (header->vl)
+ || SVE_PT_SIZE (vq, header->flags) != header->size)
+ error (_("Invalid SVE header from kernel."));
+
+ if (REG_VALID == reg_buf->get_register_status (AARCH64_SVE_VG_REGNUM))
+ reg_buf->raw_collect (AARCH64_SVE_VG_REGNUM, &vg_reg_buf);
+
+ if (vg_reg_buf != 0 && vg_reg_buf != 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 (_("SVE Vector length has changed (%ld to %d). "
+ "Cannot write back registers."),
+ vg_reg_buf, sve_vg_from_vl (header->vl));
+ }
+
+ 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 reg_buf 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 reg_buf 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 |= reg_buf->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 |= reg_buf->raw_compare (AARCH64_SVE_P0_REGNUM + i,
+ zero_reg, 0);
+ if (has_sve_state != 0)
+ break;
+ }
+
+ if (has_sve_state == 0)
+ has_sve_state |= reg_buf->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
+ == reg_buf->get_register_status (AARCH64_SVE_Z0_REGNUM + i))
+ {
+ reg_buf->raw_collect (AARCH64_SVE_Z0_REGNUM + i, zero_reg);
+ memcpy (&fpsimd->vregs[i], zero_reg, sizeof (__int128_t));
+ }
+ }
+
+ if (REG_VALID == reg_buf->get_register_status (AARCH64_FPSR_REGNUM))
+ reg_buf->raw_collect (AARCH64_FPSR_REGNUM, &fpsimd->fpsr);
+ if (REG_VALID == reg_buf->get_register_status (AARCH64_FPCR_REGNUM))
+ reg_buf->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 reg_buf. */
+
+ for (i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++)
+ if (REG_VALID == reg_buf->get_register_status (AARCH64_SVE_Z0_REGNUM + i))
+ reg_buf->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 == reg_buf->get_register_status (AARCH64_SVE_P0_REGNUM + i))
+ reg_buf->raw_collect (AARCH64_SVE_P0_REGNUM + i,
+ base + SVE_PT_SVE_PREG_OFFSET (vq, i));
+
+ if (REG_VALID == reg_buf->get_register_status (AARCH64_SVE_FFR_REGNUM))
+ reg_buf->raw_collect (AARCH64_SVE_FFR_REGNUM,
+ base + SVE_PT_SVE_FFR_OFFSET (vq));
+ if (REG_VALID == reg_buf->get_register_status (AARCH64_FPSR_REGNUM))
+ reg_buf->raw_collect (AARCH64_FPSR_REGNUM,
+ base + SVE_PT_SVE_FPSR_OFFSET (vq));
+ if (REG_VALID == reg_buf->get_register_status (AARCH64_FPCR_REGNUM))
+ reg_buf->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 2d6f5714c0..1296fa42d4 100644
--- a/gdb/nat/aarch64-sve-linux-ptrace.h
+++ b/gdb/nat/aarch64-sve-linux-ptrace.h
@@ -33,9 +33,30 @@
#include "aarch64-linux-ptrace.h"
#endif
+/* 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)
+
/* 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 zero). */
uint64_t aarch64_sve_get_vq (int tid);
+/* Read the current SVE register set using ptrace, allocating space as
+ required. */
+
+extern std::unique_ptr<gdb_byte> aarch64_sve_get_sveregs (int tid);
+
+/* Put the registers from linux structure buf into register buffer. */
+
+extern void aarch64_sve_regs_copy_to_reg_buf (struct reg_buffer_common *reg_buf,
+ const void *buf);
+
+/* Put the registers from register buffer into linux structure buf. */
+
+extern void
+aarch64_sve_regs_copy_from_reg_buf (struct reg_buffer_common *reg_buf,
+ void *buf);
+
#endif /* aarch64-sve-linux-ptrace.h */
--
2.15.1 (Apple Git-101)
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 05/10] Ptrace support for Aarch64 SVE
2018-06-06 15:17 ` [PATCH v2 05/10] Ptrace support for Aarch64 SVE Alan Hayward
@ 2018-06-10 22:52 ` Simon Marchi
0 siblings, 0 replies; 56+ messages in thread
From: Simon Marchi @ 2018-06-10 22:52 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
Hi Alan,
Mostly some nits, though there is what looks like an off by one error
in some loops. You can push after you have looked into it.
On 2018-06-06 11:16 AM, Alan Hayward wrote:
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index 1e4f937dc9..4db7d0d977 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -384,19 +384,62 @@ 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)
> +{
> + std::unique_ptr<gdb_byte> base
> + = aarch64_sve_get_sveregs (ptid_get_lwp (regcache->ptid ()));
> + aarch64_sve_regs_copy_to_reg_buf (regcache, base.get ());
> +}
> +
> +/* 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)
> +{
> + int ret;
> + struct iovec iovec;
> + int tid = ptid_get_lwp (regcache->ptid ());
> +
> + /* Obtain a dump of SVE registers from ptrace. */
> + std::unique_ptr<gdb_byte> base = aarch64_sve_get_sveregs (tid);
I am not sure if it matters in this case because no destructor is run, but
I think it should be std::unique_ptr<gdb_byte[]>.
> @@ -56,3 +60,268 @@ aarch64_sve_get_vq (int tid)
>
> return vq;
> }
> +
> +/* See nat/aarch64-sve-linux-ptrace.h. */
> +
> +std::unique_ptr<gdb_byte>
> +aarch64_sve_get_sveregs (int tid)
> +{
> + struct iovec iovec;
> + struct user_sve_header header;
> + uint64_t 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);
> + std::unique_ptr<gdb_byte> buf (new gdb_byte[iovec.iov_len]);
> + iovec.iov_base = buf.get ();
> +
> + if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec) < 0)
> + perror_with_name (_("Unable to fetch sve registers"));
> +
> + return buf;
> +}
> +
> +/* See nat/aarch64-sve-linux-ptrace.h. */
> +
> +void
> +aarch64_sve_regs_copy_to_reg_buf (struct reg_buffer_common *reg_buf,
> + const void *buf)
The indentation of the second line has one extra space.
> +{
> + char *base = (char*) buf;
Space after "char".
> + int i;
> + struct user_sve_header *header = (struct user_sve_header *) buf;
> + uint64_t vq, vg_reg_buf = 0;
> +
> + vq = sve_vq_from_vl (header->vl);
> +
> + /* Sanity check the data in the header. */
> + if (!sve_vl_valid (header->vl)
> + || SVE_PT_SIZE (vq, header->flags) != header->size)
> + error (_("Invalid SVE header from kernel."));
> +
> + if (REG_VALID == reg_buf->get_register_status (AARCH64_SVE_VG_REGNUM))
> + reg_buf->raw_collect (AARCH64_SVE_VG_REGNUM, &vg_reg_buf);
> +
> + if (vg_reg_buf == 0)
> + {
> + /* VG has not been set. */
> + vg_reg_buf = sve_vg_from_vl (header->vl);
> + reg_buf->raw_supply (AARCH64_SVE_VG_REGNUM, &vg_reg_buf);
> + }
> + else if (vg_reg_buf != 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 (_("SVE Vector length has changed (%ld to %d). "
> + "Vector registers may show incorrect data."),
> + vg_reg_buf, 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++)
Declare the loop variables in the for (), when possible.
> + reg_buf->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++)
> + reg_buf->raw_supply (AARCH64_SVE_P0_REGNUM + i,
> + base + SVE_PT_SVE_PREG_OFFSET (vq, i));
> +
> + reg_buf->raw_supply (AARCH64_SVE_FFR_REGNUM,
> + base + SVE_PT_SVE_FFR_OFFSET (vq));
> + reg_buf->raw_supply (AARCH64_FPSR_REGNUM,
> + base + SVE_PT_SVE_FPSR_OFFSET (vq));
> + reg_buf->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));
> + reg_buf->raw_supply (AARCH64_SVE_Z0_REGNUM + i, zero_reg);
> + }
I'm getting this. Is the "i <= ..." on purpose?
CXX aarch64-sve-linux-ptrace.o
/home/simark/src/binutils-gdb/gdb/nat/aarch64-sve-linux-ptrace.c: In function 'void aarch64_sve_regs_copy_to_reg_buf(reg_buffer_common*, const void*)':
/home/simark/src/binutils-gdb/gdb/nat/aarch64-sve-linux-ptrace.c:168:61: error: iteration 32 invokes undefined behavior [-Werror=aggressive-loop-optimizations]
memcpy (zero_reg, &fpsimd->vregs[i], sizeof (__int128_t));
^
/home/simark/src/binutils-gdb/gdb/nat/aarch64-sve-linux-ptrace.c:166:21: note: within this loop
for (i = 0; i <= AARCH64_SVE_Z_REGS_NUM; i++)
> +
> + reg_buf->raw_supply (AARCH64_FPSR_REGNUM, &fpsimd->fpsr);
> + reg_buf->raw_supply (AARCH64_FPCR_REGNUM, &fpsimd->fpcr);
> +
> + /* Clear the SVE only registers. */
> +
> + for (i = 0; i <= AARCH64_SVE_P_REGS_NUM; i++)
Here too, "<="?
> + reg_buf->raw_supply (AARCH64_SVE_P0_REGNUM + i, zero_reg);
> +
> + reg_buf->raw_supply (AARCH64_SVE_FFR_REGNUM, zero_reg);
> + }
> +}
> +
> +/* See nat/aarch64-sve-linux-ptrace.h. */
> +
> +void
> +aarch64_sve_regs_copy_from_reg_buf (struct reg_buffer_common *reg_buf,
> + void *buf)
The indent is off by one here too.
reg_buf could be const.
> +{
> + struct user_sve_header *header = (struct user_sve_header *) buf;
> + char *base = (char*) buf;
Space after "char".
> + uint64_t vq, vg_reg_buf = 0;
> + int i;
> +
> + vq = sve_vq_from_vl (header->vl);
> +
> + /* Sanity check the data in the header. */
> + if (!sve_vl_valid (header->vl)
> + || SVE_PT_SIZE (vq, header->flags) != header->size)
> + error (_("Invalid SVE header from kernel."));
> +
> + if (REG_VALID == reg_buf->get_register_status (AARCH64_SVE_VG_REGNUM))
> + reg_buf->raw_collect (AARCH64_SVE_VG_REGNUM, &vg_reg_buf);
> +
> + if (vg_reg_buf != 0 && vg_reg_buf != 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 (_("SVE Vector length has changed (%ld to %d). "
> + "Cannot write back registers."),
> + vg_reg_buf, sve_vg_from_vl (header->vl));
> + }
> +
> + 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 reg_buf 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;
This should become a bool, and tests should become:
if (has_sve_state)
if (!has_sve_state)
Thanks,
Simon
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 06/10] Add Aarch64 SVE dwarf regnums
2018-06-06 15:16 [PATCH v2 00/10] gdb/gdbserver support for aarch64 SVE Alan Hayward
` (8 preceding siblings ...)
2018-06-06 15:17 ` [PATCH v2 05/10] Ptrace support for Aarch64 SVE Alan Hayward
@ 2018-06-06 15:17 ` Alan Hayward
2018-06-11 0:43 ` Simon Marchi
9 siblings, 1 reply; 56+ messages in thread
From: Alan Hayward @ 2018-06-06 15:17 UTC (permalink / raw)
To: gdb-patches; +Cc: nd, Alan Hayward
Simply add the Dwarf VG register checks.
This is as per the spec:
https://developer.arm.com/products/architecture/a-profile/docs/100985/0000
2018-06-06 Alan Hayward <alan.hayward@arm.com>
* aarch64-tdep.c (aarch64_dwarf_reg_to_regnum): Add mappings.
* aarch64-tdep.h (AARCH64_DWARF_SVE_VG): Add define.
(AARCH64_DWARF_SVE_FFR): Likewise.
(AARCH64_DWARF_SVE_P0): Likewise.
(AARCH64_DWARF_SVE_Z0): Likewise.
---
gdb/aarch64-tdep.c | 13 ++++++++++++-
gdb/aarch64-tdep.h | 4 ++++
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 0438ab83cd..cfe22213cd 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -1806,9 +1806,20 @@ aarch64_dwarf_reg_to_regnum (struct gdbarch *gdbarch, int reg)
if (reg >= AARCH64_DWARF_V0 && reg <= AARCH64_DWARF_V0 + 31)
return AARCH64_V0_REGNUM + reg - AARCH64_DWARF_V0;
+ if (reg == AARCH64_DWARF_SVE_VG)
+ return AARCH64_SVE_VG_REGNUM;
+
+ if (reg == AARCH64_DWARF_SVE_FFR)
+ return AARCH64_SVE_FFR_REGNUM;
+
+ if (reg >= AARCH64_DWARF_SVE_P0 && reg <= AARCH64_DWARF_SVE_P0 + 15)
+ return AARCH64_SVE_P0_REGNUM + reg - AARCH64_DWARF_SVE_P0;
+
+ if (reg >= AARCH64_DWARF_SVE_Z0 && reg <= AARCH64_DWARF_SVE_Z0 + 15)
+ return AARCH64_SVE_Z0_REGNUM + reg - AARCH64_DWARF_SVE_Z0;
+
return -1;
}
-\f
/* Implement the "print_insn" gdbarch method. */
diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
index 5a319551e6..7e5031f0fd 100644
--- a/gdb/aarch64-tdep.h
+++ b/gdb/aarch64-tdep.h
@@ -32,6 +32,10 @@ struct regset;
#define AARCH64_DWARF_X0 0
#define AARCH64_DWARF_SP 31
#define AARCH64_DWARF_V0 64
+#define AARCH64_DWARF_SVE_VG 46
+#define AARCH64_DWARF_SVE_FFR 47
+#define AARCH64_DWARF_SVE_P0 48
+#define AARCH64_DWARF_SVE_Z0 96
/* Size of integer registers. */
#define X_REGISTER_SIZE 8
--
2.15.1 (Apple Git-101)
^ permalink raw reply [flat|nested] 56+ messages in thread