From: Alan Hayward <Alan.Hayward@arm.com>
To: Simon Marchi <simon.marchi@ericsson.com>,
Pedro Alves <palves@redhat.com>
Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>,
nd <nd@arm.com>
Subject: Re: [PATCH 4/8] Enable SVE for GDB
Date: Mon, 04 Jun 2018 11:19:00 -0000 [thread overview]
Message-ID: <21329FE3-8072-44B4-A988-D2A7A15A0E1C@arm.com> (raw)
In-Reply-To: <3641fc23-e712-88f9-327e-bc795b3a1255@ericsson.com>
Push with minor changes as suggested below.
> On 31 May 2018, at 12:55, Simon Marchi <simon.marchi@ericsson.com> wrote:
>
> On 2018-05-11 06:52 AM, Alan Hayward wrote:
>> This patch enables SVE support for GDB by reading the VQ when
>> creating a target description.
>>
>> It also ensures that SVE is taken into account when creating
>> the tdep structure, and stores the current VQ value directly
>> in tdep.
>>
>> With this patch, gdb on an aarch64 system with SVE will now detect
>> SVE. The SVE registers will be displayed (but the contents will be
>> invalid). The following patches fill out the contents.
>
> LGTM, with some nits.
>
>> @@ -2911,25 +2933,45 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>> /* Validate the descriptor provides the mandatory core R registers
>> and allocate their numbers. */
>> for (i = 0; i < ARRAY_SIZE (aarch64_r_register_names); i++)
>> - valid_p &=
>> - tdesc_numbered_register (feature, tdesc_data, AARCH64_X0_REGNUM + i,
>> - aarch64_r_register_names[i]);
>> + valid_p &= tdesc_numbered_register (feature_core, tdesc_data,
>> + AARCH64_X0_REGNUM + i,
>> + aarch64_r_register_names[i]);
>>
>> num_regs = AARCH64_X0_REGNUM + i;
>>
>> - /* Look for the V registers. */
>> - feature = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.fpu");
>> - if (feature)
>> + /* Add the V registers. */
>> + if (feature_fpu != NULL)
>> {
>> + gdb_assert (feature_sve == NULL);
>
> Again, if this situation can result from a bad input passed to GDB (a bad tdesc
> sent by the remote), we shouldn't gdb_assert on it, but show an error message
> and gracefully fail.
Ok. Updated to use _error.
if (feature_sve != NULL)
error (_("Program contains both fpu and SVE features."));
>
>> +
>> /* Validate the descriptor provides the mandatory V registers
>> - and allocate their numbers. */
>> + and allocate their numbers. */
>> for (i = 0; i < ARRAY_SIZE (aarch64_v_register_names); i++)
>> - valid_p &=
>> - tdesc_numbered_register (feature, tdesc_data, AARCH64_V0_REGNUM + i,
>> - aarch64_v_register_names[i]);
>> + valid_p &= tdesc_numbered_register (feature_fpu, tdesc_data,
>> + AARCH64_V0_REGNUM + i,
>> + aarch64_v_register_names[i]);
>>
>> num_regs = AARCH64_V0_REGNUM + i;
>> + }
>> +
>> + /* Add the SVE registers. */
>> + if (feature_sve != NULL)
>> + {
>> + gdb_assert (feature_fpu == NULL);
>
> Same here.
I removed this one as it’s redundant - it’ll be caught by the error above.
>
>> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
>> index c9fd7b3578..046de6228f 100644
>> --- a/gdb/aarch64-tdep.h
>> +++ b/gdb/aarch64-tdep.h
>> @@ -73,6 +73,15 @@ struct gdbarch_tdep
>>
>> /* syscall record. */
>> int (*aarch64_syscall_record) (struct regcache *regcache, unsigned long svc_number);
>> +
>> + /* The VQ value for SVE targets, or zero if SVE is not supported. */
>> + long vq;
>> +
>> + /* Returns true if the target supports SVE. */
>> + bool has_sve ()
>> + {
>> + return vq != 0;
>> + }
>
> This method can be const.
>
Done.
> On 31 May 2018, at 15:59, Pedro Alves <palves@redhat.com> wrote:
>
> On 05/11/2018 11:52 AM, Alan Hayward wrote:
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -2873,6 +2873,26 @@ aarch64_read_description (long vq)
>> return tdesc;
>> }
>>
>> +/* Return the VQ used when creating the target description TDESC. */
>> +
>> +static long
>> +aarch64_get_tdesc_vq (const struct target_desc *tdesc)
>
> Is this use of "long" significant? I mean, is it assuming 64-bit?
> I ask because longs are not 64-bit on x64 Windows, so it would
> do the wrong thing when cross debugging.
>
Fixed by using both follow on patch "[PATCH] Use uint64_t for SVE VQ” and
obvious patch below:
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 6674b7654e..0172e4ccd1 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -2875,7 +2875,7 @@ aarch64_read_description (uint64_t vq)
/* Return the VQ used when creating the target description TDESC. */
-static long
+static uint64_t
aarch64_get_tdesc_vq (const struct target_desc *tdesc)
{
const struct tdesc_feature *feature_sve;
@@ -2888,7 +2888,8 @@ aarch64_get_tdesc_vq (const struct target_desc *tdesc)
if (feature_sve == nullptr)
return 0;
- long vl = tdesc_register_size (feature_sve, aarch64_sve_register_names[0]);
+ uint64_t vl = tdesc_register_size (feature_sve,
+ aarch64_sve_register_names[0]);
return sve_vq_from_vl (vl);
}
diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
index b6b9b30e71..598a0aafa2 100644
--- a/gdb/aarch64-tdep.h
+++ b/gdb/aarch64-tdep.h
@@ -75,7 +75,7 @@ struct gdbarch_tdep
int (*aarch64_syscall_record) (struct regcache *regcache, unsigned long svc_number);
/* The VQ value for SVE targets, or zero if SVE is not supported. */
- long vq;
+ uint64_t vq;
/* Returns true if the target supports SVE. */
bool has_sve () const
Thanks!
Alan.
next prev parent reply other threads:[~2018-06-04 11:19 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-11 10:53 [PATCH 0/8] Add SVE support for Aarch64 GDB Alan Hayward
2018-05-11 10:53 ` [PATCH 2/8] Function for reading the Aarch64 SVE vector length Alan Hayward
2018-05-31 12:06 ` Simon Marchi
2018-05-31 14:18 ` Alan Hayward
2018-05-31 14:57 ` Pedro Alves
2018-06-05 20:01 ` Sergio Durigan Junior
2018-06-05 22:06 ` [PATCH] Guard declarations of 'sve_*_from_*' macros on Aarch64 (and unbreak build) Sergio Durigan Junior
2018-06-05 23:37 ` Sergio Durigan Junior
2018-06-06 7:34 ` Alan Hayward
2018-06-06 21:19 ` Simon Marchi
2018-06-06 21:36 ` Sergio Durigan Junior
2018-05-11 10:53 ` [PATCH 4/8] Enable SVE for GDB Alan Hayward
2018-05-31 12:22 ` Simon Marchi
2018-06-04 11:19 ` Alan Hayward [this message]
2018-05-31 14:58 ` Pedro Alves
2018-05-31 16:13 ` Pedro Alves
2018-05-31 16:20 ` Alan Hayward
2018-05-31 16:27 ` Pedro Alves
2018-05-31 18:06 ` Alan Hayward
2018-05-11 10:53 ` [PATCH 7/8] Add methods to gdbserver regcache and raw_compare Alan Hayward
2018-05-31 14:57 ` Pedro Alves
2018-05-11 10:53 ` [PATCH 8/8] Ptrace support for Aarch64 SVE Alan Hayward
2018-05-31 13:40 ` Simon Marchi
2018-05-31 14:56 ` Alan Hayward
2018-06-01 15:17 ` Simon Marchi
2018-06-04 15:49 ` Alan Hayward
2018-05-31 20:17 ` Simon Marchi
2018-05-11 10:53 ` [PATCH 6/8] Aarch64 SVE pseudo register support Alan Hayward
2018-05-31 13:26 ` Simon Marchi
2018-06-04 13:29 ` Alan Hayward
2018-05-31 14:59 ` Pedro Alves
2018-05-11 10:53 ` [PATCH 1/8] Add Aarch64 SVE target description Alan Hayward
2018-05-11 14:56 ` Eli Zaretskii
2018-05-11 16:46 ` Alan Hayward
2018-05-31 11:56 ` Simon Marchi
2018-05-31 14:12 ` Alan Hayward
2018-05-11 11:52 ` [PATCH 5/8] Add aarch64 psuedo help functions Alan Hayward
2018-05-31 13:22 ` Simon Marchi
2018-05-31 15:20 ` Pedro Alves
2018-06-04 13:13 ` Alan Hayward
2018-05-11 12:12 ` [PATCH 3/8] Add SVE register defines Alan Hayward
2018-06-01 8:33 ` Alan Hayward
2018-06-01 15:18 ` Simon Marchi
2018-05-22 11:00 ` [PATCH 0/8] Add SVE support for Aarch64 GDB Alan Hayward
2018-05-29 12:09 ` [PING 2][PATCH " Alan Hayward
2018-05-29 14:35 ` Omair Javaid
2018-05-29 14:59 ` Alan Hayward
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=21329FE3-8072-44B4-A988-D2A7A15A0E1C@arm.com \
--to=alan.hayward@arm.com \
--cc=gdb-patches@sourceware.org \
--cc=nd@arm.com \
--cc=palves@redhat.com \
--cc=simon.marchi@ericsson.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).