Message ID | 0DADF920-69B9-4F96-A153-6965E56B5DA8@arm.com |
---|---|
State | New |
Headers | show |
Alan Hayward <Alan.Hayward@arm.com> writes: > diff --git a/gdb/msp430-tdep.c b/gdb/msp430-tdep.c > index 75329dfcc5ed94fff19639db4db21dd0874d0e96..d9eebf0cc2647a079db2f822145d0fb74ea301e4 100644 > --- a/gdb/msp430-tdep.c > +++ b/gdb/msp430-tdep.c > @@ -221,10 +221,9 @@ msp430_pseudo_register_read (struct gdbarch *gdbarch, > struct regcache *regcache, > int regnum, gdb_byte *buffer) > { > - enum register_status status = REG_UNKNOWN; > - > if (MSP430_NUM_REGS <= regnum && regnum < MSP430_NUM_TOTAL_REGS) > { > + enum register_status status; > ULONGEST val; > enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > int regsize = register_size (gdbarch, regnum); > @@ -234,11 +233,10 @@ msp430_pseudo_register_read (struct gdbarch *gdbarch, > if (status == REG_VALID) > store_unsigned_integer (buffer, regsize, byte_order, val); > > + return status; > } > else > gdb_assert_not_reached ("invalid pseudo register number"); > - > - return status; > } This looks reasonable to me, but could you put it into a separate patch so that people interested in msp430 may take a look? > > /* Implement the "pseudo_register_write" gdbarch method. */ > diff --git a/gdb/nds32-tdep.c b/gdb/nds32-tdep.c > index 05c48aa27d84bc0286712f0143a9447a79ae066b..e9955d60a8132d5bc3af234dff0c86c8e59d4855 100644 > --- a/gdb/nds32-tdep.c > +++ b/gdb/nds32-tdep.c > @@ -445,11 +445,11 @@ nds32_pseudo_register_read (struct gdbarch *gdbarch, > struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > gdb_byte reg_buf[8]; > int offset, fdr_regnum; > - enum register_status status = REG_UNKNOWN; > + enum register_status status; > > /* Sanity check. */ > - if (tdep->fpu_freg == -1 || tdep->use_pseudo_fsrs == 0) > - return status; > + gdb_assert (tdep->fpu_freg != -1); > + gdb_assert (tdep->use_pseudo_fsrs > 0); > The nds32 change can go to a separate patch as well, so that nds32 people can take a look too. Secondly, it is not easy to see your change is right or not, unless I read the code in nds32_gdbarch_init, if (fpu_freg == -1) num_regs = NDS32_NUM_REGS; else if (use_pseudo_fsrs == 1) { set_gdbarch_pseudo_register_read (gdbarch, nds32_pseudo_register_read); set_gdbarch_pseudo_register_write (gdbarch, nds32_pseudo_register_write); so please add some comments in the assert like /* This function is only registered when fpu_regs != -1 and use_pseudo_fsrs == 1 in nds32_gdbarch_init. */ gdb_assert (tdep->fpu_freg != -1); gdb_assert (tdep->use_pseudo_fsrs == 1); > regnum -= gdbarch_num_regs (gdbarch); > > @@ -466,9 +466,11 @@ nds32_pseudo_register_read (struct gdbarch *gdbarch, > status = regcache_raw_read (regcache, fdr_regnum, reg_buf); > if (status == REG_VALID) > memcpy (buf, reg_buf + offset, 4); > + > + return status; > } > > - return status; > + gdb_assert_not_reached ("invalid pseudo register number"); > } also, it would be nice to do the same change in nds32_pseudo_register_write too. > @@ -379,7 +388,7 @@ regcache_restore (struct regcache *dst, > void *cooked_read_context) > { > struct gdbarch *gdbarch = dst->descr->gdbarch; > - gdb_byte buf[MAX_REGISTER_SIZE]; > + std::vector<gdb_byte> buf (max_register_size (gdbarch)); > int regnum; > > /* The dst had better not be read-only. If it is, the `restore' > @@ -395,9 +404,9 @@ regcache_restore (struct regcache *dst, > { > enum register_status status; Can we move "buf" here? and initialize it with the register_size, std::vector<gdb_byte> buf (register_size (gdbarch, regnum)); then, we don't need max_register_size (). > @@ -1480,17 +1488,19 @@ regcache_dump (struct regcache *regcache, struct ui_file *file, > fprintf_unfiltered (file, "Cooked value"); > else > { > - enum register_status status; > + struct value *value = regcache_cooked_read_value (regcache, > + regnum); > > - status = regcache_cooked_read (regcache, regnum, buf); > - if (status == REG_UNKNOWN) > - fprintf_unfiltered (file, "<invalid>"); > - else if (status == REG_UNAVAILABLE) > + if (value_optimized_out (value) > + || !value_entirely_available (value)) > fprintf_unfiltered (file, "<unavailable>"); It is still not right to me. With your changes to msp430 and nds32, we won't get REG_UNKNOWN for pseudo registers, but we may still get REG_UNKNOWN from raw registers (from regcache->register_status[]). How is this? gdb_byte *buf = NULL; enum register_status status; struct value * value = NULL; if (regnum < regcache->descr->nr_raw_registers) { regcache_raw_update (regcache, regnum); status = regcache->register_status[regnum]; buf = register_buffer (regcache, regnum); } else { value = regcache_cooked_read_value (regcache, regnum); if (value_entirely_available (value)) { status = REG_VALID; buf = value_contents_all (value); } else status = REG_REG_UNAVAILABLE; } if (status == REG_UNKNOWN) fprintf_unfiltered (file, "<invalid>"); else if (status == REG_UNAVAILABLE) fprintf_unfiltered (file, "<unavailable>"); else print_hex_chars (file, buf, regcache->descr->sizeof_register[regnum], gdbarch_byte_order (gdbarch)); if (value != NULL) { release_value (value); value_free (value); }
> On 5 Apr 2017, at 17:00, Yao Qi <qiyaoltc@gmail.com> wrote: <snip Msp430 and nds32 changes> Moved these out to a different patch >> @@ -379,7 +388,7 @@ regcache_restore (struct regcache *dst, >> void *cooked_read_context) >> { >> struct gdbarch *gdbarch = dst->descr->gdbarch; >> - gdb_byte buf[MAX_REGISTER_SIZE]; >> + std::vector<gdb_byte> buf (max_register_size (gdbarch)); >> int regnum; >> >> /* The dst had better not be read-only. If it is, the `restore' >> @@ -395,9 +404,9 @@ regcache_restore (struct regcache *dst, >> { >> enum register_status status; > > Can we move "buf" here? and initialize it with the register_size, > > std::vector<gdb_byte> buf (register_size (gdbarch, regnum)); > > then, we don't need max_register_size (). > Problem with this is that we are then creating a brand new buffer for each iteration of the loop, which is a little heavyweight. We could create an empty buf outside the loop and re-size it each iteration, but that's still going to cost. We'll still need to keep max_register_size () if we want to add checks that the FOO_MAX_REGISTER size defines are big enough. (see the BFIN_MAX_REGISTER_SIZE email thread) >> @@ -1480,17 +1488,19 @@ regcache_dump (struct regcache *regcache, struct ui_file *file, >> fprintf_unfiltered (file, "Cooked value"); >> else >> { >> - enum register_status status; >> + struct value *value = regcache_cooked_read_value (regcache, >> + regnum); >> >> - status = regcache_cooked_read (regcache, regnum, buf); >> - if (status == REG_UNKNOWN) >> - fprintf_unfiltered (file, "<invalid>"); >> - else if (status == REG_UNAVAILABLE) >> + if (value_optimized_out (value) >> + || !value_entirely_available (value)) >> fprintf_unfiltered (file, "<unavailable>"); > > It is still not right to me. With your changes to msp430 and nds32, we > won't get REG_UNKNOWN for pseudo registers, but we may still get > REG_UNKNOWN from raw registers (from regcache->register_status[]). How > is this? > > gdb_byte *buf = NULL; > enum register_status status; > struct value * value = NULL; > > if (regnum < regcache->descr->nr_raw_registers) > { > regcache_raw_update (regcache, regnum); > > status = regcache->register_status[regnum]; > buf = register_buffer (regcache, regnum); > } > else > { > value = regcache_cooked_read_value (regcache, regnum); > > if (value_entirely_available (value)) > { > status = REG_VALID; > buf = value_contents_all (value); > } > else > status = REG_REG_UNAVAILABLE; > } > > if (status == REG_UNKNOWN) > fprintf_unfiltered (file, "<invalid>"); > else if (status == REG_UNAVAILABLE) > fprintf_unfiltered (file, "<unavailable>"); > else > print_hex_chars (file, buf, > regcache->descr->sizeof_register[regnum], > gdbarch_byte_order (gdbarch)); > > if (value != NULL) > { > release_value (value); > value_free (value); > } > Yes, I’ll add those changes.
diff --git a/gdb/msp430-tdep.c b/gdb/msp430-tdep.c index 75329dfcc5ed94fff19639db4db21dd0874d0e96..d9eebf0cc2647a079db2f822145d0fb74ea301e4 100644 --- a/gdb/msp430-tdep.c +++ b/gdb/msp430-tdep.c @@ -221,10 +221,9 @@ msp430_pseudo_register_read (struct gdbarch *gdbarch, struct regcache *regcache, int regnum, gdb_byte *buffer) { - enum register_status status = REG_UNKNOWN; - if (MSP430_NUM_REGS <= regnum && regnum < MSP430_NUM_TOTAL_REGS) { + enum register_status status; ULONGEST val; enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); int regsize = register_size (gdbarch, regnum); @@ -234,11 +233,10 @@ msp430_pseudo_register_read (struct gdbarch *gdbarch, if (status == REG_VALID) store_unsigned_integer (buffer, regsize, byte_order, val); + return status; } else gdb_assert_not_reached ("invalid pseudo register number"); - - return status; } /* Implement the "pseudo_register_write" gdbarch method. */ diff --git a/gdb/nds32-tdep.c b/gdb/nds32-tdep.c index 05c48aa27d84bc0286712f0143a9447a79ae066b..e9955d60a8132d5bc3af234dff0c86c8e59d4855 100644 --- a/gdb/nds32-tdep.c +++ b/gdb/nds32-tdep.c @@ -445,11 +445,11 @@ nds32_pseudo_register_read (struct gdbarch *gdbarch, struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); gdb_byte reg_buf[8]; int offset, fdr_regnum; - enum register_status status = REG_UNKNOWN; + enum register_status status; /* Sanity check. */ - if (tdep->fpu_freg == -1 || tdep->use_pseudo_fsrs == 0) - return status; + gdb_assert (tdep->fpu_freg != -1); + gdb_assert (tdep->use_pseudo_fsrs > 0); regnum -= gdbarch_num_regs (gdbarch); @@ -466,9 +466,11 @@ nds32_pseudo_register_read (struct gdbarch *gdbarch, status = regcache_raw_read (regcache, fdr_regnum, reg_buf); if (status == REG_VALID) memcpy (buf, reg_buf + offset, 4); + + return status; } - return status; + gdb_assert_not_reached ("invalid pseudo register number"); } /* Implement the "pseudo_register_write" gdbarch method. */ diff --git a/gdb/regcache.h b/gdb/regcache.h index 1dd4127818ba1f6fa5b9e5f2d326843833d42945..f201f0c084f40ccf276a7e1ba19050cbc11208ad 100644 --- a/gdb/regcache.h +++ b/gdb/regcache.h @@ -215,6 +215,8 @@ extern struct type *register_type (struct gdbarch *gdbarch, int regnum); extern int register_size (struct gdbarch *gdbarch, int regnum); +/* Return the size of the largest register. */ +extern long max_register_size (struct gdbarch *gdbarch); /* Save/restore a register cache. The set of registers saved / restored into the DST regcache determined by the save_reggroup / diff --git a/gdb/regcache.c b/gdb/regcache.c index 8bbd3a655c007d649b91ec0e3d2374553990923f..cc621cf248c44d6159b68e01eb130fa5bf176fb3 100644 --- a/gdb/regcache.c +++ b/gdb/regcache.c @@ -73,6 +73,9 @@ struct regcache_descr /* Cached table containing the type of each register. */ struct type **register_type; + + /* Size of the largest register. */ + long max_register_size; }; static void * @@ -126,6 +129,8 @@ init_regcache_descr (struct gdbarch *gdbarch) descr->register_offset[i] = offset; offset += descr->sizeof_register[i]; gdb_assert (MAX_REGISTER_SIZE >= descr->sizeof_register[i]); + descr->max_register_size = std::max (descr->max_register_size, + descr->sizeof_register[i]); } /* Set the real size of the raw register cache buffer. */ descr->sizeof_raw_registers = offset; @@ -136,6 +141,8 @@ init_regcache_descr (struct gdbarch *gdbarch) descr->register_offset[i] = offset; offset += descr->sizeof_register[i]; gdb_assert (MAX_REGISTER_SIZE >= descr->sizeof_register[i]); + descr->max_register_size = std::max (descr->max_register_size, + descr->sizeof_register[i]); } /* Set the real size of the readonly register cache buffer. */ descr->sizeof_cooked_registers = offset; @@ -187,6 +194,13 @@ regcache_register_size (const struct regcache *regcache, int n) return register_size (get_regcache_arch (regcache), n); } +long +max_register_size (struct gdbarch *gdbarch) +{ + struct regcache_descr *descr = regcache_descr (gdbarch); + return descr->max_register_size; +} + /* The register cache for storing raw register values. */ struct regcache @@ -337,7 +351,6 @@ regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read, void *src) { struct gdbarch *gdbarch = dst->descr->gdbarch; - gdb_byte buf[MAX_REGISTER_SIZE]; int regnum; /* The DST should be `read-only', if it wasn't then the save would @@ -356,17 +369,13 @@ regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read, { if (gdbarch_register_reggroup_p (gdbarch, regnum, save_reggroup)) { - enum register_status status = cooked_read (src, regnum, buf); + gdb_byte *dst_buf = register_buffer (dst, regnum); + enum register_status status = cooked_read (src, regnum, dst_buf); - if (status == REG_VALID) - memcpy (register_buffer (dst, regnum), buf, - register_size (gdbarch, regnum)); - else + if (status != REG_VALID) { gdb_assert (status != REG_UNKNOWN); - - memset (register_buffer (dst, regnum), 0, - register_size (gdbarch, regnum)); + memset (dst_buf, 0, register_size (gdbarch, regnum)); } dst->register_status[regnum] = status; } @@ -379,7 +388,7 @@ regcache_restore (struct regcache *dst, void *cooked_read_context) { struct gdbarch *gdbarch = dst->descr->gdbarch; - gdb_byte buf[MAX_REGISTER_SIZE]; + std::vector<gdb_byte> buf (max_register_size (gdbarch)); int regnum; /* The dst had better not be read-only. If it is, the `restore' @@ -395,9 +404,9 @@ regcache_restore (struct regcache *dst, { enum register_status status; - status = cooked_read (cooked_read_context, regnum, buf); + status = cooked_read (cooked_read_context, regnum, buf.data ()); if (status == REG_VALID) - regcache_cooked_write (dst, regnum, buf); + regcache_cooked_write (dst, regnum, buf.data ()); } } } @@ -1339,7 +1348,6 @@ regcache_dump (struct regcache *regcache, struct ui_file *file, int footnote_register_offset = 0; int footnote_register_type_name_null = 0; long register_offset = 0; - gdb_byte buf[MAX_REGISTER_SIZE]; #if 0 fprintf_unfiltered (file, "nr_raw_registers %d\n", @@ -1466,8 +1474,8 @@ regcache_dump (struct regcache *regcache, struct ui_file *file, fprintf_unfiltered (file, "<unavailable>"); else { - regcache_raw_read (regcache, regnum, buf); - print_hex_chars (file, buf, + regcache_raw_update (regcache, regnum); + print_hex_chars (file, register_buffer (regcache, regnum), regcache->descr->sizeof_register[regnum], gdbarch_byte_order (gdbarch)); } @@ -1480,17 +1488,19 @@ regcache_dump (struct regcache *regcache, struct ui_file *file, fprintf_unfiltered (file, "Cooked value"); else { - enum register_status status; + struct value *value = regcache_cooked_read_value (regcache, + regnum); - status = regcache_cooked_read (regcache, regnum, buf); - if (status == REG_UNKNOWN) - fprintf_unfiltered (file, "<invalid>"); - else if (status == REG_UNAVAILABLE) + if (value_optimized_out (value) + || !value_entirely_available (value)) fprintf_unfiltered (file, "<unavailable>"); else - print_hex_chars (file, buf, + print_hex_chars (file, value_contents_all (value), regcache->descr->sizeof_register[regnum], gdbarch_byte_order (gdbarch)); + + release_value (value); + value_free (value); } }