This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Remove MAX_REGISTER_SIZE from regcache.c
- From: Yao Qi <qiyaoltc at gmail dot com>
- To: Alan Hayward <Alan dot Hayward at arm dot com>
- Cc: "gdb-patches\@sourceware.org" <gdb-patches at sourceware dot org>,
- Date: Wed, 05 Apr 2017 17:00:56 +0100
- Subject: Re: [PATCH] Remove MAX_REGISTER_SIZE from regcache.c
- Authentication-results: sourceware.org; auth=none
- References: <562B2F6F-F3C6-4A76-9489-57539F396C94@arm.com> <868tnvukjh.fsf@gmail.com> <7359B5C0-BF61-42E2-9886-B322C1825865@arm.com> <0DADF920-69B9-4F96-A153-6965E56B5DA8@arm.com>
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);
}
--
Yao (齐尧)