[10/15] Class regcache_readonly
Commit Message
This patch adds a new class (type) for readonly regcache, which is
created via regcache::save. regcache_readonly inherts from
regcache_read.
gdb:
2017-11-28 Yao Qi <yao.qi@linaro.org>
* dummy-frame.c (dummy_frame_cache) <prev_regcache>: Use
regcache_readonly.
(dummy_frame_prev_register): Use regcache->cooked_read.
* frame.c (frame_save_as_regcache): Change return type.
(frame_pop): Update.
* frame.h (frame_save_as_regcache): Update declaration.
* inferior.h (get_infcall_suspend_state_regcache): Update
declaration.
* infrun.c (infcall_suspend_state) <registers>: use
regcache_readonly.
(save_infcall_suspend_state): Don't use regcache_dup.
(get_infcall_suspend_state_regcache): Change return type.
* linux-fork.c (struct fork_info) <savedregs>: Change to
regcache_readonly.
<pc>: New field.
(fork_save_infrun_state): Don't use regcache_dup.
(info_checkpoints_command): Adjust.
* mi/mi-main.c (register_changed_p): Update declaration.
(mi_cmd_data_list_changed_registers): Use regcache_readonly.
(register_changed_p): Change parameter type to regcache_readonly.
* ppc-linux-tdep.c (ppu2spu_cache) <regcache>: Use
regcache_readonly.
(ppu2spu_sniffer): Construct a new regcache_readonly.
* regcache.c (regcache_readonly::regcache_readonly): New.
(regcache::save): Move it to reg_buffer.
(regcache::restore): Change parameter type.
(regcache_dup): Remove.
* regcache.h (reg_buffer) <save>: New method.
(regcache_readonly): New class.
* spu-tdep.c (spu2ppu_cache) <regcache>: Use regcache_readonly.
(spu2ppu_sniffer): Construct a new regcache_readonly.
---
gdb/dummy-frame.c | 6 +++---
gdb/frame.c | 10 +++++-----
gdb/frame.h | 3 ++-
gdb/inferior.h | 2 +-
gdb/infrun.c | 6 +++---
gdb/linux-fork.c | 18 ++++++++----------
gdb/mi/mi-main.c | 12 ++++++------
gdb/ppc-linux-tdep.c | 9 ++++-----
gdb/regcache.c | 27 +++++++++++----------------
gdb/regcache.h | 43 +++++++++++++++++++++++++++++++++----------
gdb/spu-tdep.c | 6 +++---
11 files changed, 79 insertions(+), 63 deletions(-)
Comments
On 2017-12-01 05:48 AM, Yao Qi wrote:
> This patch adds a new class (type) for readonly regcache, which is
> created via regcache::save. regcache_readonly inherts from
> regcache_read.
Hi Yao,
Just a note about the naming. IIUC, the important thing about this
kind of regcache is that it's detached from any target. Did you
think about naming it detached_regcache or something like that ?
Simon
On Wed, Jan 24, 2018 at 3:05 AM, Simon Marchi <simon.marchi@ericsson.com> wrote:
> On 2017-12-01 05:48 AM, Yao Qi wrote:
>> This patch adds a new class (type) for readonly regcache, which is
>> created via regcache::save. regcache_readonly inherts from
>> regcache_read.
>
> Hi Yao,
>
> Just a note about the naming. IIUC, the important thing about this
> kind of regcache is that it's detached from any target. Did you
> think about naming it detached_regcache or something like that ?
>
This kind of regcache is both detached and readonly. As I said in
https://sourceware.org/ml/gdb-patches/2017-07/msg00031.html,
detached and readonly are orthogonal in design. We have four
different kinds of regcache,
- readony detached regcache, this is what "class regcache_readonly"
about. It has several uses, 1) record infcall state, 2) give a
regcache view to frame,
- read-write detached regcache, this is what "class reg_buffer_rw"
about. It is used jit.c and record-full.c, where GDB keeps a detached
regcache, but can read and write to it.
- read-write attached regcache, that is what "class regcache" about. It
is attached to target, read and write will go through target,
- readonly attached regcache. It can be used for target 'core', but this
piece is not included in this series,
so in this patch series, "readonly" implies "detached".
The major motivation of this patch series is to differentiate these kinds
of regcache by different types, instead of by fields "m_readonly_p" or
"m_detached_p" in "class regcache".
On 2018-01-24 04:42 AM, Yao Qi wrote:
> On Wed, Jan 24, 2018 at 3:05 AM, Simon Marchi <simon.marchi@ericsson.com> wrote:
>> On 2017-12-01 05:48 AM, Yao Qi wrote:
>>> This patch adds a new class (type) for readonly regcache, which is
>>> created via regcache::save. regcache_readonly inherts from
>>> regcache_read.
>>
>> Hi Yao,
>>
>> Just a note about the naming. IIUC, the important thing about this
>> kind of regcache is that it's detached from any target. Did you
>> think about naming it detached_regcache or something like that ?
>>
>
> This kind of regcache is both detached and readonly. As I said in
> https://sourceware.org/ml/gdb-patches/2017-07/msg00031.html,
> detached and readonly are orthogonal in design. We have four
> different kinds of regcache,
>
> - readony detached regcache, this is what "class regcache_readonly"
> about. It has several uses, 1) record infcall state, 2) give a
> regcache view to frame,
>
> - read-write detached regcache, this is what "class reg_buffer_rw"
> about. It is used jit.c and record-full.c, where GDB keeps a detached
> regcache, but can read and write to it.
>
> - read-write attached regcache, that is what "class regcache" about. It
> is attached to target, read and write will go through target,
>
> - readonly attached regcache. It can be used for target 'core', but this
> piece is not included in this series,
>
> so in this patch series, "readonly" implies "detached".
>
> The major motivation of this patch series is to differentiate these kinds
> of regcache by different types, instead of by fields "m_readonly_p" or
> "m_detached_p" in "class regcache".
Just pitching some ideas, I don't think I understand the situation as well as
you do.
I assume we want to keep the "regcache" type to mean read/write and attached,
since that's the most common use case. Keeping this will reduce the amount of
changes needed throughout the code base. We can then qualify the other types
based on how they differ from "read/write" and "attached". That would give us
(in the same order as your list above):
- readonly_detached_regcache
- detached_regcache
- regcache
- readonly_regcache
This would give a predictable naming, and makes it maybe easier to know what
to expect from each type. The graph you used in message 0/15 would become:
reg_buffer
^
|
------+-----
^
|
readable_regcache (abstract)
^
|
------+------
^ ^
| |
detached_regcache readonly_detached_regcache
^
|
regcache
Simon
On Wed, Jan 24, 2018 at 4:57 PM, Simon Marchi <simon.marchi@ericsson.com> wrote:
>
> Just pitching some ideas, I don't think I understand the situation as well as
> you do.
>
> I assume we want to keep the "regcache" type to mean read/write and attached,
> since that's the most common use case. Keeping this will reduce the amount of
> changes needed throughout the code base. We can then qualify the other types
Yes.
> based on how they differ from "read/write" and "attached". That would give us
> (in the same order as your list above):
>
> - readonly_detached_regcache
> - detached_regcache
> - regcache
> - readonly_regcache
This should be readonly_attached_regcache.
>
> This would give a predictable naming, and makes it maybe easier to know what
> to expect from each type. The graph you used in message 0/15 would become:
>
> reg_buffer
> ^
> |
> ------+-----
> ^
> |
> readable_regcache (abstract)
> ^
> |
> ------+------
> ^ ^
> | |
> detached_regcache readonly_detached_regcache
> ^
> |
> regcache
>
This naming is fine to me except for readonly_detached_regcache
as it is too long. As "readonly" implies "detached" in current context,
can we name it readonly_regcache?
On 2018-01-24 12:37, Yao Qi wrote:
> On Wed, Jan 24, 2018 at 4:57 PM, Simon Marchi
> <simon.marchi@ericsson.com> wrote:
>>
>> Just pitching some ideas, I don't think I understand the situation as
>> well as
>> you do.
>>
>> I assume we want to keep the "regcache" type to mean read/write and
>> attached,
>> since that's the most common use case. Keeping this will reduce the
>> amount of
>> changes needed throughout the code base. We can then qualify the
>> other types
>
> Yes.
>
>> based on how they differ from "read/write" and "attached". That would
>> give us
>> (in the same order as your list above):
>>
>> - readonly_detached_regcache
>> - detached_regcache
>> - regcache
>> - readonly_regcache
>
> This should be readonly_attached_regcache.
The logic was that by "default" a regcache would be attached, unless
specified otherwise (because that's what the plain regcache is). So
that's why I suggested qualifying the detached regcache as such, while
the attached ones would be implicit.
>>
>> This would give a predictable naming, and makes it maybe easier to
>> know what
>> to expect from each type. The graph you used in message 0/15 would
>> become:
>>
>> reg_buffer
>> ^
>> |
>> ------+-----
>> ^
>> |
>> readable_regcache (abstract)
>> ^
>> |
>> ------+------
>> ^ ^
>> | |
>> detached_regcache readonly_detached_regcache
>> ^
>> |
>> regcache
>>
>
> This naming is fine to me except for readonly_detached_regcache
> as it is too long. As "readonly" implies "detached" in current
> context,
> can we name it readonly_regcache?
In this context readonly == detached, but aren't we going to want
readonly attached regcaches at some point? If so, there will be a
clash.
Simon
On Wed, Jan 24, 2018 at 6:00 PM, Simon Marchi <simon.marchi@polymtl.ca> wrote:
>
> In this context readonly == detached, but aren't we going to want readonly
> attached regcaches at some point? If so, there will be a clash.
>
OK, I'll use readonly_detached_regcache.
@@ -282,7 +282,7 @@ cleanup_dummy_frames (struct target_ops *target, int from_tty)
struct dummy_frame_cache
{
struct frame_id this_id;
- struct regcache *prev_regcache;
+ regcache_readonly *prev_regcache;
};
static int
@@ -352,8 +352,8 @@ dummy_frame_prev_register (struct frame_info *this_frame,
/* Use the regcache_cooked_read() method so that it, on the fly,
constructs either a raw or pseudo register from the raw
register cache. */
- regcache_cooked_read (cache->prev_regcache, regnum,
- value_contents_writeable (reg_val));
+ cache->prev_regcache->cooked_read (regnum,
+ value_contents_writeable (reg_val));
return reg_val;
}
@@ -1017,13 +1017,13 @@ do_frame_register_read (void *src, int regnum, gdb_byte *buf)
return REG_VALID;
}
-std::unique_ptr<struct regcache>
+std::unique_ptr<regcache_readonly>
frame_save_as_regcache (struct frame_info *this_frame)
{
- std::unique_ptr<struct regcache> regcache
- (new struct regcache (get_frame_arch (this_frame)));
+ std::unique_ptr<regcache_readonly> regcache
+ (new regcache_readonly (get_frame_arch (this_frame),
+ do_frame_register_read, this_frame));
- regcache->save (do_frame_register_read, this_frame);
return regcache;
}
@@ -1057,7 +1057,7 @@ frame_pop (struct frame_info *this_frame)
Save them in a scratch buffer so that there isn't a race between
trying to extract the old values from the current regcache while
at the same time writing new values into that same cache. */
- std::unique_ptr<struct regcache> scratch
+ std::unique_ptr<regcache_readonly> scratch
= frame_save_as_regcache (prev_frame);
/* FIXME: cagney/2003-03-16: It should be possible to tell the
@@ -680,8 +680,9 @@ extern void *frame_obstack_zalloc (unsigned long size);
#define FRAME_OBSTACK_CALLOC(NUMBER,TYPE) \
((TYPE *) frame_obstack_zalloc ((NUMBER) * sizeof (TYPE)))
+class regcache_readonly;
/* Create a regcache, and copy the frame's registers into it. */
-std::unique_ptr<struct regcache> frame_save_as_regcache
+std::unique_ptr<regcache_readonly> frame_save_as_regcache
(struct frame_info *this_frame);
extern const struct block *get_frame_block (struct frame_info *,
@@ -70,7 +70,7 @@ extern struct cleanup *make_cleanup_restore_infcall_control_state
extern void discard_infcall_suspend_state (struct infcall_suspend_state *);
extern void discard_infcall_control_state (struct infcall_control_state *);
-extern struct regcache *
+extern regcache_readonly *
get_infcall_suspend_state_regcache (struct infcall_suspend_state *);
extern void set_sigint_trap (void);
@@ -8780,7 +8780,7 @@ struct infcall_suspend_state
/* Other fields: */
CORE_ADDR stop_pc;
- struct regcache *registers;
+ regcache_readonly *registers;
/* Format of SIGINFO_DATA or NULL if it is not present. */
struct gdbarch *siginfo_gdbarch;
@@ -8836,7 +8836,7 @@ save_infcall_suspend_state (void)
inf_state->stop_pc = stop_pc;
- inf_state->registers = regcache_dup (regcache);
+ inf_state->registers = new regcache_readonly (*regcache);
return inf_state;
}
@@ -8893,7 +8893,7 @@ discard_infcall_suspend_state (struct infcall_suspend_state *inf_state)
xfree (inf_state);
}
-struct regcache *
+regcache_readonly *
get_infcall_suspend_state_regcache (struct infcall_suspend_state *inf_state)
{
return inf_state->registers;
@@ -45,8 +45,9 @@ struct fork_info
ptid_t ptid;
ptid_t parent_ptid;
int num; /* Convenient handle (GDB fork id). */
- struct regcache *savedregs; /* Convenient for info fork, saves
+ regcache_readonly *savedregs; /* Convenient for info fork, saves
having to actually switch contexts. */
+ CORE_ADDR pc;
int clobber_regs; /* True if we should restore saved regs. */
off_t *filepos; /* Set of open file descriptors' offsets. */
int maxfd;
@@ -294,7 +295,8 @@ fork_save_infrun_state (struct fork_info *fp, int clobber_regs)
if (fp->savedregs)
delete fp->savedregs;
- fp->savedregs = regcache_dup (get_current_regcache ());
+ fp->savedregs = new regcache_readonly (*get_current_regcache ());
+ fp->pc = regcache_read_pc (get_current_regcache ());
fp->clobber_regs = clobber_regs;
if (clobber_regs)
@@ -590,15 +592,11 @@ info_checkpoints_command (const char *arg, int from_tty)
printed = fp;
if (ptid_equal (fp->ptid, inferior_ptid))
- {
- printf_filtered ("* ");
- pc = regcache_read_pc (get_current_regcache ());
- }
+ printf_filtered ("* ");
else
- {
- printf_filtered (" ");
- pc = regcache_read_pc (fp->savedregs);
- }
+ printf_filtered (" ");
+
+ pc = fp->pc;
printf_filtered ("%d %s", fp->num, target_pid_to_str (fp->ptid));
if (fp->num == 0)
printf_filtered (_(" (main process)"));
@@ -96,8 +96,8 @@ static void mi_execute_cli_command (const char *cmd, int args_p,
const char *args);
static void mi_execute_async_cli_command (const char *cli_command,
char **argv, int argc);
-static bool register_changed_p (int regnum, regcache *,
- regcache *);
+static bool register_changed_p (int regnum, regcache_readonly *,
+ regcache_readonly *);
static void output_register (struct frame_info *, int regnum, int format,
int skip_unavailable);
@@ -931,9 +931,9 @@ mi_cmd_data_list_register_names (const char *command, char **argv, int argc)
void
mi_cmd_data_list_changed_registers (const char *command, char **argv, int argc)
{
- static std::unique_ptr<struct regcache> this_regs;
+ static std::unique_ptr<regcache_readonly> this_regs;
struct ui_out *uiout = current_uiout;
- std::unique_ptr<struct regcache> prev_regs;
+ std::unique_ptr<regcache_readonly> prev_regs;
struct gdbarch *gdbarch;
int regnum, numregs;
int i;
@@ -995,8 +995,8 @@ mi_cmd_data_list_changed_registers (const char *command, char **argv, int argc)
}
static bool
-register_changed_p (int regnum, struct regcache *prev_regs,
- struct regcache *this_regs)
+register_changed_p (int regnum, regcache_readonly *prev_regs,
+ regcache_readonly *this_regs)
{
struct gdbarch *gdbarch = this_regs->arch ();
struct value *prev_value, *this_value;
@@ -1241,7 +1241,7 @@ ppc_linux_spe_context (int wordsize, enum bfd_endian byte_order,
struct ppu2spu_cache
{
struct frame_id frame_id;
- struct regcache *regcache;
+ regcache_readonly *regcache;
};
static struct gdbarch *
@@ -1348,10 +1348,9 @@ ppu2spu_sniffer (const struct frame_unwind *self,
{
struct ppu2spu_cache *cache
= FRAME_OBSTACK_CALLOC (1, struct ppu2spu_cache);
- std::unique_ptr<struct regcache> regcache
- (new struct regcache (data.gdbarch));
-
- regcache->save (ppu2spu_unwind_register, &data);
+ std::unique_ptr<regcache_readonly> regcache
+ (new regcache_readonly (data.gdbarch, ppu2spu_unwind_register,
+ &data));
cache->frame_id = frame_id_build (base, func);
cache->regcache = regcache.release ();
@@ -226,6 +226,11 @@ regcache::regcache (readonly_t, const regcache &src)
save (do_cooked_read, (void *) &src);
}
+regcache_readonly::regcache_readonly (const regcache &src)
+ : regcache_readonly (src.arch (), do_cooked_read, (void *) &src)
+{
+}
+
gdbarch *
reg_buffer::arch () const
{
@@ -282,16 +287,14 @@ reg_buffer::register_buffer (int regnum) const
}
void
-regcache::save (regcache_cooked_read_ftype *cooked_read,
- void *src)
+reg_buffer::save (regcache_cooked_read_ftype *cooked_read,
+ void *src)
{
struct gdbarch *gdbarch = m_descr->gdbarch;
int regnum;
- /* The DST should be `read-only', if it wasn't then the save would
- end up trying to write the register values back out to the
- target. */
- gdb_assert (m_readonly_p);
+ /* It should have pseudo registers. */
+ gdb_assert (m_has_pseudo);
/* Clear the dest. */
memset (m_registers, 0, m_descr->sizeof_cooked_registers);
memset (m_register_status, 0, m_descr->nr_cooked_registers);
@@ -317,16 +320,14 @@ regcache::save (regcache_cooked_read_ftype *cooked_read,
}
void
-regcache::restore (struct regcache *src)
+regcache::restore (regcache_readonly *src)
{
struct gdbarch *gdbarch = m_descr->gdbarch;
int regnum;
gdb_assert (src != NULL);
- /* The dst had better not be read-only. If it is, the `restore'
- doesn't make much sense. */
gdb_assert (!m_readonly_p);
- gdb_assert (src->m_readonly_p);
+ gdb_assert (src->m_has_pseudo);
gdb_assert (gdbarch == src->arch ());
@@ -344,12 +345,6 @@ regcache::restore (struct regcache *src)
}
}
-struct regcache *
-regcache_dup (struct regcache *src)
-{
- return new regcache (regcache::readonly, *src);
-}
-
enum register_status
regcache_register_status (const struct regcache *regcache, int regnum)
{
@@ -243,6 +243,11 @@ protected:
gdb_byte *register_buffer (int regnum) const;
+ /* Save a register cache. The set of registers saved into the
+ regcache determined by the save_reggroup. COOKED_READ returns
+ zero iff the register's value can't be returned. */
+ void save (regcache_cooked_read_ftype *cooked_read, void *src);
+
struct regcache_descr *m_descr;
bool m_has_pseudo;
@@ -250,6 +255,8 @@ protected:
gdb_byte *m_registers;
/* Register cache status. */
signed char *m_register_status;
+
+ friend class regcache;
};
class regcache_read : public reg_buffer
@@ -282,6 +289,8 @@ protected:
bool is_raw);
};
+class regcache_readonly;
+
/* The register cache for storing raw register values. */
class regcache : public regcache_read
@@ -305,14 +314,11 @@ public:
return m_aspace;
}
-/* Save/restore a register cache. The set of registers saved /
- restored into the regcache determined by the save_reggroup /
- restore_reggroup respectively. COOKED_READ returns zero iff the
- register's value can't be returned. */
- void save (regcache_cooked_read_ftype *cooked_read, void *src);
- /* Writes to regcache will go through to the target. SRC is a
+ /* Restore a register cache. The set of registers restored into
+ the regcache determined by the restore_reggroup.
+ Writes to regcache will go through to the target. SRC is a
read-only register cache. */
- void restore (struct regcache *src);
+ void restore (regcache_readonly *src);
void cooked_write (int regnum, const gdb_byte *buf);
@@ -410,9 +416,26 @@ private:
registers_changed_ptid (ptid_t ptid);
};
-/* Duplicate the contents of a register cache to a read-only register
- cache. The operation is pass-through. */
-extern struct regcache *regcache_dup (struct regcache *regcache);
+class regcache_readonly : public regcache_read
+{
+public:
+ regcache_readonly (const regcache &src);
+
+ /* Create a readonly regcache by getting contents from COOKED_READ. */
+
+ regcache_readonly (gdbarch *gdbarch,
+ regcache_cooked_read_ftype *cooked_read,
+ void *src)
+ : regcache_read (gdbarch, true)
+ {
+ save (cooked_read, src);
+ }
+
+ DISABLE_COPY_AND_ASSIGN (regcache_readonly);
+
+ void raw_update (int regnum) override
+ {}
+};
extern void registers_changed (void);
extern void registers_changed_ptid (ptid_t);
@@ -1202,7 +1202,7 @@ spu_write_pc (struct regcache *regcache, CORE_ADDR pc)
struct spu2ppu_cache
{
struct frame_id frame_id;
- struct regcache *regcache;
+ regcache_readonly *regcache;
};
static struct gdbarch *
@@ -1229,7 +1229,7 @@ spu2ppu_prev_register (struct frame_info *this_frame,
gdb_byte *buf;
buf = (gdb_byte *) alloca (register_size (gdbarch, regnum));
- regcache_cooked_read (cache->regcache, regnum, buf);
+ cache->regcache->cooked_read (regnum, buf);
return frame_unwind_got_bytes (this_frame, regnum, buf);
}
@@ -1274,7 +1274,7 @@ spu2ppu_sniffer (const struct frame_unwind *self,
{
struct regcache *regcache;
regcache = get_thread_arch_regcache (inferior_ptid, target_gdbarch ());
- cache->regcache = regcache_dup (regcache);
+ cache->regcache = new regcache_readonly (*regcache);
*this_prologue_cache = cache;
return 1;
}