* [PATCH] Remove an instance of MAX_REGISTER_SIZE from record-full.c @ 2017-06-12 9:34 Alan Hayward 2017-06-12 11:05 ` Yao Qi 0 siblings, 1 reply; 8+ messages in thread From: Alan Hayward @ 2017-06-12 9:34 UTC (permalink / raw) To: gdb-patches; +Cc: nd In record_full_exec_insn use cooked_read_value instead of creating a temp buffer. Code to remove MAX_REGISTER_SIZE from uses of record_full_core_regbuf will be in a different patch (I'm debugging some issues with it). Tested on a --enable-targets=all build with board files unix and native-gdbserver. Ok to commit? Alan. 2017-06-12 Alan Hayward <alan.hayward@arm.com> * gdb/record-full.c (record_full_exec_insn): Use regcache->cooked_read_value. diff --git a/gdb/record-full.c b/gdb/record-full.c index 31ff558d2a633cff71d3e6082e42f5d6fb88bcf1..2f09e2fc316d7d3073de035f961f7c3bf9d3002c 100644 --- a/gdb/record-full.c +++ b/gdb/record-full.c @@ -698,7 +698,7 @@ record_full_exec_insn (struct regcache *regcache, { case record_full_reg: /* reg */ { - gdb_byte reg[MAX_REGISTER_SIZE]; + struct value *value; if (record_debug > 1) fprintf_unfiltered (gdb_stdlog, @@ -707,10 +707,13 @@ record_full_exec_insn (struct regcache *regcache, host_address_to_string (entry), entry->u.reg.num); - regcache_cooked_read (regcache, entry->u.reg.num, reg); - regcache_cooked_write (regcache, entry->u.reg.num, - record_full_get_loc (entry)); - memcpy (record_full_get_loc (entry), reg, entry->u.reg.len); + value = regcache->cooked_read_value (entry->u.reg.num); + gdb_assert (value != NULL); + regcache->cooked_write (entry->u.reg.num, record_full_get_loc (entry)); + memcpy (record_full_get_loc (entry), value_contents_all (value), + entry->u.reg.len); + release_value (value); + value_free (value); } break; ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Remove an instance of MAX_REGISTER_SIZE from record-full.c 2017-06-12 9:34 [PATCH] Remove an instance of MAX_REGISTER_SIZE from record-full.c Alan Hayward @ 2017-06-12 11:05 ` Yao Qi 2017-06-12 13:59 ` Alan Hayward 0 siblings, 1 reply; 8+ messages in thread From: Yao Qi @ 2017-06-12 11:05 UTC (permalink / raw) To: Alan Hayward; +Cc: gdb-patches, nd Alan Hayward <Alan.Hayward@arm.com> writes: > - regcache_cooked_read (regcache, entry->u.reg.num, reg); > - regcache_cooked_write (regcache, entry->u.reg.num, > - record_full_get_loc (entry)); > - memcpy (record_full_get_loc (entry), reg, entry->u.reg.len); The original code is about swapping contents of register REG in regcache and record_full_get_loc (entry), and the length is known entry->u.reg.len. > + value = regcache->cooked_read_value (entry->u.reg.num); > + gdb_assert (value != NULL); > + regcache->cooked_write (entry->u.reg.num, record_full_get_loc (entry)); > + memcpy (record_full_get_loc (entry), value_contents_all (value), > + entry->u.reg.len); > + release_value (value); > + value_free (value); It is a overkill to use value to swap these two buffers, IMO. How about xmalloc "reg" instead? -- Yao (齐尧) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Remove an instance of MAX_REGISTER_SIZE from record-full.c 2017-06-12 11:05 ` Yao Qi @ 2017-06-12 13:59 ` Alan Hayward 2017-06-21 9:31 ` Alan Hayward ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Alan Hayward @ 2017-06-12 13:59 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches, nd > On 12 Jun 2017, at 12:05, Yao Qi <qiyaoltc@gmail.com> wrote: > > Alan Hayward <Alan.Hayward@arm.com> writes: > >> - regcache_cooked_read (regcache, entry->u.reg.num, reg); >> - regcache_cooked_write (regcache, entry->u.reg.num, >> - record_full_get_loc (entry)); >> - memcpy (record_full_get_loc (entry), reg, entry->u.reg.len); > > The original code is about swapping contents of register REG in regcache > and record_full_get_loc (entry), and the length is known entry->u.reg.len. > Yes. >> + value = regcache->cooked_read_value (entry->u.reg.num); >> + gdb_assert (value != NULL); >> + regcache->cooked_write (entry->u.reg.num, record_full_get_loc (entry)); >> + memcpy (record_full_get_loc (entry), value_contents_all (value), >> + entry->u.reg.len); >> + release_value (value); >> + value_free (value); > > It is a overkill to use value to swap these two buffers, IMO. How about > xmalloc "reg" instead? > Given that the code doesn’t use any of the error checking, then agreed. Tested on a --enable-targets=all build with board files unix and native-gdbserver. Ok to commit? Alan. 2017-06-12 Alan Hayward <alan.hayward@arm.com> * gdb/record-full.c (record_full_exec_insn): Allocate buffer. diff --git a/gdb/record-full.c b/gdb/record-full.c index 31ff558d2a633cff71d3e6082e42f5d6fb88bcf1..4f73e2a5ad0d4a2407b31a1d391e813147e15798 100644 --- a/gdb/record-full.c +++ b/gdb/record-full.c @@ -698,7 +698,7 @@ record_full_exec_insn (struct regcache *regcache, { case record_full_reg: /* reg */ { - gdb_byte reg[MAX_REGISTER_SIZE]; + gdb_byte *reg = (gdb_byte *) xmalloc (entry->u.reg.len); if (record_debug > 1) fprintf_unfiltered (gdb_stdlog, @@ -711,6 +711,7 @@ record_full_exec_insn (struct regcache *regcache, regcache_cooked_write (regcache, entry->u.reg.num, record_full_get_loc (entry)); memcpy (record_full_get_loc (entry), reg, entry->u.reg.len); + xfree (reg); } break; ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Remove an instance of MAX_REGISTER_SIZE from record-full.c 2017-06-12 13:59 ` Alan Hayward @ 2017-06-21 9:31 ` Alan Hayward 2017-06-21 10:33 ` Yao Qi 2017-06-21 10:52 ` Pedro Alves 2 siblings, 0 replies; 8+ messages in thread From: Alan Hayward @ 2017-06-21 9:31 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches, nd PING > On 12 Jun 2017, at 14:59, Alan Hayward <Alan.Hayward@arm.com> wrote: > > >> On 12 Jun 2017, at 12:05, Yao Qi <qiyaoltc@gmail.com> wrote: >> >> Alan Hayward <Alan.Hayward@arm.com> writes: >> >>> - regcache_cooked_read (regcache, entry->u.reg.num, reg); >>> - regcache_cooked_write (regcache, entry->u.reg.num, >>> - record_full_get_loc (entry)); >>> - memcpy (record_full_get_loc (entry), reg, entry->u.reg.len); >> >> The original code is about swapping contents of register REG in regcache >> and record_full_get_loc (entry), and the length is known entry->u.reg.len. >> > > Yes. > >>> + value = regcache->cooked_read_value (entry->u.reg.num); >>> + gdb_assert (value != NULL); >>> + regcache->cooked_write (entry->u.reg.num, record_full_get_loc (entry)); >>> + memcpy (record_full_get_loc (entry), value_contents_all (value), >>> + entry->u.reg.len); >>> + release_value (value); >>> + value_free (value); >> >> It is a overkill to use value to swap these two buffers, IMO. How about >> xmalloc "reg" instead? >> > > Given that the code doesn’t use any of the error checking, then agreed. > > > Tested on a --enable-targets=all build with board files unix and > native-gdbserver. > > > Ok to commit? > Alan. > > 2017-06-12 Alan Hayward <alan.hayward@arm.com> > > * gdb/record-full.c (record_full_exec_insn): Allocate buffer. > > > diff --git a/gdb/record-full.c b/gdb/record-full.c > index 31ff558d2a633cff71d3e6082e42f5d6fb88bcf1..4f73e2a5ad0d4a2407b31a1d391e813147e15798 100644 > --- a/gdb/record-full.c > +++ b/gdb/record-full.c > @@ -698,7 +698,7 @@ record_full_exec_insn (struct regcache *regcache, > { > case record_full_reg: /* reg */ > { > - gdb_byte reg[MAX_REGISTER_SIZE]; > + gdb_byte *reg = (gdb_byte *) xmalloc (entry->u.reg.len); > > if (record_debug > 1) > fprintf_unfiltered (gdb_stdlog, > @@ -711,6 +711,7 @@ record_full_exec_insn (struct regcache *regcache, > regcache_cooked_write (regcache, entry->u.reg.num, > record_full_get_loc (entry)); > memcpy (record_full_get_loc (entry), reg, entry->u.reg.len); > + xfree (reg); > } > break; > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Remove an instance of MAX_REGISTER_SIZE from record-full.c 2017-06-12 13:59 ` Alan Hayward 2017-06-21 9:31 ` Alan Hayward @ 2017-06-21 10:33 ` Yao Qi 2017-06-21 10:52 ` Pedro Alves 2 siblings, 0 replies; 8+ messages in thread From: Yao Qi @ 2017-06-21 10:33 UTC (permalink / raw) To: Alan Hayward; +Cc: gdb-patches, nd Alan Hayward <Alan.Hayward@arm.com> writes: > 2017-06-12 Alan Hayward <alan.hayward@arm.com> > > * gdb/record-full.c (record_full_exec_insn): Allocate buffer. Patch is good to me. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Remove an instance of MAX_REGISTER_SIZE from record-full.c 2017-06-12 13:59 ` Alan Hayward 2017-06-21 9:31 ` Alan Hayward 2017-06-21 10:33 ` Yao Qi @ 2017-06-21 10:52 ` Pedro Alves 2017-06-22 13:44 ` Alan Hayward 2 siblings, 1 reply; 8+ messages in thread From: Pedro Alves @ 2017-06-21 10:52 UTC (permalink / raw) To: Alan Hayward, Yao Qi; +Cc: gdb-patches, nd On 06/12/2017 02:59 PM, Alan Hayward wrote: > diff --git a/gdb/record-full.c b/gdb/record-full.c > index 31ff558d2a633cff71d3e6082e42f5d6fb88bcf1..4f73e2a5ad0d4a2407b31a1d391e813147e15798 100644 > --- a/gdb/record-full.c > +++ b/gdb/record-full.c > @@ -698,7 +698,7 @@ record_full_exec_insn (struct regcache *regcache, > { > case record_full_reg: /* reg */ > { > - gdb_byte reg[MAX_REGISTER_SIZE]; > + gdb_byte *reg = (gdb_byte *) xmalloc (entry->u.reg.len); > > if (record_debug > 1) > fprintf_unfiltered (gdb_stdlog, > @@ -711,6 +711,7 @@ record_full_exec_insn (struct regcache *regcache, > regcache_cooked_write (regcache, entry->u.reg.num, > record_full_get_loc (entry)); > memcpy (record_full_get_loc (entry), reg, entry->u.reg.len); > + xfree (reg); You can use gdb::byte_vector reg (entry->u.reg.len); to avoid the explicit xfree here. (and a potential leak if regcache_* throws). Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Remove an instance of MAX_REGISTER_SIZE from record-full.c 2017-06-21 10:52 ` Pedro Alves @ 2017-06-22 13:44 ` Alan Hayward 2017-06-22 13:45 ` Pedro Alves 0 siblings, 1 reply; 8+ messages in thread From: Alan Hayward @ 2017-06-22 13:44 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, gdb-patches, nd > On 21 Jun 2017, at 11:52, Pedro Alves <palves@redhat.com> wrote: > > On 06/12/2017 02:59 PM, Alan Hayward wrote: > >> diff --git a/gdb/record-full.c b/gdb/record-full.c >> index 31ff558d2a633cff71d3e6082e42f5d6fb88bcf1..4f73e2a5ad0d4a2407b31a1d391e813147e15798 100644 >> --- a/gdb/record-full.c >> +++ b/gdb/record-full.c >> @@ -698,7 +698,7 @@ record_full_exec_insn (struct regcache *regcache, >> { >> case record_full_reg: /* reg */ >> { >> - gdb_byte reg[MAX_REGISTER_SIZE]; >> + gdb_byte *reg = (gdb_byte *) xmalloc (entry->u.reg.len); >> >> if (record_debug > 1) >> fprintf_unfiltered (gdb_stdlog, >> @@ -711,6 +711,7 @@ record_full_exec_insn (struct regcache *regcache, >> regcache_cooked_write (regcache, entry->u.reg.num, >> record_full_get_loc (entry)); >> memcpy (record_full_get_loc (entry), reg, entry->u.reg.len); >> + xfree (reg); > > You can use > gdb::byte_vector reg (entry->u.reg.len); > to avoid the explicit xfree here. > > (and a potential leak if regcache_* throws). > > Thanks, > Pedro Alves > Updated and retested. Ok to commit? Alan. diff --git a/gdb/record-full.c b/gdb/record-full.c index 13a0a78a52acdc20c8c4634118be941b1ff55871..c1a95a75ca67c6ad3b286e4b0ed892b27508108b 100644 --- a/gdb/record-full.c +++ b/gdb/record-full.c @@ -36,6 +36,7 @@ #include "observer.h" #include "infrun.h" #include "common/gdb_unlinker.h" +#include "common/byte-vector.h" #include <signal.h> @@ -698,7 +699,7 @@ record_full_exec_insn (struct regcache *regcache, { case record_full_reg: /* reg */ { - gdb_byte reg[MAX_REGISTER_SIZE]; + gdb::byte_vector reg (entry->u.reg.len); if (record_debug > 1) fprintf_unfiltered (gdb_stdlog, @@ -707,10 +708,10 @@ record_full_exec_insn (struct regcache *regcache, host_address_to_string (entry), entry->u.reg.num); - regcache_cooked_read (regcache, entry->u.reg.num, reg); + regcache_cooked_read (regcache, entry->u.reg.num, reg.data ()); regcache_cooked_write (regcache, entry->u.reg.num, record_full_get_loc (entry)); - memcpy (record_full_get_loc (entry), reg, entry->u.reg.len); + memcpy (record_full_get_loc (entry), reg.data (), entry->u.reg.len); } break; ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Remove an instance of MAX_REGISTER_SIZE from record-full.c 2017-06-22 13:44 ` Alan Hayward @ 2017-06-22 13:45 ` Pedro Alves 0 siblings, 0 replies; 8+ messages in thread From: Pedro Alves @ 2017-06-22 13:45 UTC (permalink / raw) To: Alan Hayward; +Cc: Yao Qi, gdb-patches, nd On 06/22/2017 02:44 PM, Alan Hayward wrote: > >> On 21 Jun 2017, at 11:52, Pedro Alves <palves@redhat.com> wrote: >> >> On 06/12/2017 02:59 PM, Alan Hayward wrote: >> >>> diff --git a/gdb/record-full.c b/gdb/record-full.c >>> index 31ff558d2a633cff71d3e6082e42f5d6fb88bcf1..4f73e2a5ad0d4a2407b31a1d391e813147e15798 100644 >>> --- a/gdb/record-full.c >>> +++ b/gdb/record-full.c >>> @@ -698,7 +698,7 @@ record_full_exec_insn (struct regcache *regcache, >>> { >>> case record_full_reg: /* reg */ >>> { >>> - gdb_byte reg[MAX_REGISTER_SIZE]; >>> + gdb_byte *reg = (gdb_byte *) xmalloc (entry->u.reg.len); >>> >>> if (record_debug > 1) >>> fprintf_unfiltered (gdb_stdlog, >>> @@ -711,6 +711,7 @@ record_full_exec_insn (struct regcache *regcache, >>> regcache_cooked_write (regcache, entry->u.reg.num, >>> record_full_get_loc (entry)); >>> memcpy (record_full_get_loc (entry), reg, entry->u.reg.len); >>> + xfree (reg); >> >> You can use >> gdb::byte_vector reg (entry->u.reg.len); >> to avoid the explicit xfree here. >> >> (and a potential leak if regcache_* throws). >> >> Thanks, >> Pedro Alves >> > > Updated and retested. > > Ok to commit? Sure. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-06-22 13:45 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-12 9:34 [PATCH] Remove an instance of MAX_REGISTER_SIZE from record-full.c Alan Hayward 2017-06-12 11:05 ` Yao Qi 2017-06-12 13:59 ` Alan Hayward 2017-06-21 9:31 ` Alan Hayward 2017-06-21 10:33 ` Yao Qi 2017-06-21 10:52 ` Pedro Alves 2017-06-22 13:44 ` Alan Hayward 2017-06-22 13:45 ` Pedro Alves
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).