* [PATCH 2/3] Allocate data in cached_reg_t
@ 2017-01-09 10:57 Alan Hayward
2017-01-09 20:11 ` Luis Machado
2017-01-18 9:03 ` Yao Qi
0 siblings, 2 replies; 6+ messages in thread
From: Alan Hayward @ 2017-01-09 10:57 UTC (permalink / raw)
To: gdb-patches; +Cc: nd
[-- Attachment #1: Type: text/plain, Size: 2734 bytes --]
Aarch64 SVE requires a max register size of 256. The current max size in gdb
is 64. This is part of a series demonstrating the replacement of
MAX_REGISTER_SIZE.
In cached_reg_t the data is changed to a pointer, which is allocated using the
size of the register being cached. This pointer must be manually freed when
deleting a DEF_VEC of cached_reg_t's.
Tested on x86.
Ok to commit?
Thanks,
Alan.
2017-01-09 Alan Hayward <alan.hayward@arm.com>
* remote.c (struct cached_reg): Change data into a pointer.
* (stop_reply_dtr): Free data pointers before deleting vector.
(process_stop_reply): Likewise.
(remote_parse_stop_reply): Allocate space for data
diff --git a/gdb/remote.c b/gdb/remote.c
index 6da6eb366ae442354fd6a37741335af9a4a5a056..9247d43b094925ff397eb36b450eaba521adfc99 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6306,7 +6306,7 @@ remote_console_output (char *msg)
typedef struct cached_reg
{
int num;
- gdb_byte data[MAX_REGISTER_SIZE];
+ gdb_byte *data;
} cached_reg_t;
DEF_VEC_O(cached_reg_t);
@@ -6402,6 +6402,13 @@ static void
stop_reply_dtr (struct notif_event *event)
{
struct stop_reply *r = (struct stop_reply *) event;
+ cached_reg_t *reg;
+ int ix;
+
+ for (ix = 0;
+ VEC_iterate(cached_reg_t, r->regcache, ix, reg);
+ ix++)
+ xfree (reg->data);
VEC_free (cached_reg_t, r->regcache);
}
@@ -6974,6 +6981,7 @@ Packet: '%s'\n"),
{
struct packet_reg *reg = packet_reg_from_pnum (rsa, pnum);
cached_reg_t cached_reg;
+ struct gdbarch *gdbarch = target_gdbarch ();
if (reg == NULL)
error (_("Remote sent bad register number %s: %s\n\
@@ -6981,14 +6989,14 @@ Packet: '%s'\n"),
hex_string (pnum), p, buf);
cached_reg.num = reg->regnum;
+ cached_reg.data = (gdb_byte *)
+ xmalloc (register_size (gdbarch, reg->regnum));
p = p1 + 1;
fieldsize = hex2bin (p, cached_reg.data,
- register_size (target_gdbarch (),
- reg->regnum));
+ register_size (gdbarch, reg->regnum));
p += 2 * fieldsize;
- if (fieldsize < register_size (target_gdbarch (),
- reg->regnum))
+ if (fieldsize < register_size (gdbarch, reg->regnum))
warning (_("Remote reply is too short: %s"), buf);
VEC_safe_push (cached_reg_t, event->regcache, &cached_reg);
@@ -7211,7 +7219,11 @@ process_stop_reply (struct stop_reply *stop_reply,
for (ix = 0;
VEC_iterate(cached_reg_t, stop_reply->regcache, ix, reg);
ix++)
+ {
regcache_raw_supply (regcache, reg->num, reg->data);
+ xfree (reg->data);
+ }
+
VEC_free (cached_reg_t, stop_reply->regcache);
}
[-- Attachment #2: 23maxreg.patch --]
[-- Type: application/octet-stream, Size: 1984 bytes --]
diff --git a/gdb/remote.c b/gdb/remote.c
index 6da6eb366ae442354fd6a37741335af9a4a5a056..9247d43b094925ff397eb36b450eaba521adfc99 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6306,7 +6306,7 @@ remote_console_output (char *msg)
typedef struct cached_reg
{
int num;
- gdb_byte data[MAX_REGISTER_SIZE];
+ gdb_byte *data;
} cached_reg_t;
DEF_VEC_O(cached_reg_t);
@@ -6402,6 +6402,13 @@ static void
stop_reply_dtr (struct notif_event *event)
{
struct stop_reply *r = (struct stop_reply *) event;
+ cached_reg_t *reg;
+ int ix;
+
+ for (ix = 0;
+ VEC_iterate(cached_reg_t, r->regcache, ix, reg);
+ ix++)
+ xfree (reg->data);
VEC_free (cached_reg_t, r->regcache);
}
@@ -6974,6 +6981,7 @@ Packet: '%s'\n"),
{
struct packet_reg *reg = packet_reg_from_pnum (rsa, pnum);
cached_reg_t cached_reg;
+ struct gdbarch *gdbarch = target_gdbarch ();
if (reg == NULL)
error (_("Remote sent bad register number %s: %s\n\
@@ -6981,14 +6989,14 @@ Packet: '%s'\n"),
hex_string (pnum), p, buf);
cached_reg.num = reg->regnum;
+ cached_reg.data = (gdb_byte *)
+ xmalloc (register_size (gdbarch, reg->regnum));
p = p1 + 1;
fieldsize = hex2bin (p, cached_reg.data,
- register_size (target_gdbarch (),
- reg->regnum));
+ register_size (gdbarch, reg->regnum));
p += 2 * fieldsize;
- if (fieldsize < register_size (target_gdbarch (),
- reg->regnum))
+ if (fieldsize < register_size (gdbarch, reg->regnum))
warning (_("Remote reply is too short: %s"), buf);
VEC_safe_push (cached_reg_t, event->regcache, &cached_reg);
@@ -7211,7 +7219,11 @@ process_stop_reply (struct stop_reply *stop_reply,
for (ix = 0;
VEC_iterate(cached_reg_t, stop_reply->regcache, ix, reg);
ix++)
+ {
regcache_raw_supply (regcache, reg->num, reg->data);
+ xfree (reg->data);
+ }
+
VEC_free (cached_reg_t, stop_reply->regcache);
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] Allocate data in cached_reg_t
2017-01-09 10:57 [PATCH 2/3] Allocate data in cached_reg_t Alan Hayward
@ 2017-01-09 20:11 ` Luis Machado
2017-01-10 12:59 ` Yao Qi
2017-01-18 9:03 ` Yao Qi
1 sibling, 1 reply; 6+ messages in thread
From: Luis Machado @ 2017-01-09 20:11 UTC (permalink / raw)
To: Alan Hayward, gdb-patches; +Cc: nd
On 01/09/2017 04:56 AM, Alan Hayward wrote:
> Aarch64 SVE requires a max register size of 256. The current max size in gdb
> is 64. This is part of a series demonstrating the replacement of
> MAX_REGISTER_SIZE.
>
> In cached_reg_t the data is changed to a pointer, which is allocated using the
> size of the register being cached. This pointer must be manually freed when
> deleting a DEF_VEC of cached_reg_t's.
>
> Tested on x86.
> Ok to commit?
>
> Thanks,
> Alan.
>
> 2017-01-09 Alan Hayward <alan.hayward@arm.com>
>
> * remote.c (struct cached_reg): Change data into a pointer.
> * (stop_reply_dtr): Free data pointers before deleting vector.
> (process_stop_reply): Likewise.
> (remote_parse_stop_reply): Allocate space for data
>
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 6da6eb366ae442354fd6a37741335af9a4a5a056..9247d43b094925ff397eb36b450eaba521adfc99 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -6306,7 +6306,7 @@ remote_console_output (char *msg)
> typedef struct cached_reg
> {
> int num;
> - gdb_byte data[MAX_REGISTER_SIZE];
> + gdb_byte *data;
Would it make sense to go C++ and use a data structure that can take
care of variable sizes? Just thinking if that would be easier than
handling allocation/deallocation of the data.
> } cached_reg_t;
>
> DEF_VEC_O(cached_reg_t);
> @@ -6402,6 +6402,13 @@ static void
> stop_reply_dtr (struct notif_event *event)
> {
> struct stop_reply *r = (struct stop_reply *) event;
> + cached_reg_t *reg;
> + int ix;
> +
> + for (ix = 0;
> + VEC_iterate(cached_reg_t, r->regcache, ix, reg);
> + ix++)
> + xfree (reg->data);
>
> VEC_free (cached_reg_t, r->regcache);
> }
> @@ -6974,6 +6981,7 @@ Packet: '%s'\n"),
> {
> struct packet_reg *reg = packet_reg_from_pnum (rsa, pnum);
> cached_reg_t cached_reg;
> + struct gdbarch *gdbarch = target_gdbarch ();
>
> if (reg == NULL)
> error (_("Remote sent bad register number %s: %s\n\
> @@ -6981,14 +6989,14 @@ Packet: '%s'\n"),
> hex_string (pnum), p, buf);
>
> cached_reg.num = reg->regnum;
> + cached_reg.data = (gdb_byte *)
> + xmalloc (register_size (gdbarch, reg->regnum));
>
> p = p1 + 1;
> fieldsize = hex2bin (p, cached_reg.data,
> - register_size (target_gdbarch (),
> - reg->regnum));
> + register_size (gdbarch, reg->regnum));
> p += 2 * fieldsize;
> - if (fieldsize < register_size (target_gdbarch (),
> - reg->regnum))
> + if (fieldsize < register_size (gdbarch, reg->regnum))
> warning (_("Remote reply is too short: %s"), buf);
>
> VEC_safe_push (cached_reg_t, event->regcache, &cached_reg);
> @@ -7211,7 +7219,11 @@ process_stop_reply (struct stop_reply *stop_reply,
> for (ix = 0;
> VEC_iterate(cached_reg_t, stop_reply->regcache, ix, reg);
> ix++)
> + {
> regcache_raw_supply (regcache, reg->num, reg->data);
> + xfree (reg->data);
> + }
> +
> VEC_free (cached_reg_t, stop_reply->regcache);
> }
>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] Allocate data in cached_reg_t
2017-01-09 20:11 ` Luis Machado
@ 2017-01-10 12:59 ` Yao Qi
2017-01-10 17:42 ` Luis Machado
0 siblings, 1 reply; 6+ messages in thread
From: Yao Qi @ 2017-01-10 12:59 UTC (permalink / raw)
To: Luis Machado; +Cc: Alan Hayward, gdb-patches, nd
On 17-01-09 14:11:13, Luis Machado wrote:
> >@@ -6306,7 +6306,7 @@ remote_console_output (char *msg)
> > typedef struct cached_reg
> > {
> > int num;
> >- gdb_byte data[MAX_REGISTER_SIZE];
> >+ gdb_byte *data;
>
> Would it make sense to go C++ and use a data structure that can take
> care of variable sizes? Just thinking if that would be easier than
> handling allocation/deallocation of the data.
>
Do you suggest std::vector? We still need to allocate/deallocate it
if we change "gdb_byte *data" to "std::vector<gdb_byte> *data", or we
have to convert cached_reg to class, and do RAII.
class cached_reg
{
public:
cached_reg (struct gdbarch *gdbarch, int num_)
: num (num_)
{
this->data = (gdb_byte *) xmalloc (register_size (gdbarch, num_));
}
~cached_reg ()
{
xfree (this->data);
}
private:
int num;
gdb_byte *data;
};
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] Allocate data in cached_reg_t
2017-01-10 12:59 ` Yao Qi
@ 2017-01-10 17:42 ` Luis Machado
2017-01-11 10:54 ` Alan Hayward
0 siblings, 1 reply; 6+ messages in thread
From: Luis Machado @ 2017-01-10 17:42 UTC (permalink / raw)
To: Yao Qi; +Cc: Alan Hayward, gdb-patches, nd
On 01/10/2017 06:59 AM, Yao Qi wrote:
> On 17-01-09 14:11:13, Luis Machado wrote:
>>> @@ -6306,7 +6306,7 @@ remote_console_output (char *msg)
>>> typedef struct cached_reg
>>> {
>>> int num;
>>> - gdb_byte data[MAX_REGISTER_SIZE];
>>> + gdb_byte *data;
>>
>> Would it make sense to go C++ and use a data structure that can take
>> care of variable sizes? Just thinking if that would be easier than
>> handling allocation/deallocation of the data.
>>
>
> Do you suggest std::vector? We still need to allocate/deallocate it
> if we change "gdb_byte *data" to "std::vector<gdb_byte> *data", or we
> have to convert cached_reg to class, and do RAII.
>
Something like std::vector, yes. But it is true we will still need to
allocate the vector itself.
I was pondering about the benefits of not being limited to a specific
register size. Then we wouldn't need to worry about adjusting things as
we have to do now.
As i see it, data[register_size(register_size (gdbarch, num)] wouldn't
be much different than data[MAX_REGISTER_SIZE]. The only thing is that
we're setting the max register size dynamically.
We may or may not need this flexibility right now, but who knows what
weird architectures we may have in the future.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] Allocate data in cached_reg_t
2017-01-10 17:42 ` Luis Machado
@ 2017-01-11 10:54 ` Alan Hayward
0 siblings, 0 replies; 6+ messages in thread
From: Alan Hayward @ 2017-01-11 10:54 UTC (permalink / raw)
To: Luis Machado; +Cc: Yao Qi, gdb-patches, nd
> On 10 Jan 2017, at 17:42, Luis Machado <lgustavo@codesourcery.com> wrote:
>
> On 01/10/2017 06:59 AM, Yao Qi wrote:
>> On 17-01-09 14:11:13, Luis Machado wrote:
>>>> @@ -6306,7 +6306,7 @@ remote_console_output (char *msg)
>>>> typedef struct cached_reg
>>>> {
>>>> int num;
>>>> - gdb_byte data[MAX_REGISTER_SIZE];
>>>> + gdb_byte *data;
>>>
>>> Would it make sense to go C++ and use a data structure that can take
>>> care of variable sizes? Just thinking if that would be easier than
>>> handling allocation/deallocation of the data.
>>>
>>
>> Do you suggest std::vector? We still need to allocate/deallocate it
>> if we change "gdb_byte *data" to "std::vector<gdb_byte> *data", or we
>> have to convert cached_reg to class, and do RAII.
>>
>
> Something like std::vector, yes. But it is true we will still need to allocate the vector itself.
>
> I was pondering about the benefits of not being limited to a specific register size. Then we wouldn't need to worry about adjusting things as we have to do now.
>
> As i see it, data[register_size(register_size (gdbarch, num)] wouldn't be much different than data[MAX_REGISTER_SIZE]. The only thing is that we're setting the max register size dynamically.
>
> We may or may not need this flexibility right now, but who knows what weird architectures we may have in the future.
I avoided using data[register_size(gdbarch, num)] as dynamically sized arrays are a gcc extension - it’s not something we can expect to work on other compilers.
Plus there is a danger of it using up all the stack. Aarch64 SVE will have a max register size of 256, potentially this could grow further in the future.
This is why I used alloca in patches 1/3 and 2/3.
I know that gcc avoid using std::vector because it’s fairly slow. I think gcc also have newer c++ like replacements for VEC, but am not sure what they have that’s suitable. However, I’d suggest that’s something that should be done as part of the work to move gdb to c++, and not this patch.
Alan.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] Allocate data in cached_reg_t
2017-01-09 10:57 [PATCH 2/3] Allocate data in cached_reg_t Alan Hayward
2017-01-09 20:11 ` Luis Machado
@ 2017-01-18 9:03 ` Yao Qi
1 sibling, 0 replies; 6+ messages in thread
From: Yao Qi @ 2017-01-18 9:03 UTC (permalink / raw)
To: Alan Hayward; +Cc: gdb-patches
On 17-01-09 10:56:56, Alan Hayward wrote:
> @@ -6402,6 +6402,13 @@ static void
> stop_reply_dtr (struct notif_event *event)
> {
> struct stop_reply *r = (struct stop_reply *) event;
> + cached_reg_t *reg;
> + int ix;
> +
> + for (ix = 0;
> + VEC_iterate(cached_reg_t, r->regcache, ix, reg);
A space is needed before "(".
Patch is good to me.
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-01-18 9:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-09 10:57 [PATCH 2/3] Allocate data in cached_reg_t Alan Hayward
2017-01-09 20:11 ` Luis Machado
2017-01-10 12:59 ` Yao Qi
2017-01-10 17:42 ` Luis Machado
2017-01-11 10:54 ` Alan Hayward
2017-01-18 9:03 ` Yao Qi
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).