[RFA] Submit process record and replay third time, 3/9
Pedro Alves
pedro@codesourcery.com
Mon Mar 9 20:35:00 GMT 2009
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
More information about the Gdb-patches
mailing list