Replace regbuf with regcache in record-full.c
Commit Message
In record-full.c, instead of backing up all the registers into a large
buffer, duplicate the regcache.
This enables the removal of an instance of MAX_REGISTER_SIZE.
Note that regcache_dup() create a read-only copy of a register cache,
which ensures the new regcache cannot write back to the target.
Once created, we need to be able to copy registers between the two caches,
which we want to do without creating a temporary buffer.
I've added regcache::raw_copy() to allow the copying of raw registers
between two regcaches - either of which might be set as read-only.
Alternatively, I could make the new regcache as writable (by enabling a
regcache copy constructor). But, I think this would be dangerous as it
it then has the potential to write back to the target if the wrong function
is called.
Tested on a --enable-targets=all build with board files unix and
native-gdbserver.
Alan.
2017-06-14 Alan Hayward <alan.hayward@arm.com>
* gdb/record-full.c (record_full_core_regbuf): Remove.
(record_full_core_regcache): New.
(record_full_core_open_1): Duplicate regcache
(record_full_close): Delete duplicated regcache.
(record_full_core_fetch_registers): Use duplicated regcache.
(record_full_core_store_registers): Likewise.
* gdb/regcache.c (regcache::raw_copy): New function.
* gdb/regcache.h (regcache::raw_copy): New declaration.
Comments
PING
> On 14 Jun 2017, at 15:02, Alan Hayward <Alan.Hayward@arm.com> wrote:
>
> In record-full.c, instead of backing up all the registers into a large
> buffer, duplicate the regcache.
> This enables the removal of an instance of MAX_REGISTER_SIZE.
>
> Note that regcache_dup() create a read-only copy of a register cache,
> which ensures the new regcache cannot write back to the target.
>
> Once created, we need to be able to copy registers between the two caches,
> which we want to do without creating a temporary buffer.
>
> I've added regcache::raw_copy() to allow the copying of raw registers
> between two regcaches - either of which might be set as read-only.
>
> Alternatively, I could make the new regcache as writable (by enabling a
> regcache copy constructor). But, I think this would be dangerous as it
> it then has the potential to write back to the target if the wrong function
> is called.
>
> Tested on a --enable-targets=all build with board files unix and
> native-gdbserver.
>
>
> Alan.
>
>
> 2017-06-14 Alan Hayward <alan.hayward@arm.com>
>
> * gdb/record-full.c (record_full_core_regbuf): Remove.
> (record_full_core_regcache): New.
> (record_full_core_open_1): Duplicate regcache
> (record_full_close): Delete duplicated regcache.
> (record_full_core_fetch_registers): Use duplicated regcache.
> (record_full_core_store_registers): Likewise.
> * gdb/regcache.c (regcache::raw_copy): New function.
> * gdb/regcache.h (regcache::raw_copy): New declaration.
>
>
> diff --git a/gdb/record-full.c b/gdb/record-full.c
> index 4f73e2a5ad0d4a2407b31a1d391e813147e15798..00bbda213605110324a6529b9d4d538cd292c62f 100644
> --- a/gdb/record-full.c
> +++ b/gdb/record-full.c
> @@ -167,7 +167,7 @@ struct record_full_core_buf_entry
> };
>
> /* Record buf with core target. */
> -static gdb_byte *record_full_core_regbuf = NULL;
> +static struct regcache *record_full_core_regcache = NULL;
> static struct target_section *record_full_core_start;
> static struct target_section *record_full_core_end;
> static struct record_full_core_buf_entry *record_full_core_buf_list = NULL;
> @@ -793,22 +793,17 @@ static void
> record_full_core_open_1 (const char *name, int from_tty)
> {
> struct regcache *regcache = get_current_regcache ();
> - int regnum = gdbarch_num_regs (get_regcache_arch (regcache));
> - int i;
>
> - /* Get record_full_core_regbuf. */
> + /* Get record_full_core_regcache. */
> target_fetch_registers (regcache, -1);
> - record_full_core_regbuf = (gdb_byte *) xmalloc (MAX_REGISTER_SIZE * regnum);
> - for (i = 0; i < regnum; i ++)
> - regcache_raw_collect (regcache, i,
> - record_full_core_regbuf + MAX_REGISTER_SIZE * i);
> + record_full_core_regcache = regcache_dup (regcache);
>
> /* Get record_full_core_start and record_full_core_end. */
> if (build_section_table (core_bfd, &record_full_core_start,
> &record_full_core_end))
> {
> - xfree (record_full_core_regbuf);
> - record_full_core_regbuf = NULL;
> + regcache_xfree (record_full_core_regcache);
> + record_full_core_regcache = NULL;
> error (_("\"%s\": Can't find sections: %s"),
> bfd_get_filename (core_bfd), bfd_errmsg (bfd_get_error ()));
> }
> @@ -886,11 +881,11 @@ record_full_close (struct target_ops *self)
>
> record_full_list_release (record_full_list);
>
> - /* Release record_full_core_regbuf. */
> - if (record_full_core_regbuf)
> + /* Release record_full_core_regcache. */
> + if (record_full_core_regcache)
> {
> - xfree (record_full_core_regbuf);
> - record_full_core_regbuf = NULL;
> + regcache_xfree (record_full_core_regcache);
> + record_full_core_regcache = NULL;
> }
>
> /* Release record_full_core_buf_list. */
> @@ -2051,15 +2046,12 @@ record_full_core_fetch_registers (struct target_ops *ops,
> if (regno < 0)
> {
> int num = gdbarch_num_regs (get_regcache_arch (regcache));
> - int i;
>
> - for (i = 0; i < num; i ++)
> - regcache_raw_supply (regcache, i,
> - record_full_core_regbuf + MAX_REGISTER_SIZE * i);
> + for (int i = 0; i < num; i ++)
> + regcache->raw_copy (i, record_full_core_regcache);
> }
> else
> - regcache_raw_supply (regcache, regno,
> - record_full_core_regbuf + MAX_REGISTER_SIZE * regno);
> + regcache->raw_copy (regno, record_full_core_regcache);
> }
>
> /* "to_prepare_to_store" method for prec over corefile. */
> @@ -2078,8 +2070,7 @@ record_full_core_store_registers (struct target_ops *ops,
> int regno)
> {
> if (record_full_gdb_operation_disable)
> - regcache_raw_collect (regcache, regno,
> - record_full_core_regbuf + MAX_REGISTER_SIZE * regno);
> + record_full_core_regcache->raw_copy (regno, regcache);
> else
> error (_("You can't do that without a process to debug."));
> }
> diff --git a/gdb/regcache.h b/gdb/regcache.h
> index 00b729d9af1b9d9823fc84fb424047c2b918ec8d..24257d8eafd4f7168b622a48ec1e646b9027153e 100644
> --- a/gdb/regcache.h
> +++ b/gdb/regcache.h
> @@ -324,6 +324,8 @@ public:
>
> void raw_supply_zeroed (int regnum);
>
> + void raw_copy (int regnum, const struct regcache *src_regcache);
> +
> enum register_status get_register_status (int regnum) const;
>
> void raw_set_cached_value (int regnum, const gdb_byte *buf);
> @@ -402,8 +404,8 @@ private:
> /* Is this a read-only cache? A read-only cache is used for saving
> the target's register state (e.g, across an inferior function
> call or just before forcing a function return). A read-only
> - cache can only be updated via the methods regcache_dup() and
> - regcache_cpy(). The actual contents are determined by the
> + cache can only be updated via the methods regcache_dup (), regcache_cpy ()
> + and regcache::raw_copy (). The actual contents are determined by the
> reggroup_save and reggroup_restore methods. */
> bool m_readonly_p;
> /* If this is a read-write cache, which thread's registers is
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 4e8c1ee0e648fd17ce2a120c61ecd495ff2e2467..54454e3f039ff5d6ab436cd9e0f1da607476ce32 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -1256,6 +1256,20 @@ regcache::raw_collect (int regnum, void *buf) const
> memcpy (buf, regbuf, size);
> }
>
> +/* Collect register REGNUM from SRC_REGCACHE and store its contents in
> + REGCACHE. Regcaches must have matching gdbarches. */
> +
> +void
> +regcache::raw_copy (int regnum, const struct regcache *src_regcache)
> +{
> + gdb_assert (src_regcache != NULL);
> + gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
> + gdb_assert (m_descr->gdbarch == src_regcache->m_descr->gdbarch);
> +
> + src_regcache->raw_collect (regnum, register_buffer (regnum));
> + m_register_status[regnum] = src_regcache->m_register_status[regnum];
> +}
> +
> /* Transfer a single or all registers belonging to a certain register
> set to or from a buffer. This is the main worker function for
> regcache_supply_regset and regcache_collect_regset. */
>
Alan Hayward <Alan.Hayward@arm.com> writes:
> In record-full.c, instead of backing up all the registers into a large
> buffer, duplicate the regcache.
> This enables the removal of an instance of MAX_REGISTER_SIZE.
>
> Note that regcache_dup() create a read-only copy of a register cache,
> which ensures the new regcache cannot write back to the target.
>
> Once created, we need to be able to copy registers between the two caches,
> which we want to do without creating a temporary buffer.
>
> I've added regcache::raw_copy() to allow the copying of raw registers
> between two regcaches - either of which might be set as read-only.
Can we name this method raw_supply?
>
> Alternatively, I could make the new regcache as writable (by enabling a
> regcache copy constructor). But, I think this would be dangerous as it
> it then has the potential to write back to the target if the wrong function
> is called.
regcache only interacts with target through ::raw_update and
::raw_write. Can we have a regcache class without raw_update and
raw_write? regcache has two set of methods, {raw,cooked}_{read,write}
and raw_{collect,supply}XXX. The former interacts with target, but the
latter doesn't. We can create a new class regcache_collect_supply which
has methods raw_{collect,supply}XXXX, regcache extends it. Then, add a
method "void raw_supply (int regnum, const regcache_collect_supply &src)"
and change record_full_core_regbuf to a regcache_collect_supply. The
interface looks like this, (regcache_collect_supply is regcache_1 in the
doxygen doc, because I didn't figure out a reasonable name yesterday),
http://people.linaro.org/~yao.qi/gdb/doxy/regcache-split/gdb-xref/classregcache.html
Do you like this design? One more thing is that the new class
regcache_collect_supply can be used in target_ops hooks
to_fetch_registers, to_store_registers, to_prepare_to_store,
void (*to_fetch_registers) (struct target_ops *, regcache_collect_supply *, int)
TARGET_DEFAULT_IGNORE ();
void (*to_store_registers) (struct target_ops *, regcache_collect_supply *, int)
TARGET_DEFAULT_NORETURN (noprocess ());
void (*to_prepare_to_store) (struct target_ops *, regcache_collect_supply *)
TARGET_DEFAULT_NORETURN (noprocess ());
so that all the implementations of these methods above can only access
raw_{collect,supply}XX methods, and they can't access
{raw,cooked}_{read,write}XXX methods, which in turn may call target_ops
to_fetch_registers and to_store_registers again.
> On 22 Jun 2017, at 09:44, Yao Qi <qiyaoltc@gmail.com> wrote:
>
> Alan Hayward <Alan.Hayward@arm.com> writes:
>
>> In record-full.c, instead of backing up all the registers into a large
>> buffer, duplicate the regcache.
>> This enables the removal of an instance of MAX_REGISTER_SIZE.
>>
>> Note that regcache_dup() create a read-only copy of a register cache,
>> which ensures the new regcache cannot write back to the target.
>>
>> Once created, we need to be able to copy registers between the two caches,
>> which we want to do without creating a temporary buffer.
>>
>> I've added regcache::raw_copy() to allow the copying of raw registers
>> between two regcaches - either of which might be set as read-only.
>
> Can we name this method raw_supply?
Resulting in:
void raw_supply (int regnum, const void *buf);
void raw_supply (int regnum, const struct regcache *src_regcache);
Happy to do that.
>
>>
>> Alternatively, I could make the new regcache as writable (by enabling a
>> regcache copy constructor). But, I think this would be dangerous as it
>> it then has the potential to write back to the target if the wrong function
>> is called.
>
> regcache only interacts with target through ::raw_update and
> ::raw_write. Can we have a regcache class without raw_update and
> raw_write? regcache has two set of methods, {raw,cooked}_{read,write}
> and raw_{collect,supply}XXX. The former interacts with target, but the
> latter doesn't. We can create a new class regcache_collect_supply which
> has methods raw_{collect,supply}XXXX, regcache extends it. Then, add a
> method "void raw_supply (int regnum, const regcache_collect_supply &src)"
> and change record_full_core_regbuf to a regcache_collect_supply. The
> interface looks like this, (regcache_collect_supply is regcache_1 in the
> doxygen doc, because I didn't figure out a reasonable name yesterday),
detached_regcache ?
virtual_regcache ? (Not as keen on that)
>
> http://people.linaro.org/~yao.qi/gdb/doxy/regcache-split/gdb-xref/classregcache.html
>
> Do you like this design? One more thing is that the new class
> regcache_collect_supply can be used in target_ops hooks
> to_fetch_registers, to_store_registers, to_prepare_to_store,
>
> void (*to_fetch_registers) (struct target_ops *, regcache_collect_supply *, int)
> TARGET_DEFAULT_IGNORE ();
> void (*to_store_registers) (struct target_ops *, regcache_collect_supply *, int)
> TARGET_DEFAULT_NORETURN (noprocess ());
> void (*to_prepare_to_store) (struct target_ops *, regcache_collect_supply *)
> TARGET_DEFAULT_NORETURN (noprocess ());
>
> so that all the implementations of these methods above can only access
> raw_{collect,supply}XX methods, and they can't access
> {raw,cooked}_{read,write}XXX methods, which in turn may call target_ops
> to_fetch_registers and to_store_registers again.
>
Following this through, regcache_dup and regcache_cpy should create a detached_regcache ?
Code would be free to write to the detached_regcache and there would be no need to mark
it as “read-only”.
Would there be any requirement to still have a read-only regcache ?
Alan.
Alan Hayward <Alan.Hayward@arm.com> writes:
> Resulting in:
> void raw_supply (int regnum, const void *buf);
> void raw_supply (int regnum, const struct regcache *src_regcache);
^^^^^^^^^^^^^^^^^^^^^^^^
We should use reference instead of pointer, otherwise, raw_supply (0, NULL)
is ambiguous.
>>
>> http://people.linaro.org/~yao.qi/gdb/doxy/regcache-split/gdb-xref/classregcache.html
>>
>> Do you like this design? One more thing is that the new class
>> regcache_collect_supply can be used in target_ops hooks
>> to_fetch_registers, to_store_registers, to_prepare_to_store,
>>
>> void (*to_fetch_registers) (struct target_ops *, regcache_collect_supply *, int)
>> TARGET_DEFAULT_IGNORE ();
>> void (*to_store_registers) (struct target_ops *, regcache_collect_supply *, int)
>> TARGET_DEFAULT_NORETURN (noprocess ());
>> void (*to_prepare_to_store) (struct target_ops *, regcache_collect_supply *)
>> TARGET_DEFAULT_NORETURN (noprocess ());
>>
>> so that all the implementations of these methods above can only access
>> raw_{collect,supply}XX methods, and they can't access
>> {raw,cooked}_{read,write}XXX methods, which in turn may call target_ops
>> to_fetch_registers and to_store_registers again.
>>
>
> Following this through, regcache_dup and regcache_cpy should create a
> detached_regcache ?
>
Yes, it can be.
> Code would be free to write to the detached_regcache and there would
> be no need to mark
> it as “read-only”.
No.
>
> Would there be any requirement to still have a read-only regcache ?
The detached_regcache can be read-write and read-only. regcache_dup and
regcache_cpy create the read-only one, but record-full still uses the
read-write one.
>>
>>>
>>> As I understand it, the cooked registers exist because on some architectures
>>> extra state needs saving in the cooked registers (code comment: "some
>>> architectures
>>> need to save/restore `cooked registers that live in memory.”).
>>>
>>> Therefore the cooked register state needs to be a property of detached
>>> and not of
>>> readonly.
>>>
>>
>> m_registers and m_register_status are fields of detached regcache, we
>> can definitely save cooked register state in detached regcache.
>>
>>>
>>> A different issue is that we treat save/restore differently.
>>> In your code one of the recaches has to be both read-only (checking
>>> via gdb_assert) and detached.
>>> In my code the check is that the regcache is detached or
>>> not. Read-only is not relevant.
>>
>> It is read-only in my code, but it doesn't have to be. I don't see any
>> show-stoppers in the design of splitting regcache. The attributes
>> "detached" and "read-only" are orthogonal in design. Do you have some
>> comments on the overall design rather than the code details? I'll
>> rewrite my patches, and post them. It is unfortunate that it is hard to
>> review the overall design without the code.
>
I’ve taken a look at implementing the split regcache and have run into an
interesting issue.
frame.c, infcmd.c, infrun.c, jit.c, linux-fork.c, mi/mi-main.c
These files all duplicate the register cache.
In the new patch, this will a create detached_regcache.
These files then access this copied (detached) regcache using:
regcache_cooked_read
gdbarch_pseudo_register_read_value
gdbarch_return_value
regcache_read_pc
It’s easy enough to replace the cooked_read with a raw_collect.
However, the other three are a lot trickier.
They all call out to target functions, (….which call out to other functions…)
which all make multiple calls to cooked_read and raw_read.
The target functions can’t be updated to use raw_collect, because in other case
they will be used with the attached regcache, which will be expecting reads to be
pulled from the target.
Creating duplicate functions (e.g. gdbarch_detached_return_value) would be silly.
The only solution I can see is to create virtual cooked_read and raw_read in
detached_regcache. These functions just read from the cache instead of
updating from the target.
In this new world, calling cooked_read or raw_read on either a detached_regache
or attached regcache would do the correct thing.
(The function prototypes in all the target files still need updating, but that is a fairly
simple copy/paste.)
Does this approach seem sensible?
If there are no objections, I’ll carry on working the patch using this approach.
I can post a work in progress patch if required (but that might be better to wait until
I have something fully working and split into smaller changes).
Alan.
@@ -167,7 +167,7 @@ struct record_full_core_buf_entry
};
/* Record buf with core target. */
-static gdb_byte *record_full_core_regbuf = NULL;
+static struct regcache *record_full_core_regcache = NULL;
static struct target_section *record_full_core_start;
static struct target_section *record_full_core_end;
static struct record_full_core_buf_entry *record_full_core_buf_list = NULL;
@@ -793,22 +793,17 @@ static void
record_full_core_open_1 (const char *name, int from_tty)
{
struct regcache *regcache = get_current_regcache ();
- int regnum = gdbarch_num_regs (get_regcache_arch (regcache));
- int i;
- /* Get record_full_core_regbuf. */
+ /* Get record_full_core_regcache. */
target_fetch_registers (regcache, -1);
- record_full_core_regbuf = (gdb_byte *) xmalloc (MAX_REGISTER_SIZE * regnum);
- for (i = 0; i < regnum; i ++)
- regcache_raw_collect (regcache, i,
- record_full_core_regbuf + MAX_REGISTER_SIZE * i);
+ record_full_core_regcache = regcache_dup (regcache);
/* Get record_full_core_start and record_full_core_end. */
if (build_section_table (core_bfd, &record_full_core_start,
&record_full_core_end))
{
- xfree (record_full_core_regbuf);
- record_full_core_regbuf = NULL;
+ regcache_xfree (record_full_core_regcache);
+ record_full_core_regcache = NULL;
error (_("\"%s\": Can't find sections: %s"),
bfd_get_filename (core_bfd), bfd_errmsg (bfd_get_error ()));
}
@@ -886,11 +881,11 @@ record_full_close (struct target_ops *self)
record_full_list_release (record_full_list);
- /* Release record_full_core_regbuf. */
- if (record_full_core_regbuf)
+ /* Release record_full_core_regcache. */
+ if (record_full_core_regcache)
{
- xfree (record_full_core_regbuf);
- record_full_core_regbuf = NULL;
+ regcache_xfree (record_full_core_regcache);
+ record_full_core_regcache = NULL;
}
/* Release record_full_core_buf_list. */
@@ -2051,15 +2046,12 @@ record_full_core_fetch_registers (struct target_ops *ops,
if (regno < 0)
{
int num = gdbarch_num_regs (get_regcache_arch (regcache));
- int i;
- for (i = 0; i < num; i ++)
- regcache_raw_supply (regcache, i,
- record_full_core_regbuf + MAX_REGISTER_SIZE * i);
+ for (int i = 0; i < num; i ++)
+ regcache->raw_copy (i, record_full_core_regcache);
}
else
- regcache_raw_supply (regcache, regno,
- record_full_core_regbuf + MAX_REGISTER_SIZE * regno);
+ regcache->raw_copy (regno, record_full_core_regcache);
}
/* "to_prepare_to_store" method for prec over corefile. */
@@ -2078,8 +2070,7 @@ record_full_core_store_registers (struct target_ops *ops,
int regno)
{
if (record_full_gdb_operation_disable)
- regcache_raw_collect (regcache, regno,
- record_full_core_regbuf + MAX_REGISTER_SIZE * regno);
+ record_full_core_regcache->raw_copy (regno, regcache);
else
error (_("You can't do that without a process to debug."));
}
@@ -324,6 +324,8 @@ public:
void raw_supply_zeroed (int regnum);
+ void raw_copy (int regnum, const struct regcache *src_regcache);
+
enum register_status get_register_status (int regnum) const;
void raw_set_cached_value (int regnum, const gdb_byte *buf);
@@ -402,8 +404,8 @@ private:
/* Is this a read-only cache? A read-only cache is used for saving
the target's register state (e.g, across an inferior function
call or just before forcing a function return). A read-only
- cache can only be updated via the methods regcache_dup() and
- regcache_cpy(). The actual contents are determined by the
+ cache can only be updated via the methods regcache_dup (), regcache_cpy ()
+ and regcache::raw_copy (). The actual contents are determined by the
reggroup_save and reggroup_restore methods. */
bool m_readonly_p;
/* If this is a read-write cache, which thread's registers is
@@ -1256,6 +1256,20 @@ regcache::raw_collect (int regnum, void *buf) const
memcpy (buf, regbuf, size);
}
+/* Collect register REGNUM from SRC_REGCACHE and store its contents in
+ REGCACHE. Regcaches must have matching gdbarches. */
+
+void
+regcache::raw_copy (int regnum, const struct regcache *src_regcache)
+{
+ gdb_assert (src_regcache != NULL);
+ gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
+ gdb_assert (m_descr->gdbarch == src_regcache->m_descr->gdbarch);
+
+ src_regcache->raw_collect (regnum, register_buffer (regnum));
+ m_register_status[regnum] = src_regcache->m_register_status[regnum];
+}
+
/* Transfer a single or all registers belonging to a certain register
set to or from a buffer. This is the main worker function for
regcache_supply_regset and regcache_collect_regset. */