[RFA] Submit process record and replay third time, 3/9
teawater
teawater@gmail.com
Wed Mar 18 13:54:00 GMT 2009
This is the patches updated follow cvs head.
Thanks,
Hui
On Wed, Mar 18, 2009 at 21:11, teawater <teawater@gmail.com> wrote:
> Hi Pedro,
>
> I make a new patch according to your mail.
> Please help me review it.
>
> Thanks,
> Hui
>
> 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.
>>
>>> +
>>> + 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?
>>
>>> +
>>> +#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?
>>
>>> +
>>> +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.
>>
>>> +
>>> +/* 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.
>>
>>
>>> +
>>> +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. */
>>
>>> +
>>> +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?
>>
>>> +
>>> + 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?
>>
>>> +
>>> +/* 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.
>>
>>> +
>>> +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.
>>
>>> + 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?
>>
>>> +
>>> +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.
>>
>>> +
>>> +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.
>>
>>> +
>>> +/* 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. */
>>> +
>>> +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?
>>
>>> + {
>>> + 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;
>> }
>> }
>> }
>>
>>
>>> +
>>> +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);
>>
>>> +
>>> +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.
>>
>>> +
>>> + /* 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.
>>
>>> +#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.
>>
>>> +
>>> +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?
>>
>>> +
>>> +extern struct target_ops record_ops;
>>
>> Once you get rid of TARGET_IS_PROCESS_RECORD this doesn't
>> need to be public anymore.
>>
>>> +
>>> +extern int record_arch_list_add_reg (int num);
>>> +extern int record_arch_list_add_mem (CORE_ADDR addr, int len);
>>> +extern int record_arch_list_add_end (int need_dasm);
>>> +extern void record_message (struct gdbarch *gdbarch);
>>> +extern struct cleanup * record_gdb_operation_disable_set (void);
>>> +
>>> +#endif /* _RECORD_H_ */
>>>
>>
>> --
>> Pedro Alves
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: prec3.tar.bz2
Type: application/x-bzip2
Size: 24413 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20090318/e928c654/attachment.bz2>
More information about the Gdb-patches
mailing list