Re: [RFA] Submit process record and replay third time, 3/9

[teawater <teawater at gmail dot com>]

Hi Pedro,

On Tue, Mar 10, 2009 at 04:34, Pedro Alves <pedro@codesourcery.com> wrote:
> On Monday 23 February 2009 09:20:13, teawater wrote:
>> This is the new version patches that follow cvs-head.
>
> Thanks.
>
>>
>> ?gdb/Makefile.in | ? ?4
>> ?gdb/record.c ? ?| 1283 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> ?gdb/record.h ? ?| ? 87 +++
>> ?3 files changed, 1372 insertions(+), 2 deletions(-)
>>
>> Index: src/gdb/Makefile.in
>> ===================================================================
>> --- src.orig/gdb/Makefile.in ?2009-02-21 15:53:27.000000000 +0000
>> +++ src/gdb/Makefile.in ? ? ? 2009-02-28 20:23:20.000000000 +0000
>> @@ -662,7 +662,7 @@ SFILES = ada-exp.y ada-lang.c ada-typepr
>> ? ? ? valarith.c valops.c valprint.c value.c varobj.c vec.c \
>> ? ? ? wrapper.c \
>> ? ? ? xml-tdesc.c xml-support.c \
>> - ? ? inferior.c
>> + ? ? inferior.c record.c
>>
>> ?LINTFILES = $(SFILES) $(YYFILES) $(CONFIG_SRCS) init.c
>>
>> @@ -813,7 +813,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $
>> ? ? ? solib.o solib-null.o \
>> ? ? ? prologue-value.o memory-map.o xml-support.o \
>> ? ? ? target-descriptions.o target-memory.o xml-tdesc.o xml-builtin.o \
>> - ? ? inferior.o osdata.o
>> + ? ? inferior.o osdata.o record.o
>>
>> ?TSOBS = inflow.o
>>
>> Index: src/gdb/record.c
>> ===================================================================
>> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
>> +++ src/gdb/record.c ?2009-02-28 20:23:20.000000000 +0000
>> @@ -0,0 +1,1283 @@
>> +/* Process record and replay target for GDB, the GNU debugger.
>> +
>> + ? Copyright (C) 2008 Free Software Foundation, Inc.
>
> Oops, you'll have to update this.


I will.  :)

>
>> +
>> + ? This file is part of GDB.
>> +
>> + ? This program is free software; you can redistribute it and/or modify
>> + ? it under the terms of the GNU General Public License as published by
>> + ? the Free Software Foundation; either version 3 of the License, or
>> + ? (at your option) any later version.
>> +
>> + ? This program is distributed in the hope that it will be useful,
>> + ? but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + ? MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ?See the
>> + ? GNU General Public License for more details.
>> +
>> + ? You should have received a copy of the GNU General Public License
>> + ? along with this program. ?If not, see <http://www.gnu.org/licenses/>. ?*/
>> +
>> +#include "defs.h"
>> +#include "target.h"
>> +#include "gdbcmd.h"
>> +#include "regcache.h"
>> +#include "inferior.h"
>> +#include "gdbthread.h"
>> +#include "event-top.h"
>> +#include "annotate.h"
>> +#include "observer.h"
>> +#include "record.h"
>
> Why did you need annotate.h? ?Can you check which headers are
> really needed here?

OK. I will check all of them.

>
>> +
>> +#include <signal.h>
>> +
>> +#define DEFAULT_RECORD_INSN_MAX_NUM ?200000
>> +
>> +int record_debug = 0;
>> +
>> +record_t record_first;
>> +record_t *record_list = &record_first;
>> +record_t *record_arch_list_head = NULL;
>> +record_t *record_arch_list_tail = NULL;
>> +struct regcache *record_regcache = NULL;
>> +
>> +/* 1 ask user. 0 auto delete the last record_t. ?*/
> ? ? ? ? ? ? ? ? ^ double-space please.
>
>> +static int record_stop_at_limit = 1;
>> +static int record_insn_max_num = DEFAULT_RECORD_INSN_MAX_NUM;
>> +static int record_insn_num = 0;
>> +
>> +struct target_ops record_ops;
>> +static int record_resume_step = 0;
>> +static enum target_signal record_resume_siggnal;
>> +static int record_get_sig = 0;
>> +static sigset_t record_maskall;
>> +static int record_gdb_operation_disable = 0;
>> +int record_will_store_registers = 0;
>
> You've got a bunch of magic variables here. ?Could you
> add some comments explaining what they're for?

OK. I will.

>
>> +
>> +extern struct bp_location *bp_location_chain;
>
> This is not right. ?You're accessing a breakpoints.c
> internal implementation detail. ?We need to come up with interfaces
> to replace this hack.

If I got a interface, I will remove it.

>
>> +
>> +/* The beneath function pointers. ?*/
>> +static struct target_ops *record_beneath_to_resume_ops = NULL;
>> +static void (*record_beneath_to_resume) (struct target_ops *, ptid_t, int,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? enum target_signal) = NULL;
>> +static struct target_ops *record_beneath_to_wait_ops = NULL;
>> +static ptid_t (*record_beneath_to_wait) (struct target_ops *, ptid_t,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct target_waitstatus *) = NULL;
>> +static struct target_ops *record_beneath_to_store_registers_ops = NULL;
>> +static void (*record_beneath_to_store_registers) (struct target_ops *,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct regcache *,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int regno) = NULL;
>> +static struct target_ops *record_beneath_to_xfer_partial_ops = NULL;
>> +static LONGEST (*record_beneath_to_xfer_partial) (struct target_ops * ops,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? enum target_object object,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? const char *annex,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? gdb_byte * readbuf,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? const gdb_byte * writebuf,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ULONGEST offset,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? LONGEST len) = NULL;
>> +static int (*record_beneath_to_insert_breakpoint) (struct bp_target_info *) =
>> + ?NULL;
>> +static int (*record_beneath_to_remove_breakpoint) (struct bp_target_info *) =
>> + ?NULL;
>
> I can't say I'm happy with this mechanism yet, but it is much
> better than the previous version of making target.c aware of record.c's callbacks.
>

I had tried my best on it.

>
>> +
>> +static void
>> +record_list_release (record_t * rec)
>
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?^ no space after *, please.
>
>> +{
>> + ?record_t *tmp;
>> +
>> + ?if (!rec)
>> + ? ?return;
>> +
>> + ?while (rec->next)
>> + ? ?{
>> + ? ? ?rec = rec->next;
>> + ? ?}
>> +
>> + ?while (rec->prev)
>> + ? ?{
>> + ? ? ?tmp = rec;
>> + ? ? ?rec = rec->prev;
>> + ? ? ?if (tmp->type == record_reg)
>> + ? ? xfree (tmp->u.reg.val);
>> + ? ? ?else if (tmp->type == record_mem)
>> + ? ? xfree (tmp->u.mem.val);
>> + ? ? ?xfree (tmp);
>> + ? ?}
>> +
>> + ?if (rec != &record_first)
>> + ? ?xfree (rec);
>> +}
>> +
>> +static void
>> +record_list_release_next (void)
>> +{
>> + ?record_t *rec = record_list;
>> + ?record_t *tmp = rec->next;
>> + ?rec->next = NULL;
>> + ?while (tmp)
>> + ? ?{
>> + ? ? ?rec = tmp->next;
>> + ? ? ?if (tmp->type == record_reg)
>> + ? ? record_insn_num--;
>> + ? ? ?else if (tmp->type == record_reg)
>> + ? ? xfree (tmp->u.reg.val);
>> + ? ? ?else if (tmp->type == record_mem)
>> + ? ? xfree (tmp->u.mem.val);
>> + ? ? ?xfree (tmp);
>> + ? ? ?tmp = rec;
>> + ? ?}
>> +}
>> +
>> +static void
>> +record_list_release_first (void)
>> +{
>> + ?record_t *tmp = NULL;
>> + ?enum record_type type;
>> +
>> + ?if (!record_first.next)
>> + ? ?return;
>> +
>> + ?while (1)
>> + ? ?{
>> + ? ? ?type = record_first.next->type;
>> +
>> + ? ? ?if (type == record_reg)
>> + ? ? xfree (record_first.next->u.reg.val);
>> + ? ? ?else if (type == record_mem)
>> + ? ? xfree (record_first.next->u.mem.val);
>> + ? ? ?tmp = record_first.next;
>> + ? ? ?record_first.next = tmp->next;
>> + ? ? ?xfree (tmp);
>> +
>> + ? ? ?if (!record_first.next)
>> + ? ? {
>> + ? ? ? gdb_assert (record_insn_num == 1);
>> + ? ? ? break;
>> + ? ? }
>> +
>> + ? ? ?record_first.next->prev = &record_first;
>> +
>> + ? ? ?if (type == record_end)
>> + ? ? break;
>> + ? ?}
>> +
>> + ?record_insn_num--;
>> +}
>> +
>> +/* Add a record_t to record_arch_list. ?*/
>> +static void
>> +record_arch_list_add (record_t * rec)
>> +{
>> + ?if (record_debug > 1)
>> + ? ?fprintf_unfiltered (gdb_stdlog,
>> + ? ? ? ? ? ? ? ? ? ? "Process record: record_arch_list_add %s.\n",
>> + ? ? ? ? ? ? ? ? ? ? host_address_to_string (rec));
>> +
>> + ?if (record_arch_list_tail)
>> + ? ?{
>> + ? ? ?record_arch_list_tail->next = rec;
>> + ? ? ?rec->prev = record_arch_list_tail;
>> + ? ? ?record_arch_list_tail = rec;
>> + ? ?}
>> + ?else
>> + ? ?{
>> + ? ? ?record_arch_list_head = rec;
>> + ? ? ?record_arch_list_tail = rec;
>> + ? ?}
>> +}
>> +
>> +/* Record the value of a register ("num") to record_arch_list. ?*/
>
> When we want to mention the value of a parameter, we mentions it in
> uppercase, like so:
>
> /* Record the value of register NUM to record_arch_list. ?*/
>
> Also, there should be an empty line between the comment and
> the function itself. ?Here and elsewhere.
>
>> +int
>> +record_arch_list_add_reg (int num)
>> +{
>> + ?record_t *rec;
>> +
>> + ?if (record_debug > 1)
>> + ? ?fprintf_unfiltered (gdb_stdlog,
>> + ? ? ? ? ? ? ? ? ? ? "Process record: add register num = %d to "
>> + ? ? ? ? ? ? ? ? ? ? "record list.\n",
>> + ? ? ? ? ? ? ? ? ? ? num);
>> +
>> + ?rec = (record_t *) xmalloc (sizeof (record_t));
>> + ?rec->u.reg.val = (gdb_byte *) xmalloc (MAX_REGISTER_SIZE);
>> + ?rec->prev = NULL;
>> + ?rec->next = NULL;
>> + ?rec->type = record_reg;
>> + ?rec->u.reg.num = num;
>> +
>> + ?regcache_raw_read (record_regcache, num, rec->u.reg.val);
>> +
>> + ?record_arch_list_add (rec);
>> +
>> + ?return 0;
>> +}
>> +
>> +/* Record the value of a region of memory whose address is "addr" and
>> + ? length is "len" to record_arch_list. ?*/
>
> /* Record the value of a region of memory whose address is ADDR and
> ? length is LEN to record_arch_list. ?*/
>

OK.  I will fix all of them.


>> +
>> +int
>> +record_arch_list_add_mem (CORE_ADDR addr, int len)
>> +{
>> + ?record_t *rec;
>> +
>> + ?if (record_debug > 1)
>> + ? ?fprintf_unfiltered (gdb_stdlog,
>> + ? ? ? ? ? ? ? ? ? ? "Process record: add mem addr = 0x%s len = %d to "
>> + ? ? ? ? ? ? ? ? ? ? "record list.\n",
>> + ? ? ? ? ? ? ? ? ? ? paddr_nz (addr), len);
>> +
>
>> + ?if (!addr)
>> + ? ?return 0;
>
> Why is this check for addr == 0 necessary here?

Sometime, most time is system call.  It will send a NULL address to
Linux kernel.  Process record will try to record the change of this
address.  So it need check if addr is 0.

>
>> +
>> + ?rec = (record_t *) xmalloc (sizeof (record_t));
>> + ?rec->u.mem.val = (gdb_byte *) xmalloc (len);
>> + ?rec->prev = NULL;
>> + ?rec->next = NULL;
>> + ?rec->type = record_mem;
>> + ?rec->u.mem.addr = addr;
>> + ?rec->u.mem.len = len;
>> +
>> + ?if (target_read_memory (addr, rec->u.mem.val, len))
>> + ? ?{
>> + ? ? ?if (record_debug)
>> + ? ? fprintf_unfiltered (gdb_stdlog,
>> + ? ? ? ? ? ? ? ? ? ? ? ? "Process record: error reading memory at "
>> + ? ? ? ? ? ? ? ? ? ? ? ? "addr = 0x%s len = %d.\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? paddr_nz (addr), len);
>> + ? ? ?xfree (rec->u.mem.val);
>> + ? ? ?xfree (rec);
>> + ? ? ?return -1;
>> + ? ?}
>> +
>> + ?record_arch_list_add (rec);
>> +
>> + ?return 0;
>> +}
>> +
>> +/* Add a record_end type record_t to record_arch_list. ?*/
>> +int
>> +record_arch_list_add_end (int need_dasm)
>> +{
>> + ?record_t *rec;
>> +
>> + ?if (record_debug > 1)
>> + ? ?fprintf_unfiltered (gdb_stdlog,
>> + ? ? ? ? ? ? ? ? ? ? "Process record: add end need_dasm = %d to "
>> + ? ? ? ? ? ? ? ? ? ? "arch list.\n",
>> + ? ? ? ? ? ? ? ? ? ? need_dasm);
>> +
>> + ?rec = (record_t *) xmalloc (sizeof (record_t));
>> + ?rec->prev = NULL;
>> + ?rec->next = NULL;
>> + ?rec->type = record_end;
>> +
>> + ?rec->u.need_dasm = need_dasm;
>> +
>> + ?record_arch_list_add (rec);
>> +
>> + ?return 0;
>> +}
>> +
>> +static void
>> +record_check_insn_num (int set_terminal)
>> +{
>> + ?if (record_insn_max_num)
>> + ? ?{
>> + ? ? ?gdb_assert (record_insn_num <= record_insn_max_num);
>> + ? ? ?if (record_insn_num == record_insn_max_num)
>> + ? ? {
>> + ? ? ? /* Ask user what to do. ?*/
>> + ? ? ? if (record_stop_at_limit)
>> + ? ? ? ? {
>> + ? ? ? ? ? int q;
>> + ? ? ? ? ? if (set_terminal)
>> + ? ? ? ? ? ? target_terminal_ours ();
>> + ? ? ? ? ? q = yquery (_("Do you want to auto delete previous execution "
>> + ? ? ? ? ? ? ? ? ? ? ? ? "log entries when record/replay buffer becomes "
>> + ? ? ? ? ? ? ? ? ? ? ? ? "full (record-stop-at-limit)?"));
>> + ? ? ? ? ? if (set_terminal)
>> + ? ? ? ? ? ? target_terminal_inferior ();
>> + ? ? ? ? ? if (q)
>> + ? ? ? ? ? ? record_stop_at_limit = 0;
>> + ? ? ? ? ? else
>> + ? ? ? ? ? ? error (_("Process record: inferior program stopped."));
>> + ? ? ? ? }
>> + ? ? }
>> + ? ?}
>> +}
>> +
>> +static void
>> +record_normal_stop (void)
>> +{
>> + ?finish_thread_state (minus_one_ptid);
>> +
>> + ?if (!breakpoints_always_inserted_mode () && target_has_execution)
>> + ? ?remove_breakpoints ();
>> +
>> + ?target_terminal_ours ();
>> +
>> + ?if (target_has_stack && !stop_stack_dummy)
>> + ? ?set_current_sal_from_frame (get_current_frame (), 1);
>> +
>> + ?select_frame (get_current_frame ());
>> +
>> + ?annotate_stopped ();
>> + ?if (!suppress_stop_observer)
>> + ? ?{
>> + ? ? ?if (!ptid_equal (inferior_ptid, null_ptid))
>> + ? ? observer_notify_normal_stop (inferior_thread ()->stop_bpstat, 0);
>> + ? ? ?else
>> + ? ? observer_notify_normal_stop (NULL, 0);
>> + ? ?}
>> +}
>
> Nope sorry, this is worse than before. ?When I asked to not call
> normal_stop, I didn't mean that you should inline chunks of
> it here. ?This is papering over a different problem. ?Why
> is it that record.c needs to do this, while other targets do not?
> If we need to do something like this, it should be needed by all
> targets that can throw from target_wait (which, is documented
> as something you shouldn't do anyway). ?Actually, do you still need
> any of this in current head?

I will try to make record_message throw out error in record_wait.
Then I can remove this normail_stop code.


>
>> +
>> +/* Before inferior step (when GDB record the running message, inferior
>> + ? only can step), GDB will call this function to record the values to
>> + ? record_list. ?This function will call gdbarch_process_record to
>> + ? record the running message of inferior and set them to
>> + ? record_arch_list, and add it to record_list. ?*/
>> +
>> +static void
>> +record_message_cleanups (void *ignore)
>> +{
>> + ?record_list_release (record_arch_list_tail);
>> + ?set_executing (inferior_ptid, 0);
>> + ?record_normal_stop ();
>> +}
>
> Same as above. ?This isn't a record.c problem.
>
>> +
>> +void
>> +record_message (struct gdbarch *gdbarch)
>> +{
>> + ?int ret;
>> + ?struct cleanup *old_cleanups = make_cleanup (record_message_cleanups, 0);
>> +
>> + ?record_arch_list_head = NULL;
>> + ?record_arch_list_tail = NULL;
>> +
>> + ?/* Check record_insn_num. ?*/
>> + ?record_check_insn_num (1);
>> +
>> + ?record_regcache = get_current_regcache ();
>> +
>> + ?ret = gdbarch_process_record (gdbarch,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? regcache_read_pc (record_regcache));
>> + ?if (ret > 0)
>> + ? ?error (_("Process record: inferior program stopped."));
>> + ?if (ret < 0)
>> + ? ?error (_("Process record: failed to record execution log."));
>> +
>> + ?discard_cleanups (old_cleanups);
>> +
>> + ?record_list->next = record_arch_list_head;
>> + ?record_arch_list_head->prev = record_list;
>> + ?record_list = record_arch_list_tail;
>> +
>> + ?if (record_insn_num == record_insn_max_num && record_insn_max_num)
>> + ? ?record_list_release_first ();
>> + ?else
>> + ? ?record_insn_num++;
>> +}
>> +
>
>
>> +/* Things to clean up if we error or QUIT out of function that set
>> + ? record_gdb_operation_disable (ie. command that caused target_wait to
>> + ? be called). ?*/
>> +static void
>> +record_gdb_operation_disable_cleanups (void *ignore)
>> +{
>> + ?record_gdb_operation_disable = 0;
>> +}
>> +
>> +struct cleanup *
>> +record_gdb_operation_disable_set (void)
>> +{
>> + ?struct cleanup *old_cleanups =
>> + ? ?make_cleanup (record_gdb_operation_disable_cleanups, 0);
>> + ?record_gdb_operation_disable = 1;
>> +
>> + ?return old_cleanups;
>> +}
>
> This is make_cleanup_restore_integer.

I will change this code to use make_cleanup_restore_integer.

>
>> +
>> +static void
>> +record_open (char *name, int from_tty)
>> +{
>> + ?struct target_ops *t;
>> +
>> + ?if (record_debug)
>> + ? ?fprintf_unfiltered (gdb_stdlog, "Process record: record_open\n");
>> +
>> + ?/* check exec */
>
> and core.

I will change it.

>
>> + ?if (!target_has_execution)
>> + ? ?error (_("Process record: the program is not being run."));
>> + ?if (non_stop)
>> + ? ?error (_("Process record target can't debug inferior in non-stop mode "
>> + ? ? ? ? ?"(non-stop)."));
>> + ?if (target_async_permitted)
>> + ? ?error (_("Process record target can't debug inferior in asynchronous "
>> + ? ? ? ? ?"mode (target-async)."));
>> +
>> + ?if (!gdbarch_process_record_p (current_gdbarch))
>> + ? ?error (_("Process record: the current architecture doesn't support "
>> + ? ? ? ? ?"record function."));
>> +
>> + ?/* Check if record target is already running. ?*/
>> + ?if (TARGET_IS_PROCESS_RECORD)
>> + ? ?{
>> + ? ? ?if (!nquery
>> + ? ? ? (_("Process record target already running, do you want to delete "
>> + ? ? ? ? ?"the old record log?")))
>> + ? ? return;
>> + ? ?}
>> +
>> + ?/* Set the beneath function pointers. ?*/
>> + ?for (t = current_target.beneath; t != NULL; t = t->beneath)
>> + ? ?{
>> + ? ? ?if (!record_beneath_to_resume)
>> + ? ? ? ?{
>> + ? ? ? record_beneath_to_resume = t->to_resume;
>> + ? ? ? record_beneath_to_resume_ops = t;
>> + ? ? ? ?}
>> + ? ? ?if (!record_beneath_to_wait)
>> + ? ? ? ?{
>> + ? ? ? record_beneath_to_wait = t->to_wait;
>> + ? ? ? record_beneath_to_wait_ops = t;
>> + ? ? ? ?}
>> + ? ? ?if (!record_beneath_to_store_registers)
>> + ? ? ? ?{
>> + ? ? ? record_beneath_to_store_registers = t->to_store_registers;
>> + ? ? ? record_beneath_to_store_registers_ops = t;
>> + ? ? ? ?}
>> + ? ? ?if (!record_beneath_to_xfer_partial)
>> + ? ? ? ?{
>> + ? ? ? record_beneath_to_xfer_partial = t->to_xfer_partial;
>> + ? ? ? record_beneath_to_xfer_partial_ops = t;
>> + ? ? ? ?}
>> + ? ? ?if (!record_beneath_to_insert_breakpoint)
>> + ? ? record_beneath_to_insert_breakpoint = t->to_insert_breakpoint;
>> + ? ? ?if (!record_beneath_to_remove_breakpoint)
>> + ? ? record_beneath_to_remove_breakpoint = t->to_remove_breakpoint;
>> + ? ?}
>> + ?if (!record_beneath_to_resume)
>> + ? ?error (_("Process record can't get to_resume."));
>> + ?if (!record_beneath_to_wait)
>> + ? ?error (_("Process record can't get to_wait."));
>> + ?if (!record_beneath_to_store_registers)
>> + ? ?error (_("Process record can't get to_store_registers."));
>> + ?if (!record_beneath_to_xfer_partial)
>> + ? ?error (_("Process record can't get to_xfer_partial."));
>> + ?if (!record_beneath_to_insert_breakpoint)
>> + ? ?error (_("Process record can't get to_insert_breakpoint."));
>> + ?if (!record_beneath_to_remove_breakpoint)
>> + ? ?error (_("Process record can't get to_remove_breakpoint."));
>
> As I said above, this is better than making target.c aware of
> these pointers, but, it still isn't ideal. ?For one thing, if
> some other layer/target is pushed after the record target is opened,
> these will be wrong. ?I would prefer that this beneath lookup
> would be done at each callback implementation (record_resume, etc.),
> but, I'm happy enough with this for now. ?It is now contained, so we can
> clean this up afterwards...
>
>> +
>> + ?push_target (&record_ops);
>> +
>> + ?/* Reset */
>> + ?record_insn_num = 0;
>> + ?record_list = &record_first;
>> + ?record_list->next = NULL;
>> +}
>> +
>> +static void
>> +record_close (int quitting)
>> +{
>> + ?if (record_debug)
>> + ? ?fprintf_unfiltered (gdb_stdlog, "Process record: record_close\n");
>> +
>> + ?record_list_release (record_list);
>> +}
>
> Shouldn't this clear the record_beneath_* pointers as well?

Oops, I forget it.  I will add a reset code to record_open.

>
>> +
>> +static void
>> +record_resume (struct target_ops *ops, ptid_t ptid, int step,
>> + ? ? ? ? ? ? ? enum target_signal siggnal)
>> +{
>> + ?record_resume_step = step;
>> + ?record_resume_siggnal = siggnal;
>> +
>> + ?if (!RECORD_IS_REPLAY)
>> + ? ?{
>> + ? ? ?record_message (current_gdbarch);
>> + ? ? ?record_beneath_to_resume (record_beneath_to_resume_ops, ptid, 1,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?siggnal);
>> + ? ?}
>> +}
>> +
>> +static void
>> +record_sig_handler (int signo)
>> +{
>> + ?if (record_debug)
>> + ? ?fprintf_unfiltered (gdb_stdlog, "Process record: get a signal\n");
>> +
>> + ?record_resume_step = 1;
>> + ?record_get_sig = 1;
>> +}
>
> This handler is magical. ?Why is it setting resume_step, for instance?
> It would definitelly benefic from some comments. ?In fact, most of the
> file is undercommented.

I will add comment to there.

>
>> +
>> +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;
>> +
>> + ?set_executing (inferior_ptid, 0);
>> + ?record_normal_stop ();
>> +}
>
> See comments about record_normal_stop above.

I will try it too.

>
>> +
>> +/* record_wait
>
> Please remove the function name from the comment. ?It's redundant.
>
>> + ? In replay mode, this function examines the recorded log and
>> + ? determines where to stop. ?*/

OK. I will.

>> +
>> +static ptid_t
>> +record_wait (struct target_ops *ops,
>> + ? ? ? ? ? ? ?ptid_t ptid, struct target_waitstatus *status)
>> +{
>> + ?struct cleanup *set_cleanups = record_gdb_operation_disable_set ();
>> +
>> + ?if (record_debug)
>> + ? ?fprintf_unfiltered (gdb_stdlog,
>> + ? ? ? ? ? ? ? ? ? ? "Process record: record_wait "
>> + ? ? ? ? ? ? ? ? ? ? "record_resume_step = %d\n",
>> + ? ? ? ? ? ? ? ? ? ? record_resume_step);
>> +
>> + ?if (!RECORD_IS_REPLAY)
>> + ? ?{
>> + ? ? ?if (record_resume_step)
>> + ? ? {
>> + ? ? ? /* This is a single step. ?*/
>> + ? ? ? return record_beneath_to_wait (record_beneath_to_wait_ops,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?ptid, status);
>> + ? ? }
>> + ? ? ?else
>> + ? ? {
>> + ? ? ? if (record_resume_step)
>> + ? ? ? ? {
>> + ? ? ? ? ? /* This is a single step. ?*/
>> + ? ? ? ? ? return record_beneath_to_wait (record_beneath_to_wait_ops,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?ptid, status);
>> + ? ? ? ? }
>> + ? ? ? else
>> + ? ? ? ? {
>> + ? ? ? ? ? /* This is not a single step. ?*/
>> + ? ? ? ? ? ptid_t ret;
>> + ? ? ? ? ? int is_breakpoint = 1;
>> + ? ? ? ? ? CORE_ADDR pc = 0;
>> + ? ? ? ? ? int pc_is_read = 0;
>> + ? ? ? ? ? struct bp_location *bl;
>> + ? ? ? ? ? struct breakpoint *b;
>> +
>> + ? ? ? ? ? do
>> + ? ? ? ? ? ? {
>> + ? ? ? ? ? ? ? ret = record_beneath_to_wait (record_beneath_to_wait_ops,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?ptid, status);
>> +
>> + ? ? ? ? ? ? ? if (status->kind == TARGET_WAITKIND_STOPPED
>> + ? ? ? ? ? ? ? ? ? && status->value.sig == TARGET_SIGNAL_TRAP)
>> + ? ? ? ? ? ? ? ? {
>> + ? ? ? ? ? ? ? ? ? /* Check if there is a breakpoint. ?*/
>> + ? ? ? ? ? ? ? ? ? pc_is_read = 0;
>> + ? ? ? ? ? ? ? ? ? registers_changed ();
>> + ? ? ? ? ? ? ? ? ? for (bl = bp_location_chain; bl; bl = bl->global_next)
>
> This will need to be fixed. ?Can you use the breakpoint_here-like functions
> exported by breakpoint.h instead of referencing bp_location_chain directly?

I will try to change it.

>
>> + ? ? ? ? ? ? ? ? ? ? {
>> + ? ? ? ? ? ? ? ? ? ? ? b = bl->owner;
>> + ? ? ? ? ? ? ? ? ? ? ? gdb_assert (b);
>> + ? ? ? ? ? ? ? ? ? ? ? if (b->enable_state != bp_enabled
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? && b->enable_state != bp_permanent)
>> + ? ? ? ? ? ? ? ? ? ? ? ? continue;
>> + ? ? ? ? ? ? ? ? ? ? ? if (!pc_is_read)
>> + ? ? ? ? ? ? ? ? ? ? ? ? {
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? pc =
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? regcache_read_pc (get_thread_regcache (ret));
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? pc_is_read = 1;
>> + ? ? ? ? ? ? ? ? ? ? ? ? }
>> + ? ? ? ? ? ? ? ? ? ? ? switch (b->type)
>> + ? ? ? ? ? ? ? ? ? ? ? ? {
>> + ? ? ? ? ? ? ? ? ? ? ? ? default:
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? if (bl->address == pc)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? goto breakpoint;
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? break;
>> +
>> + ? ? ? ? ? ? ? ? ? ? ? ? case bp_watchpoint:
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? /* XXX teawater: I still not very clear how to
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?deal with it. ?*/
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? goto breakpoint;
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? break;
>> +
>> + ? ? ? ? ? ? ? ? ? ? ? ? case bp_catchpoint:
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? gdb_assert (b->ops != NULL
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? && b->ops->breakpoint_hit != NULL);
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? if (b->ops->breakpoint_hit (b))
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? goto breakpoint;
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? break;
>> +
>> + ? ? ? ? ? ? ? ? ? ? ? ? case bp_hardware_watchpoint:
>> + ? ? ? ? ? ? ? ? ? ? ? ? case bp_read_watchpoint:
>> + ? ? ? ? ? ? ? ? ? ? ? ? case bp_access_watchpoint:
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? if (STOPPED_BY_WATCHPOINT (0))
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? goto breakpoint;
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? ? ? ? ? ? ? }
>> + ? ? ? ? ? ? ? ? ? ? }
>> +
>> + ? ? ? ? ? ? ? ? ? /* There is not a breakpoint. ?*/
>> + ? ? ? ? ? ? ? ? ? record_message (current_gdbarch);
>> + ? ? ? ? ? ? ? ? ? record_beneath_to_resume (record_beneath_to_resume_ops,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?ptid, 1,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? record_resume_siggnal);
>> + ? ? ? ? ? ? ? ? ? continue;
>> + ? ? ? ? ? ? ? ? }
>> +
>> + ? ? ? ? ? ? ? is_breakpoint = 0;
>> +
>> + ? ? ? ? ? ? breakpoint:
>> + ? ? ? ? ? ? ? /* Add gdbarch_decr_pc_after_break to pc because gdb will
>> + ? ? ? ? ? ? ? ? ?expect the pc to be at address plus decr_pc_after_break
>> + ? ? ? ? ? ? ? ? ?when the inferior stops at a breakpoint. ?*/
>> + ? ? ? ? ? ? ? if (is_breakpoint)
>> + ? ? ? ? ? ? ? ? {
>> + ? ? ? ? ? ? ? ? ? CORE_ADDR decr_pc_after_break =
>> + ? ? ? ? ? ? ? ? ? ? gdbarch_decr_pc_after_break (current_gdbarch);
>> + ? ? ? ? ? ? ? ? ? if (decr_pc_after_break)
>> + ? ? ? ? ? ? ? ? ? ? {
>> + ? ? ? ? ? ? ? ? ? ? ? if (!pc_is_read)
>> + ? ? ? ? ? ? ? ? ? ? ? ? pc =
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? regcache_read_pc (get_thread_regcache (ret));
>> + ? ? ? ? ? ? ? ? ? ? ? regcache_write_pc (get_thread_regcache (ret),
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?pc + decr_pc_after_break);
>> + ? ? ? ? ? ? ? ? ? ? }
>> + ? ? ? ? ? ? ? ? }
>> +
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? }
>> + ? ? ? ? ? while (1);
>> +
>> + ? ? ? ? ? return ret;
>> + ? ? ? ? }
>> + ? ? }
>> + ? ?}
>> + ?else
>> + ? ?{
>> + ? ? ?int need_dasm = 0;
>> + ? ? ?struct regcache *regcache = get_current_regcache ();
>> + ? ? ?int continue_flag = 1;
>> + ? ? ?int first_record_end = 1;
>> + ? ? ?struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0);
>> + ? ? ?CORE_ADDR tmp_pc;
>> +
>> + ? ? ?status->kind = TARGET_WAITKIND_STOPPED;
>> +
>> + ? ? ?/* Check breakpoint when forward execute. ?*/
>> + ? ? ?if (execution_direction == EXEC_FORWARD)
>> + ? ? {
>> + ? ? ? 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))
>> + ? ? ? ? ? ? ? && !record_resume_step)
>> + ? ? ? ? ? ? regcache_write_pc (regcache,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?tmp_pc +
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?gdbarch_decr_pc_after_break
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(get_regcache_arch (regcache)));
>> + ? ? ? ? ? goto replay_out;
>> + ? ? ? ? }
>> + ? ? }
>> +
>> + ? ? ?record_get_sig = 0;
>> + ? ? ?signal (SIGINT, record_sig_handler);
>> + ? ? ?/* If GDB is in terminal_inferior mode, it will not get the signal.
>> + ? ? ? ? And in GDB replay mode, GDB doesn't need to be in terminal_inferior
>> + ? ? ? ? mode, because inferior will not executed.
>> + ? ? ? ? Then set it to terminal_ours to make GDB get the signal. ?*/
>> + ? ? ?target_terminal_ours ();
>> +
>> + ? ? ?/* In EXEC_FORWARD mode, record_list points to the tail of prev
>> + ? ? ? ? instruction. ?*/
>> + ? ? ?if (execution_direction == EXEC_FORWARD && record_list->next)
>> + ? ? record_list = record_list->next;
>> +
>> + ? ? ?/* Loop over the record_list, looking for the next place to
>> + ? ? ?stop. ?*/
>> + ? ? ?do
>> + ? ? {
>> + ? ? ? /* Check for beginning and end of log. ?*/
>> + ? ? ? if (execution_direction == EXEC_REVERSE
>> + ? ? ? ? ? && record_list == &record_first)
>> + ? ? ? ? {
>> + ? ? ? ? ? /* Hit beginning of record log in reverse. ?*/
>> + ? ? ? ? ? status->kind = TARGET_WAITKIND_NO_HISTORY;
>> + ? ? ? ? ? break;
>> + ? ? ? ? }
>> + ? ? ? if (execution_direction != EXEC_REVERSE && !record_list->next)
>> + ? ? ? ? {
>> + ? ? ? ? ? /* Hit end of record log going forward. ?*/
>> + ? ? ? ? ? status->kind = TARGET_WAITKIND_NO_HISTORY;
>> + ? ? ? ? ? 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_list->u.reg.val);
>> + ? ? ? ? ? memcpy (record_list->u.reg.val, reg, MAX_REGISTER_SIZE);
>> + ? ? ? ? }
>> + ? ? ? else if (record_list->type == record_mem)
>> + ? ? ? ? {
>> + ? ? ? ? ? /* mem */
>> + ? ? ? ? ? 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 = 0x%s len = %d.\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? host_address_to_string (record_list),
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? paddr_nz (record_list->u.mem.addr),
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? record_list->u.mem.len);
>> +
>> + ? ? ? ? ? if (target_read_memory
>> + ? ? ? ? ? ? ? (record_list->u.mem.addr, mem, record_list->u.mem.len))
>> + ? ? ? ? ? ? error (_("Process record: error reading memory at "
>> + ? ? ? ? ? ? ? ? ? ? ?"addr = 0x%s len = %d."),
>> + ? ? ? ? ? ? ? ? ? ?paddr_nz (record_list->u.mem.addr),
>> + ? ? ? ? ? ? ? ? ? ?record_list->u.mem.len);
>> +
>> + ? ? ? ? ? if (target_write_memory
>> + ? ? ? ? ? ? ? (record_list->u.mem.addr, record_list->u.mem.val,
>> + ? ? ? ? ? ? ? ?record_list->u.mem.len))
>> + ? ? ? ? ? ? error (_
>> + ? ? ? ? ? ? ? ? ? ?("Process record: error writing memory at "
>> + ? ? ? ? ? ? ? ? ? ? "addr = 0x%s len = %d."),
>> + ? ? ? ? ? ? ? ? ? ?paddr_nz (record_list->u.mem.addr),
>> + ? ? ? ? ? ? ? ? ? ?record_list->u.mem.len);
>> +
>> + ? ? ? ? ? memcpy (record_list->u.mem.val, mem, record_list->u.mem.len);
>> + ? ? ? ? }
>> + ? ? ? else
>> + ? ? ? ? {
>> + ? ? ? ? ? if (record_debug > 1)
>> + ? ? ? ? ? ? fprintf_unfiltered (gdb_stdlog,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "Process record: record_end %s to "
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "inferior need_dasm = %d.\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? host_address_to_string (record_list),
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? record_list->u.need_dasm);
>> +
>> + ? ? ? ? ? if (execution_direction == EXEC_FORWARD)
>> + ? ? ? ? ? ? need_dasm = record_list->u.need_dasm;
>> + ? ? ? ? ? if (need_dasm)
>> + ? ? ? ? ? ? gdbarch_process_record_dasm (current_gdbarch);
>> +
>> + ? ? ? ? ? if (first_record_end && execution_direction == EXEC_REVERSE)
>> + ? ? ? ? ? ? {
>> + ? ? ? ? ? ? ? /* When reverse excute, the first record_end is the part of
>> + ? ? ? ? ? ? ? ? ?current instruction. ?*/
>> + ? ? ? ? ? ? ? first_record_end = 0;
>> + ? ? ? ? ? ? }
>> + ? ? ? ? ? else
>> + ? ? ? ? ? ? {
>> + ? ? ? ? ? ? ? /* 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;
>> + ? ? ? ? ? ? ? ? }
>> + ? ? ? ? ? ? }
>> + ? ? ? ? ? if (execution_direction == EXEC_REVERSE)
>> + ? ? ? ? ? ? need_dasm = record_list->u.need_dasm;
>> + ? ? ? ? }
>> +
>> +next:
>> + ? ? ? if (continue_flag)
>> + ? ? ? ? {
>> + ? ? ? ? ? if (execution_direction == EXEC_REVERSE)
>> + ? ? ? ? ? ? {
>> + ? ? ? ? ? ? ? if (record_list->prev)
>> + ? ? ? ? ? ? ? ? record_list = record_list->prev;
>> + ? ? ? ? ? ? }
>> + ? ? ? ? ? else
>> + ? ? ? ? ? ? {
>> + ? ? ? ? ? ? ? if (record_list->next)
>> + ? ? ? ? ? ? ? ? record_list = record_list->next;
>> + ? ? ? ? ? ? }
>> + ? ? ? ? }
>> + ? ? }
>> + ? ? ?while (continue_flag);
>> +
>> + ? ? ?signal (SIGINT, handle_sigint);
>> +
>> +replay_out:
>> + ? ? ?if (record_get_sig)
>> + ? ? status->value.sig = TARGET_SIGNAL_INT;
>> + ? ? ?else
>> + ? ? status->value.sig = TARGET_SIGNAL_TRAP;
>> +
>> + ? ? ?discard_cleanups (old_cleanups);
>> + ? ?}
>> +
>> + ?do_cleanups (set_cleanups);
>> + ?return inferior_ptid;
>> +}
>
> I have to say that I find that function confusing, due to
> the use of gotos, and excessive nesting. ?Personally, I much prefer
> code that does:
>
> ?if (foo)
> ? ?{
> ? ? ?/* something */
> ? ? ?return;
> ? ?}
>
> ?if (bar)
> ? ?{
> ? ? ?/* something */
> ? ? ?return;
> ? ?}
>
> ?if (lala)
> ? ?{
> ? ? ?/* something */
> ? ? ?return;
> ? ?}
>
> Over:
>
> ?if (foo)
> ? ?{
> ? ? ?/* something */
> ? ? ?return;
> ? ?}
> ?else
> ? {
> ? ? if (bar)
> ? ? ? {
> ? ? ? ? /* something */
> ? ? ? ? return;
> ? ? ? }
> ? ? else
> ? ? ?{
> ? ? ? ? if (lala)
> ? ? ? ? ? {
> ? ? ? ? ? ? /* something */
> ? ? ? ? ? ? return;
> ? ? ? ? ? }
> ? ? ?}
> ? }
>

In record_wait, all the code in else part's return need do same thing:
      signal (SIGINT, handle_sigint);
      if (record_get_sig)
	status->value.sig = TARGET_SIGNAL_INT;
      else
	status->value.sig = TARGET_SIGNAL_TRAP;
      discard_cleanups (old_cleanups);
BTW, after remove a lot of part of this function, this function must
be clear.  :)

>
>> +
>> +static void
>> +record_disconnect (struct target_ops *target, char *args, int from_tty)
>> +{
>> + ?if (record_debug)
>> + ? ?fprintf_unfiltered (gdb_stdlog, "Process record: record_disconnect\n");
>> +
>> + ?unpush_target (&record_ops);
>> + ?target_disconnect (args, from_tty);
>> +}
>> +
>> +static void
>> +record_detach (struct target_ops *ops, char *args, int from_tty)
>> +{
>> + ?if (record_debug)
>> + ? ?fprintf_unfiltered (gdb_stdlog, "Process record: record_detach\n");
>> +
>> + ?unpush_target (&record_ops);
>> + ?target_detach (args, from_tty);
>> +}
>
> This trick you're using happens to work, but, could you try
> this instead? ?Here and elsewhere similarly.
>
> ?struct target_ops *beneath = find_target_beaneath (ops);
> ?unpush_target (ops);
> ?beneath->to_detach (args, from_tty);

OK. I will.

>
>> +
>> +static void
>> +record_mourn_inferior (struct target_ops *ops)
>> +{
>> + ?if (record_debug)
>> + ? ?fprintf_unfiltered (gdb_stdlog, "Process record: "
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "record_mourn_inferior\n");
>> +
>> + ?unpush_target (&record_ops);
>> + ?target_mourn_inferior ();
>> +}
>> +
>> +/* Close process record target before killing the inferior process. ?*/
>> +static void
>> +record_kill (void)
>> +{
>> + ?if (record_debug)
>> + ? ?fprintf_unfiltered (gdb_stdlog, "Process record: record_kill\n");
>> +
>> + ?unpush_target (&record_ops);
>> + ?target_kill ();
>> +}
>> +
>> +/* Record registers change (by user or by GDB) to list as an instruction. ?*/
>> +static void
>> +record_registers_change (struct regcache *regcache, int regnum)
>> +{
>> + ?/* Check record_insn_num. ?*/
>> + ?record_check_insn_num (0);
>> +
>> + ?record_arch_list_head = NULL;
>> + ?record_arch_list_tail = NULL;
>> +
>> + ?record_regcache = get_current_regcache ();
>> +
>> + ?if (regnum < 0)
>> + ? ?{
>> + ? ? ?int i;
>> + ? ? ?for (i = 0; i < gdbarch_num_regs (get_regcache_arch (regcache)); i++)
>> + ? ? {
>> + ? ? ? if (record_arch_list_add_reg (i))
>> + ? ? ? ? {
>> + ? ? ? ? ? record_list_release (record_arch_list_tail);
>> + ? ? ? ? ? error (_("Process record: failed to record execution log."));
>> + ? ? ? ? }
>> + ? ? }
>> + ? ?}
>> + ?else
>> + ? ?{
>> + ? ? ?if (record_arch_list_add_reg (regnum))
>> + ? ? {
>> + ? ? ? record_list_release (record_arch_list_tail);
>> + ? ? ? error (_("Process record: failed to record execution log."));
>> + ? ? }
>> + ? ?}
>> + ?if (record_arch_list_add_end (0))
>> + ? ?{
>> + ? ? ?record_list_release (record_arch_list_tail);
>> + ? ? ?error (_("Process record: failed to record execution log."));
>> + ? ?}
>> + ?record_list->next = record_arch_list_head;
>> + ?record_arch_list_head->prev = record_list;
>> + ?record_list = record_arch_list_tail;
>> +
>> + ?if (record_insn_num == record_insn_max_num && record_insn_max_num)
>> + ? ?record_list_release_first ();
>> + ?else
>> + ? ?record_insn_num++;
>> +}
>> +
>> +static void
>> +record_store_registers (struct target_ops *ops, struct regcache *regcache,
>> + ? ? ? ? ? ? ? ? ? ? ? ?int regno)
>> +{
>> + ?if (!record_gdb_operation_disable)
>> + ? ?{
>> + ? ? ?if (RECORD_IS_REPLAY)
>> + ? ? {
>> + ? ? ? int n;
>> + ? ? ? struct cleanup *old_cleanups;
>> +
>> + ? ? ? /* Let user choose if he wants to write register or not. ?*/
>> + ? ? ? if (regno < 0)
>> + ? ? ? ? n =
>> + ? ? ? ? ? nquery (_("Because GDB is in replay mode, changing the "
>> + ? ? ? ? ? ? ? ? ? ? "value of a register will make the execution "
>> + ? ? ? ? ? ? ? ? ? ? "log unusable from this point onward. ?"
>> + ? ? ? ? ? ? ? ? ? ? "Change all registers?"));
>> + ? ? ? else
>> + ? ? ? ? n =
>> + ? ? ? ? ? nquery (_("Because GDB is in replay mode, changing the value "
>> + ? ? ? ? ? ? ? ? ? ? "of a register will make the execution log unusable "
>> + ? ? ? ? ? ? ? ? ? ? "from this point onward. ?Change register %s?"),
>> + ? ? ? ? ? ? ? ? ? gdbarch_register_name (get_regcache_arch (regcache),
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?regno));
>> +
>> + ? ? ? if (!n)
>> + ? ? ? ? {
>> + ? ? ? ? ? /* Invalidate the value of regcache that was set in function
>> + ? ? ? ? ? ? ?"regcache_raw_write". ?*/
>> + ? ? ? ? ? if (regno < 0)
>> + ? ? ? ? ? ? {
>> + ? ? ? ? ? ? ? int i;
>> + ? ? ? ? ? ? ? for (i = 0;
>> + ? ? ? ? ? ? ? ? ? ?i < gdbarch_num_regs (get_regcache_arch (regcache));
>> + ? ? ? ? ? ? ? ? ? ?i++)
>> + ? ? ? ? ? ? ? ? regcache_invalidate (regcache, i);
>> + ? ? ? ? ? ? }
>> + ? ? ? ? ? else
>> + ? ? ? ? ? ? regcache_invalidate (regcache, regno);
>> +
>> + ? ? ? ? ? error (_("Process record canceled the operation."));
>> + ? ? ? ? }
>> +
>> + ? ? ? /* Destroy the record from here forward. ?*/
>> + ? ? ? record_list_release_next ();
>> + ? ? }
>> +
>> + ? ? ?record_registers_change (regcache, regno);
>> + ? ?}
>> + ?record_beneath_to_store_registers (record_beneath_to_store_registers_ops,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? regcache, regno);
>> +}
>> +
>> +/* record_xfer_partial -- behavior is conditional on RECORD_IS_REPLAY.
>> + ? In replay mode, we cannot write memory unles we are willing to
>> + ? invalidate the record/replay log from this point forward. ?*/
>> +
>> +static LONGEST
>> +record_xfer_partial (struct target_ops *ops, enum target_object object,
>> + ? ? ? ? ? ? ? ? ?const char *annex, gdb_byte * readbuf,
>> + ? ? ? ? ? ? ? ? ?const gdb_byte * writebuf, ULONGEST offset, LONGEST len)
>> +{
>> + ?if (!record_gdb_operation_disable
>> + ? ? ?&& (object == TARGET_OBJECT_MEMORY
>> + ? ? ? || object == TARGET_OBJECT_RAW_MEMORY) && writebuf)
>> + ? ?{
>> + ? ? ?if (RECORD_IS_REPLAY)
>> + ? ? {
>> + ? ? ? /* Let user choose if he wants to write memory or not. ?*/
>> + ? ? ? if (!nquery (_("Because GDB is in replay mode, writing to memory "
>> + ? ? ? ? ? ? ? ? ? ? ?"will make the execution log unusable from this "
>> + ? ? ? ? ? ? ? ? ? ? ?"point onward. ?Write memory at address 0x%s?"),
>> + ? ? ? ? ? ? ? ? ? ?paddr_nz (offset)))
>> + ? ? ? ? return -1;
>> +
>> + ? ? ? /* Destroy the record from here forward. ?*/
>> + ? ? ? record_list_release_next ();
>> + ? ? }
>> +
>> + ? ? ?/* Check record_insn_num */
>> + ? ? ?record_check_insn_num (0);
>> +
>> + ? ? ?/* Record registers change to list as an instruction. ?*/
>> + ? ? ?record_arch_list_head = NULL;
>> + ? ? ?record_arch_list_tail = NULL;
>> + ? ? ?if (record_arch_list_add_mem (offset, len))
>> + ? ? {
>> + ? ? ? record_list_release (record_arch_list_tail);
>> + ? ? ? if (record_debug)
>> + ? ? ? ? fprintf_unfiltered (gdb_stdlog,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? _("Process record: failed to record "
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "execution log."));
>> + ? ? ? return -1;
>> + ? ? }
>> + ? ? ?if (record_arch_list_add_end (0))
>> + ? ? {
>> + ? ? ? record_list_release (record_arch_list_tail);
>> + ? ? ? if (record_debug)
>> + ? ? ? ? fprintf_unfiltered (gdb_stdlog,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? _("Process record: failed to record "
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "execution log."));
>> + ? ? ? return -1;
>> + ? ? }
>> + ? ? ?record_list->next = record_arch_list_head;
>> + ? ? ?record_arch_list_head->prev = record_list;
>> + ? ? ?record_list = record_arch_list_tail;
>> +
>> + ? ? ?if (record_insn_num == record_insn_max_num && record_insn_max_num)
>> + ? ? record_list_release_first ();
>> + ? ? ?else
>> + ? ? record_insn_num++;
>> + ? ?}
>> +
>> + ?return record_beneath_to_xfer_partial (record_beneath_to_xfer_partial_ops,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? object, annex, readbuf, writebuf,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? offset, len);
>> +}
>> +
>> +/* record_insert_breakpoint
>> + ? record_remove_breakpoint
>> + ? Behavior is conditional on RECORD_IS_REPLAY.
>> + ? We will not actually insert or remove breakpoints when replaying,
>> + ? nor when recording. ?*/
>> +
>> +static int
>> +record_insert_breakpoint (struct bp_target_info *bp_tgt)
>> +{
>> + ?if (!RECORD_IS_REPLAY)
>> + ? ?{
>> + ? ? ?struct cleanup *old_cleanups = record_gdb_operation_disable_set ();
>> + ? ? ?int ret = record_beneath_to_insert_breakpoint (bp_tgt);
>> +
>> + ? ? ?do_cleanups (old_cleanups);
>> +
>> + ? ? ?return ret;
>> + ? ?}
>> +
>> + ?return 0;
>> +}
>> +
>> +static int
>> +record_remove_breakpoint (struct bp_target_info *bp_tgt)
>> +{
>> + ?if (!RECORD_IS_REPLAY)
>> + ? ?{
>> + ? ? ?struct cleanup *old_cleanups = record_gdb_operation_disable_set ();
>> + ? ? ?int ret = record_beneath_to_remove_breakpoint (bp_tgt);
>> +
>> + ? ? ?do_cleanups (old_cleanups);
>> +
>> + ? ? ?return ret;
>> + ? ?}
>> +
>> + ?return 0;
>> +}
>> +
>> +static int
>> +record_can_execute_reverse (void)
>> +{
>> + ?return 1;
>> +}
>> +
>> +static void
>> +init_record_ops (void)
>> +{
>> + ?record_ops.to_shortname = "record";
>> + ?record_ops.to_longname = "Process record and replay target";
>> + ?record_ops.to_doc =
>> + ? ?"Log program while executing and replay execution from log.";
>> + ?record_ops.to_open = record_open;
>> + ?record_ops.to_close = record_close;
>> + ?record_ops.to_resume = record_resume;
>> + ?record_ops.to_wait = record_wait;
>> + ?record_ops.to_disconnect = record_disconnect;
>> + ?record_ops.to_detach = record_detach;
>> + ?record_ops.to_mourn_inferior = record_mourn_inferior;
>> + ?record_ops.to_kill = record_kill;
>> + ?record_ops.to_create_inferior = find_default_create_inferior;
>> + ?record_ops.to_store_registers = record_store_registers;
>> + ?record_ops.to_xfer_partial = record_xfer_partial;
>> + ?record_ops.to_insert_breakpoint = record_insert_breakpoint;
>> + ?record_ops.to_remove_breakpoint = record_remove_breakpoint;
>> + ?record_ops.to_can_execute_reverse = record_can_execute_reverse;
>> + ?record_ops.to_stratum = record_stratum;
>> + ?record_ops.to_magic = OPS_MAGIC;
>> +}
>> +
>> +static void
>> +show_record_debug (struct ui_file *file, int from_tty,
>> + ? ? ? ? ? ? ? ?struct cmd_list_element *c, const char *value)
>> +{
>> + ?fprintf_filtered (file, _("Debugging of process record target is %s.\n"),
>> + ? ? ? ? ? ? ? ? value);
>> +}
>> +
>> +/* cmd_record_start -- alias for "target record". ?*/
>> +
>> +static void
>> +cmd_record_start (char *args, int from_tty)
>> +{
>> + ?execute_command ("target record", from_tty);
>> +}
>> +
>> +/* cmd_record_delete -- truncate the record log from the present point
>> + ? of replay until the end. ?*/
>> +
>> +static void
>> +cmd_record_delete (char *args, int from_tty)
>> +{
>> + ?if (TARGET_IS_PROCESS_RECORD)
>> + ? ?{
>> + ? ? ?if (RECORD_IS_REPLAY)
>> + ? ? {
>> + ? ? ? if (!from_tty || query (_("Delete the log from this point forward "
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "and begin to record the running message "
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "at current PC?")))
>> + ? ? ? ? record_list_release_next ();
>> + ? ? }
>> + ? ? ?else
>> + ? ? ? printf_unfiltered (_("Already at end of record list.\n"));
>> +
>> + ? ?}
>> + ?else
>> + ? ?printf_unfiltered (_("Process record is not started.\n"));
>> +}
>> +
>> +/* cmd_record_stop -- implement the "stoprecord" command. ?*/
>> +
>> +static void
>> +cmd_record_stop (char *args, int from_tty)
>> +{
>> + ?if (TARGET_IS_PROCESS_RECORD)
>> + ? ?{
>> + ? ? ?if (!record_list || !from_tty || query (_("Delete recorded log and "
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "stop recording?")))
>> + ? ? unpush_target (&record_ops);
>> + ? ?}
>> + ?else
>> + ? ?printf_unfiltered (_("Process record is not started.\n"));
>> +}
>> +
>> +/* set_record_insn_max_num -- set upper limit of record log size. ?*/
>> +
>> +static void
>> +set_record_insn_max_num (char *args, int from_tty, struct cmd_list_element *c)
>> +{
>> + ?if (record_insn_num > record_insn_max_num && record_insn_max_num)
>> + ? ?{
>> + ? ? ?printf_unfiltered (_("Record instructions number is bigger than "
>> + ? ? ? ? ? ? ? ? ? ? ? ?"record instructions max number. ?Auto delete "
>> + ? ? ? ? ? ? ? ? ? ? ? ?"the first ones?\n"));
>> +
>> + ? ? ?while (record_insn_num > record_insn_max_num)
>> + ? ? record_list_release_first ();
>> + ? ?}
>> +}
>> +
>> +/* show_record_insn_number -- print the current index
>> + ? into the record log (number of insns recorded so far). ?*/
>> +
>> +static void
>> +show_record_insn_number (char *ignore, int from_tty)
>> +{
>> + ?printf_unfiltered (_("Record instruction number is %d.\n"),
>> + ? ? ? ? ? ? ? ? ?record_insn_num);
>> +}
>> +
>> +void
>> +_initialize_record (void)
>> +{
>> + ?/* Init record_maskall. ?*/
>> + ?if (sigfillset (&record_maskall) == -1)
>> + ? ?perror_with_name (_("Process record: sigfillset failed"));
>
> This will not build on all hosts. ?Is it still needed? ?I can't
> find any other reference to this variable in this patch.

I forget it.  I will remove it.

>
>> +
>> + ?/* Init record_first. ?*/
>> + ?record_first.prev = NULL;
>> + ?record_first.next = NULL;
>> + ?record_first.type = record_end;
>> + ?record_first.u.need_dasm = 0;
>> +
>> + ?init_record_ops ();
>> + ?add_target (&record_ops);
>> +
>> + ?add_setshow_zinteger_cmd ("record", no_class, &record_debug,
>> + ? ? ? ? ? ? ? ? ? ? ? ? _("Set debugging of record/replay feature."),
>> + ? ? ? ? ? ? ? ? ? ? ? ? _("Show debugging of record/replay feature."),
>> + ? ? ? ? ? ? ? ? ? ? ? ? _("When enabled, debugging output for "
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? "record/replay feature is displayed."),
>> + ? ? ? ? ? ? ? ? ? ? ? ? NULL, show_record_debug, &setdebuglist,
>> + ? ? ? ? ? ? ? ? ? ? ? ? &showdebuglist);
>> +
>> + ?add_com ("record", class_obscure, cmd_record_start,
>> + ? ? ? ?_("Abbreviated form of \"target record\" command."));
>> +
>> + ?add_com_alias ("rec", "record", class_obscure, 1);
>> +
>> + ?/* 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. ?*/
>> + ?add_com ("delrecord", class_obscure, cmd_record_delete,
>> + ? ? ? ?_("Delete the rest of execution log and start recording it anew."));
>> + ?add_com_alias ("dr", "delrecord", class_obscure, 1);
>> + ?add_com ("stoprecord", class_obscure, cmd_record_stop,
>> + ? ? ? ?_("Stop the record/replay target."));
>> + ?add_com_alias ("sr", "stoprecord", class_obscure, 1);
>> +
>> + ?/* Record instructions number limit command. ?*/
>> + ?add_setshow_boolean_cmd ("record-stop-at-limit", no_class,
>> + ? ? ? ? ? ? ? ? ? ? ? ? &record_stop_at_limit,
>> + ? ? ? ? ? ? ? ? ? ? ? ? _("Set whether record/replay stop when "
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? "record/replay buffer becomes full."),
>> + ? ? ? ? ? ? ? ? ? ? ? ? _("Show whether record/replay stop when "
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? "record/replay buffer becomes full."),
>> + ? ? ? ? ? ? ? ? ? ? ? ? _("Enable is default value.\n"
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? "When enabled, if the record/replay buffer "
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? "becomes full,\n"
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"ask user what to do.\n"
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"When disabled, if the record/replay buffer "
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? "becomes full,\n"
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"delete it and start new recording."),
>> + ? ? ? ? ? ? ? ? ? ? ? ? NULL, NULL, &setlist, &showlist);
>> + ?add_setshow_zinteger_cmd ("record-insn-number-max", no_class,
>> + ? ? ? ? ? ? ? ? ? ? ? ? &record_insn_max_num,
>> + ? ? ? ? ? ? ? ? ? ? ? ? _("Set record/replay buffer limit."),
>> + ? ? ? ? ? ? ? ? ? ? ? ? _("Show record/replay buffer limit."),
>> + ? ? ? ? ? ? ? ? ? ? ? ? _("Set the maximum number of instructions to be "
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"stored in the\n"
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"record/replay buffer. ?"
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"Zero means unlimited (default 200000)."),
>> + ? ? ? ? ? ? ? ? ? ? ? ? set_record_insn_max_num,
>> + ? ? ? ? ? ? ? ? ? ? ? ? NULL, &setlist, &showlist);
>> + ?add_info ("record-insn-number", show_record_insn_number,
>> + ? ? ? ? _("Show the current number of instructions in the "
>> + ? ? ? ? ? "record/replay buffer."));
>> +}
>> Index: src/gdb/record.h
>> ===================================================================
>> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
>> +++ src/gdb/record.h ?2009-02-28 20:23:20.000000000 +0000
>> @@ -0,0 +1,87 @@
>> +/* Process record and replay target for GDB, the GNU debugger.
>> +
>> + ? Copyright (C) 2008 Free Software Foundation, Inc.
>> +
>> + ? This file is part of GDB.
>> +
>> + ? This program is free software; you can redistribute it and/or modify
>> + ? it under the terms of the GNU General Public License as published by
>> + ? the Free Software Foundation; either version 3 of the License, or
>> + ? (at your option) any later version.
>> +
>> + ? This program is distributed in the hope that it will be useful,
>> + ? but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + ? MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ?See the
>> + ? GNU General Public License for more details.
>> +
>> + ? You should have received a copy of the GNU General Public License
>> + ? along with this program. ?If not, see <http://www.gnu.org/licenses/>. ?*/
>> +
>> +#ifndef _RECORD_H_
>> +#define _RECORD_H_
>> +
>> +#define TARGET_IS_PROCESS_RECORD ? \
>> + ? ? (current_target.beneath == &record_ops)
>
> Sorry, but I repeat the request I've made several times already. ?This is
> not the right way to do this. ?You need to add a new target_ops method or
> property that the core of GDB checks on. ?It is not correct that make
> the core of GDB reference record_ops directly. ?To come up with
> the target callback's name, at each call site of TARGET_IS_PROCESS_RECORD,
> consider what is the property of the current target that GDB needs to
> know about the current target. ?Is it something like:
>
> ?target_is_recording () ?
> ?target_is_replaying () ?
> ?target_is_read_only () ?
>
> etc.
>

I forget a process record has special strata "record_stratum".

What about delete "TARGET_IS_PROCESS_RECORD" and add
#define target_is_record (target) (target->to_stratum == record_stratum)
to target.h?

>> +#define RECORD_IS_REPLAY \
>> + ? ? (record_list->next || execution_direction == EXEC_REVERSE)
>
> AFAICS, this macro is not used outside of record.c. ?It should move
> there, along with anything that isn't used outside of record.c.

OK. I will put it to record.c.

>
>> +
>> +typedef struct record_reg_s
>> +{
>> + ?int num;
>> + ?gdb_byte *val;
>> +} record_reg_t;
>> +
>> +typedef struct record_mem_s
>> +{
>> + ?CORE_ADDR addr;
>> + ?int len;
>> + ?gdb_byte *val;
>> +} record_mem_t;
>> +
>> +enum record_type
>> +{
>> + ?record_end = 0,
>> + ?record_reg,
>> + ?record_mem
>> +};
>> +
>> +/* This is the core struct of record function.
>> +
>> + ? An entity of record_t is a record of the value change of a register
>> + ? ("record_reg") or a part of memory ("record_mem"). ?And each
>> + ? instruction must has a record_t ("record_end") that points out this
>> + ? is the last record_t of this instruction.
>> +
>> + ? Each record_t is linked to "record_list" by "prev" and "next".
>> + */
>> +typedef struct record_s
>> +{
>> + ?struct record_s *prev;
>> + ?struct record_s *next;
>> + ?enum record_type type;
>> + ?union
>> + ?{
>> + ? ?/* reg */
>> + ? ?record_reg_t reg;
>> + ? ?/* mem */
>> + ? ?record_mem_t mem;
>> + ? ?/* end */
>> + ? ?int need_dasm;
>> + ?} u;
>> +} record_t;
>> +
>> +extern int record_debug;
>> +extern record_t *record_list;
>> +extern record_t *record_arch_list_head;
>> +extern record_t *record_arch_list_tail;
>> +extern struct regcache *record_regcache;
>
> Most of these things don't appear to be used anywhere else other
> than in record.c. ?Can you remove these declarations from the
> public header, and make them static in record.c?

I will move them to record.c if just record.c use them.

>
>> +
>> +extern struct target_ops record_ops;
>
> Once you get rid of TARGET_IS_PROCESS_RECORD this doesn't
> need to be public anymore.

I will.


Thanks for you help.

Hui