* [RFA, 1 of 3] save/restore process record, part 1 (exec_entry)
@ 2009-10-17 1:27 Michael Snyder
2009-10-17 4:04 ` Joel Brobecker
0 siblings, 1 reply; 5+ messages in thread
From: Michael Snyder @ 2009-10-17 1:27 UTC (permalink / raw)
To: gdb-patches, Hui Zhu
[-- Attachment #1: Type: text/plain, Size: 350 bytes --]
OK, submitting on behalf of teawater and myself, this is a sync
and update to cvs head of the process record save/restore patch.
I've split it into 3 parts to make it easier to review.
This first part simply abstracts some deeply indented code
out of record_wait into its own function, record_exec_entry.
The later patches will call this function.
[-- Attachment #2: tmp1.txt --]
[-- Type: text/plain, Size: 5935 bytes --]
2009-10-16 Hui Zhu <teawater@gmail.com>
Michael Snyder <msnyder@vmware.com>
* record.c (record_exec_entry): New function. Emulate one
instruction, forward or backward. Abstracted from record_wait.
(record_wait) Call record_exec_entry.
--- record.0.c 2009-10-16 14:28:07.000000000 -0700
+++ record.1.c 2009-10-16 14:32:51.000000000 -0700
@@ -588,6 +588,76 @@ record_gdb_operation_disable_set (void)
return old_cleanups;
}
+static inline void
+record_exec_entry (struct regcache *regcache, struct gdbarch *gdbarch,
+ struct record_entry *entry)
+{
+ switch (entry->type)
+ {
+ case record_reg: /* reg */
+ {
+ gdb_byte reg[MAX_REGISTER_SIZE];
+
+ if (record_debug > 1)
+ fprintf_unfiltered (gdb_stdlog,
+ "Process record: record_reg %s to "
+ "inferior num = %d.\n",
+ host_address_to_string (entry),
+ entry->u.reg.num);
+
+ regcache_cooked_read (regcache, entry->u.reg.num, reg);
+ regcache_cooked_write (regcache, entry->u.reg.num,
+ record_get_loc (entry));
+ memcpy (record_get_loc (entry), reg, entry->u.reg.len);
+ }
+ break;
+
+ case record_mem: /* mem */
+ {
+ /* Nothing to do if the entry is flagged not_accessible. */
+ if (!entry->u.mem.mem_entry_not_accessible)
+ {
+ gdb_byte *mem = alloca (entry->u.mem.len);
+
+ if (record_debug > 1)
+ fprintf_unfiltered (gdb_stdlog,
+ "Process record: record_mem %s to "
+ "inferior addr = %s len = %d.\n",
+ host_address_to_string (entry),
+ paddress (gdbarch, entry->u.mem.addr),
+ entry->u.mem.len);
+
+ if (target_read_memory (entry->u.mem.addr, mem, entry->u.mem.len))
+ {
+ entry->u.mem.mem_entry_not_accessible = 1;
+ if (record_debug)
+ warning (_("Process record: error reading memory at "
+ "addr = %s len = %d."),
+ paddress (gdbarch, entry->u.mem.addr),
+ entry->u.mem.len);
+ }
+ else
+ {
+ if (target_write_memory (entry->u.mem.addr,
+ record_get_loc (entry),
+ entry->u.mem.len))
+ {
+ entry->u.mem.mem_entry_not_accessible = 1;
+ if (record_debug)
+ warning (_("Process record: error writing memory at "
+ "addr = %s len = %d."),
+ paddress (gdbarch, entry->u.mem.addr),
+ entry->u.mem.len);
+ }
+ else
+ memcpy (record_get_loc (entry), mem, entry->u.mem.len);
+ }
+ }
+ }
+ break;
+ }
+}
+
static void
record_open (char *name, int from_tty)
{
@@ -884,77 +954,9 @@ record_wait (struct target_ops *ops,
break;
}
- /* Set ptid, register and memory according to record_list. */
- if (record_list->type == record_reg)
- {
- /* reg */
- gdb_byte reg[MAX_REGISTER_SIZE];
- if (record_debug > 1)
- fprintf_unfiltered (gdb_stdlog,
- "Process record: record_reg %s to "
- "inferior num = %d.\n",
- host_address_to_string (record_list),
- record_list->u.reg.num);
- regcache_cooked_read (regcache, record_list->u.reg.num, reg);
- regcache_cooked_write (regcache, record_list->u.reg.num,
- record_get_loc (record_list));
- memcpy (record_get_loc (record_list), reg,
- record_list->u.reg.len);
- }
- else if (record_list->type == record_mem)
- {
- /* mem */
- /* Nothing to do if the entry is flagged not_accessible. */
- if (!record_list->u.mem.mem_entry_not_accessible)
- {
- gdb_byte *mem = alloca (record_list->u.mem.len);
- if (record_debug > 1)
- fprintf_unfiltered (gdb_stdlog,
- "Process record: record_mem %s to "
- "inferior addr = %s len = %d.\n",
- host_address_to_string (record_list),
- paddress (gdbarch,
- record_list->u.mem.addr),
- record_list->u.mem.len);
+ record_exec_entry (regcache, gdbarch, record_list);
- if (target_read_memory (record_list->u.mem.addr, mem,
- record_list->u.mem.len))
- {
- if (execution_direction != EXEC_REVERSE)
- error (_("Process record: error reading memory at "
- "addr = %s len = %d."),
- paddress (gdbarch, record_list->u.mem.addr),
- record_list->u.mem.len);
- else
- /* Read failed --
- flag entry as not_accessible. */
- record_list->u.mem.mem_entry_not_accessible = 1;
- }
- else
- {
- if (target_write_memory (record_list->u.mem.addr,
- record_get_loc (record_list),
- record_list->u.mem.len))
- {
- if (execution_direction != EXEC_REVERSE)
- error (_("Process record: error writing memory at "
- "addr = %s len = %d."),
- paddress (gdbarch, record_list->u.mem.addr),
- record_list->u.mem.len);
- else
- /* Write failed --
- flag entry as not_accessible. */
- record_list->u.mem.mem_entry_not_accessible = 1;
- }
- else
- {
- memcpy (record_get_loc (record_list), mem,
- record_list->u.mem.len);
- }
- }
- }
- }
- else
+ if (record_list->type == record_end)
{
if (record_debug > 1)
fprintf_unfiltered (gdb_stdlog,
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFA, 1 of 3] save/restore process record, part 1 (exec_entry)
2009-10-17 1:27 [RFA, 1 of 3] save/restore process record, part 1 (exec_entry) Michael Snyder
@ 2009-10-17 4:04 ` Joel Brobecker
2009-10-17 17:02 ` Michael Snyder
0 siblings, 1 reply; 5+ messages in thread
From: Joel Brobecker @ 2009-10-17 4:04 UTC (permalink / raw)
To: Michael Snyder; +Cc: gdb-patches, Hui Zhu
> 2009-10-16 Hui Zhu <teawater@gmail.com>
> Michael Snyder <msnyder@vmware.com>
>
> * record.c (record_exec_entry): New function. Emulate one
> instruction, forward or backward. Abstracted from record_wait.
> (record_wait) Call record_exec_entry.
I can personnally only comment on details, since I don't know much
about process record. Not sure who from the Global Maintainers actually
know much about it except you, Michael :).
> +static inline void
> +record_exec_entry (struct regcache *regcache, struct gdbarch *gdbarch,
> + struct record_entry *entry)
We're really pushing for having all functions properly documented.
Can you add a comment explaining that this function does? The function
name makes it more or less obvious, I guess, but I'd personally rather
have a consistent (mindless) approach of documenting everything rather
than having to judge on a case-by-case basis.
Also, I can't help but wonder about the use of "inline" in this case.
I'm always reluctant to use this sort of feature until I can be proven
that this helps performance. Since this is a static function, I would
imagine that the compiler would have more knowledge of whether the
function should be inlined or not? Or is that too naive?
Other than than, seems like a pretty mechanical change...
--
Joel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFA, 1 of 3] save/restore process record, part 1 (exec_entry)
2009-10-17 4:04 ` Joel Brobecker
@ 2009-10-17 17:02 ` Michael Snyder
2009-10-20 22:58 ` Michael Snyder
0 siblings, 1 reply; 5+ messages in thread
From: Michael Snyder @ 2009-10-17 17:02 UTC (permalink / raw)
To: Joel Brobecker; +Cc: gdb-patches, Hui Zhu
[-- Attachment #1: Type: text/plain, Size: 2178 bytes --]
Joel Brobecker wrote:
>> 2009-10-16 Hui Zhu <teawater@gmail.com>
>> Michael Snyder <msnyder@vmware.com>
>>
>> * record.c (record_exec_entry): New function. Emulate one
>> instruction, forward or backward. Abstracted from record_wait.
>> (record_wait) Call record_exec_entry.
>
> I can personnally only comment on details, since I don't know much
> about process record. Not sure who from the Global Maintainers actually
> know much about it except you, Michael :).
I know -- do you think, if I add more comments and documentation,
it will help encourage a few of the other GMs to take an interest?
;-)
In the meantime, Hui and I depend on each other's review.
>> +static inline void
>> +record_exec_entry (struct regcache *regcache, struct gdbarch *gdbarch,
>> + struct record_entry *entry)
>
> We're really pushing for having all functions properly documented.
> Can you add a comment explaining that this function does? The function
> name makes it more or less obvious, I guess, but I'd personally rather
> have a consistent (mindless) approach of documenting everything rather
> than having to judge on a case-by-case basis.
OK, see attached revision. Thanks for the request.
> Also, I can't help but wonder about the use of "inline" in this case.
> I'm always reluctant to use this sort of feature until I can be proven
> that this helps performance. Since this is a static function, I would
> imagine that the compiler would have more knowledge of whether the
> function should be inlined or not? Or is that too naive?
"Inline", like "register", is just a suggestion to the compiler.
The compiler will do as it pleases, based on other factors.
This function gets called once per instruction in playback mode.
Hence it seemed like it would benefit from inlining. It's probably
too big to be inlined anyway, but there again, the compiler will
make up its own mind.
I don't know whether we have a policy about using "inline" in
gdb code. Maybe we should. I'll be willing to take it out,
but until I hear a positive request, I'll leave it in. I doubt
if I can measure the performance difference, if any.
Revised diff attached.
[-- Attachment #2: tmp1b.txt --]
[-- Type: text/plain, Size: 6123 bytes --]
2009-10-16 Hui Zhu <teawater@gmail.com>
Michael Snyder <msnyder@vmware.com>
* record.c (record_exec_entry): New function. Emulate one
instruction, forward or backward. Abstracted from record_wait.
(record_wait) Call record_exec_entry.
--- record.0.c 2009-10-16 14:28:07.000000000 -0700
+++ record.1.c 2009-10-17 09:50:42.000000000 -0700
@@ -588,6 +588,80 @@ record_gdb_operation_disable_set (void)
return old_cleanups;
}
+/* Execute one instruction from the record log. Each instruction in
+ the log will be represented by an arbitrary sequence of register
+ entries and memory entries, followed by an 'end' entry. */
+
+static inline void
+record_exec_insn (struct regcache *regcache, struct gdbarch *gdbarch,
+ struct record_entry *entry)
+{
+ switch (entry->type)
+ {
+ case record_reg: /* reg */
+ {
+ gdb_byte reg[MAX_REGISTER_SIZE];
+
+ if (record_debug > 1)
+ fprintf_unfiltered (gdb_stdlog,
+ "Process record: record_reg %s to "
+ "inferior num = %d.\n",
+ host_address_to_string (entry),
+ entry->u.reg.num);
+
+ regcache_cooked_read (regcache, entry->u.reg.num, reg);
+ regcache_cooked_write (regcache, entry->u.reg.num,
+ record_get_loc (entry));
+ memcpy (record_get_loc (entry), reg, entry->u.reg.len);
+ }
+ break;
+
+ case record_mem: /* mem */
+ {
+ /* Nothing to do if the entry is flagged not_accessible. */
+ if (!entry->u.mem.mem_entry_not_accessible)
+ {
+ gdb_byte *mem = alloca (entry->u.mem.len);
+
+ if (record_debug > 1)
+ fprintf_unfiltered (gdb_stdlog,
+ "Process record: record_mem %s to "
+ "inferior addr = %s len = %d.\n",
+ host_address_to_string (entry),
+ paddress (gdbarch, entry->u.mem.addr),
+ entry->u.mem.len);
+
+ if (target_read_memory (entry->u.mem.addr, mem, entry->u.mem.len))
+ {
+ entry->u.mem.mem_entry_not_accessible = 1;
+ if (record_debug)
+ warning (_("Process record: error reading memory at "
+ "addr = %s len = %d."),
+ paddress (gdbarch, entry->u.mem.addr),
+ entry->u.mem.len);
+ }
+ else
+ {
+ if (target_write_memory (entry->u.mem.addr,
+ record_get_loc (entry),
+ entry->u.mem.len))
+ {
+ entry->u.mem.mem_entry_not_accessible = 1;
+ if (record_debug)
+ warning (_("Process record: error writing memory at "
+ "addr = %s len = %d."),
+ paddress (gdbarch, entry->u.mem.addr),
+ entry->u.mem.len);
+ }
+ else
+ memcpy (record_get_loc (entry), mem, entry->u.mem.len);
+ }
+ }
+ }
+ break;
+ }
+}
+
static void
record_open (char *name, int from_tty)
{
@@ -884,77 +958,9 @@ record_wait (struct target_ops *ops,
break;
}
- /* Set ptid, register and memory according to record_list. */
- if (record_list->type == record_reg)
- {
- /* reg */
- gdb_byte reg[MAX_REGISTER_SIZE];
- if (record_debug > 1)
- fprintf_unfiltered (gdb_stdlog,
- "Process record: record_reg %s to "
- "inferior num = %d.\n",
- host_address_to_string (record_list),
- record_list->u.reg.num);
- regcache_cooked_read (regcache, record_list->u.reg.num, reg);
- regcache_cooked_write (regcache, record_list->u.reg.num,
- record_get_loc (record_list));
- memcpy (record_get_loc (record_list), reg,
- record_list->u.reg.len);
- }
- else if (record_list->type == record_mem)
- {
- /* mem */
- /* Nothing to do if the entry is flagged not_accessible. */
- if (!record_list->u.mem.mem_entry_not_accessible)
- {
- gdb_byte *mem = alloca (record_list->u.mem.len);
- if (record_debug > 1)
- fprintf_unfiltered (gdb_stdlog,
- "Process record: record_mem %s to "
- "inferior addr = %s len = %d.\n",
- host_address_to_string (record_list),
- paddress (gdbarch,
- record_list->u.mem.addr),
- record_list->u.mem.len);
+ record_exec_insn (regcache, gdbarch, record_list);
- if (target_read_memory (record_list->u.mem.addr, mem,
- record_list->u.mem.len))
- {
- if (execution_direction != EXEC_REVERSE)
- error (_("Process record: error reading memory at "
- "addr = %s len = %d."),
- paddress (gdbarch, record_list->u.mem.addr),
- record_list->u.mem.len);
- else
- /* Read failed --
- flag entry as not_accessible. */
- record_list->u.mem.mem_entry_not_accessible = 1;
- }
- else
- {
- if (target_write_memory (record_list->u.mem.addr,
- record_get_loc (record_list),
- record_list->u.mem.len))
- {
- if (execution_direction != EXEC_REVERSE)
- error (_("Process record: error writing memory at "
- "addr = %s len = %d."),
- paddress (gdbarch, record_list->u.mem.addr),
- record_list->u.mem.len);
- else
- /* Write failed --
- flag entry as not_accessible. */
- record_list->u.mem.mem_entry_not_accessible = 1;
- }
- else
- {
- memcpy (record_get_loc (record_list), mem,
- record_list->u.mem.len);
- }
- }
- }
- }
- else
+ if (record_list->type == record_end)
{
if (record_debug > 1)
fprintf_unfiltered (gdb_stdlog,
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFA, 1 of 3] save/restore process record, part 1 (exec_entry)
2009-10-17 17:02 ` Michael Snyder
@ 2009-10-20 22:58 ` Michael Snyder
2009-10-21 2:38 ` Hui Zhu
0 siblings, 1 reply; 5+ messages in thread
From: Michael Snyder @ 2009-10-20 22:58 UTC (permalink / raw)
To: gdb-patches; +Cc: Joel Brobecker, Hui Zhu
Michael Snyder wrote:
> Joel Brobecker wrote:
>>> 2009-10-16 Hui Zhu <teawater@gmail.com>
>>> Michael Snyder <msnyder@vmware.com>
>>>
>>> * record.c (record_exec_entry): New function. Emulate one
>>> instruction, forward or backward. Abstracted from record_wait.
>>> (record_wait) Call record_exec_entry.
OK, committing this part.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFA, 1 of 3] save/restore process record, part 1 (exec_entry)
2009-10-20 22:58 ` Michael Snyder
@ 2009-10-21 2:38 ` Hui Zhu
0 siblings, 0 replies; 5+ messages in thread
From: Hui Zhu @ 2009-10-21 2:38 UTC (permalink / raw)
To: Michael Snyder; +Cc: gdb-patches, Joel Brobecker
Cool.
Thanks,
Hui
On Wed, Oct 21, 2009 at 06:51, Michael Snyder <msnyder@vmware.com> wrote:
> Michael Snyder wrote:
>>
>> Joel Brobecker wrote:
>>>>
>>>> 2009-10-16 Hui Zhu <teawater@gmail.com>
>>>> Michael Snyder <msnyder@vmware.com>
>>>>
>>>> * record.c (record_exec_entry): New function. Emulate one
>>>> instruction, forward or backward. Abstracted from record_wait.
>>>> (record_wait) Call record_exec_entry.
>
> OK, committing this part.
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-10-21 2:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-10-17 1:27 [RFA, 1 of 3] save/restore process record, part 1 (exec_entry) Michael Snyder
2009-10-17 4:04 ` Joel Brobecker
2009-10-17 17:02 ` Michael Snyder
2009-10-20 22:58 ` Michael Snyder
2009-10-21 2:38 ` Hui Zhu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox