* [PATCH 8/11] Add FRV_MAX_REGISTER_SIZE @ 2017-04-04 10:14 Alan Hayward 2017-04-11 10:02 ` Yao Qi 0 siblings, 1 reply; 10+ messages in thread From: Alan Hayward @ 2017-04-04 10:14 UTC (permalink / raw) To: gdb-patches; +Cc: nd Max size set to 64bits, which I determined using frv_register_type() Tested on a --enable-targets=all build using make check with board files unix and native-gdbserver. I do not have a FRV machine to test on. Ok to commit? Alan. 2017-04-04 Alan Hayward <alan.hayward@arm.com> * frv-linux-tdep.c (frv_linux_supply_gregset): Use FRV_MAX_REGISTER_SIZE. * frv-tdep.h (FRV_MAX_REGISTER_SIZE): Add. diff --git a/gdb/frv-linux-tdep.c b/gdb/frv-linux-tdep.c index eb87f93058b0287e8f05c585d1b6aa1ff2bffb78..6f5585e774c86afe7e271f90df1bf3db93340421 100644 --- a/gdb/frv-linux-tdep.c +++ b/gdb/frv-linux-tdep.c @@ -413,9 +413,9 @@ frv_linux_supply_gregset (const struct regset *regset, int regnum, const void *gregs, size_t len) { int regi; - char zerobuf[MAX_REGISTER_SIZE]; + char zerobuf[FRV_MAX_REGISTER_SIZE]; - memset (zerobuf, 0, MAX_REGISTER_SIZE); + memset (zerobuf, 0, FRV_MAX_REGISTER_SIZE); /* gr0 always contains 0. Also, the kernel passes the TBR value in this slot. */ diff --git a/gdb/frv-tdep.h b/gdb/frv-tdep.h index c1a0f35456c879c655198820f5319bf0ab42b9f7..9c8c32eb08b6e4b4d87139b201008c1ee419f6a9 100644 --- a/gdb/frv-tdep.h +++ b/gdb/frv-tdep.h @@ -92,6 +92,9 @@ enum { frv_num_pseudo_regs = last_pseudo_regnum - first_pseudo_regnum + 1, }; +/* Big enough to hold the size of the largest register in bytes. */ +#define FRV_MAX_REGISTER_SIZE 8 + /* Return the FR-V ABI associated with GDBARCH. */ enum frv_abi frv_abi (struct gdbarch *gdbarch); ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 8/11] Add FRV_MAX_REGISTER_SIZE 2017-04-04 10:14 [PATCH 8/11] Add FRV_MAX_REGISTER_SIZE Alan Hayward @ 2017-04-11 10:02 ` Yao Qi 2017-04-12 8:27 ` Alan Hayward 0 siblings, 1 reply; 10+ messages in thread From: Yao Qi @ 2017-04-11 10:02 UTC (permalink / raw) To: Alan Hayward; +Cc: gdb-patches Alan Hayward <Alan.Hayward@arm.com> writes: > - char zerobuf[MAX_REGISTER_SIZE]; > + char zerobuf[FRV_MAX_REGISTER_SIZE]; > > - memset (zerobuf, 0, MAX_REGISTER_SIZE); > + memset (zerobuf, 0, FRV_MAX_REGISTER_SIZE); > > /* gr0 always contains 0. Also, the kernel passes the TBR value in > this slot. */ The code here fills some gr registers with zeros, /* gr0 always contains 0. Also, the kernel passes the TBR value in this slot. */ regcache_raw_supply (regcache, first_gpr_regnum, zerobuf); /* Fill gr32, ..., gr63 with zeros. */ for (regi = first_gpr_regnum + 32; regi <= last_gpr_regnum; regi++) regcache_raw_supply (regcache, regi, zerobuf); the size of these gr registers are know, 8 bytes. It won't be changed. We can do, gdb_byte zerobuf[8] = { 0 }; the code is still easy to read. If you really dislike magic number (IMO, 8 is not a magic number in this context), you can define FRR_GR_REGISTER_SIZE. Alternatively, you can add a new regache api, regcache_raw_supply_zero. Many places can use this api, and some uses of MAX_REGISTER_SIZE can be removed too, for example, regcache_raw_supply (regcache, MIPS_ZERO_REGNUM, zerobuf); -- Yao (齐尧) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 8/11] Add FRV_MAX_REGISTER_SIZE 2017-04-11 10:02 ` Yao Qi @ 2017-04-12 8:27 ` Alan Hayward 2017-04-27 8:55 ` Alan Hayward 0 siblings, 1 reply; 10+ messages in thread From: Alan Hayward @ 2017-04-12 8:27 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches, nd > On 11 Apr 2017, at 11:02, Yao Qi <qiyaoltc@gmail.com> wrote: > > Alan Hayward <Alan.Hayward@arm.com> writes: > >> - char zerobuf[MAX_REGISTER_SIZE]; >> + char zerobuf[FRV_MAX_REGISTER_SIZE]; >> >> - memset (zerobuf, 0, MAX_REGISTER_SIZE); >> + memset (zerobuf, 0, FRV_MAX_REGISTER_SIZE); >> >> /* gr0 always contains 0. Also, the kernel passes the TBR value in >> this slot. */ > > The code here fills some gr registers with zeros, > > /* gr0 always contains 0. Also, the kernel passes the TBR value in > this slot. */ > regcache_raw_supply (regcache, first_gpr_regnum, zerobuf); > > /* Fill gr32, ..., gr63 with zeros. */ > for (regi = first_gpr_regnum + 32; regi <= last_gpr_regnum; regi++) > regcache_raw_supply (regcache, regi, zerobuf); > > the size of these gr registers are know, 8 bytes. It won't be changed. > We can do, > > gdb_byte zerobuf[8] = { 0 }; > > the code is still easy to read. If you really dislike magic number (IMO, 8 > is not a magic number in this context), you can define FRR_GR_REGISTER_SIZE. > > Alternatively, you can add a new regache api, regcache_raw_supply_zero. > Many places can use this api, and some uses of MAX_REGISTER_SIZE can be > removed too, for example, > > regcache_raw_supply (regcache, MIPS_ZERO_REGNUM, zerobuf); > > -- > Yao (齐尧) Went with the simpler solution. I don't have a FRV machine to test on. Tested on a --enable-targets=all build using make check with board files unix and native-gdbserver. Ok to commit? Alan. 2017-04-12 Alan Hayward <alan.hayward@arm.com> * gdb/frv-linux-tdep.c (frv_linux_supply_gregset): Remove MAX_REGISTER_SIZE. diff --git a/gdb/frv-linux-tdep.c b/gdb/frv-linux-tdep.c index eb87f93058b0287e8f05c585d1b6aa1ff2bffb78..3f91bf0ee81a5d5ca915c88337c6a4b4ed53d344 100644 --- a/gdb/frv-linux-tdep.c +++ b/gdb/frv-linux-tdep.c @@ -413,9 +413,7 @@ frv_linux_supply_gregset (const struct regset *regset, int regnum, const void *gregs, size_t len) { int regi; - char zerobuf[MAX_REGISTER_SIZE]; - - memset (zerobuf, 0, MAX_REGISTER_SIZE); + gdb_byte zerobuf[8] = { 0 }; /* gr0 always contains 0. Also, the kernel passes the TBR value in this slot. */ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 8/11] Add FRV_MAX_REGISTER_SIZE 2017-04-12 8:27 ` Alan Hayward @ 2017-04-27 8:55 ` Alan Hayward 2017-05-03 8:44 ` Yao Qi 0 siblings, 1 reply; 10+ messages in thread From: Alan Hayward @ 2017-04-27 8:55 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches, nd > On 12 Apr 2017, at 09:27, Alan Hayward <Alan.Hayward@arm.com> wrote: > > >> On 11 Apr 2017, at 11:02, Yao Qi <qiyaoltc@gmail.com> wrote: >> >> Alan Hayward <Alan.Hayward@arm.com> writes: >> >>> - char zerobuf[MAX_REGISTER_SIZE]; >>> + char zerobuf[FRV_MAX_REGISTER_SIZE]; >>> >>> - memset (zerobuf, 0, MAX_REGISTER_SIZE); >>> + memset (zerobuf, 0, FRV_MAX_REGISTER_SIZE); >>> >>> /* gr0 always contains 0. Also, the kernel passes the TBR value in >>> this slot. */ >> >> The code here fills some gr registers with zeros, >> >> /* gr0 always contains 0. Also, the kernel passes the TBR value in >> this slot. */ >> regcache_raw_supply (regcache, first_gpr_regnum, zerobuf); >> >> /* Fill gr32, ..., gr63 with zeros. */ >> for (regi = first_gpr_regnum + 32; regi <= last_gpr_regnum; regi++) >> regcache_raw_supply (regcache, regi, zerobuf); >> >> the size of these gr registers are know, 8 bytes. It won't be changed. >> We can do, >> >> gdb_byte zerobuf[8] = { 0 }; >> >> the code is still easy to read. If you really dislike magic number (IMO, 8 >> is not a magic number in this context), you can define FRR_GR_REGISTER_SIZE. >> >> Alternatively, you can add a new regache api, regcache_raw_supply_zero. >> Many places can use this api, and some uses of MAX_REGISTER_SIZE can be >> removed too, for example, >> >> regcache_raw_supply (regcache, MIPS_ZERO_REGNUM, zerobuf); >> >> -- >> Yao (齐尧) > > Went with the simpler solution. > > I don't have a FRV machine to test on. > Tested on a --enable-targets=all build using make check with board files > unix and native-gdbserver. > Looking again at my later patches, I agree that regcache_raw_supply_zero would be useful in other places too. I considered making regcache_raw_supply_zero call regcache_raw_supply, but in the end it made more sense to make it completely separate. I don't have a FRV machine to test on. Tested on a --enable-targets=all build using make check with board files unix and native-gdbserver. Ok to commit? Alan. 2017-04-27 Alan Hayward <alan.hayward@arm.com> * gdb/frv-linux-tdep.c (frv_linux_supply_gregset): Call regcache_raw_supply_zero * gdb/regcache.c (regcache_raw_supply_zero): New. * gdb/regcache.h (regcache_raw_supply_zero): New declaration. diff --git a/gdb/frv-linux-tdep.c b/gdb/frv-linux-tdep.c index eb87f93058b0287e8f05c585d1b6aa1ff2bffb78..d52eb2163437e32029c989c2caecce8533f11a65 100644 --- a/gdb/frv-linux-tdep.c +++ b/gdb/frv-linux-tdep.c @@ -413,17 +413,14 @@ frv_linux_supply_gregset (const struct regset *regset, int regnum, const void *gregs, size_t len) { int regi; - char zerobuf[MAX_REGISTER_SIZE]; - - memset (zerobuf, 0, MAX_REGISTER_SIZE); /* gr0 always contains 0. Also, the kernel passes the TBR value in this slot. */ - regcache_raw_supply (regcache, first_gpr_regnum, zerobuf); + regcache_raw_supply_zero (regcache, first_gpr_regnum); /* Fill gr32, ..., gr63 with zeros. */ for (regi = first_gpr_regnum + 32; regi <= last_gpr_regnum; regi++) - regcache_raw_supply (regcache, regi, zerobuf); + regcache_raw_supply_zero (regcache, regi); regcache_supply_regset (regset, regcache, regnum, gregs, len); } diff --git a/gdb/regcache.h b/gdb/regcache.h index f201f0c084f40ccf276a7e1ba19050cbc11208ad..ff6e583a48f9200f4a46ea5fcca69bfdd2862dac 100644 --- a/gdb/regcache.h +++ b/gdb/regcache.h @@ -152,6 +152,12 @@ extern void regcache_raw_supply (struct regcache *regcache, extern void regcache_raw_collect (const struct regcache *regcache, int regnum, void *buf); +/* Supply register REGNUM with zeroed value to REGCACHE. This is not the same + as calling regcache_raw_supply with NULL (which will set the state to + unavailable). */ + +extern void regcache_raw_supply_zero (struct regcache *regcache, int regnum); + /* Collect register REGNUM from SOURCE_REGCACHE and store its contents to DEST_REGCACHE. */ extern void regcache_raw_copy (const struct regcache *dest_regcache, diff --git a/gdb/regcache.c b/gdb/regcache.c index 20e43f486ff02f15000bebee2a4eb03db53111d4..03f7d216f7d2fb8dd2e5b1189c9743ed37facb30 100644 --- a/gdb/regcache.c +++ b/gdb/regcache.c @@ -1125,6 +1125,27 @@ regcache_raw_collect (const struct regcache *regcache, int regnum, void *buf) memcpy (buf, regbuf, size); } +/* Supply register REGNUM with zeroed value to REGCACHE. This is not the same + as calling regcache_raw_supply with NULL (which will set the state to + unavailable). */ + +void +regcache_raw_supply_zero (struct regcache *regcache, int regnum) +{ + void *regbuf; + size_t size; + + gdb_assert (regcache != NULL); + gdb_assert (regnum >= 0 && regnum < regcache->descr->nr_raw_registers); + gdb_assert (!regcache->readonly_p); + + regbuf = register_buffer (regcache, regnum); + size = regcache->descr->sizeof_register[regnum]; + + memset (regbuf, 0, size); + regcache->register_status[regnum] = REG_VALID; +} + void regcache_raw_copy (const struct regcache *dest_regcache, int regnum, struct regcache *src_regcache) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 8/11] Add FRV_MAX_REGISTER_SIZE 2017-04-27 8:55 ` Alan Hayward @ 2017-05-03 8:44 ` Yao Qi 2017-05-03 9:27 ` Pedro Alves 0 siblings, 1 reply; 10+ messages in thread From: Yao Qi @ 2017-05-03 8:44 UTC (permalink / raw) To: Alan Hayward; +Cc: gdb-patches, nd Alan Hayward <Alan.Hayward@arm.com> writes: Hi Alan, regcache.c is updated, so please update your patch. > I considered making regcache_raw_supply_zero call regcache_raw_supply, but > in the end it made more sense to make it completely separate. You can call raw_supply (regnum, NULL) and then set the status to REG_VALID. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 8/11] Add FRV_MAX_REGISTER_SIZE 2017-05-03 8:44 ` Yao Qi @ 2017-05-03 9:27 ` Pedro Alves 2017-05-03 10:56 ` Alan Hayward 0 siblings, 1 reply; 10+ messages in thread From: Pedro Alves @ 2017-05-03 9:27 UTC (permalink / raw) To: Yao Qi, Alan Hayward; +Cc: gdb-patches, nd On 05/03/2017 09:44 AM, Yao Qi wrote: > Alan Hayward <Alan.Hayward@arm.com> writes: > > Hi Alan, > regcache.c is updated, so please update your patch. > >> I considered making regcache_raw_supply_zero call regcache_raw_supply, but >> in the end it made more sense to make it completely separate. > > You can call raw_supply (regnum, NULL) and then set the status to REG_VALID. > I think I agree with Alan -- if we defer to raw_supply, then I'd still prefer that the memset is still done in regcache_raw_supply_zero, because whether unavailable registers actually have a contents buffer at all is implementation detail. We currently zero REG_UNVAILABLE registers in raw_supply, but that could change. (And if we reuse raw_supply as is, then memset, we'll memset twice.) BTW, note that gdbserver has an equivalent function, called "supply_register_zeroed". Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 8/11] Add FRV_MAX_REGISTER_SIZE 2017-05-03 9:27 ` Pedro Alves @ 2017-05-03 10:56 ` Alan Hayward 2017-05-03 11:23 ` Pedro Alves 2017-05-03 13:34 ` Yao Qi 0 siblings, 2 replies; 10+ messages in thread From: Alan Hayward @ 2017-05-03 10:56 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, gdb-patches, nd > On 3 May 2017, at 10:27, Pedro Alves <palves@redhat.com> wrote: > > On 05/03/2017 09:44 AM, Yao Qi wrote: >> Alan Hayward <Alan.Hayward@arm.com> writes: >> >> Hi Alan, >> regcache.c is updated, so please update your patch. >> >>> I considered making regcache_raw_supply_zero call regcache_raw_supply, but >>> in the end it made more sense to make it completely separate. >> >> You can call raw_supply (regnum, NULL) and then set the status to REG_VALID. >> > > I think I agree with Alan -- if we defer to raw_supply, then I'd still prefer > that the memset is still done in regcache_raw_supply_zero, because whether > unavailable registers actually have a contents buffer at all is > implementation detail. We currently zero REG_UNVAILABLE registers in raw_supply, > but that could change. (And if we reuse raw_supply as is, then memset, we'll > memset twice.) > > BTW, note that gdbserver has an equivalent function, called > "supply_register_zeroed". > Agreed. I didn't want somebody in the future to change raw_supply and cause raw_supply_zero to break. Patch updated to head and raw_supply_zero moved into regcache class. Tested on a --enable-targets=all using make check with board files unix and native-gdbserver. Ok to commit? Alan. 2017-05-03 Alan Hayward <alan.hayward@arm.com> * frv-linux-tdep.c (frv_linux_supply_gregset): Use raw_supply_zero. * regcache.c (regcache::raw_supply_zero): New function. * regcache.h (regcache::raw_supply_zero: New declaration. diff --git a/gdb/frv-linux-tdep.c b/gdb/frv-linux-tdep.c index eb87f93058b0287e8f05c585d1b6aa1ff2bffb78..30e5bf00c199a0193ea6cac3a14070445492d3fe 100644 --- a/gdb/frv-linux-tdep.c +++ b/gdb/frv-linux-tdep.c @@ -413,17 +413,14 @@ frv_linux_supply_gregset (const struct regset *regset, int regnum, const void *gregs, size_t len) { int regi; - char zerobuf[MAX_REGISTER_SIZE]; - - memset (zerobuf, 0, MAX_REGISTER_SIZE); /* gr0 always contains 0. Also, the kernel passes the TBR value in this slot. */ - regcache_raw_supply (regcache, first_gpr_regnum, zerobuf); + regcache->raw_supply_zero (first_gpr_regnum); /* Fill gr32, ..., gr63 with zeros. */ for (regi = first_gpr_regnum + 32; regi <= last_gpr_regnum; regi++) - regcache_raw_supply (regcache, regi, zerobuf); + regcache->raw_supply_zero (regi); regcache_supply_regset (regset, regcache, regnum, gregs, len); } diff --git a/gdb/regcache.h b/gdb/regcache.h index 346d290b752d8809f78b72c104ccbc2f5574d83c..ee949597d5691b692b1162a92bc328266fdc160a 100644 --- a/gdb/regcache.h +++ b/gdb/regcache.h @@ -296,6 +296,8 @@ public: void raw_supply (int regnum, const void *buf); + void raw_supply_zero (int regnum); + void raw_copy (int regnum, struct regcache *src_regcache); enum register_status get_register_status (int regnum) const; diff --git a/gdb/regcache.c b/gdb/regcache.c index 748c30e2d4c5f572e9741f73a39c4bc555d1d663..89c2eb55e29af9a806a684382dc928c307ff934b 100644 --- a/gdb/regcache.c +++ b/gdb/regcache.c @@ -1208,6 +1208,26 @@ regcache::raw_supply (int regnum, const void *buf) } } +/* 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 +regcache::raw_supply_zero (int regnum) +{ + void *regbuf; + size_t size; + + gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers); + gdb_assert (!m_readonly_p); + + regbuf = register_buffer (regnum); + size = m_descr->sizeof_register[regnum]; + + memset (regbuf, 0, size); + m_register_status[regnum] = REG_VALID; +} + /* Collect register REGNUM from REGCACHE and store its contents in BUF. */ void ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 8/11] Add FRV_MAX_REGISTER_SIZE 2017-05-03 10:56 ` Alan Hayward @ 2017-05-03 11:23 ` Pedro Alves 2017-05-03 11:36 ` Alan Hayward 2017-05-03 13:34 ` Yao Qi 1 sibling, 1 reply; 10+ messages in thread From: Pedro Alves @ 2017-05-03 11:23 UTC (permalink / raw) To: Alan Hayward; +Cc: Yao Qi, gdb-patches, nd On 05/03/2017 11:56 AM, Alan Hayward wrote: >> BTW, note that gdbserver has an equivalent function, called >> "supply_register_zeroed". > +/* 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 > +regcache::raw_supply_zero (int regnum) A very minor detail, but I'd prefer it if gdb and gdbserver agreed on terminology. Should we call this one "zeroed" too, or rename gdbserver's to "zero"? Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 8/11] Add FRV_MAX_REGISTER_SIZE 2017-05-03 11:23 ` Pedro Alves @ 2017-05-03 11:36 ` Alan Hayward 0 siblings, 0 replies; 10+ messages in thread From: Alan Hayward @ 2017-05-03 11:36 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, gdb-patches, nd > On 3 May 2017, at 12:23, Pedro Alves <palves@redhat.com> wrote: > > On 05/03/2017 11:56 AM, Alan Hayward wrote: > >>> BTW, note that gdbserver has an equivalent function, called >>> "supply_register_zeroed". > >> +/* 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 >> +regcache::raw_supply_zero (int regnum) > > A very minor detail, but I'd prefer it if gdb and gdbserver agreed > on terminology. Should we call this one "zeroed" too, or > rename gdbserver's to "zero"? > > Thanks, > Pedro Alves > I’m happy to update raw_supply to raw_supply_zeroed. I think that sounds better too. Alan. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 8/11] Add FRV_MAX_REGISTER_SIZE 2017-05-03 10:56 ` Alan Hayward 2017-05-03 11:23 ` Pedro Alves @ 2017-05-03 13:34 ` Yao Qi 1 sibling, 0 replies; 10+ messages in thread From: Yao Qi @ 2017-05-03 13:34 UTC (permalink / raw) To: Alan Hayward; +Cc: Pedro Alves, gdb-patches, nd Alan Hayward <Alan.Hayward@arm.com> writes: Hi Alan, > +/* Supply register REGNUM with zeroed value to REGCACHE. This is not the same REGCACHE don't have to be capitalized. This line is too long. > + as calling raw_supply with NULL (which will set the state to > + unavailable). */ > + > +void > +regcache::raw_supply_zero (int regnum) s/raw_supply_zero/raw_supply_zeroed/ OK with these changes. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-05-03 13:34 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-04 10:14 [PATCH 8/11] Add FRV_MAX_REGISTER_SIZE Alan Hayward 2017-04-11 10:02 ` Yao Qi 2017-04-12 8:27 ` Alan Hayward 2017-04-27 8:55 ` Alan Hayward 2017-05-03 8:44 ` Yao Qi 2017-05-03 9:27 ` Pedro Alves 2017-05-03 10:56 ` Alan Hayward 2017-05-03 11:23 ` Pedro Alves 2017-05-03 11:36 ` Alan Hayward 2017-05-03 13:34 ` 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).