Re: [RFA] Submit process record and replay fourth time, 0/8

Hui Zhu
2009-04-15T16:56:48Z

[-- Attachment #1: Type: text/plain, Size: 7327 bytes --]

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

[-- Attachment #2: prec20090416.tar.gz --]
[-- Type: application/x-gzip, Size: 31701 bytes --]