[RFA] Submit process record and replay fourth time, 0/8
Hui Zhu
teawater@gmail.com
Wed Apr 15 16:56:00 GMT 2009
Hi Pedro,
The attachment is the compress for all the patches of precord.
And I will post each patch to maillist too.
Please help me review it.
The following is what I updated according to your comment:
>
>> +record_list_release (record_t * rec)
>> ^
>
> drop the space between '*' and rec, here and elsewhere.
It was fixed in 3.
>
> Is record_t supposed to be an opaque type? Throughout
> gdb, we tend to only "_t" struct typedef types if they're
> going to be value-like, passed by type, and opaque.
> Otherwise, we just use something like:
>
> record_list_release (struct record_entry *rec)
>
record_t is changed to record_entry in 3.
>
> Add a blank line between comment and function.
>
I fixed it in all the patches of p record.
>> + record_regcache = get_current_regcache ();
>
>
> - ARGS is a `struct gdbarch', yet you access the current regcache.
> I see you pass current_gdbarch to do_record_message. We're trying
> to eliminate the current_gdbarch global.
>
>> + if (do_record_message (current_gdbarch))
record_regcache was removed. Now, regcache is the argument of a lot
of functions of precord.
> Please delete this comment:
>
>> + /* XXX: I try to use some simple commands such as "disconnect" and
>> + "detach" to support this functions. But these commands all have
>> + other affect to GDB such as call function "no_shared_libraries".
>> + So I add special commands to GDB. */
It was deleted.
> * 4-linux-record.txt
>
>> + Copyright (C) 2008 Free Software Foundation, Inc.
>
> 2009.
Done.
>> +#include <stdint.h>
> Remove this. defs.h includes stdint.h.
Done.
> I was looking at the commands record, delrecord, stoprecord,
> record-stop-at-limit, record-insn-number-max, record-insn-number,
> and wondering what would you think if we made all record commands
> under a "record" prefix?
>
> record
> record stop
> record delete
> set record stop-at-limit
> set record insn-number-max
> info record insn-number
Done.
> * 5-infrun.txt
> infun.c:
>
>> + #include "record.h"
>
>> + && current_target.to_stratum != record_stratum);
>
> Sigh, I've spent to much time trying to explain why this was
> wrong. Let's at least leave this behind the macro you used to
> have.
OK. I make it to a macro.
The following is my explain for your comment:
>
>> + q = yquery (_("Do you want to auto delete previous execution "
>> + "log entries when record/replay buffer becomes "
>> + "full (record-stop-at-limit)?"));
>
>
> What do you mean here by "when"? Didn't the user just hit
> the limit *now*, and that is why you're asking what to do?
>
Because if user said yes, it will auto delete each time. It will not
ask user again. So I say when.
> - When I read the first time, my question is: 'What is a "record message"'?
GDB record the running message.
>> +static void
>> +record_sig_handler (int signo)
>> +{
>> + if (record_debug)
>> + fprintf_unfiltered (gdb_stdlog, "Process record: get a signal\n");
>> +
>> + /* It will break the running inferior in replay mode. */
>> + record_resume_step = 1;
>
> I still don't understand this comment. Please explain *why* you
> need to set this here.
>
{
/* In EXEC_REVERSE mode, this is the record_end of prev
instruction.
In EXEC_FORWARD mode, this is the record_end of current
instruction. */
/* step */
if (record_resume_step)
{
if (record_debug > 1)
fprintf_unfiltered (gdb_stdlog,
"Process record: step.\n");
continue_flag = 0;
}
/* check breakpoint */
tmp_pc = regcache_read_pc (regcache);
if (breakpoint_inserted_here_p (tmp_pc))
{
if (record_debug)
fprintf_unfiltered (gdb_stdlog,
"Process record: break "
"at 0x%s.\n",
paddr_nz (tmp_pc));
if (gdbarch_decr_pc_after_break (get_regcache_arch (regcache))
&& execution_direction == EXEC_FORWARD
&& !record_resume_step)
regcache_write_pc (regcache,
tmp_pc +
gdbarch_decr_pc_after_break
(get_regcache_arch (regcache)));
continue_flag = 0;
}
}
When p record get a sig, I need it stop like single step. So set
record_resume_step to 1.
>
>> +struct cleanup *
>> +record_gdb_operation_disable_set (void)
>> +{
>> + struct cleanup *old_cleanups = NULL;
>> +
>> + if (!record_gdb_operation_disable)
>> + {
>> + old_cleanups =
>> + make_cleanup_restore_integer (&record_gdb_operation_disable);
>> + record_gdb_operation_disable = 1;
>> + }
>> +
>> + return old_cleanups;
>> +}
>
> This returns a NULL cleanup if record_gdb_operation_disable is
> already set, but make_cleanup also returns a legitimate
> NULL, and it is not an error. It returns NULL when for the
> first cleanup put in the chain. See make_my_cleanup2.
>
if (set_cleanups)
do_cleanups (set_cleanups);
void
do_cleanups (struct cleanup *old_chain)
{
do_my_cleanups (&cleanup_chain, old_chain);
}
static void
do_my_cleanups (struct cleanup **pmy_chain,
struct cleanup *old_chain)
{
struct cleanup *ptr;
while ((ptr = *pmy_chain) != old_chain)
{
*pmy_chain = ptr->next; /* Do this first incase recursion */
(*ptr->function) (ptr->arg);
if (ptr->free_arg)
(*ptr->free_arg) (ptr->arg);
xfree (ptr);
}
}
NULL will make all the chain clean.
>
>> +static void
>> +record_wait_cleanups (void *ignore)
>> +{
>> + if (execution_direction == EXEC_REVERSE)
>> + {
>> + if (record_list->next)
>> + record_list = record_list->next;
>> + }
>> + else
>> + record_list = record_list->prev;
>> +}
>
>> +static ptid_t
>> +record_wait (struct target_ops *ops,
>> + ptid_t ptid, struct target_waitstatus *status)
> (...)
>> +struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0);
>
> This cleanup looks suspiciously broken to me. It appears that
> is will do weird things depending on when an exception is thrown.
> But then again, record_wait's nesting and use of goto's makes
> my eyes bleed. :P
This cleanup will make record_list point to the record entry that
execution before this record entry because set this record entry get
fail.
This part is not very easy to make clear. I make clear it use a lot
of time. :P
About goto, most of them are deleted. I just keep one:
/* Check breakpoint when forward execute. */
if (execution_direction == EXEC_FORWARD)
{
xxx
xxx
goto replay_out;
}
}
> + uint32_t addr, count;
> + regcache_raw_read (record_regcache, tdep->arg2, (gdb_byte *) & addr);
>
> This (and many more similar instances) is assuming
> host-endianness == target-endianess, that the registers are 32-bit, and
> probably that signed<->unsigned bit representation is equal. Is
> this what you want? When you get to 64-bit, most of this will break,
> it seems.
For now, record just support i386 arch. Please let us keep it to the future.
Thanks,
Hui
-------------- next part --------------
A non-text attachment was scrubbed...
Name: prec20090416.tar.gz
Type: application/x-gzip
Size: 31701 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20090415/941c627e/attachment.bin>
More information about the Gdb-patches
mailing list